Bug 171042 – [CG] Provide a type identifier hint to the CGImageSource so getting the type identifier is more accurate

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

The image URL can be used to get the type identifier hint. Without providing this hint, the image type identifier is not accurate for image formats.

[Said Abou-Hallawa]

Created attachment 307557 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/31725669>

[Said Abou-Hallawa]

Created attachment 307572 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 307572 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:51
> +CFStringRef CGImageSourceGetTypeWithURL(CFURLRef, CFStringRef);

This function should not be declared in the .cpp file. Please use an actual *SPI.h header, similar to WebCore/platform/spi/cg/CoreGraphicsSPI.h. I don't think that we have ImageIOSPI.h yet - maybe we didn't need it before, or maybe those went into some other header.

Furthermore, this pattern is not quite correct. We want to redeclare functions when APPLE_INTERNAL_SDK is true - that way, the compiler has a chance to detect mistakes and changes in signatures.

Ditto for kCGImageSourceSubsampleFactor/kCGImageSourceShouldCacheImmediately. These are exported from ImageIO.framework, so we should be using the actual exported symbol even in open source builds, not create a new constant with the same name.

[Said Abou-Hallawa]

Created attachment 307598 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 307598 [details]
Patch

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

> Source/WebCore/ChangeLog:80
> +        * platform/spi/cg/ImageIOSPI.h: Added.

Not sure if this goes to cocoa/ or to cg/.

> Source/WebCore/platform/spi/cg/ImageIOSPI.h:35
> +extern const CFStringRef kCGImageSourceSubsampleFactor;
> +extern const CFStringRef kCGImageSourceShouldCacheImmediately;

I think that constants can and should be redeclared, too.

[Said Abou-Hallawa]

Created attachment 307604 [details]
Patch

[Said Abou-Hallawa]

Created attachment 307609 [details]
Patch

[Build Bot]

Comment on attachment 307609 [details]
Patch

Attachment 307609 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3571451

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 307622 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307609 [details]
Patch

Attachment 307609 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3571470

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 307624 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307609 [details]
Patch

Attachment 307609 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3571585

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 307625 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307609 [details]
Patch

Attachment 307609 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3571526

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 307631 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Said Abou-Hallawa]

Created attachment 307669 [details]
Patch

[Said Abou-Hallawa]

Created attachment 307671 [details]
Patch

[Build Bot]

Comment on attachment 307671 [details]
Patch

Attachment 307671 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3573462

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 307676 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307671 [details]
Patch

Attachment 307671 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3573464

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 307678 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307671 [details]
Patch

Attachment 307671 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3573492

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 307680 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307671 [details]
Patch

Attachment 307671 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3573475

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 307683 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Said Abou-Hallawa]

Created attachment 307743 [details]
Patch

[Build Bot]

Comment on attachment 307743 [details]
Patch

Attachment 307743 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3577069

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 307747 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Said Abou-Hallawa]

Created attachment 307749 [details]
Patch

[Build Bot]

Comment on attachment 307749 [details]
Patch

Attachment 307749 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3577349

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 307762 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307749 [details]
Patch

Attachment 307749 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3577373

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 307763 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307749 [details]
Patch

Attachment 307749 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3577368

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 307765 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Said Abou-Hallawa]

Created attachment 307773 [details]
Patch

[Build Bot]

Comment on attachment 307773 [details]
Patch

Attachment 307773 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3578125

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 307792 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307773 [details]
Patch

Attachment 307773 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3578039

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 307796 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307773 [details]
Patch

Attachment 307773 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3578202

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 307798 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307773 [details]
Patch

Attachment 307773 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3578191

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 307799 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Said Abou-Hallawa]

Created attachment 307814 [details]
Patch

[Said Abou-Hallawa]

Created attachment 307840 [details]
Patch

[Said Abou-Hallawa]

Created attachment 307844 [details]
Patch

[Said Abou-Hallawa]

Created attachment 307990 [details]
Patch

[Said Abou-Hallawa]

Created attachment 307992 [details]
Patch

[Tim Horton]

Comment on attachment 307990 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:162
> +        RetainPtr<CFMutableDictionaryRef> options = adoptCF(CFDictionaryCreateMutable(nullptr, 0, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));

Why Mutable? Should be able to just use CFDictionaryCreate with your one value.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:242
> +EncodedDataStatus PNGImageDecoder::encodedDataStatus() const

I don't think all this consting and const_casting is good. Can you just make the new thing mutable instead?

[Said Abou-Hallawa]

Created attachment 308006 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 307990 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:162
>> +        RetainPtr<CFMutableDictionaryRef> options = adoptCF(CFDictionaryCreateMutable(nullptr, 0, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));
> 
> Why Mutable? Should be able to just use CFDictionaryCreate with your one value.

Done.

>> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:242
>> +EncodedDataStatus PNGImageDecoder::encodedDataStatus() const
> 
> I don't think all this consting and const_casting is good. Can you just make the new thing mutable instead?

I agree that this looks ugly but I think it is okay since the const here is a logical const. I think the best fix is to move the call to decode() into ImageDecoder::setData() and make encodedDataStatus() just returns a member of the ImageDecoder class. Please see my comment https://bugs.webkit.org/show_bug.cgi?id=170730#c10. If Miguel takes this approach, things will be fixed the way we want. If not I am going to file a follow up bug to remove this ugliness.

[Tim Horton]

Comment on attachment 308006 [details]
Patch

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

> Source/WebCore/platform/graphics/Image.cpp:80
> -String Image::sourceURL() const
> +URL Image::sourceURL() const

If it were me, I would do the mechanical change in a separate patch, but this is fine too.

> Source/WebCore/platform/graphics/Image.h:124
> +    virtual String filenameExtension() const { return emptyString(); } // null string if unknown

The comment is no longer true. Empty string != null string. That change seems potentially problematic for callers, too.

> Source/WebCore/platform/image-decoders/ImageDecoder.h:44
> +class URL;

This file is indented wrong, but you should probably stick with it instead of doing your own thing.

[Said Abou-Hallawa]

Created attachment 308025 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 308025 [details]
Patch

Rejecting attachment 308025 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 308025, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ecoder.h
patching file Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp
patching file Source/WebCore/platform/image-decoders/png/PNGImageDecoder.h
patching file Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp
patching file Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.h
patching file Source/WebCore/platform/spi/cg/ImageIOSPI.h

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/3597784

[Said Abou-Hallawa]

Created attachment 308029 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 308029 [details]
Patch

Clearing flags on attachment: 308029

Committed r215710: <http://trac.webkit.org/changeset/215710>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Comment on attachment 308029 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:159
> +    RetainPtr<CFURLRef> url = sourceURL.createCFURL();
> +    RetainPtr<CFStringRef> utiHint = adoptCF(CGImageSourceGetTypeWithURL(url.get(), nullptr));

What does this do with (potentially huge) data URIs?

[Said Abou-Hallawa]

Comment on attachment 308029 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:159
>> +    RetainPtr<CFStringRef> utiHint = adoptCF(CGImageSourceGetTypeWithURL(url.get(), nullptr));
> 
> What does this do with (potentially huge) data URIs?

This code has changed in <http://trac.webkit.org/changeset/217246>. The URL is not used anymore to get the type hint. Instead we get the hint from the data and we call CGImageSourceGetTypeWithData() instead of calling CGImageSourceGetTypeWithURL().