Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
19111004:
Upgrade AudioRendererAlgorithm to use WSOLA, (Closed)
|
Created:
7 years, 5 months ago by turaj Modified:
7 years, 3 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUpgrade AudioRendererAlgorithm to use WSOLA,
Replaces the old and crufty time scaling solution with a
Waveform-Similarity-Overlap-Add (WSOLA) approach.
BUG=40452
TEST=media_unittests, manual performance and functional tests.
R=dalecurtis@chromium.org, marpan@google.com
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220343
Patch Set 1 #
Total comments: 66
Patch Set 2 : Andrew's comments addressed. Unit test for WSOLA algorithm is added. #Patch Set 3 : Avoid malloc in every iteration by defining some member varibles. #
Total comments: 41
Patch Set 4 : "Marco's comments are adressed." #
Total comments: 26
Patch Set 5 : "Dale's and Marco's comments are addressed." #
Total comments: 102
Patch Set 6 : Adressing Dale's comments. #
Total comments: 10
Patch Set 7 : Comments addressed, AudioRendererAlgorithmTest::WsolaTest modified. #
Total comments: 1
Patch Set 8 : Valgrind warning addressed, and small fixes to unittests. #
Total comments: 11
Patch Set 9 : Minor changes. #Patch Set 10 : Unit-test nit addressed #Patch Set 11 : Adding MEDIA_EXPORT #
Total comments: 2
Patch Set 12 : Correcting gyp #Patch Set 13 : Avoiding un-aligned memory. #Patch Set 14 : Fixes for try server failure. #
Messages
Total messages: 52 (0 generated)
Hi, This is the implementation of WSOLA. I added most of the unit tests that I wanted to add. I'm thinking of adding one more unit test where I insert a periodic signal. If there is integer number of periods in a WSOLA window, then the output should be the same signal as input with more periods. I have tested this in MATLAB and it works. But haven't implemented it here, yet. Using the same technique, I will add one or two unit test to make sure the other methods behave as they are expected. I was not sure whether it is better that the unit tests for function in audio_renderer_algorithm_util.cc to be in audio_renderer_algorithm_unittest.cc or in a separate file. I put them in audio_renderer_algorithm_unittest.cc. Thanks,... Turaj
Nice work Turaj! I'm still digging through this, some initial comments below which apply to all files. Is there someone from the signal processing team who can review this for algorithmic correctness? I'll only be reviewing from a chrome / general code correctness perspective. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... File media/filters/audio_renderer_algorithm_util.cc (right): https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm_util.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2013 and no (c). Here and other files. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm_util.cc:17: bool InInterval(int n, interval q) { Types should have the first letter capitalized; so Interval instead of interval. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm_util.cc:27: similarity_measure += dot_prod_a_b[n] / sqrt(energy_a[n] * energy_b[n] + What is 1e-12 for? Additionally vector_math.h might be a good home for some of the vector math primitives used by these functions. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm_util.cc:39: DCHECK(a->channels() == b->channels()); use DCHECK_EQ, DCHECK_GE, DCHECK_LE etc, instead.
Whoops, one comment got dropped: Instead of having AudioRendererAlgorithm::Wsola() and AudioRendererAlgorithmUtil splitting dependencies, would it be better to have a pure WSOLA class which contains the requisite bits from AudioRendererAlgorithm and merges in AudioRendererAlgorithmUtil? Then AudioRendererAlgorithm can just use the WSOLA class. In the future we may want other components which can use WSOLA, so having it available as a single unit would be pretty handy.
Thanks, for the review and the comments. Having a class performing WSOLA is a great idea. The reason I created a new file as utility was mainly for the purpose of testing. I started with having all function in audio_renderer_algorithm_util as static in audio_renderer_algorithm. But then testing became rather messy. Do you think it is fine if I have wsola.cc/.h and wsola_util.cc/.h and their corresponding unit-tests. On Mon, Jul 15, 2013 at 5:22 PM, <dalecurtis@chromium.org> wrote: > Whoops, one comment got dropped: Instead of having > AudioRendererAlgorithm::Wsola(**) and AudioRendererAlgorithmUtil splitting > dependencies, would it be better to have a pure WSOLA class which contains > the > requisite bits from AudioRendererAlgorithm and merges in > AudioRendererAlgorithmUtil? Then AudioRendererAlgorithm can just use the > WSOLA > class. > > In the future we may want other components which can use WSOLA, so having > it > available as a single unit would be pretty handy. > > https://codereview.chromium.**org/19111004/<https://codereview.chromium.org/1... >
wsola.{cc,h} definitely. No to wsola_util.{cc,h} since it's doubtful anyone
else would be using those functions. How about wsola_internals.{cc,h} and put
them in the media::internal namespace. A similar pattern is used elsewhere in
chrome.
Agreed that they are specific to WSOLA, wsola_internals.{cc, h} is good
suggestion. Will do as advised.
On Tue, Jul 16, 2013 at 2:18 PM, <dalecurtis@chromium.org> wrote:
> wsola.{cc,h} definitely. No to wsola_util.{cc,h} since it's doubtful
> anyone
> else would be using those functions. How about wsola_internals.{cc,h} and
> put
> them in the media::internal namespace. A similar pattern is used
> elsewhere in
> chrome.
>
>
https://codereview.chromium.**org/19111004/<https://codereview.chromium.org/1...
>
Hi Dale, I started to write Wsola class (there was a nasty bug, which kept me from working on time-scaling) but I noticed that in this case AudioRendererAlgorithm will basically be only a wrapper on Wsola. All calls to public APIs of AudioRendererAlgorithm will be just passed down to Wsola. I was thinking how much that make sense? What are the other modules which might call Wsola. And why cannot they do that through audio-renderer. Put it in another word. Wsola will have the same functionality/APIs as AudioRenderrerAlgorithm. Mainly Audio is pushed in and played out, perhaps with a different playback rate. What do you think? Thanks,... Turaj On Tue, Jul 16, 2013 at 2:24 PM, Turaj Zakizadeh Shabestary < turaj@webrtc.org> wrote: > Agreed that they are specific to WSOLA, wsola_internals.{cc, h} is good > suggestion. Will do as advised. > > > On Tue, Jul 16, 2013 at 2:18 PM, <dalecurtis@chromium.org> wrote: > >> wsola.{cc,h} definitely. No to wsola_util.{cc,h} since it's doubtful >> anyone >> else would be using those functions. How about wsola_internals.{cc,h} and >> put >> them in the media::internal namespace. A similar pattern is used >> elsewhere in >> chrome. >> >> https://codereview.chromium.**org/19111004/<https://codereview.chromium.org/1... >> > >
If it's entirely overlap, just having ARA is better. After review (for ease of
review) we can consider renaming ARA to wsola.{cc,h}. You should still rename
the _util class to _internals as discussed.
Aside from the internals change, is everything else ready for review here? Have
you found someone from the signals processing team who can review this for
correctness?
On 2013/07/22 18:12:29, DaleCurtis wrote:
> If it's entirely overlap, just having ARA is better. After review (for ease
of
> review) we can consider renaming ARA to wsola.{cc,h}. You should still rename
> the _util class to _internals as discussed.
>
> Aside from the internals change, is everything else ready for review here?
Have
> you found someone from the signals processing team who can review this for
> correctness?
comment from peanut gallery ... +1 to naming things after what they do (e.g.,
audio_stretcher) as opposed to the algorithm used as it's more of an
implementation detail and I doubt most people will understand that "wsola" is
(apparently it's also the name of a polish village)
if we had multiple algorithms we could split things out, but it doesn't seem
like that's going to be the case
+ajm@ who will review Signal Processing aspect of the code. I have already renamed, though not uploaded, _util to wsola_internal. Andrew (ajm@), could you please have a look on this CL. I still need to add few unit-tests for some private methods, and one for entire method. I'll upload a patch with Dale's comments addressed today, but I might need one or two days to finish unit-tests. Would be great if you can have your first round of comments before I finish unit-tests. Dale, it is up to you if you want to see Andrew's comments addressed before you start your review. Thanks,... Turaj On Mon, Jul 22, 2013 at 11:12 AM, <dalecurtis@chromium.org> wrote: > If it's entirely overlap, just having ARA is better. After review (for > ease of > review) we can consider renaming ARA to wsola.{cc,h}. You should still > rename > the _util class to _internals as discussed. > > Aside from the internals change, is everything else ready for review here? > Have > you found someone from the signals processing team who can review this for > correctness? > > https://codereview.chromium.**org/19111004/<https://codereview.chromium.org/1... >
This was longer than I suspected when Turaj asked me to review :) I'm pretty busy at the moment, so Turaj is going to ask Marco to review instead. I'll post the comments I had thus far though. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... File media/filters/audio_renderer_algorithm.cc (right): https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.cc:40: static const int kStartingBufferSizeInFrames = 16 * 512; I assume this is the "frames as in samples" usage. We don't want to mix. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.cc:69: (kWsolaSearchIntervalMs * samples_per_second_) / 1000 + 1; What's the +1 for? https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.cc:73: floor(kOlaWindowSizeMs * samples_per_second_ / 1000 / 2)) * 2; These are all integers, right? Shouldn't need the floor, or the cast. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.cc:139: playback_rate_ = floor(playback_rate_ * 100.f + 0.5f) / 100; Just use new_rate here directly. Why do you have to truncate this? https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.cc:187: rendered_frames += ReadWsolaOutput(requested_frames - rendered_frames, Do you need to break these functions up? https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.cc:195: scoped_ptr<AudioBus> optimal_frame = AudioBus::Create( I'm not sure how AudioBus works, but do you want to be creating one with every iteration? https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.cc:317: PeekAudioWithZerroAppend(optimal_index, optimal_block); Zerro -> Zero https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.cc:338: bool AudioRendererAlgorithm::PeekAudioWithZerroAppend( You don't check the return value of this anywhere. Better to make it void and DCHECK? https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... File media/filters/audio_renderer_algorithm.h (right): https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:99: // Fill |dest| with frames from |audio_buffer_| starting form frame form -> from https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:100: // |read_offset_frames|. |dest| is expected to have same number of channels the same https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:145: // WSOLA variables. Do you mention what this stands for anywhere? https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:156: // |target| target-frame, we search the input to find a frame that is most In Chrome, "frame" is used to mean a single-channel sample. We might want to use a different name such as block. Dale might have a suggestion. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:175: // 4) Search interval of input is then centered at |t_out| * |alpha| with ts_out https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:176: // the width of 2 * |tau|, i.e. |ts_out|*|alpha| + [-|tau|, |tau|]. Space around * to be consistent. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:180: // Let the optimal frame be |opt| and its center be |t_in_opt|. ts_in_opt? Or just change all ts -> t https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:192: // should be appropriately update when out samples are generated, regardless updated https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:195: // Furthermore, if samples form input |audio_buffer_| are evicted then this from https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:204: int num_candid_frames_; Is this short for candidate? Don't shorten it. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:209: // Overlap-and-add window size in frames. Perhaps refer to your description above (|L|). https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:212: // The hop size of overlap-and-add in frames, this implementation assumes frames (|L/2|). This ... https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:225: // Overlap-and-add window. Explain more, or remove the comment. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:228: // Transition window. Explain more. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... File media/filters/audio_renderer_algorithm_unittest.cc (right): https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm_unittest.cc:31: static void FillWithSquarePuleTrain( Pule -> Pulse https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... File media/filters/audio_renderer_algorithm_util.cc (right): https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm_util.cc:46: for (int k = 0; k < a->channels(); ++k) { I suppose using i here and j in the inner loop is more natural. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm_util.cc:50: dot_product[k] += *ch_a++ * *ch_b++; Any reason to prefer *ch_a++ over ch_a[n]? https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm_util.cc:61: for (int k = 0; k < input->channels(); ++k) { Again, I think i, j, k ... is more natural. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm_util.cc:66: for(int m = 0; m < frames_per_window; ++m) for ( https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm_util.cc:71: for (int n = 1; n < num_blocks; ++n, ++slide_in, ++slide_out) { This looks fine, but some comments explaining it could be instructive. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... File media/filters/audio_renderer_algorithm_util.h (right): https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm_util.h:29: void MultiChannelMovingWindowEnergies(const AudioBus* input, More comments here explaining how it works and the parameters (e.g. energy must be input->frames() - (frames_per_window + 1) in length) https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm_util.h:41: void CubicInterpol(const float* y_values, CubicInterpolation? https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm_util.h:77: void HannSym(int window_length, float* window); GetHannWindow?
+Marco to will substitute ajm@ Andrew, fill free to see if you comments are correctly addressed, or we can ask Marco to follow from here. If you don't want to be further notified regarding this CL I can remover you from the reviewer's list. Marco, I originally asked Andrew to review Signal Processing aspects of this CL, however, due to his parental leave and high work load, I would appreciate if you can help us in reviewing this CL. Andrew has already some comments and I have addressed them. With the current unit tests, we cover all "internal" functions and two unit tests I recently added test end-to-end WSOLA algorithm. I those tests, the input is a periodic signal (pulse train), hence, the output has to be the same signal but of different length. This verifies that WSOLA is working correctly. There are a few methods of AudioRendererAlgorithm which are not directly tested. I would like you to start your review, and I will add some more unit-tests when I get the first set of your comments. Thanks,... Turaj https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... File media/filters/audio_renderer_algorithm.cc (right): https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.cc:40: static const int kStartingBufferSizeInFrames = 16 * 512; The notion of frame, where frame _N_ is the set of sample _N_ of all channels, existed as you see. I tried to comply to that notion. On 2013/07/23 18:03:28, ajm wrote: > I assume this is the "frames as in samples" usage. We don't want to mix. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.cc:69: (kWsolaSearchIntervalMs * samples_per_second_) / 1000 + 1; To make the search region symmetric around |output_index| * |playback_rate|. There is no algorithmic significance regarding +1. It made my first implementation more straight forward. I might remove it in this implementation. On 2013/07/23 18:03:28, ajm wrote: > What's the +1 for? https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.cc:73: floor(kOlaWindowSizeMs * samples_per_second_ / 1000 / 2)) * 2; Right. On 2013/07/23 18:03:28, ajm wrote: > These are all integers, right? Shouldn't need the floor, or the cast. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.cc:139: playback_rate_ = floor(playback_rate_ * 100.f + 0.5f) / 100; Truncation is needed when it comes to removing frames (samples) from input. If |P| samples are removed from input then |P| / |playback_rate| samples should be removed form output (reduce |output_index| by that amount). |P| and |P| / |playback_rate| should both be integer for time scaling be accurate. Otherwise, input and output slowly drift. I haven't investigated what the result might be, but I guess we might lose lip-sync. So if we can simply avoid it, why not. By confining |playback_rate| to two decimals, which is sufficient for any application, |P| can be 100 * |playback_rate|. Otherwise we have to search for an integer that satisfies the above, and I didn't want to go along that path. Another solution would be to specify |playback_rate| by the ratio of two integer |p| and |q|. But I didn't want to touch clients of AudioRendererAlgorithm. On 2013/07/23 18:03:28, ajm wrote: > Just use new_rate here directly. Why do you have to truncate this? https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.cc:187: rendered_frames += ReadWsolaOutput(requested_frames - rendered_frames, I can define "int Wsola(requested_frames, int output_offset, AudioBus* dest)" and call ReadWsolaOutput() at the end of Wsola(). On 2013/07/23 18:03:28, ajm wrote: > Do you need to break these functions up? https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.cc:195: scoped_ptr<AudioBus> optimal_frame = AudioBus::Create( It is not very expensive basically one malloc with some calculation to get the correct size to have correct alignment. I can define it member to avoid dynamic memory allocation, Dale what do you think? On 2013/07/23 18:03:28, ajm wrote: > I'm not sure how AudioBus works, but do you want to be creating one with every > iteration? https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.cc:317: PeekAudioWithZerroAppend(optimal_index, optimal_block); On 2013/07/23 18:03:28, ajm wrote: > Zerro -> Zero Done. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.cc:338: bool AudioRendererAlgorithm::PeekAudioWithZerroAppend( I decided to check it and propagate result. On 2013/07/23 18:03:28, ajm wrote: > You don't check the return value of this anywhere. Better to make it void and > DCHECK? https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... File media/filters/audio_renderer_algorithm.h (right): https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:99: // Fill |dest| with frames from |audio_buffer_| starting form frame On 2013/07/23 18:03:28, ajm wrote: > form -> from Done. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:100: // |read_offset_frames|. |dest| is expected to have same number of channels On 2013/07/23 18:03:28, ajm wrote: > the same Done. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:145: // WSOLA variables. On 2013/07/23 18:03:28, ajm wrote: > Do you mention what this stands for anywhere? Done. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:156: // |target| target-frame, we search the input to find a frame that is most The description was written before I realize the usage of frame in this file and Chrome. I'll re-phrase the description. On 2013/07/23 18:03:28, ajm wrote: > In Chrome, "frame" is used to mean a single-channel sample. We might want to use > a different name such as block. Dale might have a suggestion. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:175: // 4) Search interval of input is then centered at |t_out| * |alpha| with On 2013/07/23 18:03:28, ajm wrote: > ts_out Done. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:176: // the width of 2 * |tau|, i.e. |ts_out|*|alpha| + [-|tau|, |tau|]. On 2013/07/23 18:03:28, ajm wrote: > Space around * to be consistent. Done. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:180: // Let the optimal frame be |opt| and its center be |t_in_opt|. On 2013/07/23 18:03:28, ajm wrote: > ts_in_opt? Or just change all ts -> t Done. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:192: // should be appropriately update when out samples are generated, regardless On 2013/07/23 18:03:28, ajm wrote: > updated Done. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:195: // Furthermore, if samples form input |audio_buffer_| are evicted then this On 2013/07/23 18:03:28, ajm wrote: > from Done. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:204: int num_candid_frames_; On 2013/07/23 18:03:28, ajm wrote: > Is this short for candidate? Don't shorten it. Done. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:209: // Overlap-and-add window size in frames. On 2013/07/23 18:03:28, ajm wrote: > Perhaps refer to your description above (|L|). Done. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:212: // The hop size of overlap-and-add in frames, this implementation assumes On 2013/07/23 18:03:28, ajm wrote: > frames (|L/2|). This ... Done. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:225: // Overlap-and-add window. On 2013/07/23 18:03:28, ajm wrote: > Explain more, or remove the comment. Done. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.h:228: // Transition window. On 2013/07/23 18:03:28, ajm wrote: > Explain more. Done. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... File media/filters/audio_renderer_algorithm_unittest.cc (right): https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm_unittest.cc:31: static void FillWithSquarePuleTrain( On 2013/07/23 18:03:28, ajm wrote: > Pule -> Pulse Done. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... File media/filters/audio_renderer_algorithm_util.cc (right): https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm_util.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/07/16 00:18:40, DaleCurtis wrote: > 2013 and no (c). Here and other files. Done. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm_util.cc:17: bool InInterval(int n, interval q) { On 2013/07/16 00:18:40, DaleCurtis wrote: > Types should have the first letter capitalized; so Interval instead of interval. Done. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm_util.cc:27: similarity_measure += dot_prod_a_b[n] / sqrt(energy_a[n] * energy_b[n] + It is to prevent dividing by zero. I change it to const. And thank for pointing to vector_math.h, but so far there are only two functions one implementing "a = a + b * scale" and "a = b * scale." I'll use them if I need such functions. On 2013/07/16 00:18:40, DaleCurtis wrote: > What is 1e-12 for? > > Additionally vector_math.h might be a good home for some of the vector math > primitives used by these functions. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm_util.cc:39: DCHECK(a->channels() == b->channels()); On 2013/07/16 00:18:40, DaleCurtis wrote: > use DCHECK_EQ, DCHECK_GE, DCHECK_LE etc, instead. Done. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... File media/filters/audio_renderer_algorithm_util.h (right): https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm_util.h:29: void MultiChannelMovingWindowEnergies(const AudioBus* input, On 2013/07/23 18:03:28, ajm wrote: > More comments here explaining how it works and the parameters (e.g. energy must > be input->frames() - (frames_per_window + 1) in length) Done. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm_util.h:41: void CubicInterpol(const float* y_values, On 2013/07/23 18:03:28, ajm wrote: > CubicInterpolation? Done. https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm_util.h:77: void HannSym(int window_length, float* window); I would keep "Symmetric" as this window is known as symmetric Hanning in MATLAB vs "periodic" Hanning. A "periodic" Hanning is not perfect reconstruction. On 2013/07/23 18:03:28, ajm wrote: > GetHannWindow?
https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... File media/filters/audio_renderer_algorithm.cc (right): https://codereview.chromium.org/19111004/diff/1/media/filters/audio_renderer_... media/filters/audio_renderer_algorithm.cc:195: scoped_ptr<AudioBus> optimal_frame = AudioBus::Create( On 2013/07/29 22:09:57, turaj wrote: > It is not very expensive basically one malloc with some calculation to get the > correct size to have correct alignment. I can define it member to avoid dynamic > memory allocation, Dale what do you think? > > > On 2013/07/23 18:03:28, ajm wrote: > > I'm not sure how AudioBus works, but do you want to be creating one with every > > iteration? > malloc is very expensive relative to the rest of the costs here; please use a member variable.
Dale's comment is addressed.
Looks good Turaj! Some comments below. https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm.cc (right): https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:77: ola_window_size_ / 2 - 1); The second term is because of offset to first frame? https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:300: const int search_region_index = GetSearchRegionIndex(); You use "search_block_index" for this above, better to use same name. https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:320: // Make a transition from target window to the optimal window if different. you mean...from target block to the optimal block..? https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:321: // Target window has the best continuation to the current current output. Target block instead of "window" https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:321: // Target window has the best continuation to the current current output. Remove one of the "current" https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:329: ch_opt[n] = ch_opt[n] * transition_window_[n] + ch_target[n] * May want to comment about transition_window. Is it of size 2L to get varying weight such that weighting term favors target near n=0 and optimal near L? https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:330: transition_window_[ola_window_size_ + n]; No change needed for this comment. Just wondering if some measure can be used in OptimalIndex() selection that incorporates both similarity and discontinuity/smoothing constraint, so to avoid this extra smoothing step (with preset weighting term) after OptimalIndex(). https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm.h (right): https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:92: // target window, and write it in |optimal_block_|. Returns false it there is target "block"? https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:126: bool TargetIsWithinSearchRegion() const; Must be fully in search region, right, to avoid search? https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:217: // Index of the beginning of the target window, counted in frames. target "block"? https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:227: int num_complete_frames_; Add description here, for num_complete_frames https://codereview.chromium.org/19111004/diff/21001/media/filters/wsola_inter... File media/filters/wsola_internals.cc (right): https://codereview.chromium.org/19111004/diff/21001/media/filters/wsola_inter... media/filters/wsola_internals.cc:58: void MultiChannelMovingWindowEnergies(const AudioBus* input, The usage of terms: frames, blocks, and windows, makes this little hard to follow. Window here refers to a block? https://codereview.chromium.org/19111004/diff/21001/media/filters/wsola_inter... media/filters/wsola_internals.cc:61: int num_blocks = input->frames() - (frames_per_window - 1); Is it more consistent to use "num_frames" here instead of "num_blocks" https://codereview.chromium.org/19111004/diff/21001/media/filters/wsola_inter... media/filters/wsola_internals.cc:71: energy[k] += input_channel[m] * input_channel[m]; Easier to follow if you put { } around this one-line loop statement. https://codereview.chromium.org/19111004/diff/21001/media/filters/wsola_inter... media/filters/wsola_internals.cc:93: *extremum = -b / (2.f * a); "a" will never be zero here because this is only called under condition of local max, i.e., when y[1] > y[2] and y[1] > y[0], right? https://codereview.chromium.org/19111004/diff/21001/media/filters/wsola_inter... media/filters/wsola_internals.cc:127: n += decimation; What if n>=num_candid_frames before entering loop, should we check if similarity[1] > similarity[0] and take 1 as optimal instead of 0. https://codereview.chromium.org/19111004/diff/21001/media/filters/wsola_inter... media/filters/wsola_internals.cc:205: int num_candid_frames = search_segment->frames() - (block_size - 1); Why the subtraction of the second term?, is that an offset in search_segment->frames(). https://codereview.chromium.org/19111004/diff/21001/media/filters/wsola_inter... File media/filters/wsola_internals.h (right): https://codereview.chromium.org/19111004/diff/21001/media/filters/wsola_inter... media/filters/wsola_internals.h:62: // most similar to |target_interval|. |energy_target_block| is the energy of the Most similar to |target_block|? https://codereview.chromium.org/19111004/diff/21001/media/filters/wsola_inter... media/filters/wsola_internals.h:65: int PartialSearch(int lim_low, Maybe rename to RefinedSearch, but PartialSearch is fine too. (Refined in the sense that it follows the coarse/decimated search)
Thanks for the review Marco, comments are addressed. https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm.cc (right): https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:77: ola_window_size_ / 2 - 1); If we have L+1 candidates, it means we have to check L/2 candidate blocks to the left of the search-block-center. The first sample of the left most candidate block is L/2 + (|ola_window_size|/2 - 1) to the left of the search-block-center. In another word, we are computing cross correlation of target-block (of size |ola_window_size|) with search-block for lags -L/2 to L/2 around the search-block-center. Therefore the left most sample needed for this computation is -(L/2 + (|ola_window_size|/2 - 1)). On 2013/08/02 17:56:54, marpan wrote: > The second term is because of offset to first frame? https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:300: const int search_region_index = GetSearchRegionIndex(); Absolutely. On 2013/08/02 17:56:54, marpan wrote: > You use "search_block_index" for this above, better to use same name. https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:320: // Make a transition from target window to the optimal window if different. You right, my mistake. On 2013/08/02 17:56:54, marpan wrote: > you mean...from target block to the optimal block..? https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:321: // Target window has the best continuation to the current current output. On 2013/08/02 17:56:54, marpan wrote: > Target block instead of "window" Done. https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:321: // Target window has the best continuation to the current current output. On 2013/08/02 17:56:54, marpan wrote: > Remove one of the "current" Done. https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:329: ch_opt[n] = ch_opt[n] * transition_window_[n] + ch_target[n] * On 2013/08/02 17:56:54, marpan wrote: > May want to comment about transition_window. Is it of size 2L to get varying > weight such that weighting term favors target near n=0 and optimal near L? > Done. https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:330: transition_window_[ola_window_size_ + n]; I guess one can do something along the lines you suggest. It you accept my notation the optimal_block at the end of this function is optimal_block = RampOutWindow * target_block + RampInWindow * most_similar; So this suggest that instead of maximizing DotProduct(target_block, candidate_block) to find most_similar, one might maximize DotProduct(target_block, RampInWindow * candidate_block). To save complexity one can evaluate DotProduct(target_block * candidate_block, RampInWindow). I can test this, if you think it makes sense. On 2013/08/02 17:56:54, marpan wrote: > No change needed for this comment. Just wondering if some measure can be used in > OptimalIndex() selection that incorporates both similarity and > discontinuity/smoothing constraint, so to avoid this extra smoothing step (with > preset weighting term) after OptimalIndex(). https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm.h (right): https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:92: // target window, and write it in |optimal_block_|. Returns false it there is On 2013/08/02 17:56:54, marpan wrote: > target "block"? Done. https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:126: bool TargetIsWithinSearchRegion() const; Absolutely. On 2013/08/02 17:56:54, marpan wrote: > Must be fully in search region, right, to avoid search? https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:217: // Index of the beginning of the target window, counted in frames. On 2013/08/02 17:56:54, marpan wrote: > target "block"? Done. https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:227: int num_complete_frames_; On 2013/08/02 17:56:54, marpan wrote: > Add description here, for num_complete_frames Done. https://codereview.chromium.org/19111004/diff/21001/media/filters/wsola_inter... File media/filters/wsola_internals.cc (right): https://codereview.chromium.org/19111004/diff/21001/media/filters/wsola_inter... media/filters/wsola_internals.cc:58: void MultiChannelMovingWindowEnergies(const AudioBus* input, I tired to be more consistent. On 2013/08/02 17:56:54, marpan wrote: > The usage of terms: frames, blocks, and windows, makes this little hard to > follow. Window here refers to a block? https://codereview.chromium.org/19111004/diff/21001/media/filters/wsola_inter... media/filters/wsola_internals.cc:61: int num_blocks = input->frames() - (frames_per_window - 1); But this is really the number of blocks. We have these many blocks (slided by one frame) to compute their energy. On 2013/08/02 17:56:54, marpan wrote: > Is it more consistent to use "num_frames" here instead of "num_blocks" https://codereview.chromium.org/19111004/diff/21001/media/filters/wsola_inter... media/filters/wsola_internals.cc:71: energy[k] += input_channel[m] * input_channel[m]; On 2013/08/02 17:56:54, marpan wrote: > Easier to follow if you put { } around this one-line loop statement. Done. https://codereview.chromium.org/19111004/diff/21001/media/filters/wsola_inter... media/filters/wsola_internals.cc:93: *extremum = -b / (2.f * a); Right, but maybe I should consider two other cases when this function has to be called, i) y[1] > y[2] and y[1] == y[0] ii) y[1] == y[2] and y[1] > y[0] Although the equality is rare computing in float, and add an assert. On 2013/08/02 17:56:54, marpan wrote: > "a" will never be zero here because this is only called under condition of local > max, i.e., when y[1] > y[2] and y[1] > y[0], right? https://codereview.chromium.org/19111004/diff/21001/media/filters/wsola_inter... media/filters/wsola_internals.cc:127: n += decimation; Although with the setting in AudioRendererAlgorithm this won't happen, but you are totally right that the function should operate correctly. On 2013/08/02 17:56:54, marpan wrote: > What if n>=num_candid_frames before entering loop, should we check if > similarity[1] > similarity[0] and take 1 as optimal instead of 0. https://codereview.chromium.org/19111004/diff/21001/media/filters/wsola_inter... media/filters/wsola_internals.cc:205: int num_candid_frames = search_segment->frames() - (block_size - 1); The total frames (samples) in search-block is all samples needed to perform cross correlation to find the most similar block. So it is larger than the number of candid frames. Let say we have to find the cross correlation of A & B for L lags where length(A) < length(B) then we need L+(length(A)-1) samples of B to cross correlate with A. Those L+(length(A)-1) samples constitute search-block in our terminology. On 2013/08/02 17:56:54, marpan wrote: > Why the subtraction of the second term?, is that an offset in > search_segment->frames(). https://codereview.chromium.org/19111004/diff/21001/media/filters/wsola_inter... File media/filters/wsola_internals.h (right): https://codereview.chromium.org/19111004/diff/21001/media/filters/wsola_inter... media/filters/wsola_internals.h:62: // most similar to |target_interval|. |energy_target_block| is the energy of the On 2013/08/02 17:56:54, marpan wrote: > Most similar to |target_block|? Done. https://codereview.chromium.org/19111004/diff/21001/media/filters/wsola_inter... media/filters/wsola_internals.h:65: int PartialSearch(int lim_low, how about "FullSearch". On 2013/08/02 17:56:54, marpan wrote: > Maybe rename to RefinedSearch, but PartialSearch is fine too. > (Refined in the sense that it follows the coarse/decimated search)
LGTM, few nits below. https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm.cc (right): https://codereview.chromium.org/19111004/diff/21001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:330: transition_window_[ola_window_size_ + n]; On 2013/08/02 23:45:59, turaj wrote: > I guess one can do something along the lines you suggest. It you accept my > notation the optimal_block at the end of this function is > > optimal_block = RampOutWindow * target_block + RampInWindow * most_similar; > > So this suggest that instead of maximizing DotProduct(target_block, > candidate_block) to find most_similar, one might maximize > DotProduct(target_block, RampInWindow * candidate_block). To save complexity one > can evaluate DotProduct(target_block * candidate_block, RampInWindow). > > I can test this, if you think it makes sense. > > > > > On 2013/08/02 17:56:54, marpan wrote: > > No change needed for this comment. Just wondering if some measure can be used > in > > OptimalIndex() selection that incorporates both similarity and > > discontinuity/smoothing constraint, so to avoid this extra smoothing step > (with > > preset weighting term) after OptimalIndex(). > No need to make any change for this comment. https://codereview.chromium.org/19111004/diff/21001/media/filters/wsola_inter... File media/filters/wsola_internals.cc (right): https://codereview.chromium.org/19111004/diff/21001/media/filters/wsola_inter... media/filters/wsola_internals.cc:61: int num_blocks = input->frames() - (frames_per_window - 1); On 2013/08/02 23:45:59, turaj wrote: > But this is really the number of blocks. We have these many blocks (slided by > one frame) to compute their energy. > > On 2013/08/02 17:56:54, marpan wrote: > > Is it more consistent to use "num_frames" here instead of "num_blocks" > Ok. Makes sense. You may want to add your comment above to the description: "We have these many blocks (slided by one frame) to compute their energy" https://codereview.chromium.org/19111004/diff/21001/media/filters/wsola_inter... File media/filters/wsola_internals.h (right): https://codereview.chromium.org/19111004/diff/21001/media/filters/wsola_inter... media/filters/wsola_internals.h:65: int PartialSearch(int lim_low, On 2013/08/02 23:45:59, turaj wrote: > how about "FullSearch". > > On 2013/08/02 17:56:54, marpan wrote: > > Maybe rename to RefinedSearch, but PartialSearch is fine too. > > (Refined in the sense that it follows the coarse/decimated search) > FullSearch is fine. https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm.cc (right): https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:325: // as the optimal-block which makes it like a weighting function where "that of" the optimal-block https://codereview.chromium.org/19111004/diff/43001/media/filters/wsola_inter... File media/filters/wsola_internals.cc (right): https://codereview.chromium.org/19111004/diff/43001/media/filters/wsola_inter... media/filters/wsola_internals.cc:151: // estimate of candid maximum. estimate of the "candidate" maximum.
Still reviewing, but here is my initial set of comments. https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm.cc (right): https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:69: (kWsolaSearchIntervalMs * samples_per_second_) / 1000 + 1; Please include your comment about symmetry for the + 1. Though I'm still not sure I follow... Are you trying to ensure an even number here? https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:72: ola_window_size_ = (kOlaWindowSizeMs * samples_per_second_ / 1000 / 2) * 2; If you need both this and num_candidate_frames_ to be even, add a small helper function inline int RoundUpToEven(int n) { return n + (n & 1); } ola_window_size_ = RoundUpToEven(kOlaWindowSizeMs * samples_per_second_ / 1000); https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:76: search_block_center_offset_ = (num_candidate_frames_ - 1) / 2 + ( Again, comments for why you have the -1s. https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:79: ola_window_.reset(new float[ola_window_size_]); Do any of these structures need to be zero initialized? https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:137: int total_frames_rendered = WsolaOutput(requested_frames, dest); I commented in the header about function naming, but WsolaOutput() is actually small enough you can just inline it here. https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm.h (right): https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:89: int WsolaOutput(int requested_frames, AudioBus* dest); It's confusing to have Wsola() and WsolaOutput() can you come up with a more descriptive name for at least one of them? https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:94: // |target_block_| or |search_block_| extend to future (need future samples s/to/into the/ s/(need future samples to be completed)/; I.e., more input is required./ https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:104: // channels as |audio_buffer_|. Negative offsets, i.e. In what cases will a negative offset be specified? https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:111: // extend the output by |ola_hop_size|, written to |wsola_output_|. Then, ola_hop_size_ https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:117: // Seek |audio_buffer_| forward to remove frames from input that is not used s/is not/are not/ https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:118: // any more. State of the WSOLA should be updated accordingly. s/should/will be/ ?
Hi, I addressed your comments. Thanks,... Turaj https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm.cc (right): https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:69: (kWsolaSearchIntervalMs * samples_per_second_) / 1000 + 1; I gave up "+ 1" it is not at all necessary in this design, in my first approach I need the symmetry. And it does not need to be necessarily even. On 2013/08/06 18:04:55, DaleCurtis wrote: > Please include your comment about symmetry for the + 1. Though I'm still not > sure I follow... Are you trying to ensure an even number here? https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:72: ola_window_size_ = (kOlaWindowSizeMs * samples_per_second_ / 1000 / 2) * 2; We only have one member which has to be even, but I like your function. On 2013/08/06 18:04:55, DaleCurtis wrote: > If you need both this and num_candidate_frames_ to be even, add a small helper > function > > inline int RoundUpToEven(int n) { return n + (n & 1); } > > ola_window_size_ = RoundUpToEven(kOlaWindowSizeMs * samples_per_second_ / 1000); https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:76: search_block_center_offset_ = (num_candidate_frames_ - 1) / 2 + ( On 2013/08/06 18:04:55, DaleCurtis wrote: > Again, comments for why you have the -1s. Done. https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:79: ola_window_.reset(new float[ola_window_size_]); I didn't comprehend what you mean by "zero initialized," If you mean at the constructor, then we don't know their size unless we have a default sampling rate. And here GetSymmetricHanningWindow() does not assume zero-initialized input. On 2013/08/06 18:04:55, DaleCurtis wrote: > Do any of these structures need to be zero initialized? https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:137: int total_frames_rendered = WsolaOutput(requested_frames, dest); True. I thought that I keep a single method as API to WSOLA. But perhaps it doesn't matter. On 2013/08/06 18:04:55, DaleCurtis wrote: > I commented in the header about function naming, but WsolaOutput() is actually > small enough you can just inline it here. https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:325: // as the optimal-block which makes it like a weighting function where On 2013/08/06 17:14:10, marpan wrote: > "that of" the optimal-block Done. https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm.h (right): https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:89: int WsolaOutput(int requested_frames, AudioBus* dest); I guess it is better with new names. On 2013/08/06 18:04:55, DaleCurtis wrote: > It's confusing to have Wsola() and WsolaOutput() can you come up with a more > descriptive name for at least one of them? https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:94: // |target_block_| or |search_block_| extend to future (need future samples On 2013/08/06 18:04:55, DaleCurtis wrote: > s/to/into the/ > s/(need future samples to be completed)/; I.e., more input is required./ Done. https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:104: // channels as |audio_buffer_|. Negative offsets, i.e. I added some comment. On 2013/08/06 18:04:55, DaleCurtis wrote: > In what cases will a negative offset be specified? https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:111: // extend the output by |ola_hop_size|, written to |wsola_output_|. Then, On 2013/08/06 18:04:55, DaleCurtis wrote: > ola_hop_size_ Done. https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:117: // Seek |audio_buffer_| forward to remove frames from input that is not used On 2013/08/06 18:04:55, DaleCurtis wrote: > s/is not/are not/ Done. https://codereview.chromium.org/19111004/diff/43001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:118: // any more. State of the WSOLA should be updated accordingly. On 2013/08/06 18:04:55, DaleCurtis wrote: > s/should/will be/ ? Done. https://codereview.chromium.org/19111004/diff/43001/media/filters/wsola_inter... File media/filters/wsola_internals.cc (right): https://codereview.chromium.org/19111004/diff/43001/media/filters/wsola_inter... media/filters/wsola_internals.cc:151: // estimate of candid maximum. On 2013/08/06 17:14:10, marpan wrote: > estimate of the "candidate" maximum. Done.
It'd be nice to see some benchmarks relative to the old code for this new version. Can you time the various FillBuffer unittests w/ your new code vs the old? I'm also curious if decimated search is actually worth the overhead for the buffer sizes we generally use (128..2048) https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm.cc (right): https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:167: // Round it to two decimal digits. Is this really necessary? I don't really follow why from the comment below, even if you don't truncate this here: int(|playback_rate_| * 100) == floor(|playback_rate_| * 100) will always be true. What am I missing? And can you document it here... :) https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:203: if (target_block_index_ + ola_window_size_ <= frames && Just return instead of if? https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:220: float* ch_opt_frame = optimal_block_->channel(k); const float* const ? https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:239: int AudioRendererAlgorithm::GetSearchRegionIndex() const { You call this pretty frequently, it might be worth computing along side output_index_ https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:241: const int search_block_center_index = static_cast<int>(floor( no need for floor + static_cast<int>, the cast is sufficient so long as the value is always positive. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:248: void AudioRendererAlgorithm::RemoveOldInputFrames() { Instead of trying to calculate how many input frames to remove, would it be better for WsolaIteration() to set some value indicating the number of frames used? https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:260: static_cast<int>(floor(playback_rate_ * kOutputFramesPerBlock + 0.5f)); Again, floor + cast is unnecessary. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:261: const int blocks_to_remove = earliest_used_index / input_frames_per_block; Is num >> den, such that integer division isn't losing precision here? https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:283: int frames_to_move = wsola_output_->frames() - rendered_frames; You should be able to use the Copy helpers in AudioBus no? https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:286: memmove(ch, &ch[rendered_frames], sizeof(*ch) * frames_to_move); Necessary vs memcpy? They don't seem to overlap? Are you expecting wsola_output_ to be zeroed after this operation (it won't be)? https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:296: if (target_block_index_ >= search_block_index && Again, just return the value directly. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:321: internal::Interval exclude_iterval = std::make_pair(last_optimal - 80, 80? Extract to constant w/ documentation. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:344: float* ch_target = target_block_->channel(k); const float* const ? https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:357: bool AudioRendererAlgorithm::PeekAudioWithZeroAppend( Technically this is prepending data, not appending. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:360: if (read_offset_frames + num_frames > audio_buffer_.frames()) Should this be a CHECK() instead? Then this function can just return void. You'd be able to remove the if (...) return false in GetOptimalBlock() then too. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:364: int num_frames_to_read = dest->frames(); You have two num_frames variables which do the same thing. Remove this one? https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:373: audio_buffer_.PeekFrames(num_frames_to_read, read_offset_frames, Should this return true if zero frames are peeked? I.e. read_offset_frames = -num_frames? https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm.h (right): https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:88: // target block, and write it in |optimal_block_|. Returns false it there is s/target block/|target_block_|/ https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:101: // channels as |audio_buffer_|. A Negative offset, i.e. s/Negative/negative/ https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:113: bool WsolaIteration(); RunOneWsolaIteration() ? https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:122: // Is the target block fully within search region? If so, we don't need to Use explicit |target_block_| and |search_block_| references in method descriptions (here and elsewhere) -- the algorithm is already confusing :) https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:150: // Waveform Similarity Overlap-and-add (WSOLA) variables. This is more of an algorithm description and should be in the .cc file not the .h. Also the listed variables don't match up to the ones actually in the class. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm_unittest.cc (right): https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:30: static const int kOutputDurationInSec = 10; As mentioned below, these tests are traditionally very slow w/o good reason. You should run this under valgrind and see how long it takes to run. If it's more than 10 or 15 seconds for all the tests, it's too long. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:242: ref->ToInterleaved(kPulseWidthSamples, sizeof(*(ref_memory_buffer.get())), Is it necessary to convert to int16? The AudioBuffer can take float samples. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:245: base::TimeDelta timestamp = base::TimeDelta::FromInternalValue(0); const these two. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:250: scoped_refptr<AudioBuffer> input = AudioBuffer::CopyFrom( Technically you should be able to create an AudioBuffer w/o copying and then a wrapped AudioBus() using AudioBuffer::writable_data(). These unittests are usually very slow in valgrind, so avoiding any copies here can be helpful. I'm investigating whether this works right now though, so don't take my advice yet :) https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:259: scoped_ptr<AudioBus> out_buffer = Don't recreate inside loop. Also what's the difference between out_buffer and output? Some better names would be nice :) https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:275: // Pulses in the first half of WSOLA AOL frame are not constructed Is this a bug? Will glitches be heard? https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:278: scoped_ptr<int16_t[]> test_memory_buffer( Again, don't allocate inside of the loops. Allocation is extremely slow relative to the rest of the operations here. Also if you can avoid interleaving for comparison that'd be helpful. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:470: for (int n = 0; n < kNumBlocks; ++n) { Comment on what you're testing here. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:526: ASSERT_FLOAT_EQ(0, energy_candid_blocks[0]); Comments on what's being tested. https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... File media/filters/wsola_internals.cc (right): https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.cc:75: const float* slide_in = &input_channel[frames_per_block]; just input_channel + frames_per_block ? https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.cc:157: int candidate_index = n - decimation + static_cast<int>( no need for cast+floor https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.cc:165: similarity[2] > best_similarity && !InInterval(n, exclude_interval)) { should align with ( https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.cc:171: memmove(similarity, &similarity[1], 2 * sizeof(*similarity)); memcpy? https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.cc:214: DCHECK(channels == target_block->channels()); DCHECK_EQ https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.cc:217: const int kSearchDecimation = 5; Comment. https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.cc:219: scoped_ptr<float[]> energy_target_block(new float[channels]); Bummer about these allocations, but don't worry about them until we have some performance numbers on this feature. https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.cc:231: int optimal_index = DecimatedSearch(kSearchDecimation, Have you run the performance numbers w/ and w/o decimated search? I.e. if you benchmark the unittests, what kind of speedup does it actually provide? https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.cc:245: const float kPi = 3.14159265f; use M_PI, see top of sinc_resampler_unittest.cc for important #include notes, essentially this needs to be done: // MSVC++ requires this to be set before any other includes to get M_PI. #define _USE_MATH_DEFINES #include <cmath> https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.cc:246: const float scale = 2.f * kPi / static_cast<float>(window_length); double https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.cc:248: window[n] = 0.5 * (1 - cos(n * scale)); 1.0 since cos takes double. https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... File media/filters/wsola_internals.h (right): https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.h:68: int FullSearch(int lim_low, low_limit, high_limit ?
Hi Dale, I tried to address your comments. Is there any specific reason that we push audio with AudioBuffer and pull with AudioBus. If we would pull by AudioBuffer as well, then perhaps I could use AudioBuffer all over place, I'm not sure but it might be a bit simpler in that case. And at few places you where concerned about memmove(), but in all places there was a risk of overlap. As far as I understood the behavior of memcpy is undefined in those case. Am I missing something? Thanks,.... Turaj https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm.cc (right): https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:167: // Round it to two decimal digits. I will rephrase the comment. Necessary in the sense that if we remove K samples from output we have to remove K * |playback_rate_| from the input. For an arbitrary |playback_rate_|, find the right value of K might be unnecessarily complex. With this rounding, we can always choose K = 100. On 2013/08/13 21:11:04, DaleCurtis wrote: > Is this really necessary? I don't really follow why from the comment below, > even if you don't truncate this here: > > int(|playback_rate_| * 100) == floor(|playback_rate_| * 100) will always be > true. > > What am I missing? And can you document it here... :) https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:203: if (target_block_index_ + ola_window_size_ <= frames && On 2013/08/13 21:11:04, DaleCurtis wrote: > Just return instead of if? Done. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:220: float* ch_opt_frame = optimal_block_->channel(k); On 2013/08/13 21:11:04, DaleCurtis wrote: > const float* const ? Done. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:239: int AudioRendererAlgorithm::GetSearchRegionIndex() const { I did that, but it feels a bit unsafe as one has to be always careful to update |search_block_index_| whenever |output_index_| is update. On 2013/08/13 21:11:04, DaleCurtis wrote: > You call this pretty frequently, it might be worth computing along side > output_index_ https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:241: const int search_block_center_index = static_cast<int>(floor( I thought without cast we get warning on Windows. On 2013/08/13 21:11:04, DaleCurtis wrote: > no need for floor + static_cast<int>, the cast is sufficient so long as the > value is always positive. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:248: void AudioRendererAlgorithm::RemoveOldInputFrames() { what we need to know is the earliest frame we need to keep for the next iteration, and all frames before that can be evicted. This is not the same as the number of frame written to output or the number of frames consumed from the input. |optimal_block_| of one iteration might overlap with the next. So it is really the following std::min() which specifies how many frames we can evict from the input. And I guess it is better to be in the function that is using it. On 2013/08/13 21:11:04, DaleCurtis wrote: > Instead of trying to calculate how many input frames to remove, would it be > better for WsolaIteration() to set some value indicating the number of frames > used? https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:260: static_cast<int>(floor(playback_rate_ * kOutputFramesPerBlock + 0.5f)); On 2013/08/13 21:11:04, DaleCurtis wrote: > Again, floor + cast is unnecessary. Done. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:261: const int blocks_to_remove = earliest_used_index / input_frames_per_block; we actually need the integer part of |earliest_used_index| / |input_frames_per_block|. On 2013/08/13 21:11:04, DaleCurtis wrote: > Is num >> den, such that integer division isn't losing precision here? https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:283: int frames_to_move = wsola_output_->frames() - rendered_frames; CopyTo() uses memcpy() and I'm concerned about the overlapping case. On 2013/08/13 21:11:04, DaleCurtis wrote: > You should be able to use the Copy helpers in AudioBus no? https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:286: memmove(ch, &ch[rendered_frames], sizeof(*ch) * frames_to_move); I'm not expecting to be zeroed, and there is no guaranty that they do not overlap. |rendered_frames| can be smaller than |frames_to_move|, right? Then source and destination overlap and the result is undefined according to memcpy manual. On 2013/08/13 21:11:04, DaleCurtis wrote: > Necessary vs memcpy? They don't seem to overlap? Are you expecting > wsola_output_ to be zeroed after this operation (it won't be)? https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:296: if (target_block_index_ >= search_block_index && On 2013/08/13 21:11:04, DaleCurtis wrote: > Again, just return the value directly. Done. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:321: internal::Interval exclude_iterval = std::make_pair(last_optimal - 80, On 2013/08/13 21:11:04, DaleCurtis wrote: > 80? Extract to constant w/ documentation. Done. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:344: float* ch_target = target_block_->channel(k); On 2013/08/13 21:11:04, DaleCurtis wrote: > const float* const ? Done. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:357: bool AudioRendererAlgorithm::PeekAudioWithZeroAppend( On 2013/08/13 21:11:04, DaleCurtis wrote: > Technically this is prepending data, not appending. Done. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:360: if (read_offset_frames + num_frames > audio_buffer_.frames()) We can do that if you advise so. I was more along the line that each method should protect itself. But maybe that is unnecessary for private methods. On 2013/08/13 21:11:04, DaleCurtis wrote: > Should this be a CHECK() instead? Then this function can just return void. You'd > be able to remove the if (...) return false in GetOptimalBlock() then too. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:364: int num_frames_to_read = dest->frames(); On 2013/08/13 21:11:04, DaleCurtis wrote: > You have two num_frames variables which do the same thing. Remove this one? Done. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:373: audio_buffer_.PeekFrames(num_frames_to_read, read_offset_frames, According to the previous comments this function is void. I don't think we need to have an if-statement to call this function only if |num_frames_to_read| > 0, do we need that? On 2013/08/13 21:11:04, DaleCurtis wrote: > Should this return true if zero frames are peeked? I.e. read_offset_frames = > -num_frames? https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm.h (right): https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:88: // target block, and write it in |optimal_block_|. Returns false it there is comment rephrased. On 2013/08/13 21:11:04, DaleCurtis wrote: > s/target block/|target_block_|/ https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:101: // channels as |audio_buffer_|. A Negative offset, i.e. On 2013/08/13 21:11:04, DaleCurtis wrote: > s/Negative/negative/ Done. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:113: bool WsolaIteration(); On 2013/08/13 21:11:04, DaleCurtis wrote: > RunOneWsolaIteration() ? Done. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:122: // Is the target block fully within search region? If so, we don't need to On 2013/08/13 21:11:04, DaleCurtis wrote: > Use explicit |target_block_| and |search_block_| references in method > descriptions (here and elsewhere) -- the algorithm is already confusing :) Done. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:150: // Waveform Similarity Overlap-and-add (WSOLA) variables. I thought a description of algorithm helps understanding of variables. Moved to .cc. On 2013/08/13 21:11:04, DaleCurtis wrote: > This is more of an algorithm description and should be in the .cc file not the > .h. Also the listed variables don't match up to the ones actually in the class. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm_unittest.cc (right): https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:30: static const int kOutputDurationInSec = 10; I run WSOLA for 4 seconds, if that results in too long test we can always reduce iterations. On 2013/08/13 21:11:04, DaleCurtis wrote: > As mentioned below, these tests are traditionally very slow w/o good reason. > You should run this under valgrind and see how long it takes to run. If it's > more than 10 or 15 seconds for all the tests, it's too long. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:242: ref->ToInterleaved(kPulseWidthSamples, sizeof(*(ref_memory_buffer.get())), I looked at the implementation of AudioBus::ToInterleaved (|ref| is audio bus not AudioBuffer) and it seems that float is not supported. On 2013/08/13 21:11:04, DaleCurtis wrote: > Is it necessary to convert to int16? The AudioBuffer can take float samples. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:245: base::TimeDelta timestamp = base::TimeDelta::FromInternalValue(0); On 2013/08/13 21:11:04, DaleCurtis wrote: > const these two. Done. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:250: scoped_refptr<AudioBuffer> input = AudioBuffer::CopyFrom( I guess you are pointing to WrapMemory(), the pain with that is the requirement of byte-alignment. On 2013/08/13 21:11:04, DaleCurtis wrote: > Technically you should be able to create an AudioBuffer w/o copying and then a > wrapped AudioBus() using AudioBuffer::writable_data(). These unittests are > usually very slow in valgrind, so avoiding any copies here can be helpful. > > I'm investigating whether this works right now though, so don't take my advice > yet :) https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:259: scoped_ptr<AudioBus> out_buffer = On 2013/08/13 21:11:04, DaleCurtis wrote: > Don't recreate inside loop. Also what's the difference between out_buffer and > output? Some better names would be nice :) Done. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:275: // Pulses in the first half of WSOLA AOL frame are not constructed No this is not a bug. WSOLA is overlap-and-add based. For the first block, there is no previous block to add-overlap. The AOL result in a smooth ramp-in for the first block. On 2013/08/13 21:11:04, DaleCurtis wrote: > Is this a bug? Will glitches be heard? https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:278: scoped_ptr<int16_t[]> test_memory_buffer( On 2013/08/13 21:11:04, DaleCurtis wrote: > Again, don't allocate inside of the loops. Allocation is extremely slow > relative to the rest of the operations here. > > Also if you can avoid interleaving for comparison that'd be helpful. Done. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:470: for (int n = 0; n < kNumBlocks; ++n) { On 2013/08/13 21:11:04, DaleCurtis wrote: > Comment on what you're testing here. Done. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:526: ASSERT_FLOAT_EQ(0, energy_candid_blocks[0]); On 2013/08/13 21:11:04, DaleCurtis wrote: > Comments on what's being tested. Done. https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... File media/filters/wsola_internals.cc (right): https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.cc:75: const float* slide_in = &input_channel[frames_per_block]; On 2013/08/13 21:11:04, DaleCurtis wrote: > just input_channel + frames_per_block ? Done. https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.cc:157: int candidate_index = n - decimation + static_cast<int>( On 2013/08/13 21:11:04, DaleCurtis wrote: > no need for cast+floor Done. https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.cc:165: similarity[2] > best_similarity && !InInterval(n, exclude_interval)) { On 2013/08/13 21:11:04, DaleCurtis wrote: > should align with ( Done. https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.cc:171: memmove(similarity, &similarity[1], 2 * sizeof(*similarity)); I thought overlapping in any direction is undefined with memcpy, do I misinterpret the manual? On 2013/08/13 21:11:04, DaleCurtis wrote: > memcpy? https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.cc:214: DCHECK(channels == target_block->channels()); On 2013/08/13 21:11:04, DaleCurtis wrote: > DCHECK_EQ Done. https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.cc:217: const int kSearchDecimation = 5; On 2013/08/13 21:11:04, DaleCurtis wrote: > Comment. Done. https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.cc:219: scoped_ptr<float[]> energy_target_block(new float[channels]); We can define local variables of a maximum expected size. It that is significantly cheaper. On 2013/08/13 21:11:04, DaleCurtis wrote: > Bummer about these allocations, but don't worry about them until we have some > performance numbers on this feature. https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.cc:231: int optimal_index = DecimatedSearch(kSearchDecimation, No I haven't, but I can do that. Computationally, beside the allocation, this should be where most CPU is spend. On 2013/08/13 21:11:04, DaleCurtis wrote: > Have you run the performance numbers w/ and w/o decimated search? I.e. if you > benchmark the unittests, what kind of speedup does it actually provide? https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.cc:245: const float kPi = 3.14159265f; On 2013/08/13 21:11:04, DaleCurtis wrote: > use M_PI, see top of sinc_resampler_unittest.cc for important #include notes, > essentially this needs to be done: > > // MSVC++ requires this to be set before any other includes to get M_PI. > #define _USE_MATH_DEFINES > > #include <cmath> Done. https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.cc:246: const float scale = 2.f * kPi / static_cast<float>(window_length); On 2013/08/13 21:11:04, DaleCurtis wrote: > double Done. https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.cc:248: window[n] = 0.5 * (1 - cos(n * scale)); Can I use cosf()? just because |window| is float. On 2013/08/13 21:11:04, DaleCurtis wrote: > 1.0 since cos takes double. https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... File media/filters/wsola_internals.h (right): https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.h:68: int FullSearch(int lim_low, On 2013/08/13 21:11:04, DaleCurtis wrote: > low_limit, high_limit ? Done.
If there's overlap, memmove is the right choice. I didn't think there was, but I think you're right after all. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm.cc (right): https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:239: int AudioRendererAlgorithm::GetSearchRegionIndex() const { On 2013/08/16 22:13:56, turaj wrote: > I did that, but it feels a bit unsafe as one has to be always careful to update > |search_block_index_| whenever |output_index_| is update. > > On 2013/08/13 21:11:04, DaleCurtis wrote: > > You call this pretty frequently, it might be worth computing along side > > output_index_ > Make this UpdateOutputIndex() and document on |output_index_| and |search_block_index_| that they can only be by calling that method so they are always set together? https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:241: const int search_block_center_index = static_cast<int>(floor( On 2013/08/16 22:13:56, turaj wrote: > I thought without cast we get warning on Windows. > > On 2013/08/13 21:11:04, DaleCurtis wrote: > > no need for floor + static_cast<int>, the cast is sufficient so long as the > > value is always positive. > Yes, you probably need the cast, but not cast and floor :) https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:360: if (read_offset_frames + num_frames > audio_buffer_.frames()) On 2013/08/16 22:13:56, turaj wrote: > We can do that if you advise so. I was more along the line that each method > should protect itself. But maybe that is unnecessary for private methods. > > On 2013/08/13 21:11:04, DaleCurtis wrote: > > Should this be a CHECK() instead? Then this function can just return void. > You'd > > be able to remove the if (...) return false in GetOptimalBlock() then too. > Chrome prefers to minimize potential paths through code in favor of CHECK()ing to make reasoning about the code simpler. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm.h (right): https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:150: // Waveform Similarity Overlap-and-add (WSOLA) variables. On 2013/08/16 22:13:56, turaj wrote: > I thought a description of algorithm helps understanding of variables. Moved to > .cc. > > On 2013/08/13 21:11:04, DaleCurtis wrote: > > This is more of an algorithm description and should be in the .cc file not the > > .h. Also the listed variables don't match up to the ones actually in the > class. > Feel free to add a comment telling readers to look at the top of the .cc file for a more elaborate description. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm_unittest.cc (right): https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:242: ref->ToInterleaved(kPulseWidthSamples, sizeof(*(ref_memory_buffer.get())), On 2013/08/16 22:13:56, turaj wrote: > I looked at the implementation of AudioBus::ToInterleaved (|ref| is audio bus > not AudioBuffer) and it seems that float is not supported. > > On 2013/08/13 21:11:04, DaleCurtis wrote: > > Is it necessary to convert to int16? The AudioBuffer can take float samples. Do you need the data to be interleaved? It seems like you are creating float data, interleaving it to int16, passing it to AudioRendererAlgorithm which converts it back to float upon FillBuffer(), then converting the result back to int16 for your final comparison. At the very least it seems like you could just pass in the float data directly in the beginning, no? I also don't see why you couldn't do the comparison in float after the fact either. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:250: scoped_refptr<AudioBuffer> input = AudioBuffer::CopyFrom( On 2013/08/16 22:13:56, turaj wrote: > I guess you are pointing to WrapMemory(), the pain with that is the requirement > of byte-alignment. > > On 2013/08/13 21:11:04, DaleCurtis wrote: > > Technically you should be able to create an AudioBuffer w/o copying and then a > > wrapped AudioBus() using AudioBuffer::writable_data(). These unittests are > > usually very slow in valgrind, so avoiding any copies here can be helpful. > > > > I'm investigating whether this works right now though, so don't take my advice > > yet :) > AudioBuffer alignment should take care of this for you. You should be able to say AudioBuffer::Create(kSampleFormatPlanarF32, ...), then AudioBus::CreateWrapper(), for(ch in AudioBuffer) AudioBus::SetChannel(ch, AudioBuffer::channel_data(ch)); You can then call FillWithSqurePulseTrain() on that AudioBus and the AudioBuffer will be populated automatically. https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... File media/filters/wsola_internals.cc (right): https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.cc:157: int candidate_index = n - decimation + static_cast<int>( On 2013/08/16 22:13:56, turaj wrote: > On 2013/08/13 21:11:04, DaleCurtis wrote: > > no need for cast+floor > > Done. Integer value, so just use static_cast instead of floor(). https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.cc:219: scoped_ptr<float[]> energy_target_block(new float[channels]); On 2013/08/16 22:13:56, turaj wrote: > We can define local variables of a maximum expected size. It that is > significantly cheaper. > > On 2013/08/13 21:11:04, DaleCurtis wrote: > > Bummer about these allocations, but don't worry about them until we have some > > performance numbers on this feature. > We should save that for a follow up optimization pass if necessary. https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.cc:248: window[n] = 0.5 * (1 - cos(n * scale)); On 2013/08/16 22:13:56, turaj wrote: > Can I use cosf()? just because |window| is float. > > On 2013/08/13 21:11:04, DaleCurtis wrote: > > 1.0 since cos takes double. > Yeah that's probably better than double everywhere. https://codereview.chromium.org/19111004/diff/74001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm.cc (right): https://codereview.chromium.org/19111004/diff/74001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:53: // Max/min supported playback rates for fast/slow audio. Audio outside of these Are these values still reasonable with the new algorithm? https://codereview.chromium.org/19111004/diff/74001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:203: playback_rate_ = floorf(new_rate * 100.f + 0.5f) / 100.f; I worry that modifying the playback rate like this might impact the AV sync as calculated by AudioRendererImpl and as expected by the client: https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/audi... Blink is still maintaining its own playback rate which will now be divergent. Practically, I don't know if any of this is noticeable though. https://codereview.chromium.org/19111004/diff/74001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:273: output_index_ * playback_rate_ + 0.5); 0.5f
CL is mostly ready for submission, but there are a few benchmarks I want to see before submission: - old method vs new for common playback rates. - w/ and w/o decimated search to see if its necessary. - time it takes to run all AudioRendererAlgorithm tests under valgrind. The first two could simply measure the time taken to speed up and slow down a significant amount of audio data.
Hi, I addressed the comments and simplified. I thought I need to use one of the new APIs in AudioBus (or AudioBuffer cannot recall) so I rebased my workspace. that's why you see the gyp file is changed. I haven't done benchmark test nor the Valgrind. I was wondering if there is a standalone app testing time-stretch I can use. Otherwise I'll write a simple one. Thanks,... Turaj https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm.cc (right): https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:239: int AudioRendererAlgorithm::GetSearchRegionIndex() const { On 2013/08/19 22:15:23, DaleCurtis wrote: > On 2013/08/16 22:13:56, turaj wrote: > > I did that, but it feels a bit unsafe as one has to be always careful to > update > > |search_block_index_| whenever |output_index_| is update. > > > > On 2013/08/13 21:11:04, DaleCurtis wrote: > > > You call this pretty frequently, it might be worth computing along side > > > output_index_ > > > > Make this UpdateOutputIndex() and document on |output_index_| and > |search_block_index_| that they can only be by calling that method so they are > always set together? Done. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:360: if (read_offset_frames + num_frames > audio_buffer_.frames()) Sure. On 2013/08/19 22:15:23, DaleCurtis wrote: > On 2013/08/16 22:13:56, turaj wrote: > > We can do that if you advise so. I was more along the line that each method > > should protect itself. But maybe that is unnecessary for private methods. > > > > On 2013/08/13 21:11:04, DaleCurtis wrote: > > > Should this be a CHECK() instead? Then this function can just return void. > > You'd > > > be able to remove the if (...) return false in GetOptimalBlock() then too. > > > > Chrome prefers to minimize potential paths through code in favor of CHECK()ing > to make reasoning about the code simpler. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm.h (right): https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.h:150: // Waveform Similarity Overlap-and-add (WSOLA) variables. On 2013/08/19 22:15:23, DaleCurtis wrote: > On 2013/08/16 22:13:56, turaj wrote: > > I thought a description of algorithm helps understanding of variables. Moved > to > > .cc. > > > > On 2013/08/13 21:11:04, DaleCurtis wrote: > > > This is more of an algorithm description and should be in the .cc file not > the > > > .h. Also the listed variables don't match up to the ones actually in the > > class. > > > Feel free to add a comment telling readers to look at the top of the .cc file > for a more elaborate description. Done. If you feel appropriate I can give reference to the paper where the algorithm is published. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm_unittest.cc (right): https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:242: ref->ToInterleaved(kPulseWidthSamples, sizeof(*(ref_memory_buffer.get())), I believe I have a better solution. On 2013/08/19 22:15:23, DaleCurtis wrote: > On 2013/08/16 22:13:56, turaj wrote: > > I looked at the implementation of AudioBus::ToInterleaved (|ref| is audio bus > > not AudioBuffer) and it seems that float is not supported. > > > > On 2013/08/13 21:11:04, DaleCurtis wrote: > > > Is it necessary to convert to int16? The AudioBuffer can take float samples. > > Do you need the data to be interleaved? It seems like you are creating float > data, interleaving it to int16, passing it to AudioRendererAlgorithm which > converts it back to float upon FillBuffer(), then converting the result back to > int16 for your final comparison. At the very least it seems like you could just > pass in the float data directly in the beginning, no? I also don't see why you > couldn't do the comparison in float after the fact either. https://codereview.chromium.org/19111004/diff/52001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:250: scoped_refptr<AudioBuffer> input = AudioBuffer::CopyFrom( I guess the current implementation is simpler. Thanks for the tip. On 2013/08/19 22:15:23, DaleCurtis wrote: > On 2013/08/16 22:13:56, turaj wrote: > > I guess you are pointing to WrapMemory(), the pain with that is the > requirement > > of byte-alignment. > > > > On 2013/08/13 21:11:04, DaleCurtis wrote: > > > Technically you should be able to create an AudioBuffer w/o copying and then > a > > > wrapped AudioBus() using AudioBuffer::writable_data(). These unittests are > > > usually very slow in valgrind, so avoiding any copies here can be helpful. > > > > > > I'm investigating whether this works right now though, so don't take my > advice > > > yet :) > > > > AudioBuffer alignment should take care of this for you. You should be able to > say AudioBuffer::Create(kSampleFormatPlanarF32, ...), then > AudioBus::CreateWrapper(), for(ch in AudioBuffer) AudioBus::SetChannel(ch, > AudioBuffer::channel_data(ch)); > > You can then call FillWithSqurePulseTrain() on that AudioBus and the AudioBuffer > will be populated automatically. https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... File media/filters/wsola_internals.cc (right): https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.cc:157: int candidate_index = n - decimation + static_cast<int>( On 2013/08/19 22:15:23, DaleCurtis wrote: > On 2013/08/16 22:13:56, turaj wrote: > > On 2013/08/13 21:11:04, DaleCurtis wrote: > > > no need for cast+floor > > > > Done. > > Integer value, so just use static_cast instead of floor(). Done. https://codereview.chromium.org/19111004/diff/52001/media/filters/wsola_inter... media/filters/wsola_internals.cc:248: window[n] = 0.5 * (1 - cos(n * scale)); Then does it make sense to have const float kPi = 3.141592f; On 2013/08/19 22:15:23, DaleCurtis wrote: > On 2013/08/16 22:13:56, turaj wrote: > > Can I use cosf()? just because |window| is float. > > > > On 2013/08/13 21:11:04, DaleCurtis wrote: > > > 1.0 since cos takes double. > > > > Yeah that's probably better than double everywhere. https://codereview.chromium.org/19111004/diff/74001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm.cc (right): https://codereview.chromium.org/19111004/diff/74001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:53: // Max/min supported playback rates for fast/slow audio. Audio outside of these The new algorithm can perform at these rates, obviously the quality degrades, but the algorithm doesn't fail. On 2013/08/19 22:15:23, DaleCurtis wrote: > Are these values still reasonable with the new algorithm? https://codereview.chromium.org/19111004/diff/74001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:203: playback_rate_ = floorf(new_rate * 100.f + 0.5f) / 100.f; It is a valid concern, and I believe I have solution for it. Let me re-state the algorithm in short: If we are at time T for the output we search vicinity of T * |playback_rate_| of input to find a block that is the best continuation of output. So there is no need to restrict |playback_rate_| to any precision. The problem is when we want to evict samples from input. If we remove K samples from input, it is equivalent to shift the reference point of time by K for input, and the equivalent for that is to shift the reference of output by K / |playback_rate_|. The rounding guaranteed that K / |playback_rate_| is integer, so the input and output time axes would not drift. The proposed solution is to relax |output_index_| to be float. Obviously, I need to rename it. It is never used as an index. On 2013/08/19 22:15:23, DaleCurtis wrote: > I worry that modifying the playback rate like this might impact the AV sync as > calculated by AudioRendererImpl and as expected by the client: > > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/audi... > > Blink is still maintaining its own playback rate which will now be divergent. > Practically, I don't know if any of this is noticeable though. https://codereview.chromium.org/19111004/diff/74001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:273: output_index_ * playback_rate_ + 0.5); On 2013/08/19 22:15:23, DaleCurtis wrote: > 0.5f Done. https://codereview.chromium.org/19111004/diff/85001/media/filters/wsola_inter... File media/filters/wsola_internals.h (right): https://codereview.chromium.org/19111004/diff/85001/media/filters/wsola_inter... media/filters/wsola_internals.h:66: // |target_block_|. |energy_candidate_blocks| is the energy of all blocks within Will be corrected in the next upload.
For benchmarking you should just be able to time the various audio renderer algorithm FillBuffer() unittests. Do you think that is not sufficient?
It should be sufficient, I thought you want them to be run on long file to be more accurate. Is the execution time reportd by unittest accurate? On Tue, Aug 20, 2013 at 6:16 PM, <dalecurtis@chromium.org> wrote: > For benchmarking you should just be able to time the various audio renderer > algorithm FillBuffer() unittests. Do you think that is not sufficient? > > https://codereview.chromium.**org/19111004/<https://codereview.chromium.org/1... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
It's a good first estimate. I was trying to suggest you adjust the values in AudioRendererAlgorithmTest::TestPlaybackRate() to be longer for the purpose of benchmarking. I.e. instead use a 48kHz sample and say 20 seconds worth of buffers.
WSOLA [ RUN ] AudioRendererAlgorithmTest.FillBuffer_SmallBufferSize [ OK ] AudioRendererAlgorithmTest.FillBuffer_SmallBufferSize (9039 ms) [ RUN ] AudioRendererAlgorithmTest.FillBuffer_LargeBufferSize [ OK ] AudioRendererAlgorithmTest.FillBuffer_LargeBufferSize (19544 ms) [ RUN ] AudioRendererAlgorithmTest.FillBuffer_LowerQualityAudio [ OK ] AudioRendererAlgorithmTest.FillBuffer_LowerQualityAudio (12315 ms) [ RUN ] AudioRendererAlgorithmTest.FillBuffer_HigherQualityAudio [ OK ] AudioRendererAlgorithmTest.FillBuffer_HigherQualityAudio (24724 ms) [ RUN ] AudioRendererAlgorithmTest.WsolaSlowdown [ OK ] AudioRendererAlgorithmTest.WsolaSlowdown (8246 ms) [ RUN ] AudioRendererAlgorithmTest.WsolaSpeedup [ OK ] AudioRendererAlgorithmTest.WsolaSpeedup (14858 ms) Why speed up is more expensive than slow down? Here are some statistics when 400 sec of output is requested in both cases. Slow-down: (playback-rate 0.6) Inserted pulses 40004 Generated pulses 66666 Number of times search should perform: 6668 Number of times target was within search: 33332 Speed-up: (playback-rate 1.6) Inserted pulses 106669 Generated pulses 66666 Number of times search should perform: 19998 Number of times target was within search: 20002 Current Implementation [ RUN ] AudioRendererAlgorithmTest.FillBuffer_SmallBufferSize [ OK ] AudioRendererAlgorithmTest.FillBuffer_SmallBufferSize (6184 ms) [ RUN ] AudioRendererAlgorithmTest.FillBuffer_LargeBufferSize [ OK ] AudioRendererAlgorithmTest.FillBuffer_LargeBufferSize (1584 ms) [ RUN ] AudioRendererAlgorithmTest.FillBuffer_LowerQualityAudio [ OK ] AudioRendererAlgorithmTest.FillBuffer_LowerQualityAudio (1585 ms) [ RUN ] AudioRendererAlgorithmTest.FillBuffer_HigherQualityAudio [ OK ] AudioRendererAlgorithmTest.FillBuffer_HigherQualityAudio (1898 ms) [ RUN ] AudioRendererAlgorithmTest.WsolaSlowdown [ OK ] AudioRendererAlgorithmTest.WsolaSlowdown (1619 ms) [ RUN ] AudioRendererAlgorithmTest.WsolaSpeedup [ OK ] AudioRendererAlgorithmTest.WsolaSpeedup (2023 ms) On Tue, Aug 20, 2013 at 6:34 PM, <dalecurtis@chromium.org> wrote: > It's a good first estimate. I was trying to suggest you adjust the values > in > AudioRendererAlgorithmTest::**TestPlaybackRate() to be longer for the > purpose of > benchmarking. I.e. instead use a 48kHz sample and say 20 seconds worth of > buffers. > > https://codereview.chromium.**org/19111004/<https://codereview.chromium.org/1... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
sorry, please ignore my last e-mail, it was not finished. On Wed, Aug 21, 2013 at 3:43 PM, Turaj Zakizadeh Shabestary < turaj@webrtc.org> wrote: > > WSOLA > > [ RUN ] AudioRendererAlgorithmTest.FillBuffer_SmallBufferSize > [ OK ] AudioRendererAlgorithmTest.FillBuffer_SmallBufferSize (9039 > ms) > [ RUN ] AudioRendererAlgorithmTest.FillBuffer_LargeBufferSize > [ OK ] AudioRendererAlgorithmTest.FillBuffer_LargeBufferSize (19544 > ms) > [ RUN ] AudioRendererAlgorithmTest.FillBuffer_LowerQualityAudio > [ OK ] AudioRendererAlgorithmTest.FillBuffer_LowerQualityAudio > (12315 ms) > [ RUN ] AudioRendererAlgorithmTest.FillBuffer_HigherQualityAudio > [ OK ] AudioRendererAlgorithmTest.FillBuffer_HigherQualityAudio > (24724 ms) > > [ RUN ] AudioRendererAlgorithmTest.WsolaSlowdown > [ OK ] AudioRendererAlgorithmTest.WsolaSlowdown (8246 ms) > [ RUN ] AudioRendererAlgorithmTest.WsolaSpeedup > [ OK ] AudioRendererAlgorithmTest.WsolaSpeedup (14858 ms) > > > > > > > > Why speed up is more expensive than slow down? Here are some statistics > when 400 sec of output is requested in both cases. > > Slow-down: (playback-rate 0.6) > Inserted pulses 40004 > Generated pulses 66666 > Number of times search should perform: 6668 > Number of times target was within search: 33332 > > Speed-up: (playback-rate 1.6) > Inserted pulses 106669 > Generated pulses 66666 > Number of times search should perform: 19998 > Number of times target was within search: 20002 > > > Current Implementation > > [ RUN ] AudioRendererAlgorithmTest.FillBuffer_SmallBufferSize > [ OK ] AudioRendererAlgorithmTest.FillBuffer_SmallBufferSize (6184 > ms) > [ RUN ] AudioRendererAlgorithmTest.FillBuffer_LargeBufferSize > [ OK ] AudioRendererAlgorithmTest.FillBuffer_LargeBufferSize (1584 > ms) > [ RUN ] AudioRendererAlgorithmTest.FillBuffer_LowerQualityAudio > [ OK ] AudioRendererAlgorithmTest.FillBuffer_LowerQualityAudio (1585 > ms) > [ RUN ] AudioRendererAlgorithmTest.FillBuffer_HigherQualityAudio > [ OK ] AudioRendererAlgorithmTest.FillBuffer_HigherQualityAudio > (1898 ms) > > [ RUN ] AudioRendererAlgorithmTest.WsolaSlowdown > [ OK ] AudioRendererAlgorithmTest.WsolaSlowdown (1619 ms) > [ RUN ] AudioRendererAlgorithmTest.WsolaSpeedup > [ OK ] AudioRendererAlgorithmTest.WsolaSpeedup (2023 ms) > > > On Tue, Aug 20, 2013 at 6:34 PM, <dalecurtis@chromium.org> wrote: > >> It's a good first estimate. I was trying to suggest you adjust the >> values in >> AudioRendererAlgorithmTest::**TestPlaybackRate() to be longer for the >> purpose of >> benchmarking. I.e. instead use a 48kHz sample and say 20 seconds worth >> of >> buffers. >> >> https://codereview.chromium.**org/19111004/<https://codereview.chromium.org/1... >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hi Dale, These are some results to benchmark WSOLA compared to the current implementation in Chrome. I also shared a document where I add more results to. WSOLA Benchmarking Test Current Implementation [ms] Wsola Implementation [ms] FillBuffer_SmallBufferSize 6046 7000 FillBuffer_LargeBufferSize 1581 18650 FillBuffer_LowerQualityAudio 1584 11753 FillBuffer_HigherQualityAudio 1894 23845 Slowdown 1984 8268 Speedup 1586 14887 For FillBuffer_* tests I had kSamplesPerSecond = 48000 and kOutputDurationInSec = 400. For Slowdown and Speedup tests 400 sec of audio is generated at the output with sampling rate of 48kHz. The reason for difference in complexity between speedup and slowdown of WSOLA can be described in the following Slowdown: (playback-rate 0.6) Inserted pulses 40004 Generated pulses 66666 Number of times search should perform: 6668 Number of times target was within search: 33332 Speedup: (playback-rate 1.6) Inserted pulses 106669 Generated pulses 66666 Number of times search should perform: 19998 Number of times target was within search: 20002 The number times that a search should perform vs the times when no search is needed to some extend is influenced by the periodic nature of the input signal. I'll continue with Valgrind and Decimated-search test. Thanks,... Turaj On Tue, Aug 20, 2013 at 6:34 PM, <dalecurtis@chromium.org> wrote: > It's a good first estimate. I was trying to suggest you adjust the values > in > AudioRendererAlgorithmTest::**TestPlaybackRate() to be longer for the > purpose of > benchmarking. I.e. instead use a 48kHz sample and say 20 seconds worth of > buffers. > > https://codereview.chromium.**org/19111004/<https://codereview.chromium.org/1... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hi Dale, I resolved Valgrind warning. After that FillBuffer_SmallBufferSize unit-test failed. The reason was obvious. The very first frame out of WSOLA is 0, therefore CheckFakeData() would set |all_zero| to true, as there is only one frame is written to output in this test. I fixed that as well. Please have a look. The only thing left is to measure the impact of PartialSearch() on complexity. Thanks,... Turaj
Partial-search Impact on Complexity Speedup and slowdown has been running to produce 400 seconds of time-scaled output. Here is the run-time in milliseconds for each test. Test Partial Search Enabled [ms] Partial Search Disabled [ms] Slowdown 9421 16542 Speedup 15406 99264 On Thu, Aug 22, 2013 at 10:21 AM, <turaj@webrtc.org> wrote: > Hi Dale, > > I resolved Valgrind warning. > > After that FillBuffer_SmallBufferSize unit-test failed. The reason was > obvious. > The very first frame out of WSOLA is 0, therefore CheckFakeData() would set > |all_zero| to true, as there is only one frame is written to output in this > test. I fixed that as well. Please have a look. > > The only thing left is to measure the impact of PartialSearch() on > complexity. > > Thanks,... Turaj > > https://codereview.chromium.**org/19111004/<https://codereview.chromium.org/1... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks for the benchmarks! It seems there is an order of magnitude slow down, but still plenty of room for real time playback. https://codereview.chromium.org/19111004/diff/74001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm.cc (right): https://codereview.chromium.org/19111004/diff/74001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:53: // Max/min supported playback rates for fast/slow audio. Audio outside of these On 2013/08/21 01:01:19, turaj wrote: > The new algorithm can perform at these rates, obviously the quality degrades, > but the algorithm doesn't fail. > > On 2013/08/19 22:15:23, DaleCurtis wrote: > > Are these values still reasonable with the new algorithm? > Okay, we should consider extending them in a future CL. https://codereview.chromium.org/19111004/diff/74001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:203: playback_rate_ = floorf(new_rate * 100.f + 0.5f) / 100.f; On 2013/08/21 01:01:19, turaj wrote: > It is a valid concern, and I believe I have solution for it. > > Let me re-state the algorithm in short: > > If we are at time T for the output we search vicinity of T * |playback_rate_| of > input to find a block that is the best continuation of output. So there is no > need to restrict |playback_rate_| to any precision. > > The problem is when we want to evict samples from input. If we remove K samples > from input, it is equivalent to shift the reference point of time by K for > input, and the equivalent for that is to shift the reference of output by K / > |playback_rate_|. The rounding guaranteed that K / |playback_rate_| is integer, > so the input and output time axes would not drift. > > The proposed solution is to relax |output_index_| to be float. Obviously, I need > to rename it. It is never used as an index. > > > On 2013/08/19 22:15:23, DaleCurtis wrote: > > I worry that modifying the playback rate like this might impact the AV sync as > > calculated by AudioRendererImpl and as expected by the client: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/audi... > > > > Blink is still maintaining its own playback rate which will now be divergent. > > Practically, I don't know if any of this is noticeable though. > Is float sufficient precision? I use a double in SincResampler to prevent drift. https://codereview.chromium.org/19111004/diff/93001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm_unittest.cc (right): https://codereview.chromium.org/19111004/diff/93001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:249: std::vector<uint8_t*> channel_data = input->channel_data(); This makes a copy of the vector, you want to use const & or just the value directly in the calls below. https://codereview.chromium.org/19111004/diff/93001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:263: float kTolerance = 0.0001f; Do you want to quantify this in terms of dbFS? See the unit tests for SincResampler. Up to you, but either way, make this const. https://codereview.chromium.org/19111004/diff/93001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:288: EXPECT_NEAR(reinterpret_cast<float*>(channel_data[m])[k], Is there an ASSERT_NEAR() ? Otherwise this is going to be ultra spammy on failure :) https://codereview.chromium.org/19111004/diff/93001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:603: WsolaTest(0.6f); Why 0.6 and 1.6? https://codereview.chromium.org/19111004/diff/93001/media/filters/wsola_inter... File media/filters/wsola_internals.cc (right): https://codereview.chromium.org/19111004/diff/93001/media/filters/wsola_inter... media/filters/wsola_internals.cc:256: const float scale = 2.0f * M_PI / static_cast<float>(window_length); cast should be unnecessary.
Hi Dale, Please have a look at the recent changes in patch set 9. Thanks,... Turaj https://codereview.chromium.org/19111004/diff/74001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm.cc (right): https://codereview.chromium.org/19111004/diff/74001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:53: // Max/min supported playback rates for fast/slow audio. Audio outside of these Sure, I did change them to play YT files at playback rate of 0.25 for my listening test. On 2013/08/22 22:36:35, DaleCurtis wrote: > On 2013/08/21 01:01:19, turaj wrote: > > The new algorithm can perform at these rates, obviously the quality degrades, > > but the algorithm doesn't fail. > > > > On 2013/08/19 22:15:23, DaleCurtis wrote: > > > Are these values still reasonable with the new algorithm? > > > > Okay, we should consider extending them in a future CL. https://codereview.chromium.org/19111004/diff/74001/media/filters/audio_rende... media/filters/audio_renderer_algorithm.cc:203: playback_rate_ = floorf(new_rate * 100.f + 0.5f) / 100.f; I doubt it would, but as we don't lose much by having it double, I change. On 2013/08/22 22:36:35, DaleCurtis wrote: > On 2013/08/21 01:01:19, turaj wrote: > > It is a valid concern, and I believe I have solution for it. > > > > Let me re-state the algorithm in short: > > > > If we are at time T for the output we search vicinity of T * |playback_rate_| > of > > input to find a block that is the best continuation of output. So there is no > > need to restrict |playback_rate_| to any precision. > > > > The problem is when we want to evict samples from input. If we remove K > samples > > from input, it is equivalent to shift the reference point of time by K for > > input, and the equivalent for that is to shift the reference of output by K / > > |playback_rate_|. The rounding guaranteed that K / |playback_rate_| is > integer, > > so the input and output time axes would not drift. > > > > The proposed solution is to relax |output_index_| to be float. Obviously, I > need > > to rename it. It is never used as an index. > > > > > > On 2013/08/19 22:15:23, DaleCurtis wrote: > > > I worry that modifying the playback rate like this might impact the AV sync > as > > > calculated by AudioRendererImpl and as expected by the client: > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/audi... > > > > > > Blink is still maintaining its own playback rate which will now be > divergent. > > > Practically, I don't know if any of this is noticeable though. > > > > Is float sufficient precision? I use a double in SincResampler to prevent > drift. https://codereview.chromium.org/19111004/diff/93001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm_unittest.cc (right): https://codereview.chromium.org/19111004/diff/93001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:249: std::vector<uint8_t*> channel_data = input->channel_data(); On 2013/08/22 22:36:36, DaleCurtis wrote: > This makes a copy of the vector, you want to use const & or just the value > directly in the calls below. Done. https://codereview.chromium.org/19111004/diff/93001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:263: float kTolerance = 0.0001f; This is a sample based comparison rather than RMS domain, so I feel a bit not to relate it to dBFs. I totally agree that it might make sense to check the difference to check the relative absolute values something like 1 - kTolerance < |abs(actual)| / |abs(expected)| < 1 + kTOlerance. However, |expected| in this test is either -1 or 1, therefore, absolute ratio is the same is absolute difference. On 2013/08/22 22:36:36, DaleCurtis wrote: > Do you want to quantify this in terms of dbFS? See the unit tests for > SincResampler. Up to you, but either way, make this const. https://codereview.chromium.org/19111004/diff/93001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:288: EXPECT_NEAR(reinterpret_cast<float*>(channel_data[m])[k], On 2013/08/22 22:36:36, DaleCurtis wrote: > Is there an ASSERT_NEAR() ? Otherwise this is going to be ultra spammy on > failure :) Done. https://codereview.chromium.org/19111004/diff/93001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:603: WsolaTest(0.6f); Pretty arbitrary, we can choose any number. On 2013/08/22 22:36:36, DaleCurtis wrote: > Why 0.6 and 1.6? https://codereview.chromium.org/19111004/diff/93001/media/filters/wsola_inter... File media/filters/wsola_internals.cc (right): https://codereview.chromium.org/19111004/diff/93001/media/filters/wsola_inter... media/filters/wsola_internals.cc:256: const float scale = 2.0f * M_PI / static_cast<float>(window_length); Done. I suppose M_PI is considered double, and the rhs is evaluated in double then down cast to float. Don't like it that much. On 2013/08/22 22:36:36, DaleCurtis wrote: > cast should be unnecessary.
lgtm % nit -- thanks turaj! https://codereview.chromium.org/19111004/diff/93001/media/filters/audio_rende... File media/filters/audio_renderer_algorithm_unittest.cc (right): https://codereview.chromium.org/19111004/diff/93001/media/filters/audio_rende... media/filters/audio_renderer_algorithm_unittest.cc:249: std::vector<uint8_t*> channel_data = input->channel_data(); On 2013/08/23 21:14:09, turaj wrote: > On 2013/08/22 22:36:36, DaleCurtis wrote: > > This makes a copy of the vector, you want to use const & or just the value > > directly in the calls below. > > Done. I think you want const std::vector<uint8_t*>& channel_data ?
Hi Dale,
Thanks for the review and the constructive comments. I fixed the last nit and
uploaded. I still don't have @chromium.org account. I just requested one today.
I sent CL to try server like
$ git cl try
Loaded authentication cookies from
/usr/local/google/home/turajs/.codereview_upload_cookies
Tried jobs on:
android_aosp: defaulttests
android_clang_dbg: defaulttests
android_dbg: defaulttests
ios_dbg_simulator: defaulttests
ios_rel_device: defaulttests
linux_asan: defaulttests
linux_aura: defaulttests
linux_chromeos: defaulttests
linux_clang: compile
linux_rel: defaulttests
mac: compile
mac_rel: defaulttests
win: compile
win7_aura: defaulttests
win_rel: defaulttests
win_x64_rel: compile
I suppose you would commit the code right? Is there anything else I should do?
Thanks,... Turaj
You can just click the "commit" checkbox on this review to send the change to the commit-queue -- it will commit after testing passes.
Thanks. The patch fails to compile in some try servers. media::internal methods cannot be found. Perhaps failed to successfully patch media.gyp. Should I still send to commit-queue, or can I do something to prevent such failure happen while in commit queue? On Tue, Aug 27, 2013 at 11:23 AM, <dalecurtis@chromium.org> wrote: > You can just click the "commit" checkbox on this review to send the change > to > the commit-queue -- it will commit after testing passes. > > > https://codereview.chromium.**org/19111004/<https://codereview.chromium.org/1... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Ah I forgot you're using those internal methods in unittests. You'll need to add MEDIA_EXPORT to each method.
On 2013/08/27 19:46:35, DaleCurtis wrote: > Ah I forgot you're using those internal methods in unittests. You'll need to add > MEDIA_EXPORT to each method. I still get error like here http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...
That try doesn't seem to be for the MEDIA_EXPORT version? https://codereview.chromium.org/19111004/diff/124001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/19111004/diff/124001/media/media.gyp#newcode381 media/media.gyp:381: 'midi/midi_manager.h', I think this is an accidental addition.
https://codereview.chromium.org/19111004/diff/124001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/19111004/diff/124001/media/media.gyp#newcode381 media/media.gyp:381: 'midi/midi_manager.h', I rebased it and seems that merge didn't do a correct job. However this is duplicate. Could that really cause failure? On 2013/08/27 23:39:36, DaleCurtis wrote: > I think this is an accidental addition.
No, I was just pointing it out. I think you're looking at the wrong try results. Wait for the new ones to come w/ MEDIA_EXPORT.
I get errors like this which seems legitimate. Please have look if the solution is acceptable. http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/163607...
Hi Dale, Patch set 14 passed all try servers. There has been a bit of changes since you gave the approval. Patch set 10-14 are those you haven't commented on. Please have a look. Thanks,... Turaj
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/turaj@webrtc.org/19111004/115007
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
Hi Dale, Could you please have a look on this error message? Is this failure because "turaj@webrtc.org" is not in the list of AUTHORS? Who has rights to edit that file? Thanks,... Turaj On Wed, Aug 28, 2013 at 2:56 PM, <commit-bot@chromium.org> wrote: > Retried try job too often on chromium_presubmit for step(s) presubmit > http://build.chromium.org/p/**tryserver.chromium/** > buildstatus?builder=chromium_**presubmit&number=22881<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=22881> > > https://chromiumcodereview.**appspot.com/19111004/<https://chromiumcodereview... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Ah crap, yeah it wants you to be listed there since you didn't use your @google or @chromium account. I don't think we want to add *@webrtc.org to the AUTHORS file and you technically fall under @google/@chromium so it doesn't make sense to add you as an individual contributor. Without having you reupload the thing under a different account I don't think there's a good fix. I'll just manually land it for you now since all bots are green.
Thanks, sorry for the inconvenience. If I knew before hand, I would use @ google.com account. Hope this doesn't cause you much trouble. On Thu, Aug 29, 2013 at 11:23 AM, <dalecurtis@chromium.org> wrote: > Ah crap, yeah it wants you to be listed there since you didn't use your > @google > or @chromium account. I don't think we want to add *@webrtc.org to the > AUTHORS > file and you technically fall under @google/@chromium so it doesn't make > sense > to add you as an individual contributor. > > Without having you reupload the thing under a different account I don't > think > there's a good fix. I'll just manually land it for you now since all bots > are > green. > > > https://codereview.chromium.**org/19111004/<https://codereview.chromium.org/1... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Committed patchset #14 manually as r220343 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
