LayerTreePixelTest::CreateCompositorFrameSink Passes Default RendererSettings to TestCompositorFrameSink |
||||||||
Issue descriptionLayerTreePixelTest should not pass default RendererSettings to create TestCompositorFrameSink. We also need to reestablish baselines for some pixel tests after the change.
,
May 30 2017
,
Jun 13 2017
,
Jun 13 2017
,
Jun 13 2017
These tests fail:
LayerTreeHostBlendingPixelTest.BlendingWithRenderPassAA_GL (../../cc/trees/layer_tree_host_pixeltest_blending.cc:351)
LayerTreeHostBlendingPixelTest.BlendingWithRenderPassColorMatrixAA_GL (../../cc/trees/layer_tree_host_pixeltest_blending.cc:400)
LayerTreeHostBlendingPixelTest.BlendingWithRenderPassShadersAA_GL (../../cc/trees/layer_tree_host_pixeltest_blending.cc:448)
LayerTreeHostBlendingPixelTest.BlendingWithRenderPassShadersColorMatrixAA_GL (../../cc/trees/layer_tree_host_pixeltest_blending.cc:490)
LayerTreeHostBlendingPixelTest.BlendingWithRenderPassShadersWithMaskAA_GL (../../cc/trees/layer_tree_host_pixeltest_blending.cc:469)
LayerTreeHostBlendingPixelTest.BlendingWithRenderPassShadersWithMaskAA_GL_TextureRect (../../cc/trees/layer_tree_host_pixeltest_blending.cc:476)
LayerTreeHostBlendingPixelTest.BlendingWithRenderPassShadersWithMaskColorMatrixAA_GL (../../cc/trees/layer_tree_host_pixeltest_blending.cc:511)
LayerTreeHostBlendingPixelTest.BlendingWithRenderPassWithMaskAA_GL (../../cc/trees/layer_tree_host_pixeltest_blending.cc:375)
LayerTreeHostBlendingPixelTest.BlendingWithRenderPassWithMaskColorMatrixAA_GL (../../cc/trees/layer_tree_host_pixeltest_blending.cc:428)
These are tests that set:
force_antialiasing = true;
force_blending_with_shaders = true;
And they fail in very bad ways, suggesting that the code is broken since it was not actually being tested.
I need to look back in time and see what happened when the tests were landed until now.
,
Jun 13 2017
It looks like these flags were set correctly before, but in 1120f4c4ee42 when we started using a Display in cc_unittests, I dropped the RendererSettings. I guess these tests broke at some point since then. I'm struggling to sync and build as far back as Sept.
,
Jun 13 2017
I bisected this to: 7bc8fc1007007ece8eaa4137967c3162bad4db08 is the first bad commit commit 7bc8fc1007007ece8eaa4137967c3162bad4db08 Author: trchen <trchen@chromium.org> Date: Mon Jan 30 17:26:08 2017 -0800 [cc] Add SkBlendMode::kDstIn support to cc::Layer This CL adds SkBlendMode::kDstIn support to cc::Layer. kDstIn is useful for applying an alpha mask to a group. There is an ongoing plan to use kDstIn to replace mask-layer-based CSS mask implementation. The SPv2 implementation of CSS mask uses this alternative approach: https://codereview.chromium.org/2641173008/ CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2659023003 Cr-Commit-Position: refs/heads/master@{#447153} With the following diff, the tests pass before this CL but not after: diff --git a/cc/test/layer_tree_pixel_test.cc b/cc/test/layer_tree_pixel_test.cc index b59c54d0b29c..b9d18eea6612 100644 --- a/cc/test/layer_tree_pixel_test.cc +++ b/cc/test/layer_tree_pixel_test.cc @@ -54,10 +54,13 @@ std::unique_ptr<TestCompositorFrameSink> !layer_tree_host()->GetSettings().single_thread_proxy_scheduler; // Allow resource reclaiming for partial raster tests to get back // resources from the Display. + RendererSettings r; + r.force_antialiasing = true; + r.force_blending_with_shaders = true; bool force_disable_reclaim_resources = false; auto delegating_output_surface = base::MakeUnique<TestCompositorFrameSink>( compositor_context_provider, std::move(worker_context_provider), - shared_bitmap_manager(), gpu_memory_buffer_manager(), RendererSettings(), + shared_bitmap_manager(), gpu_memory_buffer_manager(), r, ImplThreadTaskRunner(), synchronous_composite, force_disable_reclaim_resources); delegating_output_surface->SetEnlargePassTextureAmount(
,
Jun 13 2017
It is the 3 kDstIn columns in this test that are broken.
,
Jun 13 2017
trchen@ can you have a look please?
,
Jun 13 2017
Just to be clear this is what we actually want to do once these tests work:
diff --git a/cc/test/data/blending_render_pass.png b/cc/test/data/blending_render_pass.png
index 5ac57e32ce96..9f9485051815 100644
Binary files a/cc/test/data/blending_render_pass.png and b/cc/test/data/blending_render_pass.png differ
diff --git a/cc/test/layer_tree_pixel_test.cc b/cc/test/layer_tree_pixel_test.cc
index ce61a691e4f5..e2fe9b92ce3f 100644
--- a/cc/test/layer_tree_pixel_test.cc
+++ b/cc/test/layer_tree_pixel_test.cc
@@ -57,7 +57,7 @@ LayerTreePixelTest::CreateCompositorFrameSink(
!layer_tree_host()->GetSettings().single_thread_proxy_scheduler;
auto delegating_output_surface = base::MakeUnique<TestCompositorFrameSink>(
compositor_context_provider, std::move(worker_context_provider),
- shared_bitmap_manager(), gpu_memory_buffer_manager(), RendererSettings(),
+ shared_bitmap_manager(), gpu_memory_buffer_manager(), renderer_settings,
ImplThreadTaskRunner(), synchronous_composite, disable_display_vsync,
refresh_rate);
delegating_output_surface->SetEnlargePassTextureAmount(
,
Jun 15 2017
I figured out the root cause. It involves how do we want kDstIn blend mode to interact with edge AA. kDstIn is not a true blend mode in the sense that it doesn't commute with alpha operation. Multiplying opacity first then blend with the backdrop yields different result than blending with the backdrop first then composite with an opacity. For example, a fully opaque (src.a = 1.0) mask with 0.5 layer opacity, if we multiply opacity first, then the half-opaque pixel (src.a = 0.5) will reduce the destination pixel by half. But if we do blending first, then the blending result will be the same as the destination pixel, thus transferring the blending result onto the destination with 0.5 opacity will not change the pixel. We currently implement the former semantics, but intuitively we would expect edge AA works in the latter semantics. However, kDstIn was introduced to replace mask layer, and it is expected that the target surface to be the same size as the mask layer. The former edge AA semantics is preferred in this use case because we actually want to clear partial pixels beyond the layer bound. The latter semantics would bleed the partial pixel on the edge instead. I think the right thing to do is to properly document the restriction of kDstIn mode in layer.h, and move kDstIn related pixel tests to a dedicated test.
,
Jun 15 2017
I think Software_AA versions do the latter one, should they match?
,
Jun 15 2017
,
Jun 15 2017
Ah, that explains things. I experimented locally to switch to the latter semantics then AA_Software failed. That's why. Yes AA_Software and AA_GL should match but didn't. I should fix AA_Software to match AA_GL (actual.png, the ugly one).
,
Sep 20 2017
Hey Dana, sorry for the delay, I'm right now working on redesigning kDstIn related tests.
The first thing I did is to revert my changes to the existing pixel tests, and passing the correct renderer_settings, but all tests of the GL version still failed.
19 tests failed:
LayerTreeHostBlendingPixelTest.BlendingWithRenderPassAA_GL (../../cc/trees/layer_tree_host_pixeltest_blending.cc:349)
LayerTreeHostBlendingPixelTest.BlendingWithRenderPassColorMatrixAA_GL (../../cc/trees/layer_tree_host_pixeltest_blending.cc:398)
LayerTreeHostBlendingPixelTest.BlendingWithRenderPassColorMatrix_GL (../../cc/trees/layer_tree_host_pixeltest_blending.cc:386)
LayerTreeHostBlendingPixelTest.BlendingWithRenderPassShadersAA_GL (../../cc/trees/layer_tree_host_pixeltest_blending.cc:446)
LayerTreeHostBlendingPixelTest.BlendingWithRenderPassShadersColorMatrixAA_GL (../../cc/trees/layer_tree_host_pixeltest_blending.cc:488)
LayerTreeHostBlendingPixelTest.BlendingWithRenderPassShadersColorMatrix_GL (../../cc/trees/layer_tree_host_pixeltest_blending.cc:481)
LayerTreeHostBlendingPixelTest.BlendingWithRenderPassShadersWithMaskAA_GL (../../cc/trees/layer_tree_host_pixeltest_blending.cc:467)
LayerTreeHostBlendingPixelTest.BlendingWithRenderPassShadersWithMaskAA_GL_TextureRect (../../cc/trees/layer_tree_host_pixeltest_blending.cc:474)
LayerTreeHostBlendingPixelTest.BlendingWithRenderPassShadersWithMaskColorMatrixAA_GL (../../cc/trees/layer_tree_host_pixeltest_blending.cc:509)
LayerTreeHostBlendingPixelTest.BlendingWithRenderPassShadersWithMaskColorMatrix_GL (../../cc/trees/layer_tree_host_pixeltest_blending.cc:495)
LayerTreeHostBlendingPixelTest.BlendingWithRenderPassShadersWithMaskColorMatrix_GL_TextureRect (../../cc/trees/layer_tree_host_pixeltest_blending.cc:502)
LayerTreeHostBlendingPixelTest.BlendingWithRenderPassShadersWithMask_GL (../../cc/trees/layer_tree_host_pixeltest_blending.cc:453)
LayerTreeHostBlendingPixelTest.BlendingWithRenderPassShadersWithMask_GL_TextureRect (../../cc/trees/layer_tree_host_pixeltest_blending.cc:460)
LayerTreeHostBlendingPixelTest.BlendingWithRenderPassShaders_GL (../../cc/trees/layer_tree_host_pixeltest_blending.cc:440)
LayerTreeHostBlendingPixelTest.BlendingWithRenderPassWithMaskAA_GL (../../cc/trees/layer_tree_host_pixeltest_blending.cc:373)
LayerTreeHostBlendingPixelTest.BlendingWithRenderPassWithMaskColorMatrixAA_GL (../../cc/trees/layer_tree_host_pixeltest_blending.cc:426)
LayerTreeHostBlendingPixelTest.BlendingWithRenderPassWithMaskColorMatrix_GL (../../cc/trees/layer_tree_host_pixeltest_blending.cc:412)
LayerTreeHostBlendingPixelTest.BlendingWithRenderPassWithMask_GL (../../cc/trees/layer_tree_host_pixeltest_blending.cc:361)
LayerTreeHostBlendingPixelTest.BlendingWithRenderPass_GL (../../cc/trees/layer_tree_host_pixeltest_blending.cc:339)
I visually inspected the results, some of the diffs in AA_GL version is actually visible by eyes. Bleeding can be observed between columns (slightly brighter), by looking at the bottom row because each columns are expected to have the same color in the bottom cell. The diffs in GL version is less obvious, all errors exceeding 1 are due to some over draw between rows (but why?).
I can make a different baseline for the GL version, but some of these failures are intriguing...
,
Sep 20 2017
It may be that they have broken more since. I had bisected back to 7bc8fc1007007ece8eaa4137967c3162bad4db08 and with correct RendererSettings they passed before that but failed after. More problems may have been introduced since, as they are running without the RendererSettings. You could fix it from that CL and then bisect to the present to find further issues, if that's the case.
,
Sep 20 2017
I did some direct debugging yesterday but didn't succeed. Things I tried include forcing high texture precision, splitting each lane into 8x8 cells. I suspect something went wrong when the blended result was drawn on top of the white root background. I'll bisect and update my results here.
,
Sep 20 2017
I tried as far back to r433788, the GL tests had always been failing. Could it be my GPU being funky? I attached my results of LayerTreeHostBlendingPixelTest.BlendingWithRenderPass_GL for your reference.
,
Sep 20 2017
These tests use osmesa, no gpu involved.
,
Sep 20 2017
Did you try https://bugs.chromium.org/p/chromium/issues/detail?id=717514#c9 as a sanity check? Go before that CL, apply that diff, and tests should pass?
,
Sep 20 2017
Yes, I did that too. Checking out 7bc8fc10~1, applied that diff, and found out the test didn't pass. It would be surprising if it passed on your machine. That's why I asked about GPU, but since it's Mesa we can rule that out.
,
Sep 20 2017
Ah, that's really weird. You gclient sync'd to have the same osmesa version ofc.. it wouldn't compile otherwise right?
,
Sep 20 2017
Okay, I figured out most of the failures. They are just due to FP precision being too low. I'm not sure if Mesa supports highp, but even highp is much less precise than IEEE754 single precision. (mediump is just terrible)
Part one, our anti-aliasing curve is tight, resulting in sometimes the edge pixels end up a weight lower than 1, even if pixels are perfectly aligned. For example, let's say a layer rect (0, 0, 100, 100) and just look at the right edge, the AA weight function would be:
w = 1 | if x <= 99.5
w = 100.5 - x | if 99.5 < x < 100.5
w = 0 | if x >= 100.5
Which is actually written as the edge distance function:
w = clamp(edge_distance(x, y), 0, 1)
edge_distance(x, y) = 100.5 - x
So far so good, but our shader implementation computes edge distance of the 4 quad points in the vertex shader, then rely on built-in interpolation to compute edge distance for each fragment. So this is how it works internally:
Vertex shader:
edge_distance(-0.5, -0.5) = 101
edge_distance(100.5, -0.5) = 0
edge_distance(100.5, 100.5) = 0
Then for a edge pixel (99.5, y), its edge distance is computed using an interpolation of 3 values above. Any unfortunate rounding can cause the value lower than 1.
Changing the AA curve to a sharp drop would eliminate these failures:
--------
diff --git a/components/viz/service/display/layer_quad.cc b/components/viz/service/display/layer_quad.cc
index 0385b5d173ca..8e47ab1d430a 100644
--- a/components/viz/service/display/layer_quad.cc
+++ b/components/viz/service/display/layer_quad.cc
@@ -21,7 +21,7 @@ LayerQuad::Edge::Edge(const gfx::PointF& p, const gfx::PointF& q) {
float cross2 = p.x() * q.y() - q.x() * p.y();
set(tangent.x(), tangent.y(), cross2);
- scale(1.0f / tangent.Length());
+ scale(2.0f / tangent.Length());
}
gfx::PointF LayerQuad::Edge::Intersect(const LayerQuad::Edge& e) const {
--------
Effectively making the weight function like:
w = 1 | if x <= 99.75
w = 2 * (100.25 - x) | if 99.75 < x < 100.25
w = 0 | if x >= 100.25
Actually any value not as tight could work, say 1.0625 or something.
,
Sep 20 2017
Part two, because the texture coordinate precision is so low, it is very preferable to make texture square, sized in multiple of some large power of 2. I first made a mistake when I tried to revert the test I added. That resulted in adding some invisible layers outside of the test area (0, 0, 160, 160). One of the intermediate surface ends up being sized (176, 160), which is not a very smooth number. Removing the invisible layer alone would make all tests flew under the error allowance (8). Combined with change above can actually make maximum error under 1 for BlendingWithRenderPassAA_GL.
,
Sep 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/560130c75fac030f2b4ba2684b5c5da28490b9d9 commit 560130c75fac030f2b4ba2684b5c5da28490b9d9 Author: Tien-Ren Chen <trchen@chromium.org> Date: Fri Sep 22 21:59:03 2017 [cc] Using RendererSettings in pixel tests This CL reverts tests added in r447153: > commit 7bc8fc1007007ece8eaa4137967c3162bad4db08 > Author: trchen <trchen@chromium.org> > Date: Mon Jan 30 17:26:08 2017 -0800 > > [cc] Add SkBlendMode::kDstIn support to cc::Layer which never interoperated correctly with antialiasing shaders. With the tests removed, RendererSettings can be correctly used in the pixel tests without introducing failures. BUG= 717514 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Change-Id: Ie6aa3784395650449b5aa76e592f7573d6399caa Reviewed-on: https://chromium-review.googlesource.com/675808 Commit-Queue: Tien-Ren Chen <trchen@chromium.org> Reviewed-by: danakj <danakj@chromium.org> Cr-Commit-Position: refs/heads/master@{#503870} [modify] https://crrev.com/560130c75fac030f2b4ba2684b5c5da28490b9d9/cc/test/layer_tree_pixel_test.cc [modify] https://crrev.com/560130c75fac030f2b4ba2684b5c5da28490b9d9/cc/trees/layer_tree_host_pixeltest_blending.cc [modify] https://crrev.com/560130c75fac030f2b4ba2684b5c5da28490b9d9/components/viz/test/data/blending_and_filter.png [modify] https://crrev.com/560130c75fac030f2b4ba2684b5c5da28490b9d9/components/viz/test/data/blending_render_pass.png [modify] https://crrev.com/560130c75fac030f2b4ba2684b5c5da28490b9d9/components/viz/test/data/blending_render_pass_mask.png [modify] https://crrev.com/560130c75fac030f2b4ba2684b5c5da28490b9d9/components/viz/test/data/blending_transparent.png [modify] https://crrev.com/560130c75fac030f2b4ba2684b5c5da28490b9d9/components/viz/test/data/blending_with_root.png
,
Sep 25 2017
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by fsam...@chromium.org
, May 25 2017Labels: displaycompositor