Bug 228195 – The layout test fast/images/heic-as-background-image.html is a constant failure

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

No image is being rendered in this test.

[Per Arne Vollan]

<rdar://80334724>

[Per Arne Vollan]

Created attachment 434036 [details]
Patch

[Per Arne Vollan]

Created attachment 434088 [details]
Patch

[Per Arne Vollan]

Created attachment 434100 [details]
Patch

[Said Abou-Hallawa]

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.

[Per Arne Vollan]

Created attachment 434206 [details]
Patch

[Per Arne Vollan]

(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!

[Said Abou-Hallawa]

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 ]

[Per Arne Vollan]

Created attachment 434228 [details]
Patch

[Per Arne Vollan]

(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!

[EWS]

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].