yeah, that's what's going on.  here is part of the trace:

// VideoFrame is sent from VideoFrameSubmitter to the browser compositor.
[1:16:1215/132501.699424:ERROR:VideoFrameSubmitter.cpp(111)] AVDA: SubmitFrame

// test starts an async pixel dump
[1:1:1215/132501.702970:ERROR:test_runner.cc(1772)] AVDA: DumpPixelsAsync

// Not directly relevant, but kind of interesting.
[1:1:1215/132501.840341:ERROR:single_thread_proxy.cc(136)] AVDA: SetLayerTreeFrameSink 0x243970fb8280
[1:1:1215/132501.844228:ERROR:surface_manager.cc(54)] AVDA: SurfaceManager 0x2439711cf150
[1:1:1215/132501.844320:ERROR:frame_sink_manager_impl.cc(55)] AVDA: FrameSinkManagerImpl 0x2439711cf000

// Pixel dump completes
[1:1:1215/132501.848597:ERROR:render_widget_compositor.cc(1242)] AVDA: WillCommit

// The CompositorFrame with the VideoFrame is drawn.
[1:1:1215/132501.851354:ERROR:gl_renderer.cc(544)] AVDA: BeginDrawingFrame
[1:1:1215/132502.177198:ERROR:gl_renderer.cc(2600)] AVDA: FinishDrawingFrame
[1:1:1215/132502.177356:ERROR:gl_renderer.cc(2627)] AVDA: FinishDrawingFrame

i made the test completion postdelayed(5 seconds).  however, the GLRenderer draw that contains the video frame was still ordered after it, for some reason.

// video frame is sent by VFS.
[1:16:1215/145706.959337:ERROR:VideoFrameSubmitter.cpp(111)] AVDA: SubmitFrame

// this is when the test would normally complete.  it now does a PostDelayed(5 seconds) before it completes.
[1:1:1215/145706.962878:ERROR:test_runner.cc(1574)] AVDA: ProcessWorkSoon

// test finishes.  Note the +5sec time difference.
[1:1:1215/145711.963197:ERROR:blink_test_runner.cc(569)] AVDA: TestFinished
[1:1:1215/145711.963712:ERROR:blink_test_runner.cc(794)] AVDA: CaptureDump

// it creates a new surfacemananger and fame sink manager.  not sure why.
[1:1:1215/145711.983072:ERROR:surface_manager.cc(54)] AVDA: SurfaceManager 0x1ca715a44150
[1:1:1215/145711.983299:ERROR:frame_sink_manager_impl.cc(55)] AVDA: FrameSinkManagerImpl 0x1ca715a44000

[1:1:1215/145711.983072:ERROR:surface_manager.cc(54)] AVDA: SurfaceManager 0x1ca715a44150
[1:1:1215/145711.983299:ERROR:frame_sink_manager_impl.cc(55)] AVDA: FrameSinkManagerImpl 0x1ca715a44000

// image dump completes.
[1:1:1215/145711.993639:ERROR:render_widget_compositor.cc(1242)] AVDA: WillCommit

// now it starts drawing the new frame.
[1:1:1215/145711.998996:ERROR:gl_renderer.cc(544)] AVDA: BeginDrawingFrame

I wonder if what we are seeing with the gpu constantly waiting has something to do with this.

i'm not sure.

my guess at this point is that something like this happens:

- somehow (don't know how), shutdown of the test is postponed until the first frame of the video is @ VideoFrameCompositor.
- shutdown of the test forces a redraw of the cc layer tree and waits for it to commit
- this redraw will deterministically have the right VideoFrame drawn, since VideoLayerImpl will fetch it from VFC.

if this is true, then with cc::surfaces, it'll now have a race.  the last point above won't fetch the video frame anymore.  it'll depend on when VideoFrameSubmitter gets around to sending it and it gets processed by viz.

but, those are lots of guesses.

the test ends when the document says that it's done loading, unless the test specifically requests otherwise.  this test does not request it.

i'm not exactly sure what allows it to proceed (seems like it checks for a bunch of different conditions), but at some point, the network state changes to idle.  i suspect that it waits until the network requests have stopped, which does seem to happen.  multibuffer (== media's top-level media buffering system) conveniently closes the last loader request shortly before the loading progress goes to "done", and the document signals that the document is loaded.

i suspect that this isn't an accident.

what i don't understand is how it knows that a video frame has been decoded at this point.  in fact, it has been, but i don't see any synchronization between "the demuxer stops requesting data" and "the decoder hands a video frame to the video frame compositor".

so, i'm not sure why this isn't a race even in the normal case.  i might try making the decoder take longer or something, to see if i can break the test without cc::surfaces.  the cc::surfaces case is harder than the original case, but at least it'll tell us if there is some existing sync mechanism we can try to use for cc::surfaces.

