Commit 1c792623 authored by Brian Ho's avatar Brian Ho Committed by Commit Bot

viz: Fully invalidate entire frame on changing color usage

An earlier CL [1] attempted to fix a screen flickering issue with HDR
content by expanding the damage rect to the entire frame if the color
usage changes (e.g. SDR -> HDR).

This fix, however, doesn't work when there is no damage on the frame
(besides the color space change) because the expanded damage rect is
eventually overwritten via an intersection with the 0-sized damage
[2]. When the frame reports no damage, the draw is skipped entirely
even though we should really be invalidating everything as per the
original CL.

This can be reproduced by playing an HDR video on YouTube in a
background tab and then mousing over the tab. Chrome will process the
HDR video frame in preparation to display the tab preview and the
CompositorFrame is marked as "HDR" [4]. After the user mouses off the
tab, Chrome will eventually stop processing the HDR video and will
send a transitional "empty frame" [5] with an sRGB color space which
triggers the problematic 0-damage DrawAndSwap mentioned above. At
this point, the user will likely see the screen flicker black because
the frame wasn't properly drawn.

This CL updates SurfaceAggregator::Aggregate to stop overwriting the
full-frame damage if the color space changes.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/2369441
[2] https://source.chromium.org/chromium/chromium/src/+/master:components/viz/service/display/surface_aggregator.cc;l=1758;drc=72b4db84824e257c412ac88a4941d12619a44dd2
[3] https://source.chromium.org/chromium/chromium/src/+/master:components/viz/service/display/display.cc;l=726;drc=cf4c5571a11fd0ce94bccbd867c65b75b28350ac
[4] https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/platform/graphics/video_frame_submitter.cc;l=614;drc=72b4db84824e257c412ac88a4941d12619a44dd2
[5] https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/platform/graphics/video_frame_submitter.cc;l=388;drc=72b4db84824e257c412ac88a4941d12619a44dd2

Bug: 1132962
Change-Id: I03214c857d804b0a969e05db2853a0a5e9dadbf5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2453802
Commit-Queue: Brian Ho <hob@chromium.org>
Reviewed-by: default avatarDaniele Castagna <dcastagna@chromium.org>
Cr-Commit-Position: refs/heads/master@{#815213}
parent 76f4f5a8
......@@ -1719,7 +1719,9 @@ AggregatedFrame SurfaceAggregator::Aggregate(
// Changing color usage will cause the renderer to reshape the output surface,
// therefore the renderer might expand the damage to the whole frame. The
// following makes sure SA will produce all the quads to cover the full frame.
if (root_content_color_usage_ != prewalk_result.content_color_usage) {
bool color_usage_changed =
root_content_color_usage_ != prewalk_result.content_color_usage;
if (color_usage_changed) {
root_damage_rect_ = cc::MathUtil::MapEnclosedRectWith2dAxisAlignedTransform(
root_surface_transform_,
gfx::Rect(root_surface_frame.size_in_pixels()));
......@@ -1754,7 +1756,8 @@ AggregatedFrame SurfaceAggregator::Aggregate(
// The damage on the root render pass should not include the expanded area
// since Renderer and OverlayProcessor expect the non expanded damage.
auto* last_pass = dest_pass_list_->back().get();
if (!RenderPassNeedsFullDamage(last_pass->id, last_pass->cache_render_pass))
if (!color_usage_changed &&
!RenderPassNeedsFullDamage(last_pass->id, last_pass->cache_render_pass))
dest_pass_list_->back()->damage_rect.Intersect(surfaces_damage_rect);
// Now that we've handled our main surface aggregation, apply de-jelly effect
......
......@@ -8792,6 +8792,63 @@ TEST_F(SurfaceAggregatorValidSurfaceTest,
EXPECT_FALSE(new_aggregated_frame.delegated_ink_metadata);
}
// Tests that changing the color usage results in full-frame damage.
TEST_F(SurfaceAggregatorValidSurfaceTest, ColorUsageChangeFullFrameDamage) {
constexpr float device_scale_factor = 1.0f;
const gfx::Rect full_damage_rect(SurfaceSize());
const gfx::Rect partial_damage_rect(10, 10, 10, 10);
std::vector<Quad> quads = {
Quad::SolidColorQuad(SK_ColorRED, gfx::Rect(SurfaceSize()))};
std::vector<Pass> passes = {Pass(quads, SurfaceSize())};
passes[0].damage_rect = partial_damage_rect;
// First frame has full damage.
{
SubmitCompositorFrame(root_sink_.get(), passes, root_local_surface_id_,
device_scale_factor);
SurfaceId root_surface_id(root_sink_->frame_sink_id(),
root_local_surface_id_);
auto aggregated_frame = AggregateFrame(root_surface_id);
EXPECT_EQ(gfx::ContentColorUsage::kHDR,
aggregated_frame.render_pass_list[0]->content_color_usage);
EXPECT_EQ(full_damage_rect,
aggregated_frame.render_pass_list[0]->damage_rect);
}
// Second frame has partial damage.
{
SubmitCompositorFrame(root_sink_.get(), passes, root_local_surface_id_,
device_scale_factor);
SurfaceId root_surface_id(root_sink_->frame_sink_id(),
root_local_surface_id_);
auto aggregated_frame = AggregateFrame(root_surface_id);
EXPECT_EQ(gfx::ContentColorUsage::kHDR,
aggregated_frame.render_pass_list[0]->content_color_usage);
EXPECT_EQ(partial_damage_rect,
aggregated_frame.render_pass_list[0]->damage_rect);
}
// Finally, change the content_color_usage from HDR to sRGB. The resulting
// frame should have full damage.
{
CompositorFrame compositor_frame = MakeEmptyCompositorFrame();
compositor_frame.metadata.content_color_usage =
gfx::ContentColorUsage::kSRGB;
AddPasses(&compositor_frame.render_pass_list, passes,
&compositor_frame.metadata.referenced_surfaces);
root_sink_->SubmitCompositorFrame(root_local_surface_id_,
std::move(compositor_frame));
SurfaceId root_surface_id(root_sink_->frame_sink_id(),
root_local_surface_id_);
auto aggregated_frame = AggregateFrame(root_surface_id);
EXPECT_EQ(gfx::ContentColorUsage::kSRGB,
aggregated_frame.render_pass_list[0]->content_color_usage);
EXPECT_EQ(full_damage_rect,
aggregated_frame.render_pass_list[0]->damage_rect);
}
}
INSTANTIATE_TEST_SUITE_P(,
SurfaceAggregatorValidSurfaceWithMergingPassesTest,
testing::Bool());
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment