Bug 88565 – Add monitor profile support for Win and Mac

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Tony Payne]

To improve image color correction, images with ICC profiles should be transformed to the monitor's color space, when available.

[Tony Payne]

Created attachment 146344 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

Comment on attachment 146344 [details]
Patch

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

> Source/Platform/chromium/public/Platform.h:181
> +    virtual WebVector<char>* getMonitorColorProfile() { return new WebVector<char>(); }

What is the actual data being represented here?

It's more common for the default impl to return null for a function like this, not an empty value.

What is the ownership model of this value - is the caller supposed to delete it?  That's a bit strange.

We typically don't use "get" for functions like this (see the rest of the file).

Which monitor is this function referring to in a multi-monitor setup?

Can this value change - for instance, if the user changes the monitor's color profile, plugs a new monitor in, moves a window from one monitor to another, etc?  If so, what happens then?

> Source/WebCore/platform/image-decoders/ImageDecoder.h:36
> +#include "third_party/WebKit/Source/WebKit/chromium/public/platform/WebVector.h"

we don't use absolute include paths like this in WebKit, and this won't even be valid for most users of this file.

WebVector is a chromium-specific type and this appears to be a cross-platform file based on the build path.  This should be guarded by #if PLATFORM(CHROMIUM) or refer to a cross-platform concept

> Source/WebCore/platform/image-decoders/ImageDecoder.h:307
> +                OwnPtr<WebKit::WebVector<char> > profile = WTF::adoptPtr(WebKit::Platform::current()->getMonitorColorProfile());

This will only compile on chromium, but this doesn't appear to be a chromium-specific file or a chromium-specific block.  What should happen for other platforms?

> LayoutTests/platform/chromium/TestExpectations:3623
> +BUGWK87761 MAC LINUX WIN : css3/filters/effect-blur-hw.html = IMAGE

do these tests all contain source images with ICC profiles?  is this an intentional part of the test?

[Tony Payne]

Comment on attachment 146344 [details]
Patch

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

>> Source/Platform/chromium/public/Platform.h:181
>> +    virtual WebVector<char>* getMonitorColorProfile() { return new WebVector<char>(); }
> 
> What is the actual data being represented here?
> 
> It's more common for the default impl to return null for a function like this, not an empty value.
> 
> What is the ownership model of this value - is the caller supposed to delete it?  That's a bit strange.
> 
> We typically don't use "get" for functions like this (see the rest of the file).
> 
> Which monitor is this function referring to in a multi-monitor setup?
> 
> Can this value change - for instance, if the user changes the monitor's color profile, plugs a new monitor in, moves a window from one monitor to another, etc?  If so, what happens then?

Sorry, I hadn't intended to request a review; I was just uploading to facilitate testing on other platforms.

I'll be happy to change it to return null by default and change the name.

This is referring to the primary monitor. Ideally, this would be whatever monitor the app is running on, but what do we do when split across monitors?

Yes this value can change, but for now, I don't intend to pick up the new profile until the app restarts.

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:36
>> +#include "third_party/WebKit/Source/WebKit/chromium/public/platform/WebVector.h"
> 
> we don't use absolute include paths like this in WebKit, and this won't even be valid for most users of this file.
> 
> WebVector is a chromium-specific type and this appears to be a cross-platform file based on the build path.  This should be guarded by #if PLATFORM(CHROMIUM) or refer to a cross-platform concept

I've seen other core webkit files referring to it, but I'm happy to change over to WTF::Vector. Do you see any problem with making WTF::Vector the return type of the Platform interface method?

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:307
>> +                OwnPtr<WebKit::WebVector<char> > profile = WTF::adoptPtr(WebKit::Platform::current()->getMonitorColorProfile());
> 
> This will only compile on chromium, but this doesn't appear to be a chromium-specific file or a chromium-specific block.  What should happen for other platforms?

