Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
436053002:
Make media::AudioClock track frames written to compute time. (Closed)
|
Created:
6 years, 4 months ago by scherkus (not reviewing) Modified:
6 years, 4 months ago Reviewers:
DaleCurtis CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionMake media::AudioClock track frames written to compute time.
Instead of using AudioRendererAlgorithm's timestamp as the ending
timestamp and counting "backwards", count "forwards" from a starting
timestamp to compute the current media time. Doing so produces more
accurate time calculations during periods where the playback rate
is changing.
last_endpoint_timestamp() is replaced by a new method that computes
the amount of contiguous media data buffered by audio hardware. Using
this value gives a more accurate maximum time value to use when doing
linear interpolation.
BUG=367343, 370634
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288445
Patch Set 1 : #
Total comments: 15
Patch Set 2 : fix std::max #Patch Set 3 : fixes #Patch Set 4 : fix time_cb #
Total comments: 6
Patch Set 5 : fix things up #
Total comments: 12
Patch Set 6 : #Patch Set 7 : nits #Patch Set 8 : rebase #Patch Set 9 : fix time update #Patch Set 10 : rebase #
Messages
Total messages: 23 (0 generated)
The idea seems sound, but recomputing a values which seemingly only change during WroteAudio() makes the implementation more complex than necessary. This may suffer more from drifting than the previous implementation in some cases if the muxed timestamps and durations are off. I.e. the YT bug, previously you would snap back every once in a while to actual media timeline when config changes or splice frames occurred. https://codereview.chromium.org/436053002/diff/40001/media/filters/audio_cloc... File media/filters/audio_clock.cc (right): https://codereview.chromium.org/436053002/diff/40001/media/filters/audio_cloc... media/filters/audio_clock.cc:33: int64_t played_frames = std::max(0L, TotalFrames(buffered_) - delay_frames); Seems like you could simply keep track of TotalFrames(buffered_) instead of recomputing it every time. https://codereview.chromium.org/436053002/diff/40001/media/filters/audio_cloc... media/filters/audio_clock.cc:57: for (size_t i = 0; i < played_.size(); ++i) { played_ never shrinks, so you should just cache current_timestamp and update it only based on new frames during WroteAudio(). Otherwise the degenerative flip-flop playback-rate case is expensive. Especially so on arm. https://codereview.chromium.org/436053002/diff/40001/media/filters/audio_cloc... media/filters/audio_clock.cc:60: current_timestamp += base::TimeDelta::FromMicroseconds( calculate this as a double or float and only divide by sample rate and convert to microseconds once. https://codereview.chromium.org/436053002/diff/40001/media/filters/audio_cloc... media/filters/audio_clock.cc:68: for (size_t i = 0; i < buffered_.size() && frames_played_since_writing > 0; You could cache this too and subtract off time_since_writing, no? Then you remove this loop here and in ContiguousAudioDataBuffered. https://codereview.chromium.org/436053002/diff/40001/media/filters/audio_cloc... media/filters/audio_clock.cc:72: current_timestamp += base::TimeDelta::FromMicroseconds( Ditto. https://codereview.chromium.org/436053002/diff/40001/media/filters/audio_cloc... media/filters/audio_clock.cc:105: buffered = base::TimeDelta::FromMicroseconds( Should this be += ? I don't understand why you have the loop otherwise. In which case I'm not sure how this differs from above? https://codereview.chromium.org/436053002/diff/40001/media/filters/audio_cloc... media/filters/audio_clock.cc:131: AudioClock::AudioData::AudioData(int64_t frames, float playback_rate) Up to you, but you can remove this and use a POD-type with = {} initialization.
On 2014/08/02 00:51:33, DaleCurtis wrote: > The idea seems sound, but recomputing a values which seemingly only change > during WroteAudio() makes the implementation more complex than necessary. I don't fully agree ... all that code now ended up in the body of WroteAudio() and we have to carry more state. It's also not a performance win since the calling code in AudioRendererImpl only called each public method once. But anyway take a look and see what you think.
https://codereview.chromium.org/436053002/diff/40001/media/filters/audio_cloc... File media/filters/audio_clock.cc (right): https://codereview.chromium.org/436053002/diff/40001/media/filters/audio_cloc... media/filters/audio_clock.cc:33: int64_t played_frames = std::max(0L, TotalFrames(buffered_) - delay_frames); On 2014/08/02 00:51:32, DaleCurtis wrote: > Seems like you could simply keep track of TotalFrames(buffered_) instead of > recomputing it every time. Done. https://codereview.chromium.org/436053002/diff/40001/media/filters/audio_cloc... media/filters/audio_clock.cc:57: for (size_t i = 0; i < played_.size(); ++i) { On 2014/08/02 00:51:33, DaleCurtis wrote: > played_ never shrinks, so you should just cache current_timestamp and update it > only based on new frames during WroteAudio(). Otherwise the degenerative > flip-flop playback-rate case is expensive. Especially so on arm. have to think about it some more ... but doesn't that put us at risk for introducing error over time? do you think it's worth optimizing for such a case? https://codereview.chromium.org/436053002/diff/40001/media/filters/audio_cloc... media/filters/audio_clock.cc:60: current_timestamp += base::TimeDelta::FromMicroseconds( On 2014/08/02 00:51:32, DaleCurtis wrote: > calculate this as a double or float and only divide by sample rate and convert > to microseconds once. Done. https://codereview.chromium.org/436053002/diff/40001/media/filters/audio_cloc... media/filters/audio_clock.cc:68: for (size_t i = 0; i < buffered_.size() && frames_played_since_writing > 0; On 2014/08/02 00:51:33, DaleCurtis wrote: > You could cache this too and subtract off time_since_writing, no? Then you > remove this loop here and in ContiguousAudioDataBuffered. Done. https://codereview.chromium.org/436053002/diff/40001/media/filters/audio_cloc... media/filters/audio_clock.cc:105: buffered = base::TimeDelta::FromMicroseconds( On 2014/08/02 00:51:33, DaleCurtis wrote: > Should this be += ? I don't understand why you have the loop otherwise. In > which case I'm not sure how this differs from above? Nah this was just silly. We always break so this doesn't even need to be a loop.
I'm a little unclear on what you meant by each method being called once. Since these methods are called on the audio thread they're called at up to 375Hz, though the common case is now ~47Hz. Did you mean they're only called once per Render()? https://codereview.chromium.org/436053002/diff/40001/media/filters/audio_cloc... File media/filters/audio_clock.cc (right): https://codereview.chromium.org/436053002/diff/40001/media/filters/audio_cloc... media/filters/audio_clock.cc:57: for (size_t i = 0; i < played_.size(); ++i) { On 2014/08/02 01:55:24, scherkus wrote: > On 2014/08/02 00:51:33, DaleCurtis wrote: > > played_ never shrinks, so you should just cache current_timestamp and update > it > > only based on new frames during WroteAudio(). Otherwise the degenerative > > flip-flop playback-rate case is expensive. Especially so on arm. > > have to think about it some more ... but doesn't that put us at risk for > introducing error over time? > > do you think it's worth optimizing for such a case? I'd guess only slightly more error than you're already introducing; if you store the timestamp as a double, it'd be equivalent to your present calculation. If you want to avoid as much error as possible you need to collect frames by playback_rate. I don't know how much playback rates flip flop in practice or how long it would take for this to be noticeable. I just wanted to point it out since it's a potentially hidden performance problem. If you do decide it's worth optimizing, I think it could be done simply with a float hash_map. Just note that this is called on the AudioThread at up to 375Hz, though the common case is now ~47Hz https://codereview.chromium.org/436053002/diff/40001/media/filters/audio_cloc... media/filters/audio_clock.cc:68: for (size_t i = 0; i < buffered_.size() && frames_played_since_writing > 0; On 2014/08/02 01:55:24, scherkus wrote: > On 2014/08/02 00:51:33, DaleCurtis wrote: > > You could cache this too and subtract off time_since_writing, no? Then you > > remove this loop here and in ContiguousAudioDataBuffered. > > Done. You said done, but didn't do this. Was that your intent? https://codereview.chromium.org/436053002/diff/40001/media/filters/audio_cloc... media/filters/audio_clock.cc:72: current_timestamp += base::TimeDelta::FromMicroseconds( On 2014/08/02 00:51:32, DaleCurtis wrote: > Ditto. I meant break out the TimeDelta conversion like above. https://codereview.chromium.org/436053002/diff/90001/media/filters/audio_cloc... File media/filters/audio_clock.cc (right): https://codereview.chromium.org/436053002/diff/90001/media/filters/audio_cloc... media/filters/audio_clock.cc:61: // Update cached values. Hmm, I didn't strictly mean a cache. I.e. the total_buffered_frames_ calculation can be kept in a running fashion. If you specialize PushAudioData into PushBufferedAudioData() and PushPlayedAudioData() you could adjust total_buffered_frames_ in each respectively. You could move the playback rate checks into PushBufferedData and extend DCHECKs into PushPlayedAudioData, etc. https://codereview.chromium.org/436053002/diff/90001/media/filters/audio_cloc... media/filters/audio_clock.cc:76: current_media_timestamp_ = Again, I didn't mean as a strict cache, but a rolling calculation. If you feel that introduces too much error your original code for calculating this at call time is better. https://codereview.chromium.org/436053002/diff/90001/media/filters/audio_cloc... media/filters/audio_clock.cc:90: scaled_frames = 0; If you calculate this first you can avoid the for loop above if contiguous_scaled_frames == 0. scaled_frames can then be initialized to the same value.
On 2014/08/04 18:55:19, DaleCurtis wrote: > I'm a little unclear on what you meant by each method being called once. Since > these methods are called on the audio thread they're called at up to 375Hz, > though the common case is now ~47Hz. Did you mean they're only called once per > Render()? Called once per Render(). IOW we're still doing the same amount of work, but we're making a code complexity trade off inside of WroteAudio() as well as keeping pre-calculated values in sync w/ |buffered_| and |played_|. Anyway I wanted to get some numbers so I tried out callgrind/kcachegrind. It shows a tiny improvement but in the grand scheme of things FillBuffer() is still the biggest offender (WSOLA's DecimatedSearch() in particular is ridiculously expensive)
I doubt you'll see much difference on x86, but similar reductions of division on single-precision arm machines have yielded an order of magnitude improvement in runtime. http://crbug.com/257424 In any case it's a small thing. I'm fine with keeping some of the functionality split into functions if you find that improves readability.
PTAL https://codereview.chromium.org/436053002/diff/90001/media/filters/audio_cloc... File media/filters/audio_clock.cc (right): https://codereview.chromium.org/436053002/diff/90001/media/filters/audio_cloc... media/filters/audio_clock.cc:61: // Update cached values. On 2014/08/04 18:55:19, DaleCurtis wrote: > Hmm, I didn't strictly mean a cache. I.e. the total_buffered_frames_ > calculation can be kept in a running fashion. If you specialize PushAudioData > into PushBufferedAudioData() and PushPlayedAudioData() you could adjust > total_buffered_frames_ in each respectively. > > You could move the playback rate checks into PushBufferedData and extend DCHECKs > into PushPlayedAudioData, etc. Reorganized code and helper methods. https://codereview.chromium.org/436053002/diff/90001/media/filters/audio_cloc... media/filters/audio_clock.cc:76: current_media_timestamp_ = On 2014/08/04 18:55:19, DaleCurtis wrote: > Again, I didn't mean as a strict cache, but a rolling calculation. If you feel > that introduces too much error your original code for calculating this at call > time is better. Ditto. https://codereview.chromium.org/436053002/diff/90001/media/filters/audio_cloc... media/filters/audio_clock.cc:90: scaled_frames = 0; On 2014/08/04 18:55:19, DaleCurtis wrote: > If you calculate this first you can avoid the for loop above if > contiguous_scaled_frames == 0. scaled_frames can then be initialized to the > same value. Ditto. https://codereview.chromium.org/436053002/diff/110001/media/filters/audio_clo... File media/filters/audio_clock.cc (right): https://codereview.chromium.org/436053002/diff/110001/media/filters/audio_clo... media/filters/audio_clock.cc:17: microseconds_per_frame_( Using a rolling calculation involving division actually introduced error pretty easily, so I went the AudioTimestampHelper route and eliminated division entirely.
https://codereview.chromium.org/436053002/diff/110001/media/filters/audio_clo... File media/filters/audio_clock.cc (right): https://codereview.chromium.org/436053002/diff/110001/media/filters/audio_clo... media/filters/audio_clock.cc:69: if (audio_data_buffered_) This is always true at this point, no? Also, found_silence will be true across loops, so this just breaks on the 2nd buffer if the first buffer contains silence. Are you special casing silence at the head of the buffer? Otherwise why not just break when you encounter playback_rate == 0 above? https://codereview.chromium.org/436053002/diff/110001/media/filters/audio_clo... media/filters/audio_clock.cc:74: scaled_frames += (buffered_[i].frames * buffered_[i].playback_rate); You can reuse this value below. https://codereview.chromium.org/436053002/diff/110001/media/filters/audio_clo... media/filters/audio_clock.cc:124: buffered_.front().frames -= frames_to_pop; Is this correct? You're subtracting frames at different scales. https://codereview.chromium.org/436053002/diff/110001/media/filters/audio_clo... media/filters/audio_clock.cc:138: media_time += base::TimeDelta::FromMicroseconds( Convert to int64/TimeDelta outside of loop like above?
https://codereview.chromium.org/436053002/diff/110001/media/filters/audio_clo... File media/filters/audio_clock.cc (right): https://codereview.chromium.org/436053002/diff/110001/media/filters/audio_clo... media/filters/audio_clock.cc:69: if (audio_data_buffered_) On 2014/08/05 01:21:31, DaleCurtis wrote: > This is always true at this point, no? Also, found_silence will be true across > loops, so this just breaks on the 2nd buffer if the first buffer contains > silence. > > Are you special casing silence at the head of the buffer? Otherwise why not > just break when you encounter playback_rate == 0 above? Nah this was just a result of shuffling some code around -- fixed. https://codereview.chromium.org/436053002/diff/110001/media/filters/audio_clo... media/filters/audio_clock.cc:74: scaled_frames += (buffered_[i].frames * buffered_[i].playback_rate); On 2014/08/05 01:21:31, DaleCurtis wrote: > You can reuse this value below. Done. https://codereview.chromium.org/436053002/diff/110001/media/filters/audio_clo... media/filters/audio_clock.cc:124: buffered_.front().frames -= frames_to_pop; On 2014/08/05 01:21:31, DaleCurtis wrote: > Is this correct? You're subtracting frames at different scales. I'm assuming you mean different playback rates, but yes this is correct. The only time we need to worry about scaling frames is computing media timestamps. https://codereview.chromium.org/436053002/diff/110001/media/filters/audio_clo... media/filters/audio_clock.cc:138: media_time += base::TimeDelta::FromMicroseconds( On 2014/08/05 01:21:31, DaleCurtis wrote: > Convert to int64/TimeDelta outside of loop like above? Done.
https://codereview.chromium.org/436053002/diff/110001/media/filters/audio_clo... File media/filters/audio_clock.cc (right): https://codereview.chromium.org/436053002/diff/110001/media/filters/audio_clo... media/filters/audio_clock.cc:124: buffered_.front().frames -= frames_to_pop; On 2014/08/05 01:44:01, scherkus wrote: > On 2014/08/05 01:21:31, DaleCurtis wrote: > > Is this correct? You're subtracting frames at different scales. > > I'm assuming you mean different playback rates, but yes this is correct. > > The only time we need to worry about scaling frames is computing media > timestamps. Don't you use this above to compute the media timestamps?
https://codereview.chromium.org/436053002/diff/110001/media/filters/audio_clo... File media/filters/audio_clock.cc (right): https://codereview.chromium.org/436053002/diff/110001/media/filters/audio_clo... media/filters/audio_clock.cc:124: buffered_.front().frames -= frames_to_pop; On 2014/08/05 01:45:51, DaleCurtis wrote: > On 2014/08/05 01:44:01, scherkus wrote: > > On 2014/08/05 01:21:31, DaleCurtis wrote: > > > Is this correct? You're subtracting frames at different scales. > > > > I'm assuming you mean different playback rates, but yes this is correct. > > > > The only time we need to worry about scaling frames is computing media > > timestamps. > > Don't you use this above to compute the media timestamps? If we the audio hardware tells us it's played 100 frames of audio, we remove 100 frames of buffered audio. If 50 of those frames are at rate 2x and the other 50 at 1x, ComputeBufferedMediaTime(100) on line 48 takes that into account prior to popping those 100 frames on line 51.
lgtm https://codereview.chromium.org/436053002/diff/110001/media/filters/audio_clo... File media/filters/audio_clock.cc (right): https://codereview.chromium.org/436053002/diff/110001/media/filters/audio_clo... media/filters/audio_clock.cc:124: buffered_.front().frames -= frames_to_pop; On 2014/08/05 01:57:35, scherkus wrote: > On 2014/08/05 01:45:51, DaleCurtis wrote: > > On 2014/08/05 01:44:01, scherkus wrote: > > > On 2014/08/05 01:21:31, DaleCurtis wrote: > > > > Is this correct? You're subtracting frames at different scales. > > > > > > I'm assuming you mean different playback rates, but yes this is correct. > > > > > > The only time we need to worry about scaling frames is computing media > > > timestamps. > > > > Don't you use this above to compute the media timestamps? > > If we the audio hardware tells us it's played 100 frames of audio, we remove 100 > frames of buffered audio. > > If 50 of those frames are at rate 2x and the other 50 at 1x, > ComputeBufferedMediaTime(100) on line 48 takes that into account prior to > popping those 100 frames on line 51. Ahh, right, my mistake.
Thanks for the review. Let's sync up on Tuesday to discuss drift and discontinuities. This change makes ARI much more strict about having audio data be contiguous (which is good!) but I believe we'll need to add some checks. On Aug 4, 2014 7:13 PM, <dalecurtis@chromium.org> wrote: > lgtm > > > > > https://codereview.chromium.org/436053002/diff/110001/ > media/filters/audio_clock.cc > File media/filters/audio_clock.cc (right): > > https://codereview.chromium.org/436053002/diff/110001/ > media/filters/audio_clock.cc#newcode124 > media/filters/audio_clock.cc:124: buffered_.front().frames -= > frames_to_pop; > On 2014/08/05 01:57:35, scherkus wrote: > >> On 2014/08/05 01:45:51, DaleCurtis wrote: >> > On 2014/08/05 01:44:01, scherkus wrote: >> > > On 2014/08/05 01:21:31, DaleCurtis wrote: >> > > > Is this correct? You're subtracting frames at different scales. >> > > >> > > I'm assuming you mean different playback rates, but yes this is >> > correct. > >> > > >> > > The only time we need to worry about scaling frames is computing >> > media > >> > > timestamps. >> > >> > Don't you use this above to compute the media timestamps? >> > > If we the audio hardware tells us it's played 100 frames of audio, we >> > remove 100 > >> frames of buffered audio. >> > > If 50 of those frames are at rate 2x and the other 50 at 1x, >> ComputeBufferedMediaTime(100) on line 48 takes that into account prior >> > to > >> popping those 100 frames on line 51. >> > > Ahh, right, my mistake. > > https://codereview.chromium.org/436053002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
dalecurtis@ and I discussed offline -- the data that arrives via AudioSpliceHelper etc should have already massaged the timestamps and frames, so this change (knocking on wood...) shouldn't make things any worse than they are today
The CQ bit was checked by scherkus@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scherkus@chromium.org/436053002/170001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by scherkus@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scherkus@chromium.org/436053002/210001
Message was sent while issue was closed.
Change committed as 288445 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
