Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2557513002:
[Chromecast] Add support for different playback rates to ALSA backend (Closed)
Description[Chromecast] Add support for different playback rates to ALSA backend
BUG= internal b/27450476
TEST= cast_multizone_backend_unittests
Committed: https://crrev.com/5d51839919eb8bb4ec7e82f6e01de6df4bc7919f
Cr-Commit-Position: refs/heads/master@{#438866}
Patch Set 1 #
Total comments: 56
Patch Set 2 : Add comments etc #
Total comments: 4
Patch Set 3 : use CreateEmptyBuffer #Messages
Total messages: 21 (7 generated)
kmackay@chromium.org changed reviewers: + halliwell@chromium.org, slan@chromium.org
https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc (right): https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:348: if (got_eos_) { I don't think this branch can be hit? DCHECK(!got_eos_) at top of function https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/audio_decoder_alsa.h (right): https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.h:92: bool pending_write_pcm_; Seems like this is mostly redundant (corresponds to pending_output_frames_ being non-zero, modulo setting that for eos case)? https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.h:101: std::unique_ptr<::media::AudioRendererAlgorithm> rate_shifter_; nit, should be including <memory>
https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc (right): https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:68: ::media::AudioBus::Create(2, kDefaultFramesPerBuffer)), nit: Can we do "2 /* num_channels */" or kNumChannels? Here and below https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:77: rate_shifter_info_.push_back(RateShifterInfo(1.0f)); Why do we need this call? The queue is cleared when rate_shifter_ is created. https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:149: if (std::abs(rate - 1.0) < kPlaybackRateEpsilon) { Why this check? Are we worried about apps setting the playback rate to 0.99999 or something? https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:239: if (pending_buffer_complete_ && !rate_shifter_->IsQueueFull()) { Why do we need to check the rate_shifter queue here? https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:265: void AudioDecoderAlsa::CreateRateShifter(int samples_per_second) { Do we want to DCHECK that rate_shifter_ is flushed? https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:290: for (const RateShifterInfo& info : rate_shifter_info_) { This is dense. Could you put a simple comment inside the if statement? i.e.: // Calculate the delay based on the enqueued rate shift info. Then perhaps one below the for loop: // Account for the pending output frames. It's also slightly odd to me that we are storing a member called |last_known_delay_| whose delay information is not necessarily correct at any given time. What about just storing the timestamp? https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:326: if (error_) { Can we rename this variable to mixer_error_? This block is hard to understand otherwise. https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:338: int input_frames = decoded->data_size() / (2 * sizeof(float)); nit: kNumChannels https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:348: if (got_eos_) { On 2016/12/06 17:27:59, halliwell wrote: > I don't think this branch can be hit? DCHECK(!got_eos_) at top of function got_eos_ can mutate above (line 336) meganit, though: Can we put this block under the WritePcm call below? https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:356: const uint8_t* channels[2] = { Add comment: // Otherwise, if the rate is not 1.0, or rate-shifted frames have yet to // to be pushed, enqueue the data into |rate_shifter_|. https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:366: if (decoded->end_of_stream() || (!rate_shifter_->IsQueueFull() && Why not check got_eos_? https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:383: // rate-shifted data. Perhaps make this an anonymous function that takes |samples_per_second| to cut down on the bulk of this method. https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:396: RateShifterInfo* rate_info = &rate_shifter_info_.front(); It seems to be an implicit assumption everywhere that rate_shifter_info_ is always non-empty. Calling front() and back() is undefined behavior. Perhaps DCHECK(!rate_shifter_info_.empty()) before? https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:415: desired_output_frames, config_.samples_per_second * kMaxOutputMs / 1000); base::kMillisecondsPerSecond https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:418: rate_shifter_output_ = ::media::AudioBus::Create(2, desired_output_frames); Is seems inefficient for rate_shifter_ouptut_ to be reallocated everytime, particularly if the return statement at line 424 hits. Why not allocate some large buffer and just overwrite? https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:430: int channel_data_size = out_frames * sizeof(float); Do we have a more conventional way of going from AudioBus to DecoderBaseBuffer? Seems like something we would already be doing. A utility perhaps? https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:456: if (extra_frames > 0) { When this condition hits, what has happened? Shouldn't these frames be played out? https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:476: void AudioDecoderAlsa::OnWritePcmCompletion(BufferStatus status, Is it OK to ignore |status| here? https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:499: double rate = rate_shifter_info_.front().rate; nit: Move inside if (pending_buffer_complete_), and add another "if" https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/audio_decoder_alsa.h (right): https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.h:60: RateShifterInfo(float playback_rate); nit: explicit Also, can we do a default ctor here? https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.h:92: bool pending_write_pcm_; On 2016/12/06 17:27:59, halliwell wrote: > Seems like this is mostly redundant (corresponds to pending_output_frames_ being > non-zero, modulo setting that for eos case)? Yes, after grepping for this value, it does seem that you could check pending_output_frames_ != 0 instead. https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/stream_mixer_alsa_input_impl.cc (right): https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/stream_mixer_alsa_input_impl.cc:241: if (!being_skipped_) { Suggestion: I think that an integer |consecutive_frames_skipped_| would be a little bit more readable here.
One high-level comment: I think in general commenting blocks of code would be helpful. This might be personal preference, but to me, it makes reading the code a much more palatable task. I left a few examples; feel free to take as much or as little as desired.
https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc (right): https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:68: ::media::AudioBus::Create(2, kDefaultFramesPerBuffer)), On 2016/12/07 00:22:27, slan wrote: > nit: Can we do "2 /* num_channels */" or kNumChannels? Here and below Done. https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:77: rate_shifter_info_.push_back(RateShifterInfo(1.0f)); On 2016/12/07 00:22:27, slan wrote: > Why do we need this call? The queue is cleared when rate_shifter_ is created. We don't; removed https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:149: if (std::abs(rate - 1.0) < kPlaybackRateEpsilon) { On 2016/12/07 00:22:27, slan wrote: > Why this check? Are we worried about apps setting the playback rate to 0.99999 > or something? AudioRendererAlgorithm treats values close to 1 as exactly 1. I did this so that the rendering delay stays semi-accurate in those cases. https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:239: if (pending_buffer_complete_ && !rate_shifter_->IsQueueFull()) { On 2016/12/07 00:22:27, slan wrote: > Why do we need to check the rate_shifter queue here? We need to have flow control, so the app doesn't push an infinite amount of data in as fast as possible. https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:265: void AudioDecoderAlsa::CreateRateShifter(int samples_per_second) { On 2016/12/07 00:22:27, slan wrote: > Do we want to DCHECK that rate_shifter_ is flushed? No; it might not be in some cases (eg, sample rate change mid-stream). https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:290: for (const RateShifterInfo& info : rate_shifter_info_) { On 2016/12/07 00:22:27, slan wrote: > This is dense. Could you put a simple comment inside the if statement? i.e.: > > // Calculate the delay based on the enqueued rate shift info. > > Then perhaps one below the for loop: > > // Account for the pending output frames. > > It's also slightly odd to me that we are storing a member called > |last_known_delay_| whose delay information is not necessarily correct at any > given time. What about just storing the timestamp? Added comments. The last_known_delay_ is the last known delay from the mixer, so we need to update it for the queued data in here. https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:326: if (error_) { On 2016/12/07 00:22:27, slan wrote: > Can we rename this variable to mixer_error_? This block is hard to understand > otherwise. Done. https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:338: int input_frames = decoded->data_size() / (2 * sizeof(float)); On 2016/12/07 00:22:27, slan wrote: > nit: kNumChannels Done. https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:348: if (got_eos_) { On 2016/12/07 00:22:27, slan wrote: > On 2016/12/06 17:27:59, halliwell wrote: > > I don't think this branch can be hit? DCHECK(!got_eos_) at top of function > got_eos_ can mutate above (line 336) > > meganit, though: Can we put this block under the WritePcm call below? I prefer to set the state variables before calling out into a function that could conceivably call back into this class synchronously. https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:356: const uint8_t* channels[2] = { On 2016/12/07 00:22:27, slan wrote: > Add comment: > > // Otherwise, if the rate is not 1.0, or rate-shifted frames have yet to > // to be pushed, enqueue the data into |rate_shifter_|. Done. https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:366: if (decoded->end_of_stream() || (!rate_shifter_->IsQueueFull() && On 2016/12/07 00:22:27, slan wrote: > Why not check got_eos_? Added comment. https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:383: // rate-shifted data. On 2016/12/07 00:22:27, slan wrote: > Perhaps make this an anonymous function that takes |samples_per_second| to cut > down on the bulk of this method. Done. https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:396: RateShifterInfo* rate_info = &rate_shifter_info_.front(); On 2016/12/07 00:22:27, slan wrote: > It seems to be an implicit assumption everywhere that rate_shifter_info_ is > always non-empty. Calling front() and back() is undefined behavior. Perhaps > DCHECK(!rate_shifter_info_.empty()) before? Done. https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:415: desired_output_frames, config_.samples_per_second * kMaxOutputMs / 1000); On 2016/12/07 00:22:27, slan wrote: > base::kMillisecondsPerSecond Added constant. https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:418: rate_shifter_output_ = ::media::AudioBus::Create(2, desired_output_frames); On 2016/12/07 00:22:27, slan wrote: > Is seems inefficient for rate_shifter_ouptut_ to be reallocated everytime, > particularly if the return statement at line 424 hits. Why not allocate some > large buffer and just overwrite? It already does that? It only reallocates if it needs more space. https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:430: int channel_data_size = out_frames * sizeof(float); On 2016/12/07 00:22:27, slan wrote: > Do we have a more conventional way of going from AudioBus to DecoderBaseBuffer? > Seems like something we would already be doing. A utility perhaps? No, we don't. Usually we convert from ::media::DecoderBuffer to DecoderBufferBase, but in this case I have to use AudioBus. https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:456: if (extra_frames > 0) { On 2016/12/07 00:22:27, slan wrote: > When this condition hits, what has happened? Shouldn't these frames be played > out? They already were. I added more comments. https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:476: void AudioDecoderAlsa::OnWritePcmCompletion(BufferStatus status, On 2016/12/07 00:22:27, slan wrote: > Is it OK to ignore |status| here? Added DCHECK https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:499: double rate = rate_shifter_info_.front().rate; On 2016/12/07 00:22:27, slan wrote: > nit: Move inside if (pending_buffer_complete_), and add another "if" Done. https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/audio_decoder_alsa.h (right): https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.h:60: RateShifterInfo(float playback_rate); On 2016/12/07 00:22:27, slan wrote: > nit: explicit > > Also, can we do a default ctor here? Added explicit; I don't think a default constructor makes sense here though. https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.h:92: bool pending_write_pcm_; On 2016/12/07 00:22:27, slan wrote: > On 2016/12/06 17:27:59, halliwell wrote: > > Seems like this is mostly redundant (corresponds to pending_output_frames_ > being > > non-zero, modulo setting that for eos case)? > > Yes, after grepping for this value, it does seem that you could check > pending_output_frames_ != 0 instead. input_frames can be 0 sometimes. I added a (negative) constant to check for instead. https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.h:101: std::unique_ptr<::media::AudioRendererAlgorithm> rate_shifter_; On 2016/12/06 17:27:59, halliwell wrote: > nit, should be including <memory> Done. https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/stream_mixer_alsa_input_impl.cc (right): https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/stream_mixer_alsa_input_impl.cc:241: if (!being_skipped_) { On 2016/12/07 00:22:27, slan wrote: > Suggestion: I think that an integer |consecutive_frames_skipped_| would be a > little bit more readable here. Changed name to is_underflowing_
lgtm https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc (right): https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:239: if (pending_buffer_complete_ && !rate_shifter_->IsQueueFull()) { On 2016/12/07 22:59:57, kmackay wrote: > On 2016/12/07 00:22:27, slan wrote: > > Why do we need to check the rate_shifter queue here? > > We need to have flow control, so the app doesn't push an infinite amount of data > in as fast as possible. Acknowledged. https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:265: void AudioDecoderAlsa::CreateRateShifter(int samples_per_second) { On 2016/12/07 22:59:58, kmackay wrote: > On 2016/12/07 00:22:27, slan wrote: > > Do we want to DCHECK that rate_shifter_ is flushed? > > No; it might not be in some cases (eg, sample rate change mid-stream). In that case, wouldn't we want to play out the audio still buffered in |rate_shifter_|? https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:290: for (const RateShifterInfo& info : rate_shifter_info_) { On 2016/12/07 22:59:58, kmackay wrote: > On 2016/12/07 00:22:27, slan wrote: > > This is dense. Could you put a simple comment inside the if statement? i.e.: > > > > // Calculate the delay based on the enqueued rate shift info. > > > > Then perhaps one below the for loop: > > > > // Account for the pending output frames. > > > > It's also slightly odd to me that we are storing a member called > > |last_known_delay_| whose delay information is not necessarily correct at any > > given time. What about just storing the timestamp? > > Added comments. The last_known_delay_ is the last known delay from the mixer, so > we need to update it for the queued data in here. OK, I get it now. https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:348: if (got_eos_) { On 2016/12/07 22:59:57, kmackay wrote: > On 2016/12/07 00:22:27, slan wrote: > > On 2016/12/06 17:27:59, halliwell wrote: > > > I don't think this branch can be hit? DCHECK(!got_eos_) at top of function > > got_eos_ can mutate above (line 336) > > > > meganit, though: Can we put this block under the WritePcm call below? > > I prefer to set the state variables before calling out into a function that > could conceivably call back into this class synchronously. Yes, I suppose that's fair... https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:418: rate_shifter_output_ = ::media::AudioBus::Create(2, desired_output_frames); On 2016/12/07 22:59:57, kmackay wrote: > On 2016/12/07 00:22:27, slan wrote: > > Is seems inefficient for rate_shifter_ouptut_ to be reallocated everytime, > > particularly if the return statement at line 424 hits. Why not allocate some > > large buffer and just overwrite? > > It already does that? It only reallocates if it needs more space. Yuuuupppp, lgtm https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:430: int channel_data_size = out_frames * sizeof(float); On 2016/12/07 22:59:57, kmackay wrote: > On 2016/12/07 00:22:27, slan wrote: > > Do we have a more conventional way of going from AudioBus to > DecoderBaseBuffer? > > Seems like something we would already be doing. A utility perhaps? > > No, we don't. Usually we convert from ::media::DecoderBuffer to > DecoderBufferBase, but in this case I have to use AudioBus. Acknowledged. https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/audio_decoder_alsa.h (right): https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.h:60: RateShifterInfo(float playback_rate); On 2016/12/07 22:59:58, kmackay wrote: > On 2016/12/07 00:22:27, slan wrote: > > nit: explicit > > > > Also, can we do a default ctor here? > > Added explicit; I don't think a default constructor makes sense here though. Yeah, I'm silly.
https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc (right): https://codereview.chromium.org/2557513002/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:265: void AudioDecoderAlsa::CreateRateShifter(int samples_per_second) { On 2016/12/09 00:05:31, slan wrote: > On 2016/12/07 22:59:58, kmackay wrote: > > On 2016/12/07 00:22:27, slan wrote: > > > Do we want to DCHECK that rate_shifter_ is flushed? > > > > No; it might not be in some cases (eg, sample rate change mid-stream). > > In that case, wouldn't we want to play out the audio still buffered in > |rate_shifter_|? Ideally yes, but we're already resetting the mixer input for the new sample rate, so that queued data is lost. We don't really have any signal to tell the mixer input to switch sample rates without destroying itself, so this is the best we can do right now. In practice I don't think the sample rate will change mid-stream.
kmackay@chromium.org changed reviewers: + dalecurtis@chromium.org
+dalecurtis for DEPS approval
This is getting quite messy. You're ending up with the architecture that <audio> had in Chrome 1.0 at a very rapid rate; i.e. a combined decoder, scheduler, renderer, stream mixer, and output driver. This is very problematic from a coupling and maintenance perspective. I recommend you replace AudioDecoderAlsa with media::AudioRendererImpl and simplify your ALSA output into a media::AudioRendererSink interface; potentially wrapping the sink interface in An AudioRendererMixer primitive if you need it. You'll end up with a lot less code to maintain and since it's part of the daily web media experience it'll see more test coverage. It's also not clear to me that the way you're using the AudioRendererAlgorithm here is appropriate, WSOLA especially needs a long contiguous buffer (~200ms) to work most effectively; whereas you seem to be trying to apply playback rates at a per-buffer level instead of just at time of render. I realize I harp on this class everytime a review comes across my desk and this isn't my code, so I'm not going to block review and the DEPS is fine. DEPS lgtm https://codereview.chromium.org/2557513002/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc (right): https://codereview.chromium.org/2557513002/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:57: scoped_refptr<::media::AudioBuffer> CreateSilenceBuffer(int sample_rate) { Just use AudioBuffer::CreateEmptyBuffer() it will create a silent buffer w/ zero size and N frames. https://codereview.chromium.org/2557513002/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:161: bool AudioDecoderAlsa::SetPlaybackRate(float rate) { Hmmm, are you using this to actually change playback rates or is this for varispeed playback? The AudioRendererAlgorithm uses WSOLA which isn't appropriate for this task (compute heavy and may dramatically change the signal), so for simple varispeed at rates near ~1.0 you probably just want MultiChannelResampler and to change the io ratio. https://cs.chromium.org/chromium/src/media/base/audio_shifter.h Does this pretty much.
https://codereview.chromium.org/2557513002/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc (right): https://codereview.chromium.org/2557513002/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:161: bool AudioDecoderAlsa::SetPlaybackRate(float rate) { On 2016/12/13 20:13:35, DaleCurtis wrote: > Hmmm, are you using this to actually change playback rates or is this for > varispeed playback? The AudioRendererAlgorithm uses WSOLA which isn't > appropriate for this task (compute heavy and may dramatically change the > signal), so for simple varispeed at rates near ~1.0 you probably just want > MultiChannelResampler and to change the io ratio. > > https://cs.chromium.org/chromium/src/media/base/audio_shifter.h > > Does this pretty much. We want to allow playback at eg 2x normal rate or 0.5x normal rate without pitch shifting.
https://codereview.chromium.org/2557513002/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc (right): https://codereview.chromium.org/2557513002/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:57: scoped_refptr<::media::AudioBuffer> CreateSilenceBuffer(int sample_rate) { On 2016/12/13 20:13:35, DaleCurtis_OOO_Dec14_Jan6 wrote: > Just use AudioBuffer::CreateEmptyBuffer() it will create a silent buffer w/ zero > size and N frames. Done.
The CQ bit was checked by kmackay@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from slan@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2557513002/#ps40001 (title: "use CreateEmptyBuffer")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481823000933100,
"parent_rev": "88c86dd50e4fcbf8dc609ca2fde235fc243d7481", "commit_rev":
"743eae8469a9af62193a5cd3271d8f0142fa9830"}
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Add support for different playback rates to ALSA backend BUG= internal b/27450476 TEST= cast_multizone_backend_unittests ========== to ========== [Chromecast] Add support for different playback rates to ALSA backend BUG= internal b/27450476 TEST= cast_multizone_backend_unittests Review-Url: https://codereview.chromium.org/2557513002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Add support for different playback rates to ALSA backend BUG= internal b/27450476 TEST= cast_multizone_backend_unittests Review-Url: https://codereview.chromium.org/2557513002 ========== to ========== [Chromecast] Add support for different playback rates to ALSA backend BUG= internal b/27450476 TEST= cast_multizone_backend_unittests Committed: https://crrev.com/5d51839919eb8bb4ec7e82f6e01de6df4bc7919f Cr-Commit-Position: refs/heads/master@{#438866} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5d51839919eb8bb4ec7e82f6e01de6df4bc7919f Cr-Commit-Position: refs/heads/master@{#438866} |