Currently, USE(QCMSLIB) is only chromium. If this is WTF::Vector instead, are there any other problems for non-chromium ports?

>> LayoutTests/platform/chromium/TestExpectations:3623
>> +BUGWK87761 MAC LINUX WIN : css3/filters/effect-blur-hw.html = IMAGE
> 
> do these tests all contain source images with ICC profiles?  is this an intentional part of the test?

This CL is based on 81974, this change is part of the other CL. Yes, these contain source images with ICC profiles. No, it is not an intentional part of the tests. The author seems to have been confused between Generic RGB and sRGB, assuming they were the same thing.

[James Robinson]

(In reply to comment #4)
> (From update of attachment 146344 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146344&action=review
> 
> >> Source/Platform/chromium/public/Platform.h:181
> >> +    virtual WebVector<char>* getMonitorColorProfile() { return new WebVector<char>(); }
> > 
> > What is the actual data being represented here?
> > 
> > It's more common for the default impl to return null for a function like this, not an empty value.
> > 
> > What is the ownership model of this value - is the caller supposed to delete it?  That's a bit strange.
> > 
> > We typically don't use "get" for functions like this (see the rest of the file).
> > 
> > Which monitor is this function referring to in a multi-monitor setup?
> > 
> > Can this value change - for instance, if the user changes the monitor's color profile, plugs a new monitor in, moves a window from one monitor to another, etc?  If so, what happens then?
> 
> Sorry, I hadn't intended to request a review; I was just uploading to facilitate testing on other platforms.

Ah!  A script thought you had (try webkit-patch --no-review in the future to upload without asking for review).

> 
> I'll be happy to change it to return null by default and change the name.
> 
> This is referring to the primary monitor. Ideally, this would be whatever monitor the app is running on, but what do we do when split across monitors?

In other cases we use the dominant display in split situations i.e. the display that has more pixels.

This isn't really a property of the platform, it's a property of a view.

> 
> Yes this value can change, but for now, I don't intend to pick up the new profile until the app restarts.

That seems really unfortunate.  Why not handle that case as well?  Would that require re-decoding the image?

I think having WebKit poll this value from the embedder is a bad design - we've used this for other screen-type APIs in the past and it's always turned out badly.

> 
> >> Source/WebCore/platform/image-decoders/ImageDecoder.h:36
> >> +#include "third_party/WebKit/Source/WebKit/chromium/public/platform/WebVector.h"
> > 
> > we don't use absolute include paths like this in WebKit, and this won't even be valid for most users of this file.
> > 
> > WebVector is a chromium-specific type and this appears to be a cross-platform file based on the build path.  This should be guarded by #if PLATFORM(CHROMIUM) or refer to a cross-platform concept
> 
> I've seen other core webkit files referring to it, but I'm happy to change over to WTF::Vector. Do you see any problem with making WTF::Vector the return type of the Platform interface method?

I think you should find someone familiar with WebKit's layering and platform conventions and talk through this part of the patch.

[Tony Payne]

> > Yes this value can change, but for now, I don't intend to pick up the new profile until the app restarts.
> 
> That seems really unfortunate.  Why not handle that case as well?  Would that require re-decoding the image?

Yes, we decode directly to the monitor's color profile. Changing the profile would require a re-decode.

> I think having WebKit poll this value from the embedder is a bad design - we've used this for other screen-type APIs in the past and it's always turned out badly.

My original plan was to stash the profile in shared memory and pass the pointer in the renderer configuration struct. However, talking with Tony Chang, I understood that this way would be preferred. I very likely misunderstood, though.

> > 
> > >> Source/WebCore/platform/image-decoders/ImageDecoder.h:36
> > >> +#include "third_party/WebKit/Source/WebKit/chromium/public/platform/WebVector.h"
> > > 
> > > we don't use absolute include paths like this in WebKit, and this won't even be valid for most users of this file.
> > > 
> > > WebVector is a chromium-specific type and this appears to be a cross-platform file based on the build path.  This should be guarded by #if PLATFORM(CHROMIUM) or refer to a cross-platform concept
> > 
> > I've seen other core webkit files referring to it, but I'm happy to change over to WTF::Vector. Do you see any problem with making WTF::Vector the return type of the Platform interface method?
> 
> I think you should find someone familiar with WebKit's layering and platform conventions and talk through this part of the patch.

Added Tony. If you know of someone else that would be a better guide, please let me know.

Thanks for taking the time to straighten me out.

[Tony Chang]

(In reply to comment #5)
> I think having WebKit poll this value from the embedder is a bad design - we've used this for other screen-type APIs in the past and it's always turned out badly.

Since most images don't have ICC profiles, it seemed better to have WebKit only request the profile when needed (i.e., lazy init).  We could push the profile information down during renderer startup, but that seems wasteful (both in cpu and memory).

[James Robinson]

Having this value be very rarely used is a good reason for polling.  It seems like we should try to make the query be on an interface specific to a view/monitor, unless we want to completely not handle multi-monitor setups forever.

If we're doing this at decode time, does that mean that the color profile applies regardless of how the image is being used?  What if it's being loaded into a canvas or some other use other than being renderer directly to screen - do we care?

[James Robinson]

I'm really curious about the answer to https://bugs.webkit.org/show_bug.cgi?id=81974#c97 as well

[Adam Barth]

Comment on attachment 146344 [details]
Patch

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

>>> Source/WebCore/platform/image-decoders/ImageDecoder.h:307
>>> +                OwnPtr<WebKit::WebVector<char> > profile = WTF::adoptPtr(WebKit::Platform::current()->getMonitorColorProfile());
>> 
>> This will only compile on chromium, but this doesn't appear to be a chromium-specific file or a chromium-specific block.  What should happen for other platforms?
> 
> Currently, USE(QCMSLIB) is only chromium. If this is WTF::Vector instead, are there any other problems for non-chromium ports?

This should probably be added to <http://trac.webkit.org/browser/trunk/Source/WebCore/platform/PlatformScreen.h>.  We need to design this to work on multiple ports even if it only works on Chromium today.

[Tony Payne]

Created attachment 146438 [details]
Patch

[Tony Chang]

Peter might be interested in looking at the image decoder code.

[Tony Chang]

Comment on attachment 146438 [details]
Patch

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

> Source/WebCore/platform/PlatformScreen.h:32
>  #include <wtf/Forward.h>
>  #include <wtf/RefPtr.h>
> +#include <wtf/Vector.h>

Nit: You shouldn't need to include Vector.h since Forward.h forward declares it for you.

> Source/WebCore/platform/blackberry/PlatformScreenBlackBerry.cpp:69
> +{
> +}

Nit: Maybe call notImplemented()?

> Source/WebCore/platform/efl/PlatformScreenEfl.cpp:97
> +{
> +}

Nit: Maybe call notImplemented()?

> Source/WebCore/platform/gtk/PlatformScreenGtk.cpp:142
> +{
> +}

Nit: Maybe call notImplemented()?

> Source/WebCore/platform/image-decoders/ImageDecoder.h:303
> +            static int volatile qcmsInitialized = 0;
> +            if (atomicIncrement(&qcmsInitialized) == 1) {

Do we need to be worried about 2 threads racing here?

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:190
> +    void createColorTransform(const ColorProfile& colorProfile, bool hasAlpha)
> +    {
> +        if (m_transform)

This looks identical to the code in the jpeg decoder.  Is it possible to share the code somehow?

> Source/WebCore/platform/mac/PlatformScreenMac.mm:81
> +{
> +}

Nit: Maybe call notImplemented()?

> Source/WebCore/platform/qt/PlatformScreenQt.cpp:148
> +{
> +}

Nit: Maybe call notImplemented()?

> Source/WebCore/platform/win/PlatformScreenWin.cpp:123
> +{
> +}

Nit: Maybe call notImplemented()?

[Peter Kasting]

Comment on attachment 146438 [details]
Patch

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

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:303
>> +            if (atomicIncrement(&qcmsInitialized) == 1) {
> 
> Do we need to be worried about 2 threads racing here?

I also wonder this, I thought image decoding ran only on the main WebKit thread.

[Tony Payne]

(In reply to comment #13)
> (From update of attachment 146438 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146438&action=review
> 
> > Source/WebCore/platform/PlatformScreen.h:32
> >  #include <wtf/Forward.h>
> >  #include <wtf/RefPtr.h>
> > +#include <wtf/Vector.h>
> 
> Nit: You shouldn't need to include Vector.h since Forward.h forward declares it for you.
> 
> > Source/WebCore/platform/blackberry/PlatformScreenBlackBerry.cpp:69
> > +{
> > +}
> 
> Nit: Maybe call notImplemented()?

My intention is that the method be callable, even for ports that don't implement monitor profile. For those ports, we would use sRGB as the target profile (all of this only happens when USE(QCMSLIB) is defined).

> 
> > Source/WebCore/platform/efl/PlatformScreenEfl.cpp:97
> > +{
> > +}
> 
> Nit: Maybe call notImplemented()?
> 
> > Source/WebCore/platform/gtk/PlatformScreenGtk.cpp:142
> > +{
> > +}
> 
> Nit: Maybe call notImplemented()?
> 
> > Source/WebCore/platform/image-decoders/ImageDecoder.h:303
> > +            static int volatile qcmsInitialized = 0;
> > +            if (atomicIncrement(&qcmsInitialized) == 1) {
> 
> Do we need to be worried about 2 threads racing here?

Can we move this discussion to the other review (81974)?

> > Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:190
> > +    void createColorTransform(const ColorProfile& colorProfile, bool hasAlpha)
> > +    {
> > +        if (m_transform)
> 
> This looks identical to the code in the jpeg decoder.  Is it possible to share the code somehow?

Again, this should go on 81974. The decoders will be diverging soon, so Noel would rather they not share code.

> > Source/WebCore/platform/mac/PlatformScreenMac.mm:81
> > +{
> > +}
> 
> Nit: Maybe call notImplemented()?
> 
> > Source/WebCore/platform/qt/PlatformScreenQt.cpp:148
> > +{
> > +}
> 
> Nit: Maybe call notImplemented()?
> 
> > Source/WebCore/platform/win/PlatformScreenWin.cpp:123
> > +{
> > +}
> 
> Nit: Maybe call notImplemented()?

[Tony Chang]

(In reply to comment #15)
> (In reply to comment #13)
> > (From update of attachment 146438 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=146438&action=review
> > 
> > > Source/WebCore/platform/blackberry/PlatformScreenBlackBerry.cpp:69
> > > +{
> > > +}
> > 
> > Nit: Maybe call notImplemented()?
> 
> My intention is that the method be callable, even for ports that don't implement monitor profile. For those ports, we would use sRGB as the target profile (all of this only happens when USE(QCMSLIB) is defined).

It will still be callable. In WebCore, notImplemented just prints an error. It's not like a DCHECK.

[James Robinson]

(In reply to comment #14)
> (From update of attachment 146438 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146438&action=review
> 
> >> Source/WebCore/platform/image-decoders/ImageDecoder.h:303
> >> +            if (atomicIncrement(&qcmsInitialized) == 1) {
> > 
> > Do we need to be worried about 2 threads racing here?
> 
> I also wonder this, I thought image decoding ran only on the main WebKit thread.

This is true today, but we definitely want to be able to decode from non-main threads in the future.

[Tony Chang]

(In reply to comment #17)
> (In reply to comment #14)
> > (From update of attachment 146438 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=146438&action=review
> > 
> > >> Source/WebCore/platform/image-decoders/ImageDecoder.h:303
> > >> +            if (atomicIncrement(&qcmsInitialized) == 1) {
> > > 
> > > Do we need to be worried about 2 threads racing here?
> > 
> > I also wonder this, I thought image decoding ran only on the main WebKit thread.
> 
> This is true today, but we definitely want to be able to decode from non-main threads in the future.

Is the rest of the image decoding code already thread safe? I guess I'm worried about someone reading the code who sees this may think it's OK to use on multiple threads.

If we want to make the whole thing thread safe, we should do that in a separate patch.

[Tony Payne]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #14)
> > > (From update of attachment 146438 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=146438&action=review
> > > 
> > > >> Source/WebCore/platform/image-decoders/ImageDecoder.h:303
> > > >> +            if (atomicIncrement(&qcmsInitialized) == 1) {
> > > > 
> > > > Do we need to be worried about 2 threads racing here?
> > > 
> > > I also wonder this, I thought image decoding ran only on the main WebKit thread.
> > 
> > This is true today, but we definitely want to be able to decode from non-main threads in the future.
> 
> Is the rest of the image decoding code already thread safe? I guess I'm worried about someone reading the code who sees this may think it's OK to use on multiple threads.
> 
> If we want to make the whole thing thread safe, we should do that in a separate patch.

I really would prefer we have this discussion on bug 81974, since that is the CL where this change will land.

According to Noel, the decoders are already thread-safe and he would prefer they stay that way.

[Tony Payne]

Created attachment 146661 [details]
Patch

[WebKit Review Bot]

Attachment 146661 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 146661 [details]
Patch

Attachment 146661 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12938492

[Early Warning System Bot]

Comment on attachment 146661 [details]
Patch

Attachment 146661 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12936703

[WebKit Review Bot]

Comment on attachment 146661 [details]
Patch

Attachment 146661 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12942360

New failing tests:
css3/filters/effect-blur-hw.html
css3/filters/effect-combined.html
css3/filters/custom/custom-filter-shader-cache.html
css3/filters/effect-blur.html
css3/filters/effect-invert-hw.html
css3/filters/custom/effect-custom.html
css3/filters/effect-grayscale.html
css3/filters/effect-sepia.html
http/tests/local/file-url-sent-as-referer.html
css3/filters/custom/effect-custom-combined-missing.html
css3/filters/effect-opacity-hw.html
css3/filters/effect-brightness-hw.html
css3/filters/effect-saturate.html
css3/filters/effect-hue-rotate.html
css3/filters/effect-drop-shadow.html
css3/filters/effect-invert.html
css3/filters/effect-contrast.html
css3/filters/effect-brightness.html
css3/filters/crash-hw-sw-switch.html
css3/filters/effect-drop-shadow-hw.html
css3/filters/effect-opacity.html
css3/filters/regions-expanding.html
css3/filters/crash-filter-change.html
compositing/color-matching/image-color-matching.html
css3/filters/effect-contrast-hw.html
css3/filters/custom/missing-custom-filter-shader.html

[WebKit Review Bot]

Created attachment 146935 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[James Robinson]

Apple just landed WebKit2 support for color spaces changing (which I think is the same thing as monitor profile in this patch, or at least closely related): https://bugs.webkit.org/show_bug.cgi?id=88819.  It appears to work by listening for color space change notifications in the WK2 equivalent of the browser process and then, when it changes, sending it down to the associated view.

[James Robinson]

It also appears to pick the majority monitor in split-window situations (according to smfr on IRC)

[James Robinson]

Window split across multiple monitors, that is (that's handled by the call to [NSWindow colorspace], not by any special logic in WebKit).

[James Robinson]

Some more info on Apple's approach - it sounds like they apply color correction at render time, not decode time.  It seems that this is the only sane way to deal with multi-monitor or profile switching situations.  Is there any reason we can't do the same - is the paint time cost too much?

[Tony Payne]

(In reply to comment #29)
> Some more info on Apple's approach - it sounds like they apply color correction at render time, not decode time.  It seems that this is the only sane way to deal with multi-monitor or profile switching situations.  Is there any reason we can't do the same - is the paint time cost too much?

I am not familiar with that area of the code, so I cannot tell you if it's doable. What I can tell you is that I'd rather have support for the primary monitor than no support at all. We can revisit the split-window and multiple-monitor scenario, but that is not what I'm trying to solve with this change.

[Adam Barth]

Not sure why this is assigned to me.

[Tony Payne]

Created attachment 148147 [details]
Patch

[WebKit Review Bot]

Attachment 148147 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tony Chang]

Comment on attachment 148147 [details]
Patch

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

The code looks fine to me. One of the API owners will have to approve.

> Source/WebCore/ChangeLog:3
> +        [chromium] Add monitor profile support for Win and Mac

This isn't just [chromium] since you're adding a method to ImageDecoder.h on all platforms.

>> Source/WebCore/ChangeLog:8
>> +        No new tests. (OOPS!)
> 
> You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]

The suppressions you added in bug 81974 should cover chromium win and chromium mac, right?  I would change this line to something like:

Tests: fast/images/jpeg-with-color-profile.html
       fast/images/png-with-color-profile.html
Also covered by suppressions added to TestExpectations in r120485. These tests will be rebaselined after I verify the results on the bots.

[Tony Payne]

Created attachment 148355 [details]
Patch

[Tony Payne]

(In reply to comment #34)
> (From update of attachment 148147 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148147&action=review
> 
> The code looks fine to me. One of the API owners will have to approve.
> 
> > Source/WebCore/ChangeLog:3
> > +        [chromium] Add monitor profile support for Win and Mac
> 
> This isn't just [chromium] since you're adding a method to ImageDecoder.h on all platforms.

Done.

> >> Source/WebCore/ChangeLog:8
> >> +        No new tests. (OOPS!)
> > 
> > You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
> 
> The suppressions you added in bug 81974 should cover chromium win and chromium mac, right?  I would change this line to something like:
> 
> Tests: fast/images/jpeg-with-color-profile.html
>        fast/images/png-with-color-profile.html
> Also covered by suppressions added to TestExpectations in r120485. These tests will be rebaselined after I verify the results on the bots.

The tests have already been rebaselined.

[Tony Chang]

(In reply to comment #36)
> The tests have already been rebaselined.

In TestExpectations, I see:

BUGWK70001 SKIP LINUX WIN : fast/images/color-jpeg-with-color-profile.html = PASS

Should we start running this test?  Also, won't the results for png-with-color-profile.html change on Chromium Win and Chromium Mac?

[Tony Payne]

(In reply to comment #37)
> (In reply to comment #36)
> > The tests have already been rebaselined.
> 
> In TestExpectations, I see:
> 
> BUGWK70001 SKIP LINUX WIN : fast/images/color-jpeg-with-color-profile.html = PASS
> 
> Should we start running this test?  

Yes, we should.

> Also, won't the results for png-with-color-profile.html change on Chromium Win and Chromium Mac?

Not yet. Chromium Mac already has its monitor support. Chromium Win won't have the monitor support until the chrome half of the change lands. At that point, we'll need to rebaseline a large number of Windows tests.

[Adam Barth]

Comment on attachment 148355 [details]
Patch

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

> Source/Platform/chromium/public/Platform.h:181
> +    // Supplies the system monitor color profile.
> +    virtual void monitorColorProfile(const WebString&, WebVector<char>*) { }

Should we have a WebColorProfile type?  It could even just be a typedef for a WebVector.

> Source/WebCore/platform/PlatformScreen.h:55
> +    void screenColorProfile(Widget*, const WTF::String&, ColorProfile*);

WTF::String -> String

Should we name the second parameter?  It's not clear to me what it represents.  Also, the convention in WebKit is to pass out parameters by (non-const) reference.

[Tony Payne]

Created attachment 148428 [details]
Patch

[Tony Payne]

Comment on attachment 148355 [details]
Patch

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

>> Source/Platform/chromium/public/Platform.h:181
>> +    virtual void monitorColorProfile(const WebString&, WebVector<char>*) { }
> 
> Should we have a WebColorProfile type?  It could even just be a typedef for a WebVector.

I'm not sure what it'll buy us. Can we do it later? If I understand correctly, it'll require a chromium change and wait for WK's chromium version to update.

>> Source/WebCore/platform/PlatformScreen.h:55
>> +    void screenColorProfile(Widget*, const WTF::String&, ColorProfile*);
> 
> WTF::String -> String
> 
> Should we name the second parameter?  It's not clear to me what it represents.  Also, the convention in WebKit is to pass out parameters by (non-const) reference.

Done.

[Adam Barth]

Comment on attachment 148428 [details]
Patch

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

I feel like I'm nit-picking your function names, which probably means we're all set.  :)

> Source/Platform/chromium/public/Platform.h:181
> +    // Supplies the system monitor color profile.
> +    virtual void monitorColorProfile(const WebString&, WebVector<char>*) { }

I would have named the string here, or maybe included a description of what this function does in the comment.

> Source/WebCore/platform/chromium/PlatformScreenChromium.cpp:82
> +void screenColorProfile(Widget*, const String& type, ColorProfile& toProfile)
> +{
> +    // FIXME: Add support for multiple monitors.
> +    WebKit::WebVector<char> profile;
> +    WebKit::Platform::current()->monitorColorProfile(WebKit::WebString(type), &profile);
> +    toProfile.append(profile.data(), profile.size());
> +}

It's a bit strange that the function changes names here.  I would have expected screenColorProfile() to always return the color profile of the screen.  Similarly, I would have expected the API to be called colorProfile if it can give you back an "sRGB" profile, which would seem to have nothing to do with a monitor...

[Eric Seidel (no email)]

Comment on attachment 148428 [details]
Patch

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

> Source/WebCore/platform/image-decoders/ImageDecoder.h:48
> +#include <public/Platform.h>

I may have missed the memo, but I'm confused by the "public/" prefix on this include.

[James Robinson]

That's how we refer to chromium Platform API from WebCore.  It should only appear in files with "chromium" in the path, "Chromium" in the filename, or inside a #if PLATFORM(CHROMIUM) block.

[Eric Seidel (no email)]

Comment on attachment 148428 [details]
Patch

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

> Source/WTF/ChangeLog:18
> +        [chromium] Add monitor profile support for Win and Mac

This changelog seems superfluous?

[Tony Payne]

Created attachment 148457 [details]
Patch

[Tony Payne]

Comment on attachment 148428 [details]
Patch

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

>> Source/Platform/chromium/public/Platform.h:181
>> +    virtual void monitorColorProfile(const WebString&, WebVector<char>*) { }
> 
> I would have named the string here, or maybe included a description of what this function does in the comment.

Done.

>> Source/WebCore/platform/chromium/PlatformScreenChromium.cpp:82
>> +}
> 
> It's a bit strange that the function changes names here.  I would have expected screenColorProfile() to always return the color profile of the screen.  Similarly, I would have expected the API to be called colorProfile if it can give you back an "sRGB" profile, which would seem to have nothing to do with a monitor...

Changed to use a consistent name.

I personally would have preferred a separate API for getting a named profile rather than overloading this API.

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:48
>> +#include <public/Platform.h>
> 
> I may have missed the memo, but I'm confused by the "public/" prefix on this include.

Good catch. This is a leftover from an old, horribly wrong implementation.

[Eric Seidel (no email)]

(In reply to comment #44)
> That's how we refer to chromium Platform API from WebCore.  It should only appear in files with "chromium" in the path, "Chromium" in the filename, or inside a #if PLATFORM(CHROMIUM) block.

That stikes me as a bad idea... (that inlcude paths shoudl be chromium specific!).  Additionally, this patch uses it inside a USE(QCMSLIB) block, which is not necessarily chromium specific...

[James Robinson]

Let's talk about include paths for Platform APIs somewhere else if you want, they aren't related to this patch :)

[Tony Payne]

Created attachment 148458 [details]
Patch

[Tony Payne]

Comment on attachment 148428 [details]
Patch

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

>> Source/WTF/ChangeLog:18
>> +        [chromium] Add monitor profile support for Win and Mac
> 
> This changelog seems superfluous?

Fixed.

[Eric Seidel (no email)]

Comment on attachment 148458 [details]
Patch

LGTM.

[WebKit Review Bot]

Comment on attachment 148458 [details]
Patch

Clearing flags on attachment: 148458

Committed r120789: <http://trac.webkit.org/changeset/120789>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

(In reply to comment #29)
> Some more info on Apple's approach - it sounds like they apply color correction at render time, not decode time.  It seems that this is the only sane way to deal with multi-monitor or profile switching situations.  Is there any reason we can't do the same - is the paint time cost too much?

Yes at render time, and that means images never need re-decoding.  This is indeed the only sane way to deal with multi-monitors and profile switching.  It can be made to work on win32 with a color correcting BitBlt, but the paint cost is high compared to a normal BitBlt.

Maybe the GPU could be used to improve speed?  The mismatching colors of windowed plugins would be an issue regardless.

[James Robinson]

(In reply to comment #55)
> (In reply to comment #29)
> > Some more info on Apple's approach - it sounds like they apply color correction at render time, not decode time.  It seems that this is the only sane way to deal with multi-monitor or profile switching situations.  Is there any reason we can't do the same - is the paint time cost too much?
> 
> Yes at render time, and that means images never need re-decoding.  This is indeed the only sane way to deal with multi-monitors and profile switching.  It can be made to work on win32 with a color correcting BitBlt, but the paint cost is high compared to a normal BitBlt.

Do you know what other win32 applications that deal with color correction do?

> 
> Maybe the GPU could be used to improve speed?  The mismatching colors of windowed plugins would be an issue regardless.

Yes, when we're using the GPU for compositing we definitely should use it to perform final color correction.

[noel gordon]

(In reply to comment #56)
> (In reply to comment #55)
> > 
> > Yes at render time, and that means images never need re-decoding.  This is indeed the only sane way to deal with multi-monitors and profile switching.  It can be made to work on win32 with a color correcting BitBlt, but the paint cost is high compared to a normal BitBlt.
> 
> Do you know what other win32 applications that deal with color correction do?

The color-enabled GDI routines are listed here:

 http://msdn.microsoft.com/en-us/library/windows/desktop/dd372214(v=vs.85).aspx
 http://msdn.microsoft.com/en-us/library/windows/desktop/dd144990(v=vs.85).aspx

Apps usually BitBlt their window contents to the screen or printer, so that's the time to apply color correction.  The lists above focus on the *Blit* class of routines: aka the render time code and again that seems sane.

BitBlt is h/w accelerated (except on Vista) and has always been rocket-ship fast.  BitBlit does support color correction (ignore the lists above on that) but it's not cheap - you lose h/w acceleration (high paint cost).

[noel gordon]

(In reply to comment #56)
>>
> > Maybe the GPU could be used to improve speed?  The mismatching colors of windowed plugins would be an issue regardless.
> 
> Yes, when we're using the GPU for compositing we definitely should use it to perform final color correction.

I'm glad you said "final": it really is and should be the final render step needed before the pixels hit the target.  It's certainly that way on a Mac as outlined above.

And win32 is that way, but it's relatively slow, except if we use the GPU for color correction and make it multi-monitor aware from the outset

   http://codereview.chromium.org/10703062

[noel gordon]

Nor do we need the image decoders to provide their widget in these color correction callbacks, bug 91300.