Rework how SF generates gainmapped screenshots
Previously, SF generated an HDR screenshot by:
* Asking CompositionEngine to composite layers offscreen into SDR
* Asking CompositionEngine _again_ to composite layers offscreen into
HDR
* Asking RenderEngine to build a gainmap
* Sending the SDR buffer and the gainmap to the client
But, this had some problems:
* This path bit-rot, so the HDR layer list was totally empty :)
* LayerFE now has an assumption that only one *thing* will be consuming
the layer and emitting a release fence back. So, this fixing the above
caused ghost images, and undoing that assumption just for screenshots
is a bit unappetizing.
* If the above assumption *were* undone, then the release fence for the
layer would be delayed due to rendering to an additional render
target, which sucks.
* We talk to RenderEngine three times, which involves thread hops, which
sucks.
So, we solve these problems by:
* Only asking CompositionEngine to composite layers into HDR
* Then, ask RenderEngine _once_ to tonemap the HDR buffer into SDR, and
generate a gainmap.
* Send the SDR buffer and the gainmap to the client.
And now most things are solved: LayerFE is only used to render to one
render target, so it only gets one release fence back, as expected.
Moreover, compositing into HDR is expected to be very cheap, as the
local tonemapper only has to run when tonemapping from a very large HDR
range (like a PQ video) -- when the application is already adapting to
display conditions then we don't have to do anything. And, we only have
to talk to RenderEngine twice now.
We could optimize this further by moving the gainmap generation into
CompositionEngine, but that seems like adding complexity for the moment.
Note that in theory, screenshot quality for the SDR rendition could be a
little worse, since MouriMap introduces shadowing artifacts that may now
bleed across layers. But it doesn't seem to be noticeable in practice,
and it was a pre-existing issue for application-tonemapped content.
Also, we remove CaptureArgs::attachGainmap, since we don't need it at
all to safely trigger this path.
Bug: 329465218
Flag: com.android.graphics.surfaceflinger.flags.true_hdr_screenshots
Test: Trigger screencapture
Change-Id: I5a274886acb135b21096300a034aadca20eceeb5
diff --git a/services/surfaceflinger/RegionSamplingThread.cpp b/services/surfaceflinger/RegionSamplingThread.cpp
index 21d3396..d3483b0 100644
--- a/services/surfaceflinger/RegionSamplingThread.cpp
+++ b/services/surfaceflinger/RegionSamplingThread.cpp
@@ -346,7 +346,6 @@
constexpr bool kRegionSampling = true;
constexpr bool kGrayscale = false;
constexpr bool kIsProtected = false;
- constexpr bool kAttachGainmap = false;
SurfaceFlinger::RenderAreaBuilderVariant
renderAreaBuilder(std::in_place_type<DisplayRenderAreaBuilder>, sampledBounds,
@@ -358,7 +357,7 @@
mFlinger.getSnapshotsFromMainThread(renderAreaBuilder, getLayerSnapshotsFn, layers);
FenceResult fenceResult =
mFlinger.captureScreenshot(renderAreaBuilder, buffer, kRegionSampling, kGrayscale,
- kIsProtected, kAttachGainmap, nullptr, displayState, layers)
+ kIsProtected, nullptr, displayState, layers)
.get();
if (fenceResult.ok()) {
fenceResult.value()->waitForever(LOG_TAG);
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 59b1917..962e2fc 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -7145,8 +7145,7 @@
displayWeak, options),
getLayerSnapshotsFn, reqSize,
static_cast<ui::PixelFormat>(captureArgs.pixelFormat),
- captureArgs.allowProtected, captureArgs.grayscale,
- captureArgs.attachGainmap, captureListener);
+ captureArgs.allowProtected, captureArgs.grayscale, captureListener);
}
void SurfaceFlinger::captureDisplay(DisplayId displayId, const CaptureArgs& args,
@@ -7203,7 +7202,7 @@
static_cast<ui::Dataspace>(args.dataspace),
displayWeak, options),
getLayerSnapshotsFn, size, static_cast<ui::PixelFormat>(args.pixelFormat),
- kAllowProtected, kGrayscale, args.attachGainmap, captureListener);
+ kAllowProtected, kGrayscale, captureListener);
}
ScreenCaptureResults SurfaceFlinger::captureLayersSync(const LayerCaptureArgs& args) {
@@ -7314,8 +7313,7 @@
options),
getLayerSnapshotsFn, reqSize,
static_cast<ui::PixelFormat>(captureArgs.pixelFormat),
- captureArgs.allowProtected, captureArgs.grayscale,
- captureArgs.attachGainmap, captureListener);
+ captureArgs.allowProtected, captureArgs.grayscale, captureListener);
}
// Creates a Future release fence for a layer and keeps track of it in a list to
@@ -7373,7 +7371,7 @@
void SurfaceFlinger::captureScreenCommon(RenderAreaBuilderVariant renderAreaBuilder,
GetLayerSnapshotsFunction getLayerSnapshotsFn,
ui::Size bufferSize, ui::PixelFormat reqPixelFormat,
- bool allowProtected, bool grayscale, bool attachGainmap,
+ bool allowProtected, bool grayscale,
const sp<IScreenCaptureListener>& captureListener) {
SFTRACE_CALL();
@@ -7388,6 +7386,10 @@
std::vector<std::pair<Layer*, sp<LayerFE>>> layers;
auto displayState = getSnapshotsFromMainThread(renderAreaBuilder, getLayerSnapshotsFn, layers);
+ const bool hasHdrLayer = std::any_of(layers.cbegin(), layers.cend(), [this](const auto& layer) {
+ return isHdrLayer(*(layer.second->mSnapshot.get()));
+ });
+
const bool supportsProtected = getRenderEngine().supportsProtectedContent();
bool hasProtectedLayer = false;
if (allowProtected && supportsProtected) {
@@ -7416,9 +7418,51 @@
renderengine::impl::ExternalTexture>(buffer, getRenderEngine(),
renderengine::impl::ExternalTexture::Usage::
WRITEABLE);
- auto futureFence =
- captureScreenshot(renderAreaBuilder, texture, false /* regionSampling */, grayscale,
- isProtected, attachGainmap, captureListener, displayState, layers);
+
+ std::shared_ptr<renderengine::impl::ExternalTexture> hdrTexture;
+ std::shared_ptr<renderengine::impl::ExternalTexture> gainmapTexture;
+
+ bool hintForSeamless = std::visit(
+ [](auto&& arg) {
+ return arg.options.test(RenderArea::Options::HINT_FOR_SEAMLESS_TRANSITION);
+ },
+ renderAreaBuilder);
+ if (hasHdrLayer && !hintForSeamless && FlagManager::getInstance().true_hdr_screenshots()) {
+ const auto hdrBuffer =
+ getFactory().createGraphicBuffer(buffer->getWidth(), buffer->getHeight(),
+ HAL_PIXEL_FORMAT_RGBA_FP16, 1 /* layerCount */,
+ buffer->getUsage(), "screenshot-hdr");
+ const auto gainmapBuffer =
+ getFactory().createGraphicBuffer(buffer->getWidth(), buffer->getHeight(),
+ buffer->getPixelFormat(), 1 /* layerCount */,
+ buffer->getUsage(), "screenshot-gainmap");
+
+ const status_t hdrBufferStatus = hdrBuffer->initCheck();
+ const status_t gainmapBufferStatus = gainmapBuffer->initCheck();
+
+ if (hdrBufferStatus != OK || gainmapBufferStatus != -OK) {
+ if (hdrBufferStatus != OK) {
+ ALOGW("%s: Buffer failed to allocate for hdr: %d. Screenshoting SDR instead.",
+ __func__, hdrBufferStatus);
+ } else {
+ ALOGW("%s: Buffer failed to allocate for gainmap: %d. Screenshoting SDR instead.",
+ __func__, gainmapBufferStatus);
+ }
+ } else {
+ hdrTexture = std::make_shared<
+ renderengine::impl::ExternalTexture>(hdrBuffer, getRenderEngine(),
+ renderengine::impl::ExternalTexture::
+ Usage::WRITEABLE);
+ gainmapTexture = std::make_shared<
+ renderengine::impl::ExternalTexture>(gainmapBuffer, getRenderEngine(),
+ renderengine::impl::ExternalTexture::
+ Usage::WRITEABLE);
+ }
+ }
+
+ auto futureFence = captureScreenshot(renderAreaBuilder, texture, false /* regionSampling */,
+ grayscale, isProtected, captureListener, displayState,
+ layers, hdrTexture, gainmapTexture);
futureFence.get();
}
@@ -7461,10 +7505,11 @@
ftl::SharedFuture<FenceResult> SurfaceFlinger::captureScreenshot(
const RenderAreaBuilderVariant& renderAreaBuilder,
const std::shared_ptr<renderengine::ExternalTexture>& buffer, bool regionSampling,
- bool grayscale, bool isProtected, bool attachGainmap,
- const sp<IScreenCaptureListener>& captureListener,
- std::optional<OutputCompositionState>& displayState,
- std::vector<std::pair<Layer*, sp<LayerFE>>>& layers) {
+ bool grayscale, bool isProtected, const sp<IScreenCaptureListener>& captureListener,
+ const std::optional<OutputCompositionState>& displayState,
+ const std::vector<std::pair<Layer*, sp<LayerFE>>>& layers,
+ const std::shared_ptr<renderengine::ExternalTexture>& hdrBuffer,
+ const std::shared_ptr<renderengine::ExternalTexture>& gainmapBuffer) {
SFTRACE_CALL();
ScreenCaptureResults captureResults;
@@ -7480,74 +7525,40 @@
}
return ftl::yield<FenceResult>(base::unexpected(NO_ERROR)).share();
}
+
float displayBrightnessNits = displayState.value().displayBrightnessNits;
float sdrWhitePointNits = displayState.value().sdrWhitePointNits;
- ftl::SharedFuture<FenceResult> renderFuture =
- renderScreenImpl(renderArea.get(), buffer, regionSampling, grayscale, isProtected,
- captureResults, displayState, layers);
+ ftl::SharedFuture<FenceResult> renderFuture;
- if (captureResults.capturedHdrLayers && attachGainmap &&
- FlagManager::getInstance().true_hdr_screenshots()) {
- sp<GraphicBuffer> hdrBuffer =
- getFactory().createGraphicBuffer(buffer->getWidth(), buffer->getHeight(),
- HAL_PIXEL_FORMAT_RGBA_FP16, 1 /* layerCount */,
- buffer->getUsage(), "screenshot-hdr");
- sp<GraphicBuffer> gainmapBuffer =
- getFactory().createGraphicBuffer(buffer->getWidth(), buffer->getHeight(),
- buffer->getPixelFormat(), 1 /* layerCount */,
- buffer->getUsage(), "screenshot-gainmap");
+ if (hdrBuffer && gainmapBuffer) {
+ ftl::SharedFuture<FenceResult> hdrRenderFuture =
+ renderScreenImpl(renderArea.get(), hdrBuffer, regionSampling, grayscale,
+ isProtected, captureResults, displayState, layers);
+ captureResults.buffer = buffer->getBuffer();
+ captureResults.optionalGainMap = gainmapBuffer->getBuffer();
- const status_t bufferStatus = hdrBuffer->initCheck();
- const status_t gainmapBufferStatus = gainmapBuffer->initCheck();
+ renderFuture =
+ ftl::Future(std::move(hdrRenderFuture))
+ .then([&, displayBrightnessNits, sdrWhitePointNits,
+ dataspace = captureResults.capturedDataspace, buffer, hdrBuffer,
+ gainmapBuffer](FenceResult fenceResult) -> FenceResult {
+ if (!fenceResult.ok()) {
+ return fenceResult;
+ }
- if (bufferStatus != OK) {
- ALOGW("%s: Buffer failed to allocate for hdr: %d. Screenshoting SDR instead.", __func__,
- bufferStatus);
- } else if (gainmapBufferStatus != OK) {
- ALOGW("%s: Buffer failed to allocate for gainmap: %d. Screenshoting SDR instead.",
- __func__, gainmapBufferStatus);
- } else {
- captureResults.optionalGainMap = gainmapBuffer;
- const auto hdrTexture = std::make_shared<
- renderengine::impl::ExternalTexture>(hdrBuffer, getRenderEngine(),
- renderengine::impl::ExternalTexture::
- Usage::WRITEABLE);
- const auto gainmapTexture = std::make_shared<
- renderengine::impl::ExternalTexture>(gainmapBuffer, getRenderEngine(),
- renderengine::impl::ExternalTexture::
- Usage::WRITEABLE);
- ScreenCaptureResults unusedResults;
- ftl::SharedFuture<FenceResult> hdrRenderFuture =
- renderScreenImpl(renderArea.get(), hdrTexture, regionSampling, grayscale,
- isProtected, unusedResults, displayState, layers);
-
- renderFuture =
- ftl::Future(std::move(renderFuture))
- .then([&, hdrRenderFuture = std::move(hdrRenderFuture),
- displayBrightnessNits, sdrWhitePointNits,
- dataspace = captureResults.capturedDataspace, buffer, hdrTexture,
- gainmapTexture](FenceResult fenceResult) -> FenceResult {
- if (!fenceResult.ok()) {
- return fenceResult;
- }
-
- auto hdrFenceResult = hdrRenderFuture.get();
-
- if (!hdrFenceResult.ok()) {
- return hdrFenceResult;
- }
-
- return getRenderEngine()
- .drawGainmap(buffer, fenceResult.value()->get(), hdrTexture,
- hdrFenceResult.value()->get(),
- displayBrightnessNits / sdrWhitePointNits,
- static_cast<ui::Dataspace>(dataspace),
- gainmapTexture)
- .get();
- })
- .share();
- };
+ return getRenderEngine()
+ .tonemapAndDrawGainmap(hdrBuffer, fenceResult.value()->get(),
+ displayBrightnessNits /
+ sdrWhitePointNits,
+ static_cast<ui::Dataspace>(dataspace),
+ buffer, gainmapBuffer)
+ .get();
+ })
+ .share();
+ } else {
+ renderFuture = renderScreenImpl(renderArea.get(), buffer, regionSampling, grayscale,
+ isProtected, captureResults, displayState, layers);
}
if (captureListener) {
@@ -7569,8 +7580,8 @@
ftl::SharedFuture<FenceResult> SurfaceFlinger::renderScreenImpl(
const RenderArea* renderArea, const std::shared_ptr<renderengine::ExternalTexture>& buffer,
bool regionSampling, bool grayscale, bool isProtected, ScreenCaptureResults& captureResults,
- std::optional<OutputCompositionState>& displayState,
- std::vector<std::pair<Layer*, sp<LayerFE>>>& layers) {
+ const std::optional<OutputCompositionState>& displayState,
+ const std::vector<std::pair<Layer*, sp<LayerFE>>>& layers) {
SFTRACE_CALL();
for (auto& [_, layerFE] : layers) {
@@ -7638,9 +7649,8 @@
}
auto present = [this, buffer = capturedBuffer, dataspace = captureResults.capturedDataspace,
- sdrWhitePointNits, displayBrightnessNits, grayscale, isProtected,
- layers = std::move(layers), layerStack, regionSampling,
- renderArea = std::move(renderArea), renderIntent,
+ sdrWhitePointNits, displayBrightnessNits, grayscale, isProtected, layers,
+ layerStack, regionSampling, renderArea = std::move(renderArea), renderIntent,
enableLocalTonemapping]() -> FenceResult {
std::unique_ptr<compositionengine::CompositionEngine> compositionEngine =
mFactory.createCompositionEngine();
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index 1e2c087..236e288 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -872,7 +872,7 @@
void captureScreenCommon(RenderAreaBuilderVariant, GetLayerSnapshotsFunction,
ui::Size bufferSize, ui::PixelFormat, bool allowProtected,
- bool grayscale, bool attachGainmap, const sp<IScreenCaptureListener>&);
+ bool grayscale, const sp<IScreenCaptureListener>&);
std::optional<OutputCompositionState> getDisplayStateFromRenderAreaBuilder(
RenderAreaBuilderVariant& renderAreaBuilder) REQUIRES(kMainThreadContext);
@@ -880,16 +880,17 @@
ftl::SharedFuture<FenceResult> captureScreenshot(
const RenderAreaBuilderVariant& renderAreaBuilder,
const std::shared_ptr<renderengine::ExternalTexture>& buffer, bool regionSampling,
- bool grayscale, bool isProtected, bool attachGainmap,
- const sp<IScreenCaptureListener>& captureListener,
- std::optional<OutputCompositionState>& displayState,
- std::vector<std::pair<Layer*, sp<LayerFE>>>& layers);
+ bool grayscale, bool isProtected, const sp<IScreenCaptureListener>& captureListener,
+ const std::optional<OutputCompositionState>& displayState,
+ const std::vector<std::pair<Layer*, sp<LayerFE>>>& layers,
+ const std::shared_ptr<renderengine::ExternalTexture>& hdrBuffer = nullptr,
+ const std::shared_ptr<renderengine::ExternalTexture>& gainmapBuffer = nullptr);
ftl::SharedFuture<FenceResult> renderScreenImpl(
const RenderArea*, const std::shared_ptr<renderengine::ExternalTexture>&,
bool regionSampling, bool grayscale, bool isProtected, ScreenCaptureResults&,
- std::optional<OutputCompositionState>& displayState,
- std::vector<std::pair<Layer*, sp<LayerFE>>>& layers);
+ const std::optional<OutputCompositionState>& displayState,
+ const std::vector<std::pair<Layer*, sp<LayerFE>>>& layers);
void readPersistentProperties();