Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
156783003:
Enhance AudioSplicer to crossfade marked splice frames. (Closed)
|
Created:
6 years, 10 months ago by DaleCurtis Modified:
6 years, 9 months ago Reviewers:
acolwell GONE FROM CHROMIUM CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionEnhance AudioSplicer to crossfade marked splice frames.
The AudioSplicer now allows external clients to indicate when a
splice frame is coming up with SetSpliceTimestamp().
Once set buffers involved in the splice are queued until enough
overlapping buffers are recieved to perform a crossfade.
To do this an AudioStreamSanitizer has been factorized out from
the existing AudioSplicer code. It handles ensure streams are
contiguous much in the way the splicer did before.
AudioSplicer itself now owns three separate AudioStreamSanitizers.
|sanitizer_| is the one which AudioSplicer callers will access.
|pre_splice_sanitizer_| for buffers with timestamps before the splice.
|post_splice_sanitizer_| for buffers with timestamps after the splice.
Once enough data for the crossfade is received (or end of stream)
the pre and post sanitizers are drained, trimmed, and crossfaded
in accordance with the MSE audio splice frame algorithm:
https://dvcs.w3.org/hg/html-media/raw-file/tip/media-source/media-source.html#sourcebuffer-audio-splice-rendering-algorithm
Future changes will enable SourceBufferStream to tag DecoderBuffers
with a splice timestamp which will be consumed by AudioBufferStream
and passed along to AudioSplicer.
BUG=334493
TEST=new unittests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=254279
Patch Set 1 : Clarity. #
Total comments: 4
Patch Set 2 : Resolve comments. #
Total comments: 16
Patch Set 3 : Fixes from unittest. #Patch Set 4 : More tests. #
Total comments: 32
Patch Set 5 : Comments. #
Total comments: 4
Patch Set 6 : Fixes. #
Total comments: 23
Patch Set 7 : Comments. #
Total comments: 4
Patch Set 8 : Comments. #
Messages
Total messages: 22 (0 generated)
My proposed solution for handling crossfading. I recommend reviewing for design only at this point. This can't be landed until the AudioBufferStream is created, since currently AudioSplicer isn't told about the input buffer. WDYT?
I don't think the recursive definition is quite right here. Perhaps we need to refactor/rename the AudioSplicer. The splicer right now is an "audio sample stream sanitizer". It makes AudioBuffer streams w/ gaps and overlaps into a sane stream. From what you have here, it seems like we still need that functionality before the actual crossfading code. It seems like the splicing and crossfading code needs to operate on top of this sanitization step and we aren't really dealing with a true recursive situation. I seems like the splice/cross-fading logic operates on a sequence of sanitized AudioBuffer groups and the splice_preroll() field & config changes simply signal how to handle the overlapping sanitized groups. https://codereview.chromium.org/156783003/diff/30001/media/base/audio_splicer.cc File media/base/audio_splicer.cc (right): https://codereview.chromium.org/156783003/diff/30001/media/base/audio_splicer... media/base/audio_splicer.cc:45: bool AudioSplicer::AddInput(const scoped_refptr<DecoderBuffer>& origin_buffer, Passing origin_buffer here doesn't feel right to me. It feels like code outside this class should be setting "splice preroll" state on this class instead of using origin_buffer since audio codec delay can make it difficult to determine which output audio is actually part of the preroll. For example Vorbis have a 1 frame delay so eventhough the second DecoderBuffer may have splice_preroll() set, the first AudioBuffer that comes out of the decoder actually might not be part of the splice. There needs to be code that associates DecoderBuffer timestamp with the timestamps on the AudioBuffer and makes sure the AudioBuffers are marked appropriately. https://codereview.chromium.org/156783003/diff/30001/media/base/audio_splicer.h File media/base/audio_splicer.h (right): https://codereview.chromium.org/156783003/diff/30001/media/base/audio_splicer... media/base/audio_splicer.h:55: AudioSplicer splice_frame_splicer_; How does this compile? This is a recursive declaration.
Reworked per our offline discussions. Still needs tests and such, so I recommend high level commentary only still... but this one at least compiles :p https://codereview.chromium.org/156783003/diff/30001/media/base/audio_splicer.cc File media/base/audio_splicer.cc (right): https://codereview.chromium.org/156783003/diff/30001/media/base/audio_splicer... media/base/audio_splicer.cc:45: bool AudioSplicer::AddInput(const scoped_refptr<DecoderBuffer>& origin_buffer, On 2014/02/10 20:19:57, acolwell wrote: > Passing origin_buffer here doesn't feel right to me. It feels like code outside > this class should be setting "splice preroll" state on this class instead of > using origin_buffer since audio codec delay can make it difficult to determine > which output audio is actually part of the preroll. For example Vorbis have a 1 > frame delay so eventhough the second DecoderBuffer may have splice_preroll() > set, the first AudioBuffer that comes out of the decoder actually might not be > part of the splice. There needs to be code that associates DecoderBuffer > timestamp with the timestamps on the AudioBuffer and makes sure the AudioBuffers > are marked appropriately. I've reworked this so that the future AudioBufferStream will call SetSpliceTimestamp() when decoder buffers marked as such are received. https://codereview.chromium.org/156783003/diff/30001/media/base/audio_splicer.h File media/base/audio_splicer.h (right): https://codereview.chromium.org/156783003/diff/30001/media/base/audio_splicer... media/base/audio_splicer.h:55: AudioSplicer splice_frame_splicer_; On 2014/02/10 20:19:57, acolwell wrote: > How does this compile? This is a recursive declaration. As discussed offline, it doesn't, sorry should have been more clear that this was a quick and dirty CL for design review purposes.
This looks good to me https://codereview.chromium.org/156783003/diff/90001/media/base/audio_splicer.cc File media/base/audio_splicer.cc (right): https://codereview.chromium.org/156783003/diff/90001/media/base/audio_splicer... media/base/audio_splicer.cc:59: // Get the current timestamp. This value is computed from based on the first nit: word missing? https://codereview.chromium.org/156783003/diff/90001/media/base/audio_splicer... media/base/audio_splicer.cc:198: : sanitizer_(new AudioStreamSanitizer(samples_per_second)), Are these pointers just so that you can hide the decl in this file? https://codereview.chromium.org/156783003/diff/90001/media/base/audio_splicer... media/base/audio_splicer.cc:219: return sanitizer_->AddInput(input); nit: s/sanitizer_/output_sanitizer_/? https://codereview.chromium.org/156783003/diff/90001/media/base/audio_splicer... media/base/audio_splicer.cc:281: ExtractCrossfadeFromPreroll(crossfade_bus_wrapper.get()); nit: s/Preroll/PreSplice/ ?. It seems like you are mixing terminology here. It seems you are using preroll & presplice in the same way so I think you should just pick one term and use it everywhere. I am slightly biased towards presplice or perhaps "lead-in" just so it isn't accidentally confused with the Pipeline's preroll concept. https://codereview.chromium.org/156783003/diff/90001/media/base/audio_splicer... media/base/audio_splicer.cc:317: if (splice_timestamp_ == splice_timestamp) Why are we allowing this? https://codereview.chromium.org/156783003/diff/90001/media/base/audio_splicer... media/base/audio_splicer.cc:323: post_splice_sanitizer_->Reset(); nit: I wonder if these should be at the bottom of AudioSplicer::AddInput() where you reset splice_timestamp_ to kNoTimestamp(). WDYT? https://codereview.chromium.org/156783003/diff/90001/media/base/audio_splicer... media/base/audio_splicer.cc:342: preroll->frame_count() * preroll->duration().InMillisecondsF() / nit: Any reason to not use SecondsF? It's 5 chars sorter. ;)
https://codereview.chromium.org/156783003/diff/90001/media/base/audio_splicer.cc File media/base/audio_splicer.cc (right): https://codereview.chromium.org/156783003/diff/90001/media/base/audio_splicer... media/base/audio_splicer.cc:59: // Get the current timestamp. This value is computed from based on the first On 2014/02/18 23:22:59, acolwell wrote: > nit: word missing? Yeah, the methods below need comments too; I just said it compiles this time :p https://codereview.chromium.org/156783003/diff/90001/media/base/audio_splicer... media/base/audio_splicer.cc:198: : sanitizer_(new AudioStreamSanitizer(samples_per_second)), On 2014/02/18 23:22:59, acolwell wrote: > Are these pointers just so that you can hide the decl in this file? Correct. I could move the decl to the header file if you prefer these to be stack allocated, but "style" is typically for decl+impl to be in the .cc for inner classes like this. https://codereview.chromium.org/156783003/diff/90001/media/base/audio_splicer... media/base/audio_splicer.cc:281: ExtractCrossfadeFromPreroll(crossfade_bus_wrapper.get()); On 2014/02/18 23:22:59, acolwell wrote: > nit: s/Preroll/PreSplice/ ?. It seems like you are mixing terminology here. It > seems you are using preroll & presplice in the same way so I think you should > just pick one term and use it everywhere. I am slightly biased towards presplice > or perhaps "lead-in" just so it isn't accidentally confused with the Pipeline's > preroll concept. I'm not partial to any names, I used preroll here since that was the same terminology we added to StreamParserBuffer -- "Fade out preroll". I'm fine with renaming these to avoid confusion with the pre-decode version. https://codereview.chromium.org/156783003/diff/90001/media/base/audio_splicer... media/base/audio_splicer.cc:317: if (splice_timestamp_ == splice_timestamp) On 2014/02/18 23:22:59, acolwell wrote: > Why are we allowing this? Essentially to allow callers to not have to worry about figuring out which splice timestamps have been provided already. It only needs to check if the splice timestamp is set or not and then pass that info along to the splicer. However, I think this isn't actually sufficient due to codec delay and packet slicing that we discussed earlier. I think we actually need the concept of pending splice timestamps (either via a queue or a pending_splice_timestamp variable; probably we'd never see a delay such that > 2 splices are active). Here's my vision: As DecoderBuffers with splice timestamps are received the AudioBufferStream will call AudioSplicer::SetSpliceTimestamp(), which will enqueue the splice timestamp (if it's not at the end of the queue already). As a splice is completed the sanitizers are reset and the next splice timestamp is pulled from the queue. Some consideration may have to be given for buffers which are after the current splice but involved with the next splice. Please double check my thinking here! It's possible we can just get away with the DCHECKs() I have here for now. Which would mean splices can never be so close that the next splice's DecoderBuffer is considered before the current splice finishes. Probably either way we should just change these to CHECKs() for now and see what happens in practice. https://codereview.chromium.org/156783003/diff/90001/media/base/audio_splicer... media/base/audio_splicer.cc:323: post_splice_sanitizer_->Reset(); On 2014/02/18 23:22:59, acolwell wrote: > nit: I wonder if these should be at the bottom of AudioSplicer::AddInput() where > you reset splice_timestamp_ to kNoTimestamp(). WDYT? I wondered that as well, I think it's fine.
This should be entirely ready for review now. Unittest all the things! https://codereview.chromium.org/156783003/diff/90001/media/base/audio_splicer.cc File media/base/audio_splicer.cc (right): https://codereview.chromium.org/156783003/diff/90001/media/base/audio_splicer... media/base/audio_splicer.cc:59: // Get the current timestamp. This value is computed from based on the first On 2014/02/19 03:05:14, DaleCurtis wrote: > On 2014/02/18 23:22:59, acolwell wrote: > > nit: word missing? > > Yeah, the methods below need comments too; I just said it compiles this time :p Done. https://codereview.chromium.org/156783003/diff/90001/media/base/audio_splicer... media/base/audio_splicer.cc:219: return sanitizer_->AddInput(input); On 2014/02/18 23:22:59, acolwell wrote: > nit: s/sanitizer_/output_sanitizer_/? Done. https://codereview.chromium.org/156783003/diff/90001/media/base/audio_splicer... media/base/audio_splicer.cc:323: post_splice_sanitizer_->Reset(); On 2014/02/19 03:05:14, DaleCurtis wrote: > On 2014/02/18 23:22:59, acolwell wrote: > > nit: I wonder if these should be at the bottom of AudioSplicer::AddInput() > where > > you reset splice_timestamp_ to kNoTimestamp(). WDYT? > > I wondered that as well, I think it's fine. Done. https://codereview.chromium.org/156783003/diff/90001/media/base/audio_splicer... media/base/audio_splicer.cc:342: preroll->frame_count() * preroll->duration().InMillisecondsF() / On 2014/02/18 23:22:59, acolwell wrote: > nit: Any reason to not use SecondsF? It's 5 chars sorter. ;) Done.
looking really good. Just a few comments. https://codereview.chromium.org/156783003/diff/280001/media/base/audio_buffer.cc File media/base/audio_buffer.cc (right): https://codereview.chromium.org/156783003/diff/280001/media/base/audio_buffer... media/base/audio_buffer.cc:226: floor(static_cast<double>(duration_.InMicroseconds()) * frames_to_trim / nit: Why can't you use (duration_ * frames_to_trim) / adjusted_frame_count_ here? It looks like the necessary operators are defined. https://codereview.chromium.org/156783003/diff/280001/media/base/audio_buffer... media/base/audio_buffer.cc:242: duration_ -= base::TimeDelta::FromMicroseconds( Why does this computation differ from the one above now? Doesn't this mean that the duration could be different if you remove the same number of frames from the start vs from the end? https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... File media/base/audio_splicer.cc (right): https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer.cc:258: CalculateFrameCountFromDuration(crossfade_duration); I think you use GetFramesToTarget() on the output splicer's timestamp helper just to make sure we don't accidentally introduce error because the CalculateFrameCountFromDuration() computation always rounds up. https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer.cc:270: ExtractCrossfadeFromPreSplice(pre_splice_bus.get()); nit: Any reason to not have this method just return the scoped_ptr<AudioBus>? scoped_ptr<AudioBus> pre_splice_bus = ExtractCrossfadeFromPreSplice(frames_to_crossfade); https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer.cc:282: AudioBus::CreateWrapper(crossfade_buffer->channel_count()); nit: Lines 282-287 look like they could be moved into a AudioBus::CreateWrapper(const scoped_refptr<AudioBuffer>&) method. WDYT? https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer.cc:292: CHECK(output_sanitizer_->AddInput(crossfade_buffer)); nit: This seems weird to add it to the output before we have actually rendered the crossfade. Please move this to line 303? https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer.cc:340: // This should only happen if the splice point is within the preroll nit: s/preroll/|preroll|/ https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer.cc:341: // buffer somewhere. Early code should have put it in |sanitizer_| nit: s/sanitizer_/output_sanitizer_/? https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer.cc:343: DCHECK(preroll->timestamp() + preroll->duration() >= splice_timestamp_); I wonder if we should create an end_timestamp() to avoid (preroll->timestamp() + preroll->duration()) in a bunch of places? https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer.cc:345: ceil(preroll->frame_count() * I think we should use GetFramesToTarget() on output_sanitizer_ AudioTimestampHelper to determine how many frames we need. Using a computation like this can lead to errors that the output sanitizer would end up having to fix. In general, if you are having to do frame <-> timestamp conversions an AudioTimestampHelper should be involved to avoid accidentally introducing rounding errors. That is why I created it. :) https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splicer.h File media/base/audio_splicer.h (right): https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer.h:65: scoped_ptr<AudioStreamSanitizer> pre_splice_sanitizer_; nit: Docs for these so it is easier to understand what goes where. https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... File media/base/audio_splicer_unittest.cc (right): https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer_unittest.cc:79: CalculateFrameCountFromDuration(kExpectedPreSpliceDuration); Again. Not huge fan of this. I'd prefer it if you'd use an AudioTimestampHelper to compute/verify this info. https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer_unittest.cc:110: CalculateFrameCountFromDuration(kExpectedCrossfadeDuration); ditto https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer_unittest.cc:124: const float actual = bus->channel(0)[i]; nit: Any reason not to verify all channels? https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer_unittest.cc:136: input->end_of_stream() nit: It seems like this should be AudioBuffer::CopyFrom(input) since I'm sure this helper would be useful elsewhere. https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer_unittest.cc:173: VerifyNextBuffer(input_3, 0.3f); Thanks for cleaning all these up. I'm a little embarassed about all the duplication. :/
<Just comments> I'll take a stab at rewriting all the timestamp manipulation everywhere with AudioTimestampHelpers. https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... File media/base/audio_splicer.cc (right): https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer.cc:282: AudioBus::CreateWrapper(crossfade_buffer->channel_count()); On 2014/02/24 21:46:11, acolwell wrote: > nit: Lines 282-287 look like they could be moved into a > AudioBus::CreateWrapper(const scoped_refptr<AudioBuffer>&) method. WDYT? It's only used here, so I don't think it warrants adding to AudioBus, we expose the SetChannelData() code for one offs like this. https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer.cc:292: CHECK(output_sanitizer_->AddInput(crossfade_buffer)); On 2014/02/24 21:46:11, acolwell wrote: > nit: This seems weird to add it to the output before we have actually rendered > the crossfade. Please move this to line 303? Can't since then the buffers added during ExtractCrossfadeFromPostSplice will be out of order. https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer.cc:343: DCHECK(preroll->timestamp() + preroll->duration() >= splice_timestamp_); On 2014/02/24 21:46:11, acolwell wrote: > I wonder if we should create an end_timestamp() to avoid (preroll->timestamp() + > preroll->duration()) in a bunch of places? Only two occurrences in this code. Seems like something for a cross cutting CL later if there are instances elsewhere. https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... File media/base/audio_splicer_unittest.cc (right): https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer_unittest.cc:124: const float actual = bus->channel(0)[i]; On 2014/02/24 21:46:11, acolwell wrote: > nit: Any reason not to verify all channels? There's only one channel, so this was simpler, but sure why not :) https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer_unittest.cc:136: input->end_of_stream() On 2014/02/24 21:46:11, acolwell wrote: > nit: It seems like this should be AudioBuffer::CopyFrom(input) since I'm sure > this helper would be useful elsewhere. A general purpose method is trickier. This method only works because their are no trimmed frames. AudioBuffer::CopyFrom() wouldn't be an exact internal copy if there are trimmed frames. From the exterior it would look the same though, so maybe that's not a problem.
Rewrote to use AudioTimestampHelper for the cost of a couple hundred lines :) AudioTimestampHelper is the source of all truthiness and edge cases where the decoder fuzzes the timestamps and durations are now correctly handled (I think). https://codereview.chromium.org/156783003/diff/280001/media/base/audio_buffer.cc File media/base/audio_buffer.cc (right): https://codereview.chromium.org/156783003/diff/280001/media/base/audio_buffer... media/base/audio_buffer.cc:226: floor(static_cast<double>(duration_.InMicroseconds()) * frames_to_trim / On 2014/02/24 21:46:11, acolwell wrote: > nit: Why can't you use (duration_ * frames_to_trim) / adjusted_frame_count_ > here? It looks like the necessary operators are defined. Removed in favor of using the AudioTimestampHelper to perform these calculations when available. https://codereview.chromium.org/156783003/diff/280001/media/base/audio_buffer... media/base/audio_buffer.cc:242: duration_ -= base::TimeDelta::FromMicroseconds( On 2014/02/24 21:46:11, acolwell wrote: > Why does this computation differ from the one above now? Doesn't this mean that > the duration could be different if you remove the same number of frames from the > start vs from the end? Ditto. https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... File media/base/audio_splicer.cc (right): https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer.cc:258: CalculateFrameCountFromDuration(crossfade_duration); On 2014/02/24 21:46:11, acolwell wrote: > I think you use GetFramesToTarget() on the output splicer's timestamp helper > just to make sure we don't accidentally introduce error because the > CalculateFrameCountFromDuration() computation always rounds up. Done. https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer.cc:270: ExtractCrossfadeFromPreSplice(pre_splice_bus.get()); On 2014/02/24 21:46:11, acolwell wrote: > nit: Any reason to not have this method just return the scoped_ptr<AudioBus>? > scoped_ptr<AudioBus> pre_splice_bus = > ExtractCrossfadeFromPreSplice(frames_to_crossfade); Done. https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer.cc:340: // This should only happen if the splice point is within the preroll On 2014/02/24 21:46:11, acolwell wrote: > nit: s/preroll/|preroll|/ Removed. https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer.cc:341: // buffer somewhere. Early code should have put it in |sanitizer_| On 2014/02/24 21:46:11, acolwell wrote: > nit: s/sanitizer_/output_sanitizer_/? Removed. https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer.cc:345: ceil(preroll->frame_count() * On 2014/02/24 21:46:11, acolwell wrote: > I think we should use GetFramesToTarget() on output_sanitizer_ > AudioTimestampHelper to determine how many frames we need. Using a computation > like this can lead to errors that the output sanitizer would end up having to > fix. > > In general, if you are having to do frame <-> timestamp conversions an > AudioTimestampHelper should be involved to avoid accidentally introducing > rounding errors. That is why I created it. :) Done. https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splicer.h File media/base/audio_splicer.h (right): https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer.h:65: scoped_ptr<AudioStreamSanitizer> pre_splice_sanitizer_; On 2014/02/24 21:46:11, acolwell wrote: > nit: Docs for these so it is easier to understand what goes where. Done. https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... File media/base/audio_splicer_unittest.cc (right): https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer_unittest.cc:79: CalculateFrameCountFromDuration(kExpectedPreSpliceDuration); On 2014/02/24 21:46:11, acolwell wrote: > Again. Not huge fan of this. I'd prefer it if you'd use an AudioTimestampHelper > to compute/verify this info. Done. https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer_unittest.cc:110: CalculateFrameCountFromDuration(kExpectedCrossfadeDuration); On 2014/02/24 21:46:11, acolwell wrote: > ditto Done. https://codereview.chromium.org/156783003/diff/280001/media/base/audio_splice... media/base/audio_splicer_unittest.cc:124: const float actual = bus->channel(0)[i]; On 2014/02/24 23:04:09, DaleCurtis wrote: > On 2014/02/24 21:46:11, acolwell wrote: > > nit: Any reason not to verify all channels? > > There's only one channel, so this was simpler, but sure why not :) Done.
https://codereview.chromium.org/156783003/diff/430001/media/base/audio_splice... File media/base/audio_splicer.cc (right): https://codereview.chromium.org/156783003/diff/430001/media/base/audio_splice... media/base/audio_splicer.cc:86: scoped_ptr<AudioStreamSanitizer> ShallowCopy(); After sleeping on it, I'm going to change this to a new AudioStreamSanitizer constructor which takes a timestamp helper object since I'm already exposing the timestamp_helper below.
https://codereview.chromium.org/156783003/diff/430001/media/base/audio_splice... File media/base/audio_splicer.cc (right): https://codereview.chromium.org/156783003/diff/430001/media/base/audio_splice... media/base/audio_splicer.cc:389: std::min(pre_splice_ts_helper.frame_count() - frames_before_splice, Also this is wrong since I'm cloning the output_sanitizer for the pre-splice sanitizer. Will fix.
https://codereview.chromium.org/156783003/diff/430001/media/base/audio_splice... File media/base/audio_splicer.cc (right): https://codereview.chromium.org/156783003/diff/430001/media/base/audio_splice... media/base/audio_splicer.cc:86: scoped_ptr<AudioStreamSanitizer> ShallowCopy(); On 2014/02/26 18:51:00, DaleCurtis wrote: > After sleeping on it, I'm going to change this to a new AudioStreamSanitizer > constructor which takes a timestamp helper object since I'm already exposing the > timestamp_helper below. Fixed with a new Reset(frames, base_timestamp) method. https://codereview.chromium.org/156783003/diff/430001/media/base/audio_splice... media/base/audio_splicer.cc:389: std::min(pre_splice_ts_helper.frame_count() - frames_before_splice, On 2014/02/26 23:41:52, DaleCurtis wrote: > Also this is wrong since I'm cloning the output_sanitizer for the pre-splice > sanitizer. Will fix. Fixed with new GetFrameCount() method.
https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... File media/base/audio_splicer.cc (right): https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... media/base/audio_splicer.cc:51: buffer->TrimEnd(frames_to_trim); nit: DCHECK_EQ(buffer->timestamp(), timestamp_helper.GetTimestamp()) ? https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... media/base/audio_splicer.cc:56: typedef std::deque<scoped_refptr<AudioBuffer> > BufferQueue; nit: I think this should be inside the AudioStreamSanitizer since that appears to be the only place it is being used right now. https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... media/base/audio_splicer.cc:224: for(BufferQueue::const_iterator it = output_buffers_.begin(); nit: space after for https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... media/base/audio_splicer.cc:262: return output_sanitizer_->AddInput(input); DCHECK(!pre_splice_sanitizer_->HasNextBuffer()); DCHECK(!post_splice_sanitizer_->HasNextBuffer()); in this block just to make sure our assumptions below hold? https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... media/base/audio_splicer.cc:272: return output_sanitizer_->AddInput(input); DCHECK(!pre_splice_sanitizer_->HasNextBuffer()); here just to make sure we don't allow anything to sneak into the output_sanitizer_? https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... media/base/audio_splicer.cc:287: input->timestamp() != splice_timestamp_) { We can rely on the "input->timestamp() != splice_timestamp_" condition because we know the splice always happens in the middle of a pre-splice frame? This is a little subtle and doesn't really reflect what the comment above is saying. I wonder if something like !pre_splice_sanitizer_->IsOverlapping(input->timestamp()) where bool IsOverlapping(TimeDelta timestamp) { return timestamp >= output_timestamp_helper_.GetTimestamp() } https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... media/base/audio_splicer.cc:337: CHECK(output_sanitizer_->AddInput(crossfade_buffer)); This seems risky since AddInput() can modify crossfade_buffer's state. I realize that we shouldn't trigger the trim logic in there, but I think in general there is an implict contract that the buffer isn't going to be modified by the caller once it is passed into the sanitizer. I think ExtractCrossfadeFromPostSplice() should just pass the one potential partial buffer out through an out param if necessary and then do all the appending to the output_sanitizer_ after the crossfade is computed. https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... media/base/audio_splicer.cc:381: output_sanitizer_->SetBaseTimestamp( Could you use ResetTimestampState() here instead? I think then you won't need the SetBaseTimestamp() method. IIUC this can only happen if the splice is the first thing that happens and no buffers have been output yet. Right? https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... File media/base/audio_splicer_unittest.cc (right): https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... media/base/audio_splicer_unittest.cc:109: const base::TimeDelta kSpliceTimestamp = I'm a little concerned that we are trying to replicate too much of the real logic in the tests. How about just verifying the timestamps, durations, and sizes against constants instead? It would allow us to determine if the behavior changes. You could probably also just summarize the crossfade by summing all the samples and comparing it to frame_count * (a + b) / 2. I think that would result in far less test code.
https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... File media/base/audio_splicer.cc (right): https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... media/base/audio_splicer.cc:287: input->timestamp() != splice_timestamp_) { On 2014/02/27 17:10:14, acolwell wrote: > We can rely on the "input->timestamp() != splice_timestamp_" condition because > we know the splice always happens in the middle of a pre-splice frame? This is a > little subtle and doesn't really reflect what the comment above is saying. I > wonder if something like > !pre_splice_sanitizer_->IsOverlapping(input->timestamp()) > > where > bool IsOverlapping(TimeDelta timestamp) { > return timestamp >= output_timestamp_helper_.GetTimestamp() > } It's only strong enough when included with the !pre_splice_sanitizer_->HasNextBuffer() check, since we could have a perfect overlap. We can rely on it because we expect the first overlapping block to have a timestamp of exactly splice_timestamp (config change, first base_timestamp == splice timestamp). This is definitely subtle... but it's not sufficient to check timestamp >= GetTimestamp() since there may be decoder fuzz in the timestamps. https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... media/base/audio_splicer.cc:381: output_sanitizer_->SetBaseTimestamp( On 2014/02/27 17:10:14, acolwell wrote: > Could you use ResetTimestampState() here instead? I think then you won't need > the SetBaseTimestamp() method. > > IIUC this can only happen if the splice is the first thing that happens and no > buffers have been output yet. Right? Correct and a good idea, but I I was thinking of changing ResetTimestampState() to take only a timestamp_helper() and extract the necessary information itself. Which do you prefer? https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... File media/base/audio_splicer_unittest.cc (right): https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... media/base/audio_splicer_unittest.cc:109: const base::TimeDelta kSpliceTimestamp = On 2014/02/27 17:10:14, acolwell wrote: > I'm a little concerned that we are trying to replicate too much of the real > logic in the tests. How about just verifying the timestamps, durations, and > sizes against constants instead? It would allow us to determine if the behavior > changes. You could probably also just summarize the crossfade by summing all the > samples and comparing it to frame_count * (a + b) / 2. I think that would result > in far less test code. I generally dislike constant only verification in test code since they're: a) hard to maintain. b) not explanatory. They're more a test of "ensure this doesn't change" rather than a correctness test. However I am wary that this may silently adapt to incorrect changes since it bases expected timestamps based on what's handed out. The correctness verification is subtle. The constants in this case are slightly confusing too they won't match exactly what the buffers are constructed with. They're off by a microsecond here or there depend on floating point precision (which means these values may be different on ARM too). We can try it though and see what happens.
https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... File media/base/audio_splicer.cc (right): https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... media/base/audio_splicer.cc:51: buffer->TrimEnd(frames_to_trim); On 2014/02/27 17:10:14, acolwell wrote: > nit: DCHECK_EQ(buffer->timestamp(), timestamp_helper.GetTimestamp()) ? Done. https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... media/base/audio_splicer.cc:56: typedef std::deque<scoped_refptr<AudioBuffer> > BufferQueue; On 2014/02/27 17:10:14, acolwell wrote: > nit: I think this should be inside the AudioStreamSanitizer since that appears > to be the only place it is being used right now. Done. https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... media/base/audio_splicer.cc:224: for(BufferQueue::const_iterator it = output_buffers_.begin(); On 2014/02/27 17:10:14, acolwell wrote: > nit: space after for Done. https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... media/base/audio_splicer.cc:262: return output_sanitizer_->AddInput(input); On 2014/02/27 17:10:14, acolwell wrote: > DCHECK(!pre_splice_sanitizer_->HasNextBuffer()); > DCHECK(!post_splice_sanitizer_->HasNextBuffer()); > > in this block just to make sure our assumptions below hold? Done. https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... media/base/audio_splicer.cc:272: return output_sanitizer_->AddInput(input); On 2014/02/27 17:10:14, acolwell wrote: > DCHECK(!pre_splice_sanitizer_->HasNextBuffer()); here just to make sure we don't > allow anything to sneak into the output_sanitizer_? Done. https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... media/base/audio_splicer.cc:337: CHECK(output_sanitizer_->AddInput(crossfade_buffer)); On 2014/02/27 17:10:14, acolwell wrote: > This seems risky since AddInput() can modify crossfade_buffer's state. I realize > that we shouldn't trigger the trim logic in there, but I think in general there > is an implict contract that the buffer isn't going to be modified by the caller > once it is passed into the sanitizer. > > I think ExtractCrossfadeFromPostSplice() should just pass the one potential > partial buffer out through an out param if necessary and then do all the > appending to the output_sanitizer_ after the crossfade is computed. This is messy since then trimming and the accuracy update are split across the main loop and the extraction method. I much prefer the current setup since it the ExtractXXX methods have symmetry which makes them easy to understand. Instead I've added a DCHECK() to ensure trim is not invoked. I can promote that to a check if necessary. But in either case it wouldn't be a use after free since AudioBuffer isn't actually shrunk during trimming. https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... File media/base/audio_splicer_unittest.cc (right): https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... media/base/audio_splicer_unittest.cc:109: const base::TimeDelta kSpliceTimestamp = On 2014/02/27 19:45:01, DaleCurtis wrote: > On 2014/02/27 17:10:14, acolwell wrote: > > I'm a little concerned that we are trying to replicate too much of the real > > logic in the tests. How about just verifying the timestamps, durations, and > > sizes against constants instead? It would allow us to determine if the > behavior > > changes. You could probably also just summarize the crossfade by summing all > the > > samples and comparing it to frame_count * (a + b) / 2. I think that would > result > > in far less test code. > > I generally dislike constant only verification in test code since they're: a) > hard to maintain. b) not explanatory. They're more a test of "ensure this > doesn't change" rather than a correctness test. However I am wary that this may > silently adapt to incorrect changes since it bases expected timestamps based on > what's handed out. The correctness verification is subtle. > > The constants in this case are slightly confusing too they won't match exactly > what the buffers are constructed with. They're off by a microsecond here or > there depend on floating point precision (which means these values may be > different on ARM too). > > We can try it though and see what happens. Mostly done. I also cleaned up the verify syntax. I've left the crossfade verification in because your proposal wouldn't be any shorter.
Looks good. Just a few minor issue and I think you should be good to go. https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... File media/base/audio_splicer.cc (right): https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... media/base/audio_splicer.cc:287: input->timestamp() != splice_timestamp_) { On 2014/02/27 19:45:01, DaleCurtis wrote: > On 2014/02/27 17:10:14, acolwell wrote: > > We can rely on the "input->timestamp() != splice_timestamp_" condition because > > we know the splice always happens in the middle of a pre-splice frame? This is > a > > little subtle and doesn't really reflect what the comment above is saying. I > > wonder if something like > > !pre_splice_sanitizer_->IsOverlapping(input->timestamp()) > > > > where > > bool IsOverlapping(TimeDelta timestamp) { > > return timestamp >= output_timestamp_helper_.GetTimestamp() > > } > > It's only strong enough when included with the > !pre_splice_sanitizer_->HasNextBuffer() check, since we could have a perfect > overlap. > > We can rely on it because we expect the first overlapping block to have a > timestamp of exactly splice_timestamp (config change, first base_timestamp == > splice timestamp). > > This is definitely subtle... but it's not sufficient to check timestamp >= > GetTimestamp() since there may be decoder fuzz in the timestamps. Ok then some of this should be in the comment then because I'm sure we'll both forget it. :) https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... media/base/audio_splicer.cc:337: CHECK(output_sanitizer_->AddInput(crossfade_buffer)); On 2014/02/28 00:24:41, DaleCurtis wrote: > This is messy since then trimming and the accuracy update are split across the > main loop and the extraction method. > > I much prefer the current setup since it the ExtractXXX methods have symmetry > which makes them easy to understand. > > Instead I've added a DCHECK() to ensure trim is not invoked. I can promote that > to a check if necessary. But in either case it wouldn't be a use after free > since AudioBuffer isn't actually shrunk during trimming. I still think this is a bad idea. You could move the creation of crossfade_buffer and the actual crossfade down into ExtractCrossfadeFromPostSplice(). I don't see any reason this code needs to be up here. That would a be more natural setup. While I am sympathetic to the symmetry argument, I don't think it is a good idea to rely on internal details of AudioBuffer & AudioStreamSanitizer like this. AddInput() is essentially an ownership transfer so I don't think you should be modifying the contents of the buffer once you've passed it into this method. https://codereview.chromium.org/156783003/diff/490001/media/base/audio_splice... File media/base/audio_splicer.cc (left): https://codereview.chromium.org/156783003/diff/490001/media/base/audio_splice... media/base/audio_splicer.cc:108: // TODO(acolwell): Implement a cross-fade here so the transition is less nit: I think this comment should stay. For the "badly encoded stream" case it would be nice to have a simple crossfade to make the transition a little less jarring especially if there is a step function type change in loudness. https://codereview.chromium.org/156783003/diff/490001/media/base/audio_splice... File media/base/audio_splicer.cc (right): https://codereview.chromium.org/156783003/diff/490001/media/base/audio_splice... media/base/audio_splicer.cc:324: AudioBus::CreateWrapper(crossfade_buffer->channel_count()); nit: Please move this and the following 5 lines into a helper method. I think it makes this section less readable.
https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... File media/base/audio_splicer.cc (right): https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... media/base/audio_splicer.cc:287: input->timestamp() != splice_timestamp_) { On 2014/02/28 18:50:27, acolwell wrote: > On 2014/02/27 19:45:01, DaleCurtis wrote: > > On 2014/02/27 17:10:14, acolwell wrote: > > > We can rely on the "input->timestamp() != splice_timestamp_" condition > because > > > we know the splice always happens in the middle of a pre-splice frame? This > is > > a > > > little subtle and doesn't really reflect what the comment above is saying. I > > > wonder if something like > > > !pre_splice_sanitizer_->IsOverlapping(input->timestamp()) > > > > > > where > > > bool IsOverlapping(TimeDelta timestamp) { > > > return timestamp >= output_timestamp_helper_.GetTimestamp() > > > } > > > > It's only strong enough when included with the > > !pre_splice_sanitizer_->HasNextBuffer() check, since we could have a perfect > > overlap. > > > > We can rely on it because we expect the first overlapping block to have a > > timestamp of exactly splice_timestamp (config change, first base_timestamp == > > splice timestamp). > > > > This is definitely subtle... but it's not sufficient to check timestamp >= > > GetTimestamp() since there may be decoder fuzz in the timestamps. > > Ok then some of this should be in the comment then because I'm sure we'll both > forget it. :) Done. I also added a CHECK() for the unlikely case that the decoder fuzzes the n>1'th pre splice buffer such that splice_timestamp is seen before it's ready. https://codereview.chromium.org/156783003/diff/450001/media/base/audio_splice... media/base/audio_splicer.cc:337: CHECK(output_sanitizer_->AddInput(crossfade_buffer)); On 2014/02/28 18:50:27, acolwell wrote: > On 2014/02/28 00:24:41, DaleCurtis wrote: > > This is messy since then trimming and the accuracy update are split across the > > main loop and the extraction method. > > > > I much prefer the current setup since it the ExtractXXX methods have symmetry > > which makes them easy to understand. > > > > Instead I've added a DCHECK() to ensure trim is not invoked. I can promote > that > > to a check if necessary. But in either case it wouldn't be a use after free > > since AudioBuffer isn't actually shrunk during trimming. > > I still think this is a bad idea. You could move the creation of > crossfade_buffer and the actual crossfade down into > ExtractCrossfadeFromPostSplice(). I don't see any reason this code needs to be > up here. That would a be more natural setup. > > While I am sympathetic to the symmetry argument, I don't think it is a good idea > to rely on internal details of AudioBuffer & AudioStreamSanitizer like this. > AddInput() is essentially an ownership transfer so I don't think you should be > modifying the contents of the buffer once you've passed it into this method. Switching to a CrossfadePostSplice() method looks good. Done. https://codereview.chromium.org/156783003/diff/490001/media/base/audio_splice... File media/base/audio_splicer.cc (left): https://codereview.chromium.org/156783003/diff/490001/media/base/audio_splice... media/base/audio_splicer.cc:108: // TODO(acolwell): Implement a cross-fade here so the transition is less On 2014/02/28 18:50:27, acolwell wrote: > nit: I think this comment should stay. For the "badly encoded stream" case it > would be nice to have a simple crossfade to make the transition a little less > jarring especially if there is a step function type change in loudness. I'll leave it, but it's kind of impossible to implement given how the splicer is used, so it doesn't seem like an effective comment. https://codereview.chromium.org/156783003/diff/490001/media/base/audio_splice... File media/base/audio_splicer.cc (right): https://codereview.chromium.org/156783003/diff/490001/media/base/audio_splice... media/base/audio_splicer.cc:324: AudioBus::CreateWrapper(crossfade_buffer->channel_count()); On 2014/02/28 18:50:27, acolwell wrote: > nit: Please move this and the following 5 lines into a helper method. I think it > makes this section less readable. Done.
Awesome! \o/ thanks for doing this. LGTM
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/156783003/510001
Thanks for the review!
Message was sent while issue was closed.
Change committed as 254279 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
