Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10831020:
Refactor FFmpegAudioDecoder output timestamp logic. (Closed)
|
Created:
8 years, 4 months ago by acolwell GONE FROM CHROMIUM Modified:
8 years, 4 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRefactor FFmpegAudioDecoder output timestamp logic.
- These changes make the output timestamps & durations more accurate.
- Isolates output timestamps from rounding effects in the input timestamps.
- Fixes output timestamp error caused by the 1 frame coding delay.
- Trims off starting samples when Vorbis signals negative timestamps.
BUG=126183
TEST=None. Existing tests still pass.
Patch Set 1 #
Total comments: 26
Patch Set 2 : #Patch Set 3 : Address CR comments #
Total comments: 1
Patch Set 4 : Add fix for negative Vorbis timestamps. #Patch Set 5 : Replace round(x) with floor(0.5+x) to make windows happy. #
Total comments: 16
Patch Set 6 : Rebase and address CR comments #Patch Set 7 : Address Dale's comments #
Messages
Total messages: 22 (0 generated)
assigning a reviewer this time. :)
Just an FYI, there's also av_frame->best_effort_timestamp, although I suspect what you're doing here is better. http://git.chromium.org/gitweb/?p=chromium/third_party/ffmpeg.git;a=blob;f=li... http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... File media/filters/ffmpeg_audio_decoder.cc (right): http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... media/filters/ffmpeg_audio_decoder.cc:142: bytes_per_frame_ = codec_context_->channels is also available. http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... media/filters/ffmpeg_audio_decoder.cc:181: CHECK(input->GetTimestamp() != kNoTimestamp() || DCHECK, CHECK is a bit heavy for a decoding loop, no? http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... media/filters/ffmpeg_audio_decoder.cc:189: << " ts " << input->GetTimestamp().InSecondsF() Why not InMillisecondsF ? Are they generally O(seconds) different? http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... media/filters/ffmpeg_audio_decoder.cc:254: DCHECK_EQ(decoded_audio_size % bytes_per_frame_, 0) Is this true for all the codecs we support? I saw some comments on the DV codec recently indicating that it didn't always output full frames. Granted that was video and not audio. http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... media/filters/ffmpeg_audio_decoder.cc:300: DCHECK(total_frames_base_ != kNoTimestamp()); DCHECK_NE ? http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... media/filters/ffmpeg_audio_decoder.cc:303: int64 decoded_microseconds = Is int64 precise enough for this? Why not double?
Addressed CR comments. Can't use AVFrame.best_effort_timestamp. It doesn't appear to be set for for audio. http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... File media/filters/ffmpeg_audio_decoder.cc (right): http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... media/filters/ffmpeg_audio_decoder.cc:142: bytes_per_frame_ = On 2012/07/26 00:40:16, DaleCurtis wrote: > codec_context_->channels is also available. Done. http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... media/filters/ffmpeg_audio_decoder.cc:181: CHECK(input->GetTimestamp() != kNoTimestamp() || On 2012/07/26 00:40:16, DaleCurtis wrote: > DCHECK, CHECK is a bit heavy for a decoding loop, no? Why? The old code produces incorrect behavior and/or require adding much more logic for rebasing output buffer timestamps. The comments on 49709 seem to indicate this is fixed, so I thought the CHECK() was a reasonable compromise to smoke out content that triggers this bug again. http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... media/filters/ffmpeg_audio_decoder.cc:189: << " ts " << input->GetTimestamp().InSecondsF() On 2012/07/26 00:40:16, DaleCurtis wrote: > Why not InMillisecondsF ? Are they generally O(seconds) different? I just wanted the timestamp and difference to be in the same units. http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... media/filters/ffmpeg_audio_decoder.cc:254: DCHECK_EQ(decoded_audio_size % bytes_per_frame_, 0) On 2012/07/26 00:40:16, DaleCurtis wrote: > Is this true for all the codecs we support? I saw some comments on the DV codec > recently indicating that it didn't always output full frames. Granted that was > video and not audio. If this isn't true then I'm pretty sure other code would break since it means that we'd output only some of the channels. I'm just trying to make this expectation explicit so we can catch any unexpected behavior. http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... media/filters/ffmpeg_audio_decoder.cc:300: DCHECK(total_frames_base_ != kNoTimestamp()); On 2012/07/26 00:40:16, DaleCurtis wrote: > DCHECK_NE ? Compiler gets angry because there isn't a << operator for TimeDelta. http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... media/filters/ffmpeg_audio_decoder.cc:303: int64 decoded_microseconds = On 2012/07/26 00:40:16, DaleCurtis wrote: > Is int64 precise enough for this? Why not double? This is precise for microsecond resolution which is what I'm going for. I'll change the code to use a double though since it is unlikely that total_frames_decoded_ would ever get high enough to cause the double to start loosing precision and because that is more readable. I'm showing my age with these overflow avoidance tricks from the 32-bit & slow float days.
FYI I have the following todo inside ffmpeg_audio_decoder_unittest.cc 154 // We should have three decoded audio buffers. 155 // 156 // TODO(scherkus): timestamps are off by one packet due to decoder delay. 157 ASSERT_EQ(3u, decoded_audio_.size()); 158 ExpectDecodedAudio(0, 0, 2902); 159 ExpectDecodedAudio(1, 0, 13061); 160 ExpectDecodedAudio(2, 2902, 23219); Should that still be passing? Also does this fix http://crbug.com/126183 ? http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... File media/filters/ffmpeg_audio_decoder.cc (right): http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... media/filters/ffmpeg_audio_decoder.cc:189: << " ts " << input->GetTimestamp().InSecondsF() On 2012/07/26 00:40:16, DaleCurtis wrote: > Why not InMillisecondsF ? Are they generally O(seconds) different? I usually do InMicroseconds() myself! (see FFmpegVideoDecoder) http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... media/filters/ffmpeg_audio_decoder.cc:192: base::ResetAndReturn(&read_cb_).Run(kDecodeError, NULL); I'd be interested in knowing if/when this happens previously we would have gone back in time but continued playing where as now we will error the whole player http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... media/filters/ffmpeg_audio_decoder.cc:232: DCHECK(input->GetTimestamp() != kNoTimestamp()); somewhat of a longshot, but what if we seeked to the verrrrrrrrrrrrrrrrrrry edge of the file? if Reset() was called here (clearing total_frames_base_) and our first buffer was EOS, this would trigger as would the DCHECK inside GetNextOutputTimestamp() as well http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... media/filters/ffmpeg_audio_decoder.cc:254: DCHECK_EQ(decoded_audio_size % bytes_per_frame_, 0) On 2012/07/26 00:40:16, DaleCurtis wrote: > Is this true for all the codecs we support? I saw some comments on the DV codec > recently indicating that it didn't always output full frames. Granted that was > video and not audio. if it isn't I'd like to know about it :) http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... File media/filters/ffmpeg_audio_decoder.h (right): http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... media/filters/ffmpeg_audio_decoder.h:55: base::TimeDelta GetNextOutputTimestamp() const; docs http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... media/filters/ffmpeg_audio_decoder.h:70: int bytes_per_frame_; nit: I'd add one-liner comment describing what these 4 vars are for (calculating output timestamps) http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... media/filters/ffmpeg_audio_decoder.h:71: base::TimeDelta total_frames_base_; when I read "total_frames_base_" I thought it had something to do with frames (i.e., int) but it's a time! what about output_timestamp_base_, starting_output_timestamp_, etc?
Strange, in any case, my last tests with best_effort_timestamp (pre-parser turn on though) showed it wasn't as accurate as what we were doing. http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... File media/filters/ffmpeg_audio_decoder.cc (right): http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... media/filters/ffmpeg_audio_decoder.cc:181: CHECK(input->GetTimestamp() != kNoTimestamp() || On 2012/07/26 01:21:34, acolwell wrote: > On 2012/07/26 00:40:16, DaleCurtis wrote: > > DCHECK, CHECK is a bit heavy for a decoding loop, no? > Why? The old code produces incorrect behavior and/or require adding much more > logic for rebasing output buffer timestamps. The comments on 49709 seem to > indicate this is fixed, so I thought the CHECK() was a reasonable compromise to > smoke out content that triggers this bug again. Fair enough, it happens on all content when it does AFAIK, thus it's likely only to break when an FFmpeg roll happens, during which I run the tests in debug, asan, tsan, valgrind, crazy mode which will find it then :) We do see real world benefit in some cases by optimizing the decoder methods, though possibly (probably?) not over one CHECK! Here's a neat graph which shows Ronald's video memcpy removal patch and its effects on decoded fps: http://build.chromium.org/f/chromium/perf/linux-release/media_tests_av_perf/r... Up to you in any case.
PTAL. This does fix Bug 126183. This patch discovered that content/test/data/media/bear.ogv generates timestamps that go backwards. I suspect this file is actually invalid though and I'm talking with the Xiph folks to verify that. http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... File media/filters/ffmpeg_audio_decoder.cc (right): http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... media/filters/ffmpeg_audio_decoder.cc:189: << " ts " << input->GetTimestamp().InSecondsF() On 2012/07/26 01:43:08, scherkus wrote: > On 2012/07/26 00:40:16, DaleCurtis wrote: > > Why not InMillisecondsF ? Are they generally O(seconds) different? > > I usually do InMicroseconds() myself! (see FFmpegVideoDecoder) ok. I'll use microseconds to match the FFmpegVideoDecoder code. http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... media/filters/ffmpeg_audio_decoder.cc:192: base::ResetAndReturn(&read_cb_).Run(kDecodeError, NULL); On 2012/07/26 01:43:08, scherkus wrote: > I'd be interested in knowing if/when this happens > > previously we would have gone back in time but continued playing where as now we > will error the whole player This is intended to smoke out content that violates the assumption that input timestamps always move forward. If we encounter such content then we can determine whether it is a valid case or a bug in the demuxing code somewhere. I just don't want to allow this situation to effect downstream code like the clock anymore. http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... media/filters/ffmpeg_audio_decoder.cc:232: DCHECK(input->GetTimestamp() != kNoTimestamp()); On 2012/07/26 01:43:08, scherkus wrote: > somewhat of a longshot, but what if we seeked to the verrrrrrrrrrrrrrrrrrry edge > of the file? > > if Reset() was called here (clearing total_frames_base_) and our first buffer > was EOS, this would trigger as would the DCHECK inside GetNextOutputTimestamp() > as well Good catch! Added IsEndOfStream() check. http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... File media/filters/ffmpeg_audio_decoder.h (right): http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... media/filters/ffmpeg_audio_decoder.h:55: base::TimeDelta GetNextOutputTimestamp() const; On 2012/07/26 01:43:08, scherkus wrote: > docs Done. http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... media/filters/ffmpeg_audio_decoder.h:70: int bytes_per_frame_; On 2012/07/26 01:43:08, scherkus wrote: > nit: I'd add one-liner comment describing what these 4 vars are for (calculating > output timestamps) Done. http://codereview.chromium.org/10831020/diff/1/media/filters/ffmpeg_audio_dec... media/filters/ffmpeg_audio_decoder.h:71: base::TimeDelta total_frames_base_; On 2012/07/26 01:43:08, scherkus wrote: > when I read "total_frames_base_" I thought it had something to do with frames > (i.e., int) but it's a time! > > what about output_timestamp_base_, starting_output_timestamp_, etc? Done.
LGTM http://codereview.chromium.org/10831020/diff/8002/media/filters/ffmpeg_audio_... File media/filters/ffmpeg_audio_decoder_unittest.cc (left): http://codereview.chromium.org/10831020/diff/8002/media/filters/ffmpeg_audio_... media/filters/ffmpeg_audio_decoder_unittest.cc:156: // TODO(scherkus): timestamps are off by one packet due to decoder delay. \o/
lgtm
PTAL. Now includes logic to handle negative Vorbis timestamps. This still isn't 100% for the insanity in bear.ogv (off by ~10ms), but it should be correct for most Vorbis content that uses the 'trim from the front' feature.
lgtm w/ sanity checking q's + nits because this stuff makes my head hurt http://codereview.chromium.org/10831020/diff/5006/media/filters/ffmpeg_audio_... File media/filters/ffmpeg_audio_decoder.cc (right): http://codereview.chromium.org/10831020/diff/5006/media/filters/ffmpeg_audio_... media/filters/ffmpeg_audio_decoder.cc:184: input->IsEndOfStream()); want to << "some text" so folks know what's up instead of digging through the source for the above comment? http://codereview.chromium.org/10831020/diff/5006/media/filters/ffmpeg_audio_... media/filters/ffmpeg_audio_decoder.cc:187: demuxer_stream_->audio_decoder_config().codec() == kCodecVorbis; is the check done here in case the codec changes on the fly? if so, will we handle the change correctly? if not, should we cache the value in ::DoInitialize()? http://codereview.chromium.org/10831020/diff/5006/media/filters/ffmpeg_audio_... media/filters/ffmpeg_audio_decoder.cc:191: if (is_vorbis) { if there is some online documentation anywhere that explains this craziness it would be prudent to include it here :) http://codereview.chromium.org/10831020/diff/5006/media/filters/ffmpeg_audio_... media/filters/ffmpeg_audio_decoder.cc:193: int frames_to_drop = floor(0.5 + technially we'd wrap at the ( on next line, so what about dropping floor to next line instead? http://codereview.chromium.org/10831020/diff/5006/media/filters/ffmpeg_audio_... media/filters/ffmpeg_audio_decoder.cc:194: -input->GetTimestamp().InSecondsF() * samples_per_second_); and we have a test that covers this business? http://codereview.chromium.org/10831020/diff/5006/media/filters/ffmpeg_audio_... media/filters/ffmpeg_audio_decoder.cc:298: } else if (IsEndOfStream(result, decoded_audio_size, input)) { sanity q: if line 279 caused decoded_audio_size -> 0 is it possible in any way to mistakenly cause this condition to go true when perhaps it shouldn't have?
don't forget to update CL description to include the new vorbis-specific fun times stuff
http://codereview.chromium.org/10831020/diff/5006/media/filters/ffmpeg_audio_... File media/filters/ffmpeg_audio_decoder.cc (right): http://codereview.chromium.org/10831020/diff/5006/media/filters/ffmpeg_audio_... media/filters/ffmpeg_audio_decoder.cc:184: input->IsEndOfStream()); On 2012/08/03 22:34:57, scherkus wrote: > want to << "some text" so folks know what's up instead of digging through the > source for the above comment? Done. http://codereview.chromium.org/10831020/diff/5006/media/filters/ffmpeg_audio_... media/filters/ffmpeg_audio_decoder.cc:187: demuxer_stream_->audio_decoder_config().codec() == kCodecVorbis; On 2012/08/03 22:34:57, scherkus wrote: > is the check done here in case the codec changes on the fly? > > if so, will we handle the change correctly? > if not, should we cache the value in ::DoInitialize()? There are no plans to allow the codec to change. Added codec_ member variable and copy the value in DoInitialize(). http://codereview.chromium.org/10831020/diff/5006/media/filters/ffmpeg_audio_... media/filters/ffmpeg_audio_decoder.cc:191: if (is_vorbis) { On 2012/08/03 22:34:57, scherkus wrote: > if there is some online documentation anywhere that explains this craziness it > would be prudent to include it here :) Added a reference to the Vorbis spec. http://codereview.chromium.org/10831020/diff/5006/media/filters/ffmpeg_audio_... media/filters/ffmpeg_audio_decoder.cc:193: int frames_to_drop = floor(0.5 + On 2012/08/03 22:34:57, scherkus wrote: > technially we'd wrap at the ( on next line, so what about dropping floor to next > line instead? The floor doesn't fit. I've moved the 0.5 + down though. http://codereview.chromium.org/10831020/diff/5006/media/filters/ffmpeg_audio_... media/filters/ffmpeg_audio_decoder.cc:194: -input->GetTimestamp().InSecondsF() * samples_per_second_); On 2012/08/03 22:34:57, scherkus wrote: > and we have a test that covers this business? Yep. MediaTest.VideoBearTheora uncovered the need for this code. http://codereview.chromium.org/10831020/diff/5006/media/filters/ffmpeg_audio_... media/filters/ffmpeg_audio_decoder.cc:298: } else if (IsEndOfStream(result, decoded_audio_size, input)) { On 2012/08/03 22:34:57, scherkus wrote: > sanity q: if line 279 caused decoded_audio_size -> 0 is it possible in any way > to mistakenly cause this condition to go true when perhaps it shouldn't have? I don't believe so. Even if the decoder outputted samples and line 279 made decoded_audio_size ->0 we are still at the end of the stream. Those samples weren't supposed to be played anyway and input->IsEndOfStream() signals that no more data is coming.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/acolwell@chromium.org/10831020/18001
http://codereview.chromium.org/10831020/diff/5006/media/filters/ffmpeg_audio_... File media/filters/ffmpeg_audio_decoder.cc (right): http://codereview.chromium.org/10831020/diff/5006/media/filters/ffmpeg_audio_... media/filters/ffmpeg_audio_decoder.cc:187: demuxer_stream_->audio_decoder_config().codec() == kCodecVorbis; On 2012/08/03 22:34:57, scherkus wrote: > is the check done here in case the codec changes on the fly? > > if so, will we handle the change correctly? > if not, should we cache the value in ::DoInitialize()? codec_context_->codec_id == CODEC_ID_VORBIS seems much simpler? http://codereview.chromium.org/10831020/diff/5006/media/filters/ffmpeg_audio_... media/filters/ffmpeg_audio_decoder.cc:192: if (input->GetTimestamp() < base::TimeDelta()) { Can avoid double else below by rolling this into if (is_vorbis).
On 2012/08/03 23:15:36, DaleCurtis wrote: > http://codereview.chromium.org/10831020/diff/5006/media/filters/ffmpeg_audio_... > File media/filters/ffmpeg_audio_decoder.cc (right): > > http://codereview.chromium.org/10831020/diff/5006/media/filters/ffmpeg_audio_... > media/filters/ffmpeg_audio_decoder.cc:187: > demuxer_stream_->audio_decoder_config().codec() == kCodecVorbis; > On 2012/08/03 22:34:57, scherkus wrote: > > is the check done here in case the codec changes on the fly? > > > > if so, will we handle the change correctly? > > if not, should we cache the value in ::DoInitialize()? > > codec_context_->codec_id == CODEC_ID_VORBIS seems much simpler? > > http://codereview.chromium.org/10831020/diff/5006/media/filters/ffmpeg_audio_... > media/filters/ffmpeg_audio_decoder.cc:192: if (input->GetTimestamp() < > base::TimeDelta()) { > Can avoid double else below by rolling this into if (is_vorbis). Seems I'm too late :) LGTM in any case.
http://codereview.chromium.org/10831020/diff/5006/media/filters/ffmpeg_audio_... File media/filters/ffmpeg_audio_decoder.cc (right): http://codereview.chromium.org/10831020/diff/5006/media/filters/ffmpeg_audio_... media/filters/ffmpeg_audio_decoder.cc:187: demuxer_stream_->audio_decoder_config().codec() == kCodecVorbis; On 2012/08/03 23:15:36, DaleCurtis wrote: > On 2012/08/03 22:34:57, scherkus wrote: > > is the check done here in case the codec changes on the fly? > > > > if so, will we handle the change correctly? > > if not, should we cache the value in ::DoInitialize()? > > codec_context_->codec_id == CODEC_ID_VORBIS seems much simpler? Done. http://codereview.chromium.org/10831020/diff/5006/media/filters/ffmpeg_audio_... media/filters/ffmpeg_audio_decoder.cc:192: if (input->GetTimestamp() < base::TimeDelta()) { On 2012/08/03 23:15:36, DaleCurtis wrote: > Can avoid double else below by rolling this into if (is_vorbis). Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/acolwell@chromium.org/10831020/17003
Commit queue had an internal error. Something went really wrong, probably a crash, a hickup or simply the monkeys went out for dinner. The relevant dude was pinged already.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/acolwell@chromium.org/10831020/17003
Failed to apply patch for media/filters/ffmpeg_audio_decoder.cc: While running patch -p1 --forward --force; patching file media/filters/ffmpeg_audio_decoder.cc Hunk #1 FAILED at 16. Hunk #2 FAILED at 35. Hunk #3 FAILED at 44. Hunk #4 FAILED at 146. Hunk #5 FAILED at 181. Hunk #6 FAILED at 221. Hunk #7 FAILED at 231. Hunk #8 FAILED at 238. Hunk #9 FAILED at 281. 9 out of 9 hunks FAILED -- saving rejects to file media/filters/ffmpeg_audio_decoder.cc.rej
For the record this did apparently get checked in. http://src.chromium.org/viewvc/chrome?view=rev&revision=149991 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
