Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
151120:
OLA Algorithm and test shell. (Closed)
|
Created:
11 years, 5 months ago by kylep Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionOLA Algorithm and test shell.
BUG=16011
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=19994
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 116
Patch Set 4 : '' #
Total comments: 4
Patch Set 5 : '' #
Total comments: 13
Patch Set 6 : '' #
Total comments: 1
Patch Set 7 : '' #
Total comments: 4
Patch Set 8 : '' #
Messages
Total messages: 13 (0 generated)
Split this into 2 CLs. First (necessary) one is here: http://codereview.chromium.org/150140
ARAB and ARAD changes went through. Here's my preliminary slowdown code (working on speedup, but I'll keep that for the next CL).
Pleeeeaaasse review this. It has been tested with both speedup and slowdown.
http://codereview.chromium.org/151120/diff/1014/24 File media/filters/audio_renderer_algorithm_ola.cc (right): http://codereview.chromium.org/151120/diff/1014/24#newcode7 Line 7: #include<cmath> space between include and <cmath> http://codereview.chromium.org/151120/diff/1014/24#newcode10 Line 10: #include "media/base/buffers.h" alphabetize includes http://codereview.chromium.org/151120/diff/1014/24#newcode19 Line 19: input_step_(1), for the sake of debugging I'd set these to something bogus, such as 0 http://codereview.chromium.org/151120/diff/1014/24#newcode31 Line 31: AudioRendererAlgorithmBase::Initialize(channels, I'd comment saying that initializing the base class will call set_playback_rate(), because right now it's not as obvious http://codereview.chromium.org/151120/diff/1014/24#newcode35 Line 35: // Calculate length for crossfading. blank lines before comments, except for when it's the first comment in a new block of code http://codereview.chromium.org/151120/diff/1014/24#newcode36 Line 36: crossfade_len_ = kDefaultWindowSize/10; I'd prefer to see this be a constant defined alongside kDefaultWindowSize + a comment also spaces around binary operators http://codereview.chromium.org/151120/diff/1014/24#newcode38 Line 38: // To keep true to playback rate, modify the steps. blank lines before comments, except for when it's the first comment in a new block of code http://codereview.chromium.org/151120/diff/1014/24#newcode40 Line 40: output_step_ -= crossfade_len_; any reason why this calculation couldn't be performed inside set_playback_rate()? if we changed the playback rate halfway through, I believe we would no longer account for crossfade length http://codereview.chromium.org/151120/diff/1014/24#newcode50 Line 50: if (!saved_buf_) { You may want to move this to CopyInput() and AdvanceInput(), reason being that FillBuffer() should not have to touch saved_buf_ (separation of concerns) on one hand it's duplicated code, but you can have something like InitializeInput() that calls this bit of code http://codereview.chromium.org/151120/diff/1014/24#newcode55 Line 55: // Grab info from |buffer_out| and handle the simple case of normal playback period at end of comment http://codereview.chromium.org/151120/diff/1014/24#newcode56 Line 56: size_t dest_length = buffer_out->GetDataSize(); same here with dest_length -> dest_size http://codereview.chromium.org/151120/diff/1014/24#newcode59 Line 59: return CopyLengthToDest(dest_length, dest); you need to advance your input here http://codereview.chromium.org/151120/diff/1014/24#newcode63 Line 63: while (dest_length >= output_step_ + crossfade_len_) { this while loop condition seems strange, mostly because |dest_length| is the size of the buffer, and output_step_ and crossfade_len_ are constants (I think) so that must mean the length of the buffer is changing? it would be clearer if the logic was something along the lines of either a "dest_remaining > 0" or a "dest_written < target_write_amount" condition. http://codereview.chromium.org/151120/diff/1014/24#newcode64 Line 64: size_t copied = CopyLengthToDest(output_step_ + crossfade_len_, dest); quick comment for what these three lines are doing http://codereview.chromium.org/151120/diff/1014/24#newcode73 Line 73: size_t local_crossfade_length; length -> size you can also use crossfade_size (without trailing _) http://codereview.chromium.org/151120/diff/1014/24#newcode82 Line 82: Crossfade<int32>(samples, neat thing about c++: it'll infer the template type based on your arguments, so you can write: Crossfade(...) instead of: Crossfade<some type>(...) http://codereview.chromium.org/151120/diff/1014/24#newcode87 Line 87: Crossfade<int16>(samples, same here http://codereview.chromium.org/151120/diff/1014/24#newcode91 Line 91: default: I'd feel safer saying: case 1: Crossfade() break; default: NOTREACHED() << "Unsupported blah blah blah" http://codereview.chromium.org/151120/diff/1014/24#newcode92 Line 92: Crossfade<uint8>(samples, src.get(), dest); same here http://codereview.chromium.org/151120/diff/1014/24#newcode95 Line 95: // Advance pointers again. newline before // http://codereview.chromium.org/151120/diff/1014/24#newcode114 Line 114: if (playback_rate() > 1.0f) { quick comment for what you're doing! http://codereview.chromium.org/151120/diff/1014/24#newcode123 Line 123: AlignToBoundary(&input_step_); I think we'd get more accurate step calculations if we added the (non-aligned) step size with the (non-aligned) cross fade size then aligned the sum only really applies to funky playback rates I suppose http://codereview.chromium.org/151120/diff/1014/24#newcode128 Line 128: size_t saved_buf_remaining = saved_buf_->GetDataSize() - data_offset_; quick comment for what you're doing! http://codereview.chromium.org/151120/diff/1014/24#newcode138 Line 138: data_offset_ = bytes - saved_buf_remaining; what about a scenario where |bytes| was larger than 2 buffers? you might have to wrap this inside a while(!IsQueueEmpty()) and keep advancing data_offset_ and saved_buf_ until bytes reaches zero http://codereview.chromium.org/151120/diff/1014/24#newcode147 Line 147: size_t AudioRendererAlgorithmOLA::CopyLengthToDest(size_t length, length -> size again, what if it took multiple buffers to satisfy a request? may need a while(!IsQueueEmpty()) to copy everything over http://codereview.chromium.org/151120/diff/1014/24#newcode179 Line 179: Type* dest_end = dest + samples * channels(); I'd calculate a src_end and DCHECK it as well http://codereview.chromium.org/151120/diff/1014/23 File media/filters/audio_renderer_algorithm_ola.h (right): http://codereview.chromium.org/151120/diff/1014/23#newcode5 Line 5: // AudioRendererAlgorithmOLA is the pitch-preservation implementation of nit: I would add the abbreviation in brackets after the class name http://codereview.chromium.org/151120/diff/1014/23#newcode10 Line 10: // samples from the input data to fill |buffer_out| As ARAB is thread-unsafe, period between: "|buffer_out| As" http://codereview.chromium.org/151120/diff/1014/23#newcode54 Line 54: // Keeps track of whether further calls to FillBuffer should be ignored. FillBuffer -> FillBuffer() http://codereview.chromium.org/151120/diff/1014/23#newcode55 Line 55: bool out_of_input_done_; it seems to me you can infer this information based on whether the queue is empty.. am I understanding this variable correctly? http://codereview.chromium.org/151120/diff/1014/23#newcode58 Line 58: size_t crossfade_len_; would recommend calling this crossfade_size_ because len is more for things such as strings or possibly samples or time http://codereview.chromium.org/151120/diff/1014/23#newcode60 Line 60: // Aligns |value| to a channel and sample boundary period at end of comment http://codereview.chromium.org/151120/diff/1014/23#newcode61 Line 61: void AlignToBoundary(size_t* value); code ordering is usually: public/protected/private: ctor/dtor functions variables in otherwords, move all your functions to up under private: and move the variables near the bottom http://codereview.chromium.org/151120/diff/1014/23#newcode61 Line 61: void AlignToBoundary(size_t* value); would consider renaming this AlignToSampleBoundary http://codereview.chromium.org/151120/diff/1014/23#newcode65 Line 65: bool AdvanceInput(size_t bytes); so you're not actually checking this return value here anywhere I'd consider making it void and DCHECK-ing if we can't satisfy the advance amount (which makes sense if you consider that we usually call CopyLengthToDest before we advance anything) http://codereview.chromium.org/151120/diff/1014/23#newcode69 Line 69: size_t CopyLengthToDest(size_t length, uint8* dest); This sort of breaks the style guide, but for "read" type functions everyone likes to do (pointer, length), as in fread(), DataSource::Read() and many other places in the chrome source code Also for the sake of API-name-niceness, it'd be good if this function name had some symmetry with AdvanceInput() I'd recommend calling this CopyInput() (because Length doesn't tell you where it's coming from and Dest is redundant given you're passing in a pointer+length) http://codereview.chromium.org/151120/diff/1014/23#newcode71 Line 71: // Crossfades |samples| samples of dest with the data in |src|. Assumes there dest -> |dest| I would recommend adding more comments about the implications of Type as well as what one sample means sizeof(one sample) == sizeof(Type) * channels http://codereview.chromium.org/151120/diff/1014/26 File media/tools/alg_test.cc (right): http://codereview.chromium.org/151120/diff/1014/26#newcode4 Line 4: comment at top of file explaining what this application does http://codereview.chromium.org/151120/diff/1014/26#newcode5 Line 5: #define _CRT_SECURE_NO_WARNINGS 1 we shouldn't have to use this here you can either switch to use base/file_util.h (not that bad) or change the gyp file to declare this for you when building for windows http://codereview.chromium.org/151120/diff/1014/26#newcode18 Line 18: typedef media::DataBuffer DataBuffer; you can say using media::DataBuffer using media::AudioRendererAlgorithmOLA; http://codereview.chromium.org/151120/diff/1014/26#newcode37 Line 37: AudioRendererAlgorithmOLA ola; globals! bad!! declare these inside main and pass them as references to Dummy() via the constructor http://codereview.chromium.org/151120/diff/1014/26#newcode40 Line 40: class Dummy { comment explaining what this class does http://codereview.chromium.org/151120/diff/1014/26#newcode49 Line 49: }; DISALLOW_COPY_AND_ASSIGN(Dummy) http://codereview.chromium.org/151120/diff/1014/26#newcode92 Line 92: Dummy guy; quick comment here http://codereview.chromium.org/151120/diff/1014/26#newcode114 Line 114: size_t bytes_written = 0; quick comment here for this block of code http://codereview.chromium.org/151120/diff/1014/26#newcode118 Line 118: while ((bytes = ola.FillBuffer(b.get())) > (kDefaultWindowSize * 0.5f)) { I forget.. why are we only interested in receiving at least half of what we requested? why not check for zero? http://codereview.chromium.org/151120/diff/1014/26#newcode125 Line 125: if (fwrite(buf, 1, bytes, output) < 1) { code duplication! change your while loop logic to eliminate maybe something like: while (true) { fill buffer if (should write) write out if (should break) break }
Sounds great for speed up! Some tuning required and minor bug fix in tool. http://codereview.chromium.org/151120/diff/1014/26 File media/tools/alg_test.cc (right): http://codereview.chromium.org/151120/diff/1014/26#newcode16 Line 16: #include "media/filters/audio_renderer_algorithm_ola.h" this tool has an obscure name. I'd suggest something audio sounding, like wav_bender or audio_rate http://codereview.chromium.org/151120/diff/1014/26#newcode121 Line 121: return 1; cleanup on exit instead? (close files) http://codereview.chromium.org/151120/diff/1014/26#newcode125 Line 125: if (fwrite(buf, 1, bytes, output) < 1) { change to if (fwrite(&wav, sizeof(wav), 1, output) != bytes) { http://codereview.chromium.org/151120/diff/1014/26#newcode134 Line 134: wav.subchunk2_size = bytes_written; this could be filled in at the at the beginning, avoiding the seek, which in some applications may not be possible, and making the code more robust on errors such as the fwrite error we saw. http://codereview.chromium.org/151120/diff/2001/2003 File media/filters/audio_renderer_algorithm_ola.cc (right): http://codereview.chromium.org/151120/diff/2001/2003#newcode15 Line 15: const size_t kDefaultWindowSize = 4096; This size is sensitive to frequency and number of channels. in 6 channel audio, i found it sounded more like pitch shifting than with stereo. at 48000 khz, 4096 bytes is 21 milliseconds. You could aim to do 20 milliseconds worth of audio and compute the number of bytes based on bits per channel, number of channels and frequency. http://codereview.chromium.org/151120/diff/2001/2003#newcode114 Line 114: if (playback_rate() > 1.0f) { may want to limit the range, and for other rates, write the correct amount of silence, so that rate is still respected. also note that speed up sounds great, but slow down sounds metalic.
http://codereview.chromium.org/151120/diff/1014/24 File media/filters/audio_renderer_algorithm_ola.cc (right): http://codereview.chromium.org/151120/diff/1014/24#newcode7 Line 7: #include<cmath> On 2009/07/02 23:22:35, scherkus wrote: > space between include and <cmath> Done. http://codereview.chromium.org/151120/diff/1014/24#newcode10 Line 10: #include "media/base/buffers.h" On 2009/07/02 23:22:35, scherkus wrote: > alphabetize includes Done. http://codereview.chromium.org/151120/diff/1014/24#newcode19 Line 19: input_step_(1), On 2009/07/02 23:22:35, scherkus wrote: > for the sake of debugging I'd set these to something bogus, such as 0 Done. http://codereview.chromium.org/151120/diff/1014/24#newcode31 Line 31: AudioRendererAlgorithmBase::Initialize(channels, On 2009/07/02 23:22:35, scherkus wrote: > I'd comment saying that initializing the base class will call > set_playback_rate(), because right now it's not as obvious Done. http://codereview.chromium.org/151120/diff/1014/24#newcode35 Line 35: // Calculate length for crossfading. On 2009/07/02 23:22:35, scherkus wrote: > blank lines before comments, except for when it's the first comment in a new > block of code Done. http://codereview.chromium.org/151120/diff/1014/24#newcode36 Line 36: crossfade_len_ = kDefaultWindowSize/10; On 2009/07/02 23:22:35, scherkus wrote: > I'd prefer to see this be a constant defined alongside kDefaultWindowSize + a > comment > > also spaces around binary operators Fixed the spaces. Is it worth it to change the definition now since we know it's going to have to change to satisfy the requirements for pitch preservation in higher-channel / higher-bitrate audio? Also, recalculation is going to need to be done because a channel number might cause it to no longer be aligned. http://codereview.chromium.org/151120/diff/1014/24#newcode38 Line 38: // To keep true to playback rate, modify the steps. On 2009/07/02 23:22:35, scherkus wrote: > blank lines before comments, except for when it's the first comment in a new > block of code Done. http://codereview.chromium.org/151120/diff/1014/24#newcode40 Line 40: output_step_ -= crossfade_len_; On 2009/07/02 23:22:35, scherkus wrote: > any reason why this calculation couldn't be performed inside > set_playback_rate()? > > if we changed the playback rate halfway through, I believe we would no longer > account for crossfade length Interesting. This would mean moving the crossfade_size_ alignment to set_playback_rate() as well. Is that a problem? http://codereview.chromium.org/151120/diff/1014/24#newcode50 Line 50: if (!saved_buf_) { On 2009/07/02 23:22:35, scherkus wrote: > You may want to move this to CopyInput() and AdvanceInput(), reason being that > FillBuffer() should not have to touch saved_buf_ (separation of concerns) > > on one hand it's duplicated code, but you can have something like > InitializeInput() that calls this bit of code Will there be a case when AdvanceInput() is called before CopyInput()? It seems to me moving this block to CopyInput() is sufficient. http://codereview.chromium.org/151120/diff/1014/24#newcode55 Line 55: // Grab info from |buffer_out| and handle the simple case of normal playback On 2009/07/02 23:22:35, scherkus wrote: > period at end of comment Done. http://codereview.chromium.org/151120/diff/1014/24#newcode56 Line 56: size_t dest_length = buffer_out->GetDataSize(); On 2009/07/02 23:22:35, scherkus wrote: > same here with dest_length -> dest_size See comment below. http://codereview.chromium.org/151120/diff/1014/24#newcode59 Line 59: return CopyLengthToDest(dest_length, dest); On 2009/07/02 23:22:35, scherkus wrote: > you need to advance your input here Wow. Glad you caught that. Done. http://codereview.chromium.org/151120/diff/1014/24#newcode63 Line 63: while (dest_length >= output_step_ + crossfade_len_) { On 2009/07/02 23:22:35, scherkus wrote: > this while loop condition seems strange, mostly because |dest_length| is the > size of the buffer, and output_step_ and crossfade_len_ are constants (I think) > so that must mean the length of the buffer is changing? > > it would be clearer if the logic was something along the lines of either a > "dest_remaining > 0" or a "dest_written < target_write_amount" condition. dest_length receives the size of the buffer, but really keeps track of how much more we should copy. Should it be called dest_remaining? And it can't be > 0 because the code in the block isn't equipped to handle very small requests (hence ">= output_step_ + crossfade_len_" as (output_step_ + crossfade_len_) is the smallest chunk size we can handle). http://codereview.chromium.org/151120/diff/1014/24#newcode64 Line 64: size_t copied = CopyLengthToDest(output_step_ + crossfade_len_, dest); On 2009/07/02 23:22:35, scherkus wrote: > quick comment for what these three lines are doing Done. http://codereview.chromium.org/151120/diff/1014/24#newcode73 Line 73: size_t local_crossfade_length; On 2009/07/02 23:22:35, scherkus wrote: > length -> size > > you can also use crossfade_size (without trailing _) Done. http://codereview.chromium.org/151120/diff/1014/24#newcode82 Line 82: Crossfade<int32>(samples, On 2009/07/02 23:22:35, scherkus wrote: > neat thing about c++: it'll infer the template type based on your arguments, so > you can write: > Crossfade(...) > > instead of: > Crossfade<some type>(...) Dunno how much cleaner / clearer that makes it, but Done. http://codereview.chromium.org/151120/diff/1014/24#newcode87 Line 87: Crossfade<int16>(samples, On 2009/07/02 23:22:35, scherkus wrote: > same here Done. http://codereview.chromium.org/151120/diff/1014/24#newcode91 Line 91: default: On 2009/07/02 23:22:35, scherkus wrote: > I'd feel safer saying: > case 1: > Crossfade() > break; > > default: > NOTREACHED() << "Unsupported blah blah blah" Done. http://codereview.chromium.org/151120/diff/1014/24#newcode92 Line 92: Crossfade<uint8>(samples, src.get(), dest); On 2009/07/02 23:22:35, scherkus wrote: > same here Done. http://codereview.chromium.org/151120/diff/1014/24#newcode95 Line 95: // Advance pointers again. On 2009/07/02 23:22:35, scherkus wrote: > newline before // Done. http://codereview.chromium.org/151120/diff/1014/24#newcode114 Line 114: if (playback_rate() > 1.0f) { On 2009/07/02 23:22:35, scherkus wrote: > quick comment for what you're doing! Done. http://codereview.chromium.org/151120/diff/1014/24#newcode123 Line 123: AlignToBoundary(&input_step_); On 2009/07/02 23:22:35, scherkus wrote: > I think we'd get more accurate step calculations if we added the (non-aligned) > step size with the (non-aligned) cross fade size then aligned the sum > > only really applies to funky playback rates I suppose Yeah, but... everything would be funky when you try to crossfade |crossfade_size_| bytes onto your data and the crossfade kicks in on the second half of your third channel of audio, right? http://codereview.chromium.org/151120/diff/1014/24#newcode128 Line 128: size_t saved_buf_remaining = saved_buf_->GetDataSize() - data_offset_; On 2009/07/02 23:22:35, scherkus wrote: > quick comment for what you're doing! Done. http://codereview.chromium.org/151120/diff/1014/24#newcode138 Line 138: data_offset_ = bytes - saved_buf_remaining; On 2009/07/02 23:22:35, scherkus wrote: > what about a scenario where |bytes| was larger than 2 buffers? > > you might have to wrap this inside a while(!IsQueueEmpty()) and keep advancing > data_offset_ and saved_buf_ until bytes reaches zero Should I cross that bridge when I come to it? This all works because the window size == the buffer sizes. I know this change is necessary in the long run, but not for this prelim code? http://codereview.chromium.org/151120/diff/1014/24#newcode147 Line 147: size_t AudioRendererAlgorithmOLA::CopyLengthToDest(size_t length, On 2009/07/02 23:22:35, scherkus wrote: > length -> size > > again, what if it took multiple buffers to satisfy a request? may need a > while(!IsQueueEmpty()) to copy everything over > > See above? http://codereview.chromium.org/151120/diff/1014/24#newcode179 Line 179: Type* dest_end = dest + samples * channels(); On 2009/07/02 23:22:35, scherkus wrote: > I'd calculate a src_end and DCHECK it as well Done. http://codereview.chromium.org/151120/diff/1014/23 File media/filters/audio_renderer_algorithm_ola.h (right): http://codereview.chromium.org/151120/diff/1014/23#newcode5 Line 5: // AudioRendererAlgorithmOLA is the pitch-preservation implementation of On 2009/07/02 23:22:35, scherkus wrote: > nit: I would add the abbreviation in brackets after the class name Done. http://codereview.chromium.org/151120/diff/1014/23#newcode10 Line 10: // samples from the input data to fill |buffer_out| As ARAB is thread-unsafe, On 2009/07/02 23:22:35, scherkus wrote: > period between: "|buffer_out| As" Done. http://codereview.chromium.org/151120/diff/1014/23#newcode54 Line 54: // Keeps track of whether further calls to FillBuffer should be ignored. On 2009/07/02 23:22:35, scherkus wrote: > FillBuffer -> FillBuffer() Done. http://codereview.chromium.org/151120/diff/1014/23#newcode55 Line 55: bool out_of_input_done_; On 2009/07/02 23:22:35, scherkus wrote: > it seems to me you can infer this information based on whether the queue is > empty.. am I understanding this variable correctly? I believe so... so replace checks on this bool with (saved_buf_ || !IsQueueEmpty())? http://codereview.chromium.org/151120/diff/1014/23#newcode58 Line 58: size_t crossfade_len_; On 2009/07/02 23:22:35, scherkus wrote: > would recommend calling this crossfade_size_ because len is more for things such > as strings or possibly samples or time Done. http://codereview.chromium.org/151120/diff/1014/23#newcode60 Line 60: // Aligns |value| to a channel and sample boundary On 2009/07/02 23:22:35, scherkus wrote: > period at end of comment Done. http://codereview.chromium.org/151120/diff/1014/23#newcode61 Line 61: void AlignToBoundary(size_t* value); On 2009/07/02 23:22:35, scherkus wrote: > code ordering is usually: > > public/protected/private: > ctor/dtor > functions > variables > > in otherwords, move all your functions to up under private: and move the > variables near the bottom Done. http://codereview.chromium.org/151120/diff/1014/23#newcode61 Line 61: void AlignToBoundary(size_t* value); On 2009/07/02 23:22:35, scherkus wrote: > would consider renaming this AlignToSampleBoundary Done. http://codereview.chromium.org/151120/diff/1014/23#newcode65 Line 65: bool AdvanceInput(size_t bytes); On 2009/07/02 23:22:35, scherkus wrote: > so you're not actually checking this return value here anywhere > > I'd consider making it void and DCHECK-ing if we can't satisfy the advance > amount (which makes sense if you consider that we usually call CopyLengthToDest > before we advance anything) Umm... should it fail if we can't advance? This behavior is unclear. http://codereview.chromium.org/151120/diff/1014/23#newcode69 Line 69: size_t CopyLengthToDest(size_t length, uint8* dest); On 2009/07/02 23:22:35, scherkus wrote: > This sort of breaks the style guide, but for "read" type functions everyone > likes to do (pointer, length), as in fread(), DataSource::Read() and many other > places in the chrome source code > > Also for the sake of API-name-niceness, it'd be good if this function name had > some symmetry with AdvanceInput() > > I'd recommend calling this CopyInput() (because Length doesn't tell you where > it's coming from and Dest is redundant given you're passing in a pointer+length) Done. http://codereview.chromium.org/151120/diff/1014/23#newcode71 Line 71: // Crossfades |samples| samples of dest with the data in |src|. Assumes there On 2009/07/02 23:22:35, scherkus wrote: > dest -> |dest| > > I would recommend adding more comments about the implications of Type as well as > what one sample means > > sizeof(one sample) == sizeof(Type) * channels Done. http://codereview.chromium.org/151120/diff/1014/26 File media/tools/alg_test.cc (right): http://codereview.chromium.org/151120/diff/1014/26#newcode4 Line 4: On 2009/07/02 23:22:35, scherkus wrote: > comment at top of file explaining what this application does Done. http://codereview.chromium.org/151120/diff/1014/26#newcode5 Line 5: #define _CRT_SECURE_NO_WARNINGS 1 On 2009/07/02 23:22:35, scherkus wrote: > we shouldn't have to use this here > > you can either switch to use base/file_util.h (not that bad) or change the gyp > file to declare this for you when building for windows Now using base/file_util.h for OpenFile() and CloseFile(). http://codereview.chromium.org/151120/diff/1014/26#newcode16 Line 16: #include "media/filters/audio_renderer_algorithm_ola.h" On 2009/07/02 23:33:38, fbarchard wrote: > this tool has an obscure name. I'd suggest something audio sounding, like > wav_bender or audio_rate How's wav_ola_test? http://codereview.chromium.org/151120/diff/1014/26#newcode18 Line 18: typedef media::DataBuffer DataBuffer; On 2009/07/02 23:22:35, scherkus wrote: > you can say > > using media::DataBuffer > using media::AudioRendererAlgorithmOLA; Done. http://codereview.chromium.org/151120/diff/1014/26#newcode37 Line 37: AudioRendererAlgorithmOLA ola; On 2009/07/02 23:22:35, scherkus wrote: > globals! bad!! > > declare these inside main and pass them as references to Dummy() via the > constructor Sorry! Done. http://codereview.chromium.org/151120/diff/1014/26#newcode40 Line 40: class Dummy { On 2009/07/02 23:22:35, scherkus wrote: > comment explaining what this class does Done. http://codereview.chromium.org/151120/diff/1014/26#newcode49 Line 49: }; On 2009/07/02 23:22:35, scherkus wrote: > DISALLOW_COPY_AND_ASSIGN(Dummy) Done. http://codereview.chromium.org/151120/diff/1014/26#newcode92 Line 92: Dummy guy; On 2009/07/02 23:22:35, scherkus wrote: > quick comment here Done. http://codereview.chromium.org/151120/diff/1014/26#newcode114 Line 114: size_t bytes_written = 0; On 2009/07/02 23:22:35, scherkus wrote: > quick comment here for this block of code Done. http://codereview.chromium.org/151120/diff/1014/26#newcode118 Line 118: while ((bytes = ola.FillBuffer(b.get())) > (kDefaultWindowSize * 0.5f)) { On 2009/07/02 23:22:35, scherkus wrote: > I forget.. why are we only interested in receiving at least half of what we > requested? > > why not check for zero? Done. http://codereview.chromium.org/151120/diff/1014/26#newcode121 Line 121: return 1; On 2009/07/02 23:33:38, fbarchard wrote: > cleanup on exit instead? (close files) > Done. http://codereview.chromium.org/151120/diff/1014/26#newcode125 Line 125: if (fwrite(buf, 1, bytes, output) < 1) { On 2009/07/02 23:22:35, scherkus wrote: > code duplication! > > change your while loop logic to eliminate > > maybe something like: > > while (true) { > fill buffer > if (should write) write out > if (should break) break > } > Fixed differently. Why exit on write failure? Print the error to the screen and try to continue. Then clean up resources. http://codereview.chromium.org/151120/diff/1014/26#newcode125 Line 125: if (fwrite(buf, 1, bytes, output) < 1) { On 2009/07/02 23:33:38, fbarchard wrote: > change to > if (fwrite(&wav, sizeof(wav), 1, output) != bytes) { Done. http://codereview.chromium.org/151120/diff/1014/26#newcode134 Line 134: wav.subchunk2_size = bytes_written; On 2009/07/02 23:33:38, fbarchard wrote: > this could be filled in at the at the beginning, avoiding the seek, which in > some applications may not be possible, and making the code more robust on errors > such as the fwrite error we saw. If we fill in at the beginning, it will be a guess as to how many bytes we will write instead of a count of how many we successfully wrote. Let me know if this is still an issue with the current version. http://codereview.chromium.org/151120/diff/2001/2003 File media/filters/audio_renderer_algorithm_ola.cc (right): http://codereview.chromium.org/151120/diff/2001/2003#newcode15 Line 15: const size_t kDefaultWindowSize = 4096; On 2009/07/02 23:33:39, fbarchard wrote: > This size is sensitive to frequency and number of channels. > in 6 channel audio, i found it sounded more like pitch shifting than with > stereo. > at 48000 khz, 4096 bytes is 21 milliseconds. You could aim to do 20 > milliseconds worth of audio and compute the number of bytes based on bits per > channel, number of channels and frequency. Thanks. Added a TODO. http://codereview.chromium.org/151120/diff/2001/2003#newcode114 Line 114: if (playback_rate() > 1.0f) { On 2009/07/02 23:33:39, fbarchard wrote: > may want to limit the range, and for other rates, write the correct amount of > silence, so that rate is still respected. > > also note that speed up sounds great, but slow down sounds metalic. Added a TODO for range limitation. Any ideas as to why it sounds metallic? I'm not that well versed in waveform manipulation.
more comments http://codereview.chromium.org/151120/diff/1014/24 File media/filters/audio_renderer_algorithm_ola.cc (right): http://codereview.chromium.org/151120/diff/1014/24#newcode36 Line 36: crossfade_len_ = kDefaultWindowSize/10; On 2009/07/03 01:55:38, kylep wrote: > On 2009/07/02 23:22:35, scherkus wrote: > > I'd prefer to see this be a constant defined alongside kDefaultWindowSize + a > > comment > > > > also spaces around binary operators > > Fixed the spaces. Is it worth it to change the definition now since we know it's > going to have to change to satisfy the requirements for pitch preservation in > higher-channel / higher-bitrate audio? Also, recalculation is going to need to > be done because a channel number might cause it to no longer be aligned. You can leave it for now. http://codereview.chromium.org/151120/diff/1014/24#newcode40 Line 40: output_step_ -= crossfade_len_; On 2009/07/03 01:55:38, kylep wrote: > On 2009/07/02 23:22:35, scherkus wrote: > > any reason why this calculation couldn't be performed inside > > set_playback_rate()? > > > > if we changed the playback rate halfway through, I believe we would no longer > > account for crossfade length > > Interesting. This would mean moving the crossfade_size_ alignment to > set_playback_rate() as well. Is that a problem? I don't think it's a problem. Whenever the playback rate is changed we need to recalculate everything, so might as well centralize the logic http://codereview.chromium.org/151120/diff/1014/24#newcode50 Line 50: if (!saved_buf_) { On 2009/07/03 01:55:38, kylep wrote: > On 2009/07/02 23:22:35, scherkus wrote: > > You may want to move this to CopyInput() and AdvanceInput(), reason being that > > FillBuffer() should not have to touch saved_buf_ (separation of concerns) > > > > on one hand it's duplicated code, but you can have something like > > InitializeInput() that calls this bit of code > > Will there be a case when AdvanceInput() is called before CopyInput()? It seems > to me moving this block to CopyInput() is sufficient. sounds good I'd add a DCHECK with a message saying "Did you call forget to call CopyInput()?" http://codereview.chromium.org/151120/diff/1014/24#newcode63 Line 63: while (dest_length >= output_step_ + crossfade_len_) { On 2009/07/03 01:55:38, kylep wrote: > On 2009/07/02 23:22:35, scherkus wrote: > > this while loop condition seems strange, mostly because |dest_length| is the > > size of the buffer, and output_step_ and crossfade_len_ are constants (I > think) > > so that must mean the length of the buffer is changing? > > > > it would be clearer if the logic was something along the lines of either a > > "dest_remaining > 0" or a "dest_written < target_write_amount" condition. > > dest_length receives the size of the buffer, but really keeps track of how much > more we should copy. Should it be called dest_remaining? And it can't be > 0 > because the code in the block isn't equipped to handle very small requests > (hence ">= output_step_ + crossfade_len_" as (output_step_ + crossfade_len_) is > the smallest chunk size we can handle). > ok http://codereview.chromium.org/151120/diff/1014/24#newcode123 Line 123: AlignToBoundary(&input_step_); On 2009/07/03 01:55:38, kylep wrote: > On 2009/07/02 23:22:35, scherkus wrote: > > I think we'd get more accurate step calculations if we added the (non-aligned) > > step size with the (non-aligned) cross fade size then aligned the sum > > > > only really applies to funky playback rates I suppose > > Yeah, but... everything would be funky when you try to crossfade > |crossfade_size_| bytes onto your data and the crossfade kicks in on the second > half of your third channel of audio, right? Right.. wasn't sure what I was thinking here. The crossfade size is a constant amount. http://codereview.chromium.org/151120/diff/1014/24#newcode138 Line 138: data_offset_ = bytes - saved_buf_remaining; On 2009/07/03 01:55:38, kylep wrote: > On 2009/07/02 23:22:35, scherkus wrote: > > what about a scenario where |bytes| was larger than 2 buffers? > > > > you might have to wrap this inside a while(!IsQueueEmpty()) and keep advancing > > data_offset_ and saved_buf_ until bytes reaches zero > > Should I cross that bridge when I come to it? This all works because the window > size == the buffer sizes. I know this change is necessary in the long run, but > not for this prelim code? You either need to code it or DCHECK for the condition and add a TODO I'm fine with a DCHECK+TODO http://codereview.chromium.org/151120/diff/1014/24#newcode147 Line 147: size_t AudioRendererAlgorithmOLA::CopyLengthToDest(size_t length, On 2009/07/03 01:55:38, kylep wrote: > On 2009/07/02 23:22:35, scherkus wrote: > > length -> size > > > > again, what if it took multiple buffers to satisfy a request? may need a > > while(!IsQueueEmpty()) to copy everything over > > > > > > See above? DCHECK+TODO the case we don't handle http://codereview.chromium.org/151120/diff/1014/23 File media/filters/audio_renderer_algorithm_ola.h (right): http://codereview.chromium.org/151120/diff/1014/23#newcode55 Line 55: bool out_of_input_done_; On 2009/07/03 01:55:38, kylep wrote: > On 2009/07/02 23:22:35, scherkus wrote: > > it seems to me you can infer this information based on whether the queue is > > empty.. am I understanding this variable correctly? > > I believe so... so replace checks on this bool with (saved_buf_ || > !IsQueueEmpty())? Sounds like a function to me :) This would be related to AdvanceInput and CopyInput.. name it something like IsInputFinished() http://codereview.chromium.org/151120/diff/3004/2010 File media/filters/audio_renderer_algorithm_ola.cc (right): http://codereview.chromium.org/151120/diff/3004/2010#newcode139 Line 139: // Calculate number of usable bytes in |saved_buf_|. as mentioned above DCHECK with message here that saved_buf_ was initialized http://codereview.chromium.org/151120/diff/3004/2012 File media/tools/wav_ola_test.cc (right): http://codereview.chromium.org/151120/diff/3004/2012#newcode23 Line 23: using media::AudioRendererAlgorithmOLA; nit: alphabetize these http://codereview.chromium.org/151120/diff/3004/2012#newcode44 Line 44: Dummy(FILE* in, AudioRendererAlgorithmOLA* ola): input_(in), ola_(ola) { style: Dummy(...) : input_(in), ola_(ola) { } http://codereview.chromium.org/151120/diff/3004/2012#newcode84 Line 84: FILE* input = file_util::OpenFile(in_path.c_str(), "rb"); neat trick: combine scoped_ptr_malloc<> with ScopedFILEClose from file_util.h and you get FILE* pointers that auto-close themselves!! scoped_ptr_malloc<FILE, ScopedFILEClose> should do the trick (would recommend looking at that stuff to understand how it works) http://codereview.chromium.org/151120/diff/3004/2012#newcode128 Line 128: scoped_refptr<DataBuffer> b(new DataBuffer()); nit: single-letter variables names are usually reserved for iterators -- in this case "buffer" would be a better choice http://codereview.chromium.org/151120/diff/3004/2012#newcode136 Line 136: LOG(ERROR) << "could not write data after " << bytes_written; scary macro-with-dangling-if! Rule of thumb: never trust macros. Today it might be declared as a single statement that works without curly braces, but what if it gets changed? Or is different for Release mode? Or different platforms? Here's a sorta-realistic example: #define LOG(x) printf("Error: %s", x); exit(1); if (foo) LOG("bar"); ... would evaluate to ... if (foo) printf("Error: %s, "bar"); exit(1); // This gets executed every time. In other words, always add curly braces if your clause involves a macro.
http://codereview.chromium.org/151120/diff/1014/24 File media/filters/audio_renderer_algorithm_ola.cc (right): http://codereview.chromium.org/151120/diff/1014/24#newcode40 Line 40: output_step_ -= crossfade_len_; On 2009/07/06 20:30:39, scherkus wrote: > On 2009/07/03 01:55:38, kylep wrote: > > On 2009/07/02 23:22:35, scherkus wrote: > > > any reason why this calculation couldn't be performed inside > > > set_playback_rate()? > > > > > > if we changed the playback rate halfway through, I believe we would no > longer > > > account for crossfade length > > > > Interesting. This would mean moving the crossfade_size_ alignment to > > set_playback_rate() as well. Is that a problem? > > I don't think it's a problem. Whenever the playback rate is changed we need to > recalculate everything, so might as well centralize the logic Done. Since there is no more logic in Initialize in OLA, I'm removing it. http://codereview.chromium.org/151120/diff/1014/24#newcode50 Line 50: if (!saved_buf_) { On 2009/07/06 20:30:39, scherkus wrote: > On 2009/07/03 01:55:38, kylep wrote: > > On 2009/07/02 23:22:35, scherkus wrote: > > > You may want to move this to CopyInput() and AdvanceInput(), reason being > that > > > FillBuffer() should not have to touch saved_buf_ (separation of concerns) > > > > > > on one hand it's duplicated code, but you can have something like > > > InitializeInput() that calls this bit of code > > > > Will there be a case when AdvanceInput() is called before CopyInput()? It > seems > > to me moving this block to CopyInput() is sufficient. > > sounds good > > I'd add a DCHECK with a message saying "Did you call forget to call > CopyInput()?" Done. http://codereview.chromium.org/151120/diff/1014/24#newcode63 Line 63: while (dest_length >= output_step_ + crossfade_len_) { On 2009/07/06 20:30:39, scherkus wrote: > On 2009/07/03 01:55:38, kylep wrote: > > On 2009/07/02 23:22:35, scherkus wrote: > > > this while loop condition seems strange, mostly because |dest_length| is the > > > size of the buffer, and output_step_ and crossfade_len_ are constants (I > > think) > > > so that must mean the length of the buffer is changing? > > > > > > it would be clearer if the logic was something along the lines of either a > > > "dest_remaining > 0" or a "dest_written < target_write_amount" condition. > > > > dest_length receives the size of the buffer, but really keeps track of how > much > > more we should copy. Should it be called dest_remaining? And it can't be > 0 > > because the code in the block isn't equipped to handle very small requests > > (hence ">= output_step_ + crossfade_len_" as (output_step_ + crossfade_len_) > is > > the smallest chunk size we can handle). > > > > ok Renamed it |dest_remaining|. http://codereview.chromium.org/151120/diff/1014/24#newcode138 Line 138: data_offset_ = bytes - saved_buf_remaining; On 2009/07/06 20:30:39, scherkus wrote: > On 2009/07/03 01:55:38, kylep wrote: > > On 2009/07/02 23:22:35, scherkus wrote: > > > what about a scenario where |bytes| was larger than 2 buffers? > > > > > > you might have to wrap this inside a while(!IsQueueEmpty()) and keep > advancing > > > data_offset_ and saved_buf_ until bytes reaches zero > > > > Should I cross that bridge when I come to it? This all works because the > window > > size == the buffer sizes. I know this change is necessary in the long run, but > > not for this prelim code? > > You either need to code it or DCHECK for the condition and add a TODO > > I'm fine with a DCHECK+TODO Done. http://codereview.chromium.org/151120/diff/1014/24#newcode147 Line 147: size_t AudioRendererAlgorithmOLA::CopyLengthToDest(size_t length, On 2009/07/06 20:30:39, scherkus wrote: > On 2009/07/03 01:55:38, kylep wrote: > > On 2009/07/02 23:22:35, scherkus wrote: > > > length -> size > > > > > > again, what if it took multiple buffers to satisfy a request? may need a > > > while(!IsQueueEmpty()) to copy everything over > > > > > > > > > > See above? > > DCHECK+TODO the case we don't handle Done. DCHECK already existed ~^ http://codereview.chromium.org/151120/diff/1014/23 File media/filters/audio_renderer_algorithm_ola.h (right): http://codereview.chromium.org/151120/diff/1014/23#newcode55 Line 55: bool out_of_input_done_; On 2009/07/06 20:30:39, scherkus wrote: > On 2009/07/03 01:55:38, kylep wrote: > > On 2009/07/02 23:22:35, scherkus wrote: > > > it seems to me you can infer this information based on whether the queue is > > > empty.. am I understanding this variable correctly? > > > > I believe so... so replace checks on this bool with (saved_buf_ || > > !IsQueueEmpty())? > > Sounds like a function to me :) > > This would be related to AdvanceInput and CopyInput.. name it something like > IsInputFinished() Done. http://codereview.chromium.org/151120/diff/3004/2010 File media/filters/audio_renderer_algorithm_ola.cc (right): http://codereview.chromium.org/151120/diff/3004/2010#newcode139 Line 139: // Calculate number of usable bytes in |saved_buf_|. On 2009/07/06 20:30:39, scherkus wrote: > as mentioned above DCHECK with message here that saved_buf_ was initialized Done. http://codereview.chromium.org/151120/diff/3004/2012 File media/tools/wav_ola_test.cc (right): http://codereview.chromium.org/151120/diff/3004/2012#newcode23 Line 23: using media::AudioRendererAlgorithmOLA; On 2009/07/06 20:30:39, scherkus wrote: > nit: alphabetize these Done. http://codereview.chromium.org/151120/diff/3004/2012#newcode44 Line 44: Dummy(FILE* in, AudioRendererAlgorithmOLA* ola): input_(in), ola_(ola) { On 2009/07/06 20:30:39, scherkus wrote: > style: > Dummy(...) > : input_(in), > ola_(ola) { > } Done. http://codereview.chromium.org/151120/diff/3004/2012#newcode84 Line 84: FILE* input = file_util::OpenFile(in_path.c_str(), "rb"); On 2009/07/06 20:30:39, scherkus wrote: > neat trick: > combine scoped_ptr_malloc<> with ScopedFILEClose from file_util.h and you get > FILE* pointers that auto-close themselves!! > > scoped_ptr_malloc<FILE, ScopedFILEClose> should do the trick (would recommend > looking at that stuff to understand how it works) Done. They actually have a typedef for scoped_ptr_malloc<FILE, ScopedFILEClose>. I used that. http://codereview.chromium.org/151120/diff/3004/2012#newcode128 Line 128: scoped_refptr<DataBuffer> b(new DataBuffer()); On 2009/07/06 20:30:39, scherkus wrote: > nit: single-letter variables names are usually reserved for iterators -- in this > case "buffer" would be a better choice Done. http://codereview.chromium.org/151120/diff/3004/2012#newcode136 Line 136: LOG(ERROR) << "could not write data after " << bytes_written; On 2009/07/06 20:30:39, scherkus wrote: > scary macro-with-dangling-if! > > Rule of thumb: never trust macros. Today it might be declared as a single > statement that works without curly braces, but what if it gets changed? Or is > different for Release mode? Or different platforms? > > Here's a sorta-realistic example: > #define LOG(x) printf("Error: %s", x); exit(1); > if (foo) > LOG("bar"); > > ... would evaluate to ... > > if (foo) > printf("Error: %s, "bar"); > exit(1); // This gets executed every time. > > In other words, always add curly braces if your clause involves a macro. Wow thanks for the warning. I'll keep that in mind. Done. http://codereview.chromium.org/151120/diff/3011/3013 File media/filters/audio_renderer_algorithm_ola.cc (right): http://codereview.chromium.org/151120/diff/3011/3013#newcode122 Line 122: I know, Lint, I know. Will be gone on next revision.
really tiny nits http://codereview.chromium.org/151120/diff/3004/2012 File media/tools/wav_ola_test.cc (right): http://codereview.chromium.org/151120/diff/3004/2012#newcode84 Line 84: FILE* input = file_util::OpenFile(in_path.c_str(), "rb"); On 2009/07/06 21:48:22, kylep wrote: > On 2009/07/06 20:30:39, scherkus wrote: > > neat trick: > > combine scoped_ptr_malloc<> with ScopedFILEClose from file_util.h and you get > > FILE* pointers that auto-close themselves!! > > > > scoped_ptr_malloc<FILE, ScopedFILEClose> should do the trick (would recommend > > looking at that stuff to understand how it works) > > Done. They actually have a typedef for scoped_ptr_malloc<FILE, ScopedFILEClose>. > I used that. Cool! http://codereview.chromium.org/151120/diff/2016/3020 File media/filters/audio_renderer_algorithm_ola.cc (right): http://codereview.chromium.org/151120/diff/2016/3020#newcode93 Line 93: saved_buf_ = 0; use NULL instead of 0 http://codereview.chromium.org/151120/diff/2016/3020#newcode139 Line 139: saved_buf_ = 0; use NULL instead of 0
http://codereview.chromium.org/151120/diff/2016/3020 File media/filters/audio_renderer_algorithm_ola.cc (right): http://codereview.chromium.org/151120/diff/2016/3020#newcode93 Line 93: saved_buf_ = 0; On 2009/07/06 22:13:19, scherkus wrote: > use NULL instead of 0 Done. http://codereview.chromium.org/151120/diff/2016/3020#newcode139 Line 139: saved_buf_ = 0; On 2009/07/06 22:13:19, scherkus wrote: > use NULL instead of 0 Done.
LGTM!
Arrrrrrrrg Please fix svn:ignore. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
