Bug 47974 – [chromium] Chromium Mac should use WebKit's image decoders

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

[Adam Barth]

[chromium] Chromium Mac should use WebKit's image decoders

[Adam Barth]

Created attachment 71261 [details]
work-in-progres

[Adam Barth]

Turns out this is mostly donking around with the build system.  It looks like the main thing I have to do is implement ImageDecoderCG, which is the adapter between the image decoder output and the native bitmap.  I'll have to find some documentation about how to actually use CoreGraphics.  :)

[Adam Barth]

Created attachment 71329 [details]
builds and passes at least one test

[Adam Barth]

Created attachment 71343 [details]
looks right in test_shell

[Adam Barth]

Created attachment 71379 [details]
seems to work

[Pascal Massimino]

(In reply to comment #5)
> Created an attachment (id=71379) [details]
> seems to work

review tool is acting strange to me, so i copy-paste the comments here:



View in context: https://bugs.webkit.org/attachment.cgi?id=71379&action=review
> WebCore/platform/image-decoders/ImageDecoder.cpp:142
> +    memset(m_bytes, 0, m_size.width() * m_size.height() * sizeof(PixelData));

i'm not familiar with m_bytes, but this looks strange. Shouldn't it be something like: memset(m_bytes.data(), 0, ...) ?

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:109
> +        return True

probably not final, right?

[Adam Barth]

> View in context: https://bugs.webkit.org/attachment.cgi?id=71379&action=review
> > WebCore/platform/image-decoders/ImageDecoder.cpp:142
> > +    memset(m_bytes, 0, m_size.width() * m_size.height() * sizeof(PixelData));
> 
> i'm not familiar with m_bytes, but this looks strange. Shouldn't it be something like: memset(m_bytes.data(), 0, ...) ?

m_bytes is now just a raw data so we can write through to the buffer quickly.

> > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:109
> > +        return True
> 
> probably not final, right?

Nope.  I just don't have wdiff installed and this check was a PITA for me.

[Pascal Massimino]

(In reply to comment #5)
> Created an attachment (id=71379) [details]
> seems to work

review tool is acting strange to me, so i copy-paste the comments here:



View in context: https://bugs.webkit.org/attachment.cgi?id=71379&action=review
> WebCore/platform/image-decoders/ImageDecoder.cpp:142
> +    memset(m_bytes, 0, m_size.width() * m_size.height() * sizeof(PixelData));

i'm not familiar with m_bytes, but this looks strange. Shouldn't it be something like: memset(m_bytes.data(), 0, ...) ?

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:109
> +        return True

probably not final, right?(In reply to comment #7)
> > View in context: https://bugs.webkit.org/attachment.cgi?id=71379&action=review
> > > WebCore/platform/image-decoders/ImageDecoder.cpp:142
> > > +    memset(m_bytes, 0, m_size.width() * m_size.height() * sizeof(PixelData));
> > 
> > i'm not familiar with m_bytes, but this looks strange. Shouldn't it be something like: memset(m_bytes.data(), 0, ...) ?
> 
> m_bytes is now just a raw data so we can write through to the buffer quickly.
> 

oh, i see. then, wouldn't it be safer to replace
   m_size.width() * m_size.height() * sizeof(PixelData)
by backingStore.size() (or equivalent) ?

> > > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:109
> > > +        return True
> > 
> > probably not final, right?
> 
> Nope.  I just don't have wdiff installed and this check was a PITA for me.

oh, ok

[Adam Barth]

The color profile thing looks pretty easy too.  I'll probably do it in a separate patch though.  We'll need to use this code:

http://trac.imagemagick.org/browser/lcms/trunk/jpegicc/iccjpeg.h
http://trac.imagemagick.org/browser/lcms/trunk/jpegicc/iccjpeg.c

I'm trying to figure out what license it has.

[Adam Barth]

Created attachment 71397 [details]
Patch

[Adam Barth]

AFACT, this patch works.  Of course, it doesn't have color profile support.  I looked into that and it shouldn't be that hard.  One approach is to review this patch and the color profile patch and then land them together.

[Peter Kasting]

Let's explicitly not worry about color profiles in the first patch, and do that second.

[Peter Kasting]

Comment on attachment 71397 [details]
Patch

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

> WebCore/platform/image-decoders/ImageDecoder.h:186
> +        PixelData* m_bytes;   // The memory is backed by m_backingStore.

Do you need to change any of the other ImageDecoderXXX.cpp files due to this modification?

> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:66
> +        alphaInfo | kCGBitmapByteOrder32Host, dataProvider.get(), 0, false, kCGRenderingIntentDefault);

What! Line-wrapping?!

[Adam Barth]

Comment on attachment 71397 [details]
Patch

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

>> WebCore/platform/image-decoders/ImageDecoder.h:186
>> +        PixelData* m_bytes;   // The memory is backed by m_backingStore.
> 
> Do you need to change any of the other ImageDecoderXXX.cpp files due to this modification?

Yep!

>> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:66
>> +    return CGImageCreate(width(), height(), 8, 32, width() * sizeof(PixelData), colorSpace.get(),
>> +        alphaInfo | kCGBitmapByteOrder32Host, dataProvider.get(), 0, false, kCGRenderingIntentDefault);
> 
> What! Line-wrapping?!

Crazy, I know.

[Adam Barth]

Created attachment 71398 [details]
Patch

[Peter Kasting]

Comment on attachment 71398 [details]
Patch

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

I don't know CG well enough to review ImageDecoderCG.cpp, but the rest of the changes look fine to me.

> WebCore/WebCore.gyp/WebCore.gyp:-1184
> -            ['exclude', 'platform/image-decoders/xbm/XBMImageDecoder\\.(cpp|h)$'],

Thanks for getting rid of this!

> WebCore/platform/image-decoders/ImageDecoder.h:142
> +#else

Nit: For clarity, might want to not define this typedef at all for QT and Skia.

[Adam Barth]

Comment on attachment 71398 [details]
Patch

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

>> WebCore/platform/image-decoders/ImageDecoder.h:142
>> +#if PLATFORM(CF)
>> +        typedef RetainPtr<CFMutableDataRef> NativeBackingStore;
>> +#else
> 
> Nit: For clarity, might want to not define this typedef at all for QT and Skia.

Okiedokes.

[Adam Barth]

+eric for CG knowledge.

[Peter Kasting]

Eric: for context, the intended memory model of RGBA32Buffer is:

* Most frames should call setSize() exactly once to allocate the memory they need for a framebuffer of that size.
* Standard copying (operator=) should just result in an additional ref of the underlying pixel data.  copyBitmapData() is for when a deep copy is needed; it should drop any ref to existing framebuffer data and make a complete copy of the framebuffer in the provided object.
* asNewNativeImage should return a NativeImagePtr that holds a ref to the framebuffer data.  It does not need to copy the data, just ref it.

[Adam Barth]

> * Standard copying (operator=) should just result in an additional ref of the underlying pixel data.  copyBitmapData() is for when a deep copy is needed; it should drop any ref to existing framebuffer data and make a complete copy of the framebuffer in the provided object.

This patch does a deep copy for operator=.  I can change this to just a ref if that's important, it just costs #ifdefs.

[Peter Kasting]

(In reply to comment #20)
> This patch does a deep copy for operator=.

For performance reasons, you should probably change it.

I would make sure you've done two things:

* Read all comments in ImageDecoder.h.  For example, that notes that operator=() can just ref and not have to copy.
* Read ImageDecoderSkia.cpp carefully and understand why it has overridden every function, and what they do.  The memory/lifetime/refcount picture on Skia should be identical (or very close) to that on CG.  I think you've already gotten most of the important bits.

[Adam Barth]

> For performance reasons, you should probably change it.

Ok.

> I would make sure you've done two things:
> 
> * Read all comments in ImageDecoder.h.  For example, that notes that operator=() can just ref and not have to copy.
> * Read ImageDecoderSkia.cpp carefully and understand why it has overridden every function, and what they do.  The memory/lifetime/refcount picture on Skia should be identical (or very close) to that on CG.  I think you've already gotten most of the important bits.

Yeah, I did all of that.  We don't need to override as many of the functions because CG lets us disentangle ownership of the backing store from the image object.

[Peter Kasting]

(In reply to comment #22)
> Yeah, I did all of that.  We don't need to override as many of the functions because CG lets us disentangle ownership of the backing store from the image object.

Yep, that's what it looked like to me too.  I think the operator=() thing is the only case that needed changing.

[Adam Barth]

> Yep, that's what it looked like to me too.  I think the operator=() thing is the only case that needed changing.

I'll make that change tomorrow.

[Adam Barth]

Created attachment 71588 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/image-decoders/ImageDecoder.h:190:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1 in 14 files


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

[Adam Barth]

Created attachment 71591 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/image-decoders/ImageDecoder.h:190:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1 in 14 files


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

[Eric Seidel (no email)]

Comment on attachment 71591 [details]
Patch

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

> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:38
> +    m_backingStore = other.m_backingStore;
> +    m_bytes = reinterpret_cast<PixelData*>(CFDataGetMutableBytePtr(m_backingStore.get()));

I suspect this method might cause a copy.

> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:40
> +    // FIXME: The rest of this function seems redundant with RGBA32Buffer::copyBitmapData.
> +    m_size = other.m_size;

It seems very strange to me that these 3 are separate members.  Why does this need to be the case?

> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:60
> +    m_bytes = reinterpret_cast<PixelData*>(CFDataGetMutableBytePtr(m_backingStore.get()));

Why do you need mutable pointers?

> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:70
> +    RetainPtr<CGColorSpaceRef> colorSpace(AdoptCF, CGColorSpaceCreateWithName(kCGColorSpaceGenericRGB));

I think you want DeviceRGB, not GenericRGB.  Generic will still to color-matching for you.  You want Device color spaces which do not do color matching as they're already in the colorspace of the device.

> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:76
> +    return CGImageCreate(width(), height(), 8, 32, width() * sizeof(PixelData), colorSpace.get(),
> +        alphaInfo | kCGBitmapByteOrder32Host, dataProvider.get(), 0, false, kCGRenderingIntentDefault);

Hmmm... My data my be old here, but I would have expected you to use CGBitmapImageCreate here.

[Adam Barth]

Comment on attachment 71591 [details]
Patch

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

>> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:38
>> +    m_bytes = reinterpret_cast<PixelData*>(CFDataGetMutableBytePtr(m_backingStore.get()));
> 
> I suspect this method might cause a copy.

CFDataGetMutableBytePtr ?  The documentation says that's the way to write data into the CFMutatableData object.  I'm not sure how you'd be able to write the underly memory if this made a copy.

>> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:40
>> +    m_size = other.m_size;
> 
> It seems very strange to me that these 3 are separate members.  Why does this need to be the case?

m_size is the width / height of the buffer in pixels.  m_backingStore knows the size of the buffer in bytes, but not what its geometry.  m_bytes is just a cached pointer to the underlying memory because we need to read it for every pixel write, which is extremely hot.  We don't want to go through CF for every pixel.

>> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:60
>> +    m_bytes = reinterpret_cast<PixelData*>(CFDataGetMutableBytePtr(m_backingStore.get()));
> 
> Why do you need mutable pointers?

We need a mutable point because setRGBA writes to the memory.  We use CF to allocate the memory so that CGImage can hold onto it.  However, after allocating the memory, we write to directly.

>> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:70
>> +    // FIXME: Figure out the right color space.
>> +    RetainPtr<CGColorSpaceRef> colorSpace(AdoptCF, CGColorSpaceCreateWithName(kCGColorSpaceGenericRGB));
> 
> I think you want DeviceRGB, not GenericRGB.  Generic will still to color-matching for you.  You want Device color spaces which do not do color matching as they're already in the colorspace of the device.

I'll research this, but let me try to confirm my understanding:

1) We have a bunch of bytes in an image file.
2) We have a device that transforms bytes in a given color space to a bunch of photons in a room.

What I'd like to say on this line of code is that the bytes in my image file are expressed as coordinates in a particular colorspace, the moral equivalent of sRGB on Windows.  Now, of course, that's probably a lie, but that's the lie I want to tell.

I'm fine with the operating system transforming the sRGB color space to a device specific color space (e.g., that the user has created to calibrate the photons emitted by their monitor).  My initial reading of the documentation was that DeviceRGB is a device-specific color space that's after this transformation.  Does that differ from your understanding?  (In either case, I'll go back an read some more.)

>> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:76
>> +    return CGImageCreate(width(), height(), 8, 32, width() * sizeof(PixelData), colorSpace.get(),
>> +        alphaInfo | kCGBitmapByteOrder32Host, dataProvider.get(), 0, false, kCGRenderingIntentDefault);
> 
> Hmmm... My data my be old here, but I would have expected you to use CGBitmapImageCreate here.

I can't find a function by that name.  I'm looking at Table 11-1 of http://developer.apple.com/library/mac/#documentation/GraphicsImaging/Conceptual/drawingwithquartz2d/dq_images/dq_images.html#//apple_ref/doc/uid/TP30001066-CH212-TPXREF101

Do you mean CGBitmapContextCreateImage?  That seems a bit different: "Creates an image by copying the bits from a bitmap graphics context."  In particular, we don't want the copy.

[Adam Barth]

Created attachment 71598 [details]
Patch

[Adam Barth]

> >> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:70
> >> +    // FIXME: Figure out the right color space.
> >> +    RetainPtr<CGColorSpaceRef> colorSpace(AdoptCF, CGColorSpaceCreateWithName(kCGColorSpaceGenericRGB));
> > 
> > I think you want DeviceRGB, not GenericRGB.  Generic will still to color-matching for you.  You want Device color spaces which do not do color matching as they're already in the colorspace of the device.
> 
> I'll research this, but let me try to confirm my understanding:
> 
> 1) We have a bunch of bytes in an image file.
> 2) We have a device that transforms bytes in a given color space to a bunch of photons in a room.
> 
> What I'd like to say on this line of code is that the bytes in my image file are expressed as coordinates in a particular colorspace, the moral equivalent of sRGB on Windows.  Now, of course, that's probably a lie, but that's the lie I want to tell.
> 
> I'm fine with the operating system transforming the sRGB color space to a device specific color space (e.g., that the user has created to calibrate the photons emitted by their monitor).  My initial reading of the documentation was that DeviceRGB is a device-specific color space that's after this transformation.  Does that differ from your understanding?  (In either case, I'll go back an read some more.)

Ok.  I've looked into this, both in the documentation in elsewhere in WebKit.  My understanding was a bit simplistic.  There's apparently a third layer hiding below.  WebKit generally looks to use CGColorSpaceCreateDeviceRGB(), which seems to have changed behavior after 10.4 to be the same as kCGColorSpaceGenericRGB, which is different than kCGColorSpaceSRGB.  I've updated the patch to use the same idiom as the example code I saw in WebKit.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/image-decoders/ImageDecoder.h:190:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1 in 14 files


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

[Eric Seidel (no email)]

Comment on attachment 71598 [details]
Patch

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

Seems you have some style errors.  I won't be able to r+ this because I feel this is a policy decision for the chromium port to make.

> WebCore/platform/image-decoders/ImageDecoder.h:92
> +        void copyReferenceToBitmapData(const RGBA32Buffer& other);

"other" not needed.

> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:70
> +    RetainPtr<CGColorSpaceRef> colorSpace(AdoptCF, CGColorSpaceCreateDeviceRGB());

Seems this could be static, we don't need a new colorspace for each image.

[Adam Barth]

Created attachment 71602 [details]
updated with style fixes

[Adam Barth]

Committed r70369: <http://trac.webkit.org/changeset/70369>

[Dimitri Glazkov (Google)]

(In reply to comment #36)
> Committed r70369: <http://trac.webkit.org/changeset/70369>

We might have to roll this one out and let it cook a bit more. I started looking at the expectation changes, and there are new artifacts on the page that weren't there before, like:

http://build.chromium.org/buildbot/layout_test_results/webkit-rel-mac-webkit-org/results/layout-test-results/fast/inline-block/001-actual.png

vs.

http://trac.webkit.org/export/70391/trunk/LayoutTests/platform/mac/fast/inline-block/001-expected.png

See it here: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Finline-block%2F001.html&showExpectations=true&useWebKitCanary=true 

I am sure it's just some mis-interaction of the pixel dump with the new decoders, but until we're sure, we shouldn't be rolling this into Chromium.

[Adam Barth]

Do we have those problems on win or linux?

[Dimitri Glazkov (Google)]

(In reply to comment #38)
> Do we have those problems on win or linux?

Nope.

[Dimitri Glazkov (Google)]

Reverted r70369 for reason:

Caused weird artifacts in expected results.

Committed r70394: <http://trac.webkit.org/changeset/70394>

[Adam Barth]

I regenerated new pixel results on my machine and looked through all 2500 image mismatches (this includes Leopard / Snow Leopard diff and the diffs from this patch).

I can't reproduce that artifact.  With the patch we fail these tests:

fast/css/color-correction-on-backgrounds.html
fast/css/color-correction.html
fast/image/pdf-as-background.html
svg/W3C-SVG-1.1/struct-image-03-t.svg

which seems expected b/c we don't have color correction yet.  I certainly could have missed a needle in the haystack, however.

[Adam Barth]

AFACT, this patch is ready to land.  However, I can't even find the chromium canary bots on the FYI waterfall, much less see if they are green.

[Peter Kasting]

(In reply to comment #42)
> AFACT, this patch is ready to land.  However, I can't even find the chromium canary bots on the FYI waterfall, much less see if they are green.

See the thread "Changes to the chromium buildbots and build.chromium.org" on chromium-dev.

The canaries now have a dedicated waterfall at http://build.chromium.org/p/chromium.webkit/waterfall .

[Adam Barth]

> The canaries now have a dedicated waterfall at http://build.chromium.org/p/chromium.webkit/waterfall .

Cool.  That's much nicer than the tinyurl I had been using.

[Adam Barth]

Committed r70846: <http://trac.webkit.org/changeset/70846>

[Adam Barth]

Committed r70858: <http://trac.webkit.org/changeset/70858>