ah -- it stops when it's decoded enough frames in the video renderer.  so it isn't async in the normal case.  i should have remembered that.  :)

to recap, here's what i think is happening:
- document starts loading
- wmpi starts the pipeline, starts network requests
- demuxing / decoding happens until >0 frames are processed by VideoRendererImpl
- demuxer is stopped
- document loading is completed.
  - not sure if this happens because multibuffer shuts of the request, or
  - if it somehow sets the network state of the media element in response.
  - doesn't matter, really.
- test runner believes that the test has finished.
- test starts pixel dump
- VideoLayerImpl is run one more time as part of the pixel dump
- VFC provides a frame
- success.

in the cc::Surfaces case, the nothing in the layer tree gets a frame from VFC.  it has no idea if the surface it's embedding has a video frame yet or not.  in fact, i don't think we even know that VFS has submitted the frame.  we know that it's posted a task to the media thread to do so, but we have no idea if that post has run yet (i think).

it happens that VFS has sent a frame in my local tests, but it's not displayed yet.  not sure if SurfaceLayerImpl needs to take action as a result of this first frame or not.  i don't remember.

plus, it's still creating a new surface manager after starting the pixel dump.  no idea why, but that seems like no amount of synchronization will fix it until we fix that.

for example, making SurfaceLayerImpl::AppendQuads wait for a second before doing work isn't enough.

the second surface_manager is coming from the RenderWidgetCompositor.

this has to do with the readback.  however, the RWC is created very early in the test.  it doesn't create a new surface manager at that point.

also note that the offscreen canvas provider runs in the same pid that creates the first surface manager (browser, i guess).  this isn't the same one as RWC, which is the renderer.

so, it looks like the renderer is fiddling with the LTH to get a screen shot (locally, since gl_renderer runs in the renderer the second time around but the browser the first time), but that doesn't help much if some of the state needed to create the screen shot has been sent to the browser already.

now, what's a little odd is that the dump pixles code specifically mentions out-of-process iframes.  it seems like those would not work at all either.  i'll see if it does something special in that case.  maybe we need to do that too.

not sure why it isn't just letting the browser do the screen shot.

ah, the OOPIF stuff is a todo that says "ask the delegate to do this"... :)

https://cs.chromium.org/chromium/src/content/shell/test_runner/test_runner.cc?rcl=9161f6b9435b297e25da90ddf6d226b9fd9bbb34&l=1813

the delegate does not expose anything like this currently, i think.

so, it looks like we'll have to add it if we want these tests to pass, or else we'll have to teach the VideoFrameSubmitter and friends to deal with the switch.  persoinally, i prefer the former.

separately, we'll also need to make sure that there's some synchronization between VFS and the test finishing -- we need to be sure that the CompositorFrame for the current VideoFrame has been processed by the browser before we finish the test.  otherwise, the browser might still not have the frame when it reads back the pixels.

i guess there's one other option: switch to a local surface manager immediately, such as when RWC is created in the renderer.

cc'ing some relevant folks

here's where layout tests make their LayerTreeFrameSink for the renderer: https://cs.chromium.org/chromium/src/content/test/layouttest_support.cc?rcl=1663dc2635b74978b02e921680992732f1a16b96&l=336

it makes a TestLayerTreeFrameSink, which has its own Display compositor built into it.

The whole layout test harness is extremely legacy, from before the time compositing even existed, and we've managed to build compositing into it, in a way that it was designed, to produce pixels in the renderer process. Moving it to the browser process requires some rework of the IPC relationship between renderer/browser side of the test harness, but would let video and html stuff all go to the same place.

Alternatively, video can be re-routed for layout tests to this test LayerTreeFrameSink's Display. Presumably the mojo bindings can be set up to do that in layout tests.

Dana, do you know of a good person who could take up this work? I am a little out of my depth here.

Assigning to danakj@ for triage per above comment. +enne@ for visibility.

Updating priority as it's a launch blocker for the feature.

I don't, it's not part of the graphics teams plans at this time. Maybe someone from the blink rendering/OOPIF teams can own this.

We don't have cycles for this work at the moment but I  think this work is important to validate some of the stuff we're doing for Viz. Viz cannot launch without site isolation support and stability of site isolation requires support from Viz folks. I'd like to see this work happen and owned by....someone, so I'll raise awareness of it with relevant parties in an email. Thanks!

Is this bug related to / a dup of 667551?

Yeah, it is. It's not "too soon" its just excluding child surfaces atm.

FWIW dupe of 667551 isn't quite accurate here, I realized. It's more of a subset.

667551 would resolve this, but redirecting video to the layout test display compositor in the renderer process, and synchronizing to wait for child surfaces, would also work for video/in-process surfaces.