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
