New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 704285 link

Starred by 3 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 704287
issue 706860



Sign in to add a comment

Add overdraw regression test harness

Project Member Reported by rjkroege@chromium.org, Mar 22 2017

Issue description

Implement a mechanism to measure overdraw in CQ/bot tests of various actions such as Ash user animations and ensure that they do not regress. See bot overdraw tests of https://goo.gl/1wk60Y.

sadrul@ assign as appropriate
 
Blocking: 704287
Labels: mustash-2
Blocking: 706860
Status: Started (was: Assigned)
Summary: Add overdraw regression test harness (was: add overdraw regression test harness)

Comment 5 by sky@chromium.org, Jun 8 2017

Blocking: 731255
Project Member

Comment 6 by bugdroid1@chromium.org, Jun 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1e117683d1819528521a763e94162035d42f1826

commit 1e117683d1819528521a763e94162035d42f1826
Author: sadrul <sadrul@chromium.org>
Date: Mon Jun 12 23:23:34 2017

ozone: Allow using swiftshader with --use-gl=swiftshader.

This hooks up the ozone code so it can deal with --use-gl=swiftshader
flag correctly.

BUG=704285
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2926423002
Cr-Commit-Position: refs/heads/master@{#478817}

[modify] https://crrev.com/1e117683d1819528521a763e94162035d42f1826/ui/ozone/common/BUILD.gn
[modify] https://crrev.com/1e117683d1819528521a763e94162035d42f1826/ui/ozone/common/egl_util.cc
[modify] https://crrev.com/1e117683d1819528521a763e94162035d42f1826/ui/ozone/common/egl_util.h
[modify] https://crrev.com/1e117683d1819528521a763e94162035d42f1826/ui/ozone/common/gl_ozone_egl.cc
[modify] https://crrev.com/1e117683d1819528521a763e94162035d42f1826/ui/ozone/common/gl_ozone_egl.h
[modify] https://crrev.com/1e117683d1819528521a763e94162035d42f1826/ui/ozone/platform/cast/gl_ozone_egl_cast.cc
[modify] https://crrev.com/1e117683d1819528521a763e94162035d42f1826/ui/ozone/platform/cast/gl_ozone_egl_cast.h
[modify] https://crrev.com/1e117683d1819528521a763e94162035d42f1826/ui/ozone/platform/drm/gpu/gbm_surface_factory.cc
[modify] https://crrev.com/1e117683d1819528521a763e94162035d42f1826/ui/ozone/platform/wayland/wayland_surface_factory.cc
[modify] https://crrev.com/1e117683d1819528521a763e94162035d42f1826/ui/ozone/platform/x11/x11_surface_factory.cc

Comment 7 by sadrul@chromium.org, Jun 13 2017

Cc: rjkroege@chromium.org piman@chromium.org danakj@chromium.org mfomitchev@chromium.org fsam...@chromium.org
The initial plan for this was to introduce a DirectRenderer subclass (say, OverdrawRenderer) that would approximate the amount of overdraw. There are two good reasons for this: (1) this would continue to work after we have draw occlusion ( issue 672929 ), and will actually show the effect of the draw occlusion, and (2) it will not be necessary to change production code, i.e. GLRenderer for this.

As I think more about it, I am not sure anymore (2) is a good reason. Because the GLRenderer does some additional work that this new DirectRenderer would have to replicate (e.g. [1, 2]), which would be unfortunate. It would be nice if that wasn't necessary. I am thinking it would be simpler to add the relevant code into GLRenderer itself. I have an experimental WIP CL [3] for this that prints out some info to stdout. It wouldn't be too bad to plumb that data back into a test, possibly through some DrawInfoCollector interface set through RendererSettings?

Thoughts on whether adding this support to GLRenderer seems reasonable?

[1] https://cs.chromium.org/chromium/src/cc/output/gl_renderer.cc?sq=package:chromium&dr=CSs&l=1076
[2] https://cs.chromium.org/chromium/src/cc/output/gl_renderer.cc?sq=package:chromium&dr=CSs&l=1740
[3] https://codereview.chromium.org/2934963002/
This seems reasonable but if we're modifying the renderer then why not use the existing stencil based overdraw output?

Comment 9 by danakj@chromium.org, Jun 13 2017

>  am not sure anymore (2) is a good reason. Because the GLRenderer does some additional work that this new DirectRenderer would have to replicate (e.g. [1, 2])

I am wondering why this is GLRenderer and not DirectRenderer doing this sorta thing with GL-specific helpers, so that it can do this for Software compositing too?
> This seems reasonable but if we're modifying the renderer then why not use
> the existing stencil based overdraw output?

When I try the stencil based overdraw results locally in a test, I get somewhat confusing numbers in [1], where it always spits out "1000". I haven't looked closely into it yet, but is the regular test-infra expected to work? (PixelTestOutputSurface is used in the test, which does turn on |supports_stencil| flag [2], fwiw).

We also discussed measuring overdraw separately for opaque and translucent quads, and doing something different for render-passes. Would it be easy to update the stencil-based overdraw output for that?

[1] https://cs.chromium.org/chromium/src/cc/output/gl_renderer.cc?sq=package:chromium&dr=CSs&l=3618
[2] https://cs.chromium.org/chromium/src/cc/test/pixel_test_output_surface.cc?sq=package:chromium&dr=CSs&l=24

Are occlusion queries [1] supported? It's not going to work otherwise.

[1] https://cs.chromium.org/chromium/src/gpu/command_buffer/common/capabilities.h?l=148

Accumulated overdraw from render passes is hard to do using the stencil approach but I'm not sure that makes sense in general. It might be better to highlight render passes in a more distinct way (e.g. consider render pass output as infinite overdraw) but provide the tools to diagnose overdraw of specific render passes individually.
Blockedon: 644851
Since we are going to have a SkiaRenderer [1], and skia has SkOverdrawCanvas [2], which we can use to measure the amount of overdraw (e.g. [3]), it makes sense for us to use that, instead of creating a custom renderer for measuring the overdraw, I think. So I am going to mark this blocked on issue 644851 (for SkiaRenderer), and will start experimenting with this approach.

[1] https://chromium-review.googlesource.com/c/591780
[2] https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkOverdrawCanvas.h?type=cs&sq=package:chromium&l=18
[3] https://cs.chromium.org/chromium/src/third_party/skia/tests/SurfaceTest.cpp?q=SurfaceTest.cpp&sq=package:chromium&l=925
Project Member

Comment 13 by bugdroid1@chromium.org, Aug 18 2017

The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/4189d1b8b7dfa62f10845b0afcadba3a956ec939

commit 4189d1b8b7dfa62f10845b0afcadba3a956ec939
Author: Sadrul Habib Chowdhury <sadrul@chromium.org>
Date: Fri Aug 18 16:53:49 2017

Add SK_API to SkOverdrawCanvas and SkOverdrawColorFilter.

The SkiaRenderer in chromium is going to use the overdraw canvas and
filter for measuring overdraw. Move these headers out of src/ into
include/.

Bug: chromium:704285
Change-Id: I2abb1671b73e3d26552462cf700340a7e3b874f0
Reviewed-on: https://skia-review.googlesource.com/36160
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>

[modify] https://crrev.com/4189d1b8b7dfa62f10845b0afcadba3a956ec939/gn/effects.gni
[modify] https://crrev.com/4189d1b8b7dfa62f10845b0afcadba3a956ec939/src/ports/SkGlobalInitialization_default.cpp
[rename] https://crrev.com/4189d1b8b7dfa62f10845b0afcadba3a956ec939/include/core/SkOverdrawCanvas.h
[rename] https://crrev.com/4189d1b8b7dfa62f10845b0afcadba3a956ec939/include/effects/SkOverdrawColorFilter.h
[modify] https://crrev.com/4189d1b8b7dfa62f10845b0afcadba3a956ec939/gn/core.gni

Project Member

Comment 14 by bugdroid1@chromium.org, Aug 22 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/20ae366e936073f3d6f8f942b130849655b804a4

commit 20ae366e936073f3d6f8f942b130849655b804a4
Author: Sadrul Habib Chowdhury <sadrul@chromium.org>
Date: Tue Aug 22 06:28:40 2017

viz: Add overdraw visualization support to SkiaRenderer.

Make the --show-overdraw-feedback flag work with SkiaRenderer.

Bug: 704285

Change-Id: Ide03c33388425d37ae5709c7533725369d887772
Reviewed-on: https://chromium-review.googlesource.com/619892
Commit-Queue: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Brian Salomon <bsalomon@chromium.org>
Reviewed-by: weiliangc <weiliangc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496232}
[modify] https://crrev.com/20ae366e936073f3d6f8f942b130849655b804a4/components/viz/service/display/skia_renderer.cc
[modify] https://crrev.com/20ae366e936073f3d6f8f942b130849655b804a4/components/viz/service/display/skia_renderer.h

