Currently, when layout tests do an image diff, any OOPIFs on the page get ignored in the generated image. For example, this came up in https://codereview.chromium.org/2507023002/#ps140001, where running the added test with --additional-drt-flag=--site-per-process resulted in a blank subframe in the image diffs.
Since #c15 mentioned different kinds of DrawQuads, I added logging the quad contents - I see that in the broken, all-white-pixels case, SubmitCompositorFrame seems to contain 2 quads of TILED_CONTENT material:
LayerTreHostImpl::DrawLayers - calling SubmitCompositorFrame ...; compositor_frame.render_pass_list.size() = 1
RenderPass; rp->quad_list.size() = 2; rp->damage_rect = @(0,0) x (800,300)
DrawQuad; quad->rect = @(0,0) x (255,255); quad->material = 8
DrawQuad; quad->rect = @(255,0) x (254,255); quad->material = 8
LayerTreHostImpl::DrawLayers - calling SubmitCompositorFrame ... done.
In the non-broken case, I see 2 calls to SubmitCompositorFrame in the OOPIF renderer. The first has 0 quads (although damage_rect covers the whole 800x300 surface of the frame). The second happens before the browser captures the snapshot and looks *exactly* like the one above (i.e. 2 TILED_CONTENT quads with the same dimensions).
I think the above means that maybe something is wrong on the browser-side? (although maybe I should somehow confirm that the 2 quads are indeed painting the same content in both cases).
FWIW I am still trying to trace with cc.debug.picture. Initially, I got a trace.json but without any contents (maybe I should call it disabled-by-default-cc.debug.picture?). Unfortunately, now I am not getting any trace.json file, even when using the old tracing parameters... :-/ I'll keep trying.
Are you seeing any content coming from the root renderer, or is that missing as well? If you're missing root content, you should check that DelegatedFrameHost::SubmitCompositorFrame isn't hitting ShouldSkipFrame() == true.
If it's just missing the OOPIF renderer, it's possible the captured frame is from before ChildFrameCompositingHelper::OnSetSurface was called. Maybe that call isn't triggering a new frame in the case where threading is disabled.
The root pixels are present / only the OOPIF pixels are missing (sorry for potentially misleading when saying before "all-white-pixels" - the test case I was previously using had an OOPIF filling the whole page).
The ChildFrameCompositingHelper::OnSetSurface hint was quite helpful. Before this comment, I was triggering compositing in each renderer in parallel. Now I think that maybe I need to composite parent frames/widgets before their children (?).
At any rate, right now I see the following sequence of events and still no pixels from the OOPIF get captured:
1. main renderer: Test finishes + asks the browser for a pixel dump.
2. browser: asks the main frame to trigger compositing (via new, test-only IPC / mojo method)3.
3. main renderer: Calls LayerTreeHostImpl::DrawLayers and SubmitCompositorFrame
4. browser: Gets RenderWidgetHostImpl::SubmitCompositorFrame
main renderer: Sends an ACK confirming that compositing was trigerrer
5. browser: Gets the ACK, asks the OOPIF frame to trigger compositing
6. OOPIF renderer: Calls LayerTreeHostImpl::DrawLayers and SubmitCompositorFrame
7. browser: Gets RenderWidgetHostImpl::SubmitCompositorFrame
Calls CrossProcessFrameConnector::SetChildFrameSurface
(and sends FrameMsg_SetChildFrameSurface IPC to the main renderer)
OOPIF renderer: Sends an ACK confirming that compositing was trigerrer
8. browser: Gets the ACK, posts a task 10 seconds from now
9. browser: tries to capture the pixels via web_contents()->GetRenderWidgetHostView()->CopyFromSurface
So, from the perspective of the browser we have:
A. SubmitCompositorFrame for the main frame
B. SubmitCompositorFrame for the OOPIF
C. CrossProcessFrameConnector::SetChildFrameSurface
D. 10 seconds wait
E. web_contents()->GetRenderWidgetHostView()->CopyFromSurface(...)
Does the sequence above look reasonable? Do I need to trigger compositing in the OOPIF again (after CrossProcessFrameConnector::SetChildFrameSurface)?
FWIW, going through the tree in bfs order (waiting for parents to finish before triggering compositing in children) works if I go over the frame tree twice. Having to trigger the compositing twice smells fishy, but at least know I have something that works end-to-end and can try to clean it up / code it right.
I think you want parent renderers to composite after the child renderers composite their first frame, to ensure that the parent frame actually references the child surface ids. So ChildFrameCompositingHelper::OnSetSurface should somehow be triggering a new SubmitCompositorFrame in the parent renderer. You could check if the parent compositor's frame has any SurfaceDrawQuads in it.
I'm wondering if surface_layer->GetLayerTreeHostForTesting() is nullptr, in which case we may need a layout to connect it to the right parent layers.
Or it's possible that RenderWidgetCompositor::RequestScheduleComposite() needs to be implemented so it calls delegate_->RequestScheduleAnimation(). I'm not sure why that doesn't exist.
Status update - I have a CL that succeeds in taking a pixel
dump that includes OOPIFs (while still running layout tests
without a separate compositor thread). After applying
https://codereview.chromium.org/2962073002/#ps80001 on top
of 70bd26799e43 the following layout test succeeds:
$ DISPLAY=:20 third_party/WebKit/Tools/Scripts/run-webkit-tests \
http/tests/cross-site-frame.html \
-t gn --no-retry --time-out-ms=30000 \
--additional-drt-flag=--site-per-process \
--additional-drt-flag=--no-sandbox
I've tried listing the remaining issues below. Fixing just
#1 might be sufficient to try running tryjobs. The rest are
ordered from most to least important (in my subjective
opinion).
Remaining issues:
1. There are pixel differences - https://crbug.com/740583.
- Hopefully these are not inevitable - should text
antialiasing behave the same regardless of the widget
size?
- What should we do here?
- Is allowing small (within 1/255 rgb value) pixel
differences something we would consider?
- Would we consider turninng off antialiasing for
a subset (http/tests?) of layout tests?
- Do we have other options? Can we tweak the product
code to avoid the differences altogether? (assumming
that subpixel rendering is turned off).
2. I am not sure what to do about WebWidget::GetPagePopup.
Can the browser-side handle these as well?
Not sure how to test this / which tests exercise this.
3. Waiting for composited frames should be refactored.
- We should wait on the browser side - see the
comment from kenrb@ in
SurfaceHitTestReadyNotifier::WaitForSurfaceReady
- We should delete lots of code on the renderer side:
LayoutTestDependencies, CopyRequestSwapPromise,
RenderWidget::RequestCopyOfOutputForLayoutTest,
test_runner::DumpPixelsAsync function, etc.
4. Bottom-up traversal works, but is ugly.
- I am not sure how to do better than
TriggerCompositingIfAllChildrenHaveBeenComposited
TriggerCompositing, OnTriggerCompositingResponse
in BlinkTestController
- I especially dislike
stateful tracking via composited_frames_
(this is worse than tracking for the layout dump,
because requests for pixel dumps can also come
*during* a test, *multiple* times).
5. Many pieces of code assume one-pixel-dump-at-a-time
- BlinkTestController::composited_frames_
- BlinkTestRunner::pixels_dump_callback_
Maybe mojoifying things would help here?
OTOH, maybe this restriction is okay (if 2 pixel dumps
happen simultaneously, then there is a race wrt which
state will be captured - from the first or second
SubmitCompositedFrame).
6. Browser round-trip delays when compositing happens
for the main frame. There is a risk that tests
depend on exact timing. The fallback would be
the same as we did for layout dumps - if there
is only 1 frame, then do part of the pixel dump
inside the renderer process, without the browser
round-trip (i.e. trigger compositing before
asking the browser for a pixel dump; this might
be a bit tricky wrt waiting for the next composited
frame on the browser side - item #3).
It sounds like we're not using the display compositor for layout tests and there's substantial work to do there. Exercising the display compositor during layout tests would be a win for Viz too. I wonder if we can collaborate here?
It seems worrisome to not use the display compositor for layout tests given that its absence means that all tests with an out-of-process iframes will not draw in the test as they would be displayed to the user
So I have a question: should layout tests only ever concern themselves with the operation of a single renderer?
If the answer is no, then we need a way to run the display compositor for layout tests and retrieve its output. Given our ongoing effort to relocate the display compositor, this will become more problematic.
wjmaclean@ Can you find someone in OOPIF graphics/events who can design and implement such a scheme in consultation with the people working on the display compositor?
If the answer is yes, then we should mark this bug as "won't fix"
Not at present ... other OOPIF breakages taking precedence.
Someone else can take a peek at this in the meantime if they wish (I'll assist as best I can), if it's needed for something in a hurry.
On the topic of single threaded compositing. The way we're trying to stage these changes is to capture a screenshot from the display compositor with surface sync.
I'm still investigating why we time out if we're using a single threaded compositor, but I noticed that simply specifying "--disable-threaded-compositing" for Chromium makes every renderer be blank (ie I'm not actually seeing any web content).
enne, do you know if this meant to be working, or is the intent for the the layout test controller/runner to orchestrate the interaction between the renderer in single threaded mode and the browser? Currently we just read off the pixels from the renderer, but in order for us to read those off from the browser, we need the renderer to submit frames, since we rely on synchronizing those from the browser. As far as I can tell, this simply isn't happening (in layout tests or in Chromium).
I see what you're seeing, that things aren't working and the renderer is blank. I suspect that there's something wrong at the RenderWidgetCompositor/RenderThreadImpl initialization level for this mode. It's not something we ship, so it's maybe not surprisingly that the integration doesn't work.
You are probably more of an expert in terms of how layout test controller/runner should work at this point. I thought we were still submitting frames to a TestLayerTreeFrameSink which had its own display. It seems on the face of it that you could still have the layout test drive the compositing, but then submit frames somewhere else, but I don't really know that code very well.
Sorry. I don't mean to be that grumpy about threaded layout tests. Please ignore me if you think it makes more sense to just make more tests be threaded.
I have a WIP patch: https://chromium-review.googlesource.com/c/chromium/src/+/994240 that adds a flag to do pixel dumps using the display compositor (which makes it possible for oopif pixels to be property captured). There are a few issues remaining:
I ran all of the layout tests locally on linux
493 tests failed, and 64583 passed. Out of failures:
- 185 are crashes due to the renderer not having a valid local surface id
- this seems to happen when the tests adjust the dsf of the renderer, which
does not go through a path that is supported anywhere outside of layout
tests. There is a comment here:
https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?l=1971
that is a symptom of the problem. That is, the path goes down a way that
ends up not having a valid local surface id in some cases.
- 17 timeouts, 7 of which are from wpt/css/css-paint-api
- Not sure what's going on here yet
- 3 text failures
- 1 "missing"
- 287 image failures
- ~80 seem to be rebaselines (small pixel differences on transforms)
- ~110 are exotic color spaces (the colors look wildly different with display compositor)
- the rest are either popups or selection rects, which are not implemented
in this path yet.
I'm very very curious about the CHECK you're hitting Vlad and I'd like to help debug it. You may have a local repro of the race in cc that I've been chasing for a while!
This is currently blocked on fixing crashes with --enable-display-compositor-pixel-dump enabled:
compositing/transitions/transform-on-large-layer.html
css3/filters/effect-reference-subregion-hidpi-hw.html
css3/filters/effect-reference-subregion-hidpi.html
editing/caret/caret-painting-low-dpi.html
fast/canvas/canvas-hidpi-blurry.html
fast/css-grid-layout/flex-content-sized-columns-resize.html
fast/dom/rtl-scroll-to-leftmost-and-resize.html
fast/hidpi/border-background-align.html
fast/hidpi/resize-corner-hidpi.html
fast/hidpi/video-controls-in-hidpi.html
fast/sub-pixel/shadows-hidpi.html
paint/invalidation/scroll/scrollbar-damage-and-full-viewport-repaint.html
paint/invalidation/window-resize/window-resize-background-image-generated.html
paint/invalidation/window-resize/window-resize-frameset.html
paint/invalidation/window-resize/window-resize-media-query.html
paint/invalidation/window-resize/window-resize-no-layout-change1.html
paint/invalidation/window-resize/window-resize-percent-html.html
paint/invalidation/window-resize/window-resize-percent-width-height.html
paint/invalidation/window-resize/window-resize-positioned-bottom.html
paint/invalidation/window-resize/window-resize-vertical-writing-mode.html
scrollbars/resize-scales-with-dpi-150.html
virtual/gpu/fast/canvas/canvas-filter-width-height-hidpi.html
virtual/gpu/fast/canvas/canvas-hidpi-blurry.html
(possibly more)
chrishtr@, assigning to you until that is done, please assign back to me after.
Here's a pending fix to the crashes that is due to an inadvertent feedback loop that keeps growing viewport size on Mac: https://chromium-review.googlesource.com/c/chromium/src/+/1080031
However I observed the device scale factor tests still flake on all platforms including Linux. It seems to be a race condition between resizing and snapshot taking, but it could also be that we are using a stale viewport size value. I'll continue to investigate.
Can somebody familiar with compositing please help figure out why rwhv->CopyFromSurface (called from BlinkTestController::OnInitiateCaptureDump) never comes back / never calls into BlinkTestController::OnPixelDumpCaptured when processing the 2nd URL (http://web-platform.test:8001/infrastructure/reftest/green.html)? I wasn't sure who to assign the bug to (vmpstr@ / chrishtr@ / fsamuel@).
Repro steps (kindly provided by robertma@):
1. third_party/blink/tools/run_blink_wptserve.py
2. out/rel/content_shell --run-web-tests --enable-display-compositor-pixel-dump https://web-platform.test:8444/infrastructure/reftest/reftest.https.html\'--pixel-testhttp://web-platform.test:8001/infrastructure/reftest/green.html\'--pixel-test
...
<pixel dump results from the 1st URL seem fine>
...
[56416:56416:0606/085817.942611:ERROR:blink_test_controller.cc(363)] BlinkTestController::PrepareForLayoutTest; test_url = http://web-platform.test:8001/infrastructure/reftest/green.html
[56416:56416:0606/085817.942802:ERROR:navigation_controller_impl.cc(784)] NavigationControllerImpl::LoadURLWithParams; params.url = http://web-platform.test:8001/infrastructure/reftest/green.html
[1:1:0606/085818.169392:ERROR:blink_test_runner.cc(462)] BlinkTestRunner::TestFinished
[1:1:0606/085818.169476:ERROR:blink_test_runner.cc(471)] BlinkTestRunner::TestFinished - for real!
[1:1:0606/085818.169866:ERROR:blink_test_runner.cc(507)] BlinkTestRunner::TestFinished; browser_should_capture_pixels = 1
[56416:56416:0606/085818.170272:ERROR:blink_test_controller.cc(553)] BlinkTestController::OnInitiateCaptureDump; capture_pixels = 1
[56416:56416:0606/085818.170333:ERROR:blink_test_controller.cc(557)] BlinkTestController::OnInitiateCaptureDump - during test.
[56416:56416:0606/085818.173182:ERROR:blink_test_controller.cc(957)] BlinkTestController::OnCaptureDumpCompleted
[56416:56416:0606/085818.173227:ERROR:blink_test_controller.cc(982)] BlinkTestController::ReportResults; waiting_for_pixel_results_ = 1; waiting_for_main_frame_dump_ = 0
...timeout eventually...
Update: I'm still investigating why pixel dumps time out when a renderer process is switched out.
It seems to me that the new renderer process were launched correctly and generating new frames, and in DisplayScheduler the new surface were marked ready to draw. But the scheduler keeps wake up at 60 FPS without drawing anything. Maybe the root surface hasn't update its reference to the new renderer?
Okay I think it is a frame deadlock in the browser side.
When the new renderer process swapped in, it did submit a new frame and told browser process its surface ID, and the browser compositor invoked SurfaceLayer::SetPrimarySurfaceId().
However a main frame commit never happened on the browser compositor because it is throttled due to a pending frame previously submitted to the surface. The pending frame doesn't have a deadline (probably because in layout test we never want to checkerboard?), and cannot be activated because it depends on the old renderer surface. I think the throttling is to avoid the live lock due to chasing a moving target, but with no deadline it becomes a dead lock instead.
What if I disable draw throttling in layout tests? What can go wrong?
I'm confused...what code path is this...this doesn't sound like surface synchronization. Typically the browser allocates the surface ID and passes it to the renderer, not the other way around. Could you please point at the code.
Ah, I see why you're confused... I couldn't find anywhere we set an infinite deadline if not in surface synchronization mode... Now I'm confused too. Why the pending frame never activated? Investigating...
There are a few things I don't understand which I think is related to the deadlock:
1. When the old renderer process dies, it told the surface manager to destroy the surface, but that doesn't immediately unblock pending frames that depend on it. Why is that? Can we notify the dependency tracker in SurfaceManager::DestroySurface() instead of SurfaceManager::SurfaceDiscarded()?
2. When the surface actually get discarded, i.e. when SurfaceDependencyTracker::OnSurfaceDiscarded() is invoked, it only unblock others when the surface itself has a pending frame.
https://cs.chromium.org/chromium/src/components/viz/service/surfaces/surface_dependency_tracker.cc?rcl=1f62e26d8bca1004bd58a24f15f4e505d8a62a3d&l=76https://cs.chromium.org/chromium/src/components/viz/service/surfaces/surface_dependency_tracker.cc?rcl=1f62e26d8bca1004bd58a24f15f4e505d8a62a3d&l=101
Shouldn't it always unblock everyone else? Also I think we should set the local_surface_id_ to UINT_MAX in the parameter to NotifySurfaceIdAvailable, because we should unblock all references to a potential future frame?
That said, even with those changes I'm still not able to disentangle the deadlock. I wonder what else I missed here. :/
I've run the bots on https://chromium-review.googlesource.com/c/chromium/src/+/1075891/3 again and looked at the results. Among others:
https://test-results.appspot.com/data/layout_results/linux-blink-rel/323/layout-test-results/results.html
There aren't any crashes any more, but still plenty of test changes that look to me like regressions. There are 29 new timeouts related to scrolling or input that seem like real regressions that could plausibly be caused by OOPIF. Many of the things classified as "Harness failure" also seem like real problem.
It's only looking at pixel tests that I see things that are clearly not regressions but just tiny changes in rastering. But among the pixel tests are also real regressions, like lots of forms tests where the dropdowns are no longer open.
I'll assign back to trchen@ to look at the results and to keep triggering the linux-blink-rel bot again until the remaining regressions are ones that we should accept. You should be able to land a CL like https://chromium-review.googlesource.com/c/chromium/src/+/1075891 by using the rebaseline-cl script, but please let me know if you need further assistance with that final step.
Stephen could you please look through the test results and triage ones that look like
real failures? Of so we need to find owners for these bugs outside the paint team.
It will take some time to try to match these failures up with existing known issues with OOPIF. Here's an initial pass.
Could be rebaselined:
animations/rotate-transform-equivalent.html
compositing/3d-corners.html
compositing/direct-image-compositing.html
compositing/flat-with-transformed-child.html
compositing/lots-of-img-layers-with-opacity.html
compositing/lots-of-img-layers.html
compositing/perpendicular-layer-sorting.html
compositing/perspective-interest-rect.html
compositing/video-frame-size-change.html
compositing/geometry/fixed-position-transform-composited-page-scale-down.html
compositing/geometry/fixed-position-transform-composited-page-scale.html
compositing/geometry/layer-due-to-layer-children-deep.html
compositing/geometry/layer-due-to-layer-children.html
compositing/geometry/vertical-scroll-composited.html
compositing/masks/mask-with-removed-filters.html
compositing/overflow/border-radius-styles-with-composited-child.html
compositing/overflow/mask-with-filter.html
compositing/overflow/nested-render-surfaces-with-rotation.html
compositing/overflow/scaled-overflow.html
compositing/overflow/tiled-mask.html
compositing/reflections/nested-reflection-anchor-point.html
css3/blending/background-blend-mode-overlapping-accelerated-elements.html fast/borders
fast/clip
media/video-layer-crash.html
media/video-zoom-controls.html
paint/invalidation/scroll/scrollbar-damage-and-full-viewport-repaint.html
transforms/3d/
virtual/android/fullscreen/full-screen-iframe-allowed-video.html
virtual/android/fullscreen/video-controls-timeline.html
virtual/android/fullscreen/video-scrolled-iframe.html
virtual/prefer_compositing_to_lcd_text/compositing/overflow/border-radius-styles-with-composited-child.html
virtual/prefer_compositing_to_lcd_text/compositing/overflow/mask-with-filter.html
virtual/prefer_compositing_to_lcd_text/compositing/overflow/nested-render-surfaces-with-rotation.html
virtual/prefer_compositing_to_lcd_text/compositing/overflow/scaled-overflow.html
virtual/prefer_compositing_to_lcd_text/compositing/overflow/tiled-mask.html
Just not there:
hdr/
Missing focus rings? Some other content?
editing/selection
paint/invalidation/svg/text-selection-text-05-t.svg
svg/text/
Missing content entirely:
virtual/android/fullscreen/rendering/backdrop-video.html
Not scrolling, same as other event issues?
virtual/android/rootscroller/set-rootscroller-before-load.html
Color spaces are busted on images. Real failure.
virtual/exotic-color-space/images
Incorrect position for elements. Real failure.
media/controls-after-reload.html
media/controls-strict.html
media/controls-styling-strict.html
media/controls-styling.html
media/controls-without-preload.html
media/video-controls-rendering.html media/video-display-toggle.html
media/video-no-audio.html
media/controls/
virtual/new-remote-playback-pipeline/media/controls/
Dropdown option forms not dropping down. Events? Locations?
compositing/overflow/update-widget-positions-on-nested-frames-and-scrollers.html
fast/forms/calendar-picker/
fast/forms/color/
fast/forms/select-popup/
fast/forms/suggestion-picker/
http/tests/media/controls/video-controls-overflow-menu-correct-ordering.html
http/tests/media/controls/video-controls-overflow-menu-updates-appropriately.html
http/tests/webfont/popup-menu-load-webfont-after-open.html
virtual/prefer_compositing_to_lcd_text/compositing/overflow/update-widget-positions-on-nested-frames-and-scrollers.html
virtual/scalefactor150/fast/hidpi/static/*
virtual/scalefactor200withzoom/fast/hidpi/static/*
virtual/new-remote-playback-pipeline/http/tests/media/controls/ (probably)
Some animations not completing or timing out:
virtual/threaded/animations/responsive/viewport-unit-transform-responsive.html
virtual/threaded/animations/responsive/viewport-unit-translate-responsive.html
virtual/threaded/animations/svg-attribute-composition/svg-startOffset-composition-003.html
virtual/threaded/animations/timing/animation-duration-infinite.html
Devtools Timeline doesn't have paint data about subframes.
http/tests/devtools/tracing/frame-model-instrumentation.js
virtual/threaded/http/tests/devtools/tracing/
Issues with autoscroll, middle mouse in particular, and pinch zoom seem real and related:
fast/events/
virtual/threaded/fast/events/pinch/
virtual/threaded/synthetic_gestures/synthetic-pinch-zoom-gesture-touchscreen-desktop.html
virtual/user-activation-v2/fast/events/*
virtual/mouseevent_fractional/fast/events/*
Printing probably not blocking:
virtual/threaded/printing/webgl-repeated-printing-preservedrawingbuffer.html
More investigation required:
virtual/threaded/synthetic_gestures/synthetic-pinch-zoom-gesture-touchscreen-desktop.html
Still to look at:
Bugs fall into a few classes:
- Issues with events, might also be the dropdown problem
- Animation issues
- hdr issues
- Media controls that are possibly paint
- Color profiles on images (direct compoSited content?)
- Selection focus rings missing (paint?)
I'll look to assigning owners.
Ok, with the fix for middle-click autoscroll landed (https://chromium-review.googlesource.com/c/chromium/src/+/1229354), the list of still-failing layout tests is down to those listed in crbug.com/891427. There are 6 tests that fail on all platforms, 17 that are single-platform-specific, and 10 that appeared flaky in my testing. Some of the above may have been pre-existing conditions.
My preference at this point would be to make --enable-display-compositor-pixel-dump the default for all tests, and send out a PSA referencing the tests that now fail, to see who might be willing/able to fix them. But schenney@, I leave that decision up to you. You can see the list of additions I had to make to TestExpectations in https://chromium-review.googlesource.com/c/chromium/src/+/1213864.
Let me know your thoughts, and in particular, let me know if you want me to land 1213864 as-is.
You should definitely go ahead and make the switch and leave the remaining few tests to be fixed, recording in TestExpectations. The testing of OOPIF is more important at this point.
Thanks for all the work getting it resolved.
The following revision refers to this bug:
https://chromium.googlesource.com/chromium/src.git/+/c41b2d08296c7d720343f716641e40c9c2f68ad0
commit c41b2d08296c7d720343f716641e40c9c2f68ad0
Author: Mason Freed <masonfreed@chromium.org>
Date: Fri Oct 26 17:20:21 2018
Enable display compositor pixel dumps by default.
SHERIFFS: PLEASE DO NOT REVERT THIS CL BECAUSE OF A SMALL AMOUNT OF LAYOUT
TEST FLAKINESS. If a few layout tests begin to show flakiness after this CL
lands, please add them to TestExpectations and email me (masonfreed@) to
investigate. Though I tried to identify all potentially flaky tests, a few
may have slipped through.
With this CL, the --enable-display-compositor-pixel-dump flag becomes the default
for content_shell. With this flag in place, layout test pixel dumps are performed
from the browser side, instead of from the renderer side. Note that to avoid a
significant amount of layout test flakiness, another change was also made to
not add the --run-all-compositor-stages-before-draw flag by default. There is
a bug (crbug.com/894613) tracking that problem separately.
With the flip of this switch, several modifications had to be made to the
TestExpectations file. First, there are a number of tests that change their
appearance slightly when being captured from the browser, and these tests need
to be rebaselined. These are summarized below, and will be rebaselined as a
separate CL, once this one lands and has had time to stabilize.
These bugs track the items added to TestExpectations:
- crbug.com/887140: HDR support
- crbug.com/881040: Media controls now contain an overflow menu.
- crbug.com/667551: A bunch of tests are listed under this bug, and just
require rebaselining to fix non-material single-pixel
antialiasing failures.
- crbug.com/891427: These either start failing, or become flaky, when the
--enable-display-compositor-pixel-dump flag is enabled.
They need to be debugged prior to re-enabling.
- crbug.com/895556: These tests double their background size when the flag
is enabled. They need to be fixed or rebaselined.
Bug: 667551, 891427, 881040, 887140, 894613, 895556
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: I46946e6377f659c9dedc0dfaa20e7658e8cc519d
Reviewed-on: https://chromium-review.googlesource.com/c/1213864
Commit-Queue: Mason Freed <masonfreed@chromium.org>
Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Stephen Chenney <schenney@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603127}
[modify] https://crrev.com/c41b2d08296c7d720343f716641e40c9c2f68ad0/content/shell/app/shell_main_delegate.cc
[modify] https://crrev.com/c41b2d08296c7d720343f716641e40c9c2f68ad0/docs/testing/writing_layout_tests.md
[modify] https://crrev.com/c41b2d08296c7d720343f716641e40c9c2f68ad0/third_party/WebKit/LayoutTests/TestExpectations
I would like to mark this fixed. See https://crrev.com/c/1213864. There is a tracking bug, https://crbug.com/895912, which tracks a few remaining odds and ends. I'll be monitoring that one. If you find problems, please report them there, not here.
