Skip to content

Placeholder canvas does not always propagate HDR content format correctly to the layer #47292

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Placeholder canvas does not always propagate HDR content format correctly to the layer #47292

wants to merge 1 commit into from

Conversation

kkinnunen-apple
Copy link
Contributor

@kkinnunen-apple kkinnunen-apple commented Jun 27, 2025 •
edited by webkit-early-warning-system
Loading

0fa785e

Placeholder canvas does not always propagate HDR content format correctly to the layer
https://bugs.webkit.org/show_bug.cgi?id=295095
rdar://154478292

Reviewed by NOBODY (OOPS!).

Propagate the HDR content format from the image buffer.
Fixes a race where the content format was not set if the buffer had
not reached the main thread when the delegate was created.

* Source/WebCore/html/canvas/PlaceholderRenderingContext.cpp:
(WebCore::PlaceholderRenderingContextSource::setContentsToLayer):
(WebCore::PlaceholderRenderingContext::setContentsToLayer):
(WebCore::PlaceholderRenderingContext::setPlaceholderBuffer):
(WebCore::PlaceholderRenderingContext::pixelFormat const):
(WebCore::pixelFormatToContentsFormat): Deleted.
* Source/WebCore/html/canvas/PlaceholderRenderingContext.h:
* Source/WebCore/platform/graphics/GraphicsLayerContentsDisplayDelegate.h:
(WebCore::GraphicsLayerAsyncContentsDisplayDelegate::isGraphicsLayerCARemoteAsyncContentsDisplayDelegate const):
(WebCore::GraphicsLayerAsyncContentsDisplayDelegate::setContentsFormat): Deleted.
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerTreePropertyApplier.h:
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerTreePropertyApplier.mm:
(WebKit::applyContentsFormatToLayer):
(WebKit::RemoteLayerTreePropertyApplier::applyPropertiesToLayer):
(WebKit::RemoteLayerTreePropertyApplier::applyAsyncContentsUpdate):
* Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.h:
* Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.messages.in:
* Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.mm:
(WebKit::RemoteLayerTreeDrawingAreaProxy::asyncSetLayerContents):
* Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeHost.h:
* Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeHost.mm:
(WebKit::RemoteLayerTreeHost::asyncSetLayerContents):
* Source/WebKit/WebProcess/WebPage/RemoteLayerTree/GraphicsLayerCARemote.mm:
(WebKit::pixelFormatToContentsFormat):

0fa785e

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ❌ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ❌ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ❌ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ❌ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@kkinnunen-apple
Placeholder canvas does not always propagate HDR content format corre…
0fa785e 
…ctly to the layer

https://bugs.webkit.org/show_bug.cgi?id=295095
rdar://154478292

Reviewed by NOBODY (OOPS!).

Propagate the HDR content format from the image buffer.
Fixes a race where the content format was not set if the buffer had
not reached the main thread when the delegate was created.

* Source/WebCore/html/canvas/PlaceholderRenderingContext.cpp:
(WebCore::PlaceholderRenderingContextSource::setContentsToLayer):
(WebCore::PlaceholderRenderingContext::setContentsToLayer):
(WebCore::PlaceholderRenderingContext::setPlaceholderBuffer):
(WebCore::PlaceholderRenderingContext::pixelFormat const):
(WebCore::pixelFormatToContentsFormat): Deleted.
* Source/WebCore/html/canvas/PlaceholderRenderingContext.h:
* Source/WebCore/platform/graphics/GraphicsLayerContentsDisplayDelegate.h:
(WebCore::GraphicsLayerAsyncContentsDisplayDelegate::isGraphicsLayerCARemoteAsyncContentsDisplayDelegate const):
(WebCore::GraphicsLayerAsyncContentsDisplayDelegate::setContentsFormat): Deleted.
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerTreePropertyApplier.h:
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerTreePropertyApplier.mm:
(WebKit::applyContentsFormatToLayer):
(WebKit::RemoteLayerTreePropertyApplier::applyPropertiesToLayer):
(WebKit::RemoteLayerTreePropertyApplier::applyAsyncContentsUpdate):
* Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.h:
* Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.messages.in:
* Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.mm:
(WebKit::RemoteLayerTreeDrawingAreaProxy::asyncSetLayerContents):
* Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeHost.h:
* Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeHost.mm:
(WebKit::RemoteLayerTreeHost::asyncSetLayerContents):
* Source/WebKit/WebProcess/WebPage/RemoteLayerTree/GraphicsLayerCARemote.mm:
(WebKit::pixelFormatToContentsFormat):
@kkinnunen-apple kkinnunen-apple self-assigned this Jun 27, 2025
@kkinnunen-apple kkinnunen-apple added the Canvas Bugs related to the canvas element. label Jun 27, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 27, 2025 •
edited
Loading

