Apply color profile found to decoded bitmap (Skia on Mac)
Created attachment 109292 [details] Patch
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 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.
Created attachment 109342 [details] Patch
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 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 on attachment 109342 [details] Patch Make sure that corresponding Chromium DEPS has rolled and that there is no layout regression before committing.
Comment on attachment 109342 [details] Patch test expectations have been updated to disable Skia on Mac for now.
Comment on attachment 109342 [details] Patch Clearing flags on attachment: 109342 Committed r96609: <http://trac.webkit.org/changeset/96609>
All reviewed patches have been landed. Closing bug.
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?
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?
(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?
Bug 80892.