Bug 69144 – Apply color profile found to decoded bitmap (Skia on Mac)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Cary Clark]

Apply color profile found to decoded bitmap (Skia on Mac)

[Cary Clark]

Created attachment 109292 [details]
Patch

[Stephen White]

Comment on attachment 109292 [details]
Patch

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

Please fix the #include path; the rest is up to you.

> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:34
> +#include "third_party/skia/include/utils/mac/SkCGUtils.h"

We don't hardcode chrome's third_party paths in WebKit code.  AFAICT, we -I the skia include dirs in (chrome's) skia/skia.gyp.  It looks like it's already there (under 'OS == "mac"' [...] 'include_dirs"), so you should just be able to #include "SkCGUtils.h" here.  If not, you may have to land a Chrome change to fix it first.

> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:124
> +    bitmap.lockPixels();

Nit:  Could use an SkAutoLockPixels guard instead.

> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:127
> +    if (bmap) {

Nit:  WebKit style is to use early-return where possible, so if you used the SkAutoLockPixels above, you could just 

if (!bmap)
    return;

here.

> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:150
> +    m_colorProfile = colorProfile;

Is this file compiled on non-mac platforms?  (I've never looked into the image decoders much before).  If so, I guess this will just be setting a color profile that we subsequently ignore on non-mac.  Not a big deal, I suppose, just that it will no generate a log message on incorrect code.

[Adam Barth]

Comment on attachment 109292 [details]
Patch

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

> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:126
> +    RetainPtr<CGContextRef> bmap(AdoptCF, CGBitmapContextCreate(pixels, width, height, 8, width * 4, deviceRGBColorSpaceRef(), kCGBitmapByteOrder32Host | kCGImageAlphaPremultipliedFirst));

Another nit: Please use complete words when naming variables.  "bmap" isn't really a WebKit-style name.

> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:130
> +        CGContextDrawImage(bmap.get(), bounds, srcImage);

This does a memcpy of the image?  I suppose that's unavoidable unless we resolve the color space as we decode the image.

[Cary Clark]

Created attachment 109342 [details]
Patch

[Cary Clark]

Comment on attachment 109292 [details]
Patch

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

>> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:34
>> +#include "third_party/skia/include/utils/mac/SkCGUtils.h"
> 
> We don't hardcode chrome's third_party paths in WebKit code.  AFAICT, we -I the skia include dirs in (chrome's) skia/skia.gyp.  It looks like it's already there (under 'OS == "mac"' [...] 'include_dirs"), so you should just be able to #include "SkCGUtils.h" here.  If not, you may have to land a Chrome change to fix it first.

Done.

>> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:124
>> +    bitmap.lockPixels();
> 
> Nit:  Could use an SkAutoLockPixels guard instead.

Done.

>> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:130
>> +        CGContextDrawImage(bmap.get(), bounds, srcImage);
> 
> This does a memcpy of the image?  I suppose that's unavoidable unless we resolve the color space as we decode the image.

Yes. Not sure how to avoid this even with our own version of CMS. That's certainly something to keep in mind in the future.

>> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:150
>> +    m_colorProfile = colorProfile;
> 
> Is this file compiled on non-mac platforms?  (I've never looked into the image decoders much before).  If so, I guess this will just be setting a color profile that we subsequently ignore on non-mac.  Not a big deal, I suppose, just that it will no generate a log message on incorrect code.

Fixed not to compile code on non-Mac platforms.

[Stephen White]

Comment on attachment 109342 [details]
Patch

Looks good.  Thanks for the fixes.  However, note that you'll need to roll DEPS in WebKit past your chrome change before this can land, otherwise the build.webkit.org builders will fail to compile.  The file is in WebKit/Source/WebKit/chromium/DEPS.  You can add make that change on landing this patch if you like, although if it's a big roll you might want to upload it here to get some EWS coverage.

r=me

[Cary Clark]

Comment on attachment 109342 [details]
Patch

Make sure that corresponding Chromium DEPS has rolled and that there is no layout regression before committing.

[Cary Clark]

Comment on attachment 109342 [details]
Patch

test expectations have been updated to disable Skia on Mac for now.

[WebKit Review Bot]

Comment on attachment 109342 [details]
Patch

Clearing flags on attachment: 109342

Committed r96609: <http://trac.webkit.org/changeset/96609>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

Cary sorry for the question: I waded into this code recently and noticed there is a CGImageRef created at line 123 ImageDecoderSkia.cpp in your patch.  Who owns that CGImageRef, and does it need to be released?

[Cary Clark]

Good catch; looks like a leak to me. I assume that this should be 

RetainPtr<CGImageRef> srcImage = ...

(In reply to comment #11)
> Cary sorry for the question: I waded into this code recently and noticed there is a CGImageRef created at line 123 ImageDecoderSkia.cpp in your patch.  Who owns that CGImageRef, and does it need to be released?

[noel gordon]

(In reply to comment #12)
> Good catch; looks like a leak to me. I assume that this should be 
> 
> RetainPtr<CGImageRef> srcImage = ...

I believe that's correct.  How will CGContextDrawImage respond if given a NULL imageRef?  SkCreateCGImageRefWithColorspace() might fail, right?

[noel gordon]

Bug 80892.