Bug 47196 - Implement UNPACK_COLORSPACE_CONVERSION_WEBGL
Summary: Implement UNPACK_COLORSPACE_CONVERSION_WEBGL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Zhenyao Mo
URL:
Keywords:
: 35050 44568 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-10-05 10:38 PDT by Kenneth Russell
Modified: 2010-11-18 15:56 PST (History)
5 users (show)

See Also:


Attachments
patch (38.50 KB, patch)
2010-11-10 15:11 PST, Zhenyao Mo
zmo: commit-queue-
Details | Formatted Diff | Diff
revised patch : resync texture-transparant-... test. (38.74 KB, patch)
2010-11-11 14:26 PST, Zhenyao Mo
kbr: review+
zmo: commit-queue-
Details | Formatted Diff | Diff
patch: hook up UNPACK_COLORSPACE_CONVERSION setting to texture uploading. (29.85 KB, patch)
2010-11-11 16:30 PST, Zhenyao Mo
kbr: review-
zmo: commit-queue-
Details | Formatted Diff | Diff
revised patch: fixed the parameter names and also in the comments. (30.37 KB, patch)
2010-11-15 10:57 PST, Zhenyao Mo
kbr: review+
zmo: commit-queue-
Details | Formatted Diff | Diff
patch: hooking up COLORSPACE_CONVERSION setting with ICC profile behavior (37.30 KB, patch)
2010-11-17 17:41 PST, Zhenyao Mo
abarth: review+
zmo: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2010-10-05 10:38:21 PDT
In a recent WebGL face-to-face meeting it was resolved to provide explicit control over any colorspace conversion the browser may apply during image uploading. This is likely the cause of some of the gamma ramp tests in gl-teximage.html failing. Per the spec, the new pixelStorei parameter UNPACK_COLORSPACE_CONVERSION_WEBGL needs to be implemented, with the default value of BROWSER_DEFAULT_WEBGL.
Comment 1 Kenneth Russell 2010-11-09 11:32:06 PST
*** Bug 44568 has been marked as a duplicate of this bug. ***
Comment 2 Kenneth Russell 2010-11-09 11:34:18 PST
*** Bug 35050 has been marked as a duplicate of this bug. ***
Comment 3 Zhenyao Mo 2010-11-10 15:11:34 PST
Created attachment 73547 [details]
patch

This is the first patch for this bug.  We add constants and methods, but haven't hooked it up with texture uploading yet.  Also, in order to check in constants.html, we need to remove out-dated constants, and update affected tests correspondingly.

All the tests are in sync with khronos.
Comment 4 Zhenyao Mo 2010-11-11 14:26:26 PST
Created attachment 73661 [details]
revised patch : resync texture-transparant-... test.
Comment 5 Kenneth Russell 2010-11-11 14:44:38 PST
Comment on attachment 73661 [details]
revised patch : resync texture-transparant-... test.

Looks good to me. Note to any other reviewers: some of the test changes which might seem unrelated are because of the removal of the TRUE and FALSE constants and attendant resyncing of those tests with the Khronos repo.
Comment 6 Zhenyao Mo 2010-11-11 15:04:34 PST
Committed r71855: <http://trac.webkit.org/changeset/71855>
Comment 7 Zhenyao Mo 2010-11-11 15:31:46 PST
Accidentally closed this bug when committing the first patch: it's not fixed yet.
Comment 8 Zhenyao Mo 2010-11-11 16:30:05 PST
Created attachment 73679 [details]
patch: hook up UNPACK_COLORSPACE_CONVERSION setting to texture uploading.
Comment 9 Zhenyao Mo 2010-11-11 16:50:59 PST
This second patch fixes the gamma correction behavior.  Once this is reviewed and landed, I'll upload a third patch to control ICC profiles.  Then this bug should be fixed.
Comment 10 Kenneth Russell 2010-11-12 18:39:30 PST
Comment on attachment 73679 [details]
patch: hook up UNPACK_COLORSPACE_CONVERSION setting to texture uploading.

View in context: https://bugs.webkit.org/attachment.cgi?id=73679&action=review

r- for naming inconsistency throughout the code. I think the argument should be more properly named ignoreGammaAndColorProfile.

> WebCore/platform/graphics/qt/ImageDecoderQt.cpp:40
> +ImageDecoder* ImageDecoder::create(const SharedBuffer& data, bool premultiplyAlpha, bool ignoreGamma)