EWS run on current version of this PR (hash 0fa785e)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ❌ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ❌ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ❌ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ❌ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 27, 2025
mwyrzykowski
mwyrzykowski reviewed Jun 27, 2025
View reviewed changes
Source/WebKit/Shared/RemoteLayerTree/RemoteLayerTreePropertyApplier.mm
ALLOW_DEPRECATED_DECLARATIONS_BEGIN
[layer setWantsExtendedDynamicRangeContent:true];
ALLOW_DEPRECATED_DECLARATIONS_END
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggested addition:

diff --git a/Source/WebKit/Shared/RemoteLayerTree/RemoteLayerTreePropertyApplier.mm b/Source/WebKit/Shared/RemoteLayerTree/RemoteLayerTreePropertyApplier.mm
index 6a6be5e4c328..aed53b4b988e 100644
--- a/Source/WebKit/Shared/RemoteLayerTree/RemoteLayerTreePropertyApplier.mm
+++ b/Source/WebKit/Shared/RemoteLayerTree/RemoteLayerTreePropertyApplier.mm
@@ -387,6 +387,7 @@ static void applyContentsFormatToLayer(CALayer *layer, ContentsFormat contentsFo
         ALLOW_DEPRECATED_DECLARATIONS_BEGIN
         [layer setWantsExtendedDynamicRangeContent:true];
         ALLOW_DEPRECATED_DECLARATIONS_END
+        [layer setToneMapMode:CAToneMapModeIfSupported];
     }
 #endif
 }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would enable tone mapping for painted content too, which I don't think we want to do.

mwyrzykowski
mwyrzykowski previously approved these changes Jun 27, 2025
View reviewed changes
Copy link
Contributor

@mwyrzykowski mwyrzykowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r+ with suggested addition:

diff --git a/Source/WebKit/Shared/RemoteLayerTree/RemoteLayerTreePropertyApplier.mm b/Source/WebKit/Shared/RemoteLayerTree/RemoteLayerTreePropertyApplier.mm
index 6a6be5e4c328..aed53b4b988e 100644
--- a/Source/WebKit/Shared/RemoteLayerTree/RemoteLayerTreePropertyApplier.mm
+++ b/Source/WebKit/Shared/RemoteLayerTree/RemoteLayerTreePropertyApplier.mm
@@ -387,6 +387,7 @@ static void applyContentsFormatToLayer(CALayer *layer, ContentsFormat contentsFo
         ALLOW_DEPRECATED_DECLARATIONS_BEGIN
         [layer setWantsExtendedDynamicRangeContent:true];
         ALLOW_DEPRECATED_DECLARATIONS_END
+        [layer setToneMapMode:CAToneMapModeIfSupported];
     }
 #endif
 }

@mwyrzykowski mwyrzykowski dismissed their stale review June 30, 2025 20:37

Defer to @mattwoodrow for approval for how to correctly handle tonemapping in the layer tree

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattwoodrow mattwoodrow mattwoodrow left review comments

@mwyrzykowski mwyrzykowski mwyrzykowski left review comments

@cdumez cdumez Awaiting requested review from cdumez cdumez is a code owner

@rniwa rniwa Awaiting requested review from rniwa rniwa is a code owner

Assignees

@kkinnunen-apple kkinnunen-apple

Labels
Canvas Bugs related to the canvas element. merging-blocked Applied to prevent a change from being merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kkinnunen-apple @webkit-early-warning-system @mattwoodrow @mwyrzykowski @webkit-ews-buildbot