Project Member

Comment 15 by bugdroid1@chromium.org, Aug 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6f32734610f76a586f81b1cf3fa6df17cec131f7

commit 6f32734610f76a586f81b1cf3fa6df17cec131f7
Author: Sadrul Habib Chowdhury <sadrul@chromium.org>
Date: Sat Aug 26 00:16:57 2017

viz: Update RendererSettings mojom for use_skia_renderer.

Update RendererSettings mojom for use_skia_renderer, which was
introduced in crrev.com/494660. This allows using the skia
renderer with the mus window server.

BUG=704285

Change-Id: I21e6b5f1a2478c2535261486075aab7accbf9271
Reviewed-on: https://chromium-review.googlesource.com/625484
Reviewed-by: weiliangc <weiliangc@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#497605}
[modify] https://crrev.com/6f32734610f76a586f81b1cf3fa6df17cec131f7/services/viz/privileged/interfaces/compositing/renderer_settings.mojom
[modify] https://crrev.com/6f32734610f76a586f81b1cf3fa6df17cec131f7/services/viz/privileged/interfaces/compositing/renderer_settings_struct_traits.cc
[modify] https://crrev.com/6f32734610f76a586f81b1cf3fa6df17cec131f7/services/viz/privileged/interfaces/compositing/renderer_settings_struct_traits.h
[modify] https://crrev.com/6f32734610f76a586f81b1cf3fa6df17cec131f7/services/viz/privileged/interfaces/struct_traits_unittest.cc

Blockedon: -644851
The SkiaRenderer has landed, and has enough that this bug is no longer blocked on that.
Cc: sadrul@chromium.org
Owner: ----
Status: Available (was: Started)
I am not actively working on this right now.
Blocking: -731255
Cc: yiyix@chromium.org
Components: Internals>Compositing
Cc: -mfomitchev@chromium.org
Components: -Internals>MUS Internals>Services>WindowService
Labels: -Proj-Mustash-Mus-GPU
Cleaning up old Proj-Mustash labels.

Sign in to add a comment