Re: svn commit: r1534322 - in /commons/proper/imaging/trunk/src: changes/changes.xml main/java/org/apache/commons/imaging/formats/jpeg/JpegImageParser.java main/java/org/apache/commons/imaging/formats/jpeg/segments/ComSegment.java

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1534322 - in /commons/proper/imaging/trunk/src: changes/changes.xml main/java/org/apache/commons/imaging/formats/jpeg/JpegImageParser.java main/java/org/apache/commons/imaging/formats/jpeg/segments/ComSegment.java

sebb-2-2
On 21 October 2013 19:59,  <[hidden email]> wrote:

> Author: damjan
> Date: Mon Oct 21 18:59:04 2013
> New Revision: 1534322
>
> URL: http://svn.apache.org/r1534322
> Log:
> Encapsulate public field.
>
> Jira issue key: IMAGING-114
>
>
> Modified:
>     commons/proper/imaging/trunk/src/changes/changes.xml
>     commons/proper/imaging/trunk/src/main/java/org/apache/commons/imaging/formats/jpeg/JpegImageParser.java
>     commons/proper/imaging/trunk/src/main/java/org/apache/commons/imaging/formats/jpeg/segments/ComSegment.java
>
> Modified: commons/proper/imaging/trunk/src/changes/changes.xml
> URL: http://svn.apache.org/viewvc/commons/proper/imaging/trunk/src/changes/changes.xml?rev=1534322&r1=1534321&r2=1534322&view=diff
> ==============================================================================
> --- commons/proper/imaging/trunk/src/changes/changes.xml (original)
> +++ commons/proper/imaging/trunk/src/changes/changes.xml Mon Oct 21 18:59:04 2013
> @@ -46,8 +46,11 @@ The <action> type attribute can be add,u
>    <body>
>
>      <release version="1.0" date="TBA" description="TBA">
> +      <action issue="IMAGING-114" dev="damjan" type="fix">
> +        ComSegment.comment is a public final byte array.
> +      </action>
>        <action issue="IMAGING-109" dev="damjan" type="fix">
> -        Several files contain non-UTF8 characters
> +        Several files contain non-UTF8 characters.
>        </action>
>        <action issue="IMAGING-113" dev="damjan" type="fix">
>          Cannot read multipage tiff.
>
> Modified: commons/proper/imaging/trunk/src/main/java/org/apache/commons/imaging/formats/jpeg/JpegImageParser.java
> URL: http://svn.apache.org/viewvc/commons/proper/imaging/trunk/src/main/java/org/apache/commons/imaging/formats/jpeg/JpegImageParser.java?rev=1534322&r1=1534321&r2=1534322&view=diff
> ==============================================================================
> --- commons/proper/imaging/trunk/src/main/java/org/apache/commons/imaging/formats/jpeg/JpegImageParser.java (original)
> +++ commons/proper/imaging/trunk/src/main/java/org/apache/commons/imaging/formats/jpeg/JpegImageParser.java Mon Oct 21 18:59:04 2013
> @@ -773,7 +773,7 @@ public class JpegImageParser extends Ima
>              final ComSegment comSegment = (ComSegment) commentSegments.get(i);
>              String comment = "";
>              try {
> -                comment = new String(comSegment.comment, "UTF-8");
> +                comment = new String(comSegment.getComment(), "UTF-8");
>              } catch (final UnsupportedEncodingException cannotHappen) {
>              }
>              Comments.add(comment);
>
> Modified: commons/proper/imaging/trunk/src/main/java/org/apache/commons/imaging/formats/jpeg/segments/ComSegment.java
> URL: http://svn.apache.org/viewvc/commons/proper/imaging/trunk/src/main/java/org/apache/commons/imaging/formats/jpeg/segments/ComSegment.java?rev=1534322&r1=1534321&r2=1534322&view=diff
> ==============================================================================
> --- commons/proper/imaging/trunk/src/main/java/org/apache/commons/imaging/formats/jpeg/segments/ComSegment.java (original)
> +++ commons/proper/imaging/trunk/src/main/java/org/apache/commons/imaging/formats/jpeg/segments/ComSegment.java Mon Oct 21 18:59:04 2013
> @@ -22,7 +22,7 @@ import java.io.InputStream;
>  import java.io.UnsupportedEncodingException;
>
>  public class ComSegment extends Segment {
> -    public final byte[] comment;
> +    private final byte[] comment;
>
>      public ComSegment(final int marker, final byte segmentData[]) throws IOException {
>          this(marker, segmentData.length, new ByteArrayInputStream(segmentData));
> @@ -43,6 +43,10 @@ public class ComSegment extends Segment
>              System.out.println("");
>          }
>      }
> +
> +    public byte[] getComment() {
> +        return comment;

This is better, but the array can still be modified externally.

Might make more sense to convert the array to a String and return that
instead - Strings are immutable, but arrays are not.
Otherwise perhaps return a copy.

> +    }
>
>      @Override
>      public String getDescription() {
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1534322 - in /commons/proper/imaging/trunk/src: changes/changes.xml main/java/org/apache/commons/imaging/formats/jpeg/JpegImageParser.java main/java/org/apache/commons/imaging/formats/jpeg/segments/ComSegment.java

Damjan Jovanovic-2
On Tue, Oct 22, 2013 at 3:02 AM, sebb <[hidden email]> wrote:

>> +
>> +    public byte[] getComment() {
>> +        return comment;
>
> This is better, but the array can still be modified externally.
>
> Might make more sense to convert the array to a String and return that
> instead - Strings are immutable, but arrays are not.
> Otherwise perhaps return a copy.
>

The text encoding for the JPEG COM segment is unspecified, and can be
anything in practice, including binary data instead of text, so we
have to return a byte[].

I am concerned about the performance implications of copying data
around so much, but guess it's not much data.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1534322 - in /commons/proper/imaging/trunk/src: changes/changes.xml main/java/org/apache/commons/imaging/formats/jpeg/JpegImageParser.java main/java/org/apache/commons/imaging/formats/jpeg/segments/ComSegment.java

Bernd Eckenfels
You could wrap it in a Java NIO read only ByteBuffer, but for small data this might not safe much as it is an additional object instance and You need to keep the backing Array unmodified.

Greetings
Bernd

> Am 22.10.2013 um 06:49 schrieb Damjan Jovanovic <[hidden email]>:
>
> On Tue, Oct 22, 2013 at 3:02 AM, sebb <[hidden email]> wrote:
>>> +
>>> +    public byte[] getComment() {
>>> +        return comment;
>>
>> This is better, but the array can still be modified externally.
>>
>> Might make more sense to convert the array to a String and return that
>> instead - Strings are immutable, but arrays are not.
>> Otherwise perhaps return a copy.
>
> The text encoding for the JPEG COM segment is unspecified, and can be
> anything in practice, including binary data instead of text, so we
> have to return a byte[].
>
> I am concerned about the performance implications of copying data
> around so much, but guess it's not much data.
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1534322 - in /commons/proper/imaging/trunk/src: changes/changes.xml main/java/org/apache/commons/imaging/formats/jpeg/JpegImageParser.java main/java/org/apache/commons/imaging/formats/jpeg/segments/ComSegment.java

sebb-2-2
In reply to this post by Damjan Jovanovic-2
On 22 October 2013 05:49, Damjan Jovanovic <[hidden email]> wrote:

> On Tue, Oct 22, 2013 at 3:02 AM, sebb <[hidden email]> wrote:
>>> +
>>> +    public byte[] getComment() {
>>> +        return comment;
>>
>> This is better, but the array can still be modified externally.
>>
>> Might make more sense to convert the array to a String and return that
>> instead - Strings are immutable, but arrays are not.
>> Otherwise perhaps return a copy.
>>
>
> The text encoding for the JPEG COM segment is unspecified, and can be
> anything in practice, including binary data instead of text, so we
> have to return a byte[].

I suggested returning a String because the only usage I found
immediately converted the bytes into a String.

> I am concerned about the performance implications of copying data
> around so much, but guess it's not much data.

Sometimes it may be necessary to pass around arrays, in which case the
mutability needs to be clearly documented.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]