Bug 197384 – [CG] Add support for HEIF-sequence (‘public.heics’) images

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

We need to add support for these images in two places:

1) ImageDecoderCG::repetitionCount(): Since HEICS images do not support loopCount, we will be returning RepetitionCountInfinite.
2) ImageDecoderCG::frameDurationAtIndex(): We need to read the values of the following dictionary keys for the frame duration kCGImagePropertyHEICSUnclampedDelayTime and kCGImagePropertyHEICSDelayTime.

[Said Abou-Hallawa]

Created attachment 368485 [details]
Patch

[Said Abou-Hallawa]

<rdar://problem/48781098>

[Said Abou-Hallawa]

Created attachment 368632 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 368632 [details]
Patch

Can we add layout tests for this?

[Said Abou-Hallawa]

Created attachment 369055 [details]
Patch

[Said Abou-Hallawa]

Created attachment 369059 [details]
Patch

[EWS Watchlist]

Comment on attachment 369059 [details]
Patch

Attachment 369059 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12097566

New failing tests:
fast/images/animated-heics.html

[EWS Watchlist]

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

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

[EWS Watchlist]

Comment on attachment 369059 [details]
Patch

Attachment 369059 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12098014

New failing tests:
svg/repaint/change-background-color.html

[EWS Watchlist]

Created attachment 369067 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Simon Fraser (smfr)]

Comment on attachment 369059 [details]
Patch

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

r- because I don't think this is correctly resetting state between tests.

> Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:81
>  void setAdditionalSupportedImageTypes(const Vector<String>& imageTypes)

This seem more like appendAdditionSupportedImageTypes, since you can't unregister a type, right?

That also implies that you're never cleaning up this state between tests correctly.

> Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:94
> +void setAdditionalSupportedImageTypes(const String& imageTypes)

If this is only used for testing, maybe append "ForTesting" to the function name.

[Said Abou-Hallawa]

Created attachment 369128 [details]
Patch

[Sam Weinig]

Comment on attachment 369128 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:50
> +const CFStringRef WebCoreCGImagePropertyHEICSUnclampedDelayTime = CFSTR("UnclampedDelayTime");

This is unused.

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:367
> +            if (CFNumberRef num = (CFNumberRef)CFDictionaryGetValue(heicsProperties, WebCoreCGImagePropertyHEICSDictionary))

Is the property name for the number value really also WebCoreCGImagePropertyHEICSDictionary? It seems like maybe this was supposed to be WebCoreCGImagePropertyHEICSUnclampedDelayTime?

[Said Abou-Hallawa]

Created attachment 369136 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 369059 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:81
>>  void setAdditionalSupportedImageTypes(const Vector<String>& imageTypes)
> 
> This seem more like appendAdditionSupportedImageTypes, since you can't unregister a type, right?
> 
> That also implies that you're never cleaning up this state between tests correctly.

For DRT and WTR, this function is now called twice. It is called first for the TestOptions then it is called when WebView or WebPage is created. So it is set correctly the first time but it is cleared the second time. I did it this way to prevent clearing it from creating the WebView and the WebPage. 

But you are right, this fix will not clean the state between tests correctly. I think the solution is to move the condition imageTypes.isEmpty() to WebView and WebPage creation functions.

>> Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:94
>> +void setAdditionalSupportedImageTypes(const String& imageTypes)
> 
> If this is only used for testing, maybe append "ForTesting" to the function name.

Done.

[Said Abou-Hallawa]

Comment on attachment 369128 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:367
>> +            if (CFNumberRef num = (CFNumberRef)CFDictionaryGetValue(heicsProperties, WebCoreCGImagePropertyHEICSDictionary))
> 
> Is the property name for the number value really also WebCoreCGImagePropertyHEICSDictionary? It seems like maybe this was supposed to be WebCoreCGImagePropertyHEICSUnclampedDelayTime?

Fixed. Thanks for catching this error.

[Sam Weinig]

