Bug 48170

Summary: [Chromium] Add ICC support for PNG on Mac
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, eric, pascal.massimino, pkasting, thakis, webkit-ews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on: 48801    
Bug Blocks: 76804    
Attachments:
Description Flags
work in progress
none
work in progress
none
Patch
none
Patch
none
Patch
none
for landing
none
now with null check none

Description Adam Barth 2010-10-22 18:39:29 PDT
[Chromium] Add ICC support for PNG on Mac
Comment 1 Adam Barth 2010-10-22 18:40:24 PDT
Created attachment 71617 [details]
work in progress
Comment 2 Adam Barth 2010-10-23 01:13:44 PDT
Created attachment 71629 [details]
work in progress
Comment 3 Adam Barth 2010-10-23 01:14:20 PDT
This patch is also required on the Chromium side to enable this feature of libpng:
http://codereview.chromium.org/4060002
Comment 4 Adam Barth 2010-10-30 01:32:41 PDT
Created attachment 72432 [details]
Patch
Comment 5 WebKit Review Bot 2010-10-30 01:35:29 PDT
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.
Comment 6 Early Warning System Bot 2010-10-30 01:41:23 PDT
Attachment 72432 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4870043
Comment 7 WebKit Review Bot 2010-10-30 12:34:05 PDT
Attachment 72432 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4763085
Comment 8 Eric Seidel (no email) 2010-10-31 23:20:54 PDT
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?
Comment 9 Adam Barth 2010-11-01 01:32:12 PDT
Created attachment 72490 [details]
Patch
Comment 10 Adam Barth 2010-11-01 13:50:27 PDT
> > 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.
Comment 11 Adam Barth 2010-11-01 13:51:19 PDT
Created attachment 72552 [details]
Patch
Comment 12 WebKit Review Bot 2010-11-01 13:53:14 PDT
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.
Comment 13 Eric Seidel (no email) 2010-11-01 15:07:13 PDT
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.
Comment 14 Adam Barth 2010-11-01 15:28:07 PDT
Created attachment 72574 [details]
for landing
Comment 15 Adam Barth 2010-11-01 15:34:05 PDT
Committed r71065: <http://trac.webkit.org/changeset/71065>
Comment 16 WebKit Review Bot 2010-11-01 16:04:52 PDT
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
Comment 17 Eric Seidel (no email) 2010-11-01 16:27:54 PDT
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.
Comment 18 Adam Barth 2010-11-01 17:35:26 PDT
Created attachment 72608 [details]
now with null check
Comment 19 Adam Barth 2010-11-01 21:16:15 PDT
Committed r71098: <http://trac.webkit.org/changeset/71098>