Surfaces for Videos Crash after renabling gpu |
||||
Issue descriptionChrome Version: 66.0.3348.0 (Developer Build) (64-bit) OS: Linux What steps will reproduce the problem? (1) Build and run Chrome with --enable-features=UseSurfaceLayerForVideo. Point to any youtube video. See that it runs fine. (2) Run chrome with --enable-features=UseSurfaceLayerForVideo --disable-gpu. Point to any youtube video. See that it doesn't crash, but no video. (3) Run again with just --enable-features=UseSurfaceLayerForVideo. Point to any youtube video. What is the expected result? Video should play as expected. What happens instead? Video studders, and the tab crashes.
,
Feb 16 2018
So the video chugs after following these steps and then crashes because it hits a race condition between |is_renderering_| being set to true and StopRendering being called. Both these calls get post to the media thread so I'm not sure why they don't happen in sequence, but if one changes the DCHECK around |is_renderering_| to a conditional, the video recovers and then plays smoothly. It updates the state back to renderering and resumes. I'll upload a CL shortly to make this more clear. The CL seems to address the symptom but not the cause in this case. I am not sure what would make this chug though like this.
,
Feb 16 2018
So, what it seems like is that we have a race case between EnableSubmission() (where the previously null |client_| now points to |submitter_| and StopRendering(). The sequence is as follows: - We update the VideoFrameCompositor to start rendering in OnRendererStateUpdate, but since we do not have a client yet, we return. - We provide a client via EnableSubmission - We call OnRendererStateUpdate again to stop rendering. - Because we now have a client, the call occurs, but |is_rendering_| is still false, and now we hit this check. The CL here https://chromium-review.googlesource.com/c/chromium/src/+/923709 puts a bandaid on this problem as described previously. I still don't know why this now happens when you switch disable-gpu on and off.
,
Feb 16 2018
Would it make sense to have the bandaid landed with a test in order to resolve the fix or do we suspect this might be the rootcause of a larger problem?
,
Feb 16 2018
re c#3: good summary! here's what bothers me. most of that seems exactly the same in the non-surfaces path: we still create VideoLayer in WMPI::OnMetadata, which still posts to the compositor thread to create VLI (i think), which still registers as a client to VFC. all of that seems extremely similar to the surfaces path with respect to the race condition. in VideoFrameProviderClientImpl::StopRendering, there's a similar dcheck. yet, we've not run into this on the non-surfaces path. any idea what the difference is? is it that we're now serialized on the media thread rather than fully async on the compositor thread, and we were just getting lucky? if so, that seems testable.
,
May 25 2018
Isn't this fixed?
,
Jun 7 2018
So inadvertently we handle the race condition described by now checking for the compositor_frame_sink_ (https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/graphics/video_frame_submitter.cc?rcl=022934873f6e8e16f07e36fabe2bebc1635e91c3&l=84). Since this is only set when we have called StartSubmitting, we don't need to worry. I checked to see if the problem still exists top of tree, and I don't think I'm able to repro it. However, I am still puzzled as to why the repro steps would have triggered this in the first place. |
||||
►
Sign in to add a comment |
||||
Comment 1 by lethalantidote@chromium.org
, Feb 15 2018