Bug 197384

Summary: [CG] Add support for HEIF-sequence (‘public.heics’) images
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: ImagesAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, rniwa, sam, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews107 for mac-highsierra-wk2
none
Archive of layout-test-results from ews213 for win-future
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews213 for win-future
none
Patch
none
Patch
none
Patch
none
Patch none

Description Said Abou-Hallawa 2019-04-29 13:32:53 PDT
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.
Comment 1 Said Abou-Hallawa 2019-04-29 13:39:27 PDT
Created attachment 368485 [details]
Patch
Comment 2 Said Abou-Hallawa 2019-04-29 13:40:12 PDT
<rdar://problem/48781098>
Comment 3 Said Abou-Hallawa 2019-04-30 17:27:21 PDT
Created attachment 368632 [details]
Patch
Comment 4 Simon Fraser (smfr) 2019-04-30 17:31:07 PDT
Comment on attachment 368632 [details]
Patch

Can we add layout tests for this?
Comment 5 Said Abou-Hallawa 2019-05-03 22:31:16 PDT
Created attachment 369055 [details]
Patch
Comment 6 Said Abou-Hallawa 2019-05-03 23:16:54 PDT
Created attachment 369059 [details]
Patch
Comment 7 EWS Watchlist 2019-05-04 00:37:40 PDT
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
Comment 8 EWS Watchlist 2019-05-04 00:37:42 PDT
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
Comment 9 EWS Watchlist 2019-05-04 02:25:56 PDT
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
Comment 10 EWS Watchlist 2019-05-04 02:25:58 PDT
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
Comment 11 Simon Fraser (smfr) 2019-05-04 09:56:30 PDT
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.
Comment 12 Said Abou-Hallawa 2019-05-06 09:49:49 PDT
Created attachment 369128 [details]
Patch
Comment 13 Sam Weinig 2019-05-06 10:22:10 PDT
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?
Comment 14 Said Abou-Hallawa 2019-05-06 10:48:20 PDT
Created attachment 369136 [details]
Patch
Comment 15 Said Abou-Hallawa 2019-05-06 10:57:03 PDT
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.
Comment 16 Said Abou-Hallawa 2019-05-06 10:57:53 PDT
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.
Comment 17 Sam Weinig 2019-05-06 13:02:11 PDT
(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.
Comment 18 Simon Fraser (smfr) 2019-05-06 13:09:31 PDT
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?
Comment 19 Said Abou-Hallawa 2019-05-06 14:42:49 PDT
Created attachment 369169 [details]
Patch
Comment 20 Said Abou-Hallawa 2019-05-06 15:13:04 PDT
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.
Comment 21 Said Abou-Hallawa 2019-05-06 15:14:46 PDT
(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.
Comment 22 EWS Watchlist 2019-05-06 16:44:31 PDT
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
Comment 23 EWS Watchlist 2019-05-06 16:44:34 PDT
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
Comment 24 Sam Weinig 2019-05-06 19:00:16 PDT
(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?
Comment 25 Said Abou-Hallawa 2019-05-09 14:59:58 PDT
Created attachment 369524 [details]
Patch
Comment 26 Said Abou-Hallawa 2019-05-09 18:59:30 PDT
Created attachment 369538 [details]
Patch
Comment 27 Said Abou-Hallawa 2019-05-10 09:43:10 PDT
Created attachment 369556 [details]
Patch
Comment 28 Said Abou-Hallawa 2019-05-10 09:44:50 PDT
I cleaned ImageDecoderCG::repetitionCount() and ImageDecoderCG::frameDurationAtIndex() by deleting the repeating code and avoiding unnecessary calls to the image properties dictionary.
Comment 29 Simon Fraser (smfr) 2019-05-13 14:32:31 PDT
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.
Comment 30 Said Abou-Hallawa 2019-05-13 21:41:11 PDT
Created attachment 369819 [details]
Patch
Comment 31 WebKit Commit Bot 2019-05-14 08:14:03 PDT
Comment on attachment 369819 [details]
Patch

Clearing flags on attachment: 369819

Committed r245280: <https://trac.webkit.org/changeset/245280>
Comment 32 WebKit Commit Bot 2019-05-14 08:14:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 33 Said Abou-Hallawa 2019-05-14 11:28:22 PDT
The tests, which were submitted with this patch, have to be skipped in WebKit for now.

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