Bug 228195 - The layout test fast/images/heic-as-background-image.html is a constant failure
Summary: The layout test fast/images/heic-as-background-image.html is a constant failure
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-07-22 13:58 PDT by Per Arne Vollan
Modified: 2021-08-09 11:14 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.73 KB, patch)
2021-07-22 14:05 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (2.76 KB, patch)
2021-07-23 07:55 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (3.90 KB, patch)
2021-07-23 11:24 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (4.08 KB, patch)
2021-07-26 07:53 PDT, Per Arne Vollan
sabouhallawa: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (6.08 KB, patch)
2021-07-26 13:00 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].