Bug 48170 – [Chromium] Add ICC support for PNG on Mac

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

[Adam Barth]

[Chromium] Add ICC support for PNG on Mac

[Adam Barth]

Created attachment 71617 [details]
work in progress

[Adam Barth]

Created attachment 71629 [details]
work in progress

[Adam Barth]

This patch is also required on the Chromium side to enable this feature of libpng:
http://codereview.chromium.org/4060002

[Adam Barth]

Created attachment 72432 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/image-decoders/ImageDecoder.h:50:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 7 files


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

[Early Warning System Bot]

Attachment 72432 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4870043

[WebKit Review Bot]

Attachment 72432 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4763085

[Eric Seidel (no email)]

Comment on attachment 72432 [details]
Patch

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

Overall this looks like a fantastic patch!  I look forward to seeing other platforms support this!

I'd like to see the code moved/my comments answered before landing though.

r- for the below and the build break.

> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:80
> +        RetainPtr<CGDataProviderRef> profileDataProvider(AdoptCF, CGDataProviderCreateWithCFData(profileData.get()));
> +        CGFloat ranges[] = {0.0, 255.0, 0.0, 255.0, 0.0, 255.0};
> +        colorSpace.adoptCF(CGColorSpaceCreateICCBased(3, ranges, profileDataProvider.get(), deviceColorSpace.get()));

If you worked at Apple, you would just add a WKCGColorSpaceCreateWithICCProfile to WebKitSystemInterface.lib (since undoubtably this method existed on Leopard but was just private). Since you don't, I would suggest you create a WKCGColorSpaceCreateWithICCProfile anyway, just make it local to this file, and on SnowLeopard and later use the nice API and have this crazy code only exist on Leopard.

Make sense?

Then an Apple person could move your method into WebKitSystemInterface if desired at some point, but it's likely better to keep it public until we drop Leopard support anyway.

We should consider a deterimineColorSpace() helper method here which just returned the CGColorSpace?  Or maybe this is so clean with WKCGColorSpaceCreateWithICCProfile that we won't need that?

> WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:257
> +    png_get_iCCP(png, info, &profileName, &compression, &profile, &profileLength);

Who owns the returned strings?  Do we need to release them?

Maybe we should put this into a separate function with a nice WebKity API?  One that returns the m_colorProfile or some such?

[Adam Barth]

Created attachment 72490 [details]
Patch

[Adam Barth]

> > WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:257
> > +    png_get_iCCP(png, info, &profileName, &compression, &profile, &profileLength);
> 
> Who owns the returned strings?  Do we need to release them?

They're owned by libpng, that's why we copy them into a vector immediately.  libpng is responsible for deleting them.

> Maybe we should put this into a separate function with a nice WebKity API?  One that returns the m_colorProfile or some such?

The goal of PNGImageDecoder::headerAvailable is to copy data out of the png_infop and into WebKity land.  PNGImageDecoder then makes it available with a more sensible API.

[Adam Barth]

Created attachment 72552 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/image-decoders/ImageDecoder.h:50:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 7 files


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

[Eric Seidel (no email)]

Comment on attachment 72552 [details]
Patch

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

> WebCore/platform/image-decoders/ImageDecoder.h:50
> +    typedef Vector<char> ColorProfile;

Should this just be String?

> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:74
>      DEFINE_STATIC_LOCAL(RetainPtr<CGColorSpaceRef>, deviceColorSpace, (AdoptCF, CGColorSpaceCreateDeviceRGB()));
> +
> +    if (colorProfile.isEmpty()) {
> +        CFRetain(deviceColorSpace.get());
> +        return deviceColorSpace.get();
> +    }

Why bother storing the local?  Do we know that CG doesn't already use a shared instance under the covers?

> WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:260
> +    char* profileName;
> +    int compression;
> +    char* profile;
> +    png_uint_32 profileLength;
> +    png_get_iCCP(png, info, &profileName, &compression, &profile, &profileLength);
> +    m_colorProfile.clear();
> +    if (profile)
> +        m_colorProfile.append(profile, profileLength);

I still think this should be a helper function which just returns a "ColorProfile" (string or otherwise) for better encapsulation.  No need for all those locals to be in the calling function, etc.

[Adam Barth]

Created attachment 72574 [details]
for landing

[Adam Barth]

Committed r71065: <http://trac.webkit.org/changeset/71065>

[WebKit Review Bot]

http://trac.webkit.org/changeset/71065 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
accessibility/aria-activedescendant-crash.html
accessibility/aria-checkbox-text.html
accessibility/aria-hidden-update.html
accessibility/contenteditable-hidden-div.html
accessibility/crash-with-noelement-selectbox.html
accessibility/crashing-a-tag-in-map.html
accessibility/document-attributes.html
accessibility/first-letter-text-transform-causes-crash.html
accessibility/hang-in-isignored.html
accessibility/nested-layout-crash.html
accessibility/nochildren-elements.html
accessibility/non-data-table-cell-title-ui-element.html
accessibility/non-native-image-crash.html
accessibility/radio-button-checkbox-size.html
accessibility/removed-anonymous-block-child-causes-crash.html
accessibility/removed-continuation-element-causes-crash.html
accessibility/table-modification-crash.html
accessibility/table-nofirstbody.html
accessibility/table-notbody.html
accessibility/table-with-empty-thead-causes-crash.html

[Eric Seidel (no email)]

Comment on attachment 72574 [details]
for landing

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

> WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:230
> +    png_get_iCCP(png, info, &profileName, &compressionType, &profile, &profileLength);

Turns out you need to check the return value.  Otherwise your values are garbage.

[Adam Barth]

Created attachment 72608 [details]
now with null check

[Adam Barth]

Committed r71098: <http://trac.webkit.org/changeset/71098>