-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Apply dynamic-range-limit to HDR WebGPU canvas #44915
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.
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
base: main
Are you sure you want to change the base?
Conversation
EWS run on previous version of this PR (hash 0fa9248) |
0fa9248
to
216fdd0
Compare
EWS run on previous version of this PR (hash 216fdd0) |
216fdd0
to
95fd691
Compare
EWS run on previous version of this PR (hash 95fd691) |
95fd691
to
1d52d53
Compare
EWS run on previous version of this PR (hash 1d52d53) |
1d52d53
to
5710014
Compare
EWS run on previous version of this PR (hash 5710014) |
https://bugs.webkit.org/show_bug.cgi?id=294831 rdar://148558614 Reviewed by NOBODY (OOPS!). *** TEMPORARY EXTRA LOGGING, DO NOT MERGE *** * Source/WebCore/Modules/WebGPU/Implementation/WebGPUCompositorIntegrationImpl.cpp: (WebCore::WebGPU::CompositorIntegrationImpl::updateContentsHeadroom): * Source/WebCore/dom/Document.cpp: (WebCore::Document::shouldSuppressHDRDidChange): * Source/WebCore/html/HTMLCanvasElement.cpp: (WebCore::HTMLCanvasElement::computeContextDynamicRangeLimit const): (WebCore::HTMLCanvasElement::dynamicRangeLimitDidChange): Store the current dynamic-range-limit, and set the effective limit (which accounts for the suppress-HDR state) in the rendering context. * Source/WebCore/html/HTMLCanvasElement.h: * Source/WebCore/html/canvas/CanvasRenderingContext.h: (WebCore::CanvasRenderingContext::setDynamicRangeLimit): Default dynamic-range-limit handler, does nothing. * Source/WebCore/html/canvas/GPUCanvasContextCocoa.h: * Source/WebCore/html/canvas/GPUCanvasContextCocoa.mm: (WebCore::GPUCanvasContextCocoa::updateContentsHeadroom): (WebCore::GPUCanvasContextCocoa::setDynamicRangeLimit): WebGPU dynamic-range-limit handler, stores the limit in the display delegate, which sets the corresponding property on the actual display PlatformCALayer (remote or not). * Source/WebCore/page/Page.cpp: (WebCore::Page::setShouldSuppressHDR): * Source/WebCore/platform/graphics/ca/PlatformCALayer.h: dynamic-range-limit property interface. * Source/WebCore/platform/graphics/ca/cocoa/PlatformCALayerCocoa.h: * Source/WebCore/platform/graphics/ca/cocoa/PlatformCALayerCocoa.mm: (WebCore::PlatformCALayerCocoa::dynamicRangeLimit const): (WebCore::PlatformCALayerCocoa::setDynamicRangeLimit): Handle setting the dynamic-range-limit on the actual CALayer. * Source/WebCore/rendering/RenderHTMLCanvas.cpp: (WebCore::RenderHTMLCanvas::styleDidChange): Catch dynamic-range-limit changes, and notify the canvas. * Source/WebCore/rendering/RenderHTMLCanvas.h: * Source/WebKit/Shared/RemoteLayerTree/LayerProperties.h: * Source/WebKit/Shared/RemoteLayerTree/RemoteLayerTree.serialization.in: * Source/WebKit/Shared/RemoteLayerTree/RemoteLayerTreePropertyApplier.mm: (WebKit::RemoteLayerTreePropertyApplier::applyPropertiesToLayer): * Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm: (WebKit::WebProcessPool::screenPropertiesChanged): * Source/WebKit/UIProcess/mac/WebViewImpl.mm: (WebKit::WebViewImpl::applicationShouldSuppressHDR): * Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.h: * Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.mm: (WebKit::PlatformCALayerRemote::dynamicRangeLimit const): (WebKit::PlatformCALayerRemote::setDynamicRangeLimit): Handle setting the dynamic-range-limit, and forward it as a layer property to the counterpart layer in the appropriate process.
5710014
to
b0b01fe
Compare
EWS run on current version of this PR (hash b0b01fe)
|
|
||
m_dynamicRangeLimit = dynamicRangeLimit; | ||
|
||
setLayerDynamicRangeLimit(m_layer.get(), dynamicRangeLimit); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think using setLayerDynamicRangeLimit results in the intended behavior and it also does not allow us to correctly implement https://www.w3.org/TR/css-color-hdr-1/#dynamic-range-limit-mix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the 2nd point, I think this is moot: dynamic-range-limit-mix()
is currently disabled anyway, because [CALayer setPreferredDynamicRange:]
(called by setLayerDynamicRangeLimit
) doesn't support arbitrary values. Once CALayer
supports these, mix
can be implemented.
I'm not sure why you think setLayerDynamicRangeLimit
and therefore setPreferredDynamicRange
doesn't have the intended behavior? Maybe we should discuss more... (Later this week, I won't be available much until then.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless we can resolve the issue where SDR white becomes gray, I don't think we can use it. It is very noticeable in content and does not look correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But please remember that dynamic-range-limit
is an author's choice (unlike OS-imposed dimming/suppressing), so the same author that chooses to use standard
should be able to see the effects and deal with them, for example by having their WebGPU code generate pixel values that are appropriate to the SDR case.
@@ -62,8 +62,10 @@ void CompositorIntegrationImpl::prepareForDisplay(uint32_t frameIndex, Completio | |||
void CompositorIntegrationImpl::updateContentsHeadroom(float headroom) | |||
{ | |||
#if HAVE(SUPPORT_HDR_DISPLAY) | |||
for (auto& ioSurface : m_renderBuffers) | |||
for (auto& ioSurface : m_renderBuffers) { | |||
ALWAYS_LOG_WITH_STREAM(stream << "__GS__ CompositorIntegrationImpl[" << this << "]::updateContentsHeadroom(" << headroom << ") -> ioSurface[" << &ioSurface << "]->setContentEDRHeadroom(" << headroom << ")"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an 'HDR' log channel in WebCore now, if you want to make some of this logging more-permanent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for letting me know. But this is just temporary logging while we're debugging; I'll remove them all.
b0b01fe
b0b01fe
🧪 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🧪 mac-intel-wk2🛠 playstation🛠 tv🛠 mac-safer-cpp🛠 tv-sim🛠 watch🛠 watch-sim