(In reply to Said Abou-Hallawa from comment #16)
> Comment on attachment 369128 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369128&action=review
> 
> >> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:367
> >> +            if (CFNumberRef num = (CFNumberRef)CFDictionaryGetValue(heicsProperties, WebCoreCGImagePropertyHEICSDictionary))
> > 
> > Is the property name for the number value really also WebCoreCGImagePropertyHEICSDictionary? It seems like maybe this was supposed to be WebCoreCGImagePropertyHEICSUnclampedDelayTime?
> 
> Fixed. Thanks for catching this error.

Please add a test which exercise this path.

[Simon Fraser (smfr)]

Comment on attachment 369136 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:642
> +    if (!parameters.additionalSupportedImageTypes.isEmpty())
> +        WebCore::setAdditionalSupportedImageTypes(parameters.additionalSupportedImageTypes);

Why the isEmpty check here? Is this making up for the lack of an appendAdditionalSupportedImageTypes?

> Source/WebKitLegacy/mac/WebView/WebView.mm:2886
> +    if ([[preferences additionalSupportedImageTypes] count])
> +        WebCore::setAdditionalSupportedImageTypes(WebCore::webCoreStringVectorFromNSStringArray([preferences additionalSupportedImageTypes]));

Why the count check here?

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h:173
>      bool booleanForKey(WKDictionaryRef, const char* key);
> +    String stringForKey(WKDictionaryRef, const char* key);

These can be static, I think?

[Said Abou-Hallawa]

Created attachment 369169 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 369136 [details]
Patch

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

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:642
>> +        WebCore::setAdditionalSupportedImageTypes(parameters.additionalSupportedImageTypes);
> 
> Why the isEmpty check here? Is this making up for the lack of an appendAdditionalSupportedImageTypes?

Fixed. The isEmpty() check was removed.

I thought the TestOptions will parsed first. Then [WKWebView _initializeWithConfiguration] will be called to create the WTR WKWebView. It turned out I was wrong and [WKWebView _initializeWithConfiguration] is not called in the WTR case.

>> Source/WebKitLegacy/mac/WebView/WebView.mm:2886
>> +        WebCore::setAdditionalSupportedImageTypes(WebCore::webCoreStringVectorFromNSStringArray([preferences additionalSupportedImageTypes]));
> 
> Why the count check here?

Fixed. the count check was removed.

>> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h:173
>> +    String stringForKey(WKDictionaryRef, const char* key);
> 
> These can be static, I think?

No. They can't be static nor can they even be const. The reason is these functions call InjectedBundle::outputText() which is not static or const.

[Said Abou-Hallawa]

(In reply to Sam Weinig from comment #17)
> (In reply to Said Abou-Hallawa from comment #16)
> > Comment on attachment 369128 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=369128&action=review
> > 
> > >> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:367
> > >> +            if (CFNumberRef num = (CFNumberRef)CFDictionaryGetValue(heicsProperties, WebCoreCGImagePropertyHEICSDictionary))
> > > 
> > > Is the property name for the number value really also WebCoreCGImagePropertyHEICSDictionary? It seems like maybe this was supposed to be WebCoreCGImagePropertyHEICSUnclampedDelayTime?
> > 
> > Fixed. Thanks for catching this error.
> 
> Please add a test which exercise this path.

I do not know any tool which can edit/create a HEICS image.

[EWS Watchlist]

Comment on attachment 369169 [details]
Patch

Attachment 369169 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12116421

New failing tests:
legacy-animation-engine/compositing/reflections/load-video-in-reflection.html

[EWS Watchlist]

Created attachment 369205 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Sam Weinig]

(In reply to Said Abou-Hallawa from comment #21)
> (In reply to Sam Weinig from comment #17)
> > (In reply to Said Abou-Hallawa from comment #16)
> > > Comment on attachment 369128 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=369128&action=review
> > > 
> > > >> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:367
> > > >> +            if (CFNumberRef num = (CFNumberRef)CFDictionaryGetValue(heicsProperties, WebCoreCGImagePropertyHEICSDictionary))
> > > > 
> > > > Is the property name for the number value really also WebCoreCGImagePropertyHEICSDictionary? It seems like maybe this was supposed to be WebCoreCGImagePropertyHEICSUnclampedDelayTime?
> > > 
> > > Fixed. Thanks for catching this error.
> > 
> > Please add a test which exercise this path.
> 
> I do not know any tool which can edit/create a HEICS image.

Can you ask the ImageIO folks for one?

[Said Abou-Hallawa]

Created attachment 369524 [details]
Patch

[Said Abou-Hallawa]

Created attachment 369538 [details]
Patch

[Said Abou-Hallawa]

Created attachment 369556 [details]
Patch

[Said Abou-Hallawa]

I cleaned ImageDecoderCG::repetitionCount() and ImageDecoderCG::frameDurationAtIndex() by deleting the repeating code and avoiding unnecessary calls to the image properties dictionary.

[Simon Fraser (smfr)]

Comment on attachment 369556 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:340
> +    if (properties)
> +        frameProperties = (CFDictionaryRef)CFDictionaryGetValue(properties.get(), kCGImagePropertyGIFDictionary);
> +    
> +    if (properties && !frameProperties)
> +        frameProperties = (CFDictionaryRef)CFDictionaryGetValue(properties.get(), kCGImagePropertyPNGDictionary);
> +

This looks like the code above. Maybe share into a small static function.

[Said Abou-Hallawa]

Created attachment 369819 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 369819 [details]
Patch

Clearing flags on attachment: 369819

Committed r245280: <https://trac.webkit.org/changeset/245280>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

The tests, which were submitted with this patch, have to be skipped in WebKit for now.

Committed r245292: <https://trac.webkit.org/changeset/245292>