VideoRendererImpl::FrameReady() could be called after reinitialization failure |
||
Issue descriptionIn VideoRendererImpl, we could pass in FrameReady() or FrameReadyForCopyingToGpuMemoryBuffers() as the callback for VideoFrameStream::Read(). FrameReadyForCopyingToGpuMemoryBuffers() wraps FrameReady() and can call it asynchronously. There's a chance where after FrameReadyForCopyingToGpuMemoryBuffers() is called (read completed), VideoFrameStream tries to read another buffer which hit config change and cause the decoder to reinitialize. The reinitialization could succeed or fail. If it fails, we may not have a valid decoder (after reinit failure, we'll try to select another decoder, which may fail). Then FrameReady() could get called. At this point, VideoFrameStream() doesn't have a valid decoder, and we are in a tricky state. For example, we could call video_decoder_stream_->CanReadWithoutStalling() which will call decoder_->CanReadWithoutStalling() and cause access violation. Here's a sample log: [64255:98827:0106/233008.991763:48920433038268:ERROR:video_renderer_impl.cc(357)] FrameReadyForCopyingToGpuMemoryBuffers: status=0 [64255:98827:0106/233008.991867:48920433141932:ERROR:decoder_selector.cc(232)] ReturnNullDecoder [64255:98827:0106/233008.991986:48920433260700:ERROR:video_renderer_impl.cc(375)] FrameReady BrowserTestBase received signal: Segmentation fault: 11. Backtrace:
,
Jan 12 2017
Sorry I wasn't super clear in the bug description. When I filed the bug it's a bit late so I didn't provide as much detail as I should. The problem is as follows: 1. DecoderStream::Read() succeeded so FrameReadyForCopyingToGpuMemoryBuffers() is called, which calls FrameReady() asynchronously (in step 6). 2. Internally DecoderStream tries to read the DemuxerStream again and hit a config change. 3. Upon receiving the config change, DecoderStream tries to reinitialize the existing decoder. 4. Decoder reinitialization failed. So it tries to select another decoder, which also failed. 5. At this time, there's valid decoder in the DecoderStream: |decoder_| is null. 6. Finally FrameReady() is called (see step 1). Since it's a successful read, we could end up calling video_decoder_stream_->CanReadWithoutStalling() [1] which will try to call decoder_->CanReadWithoutStalling() and cause a crash. It seems to me whether the |decoder_| is null or not is an implementation detail of the DecoderStream, and the caller (e.g. VRI) should not worry about it. So maybe we should just check |decoder_| before using it in video_decoder_stream_->CanReadWithoutStalling()? [1] https://cs.chromium.org/chromium/src/media/renderers/video_renderer_impl.cc?rcl=0&l=394
,
Jan 12 2017
That was going to be my other suggestion, though I worry if there are other methods which might also need the null decoder check then. I guess at step #5, we don't have a decode_cb, so we can't return an error?
,
Jan 18 2017
Give to xhwang@ to start rolling ball. |
||
►
Sign in to add a comment |
||
Comment 1 by dalecur...@chromium.org
, Jan 9 2017