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

Per Arne Vollan
Reported 2021-07-22 13:58:19 PDT
No image is being rendered in this test.
Attachments
Patch (2.73 KB, patch)
2021-07-22 14:05 PDT, Per Arne Vollan
no flags
Patch (2.76 KB, patch)
2021-07-23 07:55 PDT, Per Arne Vollan
no flags
Patch (3.90 KB, patch)
2021-07-23 11:24 PDT, Per Arne Vollan
no flags
Patch (4.08 KB, patch)
2021-07-26 07:53 PDT, Per Arne Vollan
sabouhallawa: review+
ews-feeder: commit-queue-
Patch (6.08 KB, patch)
2021-07-26 13:00 PDT, Per Arne Vollan
no flags
Per Arne Vollan
Comment 1 2021-07-22 13:58:47 PDT
Per Arne Vollan
Comment 2 2021-07-22 14:05:40 PDT
Per Arne Vollan
Comment 3 2021-07-23 07:55:08 PDT
Per Arne Vollan
Comment 4 2021-07-23 11:24:09 PDT
Said Abou-Hallawa
Comment 5 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.
Per Arne Vollan
Comment 6 2021-07-26 07:53:10 PDT
Per Arne Vollan
Comment 7 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!
Said Abou-Hallawa
Comment 8 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 ]
Per Arne Vollan
Comment 9 2021-07-26 13:00:18 PDT
Per Arne Vollan
Comment 10 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!
EWS
Comment 11 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].
Note You need to log in before you can comment on or make changes to this bug.