Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
259453003:
Introduce AudioDiscardHelper. Refactor audio decoders to use it. (Closed)
|
Created:
6 years, 8 months ago by DaleCurtis Modified:
6 years, 7 months ago Reviewers:
wolenetz CC:
chromium-reviews, feature-media-reviews_chromium.org, acolwell GONE FROM CHROMIUM Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionIntroduce AudioDiscardHelper. Refactor audio decoders to use it.
We've long had duplicated code in the opus and ffmpeg audio decoders
based around discarding samples from the front of back of output
buffers. This change standarizes this into a better tested helper
class.
This paves the way for a later change which will expand discard_padding()
for use by media source.
It also fixed the chained ogg case with opus by allowing non-monotonic
timestamps for the opus decoder.
BUG=175281, 360961
TEST=new unittest. existing tests pass.
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266953
Patch Set 1 #Patch Set 2 : Simplify! #
Total comments: 48
Patch Set 3 : Comments. #
Total comments: 7
Patch Set 4 : Fix comments. #Patch Set 5 : Fix x64 type. #
Messages
Total messages: 36 (0 generated)
wolenetz: Have a look! I'm a little iffy on the name, but AudioDecoderHelper seemed too generic. cc: acolwell in case he wants to chime in.
Despite our earlier discussion, this is now ready for review :) Turns out we don't need timestamp offset for opus, which simplifies the interface here a bit. This now depends on: https://codereview.chromium.org/257563007/ https://codereview.chromium.org/251583003/
First round of comments. https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... File media/base/audio_discard_helper.cc (right): https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper.cc:31: last_input_timestamp_(kNoTimestamp()) { nit: is any sanity checking needed on sample_rate_ (zero, negative, or too large?) https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper.cc:39: return std::ceil(duration.InSecondsF() * sample_rate_); With ceil(), might we hit off-by-one stuff here? (Does this matter?) https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper.cc:77: decoded_buffer->TrimStart(frames_to_discard); TrimStart() will modify decoded_buffer's duration_ and potential later TrimEnd() called from this method will use that modified duration_ prior to this method explicitly setting the decoded_buffer's duration. ISTM like this could introduce variant processing or at least be hard to maintain and keep bug-free (which calculation of decoded buffer duration should be used, and when?) Note also that TrimEnd() also adjusts the buffer's duration. TrimEnd() & TrimStart() directly impact the buffer's frame_count(), but perhaps in ways different from this method's expectation. https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper.cc:83: // trimming. nit: reference bug? https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper.cc:88: if (end_frames_to_discard > decoded_frames) { Is end discard aware of start discard? (If not, could a 'valid' stream contain more trimming than actual start+end discard?) https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper.cc:89: DLOG(ERROR) << "Invalid file. Incorrect discard padding value."; nit: If this helper will be used by MSE, not just ffmpeg, then s/file/stream? https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... File media/base/audio_discard_helper.h (right): https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper.h:26: // |duration| must be postive. nit: s/postive/positive https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper.h:27: int TimeDeltaToFrames(base::TimeDelta duration) const; nit: why int? seems possible it could hit overflow. elsewhere, like Reset(), size_t is used. Use size_t here? https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper.h:36: bool ProcessBuffers(const scoped_refptr<DecoderBuffer>& encoded_buffer, What does the bool return value signify? https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper.h:56: #endif nit: include // MEDIA_BASE_AUDIO_DISCARD_HELPER_H_ https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... File media/base/audio_discard_helper_unittest.cc (right): https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper_unittest.cc:158: // Process the same input buffer again to ensure input timestamps which go nit: timestamp really isn't going backwards in time here, just staying the same, right? https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper_unittest.cc:166: TEST(AudioDiscardHelperTest, DiscardPadding) { Versus ProcessBuffersWithInitialDiscard and InitialDiscardAndDiscardPadding, these tests don't confirm *which* chunk of the decoded_buffer is trimmed (TrimStart() or TrimEnd()). Please include such verification. https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper_unittest.cc:212: nit: remove extra line. https://codereview.chromium.org/259453003/diff/50009/media/filters/ffmpeg_aud... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/259453003/diff/50009/media/filters/ffmpeg_aud... media/filters/ffmpeg_audio_decoder.cc:255: if (!buffer->end_of_stream() && buffer->timestamp() == kNoTimestamp()) { Was it incorrect previously to also condition this error on what looks to me to be equivalent to !discard_helper_->initialized()? https://codereview.chromium.org/259453003/diff/50009/media/filters/ffmpeg_aud... media/filters/ffmpeg_audio_decoder.cc:267: const int discard_frames = If a stream has both a codec_delay and a negative initial timestamp, what should behavior be? IIUC, this code would ignore the codec delay and just use the negative timestamp's conversion to a discard framecount. Is that really correct? https://codereview.chromium.org/259453003/diff/50009/media/filters/ffmpeg_aud... media/filters/ffmpeg_audio_decoder.cc:268: discard_helper_->TimeDeltaToFrames(-buffer->timestamp()); Previously, we rounded vs currently using ceil. Which version is correct? https://codereview.chromium.org/259453003/diff/50009/media/filters/ffmpeg_aud... media/filters/ffmpeg_audio_decoder.cc:269: discard_helper_->Reset(discard_frames); Is the following piece of that section A.2 Vorbis spec verified somewhere here? :: "The number of samples to be discarded must not exceed the overlap-add span of the first two audio packets." https://codereview.chromium.org/259453003/diff/50009/media/filters/ffmpeg_aud... media/filters/ffmpeg_audio_decoder.cc:382: } else if (discard_helper_->ProcessBuffers(buffer, output)) { Should any encoded buffer originating from ffmpeg_audio_decoder.cc *not* have any discard_padding()? In previous code, it looks like any such padding was not discarded. In current code, it will be discarded. Which is correct? If the previous code was correct, include a DCHECK (or CHECK if that important) to flag any occurrences of nonzero discard_padding() processed in this codepath? https://codereview.chromium.org/259453003/diff/50009/media/filters/opus_audio... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/259453003/diff/50009/media/filters/opus_audio... media/filters/opus_audio_decoder.cc:309: if (input->timestamp() == kNoTimestamp()) { Ditto of similar question in ffmpeg_audio_decoder.cc https://codereview.chromium.org/259453003/diff/50009/media/filters/opus_audio... media/filters/opus_audio_decoder.cc:315: // Apply the necessary codec delay. Versus ffmpeg audio decoder, why do we wait until now to apply the codec delay to discard_helper_ instead of some earlier time (like ConfigureDecoder())? https://codereview.chromium.org/259453003/diff/50009/media/filters/opus_audio... media/filters/opus_audio_decoder.cc:320: discard_helper_->Reset(config_.codec_delay()); Is this correctly ignoring possible config_.seek_preroll() in favor of whatever the config_.codec_delay() is? Or am I missing something?
Just comments. I'll resolve your nits in the next patch set. https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... File media/base/audio_discard_helper.cc (right): https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper.cc:77: decoded_buffer->TrimStart(frames_to_discard); On 2014/04/28 21:52:07, wolenetz wrote: > TrimStart() will modify decoded_buffer's duration_ and potential later TrimEnd() > called from this method will use that modified duration_ prior to this method > explicitly setting the decoded_buffer's duration. ISTM like this could introduce > variant processing or at least be hard to maintain and keep bug-free (which > calculation of decoded buffer duration should be used, and when?) Note also that > TrimEnd() also adjusts the buffer's duration. TrimEnd() & TrimStart() directly > impact the buffer's frame_count(), but perhaps in ways different from this > method's expectation. It's this codes expectation that it is the sole authority of decoded buffer durations and timestamps. Prior to setting the duration below, it does not matter what the duration() or timestamp() are. https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper.cc:88: if (end_frames_to_discard > decoded_frames) { On 2014/04/28 21:52:07, wolenetz wrote: > Is end discard aware of start discard? (If not, could a 'valid' stream contain > more trimming than actual start+end discard?) It's expected that users of discard_padding() will ensure there's no overflow. Hence the error log. End trimming only works in certain circumstances as you can see in part II of this CL, so for now this error tracks the existing use case. https://codereview.chromium.org/259453003/diff/50009/media/filters/ffmpeg_aud... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/259453003/diff/50009/media/filters/ffmpeg_aud... media/filters/ffmpeg_audio_decoder.cc:255: if (!buffer->end_of_stream() && buffer->timestamp() == kNoTimestamp()) { On 2014/04/28 21:52:07, wolenetz wrote: > Was it incorrect previously to also condition this error on what looks to me to > be equivalent to !discard_helper_->initialized()? I've removed this exception as the only time this should happen is when the file is broken. https://codereview.chromium.org/259453003/diff/50009/media/filters/ffmpeg_aud... media/filters/ffmpeg_audio_decoder.cc:267: const int discard_frames = On 2014/04/28 21:52:07, wolenetz wrote: > If a stream has both a codec_delay and a negative initial timestamp, what should > behavior be? IIUC, this code would ignore the codec delay and just use the > negative timestamp's conversion to a discard framecount. Is that really correct? For now this code is sufficient since it just implements existing behavior. See part II of this CL for how I resolve this. https://codereview.chromium.org/259453003/diff/50009/media/filters/ffmpeg_aud... media/filters/ffmpeg_audio_decoder.cc:268: discard_helper_->TimeDeltaToFrames(-buffer->timestamp()); On 2014/04/28 21:52:07, wolenetz wrote: > Previously, we rounded vs currently using ceil. Which version is correct? ceil(). It doesn't make sense to discard 1.5 frames. https://codereview.chromium.org/259453003/diff/50009/media/filters/ffmpeg_aud... media/filters/ffmpeg_audio_decoder.cc:269: discard_helper_->Reset(discard_frames); On 2014/04/28 21:52:07, wolenetz wrote: > Is the following piece of that section A.2 Vorbis spec verified somewhere here? > :: > "The number of samples to be discarded must not exceed the overlap-add span of > the first two audio packets." This is not verified. I'd fix that in a separate CL if that's something we need to do. https://codereview.chromium.org/259453003/diff/50009/media/filters/ffmpeg_aud... media/filters/ffmpeg_audio_decoder.cc:382: } else if (discard_helper_->ProcessBuffers(buffer, output)) { On 2014/04/28 21:52:07, wolenetz wrote: > Should any encoded buffer originating from ffmpeg_audio_decoder.cc *not* have > any discard_padding()? In previous code, it looks like any such padding was not > discarded. In current code, it will be discarded. Which is correct? If the > previous code was correct, include a DCHECK (or CHECK if that important) to flag > any occurrences of nonzero discard_padding() processed in this codepath? It's possible for buffers originating from ffmpeg to have discard padding, previously these were not handled correctly. See part II of this CL (specifically ffmpeg_demuxer) to see where this happens. https://codereview.chromium.org/259453003/diff/50009/media/filters/opus_audio... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/259453003/diff/50009/media/filters/opus_audio... media/filters/opus_audio_decoder.cc:309: if (input->timestamp() == kNoTimestamp()) { On 2014/04/28 21:52:07, wolenetz wrote: > Ditto of similar question in ffmpeg_audio_decoder.cc Ditto. This was copy pasted from the other decoder. https://codereview.chromium.org/259453003/diff/50009/media/filters/opus_audio... media/filters/opus_audio_decoder.cc:315: // Apply the necessary codec delay. On 2014/04/28 21:52:07, wolenetz wrote: > Versus ffmpeg audio decoder, why do we wait until now to apply the codec delay > to discard_helper_ instead of some earlier time (like ConfigureDecoder())? Because it should only be applied when we see start_input_timestamp_ which we don't know until now. https://codereview.chromium.org/259453003/diff/50009/media/filters/opus_audio... media/filters/opus_audio_decoder.cc:320: discard_helper_->Reset(config_.codec_delay()); On 2014/04/28 21:52:07, wolenetz wrote: > Is this correctly ignoring possible config_.seek_preroll() in favor of whatever > the config_.codec_delay() is? Or am I missing something? Yes it is ignoring that value when we are at the start of the stream, otherwise the seek value should be used.
Thanks for rapid response :) A couple replies to PS3 comments: https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... File media/base/audio_discard_helper.cc (right): https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper.cc:77: decoded_buffer->TrimStart(frames_to_discard); On 2014/04/28 22:03:51, DaleCurtis wrote: > On 2014/04/28 21:52:07, wolenetz wrote: > > TrimStart() will modify decoded_buffer's duration_ and potential later > TrimEnd() > > called from this method will use that modified duration_ prior to this method > > explicitly setting the decoded_buffer's duration. ISTM like this could > introduce > > variant processing or at least be hard to maintain and keep bug-free (which > > calculation of decoded buffer duration should be used, and when?) Note also > that > > TrimEnd() also adjusts the buffer's duration. TrimEnd() & TrimStart() directly > > impact the buffer's frame_count(), but perhaps in ways different from this > > method's expectation. > > It's this codes expectation that it is the sole authority of decoded buffer > durations and timestamps. Prior to setting the duration below, it does not > matter what the duration() or timestamp() are. Regarding assumption of ability to set duration/timestamp externally, after construction, of an AudioBuffer, it already looks like there is a partial plan to disallow that (https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_b...). Hence, maintainability of the assumption that AudioDiscardHelper "owns" the maintenance externally of AudioBuffer duration and timestamp seems fragile already. Perhaps adjust this comment in AudioBuffer? https://codereview.chromium.org/259453003/diff/50009/media/filters/ffmpeg_aud... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/259453003/diff/50009/media/filters/ffmpeg_aud... media/filters/ffmpeg_audio_decoder.cc:268: discard_helper_->TimeDeltaToFrames(-buffer->timestamp()); On 2014/04/28 22:03:51, DaleCurtis wrote: > On 2014/04/28 21:52:07, wolenetz wrote: > > Previously, we rounded vs currently using ceil. Which version is correct? > > ceil(). It doesn't make sense to discard 1.5 frames. Suppose we have frame rate of 48000 frames/sec and encoder wishes to discard 1 frame: 1 frame = 20.833333us: would buffer->timestamp() in this case (limited to microsecond granularity) be -21 or -20? If -21, then discard_helper_->TimeDeltaToFrames(21) would be std::ceil(0.000021 * 48000) == std::ceil(1.008) == 2 frames. If -20, then it would be 1 frame. I'm unfamiliar with the bitstream and the parsers, but this seems fragile to off-by-one if not rounding.
On 2014/04/28 22:37:56, wolenetz wrote: > Thanks for rapid response :) > A couple replies to PS3 comments: > > https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... > File media/base/audio_discard_helper.cc (right): > > https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... > media/base/audio_discard_helper.cc:77: > decoded_buffer->TrimStart(frames_to_discard); > On 2014/04/28 22:03:51, DaleCurtis wrote: > > On 2014/04/28 21:52:07, wolenetz wrote: > > > TrimStart() will modify decoded_buffer's duration_ and potential later > > TrimEnd() > > > called from this method will use that modified duration_ prior to this > method > > > explicitly setting the decoded_buffer's duration. ISTM like this could > > introduce > > > variant processing or at least be hard to maintain and keep bug-free (which > > > calculation of decoded buffer duration should be used, and when?) Note also > > that > > > TrimEnd() also adjusts the buffer's duration. TrimEnd() & TrimStart() > directly > > > impact the buffer's frame_count(), but perhaps in ways different from this > > > method's expectation. > > > > It's this codes expectation that it is the sole authority of decoded buffer > > durations and timestamps. Prior to setting the duration below, it does not > > matter what the duration() or timestamp() are. > > Regarding assumption of ability to set duration/timestamp externally, after > construction, of an AudioBuffer, it already looks like there is a partial plan > to disallow that > (https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_b...). > Hence, maintainability of the assumption that AudioDiscardHelper "owns" the > maintenance externally of AudioBuffer duration and timestamp seems fragile > already. Perhaps adjust this comment in AudioBuffer? > > https://codereview.chromium.org/259453003/diff/50009/media/filters/ffmpeg_aud... > File media/filters/ffmpeg_audio_decoder.cc (right): > > https://codereview.chromium.org/259453003/diff/50009/media/filters/ffmpeg_aud... > media/filters/ffmpeg_audio_decoder.cc:268: > discard_helper_->TimeDeltaToFrames(-buffer->timestamp()); > On 2014/04/28 22:03:51, DaleCurtis wrote: > > On 2014/04/28 21:52:07, wolenetz wrote: > > > Previously, we rounded vs currently using ceil. Which version is correct? > > > > ceil(). It doesn't make sense to discard 1.5 frames. > > Suppose we have frame rate of 48000 frames/sec and encoder wishes to discard 1 > frame: > 1 frame = 20.833333us: would buffer->timestamp() in this case (limited to > microsecond granularity) be -21 or -20? > If -21, then discard_helper_->TimeDeltaToFrames(21) would be std::ceil(0.000021 > * 48000) == std::ceil(1.008) == 2 frames. If -20, then it would be 1 frame. I'm > unfamiliar with the bitstream and the parsers, but this seems fragile to > off-by-one if not rounding. s/PS3/PS2...
PTAL. https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... File media/base/audio_discard_helper.cc (right): https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper.cc:31: last_input_timestamp_(kNoTimestamp()) { On 2014/04/28 21:52:07, wolenetz wrote: > nit: is any sanity checking needed on sample_rate_ (zero, negative, or too > large?) As far as this code is concerned it just needs to be > 0. Added a DCHECK(). https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper.cc:39: return std::ceil(duration.InSecondsF() * sample_rate_); On 2014/04/28 21:52:07, wolenetz wrote: > With ceil(), might we hit off-by-one stuff here? (Does this matter?) Since this floating point value is calculated from two integers it's unlikely we'll see floating point error here. I don't think it matters in any case. https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper.cc:77: decoded_buffer->TrimStart(frames_to_discard); On 2014/04/28 22:37:56, wolenetz wrote: > On 2014/04/28 22:03:51, DaleCurtis wrote: > > On 2014/04/28 21:52:07, wolenetz wrote: > > > TrimStart() will modify decoded_buffer's duration_ and potential later > > TrimEnd() > > > called from this method will use that modified duration_ prior to this > method > > > explicitly setting the decoded_buffer's duration. ISTM like this could > > introduce > > > variant processing or at least be hard to maintain and keep bug-free (which > > > calculation of decoded buffer duration should be used, and when?) Note also > > that > > > TrimEnd() also adjusts the buffer's duration. TrimEnd() & TrimStart() > directly > > > impact the buffer's frame_count(), but perhaps in ways different from this > > > method's expectation. > > > > It's this codes expectation that it is the sole authority of decoded buffer > > durations and timestamps. Prior to setting the duration below, it does not > > matter what the duration() or timestamp() are. > > Regarding assumption of ability to set duration/timestamp externally, after > construction, of an AudioBuffer, it already looks like there is a partial plan > to disallow that > (https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_b...). > Hence, maintainability of the assumption that AudioDiscardHelper "owns" the > maintenance externally of AudioBuffer duration and timestamp seems fragile > already. Perhaps adjust this comment in AudioBuffer? I think that comment can be removed now. I'll do so in the next patch set. https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper.cc:83: // trimming. On 2014/04/28 21:52:07, wolenetz wrote: > nit: reference bug? Done. https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper.cc:89: DLOG(ERROR) << "Invalid file. Incorrect discard padding value."; On 2014/04/28 21:52:07, wolenetz wrote: > nit: If this helper will be used by MSE, not just ffmpeg, then s/file/stream? Done. https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... File media/base/audio_discard_helper.h (right): https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper.h:26: // |duration| must be postive. On 2014/04/28 21:52:07, wolenetz wrote: > nit: s/postive/positive Done. https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper.h:27: int TimeDeltaToFrames(base::TimeDelta duration) const; On 2014/04/28 21:52:07, wolenetz wrote: > nit: why int? seems possible it could hit overflow. elsewhere, like Reset(), > size_t is used. Use size_t here? Done. https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper.h:36: bool ProcessBuffers(const scoped_refptr<DecoderBuffer>& encoded_buffer, On 2014/04/28 21:52:07, wolenetz wrote: > What does the bool return value signify? Docs updated. https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper.h:56: #endif On 2014/04/28 21:52:07, wolenetz wrote: > nit: include // MEDIA_BASE_AUDIO_DISCARD_HELPER_H_ Done. https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... File media/base/audio_discard_helper_unittest.cc (right): https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper_unittest.cc:158: // Process the same input buffer again to ensure input timestamps which go On 2014/04/28 21:52:07, wolenetz wrote: > nit: timestamp really isn't going backwards in time here, just staying the same, > right? kTimestamp on buffer 2 is now behind kTimestamp+kDuration which is where current time points after buffer 1 is processed. https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper_unittest.cc:166: TEST(AudioDiscardHelperTest, DiscardPadding) { On 2014/04/28 21:52:07, wolenetz wrote: > Versus ProcessBuffersWithInitialDiscard and InitialDiscardAndDiscardPadding, > these tests don't confirm *which* chunk of the decoded_buffer is trimmed > (TrimStart() or TrimEnd()). Please include such verification. Done. Doing so required improving the accuracy of MakeTestBuffer(), which was cumulatively adding to get the next step value. https://codereview.chromium.org/259453003/diff/50009/media/base/audio_discard... media/base/audio_discard_helper_unittest.cc:212: On 2014/04/28 21:52:07, wolenetz wrote: > nit: remove extra line. Done. https://codereview.chromium.org/259453003/diff/50009/media/filters/ffmpeg_aud... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/259453003/diff/50009/media/filters/ffmpeg_aud... media/filters/ffmpeg_audio_decoder.cc:268: discard_helper_->TimeDeltaToFrames(-buffer->timestamp()); On 2014/04/28 22:37:56, wolenetz wrote: > On 2014/04/28 22:03:51, DaleCurtis wrote: > > On 2014/04/28 21:52:07, wolenetz wrote: > > > Previously, we rounded vs currently using ceil. Which version is correct? > > > > ceil(). It doesn't make sense to discard 1.5 frames. > > Suppose we have frame rate of 48000 frames/sec and encoder wishes to discard 1 > frame: > 1 frame = 20.833333us: would buffer->timestamp() in this case (limited to > microsecond granularity) be -21 or -20? > If -21, then discard_helper_->TimeDeltaToFrames(21) would be std::ceil(0.000021 > * 48000) == std::ceil(1.008) == 2 frames. If -20, then it would be 1 frame. I'm > unfamiliar with the bitstream and the parsers, but this seems fragile to > off-by-one if not rounding. You can see how the values are converted in ffmpeg_demuxer: base::TimeDelta::FromMicroseconds( discard_padding_samples * 1000000.0 / audio_decoder_config().samples_per_second())); Which gives a timestamp of 20us with your example data. With vorbis and negative timestamps, an example value from bear.ogv is -2902us => 127.9782 frames. I don't have evidence that we always need ceil and the cases you point out do seem a bit fragile, so I'll change this to a rounding solution. I also found a bug with my output time-stamping while evaluating this.
All nits now. lgtm % nits: https://codereview.chromium.org/259453003/diff/50009/media/filters/ffmpeg_aud... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/259453003/diff/50009/media/filters/ffmpeg_aud... media/filters/ffmpeg_audio_decoder.cc:269: discard_helper_->Reset(discard_frames); On 2014/04/28 22:03:51, DaleCurtis wrote: > On 2014/04/28 21:52:07, wolenetz wrote: > > Is the following piece of that section A.2 Vorbis spec verified somewhere > here? > > :: > > "The number of samples to be discarded must not exceed the overlap-add span of > > the first two audio packets." > > This is not verified. I'd fix that in a separate CL if that's something we need > to do. Seems low-pri (we might reject streams that are slightly out-of-spec that we might otherwise allow). File a bug and reference it if you think this is something we need to do please. https://codereview.chromium.org/259453003/diff/110001/media/base/test_helpers.cc File media/base/test_helpers.cc (right): https://codereview.chromium.org/259453003/diff/110001/media/base/test_helpers... media/base/test_helpers.cc:188: const float v = start + ch * output_size * increment; s/float/T ? https://codereview.chromium.org/259453003/diff/110001/media/filters/ffmpeg_au... File media/filters/ffmpeg_audio_decoder.h (right): https://codereview.chromium.org/259453003/diff/110001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.h:84: // Used for computing output timestamps. nit: discard_helper_ is used for more than that. Remove the comment? https://codereview.chromium.org/259453003/diff/110001/media/filters/opus_audi... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/259453003/diff/110001/media/filters/opus_audi... media/filters/opus_audio_decoder.cc:435: discard_helper_->TimeDeltaToFrames(config_.seek_preroll())); nit: disallow negative seek_preroll() from resetting the discard_helper in release builds? TimeDeltaToFrames() should DCHECK in debug builds, but would just return a really large number of frames in release builds.
Thanks for review. I'll rebase part II momentarily. https://codereview.chromium.org/259453003/diff/50009/media/filters/ffmpeg_aud... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/259453003/diff/50009/media/filters/ffmpeg_aud... media/filters/ffmpeg_audio_decoder.cc:269: discard_helper_->Reset(discard_frames); On 2014/04/29 00:22:44, wolenetz wrote: > On 2014/04/28 22:03:51, DaleCurtis wrote: > > On 2014/04/28 21:52:07, wolenetz wrote: > > > Is the following piece of that section A.2 Vorbis spec verified somewhere > > here? > > > :: > > > "The number of samples to be discarded must not exceed the overlap-add span > of > > > the first two audio packets." > > > > This is not verified. I'd fix that in a separate CL if that's something we > need > > to do. > > Seems low-pri (we might reject streams that are slightly out-of-spec that we > might otherwise allow). File a bug and reference it if you think this is > something we need to do please. I don't think it's necessary to worry about. https://codereview.chromium.org/259453003/diff/110001/media/base/test_helpers.cc File media/base/test_helpers.cc (right): https://codereview.chromium.org/259453003/diff/110001/media/base/test_helpers... media/base/test_helpers.cc:188: const float v = start + ch * output_size * increment; On 2014/04/29 00:22:44, wolenetz wrote: > s/float/T ? Whoops, good catch. Fixed. https://codereview.chromium.org/259453003/diff/110001/media/filters/ffmpeg_au... File media/filters/ffmpeg_audio_decoder.h (right): https://codereview.chromium.org/259453003/diff/110001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.h:84: // Used for computing output timestamps. On 2014/04/29 00:22:44, wolenetz wrote: > nit: discard_helper_ is used for more than that. Remove the comment? Done. https://codereview.chromium.org/259453003/diff/110001/media/filters/opus_audi... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/259453003/diff/110001/media/filters/opus_audi... media/filters/opus_audio_decoder.cc:435: discard_helper_->TimeDeltaToFrames(config_.seek_preroll())); On 2014/04/29 00:22:44, wolenetz wrote: > nit: disallow negative seek_preroll() from resetting the discard_helper in > release builds? TimeDeltaToFrames() should DCHECK in debug builds, but would > just return a really large number of frames in release builds. Check added in ConfigureDecoder() to ensure seek_preroll > 0.
https://codereview.chromium.org/259453003/diff/110001/media/filters/opus_audi... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/259453003/diff/110001/media/filters/opus_audi... media/filters/opus_audio_decoder.cc:435: discard_helper_->TimeDeltaToFrames(config_.seek_preroll())); On 2014/04/29 00:28:59, DaleCurtis wrote: > On 2014/04/29 00:22:44, wolenetz wrote: > > nit: disallow negative seek_preroll() from resetting the discard_helper in > > release builds? TimeDeltaToFrames() should DCHECK in debug builds, but would > > just return a really large number of frames in release builds. > > Check added in ConfigureDecoder() to ensure seek_preroll > 0. Actually not, since IsValidConfig() already verifies this.
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/259453003/130011
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_x64_rel
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/259453003/150001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/259453003/150001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on mac_chromium_rel
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/259453003/150001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/259453003/150001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/259453003/150001
The CQ bit was unchecked by dalecurtis@chromium.org
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/259453003/150001
Message was sent while issue was closed.
Change committed as 266953 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
