Bug 228195

Summary: The layout test fast/images/heic-as-background-image.html is a constant failure
Product: WebKit Reporter: Per Arne Vollan <pvollan>
Component: WebKit Misc.Assignee: Per Arne Vollan <pvollan>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, bfulgham, cdumez, cmarcelo, ews-watchlist, sabouhallawa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=226869
https://bugs.webkit.org/show_bug.cgi?id=228812
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
sabouhallawa: review+, ews-feeder: commit-queue-
Patch none

Description Per Arne Vollan 2021-07-22 13:58:19 PDT 
No image is being rendered in this test.
Comment 1 Per Arne Vollan 2021-07-22 13:58:47 PDT 
<rdar://80334724>
Comment 2 Per Arne Vollan 2021-07-22 14:05:40 PDT 
Created attachment 434036 [details]
Patch
Comment 3 Per Arne Vollan 2021-07-23 07:55:08 PDT 
Created attachment 434088 [details]
Patch
Comment 4 Per Arne Vollan 2021-07-23 11:24:09 PDT 
Created attachment 434100 [details]
Patch
Comment 5 Said Abou-Hallawa 2021-07-24 02:27:44 PDT 
Comment on attachment 434100 [details]
Patch

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

> Source/WTF/wtf/PlatformHave.h:1102
> +#define HAVE_CMPHOTOISTILEDECODERAVAILABLE 1

I think we should add underscores to make this macro more readable. I think also we should not have "IS" in the macro:

#define HAVE_CMPHOTO_TILE_DECODER_AVAILABLE 1

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:176
> +SOFT_LINK_PRIVATE_FRAMEWORK_OPTIONAL(CMPhoto)
> +SOFT_LINK_FUNCTION_MAY_FAIL_FOR_SOURCE(WebKit, CMPhoto, CMPhotoIsTileDecoderAvailable, Boolean, (CMVideoCodecType decoder), (decoder))

Should these macros be surrounded by #if HAVE(CMPHOTO_TILE_DECODER_AVAILABLE) and #endif

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:390
>              if (PAL::isMediaToolboxFrameworkAvailable() && PAL::canLoad_MediaToolbox_FigPhotoSupportsHEVCHWDecode())
>                  PAL::softLinkMediaToolboxFigPhotoSupportsHEVCHWDecode();

Why do we run this code on Big Sur? I think the hardware decoding for HEIF was only added in Monterey.
Comment 6 Per Arne Vollan 2021-07-26 07:53:10 PDT 
Created attachment 434206 [details]
Patch
Comment 7 Per Arne Vollan 2021-07-26 07:56:33 PDT 
(In reply to Said Abou-Hallawa from comment #5)
> Comment on attachment 434100 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=434100&action=review
> 
> > Source/WTF/wtf/PlatformHave.h:1102
> > +#define HAVE_CMPHOTOISTILEDECODERAVAILABLE 1
> 
> I think we should add underscores to make this macro more readable. I think
> also we should not have "IS" in the macro:
> 
> #define HAVE_CMPHOTO_TILE_DECODER_AVAILABLE 1
> 

Fixed!

> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:176
> > +SOFT_LINK_PRIVATE_FRAMEWORK_OPTIONAL(CMPhoto)
> > +SOFT_LINK_FUNCTION_MAY_FAIL_FOR_SOURCE(WebKit, CMPhoto, CMPhotoIsTileDecoderAvailable, Boolean, (CMVideoCodecType decoder), (decoder))
> 
> Should these macros be surrounded by #if
> HAVE(CMPHOTO_TILE_DECODER_AVAILABLE) and #endif
>

Yes, done.
 
> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:390
> >              if (PAL::isMediaToolboxFrameworkAvailable() && PAL::canLoad_MediaToolbox_FigPhotoSupportsHEVCHWDecode())
> >                  PAL::softLinkMediaToolboxFigPhotoSupportsHEVCHWDecode();
> 
> Why do we run this code on Big Sur? I think the hardware decoding for HEIF
> was only added in Monterey.

For the test to succeed on Big Sur, we still need to run this code, since the sandbox restrictions were done for all OS versions.

Thanks for reviewing!
Comment 8 Said Abou-Hallawa 2021-07-26 10:50:36 PDT 
Comment on attachment 434206 [details]
Patch

I do not see a change in LayoutTests/platform/Mac/TestExpectations in this patch.This file has the following lines for this test:

LayoutTests/platform/mac/TestExpectations:[ BigSur ] fast/images/heic-as-background-image.html [ Pass ]
LayoutTests/platform/mac/TestExpectations:# rdar://80334724 ([ Monterey ] fast/images/heic-as-background-image.html is a constant failure)
LayoutTests/platform/mac/TestExpectations:[ Monterey ] fast/images/heic-as-background-image.html [ ImageOnlyFailure ]

I think they should be replaced by  

LayoutTests/platform/mac/TestExpectations:[ BigSur+ ] fast/images/heic-as-background-image.html [ Pass ]
Comment 9 Per Arne Vollan 2021-07-26 13:00:18 PDT 
Created attachment 434228 [details]
Patch
Comment 10 Per Arne Vollan 2021-07-26 13:01:58 PDT 
(In reply to Said Abou-Hallawa from comment #8)
> Comment on attachment 434206 [details]
> Patch
> 
> I do not see a change in LayoutTests/platform/Mac/TestExpectations in this
> patch.This file has the following lines for this test:
> 
> LayoutTests/platform/mac/TestExpectations:[ BigSur ]
> fast/images/heic-as-background-image.html [ Pass ]
> LayoutTests/platform/mac/TestExpectations:# rdar://80334724 ([ Monterey ]
> fast/images/heic-as-background-image.html is a constant failure)
> LayoutTests/platform/mac/TestExpectations:[ Monterey ]
> fast/images/heic-as-background-image.html [ ImageOnlyFailure ]
> 
> I think they should be replaced by  
> 
> LayoutTests/platform/mac/TestExpectations:[ BigSur+ ]
> fast/images/heic-as-background-image.html [ Pass ]

Good point, fixed in latest patch.

Thanks!
Comment 11 EWS 2021-07-26 15:07:29 PDT 
Committed r280318 (239966@main): <https://commits.webkit.org/239966@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 434228 [details].