Bug 47196 – Implement UNPACK_COLORSPACE_CONVERSION_WEBGL

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kenneth Russell]

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.

[Kenneth Russell]

*** Bug 44568 has been marked as a duplicate of this bug. ***

[Kenneth Russell]

*** Bug 35050 has been marked as a duplicate of this bug. ***

[Zhenyao Mo]

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.

[Zhenyao Mo]

Created attachment 73661 [details]
revised patch : resync texture-transparant-... test.

[Kenneth Russell]

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.

[Zhenyao Mo]

Committed r71855: <http://trac.webkit.org/changeset/71855>

[Zhenyao Mo]

Accidentally closed this bug when committing the first patch: it's not fixed yet.

[Zhenyao Mo]

Created attachment 73679 [details]
patch: hook up UNPACK_COLORSPACE_CONVERSION setting to texture uploading.

[Zhenyao Mo]

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.

[Kenneth Russell]

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.

[Adam Barth]

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?

[Zhenyao Mo]

(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.

[Zhenyao Mo]

Created attachment 73913 [details]
revised patch: fixed the parameter names and also in the comments.

Use ignoreGammaAndColorProfile all over.

[Kenneth Russell]

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.

[Kenneth Russell]

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.

[Kenneth Russell]

Comment on attachment 73913 [details]
revised patch: fixed the parameter names and also in the comments.

R=me

[Zhenyao Mo]

Committed r72130: <http://trac.webkit.org/changeset/72130>

[Zhenyao Mo]

Forget about --no-close again.  Re-open this bug for ICC profile issue.

[Adam Barth]

(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?

[Adam Barth]

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.

[Zhenyao Mo]

(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.

[Zhenyao Mo]

(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.

[Adam Barth]

> > 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.

[Zhenyao Mo]

(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.

[Adam Barth]

> 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.  :)

[Zhenyao Mo]

(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.  :)

[Adam Barth]

(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?

[Kenneth Russell]

(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.

[Adam Barth]

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.

[Zhenyao Mo]

(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.  :)

[Adam Barth]

> 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!

[Zhenyao Mo]

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.

[Adam Barth]

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.

[Zhenyao Mo]

(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.

[Kenneth Russell]

Comment on attachment 74184 [details]
patch: hooking up COLORSPACE_CONVERSION setting with ICC profile behavior

Looks good to me too.

[Zhenyao Mo]

Committed r72341: <http://trac.webkit.org/changeset/72341>