Bug 69144 - Apply color profile found to decoded bitmap (Skia on Mac)
Summary: Apply color profile found to decoded bitmap (Skia on Mac)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Cary Clark
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-30 06:23 PDT by Cary Clark
Modified: 2012-03-12 16:39 PDT (History)
4 users (show)

See Also:


Attachments
Patch (4.37 KB, patch)
2011-09-30 06:43 PDT, Cary Clark
no flags Details | Formatted Diff | Diff
Patch (4.43 KB, patch)
2011-09-30 14:08 PDT, Cary Clark
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cary Clark 2011-09-30 06:23:27 PDT
Apply color profile found to decoded bitmap (Skia on Mac)
Comment 1 Cary Clark 2011-09-30 06:43:16 PDT
Created attachment 109292 [details]
Patch
Comment 2 Stephen White 2011-09-30 11:45:14 PDT
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.
Comment 3 Adam Barth 2011-09-30 11:59:05 PDT
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.
Comment 4 Cary Clark 2011-09-30 14:08:01 PDT
Created attachment 109342 [details]
Patch
Comment 5 Cary Clark 2011-09-30 14:09:41 PDT
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.
Comment 6 Stephen White 2011-09-30 14:40:08 PDT
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
Comment 7 Cary Clark 2011-10-03 07:52:25 PDT
Comment on attachment 109342 [details]
Patch

Make sure that corresponding Chromium DEPS has rolled and that there is no layout regression before committing.
Comment 8 Cary Clark 2011-10-04 10:18:23 PDT
Comment on attachment 109342 [details]
Patch

test expectations have been updated to disable Skia on Mac for now.
Comment 9 WebKit Review Bot 2011-10-04 10:48:53 PDT
Comment on attachment 109342 [details]
Patch

Clearing flags on attachment: 109342

Committed r96609: <http://trac.webkit.org/changeset/96609>
Comment 10 WebKit Review Bot 2011-10-04 10:48:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 noel gordon 2012-01-18 16:42:42 PST
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?
Comment 12 Cary Clark 2012-01-19 06:44:05 PST
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?
Comment 13 noel gordon 2012-01-22 22:15:36 PST
(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?
Comment 14 noel gordon 2012-03-12 16:39:26 PDT
Bug 80892.