Everywhere else throughout the code this is called "ignoreColorProfile". The code should be consistent.

> WebCore/platform/graphics/qt/ImageDecoderQt.cpp:46
> +    return new ImageDecoderQt(premultiplyAlpha, ignoreGamma);

Here too.

> WebCore/platform/graphics/qt/ImageDecoderQt.cpp:50
> +ImageDecoderQt::ImageDecoderQt(bool premultiplyAlpha, bool ignoreGamma)
> +    : ImageDecoder(premultiplyAlpha, ignoreGamma)

Here too.

> WebCore/platform/graphics/qt/ImageDecoderQt.h:44
> +    ImageDecoderQt(bool premultiplyAlpha, bool ignoreGamma);

Here too.

> WebKit/chromium/ChangeLog:9
> +        (WebKit::WebImageDecoder::init): Add ignoreColorSpace parameter.

ignoreColorSpace doesn't match the naming of the argument throughout the code.
Comment 11 Adam Barth 2010-11-12 18:56:56 PST
Comment on attachment 73679 [details]
patch: hook up UNPACK_COLORSPACE_CONVERSION setting to texture uploading.

View in context: https://bugs.webkit.org/attachment.cgi?id=73679&action=review

What about the ICC information in JPEG?  What about the CoreGraphics image decoders?  It's lame that you push this all the way down into the decoder instead of doing it in image source.  If the image doesn't provide an ICC profile, we assume it has a Device color profile (on Mac).  Is that what you want?  Do you want sRGB?

> WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:299
> -    if (png_get_gAMA(png, info, &gamma)) {
> +    if (!m_ignoreColorProfile && png_get_gAMA(png, info, &gamma)) {

This is only the gamma correction.  What about the ICC correction?

> WebKit/chromium/src/WebImageDecoder.cpp:59
> -        m_private = new BMPImageDecoder(true);
> +        m_private = new BMPImageDecoder(true, false);

Isn't false implicit?
Comment 12 Zhenyao Mo 2010-11-15 10:00:07 PST
(In reply to comment #11)
> (From update of attachment 73679 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73679&action=review
> 
> What about the ICC information in JPEG?  What about the CoreGraphics image decoders?  It's lame that you push this all the way down into the decoder instead of doing it in image source.  If the image doesn't provide an ICC profile, we assume it has a Device color profile (on Mac).  Is that what you want?  Do you want sRGB?

If we don't push it down to the decoder, gamma correction will be applied and we have to unapply, which won't be lossless.   For certain WebGL settings, lossless is required.  If there is a way that I fails to see, please enlighten me.

Yes, sRGB seems the correct profile for raw unaltered RGBA data.

As for CoreGraphics image decoder, they don't apply gamma correction and ICC profiles.  Maybe there is a setting to control that.  I need to do more digging.  It will be a separate patch.

> 
> > WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:299
> > -    if (png_get_gAMA(png, info, &gamma)) {
> > +    if (!m_ignoreColorProfile && png_get_gAMA(png, info, &gamma)) {
> 
> This is only the gamma correction.  What about the ICC correction?

I am going to fix the ICC profile behavior in another patch.

> 
> > WebKit/chromium/src/WebImageDecoder.cpp:59
> > -        m_private = new BMPImageDecoder(true);
> > +        m_private = new BMPImageDecoder(true, false);
> 
> Isn't false implicit?

No, I only use default values for ImageSource.  All others we need to explicitly express the desired settings.
Comment 13 Zhenyao Mo 2010-11-15 10:57:36 PST
Created attachment 73913 [details]
revised patch: fixed the parameter names and also in the comments.

Use ignoreGammaAndColorProfile all over.
Comment 14 Kenneth Russell 2010-11-15 14:28:42 PST
Comment on attachment 73913 [details]
revised patch: fixed the parameter names and also in the comments.

This looks good to me as a step forward but I'll wait to r+ it until Adam has a chance to comment.
Comment 15 Kenneth Russell 2010-11-16 12:08:44 PST
I think Adam is busy for the next couple of days so I'm going to r+ this now and we can fix up any concerns in a follow-up patch.
Comment 16 Kenneth Russell 2010-11-16 12:08:58 PST
Comment on attachment 73913 [details]
revised patch: fixed the parameter names and also in the comments.

R=me
Comment 17 Zhenyao Mo 2010-11-16 13:24:05 PST
Committed r72130: <http://trac.webkit.org/changeset/72130>
Comment 18 Zhenyao Mo 2010-11-16 13:25:00 PST
Forget about --no-close again.  Re-open this bug for ICC profile issue.
Comment 19 Adam Barth 2010-11-16 13:43:54 PST
(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 73679 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=73679&action=review
> > 
> > What about the ICC information in JPEG?  What about the CoreGraphics image decoders?  It's lame that you push this all the way down into the decoder instead of doing it in image source.  If the image doesn't provide an ICC profile, we assume it has a Device color profile (on Mac).  Is that what you want?  Do you want sRGB?
> 
> If we don't push it down to the decoder, gamma correction will be applied and we have to unapply, which won't be lossless.   For certain WebGL settings, lossless is required.  If there is a way that I fails to see, please enlighten me.

How can you have losses JPEG?  That doesn't make any sense to me.

> Yes, sRGB seems the correct profile for raw unaltered RGBA data.

How can something be both "unaltered" and sRGB?  Doesn't sRGB imply alteration?  Maybe I don't understand what you're trying to do.

> As for CoreGraphics image decoder, they don't apply gamma correction and ICC profiles.  Maybe there is a setting to control that.  I need to do more digging.  It will be a separate patch.

Huh?  I'm pretty CoreGraphics uses ICC profiles.  Do you have tests?  Are you tests failing in Safari?

> > > WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:299
> > > -    if (png_get_gAMA(png, info, &gamma)) {
> > > +    if (!m_ignoreColorProfile && png_get_gAMA(png, info, &gamma)) {
> > 
> > This is only the gamma correction.  What about the ICC correction?
> 
> I am going to fix the ICC profile behavior in another patch.

Ok.  In the meantime, the code super confusing because m_ignoreColorProfile actually affects the gamma instead of the color profile.  Perhaps we should rename the variable as we go so it actually reflects what it does?
Comment 20 Adam Barth 2010-11-16 13:48:56 PST
Comment on attachment 73913 [details]
revised patch: fixed the parameter names and also in the comments.

View in context: https://bugs.webkit.org/attachment.cgi?id=73913&action=review

> WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:299
> -    if (png_get_gAMA(png, info, &gamma)) {
> +    if (!m_ignoreGammaAndColorProfile && png_get_gAMA(png, info, &gamma)) {

This line of code really seems wrong.  m_ignoreGammaAndColorProfile doesn't ignore color profiles.

> WebKit/chromium/src/WebImageDecoder.cpp:59
> -        m_private = new BMPImageDecoder(true);
> +        m_private = new BMPImageDecoder(true, false);

Please use an enum instead of a bool.  It's really hard to understand what this line of code does.  If you'd like to improve the premultiply alpha parameter to use an enum too, that would be great.  (Feel free to do both in a follow-up patch.)

> LayoutTests/fast/canvas/webgl/gl-teximage-expected.txt:42
> +Check that gamma settings don't effect 8bit pngs
> +PASS getError was expected value: NO_ERROR : Should be no errors from setup.

Can we test JPEGs as well as PNG?  I don't understand how this test passes in Safari.  We should also have tests with color profiles to document that this patch is incomplete.  It's fine to check in expected.txt files that say FAIL.
Comment 21 Zhenyao Mo 2010-11-16 13:57:30 PST
(In reply to comment #19)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (From update of attachment 73679 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=73679&action=review
> > > 
> > > What about the ICC information in JPEG?  What about the CoreGraphics image decoders?  It's lame that you push this all the way down into the decoder instead of doing it in image source.  If the image doesn't provide an ICC profile, we assume it has a Device color profile (on Mac).  Is that what you want?  Do you want sRGB?
> > 
> > If we don't push it down to the decoder, gamma correction will be applied and we have to unapply, which won't be lossless.   For certain WebGL settings, lossless is required.  If there is a way that I fails to see, please enlighten me.
> 
> How can you have losses JPEG?  That doesn't make any sense to me.

Lossless texture is mostly PNG.

> 
> > Yes, sRGB seems the correct profile for raw unaltered RGBA data.
> 
> How can something be both "unaltered" and sRGB?  Doesn't sRGB imply alteration?  Maybe I don't understand what you're trying to do.

We assume the original PNG is in RGB color space.  Otherwise lossless can't be possible.

> 
> > As for CoreGraphics image decoder, they don't apply gamma correction and ICC profiles.  Maybe there is a setting to control that.  I need to do more digging.  It will be a separate patch.
> 
> Huh?  I'm pretty CoreGraphics uses ICC profiles.  Do you have tests?  Are you tests failing in Safari?

We tested Gamma correction and it's not applied in CG decoder (at least not in the way we use it).  We haven't dealt with ICC profiles yet.

> 
> > > > WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:299
> > > > -    if (png_get_gAMA(png, info, &gamma)) {
> > > > +    if (!m_ignoreColorProfile && png_get_gAMA(png, info, &gamma)) {
> > > 
> > > This is only the gamma correction.  What about the ICC correction?
> > 
> > I am going to fix the ICC profile behavior in another patch.
> 
> Ok.  In the meantime, the code super confusing because m_ignoreColorProfile actually affects the gamma instead of the color profile.  Perhaps we should rename the variable as we go so it actually reflects what it does?

I fixed the names already.  Used ignoreGammaAndColorProfile instead.
Comment 22 Zhenyao Mo 2010-11-16 14:01:30 PST
(In reply to comment #20)
> (From update of attachment 73913 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73913&action=review
> 
> > WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:299
> > -    if (png_get_gAMA(png, info, &gamma)) {
> > +    if (!m_ignoreGammaAndColorProfile && png_get_gAMA(png, info, &gamma)) {
> 
> This line of code really seems wrong.  m_ignoreGammaAndColorProfile doesn't ignore color profiles.

It's not wrong.  Only that we also need to hook m_ignoreGammaAndColorProfile with ICC profiles too, as this parameter controls both.

> 
> > WebKit/chromium/src/WebImageDecoder.cpp:59
> > -        m_private = new BMPImageDecoder(true);
> > +        m_private = new BMPImageDecoder(true, false);
> 
> Please use an enum instead of a bool.  It's really hard to understand what this line of code does.  If you'd like to improve the premultiply alpha parameter to use an enum too, that would be great.  (Feel free to do both in a follow-up patch.)

Will do.

> 
> > LayoutTests/fast/canvas/webgl/gl-teximage-expected.txt:42
> > +Check that gamma settings don't effect 8bit pngs
> > +PASS getError was expected value: NO_ERROR : Should be no errors from setup.
> 
> Can we test JPEGs as well as PNG?  I don't understand how this test passes in Safari.  We should also have tests with color profiles to document that this patch is incomplete.  It's fine to check in expected.txt files that say FAIL.

Honestly we really don't care about JPEGs in WebGL, as if a programmer wants lossless data, he should use PNG in RGB colorspace only.
Comment 23 Adam Barth 2010-11-17 10:45:26 PST
> > How can you have losses JPEG?  That doesn't make any sense to me.
> 
> Lossless texture is mostly PNG.

What does mostly mean?  This patch seems incomplete because it doesn't handle JPEGs.  That seems like a bug.

> > > Yes, sRGB seems the correct profile for raw unaltered RGBA data.
> > 
> > How can something be both "unaltered" and sRGB?  Doesn't sRGB imply alteration?  Maybe I don't understand what you're trying to do.
> 
> We assume the original PNG is in RGB color space.  Otherwise lossless can't be possible.

Do you mean sRGB instead of RGB?  I don't know what lossless means, so I can't comment on when it is or is not possible.

> > > As for CoreGraphics image decoder, they don't apply gamma correction and ICC profiles.  Maybe there is a setting to control that.  I need to do more digging.  It will be a separate patch.
> > 
> > Huh?  I'm pretty CoreGraphics uses ICC profiles.  Do you have tests?  Are you tests failing in Safari?
> 
> We tested Gamma correction and it's not applied in CG decoder (at least not in the way we use it).  We haven't dealt with ICC profiles yet.

It would good to document all this information with test cases.

> > > I am going to fix the ICC profile behavior in another patch.
> > 
> > Ok.  In the meantime, the code super confusing because m_ignoreColorProfile actually affects the gamma instead of the color profile.  Perhaps we should rename the variable as we go so it actually reflects what it does?
> 
> I fixed the names already.  Used ignoreGammaAndColorProfile instead.

> > > WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:299
> > > -    if (png_get_gAMA(png, info, &gamma)) {
> > > +    if (!m_ignoreGammaAndColorProfile && png_get_gAMA(png, info, &gamma)) {
> > 
> > This line of code really seems wrong.  m_ignoreGammaAndColorProfile doesn't ignore color profiles.
> 
> It's not wrong.  Only that we also need to hook m_ignoreGammaAndColorProfile with ICC profiles too, as this parameter controls both.

What I'm saying is that this patch, on its face, is incorrect.  In the future, you plan to make it correct by finishing the job, but between now and then the code will be inaccurate.

> > > WebKit/chromium/src/WebImageDecoder.cpp:59
> > > -        m_private = new BMPImageDecoder(true);
> > > +        m_private = new BMPImageDecoder(true, false);
> > 
> > Please use an enum instead of a bool.  It's really hard to understand what this line of code does.  If you'd like to improve the premultiply alpha parameter to use an enum too, that would be great.  (Feel free to do both in a follow-up patch.)
> 
> Will do.

Thanks.

> > > LayoutTests/fast/canvas/webgl/gl-teximage-expected.txt:42
> > > +Check that gamma settings don't effect 8bit pngs
> > > +PASS getError was expected value: NO_ERROR : Should be no errors from setup.
> > 
> > Can we test JPEGs as well as PNG?  I don't understand how this test passes in Safari.  We should also have tests with color profiles to document that this patch is incomplete.  It's fine to check in expected.txt files that say FAIL.
> 
> Honestly we really don't care about JPEGs in WebGL, as if a programmer wants lossless data, he should use PNG in RGB colorspace only.

Should these APIs throw when given a JPEG?  In your patch, it seems like we just have the wrong behavior.  Again, do you mean sRGB instead of RGB?

This patch seems incorrect (or at least incomplete) in multiple dimensions, and I still don't understand what "lossless" means.

The whole thing just seems like a misfeature.  If you don't want color management, don't include a color profile in your image.  The results will be unpredictable, of course, since the result will depend on what color space we choose to interpret the image in.  That varies at least by platform and likely also by how the user has configured their system and what hardware they're using.

Moving forward, I think we should do the following:

1) Add tests for all the cases discussed above.
2) Change this patch to use accurate variable names.
3) Follow up and finish implementing the feature for all image types and for ICC as well as gamma.

This patch incurs technical debt for the project, which is generally something we'd like to avoid.  I trust you to pay off that debt in short order, but understand the costs you're imposing on the project because of your schedule pressure.
Comment 24 Zhenyao Mo 2010-11-17 11:02:52 PST
(In reply to comment #23)
> > > How can you have losses JPEG?  That doesn't make any sense to me.
> > 
> > Lossless texture is mostly PNG.
> 
> What does mostly mean?  This patch seems incomplete because it doesn't handle JPEGs.  That seems like a bug.
> 
> > > > Yes, sRGB seems the correct profile for raw unaltered RGBA data.
> > > 
> > > How can something be both "unaltered" and sRGB?  Doesn't sRGB imply alteration?  Maybe I don't understand what you're trying to do.
> > 
> > We assume the original PNG is in RGB color space.  Otherwise lossless can't be possible.
> 
> Do you mean sRGB instead of RGB?  I don't know what lossless means, so I can't comment on when it is or is not possible.
> 
> > > > As for CoreGraphics image decoder, they don't apply gamma correction and ICC profiles.  Maybe there is a setting to control that.  I need to do more digging.  It will be a separate patch.
> > > 
> > > Huh?  I'm pretty CoreGraphics uses ICC profiles.  Do you have tests?  Are you tests failing in Safari?
> > 
> > We tested Gamma correction and it's not applied in CG decoder (at least not in the way we use it).  We haven't dealt with ICC profiles yet.
> 
> It would good to document all this information with test cases.
> 
> > > > I am going to fix the ICC profile behavior in another patch.
> > > 
> > > Ok.  In the meantime, the code super confusing because m_ignoreColorProfile actually affects the gamma instead of the color profile.  Perhaps we should rename the variable as we go so it actually reflects what it does?
> > 
> > I fixed the names already.  Used ignoreGammaAndColorProfile instead.
> 
> > > > WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:299
> > > > -    if (png_get_gAMA(png, info, &gamma)) {
> > > > +    if (!m_ignoreGammaAndColorProfile && png_get_gAMA(png, info, &gamma)) {
> > > 
> > > This line of code really seems wrong.  m_ignoreGammaAndColorProfile doesn't ignore color profiles.
> > 
> > It's not wrong.  Only that we also need to hook m_ignoreGammaAndColorProfile with ICC profiles too, as this parameter controls both.
> 
> What I'm saying is that this patch, on its face, is incorrect.  In the future, you plan to make it correct by finishing the job, but between now and then the code will be inaccurate.
> 
> > > > WebKit/chromium/src/WebImageDecoder.cpp:59
> > > > -        m_private = new BMPImageDecoder(true);
> > > > +        m_private = new BMPImageDecoder(true, false);
> > > 
> > > Please use an enum instead of a bool.  It's really hard to understand what this line of code does.  If you'd like to improve the premultiply alpha parameter to use an enum too, that would be great.  (Feel free to do both in a follow-up patch.)
> > 
> > Will do.
> 
> Thanks.
> 
> > > > LayoutTests/fast/canvas/webgl/gl-teximage-expected.txt:42
> > > > +Check that gamma settings don't effect 8bit pngs
> > > > +PASS getError was expected value: NO_ERROR : Should be no errors from setup.
> > > 
> > > Can we test JPEGs as well as PNG?  I don't understand how this test passes in Safari.  We should also have tests with color profiles to document that this patch is incomplete.  It's fine to check in expected.txt files that say FAIL.
> > 
> > Honestly we really don't care about JPEGs in WebGL, as if a programmer wants lossless data, he should use PNG in RGB colorspace only.
> 
> Should these APIs throw when given a JPEG?  In your patch, it seems like we just have the wrong behavior.  Again, do you mean sRGB instead of RGB?
> 
> This patch seems incorrect (or at least incomplete) in multiple dimensions, and I still don't understand what "lossless" means.
> 
> The whole thing just seems like a misfeature.  If you don't want color management, don't include a color profile in your image.  The results will be unpredictable, of course, since the result will depend on what color space we choose to interpret the image in.  That varies at least by platform and likely also by how the user has configured their system and what hardware they're using.
> 
> Moving forward, I think we should do the following:
> 
> 1) Add tests for all the cases discussed above.
> 2) Change this patch to use accurate variable names.
> 3) Follow up and finish implementing the feature for all image types and for ICC as well as gamma.
> 
> This patch incurs technical debt for the project, which is generally something we'd like to avoid.  I trust you to pay off that debt in short order, but understand the costs you're imposing on the project because of your schedule pressure.

OK, for the JPEG I'll create a bug for that and will fix it in the future.

For ICC, I am working on it right now, so this debt will be paid soon.
Comment 25 Adam Barth 2010-11-17 11:19:52 PST
> OK, for the JPEG I'll create a bug for that and will fix it in the future.

The future = ???

> For ICC, I am working on it right now, so this debt will be paid soon.

Thanks.  :)
Comment 26 Zhenyao Mo 2010-11-17 11:38:51 PST
(In reply to comment #25)
> > OK, for the JPEG I'll create a bug for that and will fix it in the future.
> 
> The future = ???

I already created the bug and assigned to myself.  The fact is that for JPEG, ignoreGammaAndColorProfile = true or false doesn't matter at all in WebGL (and WebGL is the only client that might set ignoreGammaAndColorProfile to true), so it has to wait until more urgent tasks are out of my todo list.

> 
> > For ICC, I am working on it right now, so this debt will be paid soon.
> 
> Thanks.  :)
Comment 27 Adam Barth 2010-11-17 12:25:06 PST
(In reply to comment #26)
> (In reply to comment #25)
> > > OK, for the JPEG I'll create a bug for that and will fix it in the future.
> > 
> > The future = ???
> 
> I already created the bug and assigned to myself.  The fact is that for JPEG, ignoreGammaAndColorProfile = true or false doesn't matter at all in WebGL (and WebGL is the only client that might set ignoreGammaAndColorProfile to true), so it has to wait until more urgent tasks are out of my todo list.

What do you mean by "doesn't matter at all"?  Do you mean the API isn't exposed to the web or that you don't think many folks will run into the bug?
Comment 28 Kenneth Russell 2010-11-17 12:46:12 PST
(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > > OK, for the JPEG I'll create a bug for that and will fix it in the future.
> > > 
> > > The future = ???
> > 
> > I already created the bug and assigned to myself.  The fact is that for JPEG, ignoreGammaAndColorProfile = true or false doesn't matter at all in WebGL (and WebGL is the only client that might set ignoreGammaAndColorProfile to true), so it has to wait until more urgent tasks are out of my todo list.
> 
> What do you mean by "doesn't matter at all"?  Do you mean the API isn't exposed to the web or that you don't think many folks will run into the bug?

The WebGL setting to skip colorspace conversions by the browser (which sets this flag to true) is intended to allow authors to load image data untouched by the browser. It's most often used when loading non-image data, but using images to hold it. Examples include packing vertex data into pixels, or holding certain kinds of normal or light maps where the alpha channel holds additional, non-alpha data. In all of these cases it is basically never the case that the content author would use a lossy format such as JPEG as the container image; PNGs are the only kinds of images on the web today used for this purpose.
Comment 29 Adam Barth 2010-11-17 12:56:31 PST
So, it sounds like you mean that the API is exposed to the web and will be buggy, but that you don't think folks will run into this bug in practice.

I don't think it's a good idea to ship APIs that we know are buggy.  We should either not ship the API, fix the bug, or change the API.
Comment 30 Zhenyao Mo 2010-11-17 13:31:23 PST
(In reply to comment #29)
> So, it sounds like you mean that the API is exposed to the web and will be buggy, but that you don't think folks will run into this bug in practice.
> 
> I don't think it's a good idea to ship APIs that we know are buggy.  We should either not ship the API, fix the bug, or change the API.

I just had a look at the JPEG decoder.  Seems like an easy enough fix.  I'll just fix JPEG together with PNG then.  This should settle the argument.  :)
Comment 31 Adam Barth 2010-11-17 13:44:02 PST
> I just had a look at the JPEG decoder.  Seems like an easy enough fix.  I'll just fix JPEG together with PNG then.  This should settle the argument.  :)

Thanks!
Comment 32 Zhenyao Mo 2010-11-17 17:41:04 PST
Created attachment 74184 [details]
patch: hooking up COLORSPACE_CONVERSION setting with ICC profile behavior

Haven't updated the khronos side of the gl-teximage.html.  Will do so once it's reviewed.
Comment 33 Adam Barth 2010-11-17 17:45:57 PST
Comment on attachment 74184 [details]
patch: hooking up COLORSPACE_CONVERSION setting with ICC profile behavior

View in context: https://bugs.webkit.org/attachment.cgi?id=74184&action=review

Thanks.  Please address the comments below before landing.

> WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:247
> -            m_decoder->setColorProfile(readColorProfile(info()));              
> +            if (m_decoder->ignoresGammaAndColorProfile())
> +                m_decoder->setColorProfile(ColorProfile());
> +            else
> +                m_decoder->setColorProfile(readColorProfile(info()));

You don't need to call setColorProfile if you want the default..

> WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:269
> -    if (colorType == PNG_COLOR_TYPE_RGB || colorType == PNG_COLOR_TYPE_RGB_ALPHA) {
> +    if ((colorType == PNG_COLOR_TYPE_RGB || colorType == PNG_COLOR_TYPE_RGB_ALPHA)
> +        && !m_ignoreGammaAndColorProfile) {

One line please.
Comment 34 Zhenyao Mo 2010-11-18 09:55:33 PST
(In reply to comment #33)
> (From update of attachment 74184 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74184&action=review
> 
> Thanks.  Please address the comments below before landing.

Will do.

> 
> > WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:247
> > -            m_decoder->setColorProfile(readColorProfile(info()));              
> > +            if (m_decoder->ignoresGammaAndColorProfile())
> > +                m_decoder->setColorProfile(ColorProfile());
> > +            else
> > +                m_decoder->setColorProfile(readColorProfile(info()));
> 
> You don't need to call setColorProfile if you want the default..
> 
> > WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:269
> > -    if (colorType == PNG_COLOR_TYPE_RGB || colorType == PNG_COLOR_TYPE_RGB_ALPHA) {
> > +    if ((colorType == PNG_COLOR_TYPE_RGB || colorType == PNG_COLOR_TYPE_RGB_ALPHA)
> > +        && !m_ignoreGammaAndColorProfile) {
> 
> One line please.
Comment 35 Kenneth Russell 2010-11-18 13:35:00 PST
Comment on attachment 74184 [details]
patch: hooking up COLORSPACE_CONVERSION setting with ICC profile behavior

Looks good to me too.
Comment 36 Zhenyao Mo 2010-11-18 15:56:31 PST
Committed r72341: <http://trac.webkit.org/changeset/72341>