[OpenJDK 2D-Dev] Review Request for JDK-8160943 : [PIT] new failure of closed/javax/imageio/ReadAllThumbnailsTest.java
Jim Graham
james.graham at oracle.com
Fri Jul 22 20:20:47 UTC 2016
I found it. The bug is still there. ImageIO has been writing out bad
length fields for APP0/JFXX thumbnails for years now.
When we write a JFXX APP0 marker in JFIFMarkerSegment.java, we write out
the length as:
LENGTH_SIZE + DATA_SIZE + thumb.getLength()
LENGTH_SIZE is 2 which accounts for the APP0 size field so we are good
there.
DATA_SIZE is 6 which accounts for the "JFXX\0" and the 0x13 marker that
defines this as an RGB thumbnail (both of which are written out by the
code that computed the size above).
thumb.getLength() is (w * h * 3) which accounts for the pixels in the
thumbnail.
But, nobody accounts for the 2 bytes for the w,h of the thumbnail, so
the length field is written out 2 bytes short of the correct total. The
"thumb" object is responsible for writing out that w,h so it should
probably include them in its return value for its length I imagin, but
the length only accounts for the RGB triples in the actual pixels.
There are other Thumbnail classes which I believe also omit the 2 bytes
for the w,h from their getLength() field (the w,h are written by
super.write() in all cases so they are computing the bytes that they
themselves write and ignoring what their superclass writes).
There is also some JFIF thumbnail writing code in the same file which I
think does its math correctly, but we should write a test and see...
...jim
On 07/21/2016 02:46 PM, Jayathirth D V wrote:
> Hi Jim,
>
> I just ran the test case attached in https://bugs.openjdk.java.net/browse/JDK-4958271 .
> It is actually generating an image and using it to do reader.readAll().
>
> Thanks,
> Jay
> -----Original Message-----
> From: Jim Graham
> Sent: Thursday, July 21, 2016 2:22 PM
> To: Jayathirth D V; Philip Race
> Cc: 2d-dev
> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-8160943 : [PIT] new failure of closed/javax/imageio/ReadAllThumbnailsTest.java
>
> Hi Jay,
>
> How did you generate that image with thumbnail? With ImageIO?
>
> ...jim
>
> On 07/19/2016 12:18 PM, Jayathirth D V wrote:
>> Hi Phil,
>>
>> I generated Jpeg image with thumbnail as given in test case attached for https://bugs.openjdk.java.net/browse/JDK-4958271 with JDK 9.
>> The image has only two APP0 markers and after the second marker it has "00 FF" which we can consider as "X FF" as per https://www.w3.org/Graphics/JPEG/itu-t81.pdf. But it is just continuation of pattern "00 00 FF" which we see around the end of second APP0 marker.
>>
>> For this new image I don't see whether we are writing improperly in JDK9(If we consider "X FF" bits between markers).
>> So we can just apply http://cr.openjdk.java.net/~jdv/8160943/webrev.00/ and open up the test case(after verifying whether it is the only test case which uses this particular image) with new image that I have created.
>>
>> Thanks,
>> Jay
>>
>> -----Original Message-----
>> From: Philip Race
>> Sent: Wednesday, July 13, 2016 10:06 AM
>> To: Jim Graham
>> Cc: Jayathirth D V; 2d-dev
>> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-8160943 : [PIT]
>> new failure of closed/javax/imageio/ReadAllThumbnailsTest.java
>>
>> Whilst looking at the reason this test was originally created
>> https://bugs.openjdk.java.net/browse/JDK-4958271
>> it started to look likely that the image was (as I sort of suspected) was generated by IIO itself.
>>
>> I think there a couple of things we can do here
>> 1) Open up the test (and image - although if moving it make sure no other tests in that location need the image).
>> 2) Investigate to see if the bug such that the "bad padding" is written out is still reproducible with current IIO ... or if it was fixed sometime between when that image was created and now.
>>
>> -phil.
>>
>> On 7/12/16, 7:48 PM, Jim Graham wrote:
>>> I think this is fine, but I noticed that some of the recently added
>>> comments have some grammar issues. It might be nice to change the
>>> following:
>>>
>>> 565 // markers which do not contain length data
>>> (doesn't => do not)
>>>
>>> 576 // markers which contain length data
>>> (contains => contain)
>>>
>>> I don't need to review those changes...
>>>
>>> ...jim
>>>
>>> On 7/12/16 9:15 AM, Jayathirth D V wrote:
>>>> Hi,
>>>>
>>>>
>>>>
>>>> Thanks for your input Phil.
>>>>
>>>> I have made changes just to parse "FF FF"(Like "FF 00" or any marker
>>>> without length)and not consider it as an invalid marker in
>>>> skipImage() of JPEGImageReader.java.
>>>>
>>>> Also I have removed closed/javax/imageio/ReadAllThumbnailsTest.java
>>>> from ProblemList.txt as part of fix.
>>>>
>>>>
>>>>
>>>> Bug : https://bugs.openjdk.java.net/browse/JDK-8160943
>>>>
>>>>
>>>>
>>>> Please find webrev for review for JDK9:
>>>>
>>>> http://cr.openjdk.java.net/~jdv/8160943/webrev.00/
>>>>
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Jay
>>>>
>>>>
>>>>
>>>> *From:*Phil Race
>>>> *Sent:* Saturday, July 09, 2016 12:37 AM
>>>> *To:* Jayathirth D V
>>>> *Cc:* Jim Graham; 2d-dev
>>>> *Subject:* Re: REG : JDK-8160943 : [PIT] new failure of
>>>> closed/javax/imageio/ReadAllThumbnailsTest.java
>>>>
>>>>
>>>>
>>>> On 07/08/2016 04:08 AM, Jayathirth D V wrote:
>>>>
>>>> Hi,
>>>>
>>>>
>>>>
>>>> In JDK-8152672
>>>> <https://bugs.openjdk.java.net/browse/JDK-8152672>
>>>> we modified skipImage() in JpegImageReader.java
>>>> and added tighter checks for parsing Jpeg data.
>>>>
>>>>
>>>>
>>>> We have to find any marker,0 or EOF after we find "FF" while
>>>> parsing JPEG data.
>>>>
>>>> But in JDK-8160943
>>>> <https://bugs.openjdk.java.net/browse/JDK-8160943> we have gap
>>>> between APP0 marker and DQT(FF DB)
>>>> marker which contains data "00 FF".
>>>>
>>>>
>>>>
>>>> APP0_End -> 00 FF -> FF DB(DQT)
>>>>
>>>>
>>>>
>>>> So after we skip APP0 marker we find two bytes of data which is
>>>> "FF FF". In the present code we consider this as
>>>> invalid marker.
>>>>
>>>>
>>>> See https://www.w3.org/Graphics/JPEG/itu-t81.pdf
>>>>
>>>> B.1.1.2 Markers
>>>> Markers serve to identify the various structural parts of the
>>>> compressed data formats.
>>>> Most markers start marker segments containing a related group of
>>>> parameters; some markers stand alone. All markers are assigned
>>>> two-byte codes: an X'FF' byte followed by a byte which is not equal
>>>> to 0 or X'FF' (see Table B.1).
>>>> Any marker may optionally be preceded by any number of fill bytes,
>>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>> which are bytes assigned code X'FF
>>>> ^^^^^^^^^^^^^^^^^^^^^^
>>>>
>>>> -phil.
>>>>
>>>>
>>>> Because of this JDK-8160943
>>>> <https://bugs.openjdk.java.net/browse/JDK-8160943> is failing.
>>>>
>>>>
>>>>
>>>> Is the length of APP0 marker not valid in the image or we should
>>>> not consider "FF FF" as invalid maker?
>>>>
>>>> Please let me know your input.
>>>>
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Jay
>>>>
>>>>
>>>>
More information about the 2d-dev
mailing list