[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] <OpenJDK 2D-Dev> Review request for JDK-8143562: JPEG reader returns null for g
From: Ambarish Rapte <ambarish.rapte () oracle ! com>
Date: 2015-12-17 6:34:09
Message-ID: a081bcfe-84a5-41d3-8c91-bba99efbf940 () default
[Download RAW message or body]
Hi Jay,
As per the specification getRawImageType() should return a valid \
ImageTypeSpecifier for each color space, which was missing for YCbCr.
This patch fixes the issue & seems fine to me.
Thanks,
Ambarish
From: Jayathirth D V
Sent: Wednesday, December 16, 2015 2:57 PM
To: Philip Race
Cc: 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] <OpenJDK 2D-Dev> Review request for JDK-8143562: JPEG \
reader returns null for getRawImageType()
Thanks for the review Phil.
Can I get one more review for new webrev : HYPERLINK \
"http://cr.openjdk.java.net/%7Ejdv/8143562/webrev.01/"http://cr.openjdk.java.net/~jdv/8143562/webrev.01/
Regards,
Jay
From: Phil Race
Sent: Wednesday, December 16, 2015 12:00 AM
To: Jayathirth D V
Cc: HYPERLINK "mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net; brian \
Burkhalter
Subject: Re: <OpenJDK 2D-Dev> Review request for JDK-8143562: JPEG reader returns \
null for getRawImageType()
Looks OK.
-phil.
On 12/14/2015 01:37 AM, Jayathirth D V wrote:
Hi Phil,
I have made changes based on your suggestions.
I have removed specific comment which mentioned about how we handle YCbCr in JPEG and \
just added common comment(If there is no close match then a type which preserves the \
most information from the image should be returned) in ImageReader.java.
Please find the updated webrev:
HYPERLINK "http://cr.openjdk.java.net/%7Ejdv/8143562/webrev.01/"http://cr.openjdk.java.net/~jdv/8143562/webrev.01/
After technical review is done I will raise CCC request since there is spec change in \
public API. Please review.
Thanks,
Jay
From: Philip Race
Sent: Friday, December 11, 2015 2:58 AM
To: prasanta sadhukhan
Cc: Jayathirth D V; HYPERLINK "mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net
Subject: Re: <OpenJDK 2D-Dev> Review request for JDK-8143562: JPEG reader returns \
null for getRawImageType()
The quoted code comment is essentially disclaiming the spec comment.
And the spec. comment is misleading in that it strongly suggests getRawImageType
will hand you an ImageTypeSpecifier representing YCbCr when in fact it does not.
In fact all these comments and behaviours together seem to suggest that there
isn't a consistent view of what should happen here.
- one says we can return something that represents YCbCr
- another says don't add it to the returned list of types because we can't support \
this raw type after all.
- the spec says return something (anything!) no matter how dissimilar (implied
by there being at least one (a non-null return)).
Basically that seems to be all the options except supporting it.
All of these need to line up and agree.
So I think we need to update the spec. below to say something like
"Returns an ImageTypeSpecifier indicating the SampleModel and ColorModel
which most closely represents the "raw" internal format of the image.
If there is no close match then a type which preserves the most
information from the image should be returned."
-phil
On 12/8/15, 11:31 PM, prasanta sadhukhan wrote:
The fix looks good to me.
The spec says "Returns an ImageTypeSpecifier indicating the SampleModel and \
ColorModel which most closely represents the "raw" internal format of the image. For \
example, for a JPEG image the raw type might have a YCbCr color space even though the \
image would conventionally be transformed into an RGB color space prior to display." \
Also,
private Iterator<ImageTypeSpecifier> getImageTypesOnThread(int imageIndex)adds RGB \
for YcbCr raw type case JPEG.JCS_YCbCr:
832 // As there is no YCbCr ColorSpace, we can't support
833 // the raw type.
834
835 // due to 4705399, use RGB as default in order to avoid
836 // slowing down of drawing operations with result image.
837 list.add(getImageType(JPEG.JCS_RGB));
Regards
Prasanta
On 12/1/2015 3:42 PM, Jayathirth D V wrote:
Hi,
Please review following fix in JDK9:
Bug : https://bugs.openjdk.java.net/browse/JDK-8143562
Webrev : HYPERLINK "http://cr.openjdk.java.net/%7Ejdv/8143562/webrev.00/"http://cr.openjdk.java.net/~jdv/8143562/webrev.00/
Issue : We are getting null for ImageTypeSpecifier when we use getRawImageType() API \
for YCbCr Image.
Root cause : When colorspace is YCbCr, there is no condition to create \
ImageTypeSpecifier in produce() function
Solution : Since we add RGB as default ImageType for YCbCr colorspace in \
getImageTypes() API. Followed same approach
and considering it as RGB in getRawImageType().
Thanks,
Jay
[Attachment #3 (text/html)]
<html xmlns:v="urn:schemas-microsoft-com:vml" \
xmlns:o="urn:schemas-microsoft-com:office:office" \
xmlns:w="urn:schemas-microsoft-com:office:word" \
xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" \
xmlns="http://www.w3.org/TR/REC-html40"><head><meta http-equiv=Content-Type \
content="text/html; charset=us-ascii"><meta name=Generator content="Microsoft Word 12 \
(filtered medium)"><style><!-- /* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Tahoma;
panose-1:2 11 6 4 3 5 4 4 2 4;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}
@font-face
{font-family:"Times New Roman \,serif";
panose-1:0 0 0 0 0 0 0 0 0 0;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri","sans-serif";
color:black;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:#0563C1;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:#954F72;
text-decoration:underline;}
code
{mso-style-priority:99;
font-family:"Courier New";}
pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0in;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:"Courier New";
color:black;}
p.MsoAcetate, li.MsoAcetate, div.MsoAcetate
{mso-style-priority:99;
mso-style-link:"Balloon Text Char";
margin:0in;
margin-bottom:.0001pt;
font-size:8.0pt;
font-family:"Tahoma","sans-serif";
color:black;}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:Consolas;
color:black;}
span.EmailStyle20
{mso-style-type:personal;
font-family:"Calibri","sans-serif";
color:windowtext;}
span.EmailStyle21
{mso-style-type:personal;
font-family:"Calibri","sans-serif";
color:#1F497D;}
span.EmailStyle22
{mso-style-type:personal;
font-family:"Calibri","sans-serif";
color:#1F497D;}
span.EmailStyle23
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
span.BalloonTextChar
{mso-style-name:"Balloon Text Char";
mso-style-priority:99;
mso-style-link:"Balloon Text";
font-family:"Tahoma","sans-serif";
color:black;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]--></head><body bgcolor=white lang=EN-US \
link="#0563C1" vlink="#954F72"><div class=WordSection1><p class=MsoNormal><span \
style='color:#1F497D'>Hi Jay,<o:p></o:p></span></p><p class=MsoNormal><span \
style='color:#1F497D'> \
<o:p></o:p></span></p><p class=MsoNormal><span \
style='color:#1F497D'> \
As per the specification getRawImageType() should return a valid ImageTypeSpecifier \
for each color space, which was missing for YCbCr.<o:p></o:p></span></p><p \
class=MsoNormal style='text-indent:.5in'><span style='color:#1F497D'>This patch fixes \
the issue & seems fine to me.<o:p></o:p></span></p><p class=MsoNormal><span \
style='color:#1F497D'> \
<o:p></o:p></span></p><p class=MsoNormal><span \
style='color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span \
style='color:#1F497D'>Thanks,<o:p></o:p></span></p><p class=MsoNormal><span \
style='color:#1F497D'>Ambarish<o:p></o:p></span></p><p class=MsoNormal><span \
style='color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span \
style='color:#1F497D'><o:p> </o:p></span></p><div><div \
style='border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in'><p \
class=MsoNormal><b><span \
style='font-size:10.0pt;font-family:"Tahoma","sans-serif";color:windowtext'>From:</span></b><span \
style='font-size:10.0pt;font-family:"Tahoma","sans-serif";color:windowtext'> \
Jayathirth D V <br><b>Sent:</b> Wednesday, December 16, 2015 2:57 PM<br><b>To:</b> \
Philip Race<br><b>Cc:</b> 2d-dev@openjdk.java.net<br><b>Subject:</b> Re: [OpenJDK \
2D-Dev] <OpenJDK 2D-Dev> Review request for JDK-8143562: JPEG reader returns \
null for getRawImageType()<o:p></o:p></span></p></div></div><p \
class=MsoNormal><o:p> </o:p></p><p class=MsoNormal><span \
style='color:#1F497D'>Thanks for the review Phil.<o:p></o:p></span></p><p \
class=MsoNormal><span style='color:#1F497D'>Can I get one more review for new webrev \
: <a href="http://cr.openjdk.java.net/%7Ejdv/8143562/webrev.01/">http://cr.openjdk.java.net/~jdv/8143562/webrev.01/</a></span><o:p></o:p></p><p \
class=MsoNormal><span style='color:#1F497D'><o:p> </o:p></span></p><p \
class=MsoNormal><span style='color:#1F497D'>Regards,<o:p></o:p></span></p><p \
class=MsoNormal><span style='color:#1F497D'>Jay<o:p></o:p></span></p><p \
class=MsoNormal><span style='color:#1F497D'><o:p> </o:p></span></p><div><div \
style='border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in'><p \
class=MsoNormal><b><span style='color:windowtext'>From:</span></b><span \
style='color:windowtext'> Phil Race <br><b>Sent:</b> Wednesday, December 16, 2015 \
12:00 AM<br><b>To:</b> Jayathirth D V<br><b>Cc:</b> <a \
href="mailto:2d-dev@openjdk.java.net">2d-dev@openjdk.java.net</a>; brian \
Burkhalter<br><b>Subject:</b> Re: <OpenJDK 2D-Dev> Review request for \
JDK-8143562: JPEG reader returns null for \
getRawImageType()<o:p></o:p></span></p></div></div><p \
class=MsoNormal><o:p> </o:p></p><div><p class=MsoNormal>Looks \
OK.<br><br>-phil.<br><br>On 12/14/2015 01:37 AM, Jayathirth D V wrote:<span \
style='font-size:12.0pt'><o:p></o:p></span></p></div><blockquote \
style='margin-top:5.0pt;margin-bottom:5.0pt'><p class=MsoNormal><span \
style='color:#1F497D'>Hi Phil,</span><o:p></o:p></p><p class=MsoNormal><span \
style='color:#1F497D'> </span><o:p></o:p></p><p class=MsoNormal><span \
style='color:#1F497D'>I have made changes based on your \
suggestions.</span><o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'>I \
have removed specific comment which mentioned about how we handle YCbCr in JPEG and \
just added common comment(If there is no close match then a type which preserves the \
most information from the image should be returned) in \
ImageReader.java.</span><o:p></o:p></p><p class=MsoNormal><span \
style='color:#1F497D'> </span><o:p></o:p></p><p class=MsoNormal><u><span \
style='color:#1F497D'>Please find the updated webrev:</span></u><o:p></o:p></p><p \
class=MsoNormal><span style='color:#1F497D'><a \
href="http://cr.openjdk.java.net/%7Ejdv/8143562/webrev.01/">http://cr.openjdk.java.net/~jdv/8143562/webrev.01/</a></span><o:p></o:p></p><p \
class=MsoNormal><span style='color:#1F497D'> </span><o:p></o:p></p><p \
class=MsoNormal><span style='color:#1F497D'>After technical review is done I will \
raise CCC request since there is spec change in public API. Please \
review.</span><o:p></o:p></p><p class=MsoNormal><span \
style='color:#1F497D'> </span><o:p></o:p></p><p class=MsoNormal><span \
style='color:#1F497D'>Thanks,</span><o:p></o:p></p><p class=MsoNormal><span \
style='color:#1F497D'>Jay</span><o:p></o:p></p><div><div \
style='border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in'><p \
class=MsoNormal><b><span style='color:windowtext'>From:</span></b><span \
style='color:windowtext'> Philip Race <br><b>Sent:</b> Friday, December 11, 2015 2:58 \
AM<br><b>To:</b> prasanta sadhukhan<br><b>Cc:</b> Jayathirth D V; <a \
href="mailto:2d-dev@openjdk.java.net">2d-dev@openjdk.java.net</a><br><b>Subject:</b> \
Re: <OpenJDK 2D-Dev> Review request for JDK-8143562: JPEG reader returns null \
for getRawImageType()</span><o:p></o:p></p></div></div><p \
class=MsoNormal> <o:p></o:p></p><p class=MsoNormal>The quoted code comment is \
essentially disclaiming the spec comment.<br>And the spec. comment is misleading in \
that it strongly suggests getRawImageType<br>will hand you an ImageTypeSpecifier \
representing YCbCr when in fact it does not.<br><br>In fact all these comments and \
behaviours together seem to suggest that there<br>isn't a consistent view of what \
should happen here.<br>- one says we can return something that represents YCbCr<br>- \
another says don't add it to the returned list of types because we can't support this \
raw type after all.<br>- the spec says return something (anything!) no matter how \
dissimilar (implied<br> by there being at least one (a non-null \
return)).<br><br>Basically that seems to be all the options except supporting \
it.<br>All of these need to line up and agree.<br><br>So I think we need to update \
the spec. below to say something like<br>"Returns an <code><span \
style='font-size:10.0pt'>ImageTypeSpecifier</span></code> indicating the <code><span \
style='font-size:10.0pt'>SampleModel</span></code> and <code><span \
style='font-size:10.0pt'>ColorModel</span></code><br>which most closely represents \
the "raw" internal format of the image.<br>If there is no close match then \
a type which preserves the most<br>information from the image should be \
returned."<br><br>-phil<br><br>On 12/8/15, 11:31 PM, prasanta sadhukhan wrote: \
<o:p></o:p></p><blockquote style='margin-top:5.0pt;margin-bottom:5.0pt'><p \
class=MsoNormal>The fix looks good to me. <br>The spec says "Returns an \
<code><span style='font-size:10.0pt'>ImageTypeSpecifier</span></code> indicating the \
<code><span style='font-size:10.0pt'>SampleModel</span></code> and <code><span \
style='font-size:10.0pt'>ColorModel</span></code> which most closely represents the \
"raw" internal format of the image. For example, for a JPEG image the raw \
type might have a YCbCr color space even though the image would conventionally be \
transformed into an RGB color space prior to display."<br>Also, \
<o:p></o:p></p><pre>private Iterator<ImageTypeSpecifier> \
getImageTypesOnThread(int imageIndex)adds RGB for YcbCr raw \
type<o:p></o:p></pre><pre>case JPEG.JCS_YCbCr:<o:p></o:p></pre><pre> 832 \
// As there is no \
YCbCr ColorSpace, we can't support<o:p></o:p></pre><pre> \
833 // the \
raw type.<o:p></o:p></pre><pre> 834 \
<o:p></o:p></pre><pre> 835 \
// due to 4705399, use RGB as default in order to avoid<o:p></o:p></pre><pre> \
836 // \
slowing down of drawing operations with result image.<o:p></o:p></pre><pre> 837 \
list.add(getImageType(JPEG.JCS_RGB));<o:p></o:p></pre><p \
class=MsoNormal><br><br>Regards<br>Prasanta<o:p></o:p></p><div><p class=MsoNormal>On \
12/1/2015 3:42 PM, Jayathirth D V wrote:<o:p></o:p></p></div><blockquote \
style='margin-top:5.0pt;margin-bottom:5.0pt'><p class=MsoNormal>Hi,<o:p></o:p></p><p \
class=MsoNormal> <o:p></o:p></p><p class=MsoNormal><u>Please review following \
fix in JDK9:</u><o:p></o:p></p><p class=MsoNormal> <o:p></o:p></p><p \
class=MsoNormal>Bug : <a \
href="https://bugs.openjdk.java.net/browse/JDK-8143562">https://bugs.openjdk.java.net/browse/JDK-8143562</a><o:p></o:p></p><p \
class=MsoNormal> <o:p></o:p></p><p class=MsoNormal>Webrev : <a \
href="http://cr.openjdk.java.net/%7Ejdv/8143562/webrev.00/">http://cr.openjdk.java.net/~jdv/8143562/webrev.00/</a><o:p></o:p></p><p \
class=MsoNormal> <o:p></o:p></p><p class=MsoNormal>Issue : We are getting null \
for ImageTypeSpecifier when we use getRawImageType() API for YCbCr \
Image.<o:p></o:p></p><p class=MsoNormal> <o:p></o:p></p><p class=MsoNormal>Root \
cause : When colorspace is YCbCr, there is no condition to create ImageTypeSpecifier \
in produce() function<o:p></o:p></p><p class=MsoNormal> <o:p></o:p></p><p \
class=MsoNormal>Solution : Since we add RGB as default ImageType for YCbCr colorspace \
in getImageTypes() API. Followed same approach<o:p></o:p></p><p \
class=MsoNormal> \
and considering it as RGB in getRawImageType().<o:p></o:p></p><p \
class=MsoNormal> <o:p></o:p></p><p class=MsoNormal>Thanks,<o:p></o:p></p><p \
class=MsoNormal>Jay<o:p></o:p></p></blockquote><p class=MsoNormal><span \
style='font-size:12.0pt;font-family:"Times New Roman \
,serif","serif"'> </span><o:p></o:p></p></blockquote></blockquote><p \
class=MsoNormal><span style='font-size:12.0pt;font-family:"Times New \
Roman","serif"'><o:p> </o:p></span></p></div></body></html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic