DecoderStream can drop valid demuxer buffers when falling back to a new decoder. |
||||
Issue descriptionI set up GpuVideoDecoder to fail 10%, and held the refresh button for a few minutes to see if I ever got into the overlapping demuxer reads from 613958. Instead I ran into a DCHECK on this line: https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/decoder_stream.cc&q=decoder_stream&sq=package:chromium&type=cs&l=586 When we were in STATE_NORMAL This was not caught earlier mainly because this line: https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/decoder_stream.cc&q=decoder_stream&sq=package:chromium&type=cs&l=546 is not a condition, and will never be false (it is missing the 'state_ ==') This lead me to realize that, currently, the following scenario can happen: - DecoderStream requests buffers from the demuxer (STATE_PENDING_DEMUXER_READ). - |decoder_| returns a DECODE_ERROR before a frame was outputted, initiating a decoder fallback (STATE_REINITIALIZING_DECODER). - OnDecoderSelected() successfully completes (STATE_NORMAL). - OnBufferReady() is called back (the typo in the DCHECK meant we never considered when we enter the function in STATE_NORMAL). We return at line 601 because the state is not STATE_PENDING_DEMUXER_READ, and drop the buffer.
,
Jun 2 2016
,
Jun 30 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/62136bd4d4f641a0f516badd93c200eaf2bca285 commit 62136bd4d4f641a0f516badd93c200eaf2bca285 Author: Dale Curtis <dalecurtis@chromium.org> Date: Thu Jun 30 01:35:42 2016 Merge M52: "Fix dropped demuxer buffers for fallback decoder" An edge case was missed due to a typo in a DCHECK, during the original fallback decoder submission. Currently, the following scenario can happen: - DecoderStream requests buffers from the demuxer (STATE_PENDING_DEMUXER_READ). - |decoder_| returns a DECODE_ERROR before a frame was ouptutted, initiating a decoder fallback (STATE_REINITIALIZING_DECODER). - OnDecoderSelected() successfully completes (STATE_NORMAL). - OnBufferReady() is called back (the typo in the DCHECK meant we never considered when we enter the function in STATE_NORMAL). We return at line 601 because the state is not STATE_PENDING_DEMUXER_READ, and drop the buffer. This CL fixes the typo, and changes so we only drop the buffer whilst in STATE_ERROR, and also handles the case where we had saved buffers in the fallback buffer queue. BUG=597605, 615141 TEST= Setup GpuVideoDecoder to fail 10% and refreshed the page for several minutes. Hard to test the end to end though... Review-Url: https://codereview.chromium.org/2012293002 Cr-Commit-Position: refs/heads/master@{#396371} (cherry picked from commit 4849043c3e003ef57a431552f9a97cf159debd06) Review URL: https://codereview.chromium.org/2109083003 . Cr-Commit-Position: refs/branch-heads/2743@{#541} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} [modify] https://crrev.com/62136bd4d4f641a0f516badd93c200eaf2bca285/media/filters/decoder_stream.cc
,
Jul 13 2016
tguilbert@, can you please let us know if this can be manually tested ? If so, please provide us with the steps so that we can verify it at our end ?
,
Jul 13 2016
In order to test it, you would have to compile a version of Chrome where the HW accelerated decoders arbitrarily fail some % of the time (10% is what I for example). Then, within 5 minutes of holding the refresh button and trying to play a video format that supports HW acceleration, Chrome would crash. I had done some manual testing, and with the fix, I was not able to repro anymore (using the same setup that found the bug, Chrome didn't crash within about 10 minutes of refreshing). Furthermore, Dale's fix for 597605 removed STATE_REINIALIZING_DECODER, which was one of the ingredients necessary for running into this bug. I think it is acceptable to mark this bug as Verified (if that was the intent of your question). |
||||
►
Sign in to add a comment |
||||
Comment 1 by bugdroid1@chromium.org
, May 27 2016