Bug 88565 - Add monitor profile support for Win and Mac
Summary: Add monitor profile support for Win and Mac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tony Payne
URL:
Keywords:
Depends on: 81974 91300
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-07 11:46 PDT by Tony Payne
Modified: 2012-07-13 22:09 PDT (History)
11 users (show)

See Also:


Attachments
Patch (34.49 KB, patch)
2012-06-07 11:47 PDT, Tony Payne
no flags Details | Formatted Diff | Diff
Patch (31.52 KB, patch)
2012-06-07 18:04 PDT, Tony Payne
no flags Details | Formatted Diff | Diff
Patch (31.71 KB, patch)
2012-06-08 16:34 PDT, Tony Payne
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.54 MB, application/zip)
2012-06-11 15:40 PDT, WebKit Review Bot
no flags Details
Patch (11.48 KB, patch)
2012-06-18 12:11 PDT, Tony Payne
no flags Details | Formatted Diff | Diff
Patch (11.60 KB, patch)
2012-06-19 10:26 PDT, Tony Payne
tpayne: commit-queue-
Details | Formatted Diff | Diff
Patch (11.54 KB, patch)
2012-06-19 14:33 PDT, Tony Payne
no flags Details | Formatted Diff | Diff
Patch (11.47 KB, patch)
2012-06-19 16:39 PDT, Tony Payne
no flags Details | Formatted Diff | Diff
Patch (10.87 KB, patch)
2012-06-19 16:42 PDT, Tony Payne
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Payne 2012-06-07 11:46:29 PDT
To improve image color correction, images with ICC profiles should be transformed to the monitor's color space, when available.
Comment 1 Tony Payne 2012-06-07 11:47:19 PDT
Created attachment 146344 [details]
Patch
Comment 2 WebKit Review Bot 2012-06-07 11:50:20 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 James Robinson 2012-06-07 11:59:38 PDT
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?
Comment 4 Tony Payne 2012-06-07 12:12:29 PDT
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.
Comment 5 James Robinson 2012-06-07 12:25:55 PDT
(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.
Comment 6 Tony Payne 2012-06-07 12:39:06 PDT
> > 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.
Comment 7 Tony Chang 2012-06-07 13:17:33 PDT
(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).
Comment 8 James Robinson 2012-06-07 14:53:13 PDT
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?
Comment 9 James Robinson 2012-06-07 14:54:45 PDT
I'm really curious about the answer to https://bugs.webkit.org/show_bug.cgi?id=81974#c97 as well
Comment 10 Adam Barth 2012-06-07 15:56:57 PDT
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.
Comment 11 Tony Payne 2012-06-07 18:04:35 PDT
Created attachment 146438 [details]
Patch
Comment 12 Tony Chang 2012-06-08 09:57:56 PDT
Peter might be interested in looking at the image decoder code.
Comment 13 Tony Chang 2012-06-08 10:07:55 PDT
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()?
Comment 14 Peter Kasting 2012-06-08 10:36:29 PDT
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.
Comment 15 Tony Payne 2012-06-08 10:41:42 PDT
(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()?
Comment 16 Tony Chang 2012-06-08 11:51:02 PDT
(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.
Comment 17 James Robinson 2012-06-08 12:47:28 PDT
(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.
Comment 18 Tony Chang 2012-06-08 13:27:50 PDT
(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.
Comment 19 Tony Payne 2012-06-08 13:39:29 PDT
(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.
Comment 20 Tony Payne 2012-06-08 16:34:48 PDT
Created attachment 146661 [details]
Patch
Comment 21 WebKit Review Bot 2012-06-11 11:39:57 PDT
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.
Comment 22 Early Warning System Bot 2012-06-11 12:26:57 PDT
Comment on attachment 146661 [details]
Patch

Attachment 146661 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12938492
Comment 23 Early Warning System Bot 2012-06-11 12:39:17 PDT
Comment on attachment 146661 [details]
Patch

Attachment 146661 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12936703
Comment 24 WebKit Review Bot 2012-06-11 15:40:48 PDT
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
Comment 25 WebKit Review Bot 2012-06-11 15:40:53 PDT
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
Comment 26 James Robinson 2012-06-11 17:36:14 PDT
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.
Comment 27 James Robinson 2012-06-11 17:40:42 PDT
It also appears to pick the majority monitor in split-window situations (according to smfr on IRC)
Comment 28 James Robinson 2012-06-11 17:41:12 PDT
Window split across multiple monitors, that is (that's handled by the call to [NSWindow colorspace], not by any special logic in WebKit).
Comment 29 James Robinson 2012-06-11 17:56:04 PDT
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?
Comment 30 Tony Payne 2012-06-14 15:25:02 PDT
(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.
Comment 31 Adam Barth 2012-06-15 20:55:29 PDT
Not sure why this is assigned to me.
Comment 32 Tony Payne 2012-06-18 12:11:06 PDT
Created attachment 148147 [details]
Patch
Comment 33 WebKit Review Bot 2012-06-18 12:14:08 PDT
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.
Comment 34 Tony Chang 2012-06-19 10:22:54 PDT
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.
Comment 35 Tony Payne 2012-06-19 10:26:36 PDT
Created attachment 148355 [details]
Patch
Comment 36 Tony Payne 2012-06-19 10:27:40 PDT
(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.
Comment 37 Tony Chang 2012-06-19 10:35:33 PDT
(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?
Comment 38 Tony Payne 2012-06-19 10:39:40 PDT
(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.
Comment 39 Adam Barth 2012-06-19 13:57:15 PDT
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.
Comment 40 Tony Payne 2012-06-19 14:33:28 PDT
Created attachment 148428 [details]
Patch
Comment 41 Tony Payne 2012-06-19 14:43:06 PDT
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.
Comment 42 Adam Barth 2012-06-19 15:56:39 PDT
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...
Comment 43 Eric Seidel (no email) 2012-06-19 16:15:56 PDT
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.
Comment 44 James Robinson 2012-06-19 16:18:51 PDT
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.
Comment 45 Eric Seidel (no email) 2012-06-19 16:38:59 PDT
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?
Comment 46 Tony Payne 2012-06-19 16:39:09 PDT
Created attachment 148457 [details]
Patch
Comment 47 Tony Payne 2012-06-19 16:40:15 PDT
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.
Comment 48 Eric Seidel (no email) 2012-06-19 16:40:33 PDT
(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...
Comment 49 James Robinson 2012-06-19 16:41:28 PDT
Let's talk about include paths for Platform APIs somewhere else if you want, they aren't related to this patch :)
Comment 50 Tony Payne 2012-06-19 16:42:00 PDT
Created attachment 148458 [details]
Patch
Comment 51 Tony Payne 2012-06-19 16:42:40 PDT
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.
Comment 52 Eric Seidel (no email) 2012-06-19 16:50:20 PDT
Comment on attachment 148458 [details]
Patch

LGTM.
Comment 53 WebKit Review Bot 2012-06-19 19:15:14 PDT
Comment on attachment 148458 [details]
Patch

Clearing flags on attachment: 148458

Committed r120789: <http://trac.webkit.org/changeset/120789>
Comment 54 WebKit Review Bot 2012-06-19 19:15:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 55 noel gordon 2012-06-29 11:22:26 PDT
(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.
Comment 56 James Robinson 2012-06-29 11:31:08 PDT
(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.
Comment 57 noel gordon 2012-07-03 06:13:24 PDT
(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).
Comment 58 noel gordon 2012-07-03 06:30:26 PDT
(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
Comment 59 noel gordon 2012-07-13 22:09:46 PDT
Nor do we need the image decoders to provide their widget in these color correction callbacks, bug 91300.