Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11745012:
Handle config change during pending reset correctly in FFmpegVideoDecoder. (Closed)
|
Created:
7 years, 11 months ago by xhwang Modified:
7 years, 11 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionHandle config change during pending reset correctly in FFmpegVideoDecoder.
As per VideoDecoder API, "After this (Reset) call, the decoder is back to an
initialized clean state", which means that the render should be able to call
decoder->Read() again. To achieve this, the decoder should be initialized with
the new config even if it's in a pending reset.
BUG=168128
TEST=Added unittest to cover this.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175202
Patch Set 1 #
Total comments: 8
Patch Set 2 : comments resolved #Patch Set 3 : change init config to vp8 as h264 is not supported on all platforms #Patch Set 4 : add more comments #
Messages
Total messages: 18 (0 generated)
PTAL!
I am waiting for feedback on this to continue updating DecryptingXXX. A quick comment about whether I am on the right track would be greatly appreciated.
Backing up a bit... before the break we chatted about how to simplify handling pending reads. Do you happen to recall the details of that conversation? I had a feeling it help handle cases like this one, but my memory is a bit foggy.
On 2013/01/03 16:20:44, scherkus wrote: > Backing up a bit... before the break we chatted about how to simplify handling > pending reads. Do you happen to recall the details of that conversation? I had a > feeling it help handle cases like this one, but my memory is a bit foggy. uhh, my memory is even foggier. Is that about reset not waiting for pending reads? I guess this CL only covers a corner case that probably doesn't have any use case. Reset() is mainly (if not solely) used for seeking. IIUC, based on MSE spec (http://dvcs.w3.org/hg/html-media/raw-file/tip/media-source/media-source.html#...), after seeking, "The media element resets all decoders and initializes each one with data from the appropriate initialization segment". In that case, the current implementation should work. Is this correct? +acolwell for comments. That said, this CL still has its value, as the decoders knows nothing about the demuxer and MSE spec, and should implement VideoDecoder API faithfully. If we need more discussion or bigger changes on this, I might go ahead and commit my change in DecryptingDemuxerStream (https://codereview.chromium.org/11722008/) first, which fixes a crash and needs to be merged to M25.
OK -- so really this change is more about always respecting a config change whenever we see one It is a bit confusing though... we're Reset()'ing to seek but we do a config change right before we signal reset and crossing our fingers that subsequent buffers for the demuxer are for the new config?
On 2013/01/03 17:30:26, scherkus wrote: > OK -- so really this change is more about always respecting a config change > whenever we see one > > It is a bit confusing though... we're Reset()'ing to seek but we do a config > change right before we signal reset and crossing our fingers that subsequent > buffers for the demuxer are for the new config? I agree it's confusing :) But from API's perspective, it's a legitimate combination. Reset() is only a concept for the decoder. The demuxer stream knows nothing about the Reset(). To the demuxer stream, after a kConfigChange, the next frame is associated with the new config. In reality, decoder Reset() is always associated with Seek() in the demuxer. So even though the above case is legitimate, we probably won't really hit it...
CC'ed ddorwin for his information.
In general the changes look good, but this raises a few questions for me. 1. Shouldn't a failed config change cause a kDecoderError to be reported for all future Read()s? 2. Should a Reset() clear the decode error state or keep it in place for the life of the object? 3. Should the decoder have a DCHECK to verify that a caller doesn't call Read() after the decoder has returned a kDecoderError? https://codereview.chromium.org/11745012/diff/1/media/filters/ffmpeg_video_de... File media/filters/ffmpeg_video_decoder.cc (right): https://codereview.chromium.org/11745012/diff/1/media/filters/ffmpeg_video_de... media/filters/ffmpeg_video_decoder.cc:239: state_ = kDecodeFinished; Why is this needed now? The Read() shouldn't be called again after kDecodeError is returned. If anything we should transition to a state that always returns kDecodeError for all future Read() calls until a Reset() happens. https://codereview.chromium.org/11745012/diff/1/media/filters/ffmpeg_video_de... media/filters/ffmpeg_video_decoder.cc:245: if (!reset_cb_.is_null()) { nit: If you flip the condition here you can reuse the DoReset() code below instead of duplicating it here. https://codereview.chromium.org/11745012/diff/1/media/filters/ffmpeg_video_de... File media/filters/ffmpeg_video_decoder_unittest.cc (right): https://codereview.chromium.org/11745012/diff/1/media/filters/ffmpeg_video_de... media/filters/ffmpeg_video_decoder_unittest.cc:537: EXPECT_CALL(*demuxer_, Read(_)) It would be nice if this was slightly reworked so that it verifies that at least one video_decoder_config() call happens between the kConfigChanged callback and the next read. https://codereview.chromium.org/11745012/diff/1/media/filters/ffmpeg_video_de... media/filters/ffmpeg_video_decoder_unittest.cc:602: } A test should also be added to test the config failure with pending reset path.
On 2013/01/03 17:53:31, acolwell wrote: > In general the changes look good, but this raises a few questions for me. > > 1. Shouldn't a failed config change cause a kDecoderError to be reported for all > future Read()s? The current implementation says the state transits from kNormal to kDecodeFinished when a decoding error occurs and decoding needs to stop. Then it says all (Read) calls return empty frames when the state is kDecodeFinished. I agree this isn't great, but if we want to improve this, we probably should do that in another CL. To fix this, how about adding a kError state specific for the error state, instead of abusing kDeocdeFinished? > 2. Should a Reset() clear the decode error state or keep it in place for the > life of the object? At least for now, we don't recover from decode error. Do we want this? > 3. Should the decoder have a DCHECK to verify that a caller doesn't call Read() > after the decoder has returned a kDecoderError? This is related to 1). If we add a kError state, this will be easily implementable. For now, we return empty frames after error.
https://codereview.chromium.org/11745012/diff/1/media/filters/ffmpeg_video_de... File media/filters/ffmpeg_video_decoder.cc (right): https://codereview.chromium.org/11745012/diff/1/media/filters/ffmpeg_video_de... media/filters/ffmpeg_video_decoder.cc:239: state_ = kDecodeFinished; On 2013/01/03 17:53:31, acolwell wrote: > Why is this needed now? The Read() shouldn't be called again after kDecodeError > is returned. If anything we should transition to a state that always returns > kDecodeError for all future Read() calls until a Reset() happens. This transition follows the comment on line 298. I agree that kDecodeFinished may not be the best option. But that'd be in another CL. https://codereview.chromium.org/11745012/diff/1/media/filters/ffmpeg_video_de... media/filters/ffmpeg_video_decoder.cc:245: if (!reset_cb_.is_null()) { On 2013/01/03 17:53:31, acolwell wrote: > nit: If you flip the condition here you can reuse the DoReset() code below > instead of duplicating it here. Done. https://codereview.chromium.org/11745012/diff/1/media/filters/ffmpeg_video_de... File media/filters/ffmpeg_video_decoder_unittest.cc (right): https://codereview.chromium.org/11745012/diff/1/media/filters/ffmpeg_video_de... media/filters/ffmpeg_video_decoder_unittest.cc:537: EXPECT_CALL(*demuxer_, Read(_)) On 2013/01/03 17:53:31, acolwell wrote: > It would be nice if this was slightly reworked so that it verifies that at least > one video_decoder_config() call happens between the kConfigChanged callback and > the next read. Hmm, this is harder than what I thought. Actually FFmpegVideoDecoder::GetVideoBuffer() also calls video_decoder_config() (for the natural size). So it's hard to insert a EXPECT_CALL() for video_decoder_config() in a specific place. But I do improved the test such that it's initialized with a random config first. The config change has to pick up the new config to be able to decode the frame. https://codereview.chromium.org/11745012/diff/1/media/filters/ffmpeg_video_de... media/filters/ffmpeg_video_decoder_unittest.cc:602: } On 2013/01/03 17:53:31, acolwell wrote: > A test should also be added to test the config failure with pending reset path. Done.
LGTM
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/11745012/2003
Retried try job too often on linux_rel for step(s) media_unittests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/11745012/9005
Sorry for I got bad news for ya. Compile failed with a clobber build on ios_dbg_simulator. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/11745012/9005
Message was sent while issue was closed.
Change committed as 175202 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
