Add support for midstream audio configuration changes.

Add support for midstream audio configuration changes.

This introduces a new class, AudioBufferConverter, which takes AudioBuffers of arbitrary formats and converts them to AudioBuffers of a common format.

To support config changes seamlessly we use the AudioBufferConverter to convert incoming AudioBuffers to the hardware output format as we get them from the AudioBufferStream. This way the rest of the audio pipeline doesn't need to handle config changes.

We only enable this conversion step for DemuxerStreams that support config changes (namely ChunkDemuxerStream).

BUG=347270
TEST=AudioBufferConverterTest
NOTRY=true
R=dalecurtis@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260071

[rileya (GONE FROM CHROMIUM), 2014-03-07 01:19:29]

Not ready for full review, I just want some high-level feedback.

I added a new class, AudioBufferConverter, which handles converting incoming
AudioBuffers into the hardware output params. This happens in the
AudioRendererImpl just before they get sent to the AudioSplicer. It tracks how
many output frames it can expect to get from the queued input (computed based on
the ratio of sample rates) and then only calls AudioConverter::Convert when we
know we have enough data queued up to make a full output buffer.

https://codereview.chromium.org/177333003/diff/120001/media/base/audio_buffer...
File media/base/audio_buffer_converter.cc (right):

https://codereview.chromium.org/177333003/diff/120001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:51: while(output_frames_ >=
output_params_.frames_per_buffer() * 2) {
I'm not quite sure here. Since the SincResampler doesn't want to output
everything it has before asking for more input data (presumably because it needs
some overlap with prior frames), I only do the conversion when we have enough
input queued up for at least 2 output frames, which seems arbitrary.

Also see the comment in audio_renderer_impl_unittest, as this buffering causes
some issues there.

https://codereview.chromium.org/177333003/diff/120001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:160: if (frames <
output_params_.frames_per_buffer()) {
This is kinda ugly...

https://codereview.chromium.org/177333003/diff/120001/media/base/audio_buffer...
File media/base/audio_buffer_converter.h (right):

https://codereview.chromium.org/177333003/diff/120001/media/base/audio_buffer...
media/base/audio_buffer_converter.h:23: class AudioBufferConverter : public
AudioConverter::InputCallback {
Not sure on the name...

https://codereview.chromium.org/177333003/diff/120001/media/filters/audio_ren...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/177333003/diff/120001/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:283: // TODO(rileya): Support hardware
config changes
What do we currently do when the hardware config changes?

https://codereview.chromium.org/177333003/diff/120001/media/filters/audio_ren...
File media/filters/audio_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/177333003/diff/120001/media/filters/audio_ren...
media/filters/audio_renderer_impl_unittest.cc:334:
SatisfyPendingRead(frames_remaining_in_buffer() * 2);
Hack to make the tests pass... this needs some work.

These tests expect delivering a buffer of the size of the
AudioRendererAlgorithm's queue will fill that buffer (and complete Prerolling).
However the AudioConverter ends up holding onto some of that data, since even if
we have enough for a full output frame, we may not output one until we get some
more data (since SincResampler wants some amount of overlap).

https://codereview.chromium.org/177333003/diff/120001/media/filters/ffmpeg_au...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/177333003/diff/120001/media/filters/ffmpeg_au...
media/filters/ffmpeg_audio_decoder.cc:377: if (av_frame_->sample_rate !=
config_.samples_per_second() ||
jk on the TODO, we still shouldn't expect to see changes here...

[DaleCurtis, 2014-03-07 02:00:09]

We should also figure out some way to disable this for non-MSE paths as
resampling every input is expensive.

https://codereview.chromium.org/177333003/diff/120001/media/base/audio_buffer...
File media/base/audio_buffer_converter.cc (right):

https://codereview.chromium.org/177333003/diff/120001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:40: if
(RequiresConverterReset(buffer_params))
It'd be more readable to use names related to "ConfigChange" for the sake of
clarity.

https://codereview.chromium.org/177333003/diff/120001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:51: while(output_frames_ >=
output_params_.frames_per_buffer() * 2) {
On 2014/03/07 01:19:29, rileya wrote:
> I'm not quite sure here. Since the SincResampler doesn't want to output
> everything it has before asking for more input data (presumably because it
needs
> some overlap with prior frames), I only do the conversion when we have enough
> input queued up for at least 2 output frames, which seems arbitrary.
> 
> Also see the comment in audio_renderer_impl_unittest, as this buffering causes
> some issues there.

As discussed offline, I think instead you want to size the output buffers to be
the maximum you can get out of conversion pass without needing to pad with
silence.  Doing so will be more efficient and reduce the complexity a fair bit.

https://codereview.chromium.org/177333003/diff/120001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:133: scoped_refptr<AudioBuffer>
output_buffer =
You should avoid conversion if possible.

https://codereview.chromium.org/177333003/diff/120001/media/filters/audio_ren...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/177333003/diff/120001/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:283: // TODO(rileya): Support hardware
config changes
On 2014/03/07 01:19:29, rileya wrote:
> What do we currently do when the hardware config changes?

Nothing currently. It'll get resampled / remixed again on the output side which
stinks.  See http://crbug.com/157216

https://codereview.chromium.org/177333003/diff/120001/media/filters/ffmpeg_au...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/177333003/diff/120001/media/filters/ffmpeg_au...
media/filters/ffmpeg_audio_decoder.cc:378: channels !=
ChannelLayoutToChannelCount(config_.channel_layout()) ||
Store channels_ ?  No need to recompute every packet.

[rileya (GONE FROM CHROMIUM), 2014-03-20 00:55:11]

I gave this some more attention, PTAL. Still needs unit test tweaks (and
probably new unit tests...) and other polish, so I mainly want high-level
feedback.

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
File media/base/audio_buffer_converter.cc (right):

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:53: if (timestamp_helper_.base_timestamp()
== kNoTimestamp()) {
I'm not very familiar with how timestamps are handled, is this an acceptable way
of setting the base timestamp?

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:132: sample_rate_ratio_ =
static_cast<double>(output_params_.sample_rate()) /
I just realized this is only used in one place and we still have all the info
necessary to compute it there -- I'll remove this.

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:146: 32,  // This is a dummy value.
bits_per_channel is unused by the AudioConverter. The alternative here is adding
a sample_format() getter to AudioBuffer.

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:163: if (!request_frames)
General style question: is "== 0" preferable in cases like this?

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:175: // AudioBus of that size across the
|output_buffer|.
Since AudioConverter now supports arbitrarily sized requests, we could just make
one request, but we'd run into issues with its temp buffers being reallocated a
lot to match our request sizes. Unless calling Convert() multiple times is a
significant performance concern (I don't think it is), I think this works fine.

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
File media/base/audio_buffer_converter.h (right):

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.h:74: int offset_into_queue_;
This probably needs a better name.

https://codereview.chromium.org/177333003/diff/140001/media/filters/audio_ren...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/177333003/diff/140001/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:175: // TODO: add a Reset() or Flush()
This is an immediate TODO for this CL that I forgot to address, I'll do so in
the next patchset.

[DaleCurtis, 2014-03-20 01:36:26]

Partial review!

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
File media/base/audio_buffer_converter.cc (right):

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:43: AudioParameters buffer_params =
AudioBufferToAudioParameters(buffer);
I'm loathe to create a new AudioParameters object for every AddInput(). I'd
change IsConfigChange() to work on buffer and drop AudioBufferToAudioParamaters
method since it's only done here.

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:53: if (timestamp_helper_.base_timestamp()
== kNoTimestamp()) {
On 2014/03/20 00:55:11, rileya wrote:
> I'm not very familiar with how timestamps are handled, is this an acceptable
way
> of setting the base timestamp?

Yeah, this is fine, this is what we do in the AudioSplicer.

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:60: scoped_refptr<AudioBuffer>
output_buffer = Convert();
Every Convert() call seems to do the same thing, so perhaps move the append of
output_buffers into Convert() ? Rename it ConvertIfPossible() or something like
that?

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:85: int frames_to_read =
requested_frames_left;
I think the logic below can be simplified a bit if you switch to:

int frames_to_read = std::min(input_buffer->frames() - offset_into_queue_,
requested_frames_left);

Then you should only need one ReadFrames() call.

(I haven't thought it all the way through though)

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:113: input_frames_ -= (audio_bus->frames()
- requested_frames_left);
You'll want to Zero out any unfilled frames. See ZeroFramesPartial().

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:129: if (!IsConfigChange(output_params_))
Hmm, maybe make this a bool that's passed in?

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
File media/base/audio_buffer_converter.h (right):

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.h:24: public:
1 space indent, no line after.  I'd just run "git cl format" and it should fix
all this...

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.h:26: AudioBufferConverter(const
AudioParameters& output_params);
explicit

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.h:34: private:
ditto on style

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.h:77: std::list<scoped_refptr<AudioBuffer> >
queued_outputs_;
deque?

[DaleCurtis, 2014-03-20 19:29:00]

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
File media/base/audio_buffer_converter.cc (right):

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:116: // Assume full volume (is this
correct?)
Yes.  Volume is applied at the output device.  If we applied it here then it
gets stored for rendering at that volume, even though the volume might change
later.

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:140: AudioParameters
AudioBufferConverter::AudioBufferToAudioParameters(
This can be a static .cc only method. Prefer that when possible.

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:154: int chunks = input_frames_ /
audio_converter_->RequestSize();
ChunkSize?

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:177: 
No extra line.

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:178: int frames_this_iteration =
output_params_.frames_per_buffer();
use std::min() to simplify this + conditional?

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:186: scoped_ptr<AudioBus> output_bus =
Create this outside of the while() loop and just set the channel pointers/frame
count inside.

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:200: 
You need to now trim off silence from the output using AudioBuffer::TrimEnd(). 
See the AudioSplicer's AccurateTrimEnd() for reference too.

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_conver...
File media/base/audio_converter.h (right):

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_conver...
media/base/audio_converter.h:93: // How many frames will be requested via a
single ProvideInput() call.
You should be able to calculate this within your converter. Just don't set
disable_fifo == true.  It shouldn't be a performance issue since you won't
create a converter just for buffer size mismatches.

https://codereview.chromium.org/177333003/diff/140001/media/filters/audio_ren...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/177333003/diff/140001/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:373: bool need_another_buffer = false;
Move with while below?

https://codereview.chromium.org/177333003/diff/140001/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:401: 
No extra line.

[rileya (GONE FROM CHROMIUM), 2014-03-21 20:58:48]

Alright, I think I covered most everything.

We probably want to disable this entirely for the non-MSE case, for performance
reasons, correct? Any thoughts on how to cleanly tell the AudioRendererImpl
about this?

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
File media/base/audio_buffer_converter.cc (right):

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:43: AudioParameters buffer_params =
AudioBufferToAudioParameters(buffer);
On 2014/03/20 01:36:26, DaleCurtis wrote:
> I'm loathe to create a new AudioParameters object for every AddInput(). I'd
> change IsConfigChange() to work on buffer and drop
AudioBufferToAudioParamaters
> method since it's only done here.

Good point, I removed this and instead check directly against the buffer for
config changes.

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:60: scoped_refptr<AudioBuffer>
output_buffer = Convert();
On 2014/03/20 01:36:26, DaleCurtis wrote:
> Every Convert() call seems to do the same thing, so perhaps move the append of
> output_buffers into Convert() ? Rename it ConvertIfPossible() or something
like
> that?

Sounds good. Done.

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:85: int frames_to_read =
requested_frames_left;
On 2014/03/20 01:36:26, DaleCurtis wrote:
> I think the logic below can be simplified a bit if you switch to:
> 
> int frames_to_read = std::min(input_buffer->frames() - offset_into_queue_,
> requested_frames_left);
> 
> Then you should only need one ReadFrames() call.
> 
> (I haven't thought it all the way through though)

Sounds good. A conditional is still necessary for popping off a buffer when it's
exhausted, but I simplified the rest a fair bit.

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:113: input_frames_ -= (audio_bus->frames()
- requested_frames_left);
On 2014/03/20 01:36:26, DaleCurtis wrote:
> You'll want to Zero out any unfilled frames. See ZeroFramesPartial().

Done.

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:140: AudioParameters
AudioBufferConverter::AudioBufferToAudioParameters(
On 2014/03/20 19:29:01, DaleCurtis wrote:
> This can be a static .cc only method. Prefer that when possible.

Removed entirely. (IsConfigChange is now static though, and compares an
AudioParameters to an AudioBuffer).

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:154: int chunks = input_frames_ /
audio_converter_->RequestSize();
On 2014/03/20 19:29:01, DaleCurtis wrote:
> ChunkSize?

ChunkSize is in output frames, this is to determine how many ProvideInput()
calls worth of input frames we have. I did kill RequestSize though, so this can
just be input_params_.frames_per_buffer() now.

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:177: 
On 2014/03/20 19:29:01, DaleCurtis wrote:
> No extra line.

Fixed.

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:186: scoped_ptr<AudioBus> output_bus =
On 2014/03/20 19:29:01, DaleCurtis wrote:
> Create this outside of the while() loop and just set the channel
pointers/frame
> count inside.

Done.

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:200: 
On 2014/03/20 19:29:01, DaleCurtis wrote:
> You need to now trim off silence from the output using AudioBuffer::TrimEnd().

> See the AudioSplicer's AccurateTrimEnd() for reference too.

Is this actually necessary here? The |output_buffer| should be sized to exactly
as many non-silence frames as we're getting out of the converter (give or take
one due to rounding when we flush), so I don't think there will be any silence
to trim?

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
File media/base/audio_buffer_converter.h (right):

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.h:24: public:
On 2014/03/20 01:36:26, DaleCurtis wrote:
> 1 space indent, no line after.  I'd just run "git cl format" and it should fix
> all this...

Fixed.

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.h:26: AudioBufferConverter(const
AudioParameters& output_params);
On 2014/03/20 01:36:26, DaleCurtis wrote:
> explicit

Done.

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.h:34: private:
On 2014/03/20 01:36:26, DaleCurtis wrote:
> ditto on style

Fixed.

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.h:77: std::list<scoped_refptr<AudioBuffer> >
queued_outputs_;
On 2014/03/20 01:36:26, DaleCurtis wrote:
> deque?

Sounds good. Done.

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_conver...
File media/base/audio_converter.h (right):

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_conver...
media/base/audio_converter.h:93: // How many frames will be requested via a
single ProvideInput() call.
On 2014/03/20 19:29:01, DaleCurtis wrote:
> You should be able to calculate this within your converter. Just don't set
> disable_fifo == true.  It shouldn't be a performance issue since you won't
> create a converter just for buffer size mismatches.

Good point, I'd previously disabled the fifo, but not doing so makes things
simpler... Removed this.

https://codereview.chromium.org/177333003/diff/140001/media/filters/audio_ren...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/177333003/diff/140001/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:373: bool need_another_buffer = false;
On 2014/03/20 19:29:01, DaleCurtis wrote:
> Move with while below?

Done.

https://codereview.chromium.org/177333003/diff/140001/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:401: 
On 2014/03/20 19:29:01, DaleCurtis wrote:
> No extra line.

Done.

[DaleCurtis, 2014-03-21 23:18:56]

Looks great, just needs a unit test for audio_buffer_converter now.

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
File media/base/audio_buffer_converter.cc (right):

https://codereview.chromium.org/177333003/diff/140001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:200: 
On 2014/03/21 20:58:49, rileya wrote:
> On 2014/03/20 19:29:01, DaleCurtis wrote:
> > You need to now trim off silence from the output using
AudioBuffer::TrimEnd().
> 
> > See the AudioSplicer's AccurateTrimEnd() for reference too.
> 
> Is this actually necessary here? The |output_buffer| should be sized to
exactly
> as many non-silence frames as we're getting out of the converter (give or take
> one due to rounding when we flush), so I don't think there will be any silence
> to trim?

Hmm, yeah you're right, this should be correct.

https://codereview.chromium.org/177333003/diff/160001/media/base/audio_buffer...
File media/base/audio_buffer_converter.cc (right):

https://codereview.chromium.org/177333003/diff/160001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:82: audio_converter_.reset(NULL);
shouldn't need NULL.

https://codereview.chromium.org/177333003/diff/160001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:123: requested_frames_left);
alignment is off and multiline if's require {}

https://codereview.chromium.org/177333003/diff/160001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:150: std::max(buffer->frame_count(),
Probably this should round to the nearest multiple of kKernelSize, but is
probably fine.

https://codereview.chromium.org/177333003/diff/160001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:153: sample_rate_ratio_ =
static_cast<double>(input_params_.sample_rate()) /
alignment is off. git cl format is your friend :)

https://codereview.chromium.org/177333003/diff/160001/media/base/audio_buffer...
File media/base/audio_buffer_converter.h (right):

https://codereview.chromium.org/177333003/diff/160001/media/base/audio_buffer...
media/base/audio_buffer_converter.h:80: double sample_rate_ratio_;
io_sample_rate_ratio_ then?

[rileya (GONE FROM CHROMIUM), 2014-03-24 18:07:04]

Added unit tests and addressed comments.

For performance reasons we probably want to switch this off for the non-MSE
case, it doesn't look like AudioRendererImpl is currently aware of whether we're
using MSE; perhaps adding a DemuxerStream::SupportsConfigChanges() could signal
whether we want to use this?

https://codereview.chromium.org/177333003/diff/160001/media/base/audio_buffer...
File media/base/audio_buffer_converter.cc (right):

https://codereview.chromium.org/177333003/diff/160001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:82: audio_converter_.reset(NULL);
On 2014/03/21 23:18:56, DaleCurtis wrote:
> shouldn't need NULL.

Done.

https://codereview.chromium.org/177333003/diff/160001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:123: requested_frames_left);
On 2014/03/21 23:18:56, DaleCurtis wrote:
> alignment is off and multiline if's require {}

Done.

https://codereview.chromium.org/177333003/diff/160001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:153: sample_rate_ratio_ =
static_cast<double>(input_params_.sample_rate()) /
On 2014/03/21 23:18:56, DaleCurtis wrote:
> alignment is off. git cl format is your friend :)

Fixed.

https://codereview.chromium.org/177333003/diff/160001/media/base/audio_buffer...
File media/base/audio_buffer_converter.h (right):

https://codereview.chromium.org/177333003/diff/160001/media/base/audio_buffer...
media/base/audio_buffer_converter.h:80: double sample_rate_ratio_;
On 2014/03/21 23:18:56, DaleCurtis wrote:
> io_sample_rate_ratio_ then?

Done.

[DaleCurtis, 2014-03-24 18:29:33]

Yeah, I think the DemuxerStream idea sounds great. Want to write that up in a
separate patch set, then use it in this one?

https://codereview.chromium.org/177333003/diff/220001/media/base/audio_buffer...
File media/base/audio_buffer_converter_unittest.cc (right):

https://codereview.chromium.org/177333003/diff/220001/media/base/audio_buffer...
media/base/audio_buffer_converter_unittest.cc:23: channel_layout,
Alignment is off.

https://codereview.chromium.org/177333003/diff/220001/media/base/audio_buffer...
media/base/audio_buffer_converter_unittest.cc:83: 
No test for ABC::Reset() ?

https://codereview.chromium.org/177333003/diff/220001/media/base/audio_buffer...
media/base/audio_buffer_converter_unittest.cc:113:
AddInput(MakeTestBuffer(48000, CHANNEL_LAYOUT_STEREO, 100));
make 48000 a constant?

[rileya (GONE FROM CHROMIUM), 2014-03-24 21:27:18]

Added a couple tests and used DecoderStream::SupportsConfigChanges to switch
this on/off.

https://codereview.chromium.org/177333003/diff/220001/media/base/audio_buffer...
File media/base/audio_buffer_converter_unittest.cc (right):

https://codereview.chromium.org/177333003/diff/220001/media/base/audio_buffer...
media/base/audio_buffer_converter_unittest.cc:23: channel_layout,
On 2014/03/24 18:29:33, DaleCurtis wrote:
> Alignment is off. 

Fixed.

https://codereview.chromium.org/177333003/diff/220001/media/base/audio_buffer...
media/base/audio_buffer_converter_unittest.cc:83: 
On 2014/03/24 18:29:33, DaleCurtis wrote:
> No test for ABC::Reset() ?

Added a couple.

https://codereview.chromium.org/177333003/diff/220001/media/base/audio_buffer...
media/base/audio_buffer_converter_unittest.cc:113:
AddInput(MakeTestBuffer(48000, CHANNEL_LAYOUT_STEREO, 100));
On 2014/03/24 18:29:33, DaleCurtis wrote:
> make 48000 a constant?

Sounds good, done.

[DaleCurtis, 2014-03-24 23:04:52]

https://codereview.chromium.org/177333003/diff/240001/media/filters/source_bu...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/177333003/diff/240001/media/filters/source_bu...
media/filters/source_buffer_stream.cc:1338: /*if
(audio_configs_[0].samples_per_second() != config.samples_per_second()) {
Stay / go?

[rileya (GONE FROM CHROMIUM), 2014-03-24 23:09:42]

https://codereview.chromium.org/177333003/diff/240001/media/filters/source_bu...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/177333003/diff/240001/media/filters/source_bu...
media/filters/source_buffer_stream.cc:1338: /*if
(audio_configs_[0].samples_per_second() != config.samples_per_second()) {
On 2014/03/24 23:04:53, DaleCurtis wrote:
> Stay / go?

Whoops, missed this, removed.

[rileya (GONE FROM CHROMIUM), 2014-03-26 03:22:13]

Latest patchset uses the method we discussed offline. It simplifies the
AudioBufferConverter greatly.

However, I found that if I flush the AudioConverter on each call, sound quality
is bad (there isn't overlap for the resampler, so we get pops at buffer
boundaries). If I don't flush the AudioConverter, then the silence frames we pad
in in ProvideInput remain and will be consumed in the next Convert() call
(instead of the start of the next input buffer). It's worth noting that it
seemed to sound OK without flushing the AudioConverter, so I could be missing
something, but it doesn't seem like it should be correct.

As satisfying as it was to simplify this down to a single Convert() method, I'm
thinking I'll need to keep the input/output queues (and associated logic that
ensures we never pad silence to the AudioConverter input callback) from before
and just always output buffers that mirror the scaled sizes +
timestamps/durations of the inputs (I'll probably need to keep a queue of size +
timestamp/duration for the outputs to use). We should discuss tomorrow.

[rileya (GONE FROM CHROMIUM), 2014-03-26 15:51:55]

On second thought it may be possible to flush the AudioConverter after each
buffer, and largely maintain the simplified version of the code. We could hold
onto the two previous buffers, such that when we get a new buffer we'll have 3;
then we can convert the middle of the 3 and pad in just enough of the
previous/next buffers to make the resample seamless (SincResampler's kernel size
I'd guess?). This requires some extra conversion each step (to advance the
AudioConverter past the previous buffer's frames we padded in), but avoids the
potential precision nightmare of trying to map all of the data coming out of the
AudioConverter to match the original input buffers (I can't help but think that
over time we'd drift due to rounding and being off by a frame on some buffer
boundaries).

[rileya (GONE FROM CHROMIUM), 2014-03-26 22:56:47]

Explanation of PS16:

In order to reuse timestamp/duration from our input buffers we want a one-to-one
relationship between input and output buffers (albeit with different frame
counts if we're resampling). Just taking a single AudioBuffer in isolation and
running the AudioConverter on it produces poor resampling results; there's no
overlap between adjacent buffers, so we get artifacts on the buffer boundaries.

At any given point |current_buffer_| is the input buffer we will next convert. I
also maintain 2 AudioBufferQueues |previous_frames_| and |next_frames_| which
hold buffers before and after |current_buffer_|.

Each call to ConvertIfPossible() will convert |current_buffer_| if possible, and
add up to kBoundaryPaddingFrames* frames from |previous_frames_| and
|next_frames_| on the beginning and end of it. This gives us seamless
resampling, provided we have enough frames in the 2 AudioBufferQueues.

Except when we hit a config change or EOS I wait until we have at least
kBoundaryPaddingFrames in |next_frames_| before calling ConvertIfPossible(). I
also take care to ensure that |previous_frames_|'s read position is always
seeked to the correct place.

* kBoundaryPaddingFrames (probably could use a better name) is equal to the
resampler's kernel size.

[rileya (GONE FROM CHROMIUM), 2014-03-27 05:10:48]

Okay PS19 is back to the approach from before. I added a callback for config
changes to DecoderStream, and reset the timestamp state on the
AudioBufferConverter upon such changes. The TimestampOffset tests that were
failing before now pass (as do all other media_unittests) and we get good sound
quality.

[DaleCurtis, 2014-03-27 06:00:44]

Looking great!  I'll stamp after solving the lifetime of the input frame
calculation.

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_buffer...
File media/base/audio_buffer_converter.cc (right):

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:7: #include <cstdlib>
?

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:42: 
extra line

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:106: input_buffer->frame_count() -
offset_into_queue_);
s/offset_into_queue_/last_input_buffer_offset_/ ?

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:130: input_frames_ -= (audio_bus->frames()
- requested_frames_left);
no extra parens.

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:142: audio_converter_.reset(NULL);
no NULL necessary.

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:153: std::max(buffer->frame_count(),
Does quality seem better if you always use kDefaultRequestSize here? I wonder if
alternating buffer sizes might make this weird.

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:177: request_frames =
When flushing it might be easier to always extract fixed ChunkSize() buffers and
repeat until you run out of input frames.  You should be able to avoid tracking
buffered_input_frames_ over the lifetime of the conversion then. You only need
to track the last number of input frames handed in.

It's fine to always return multiple buffers instead of one giant one too if that
simplifies the code below.

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:230:
queued_outputs_.push_back(output_buffer);
Seems like there could only be one of these with your current algorithm?

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_buffer...
File media/base/audio_buffer_converter.h (right):

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_buffer...
media/base/audio_buffer_converter.h:68: std::list<scoped_refptr<AudioBuffer> >
queued_inputs_;
deque maybe make a BufferQueue typedef above for both inputs and outputs.

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_conver...
File media/base/audio_converter.h (right):

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_conver...
media/base/audio_converter.h:93: // How many frames will be requested via a
single ProvideInput() call.
Seems unused?

https://codereview.chromium.org/177333003/diff/420001/media/filters/audio_ren...
File media/filters/audio_renderer_impl.h (right):

https://codereview.chromium.org/177333003/diff/420001/media/filters/audio_ren...
media/filters/audio_renderer_impl.h:207: const AudioHardwareConfig&
hardware_config_;
storing const& is unusual in CHrome land. I'd check the style guide :)

[rileya (GONE FROM CHROMIUM), 2014-03-27 17:51:40]

Addressed most everything.

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_buffer...
File media/base/audio_buffer_converter.cc (right):

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:7: #include <cstdlib>
On 2014/03/27 06:00:45, DaleCurtis wrote:
> ?

Whoops, removed.

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:42: 
On 2014/03/27 06:00:45, DaleCurtis wrote:
> extra line

Removed.

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:106: input_buffer->frame_count() -
offset_into_queue_);
On 2014/03/27 06:00:45, DaleCurtis wrote:
> s/offset_into_queue_/last_input_buffer_offset_/ ?

Sounds good, done.

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:130: input_frames_ -= (audio_bus->frames()
- requested_frames_left);
On 2014/03/27 06:00:45, DaleCurtis wrote:
> no extra parens.

Removed.

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:142: audio_converter_.reset(NULL);
On 2014/03/27 06:00:45, DaleCurtis wrote:
> no NULL necessary.

Done.

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:153: std::max(buffer->frame_count(),
On 2014/03/27 06:00:45, DaleCurtis wrote:
> Does quality seem better if you always use kDefaultRequestSize here? I wonder
if
> alternating buffer sizes might make this weird.

I'm not sure... I think that'd depend on whether the SincResampler works better
with multiple of its kernel size as the request size?

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:177: request_frames =
On 2014/03/27 06:00:45, DaleCurtis wrote:
> When flushing it might be easier to always extract fixed ChunkSize() buffers
and
> repeat until you run out of input frames.  You should be able to avoid
tracking
> buffered_input_frames_ over the lifetime of the conversion then. You only need
> to track the last number of input frames handed in.
> 
> It's fine to always return multiple buffers instead of one giant one too if
that
> simplifies the code below.

But if we only track the last number of input frames I think we might miss some
extra frames that are leftover from prior conversions in the resampler.

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:230:
queued_outputs_.push_back(output_buffer);
On 2014/03/27 06:00:45, DaleCurtis wrote:
> Seems like there could only be one of these with your current algorithm?

Almost always, but in the case where we get a buffer that is a config change, we
will flush, likely producing an output buffer, reset the converter, then
potentially convert the new buffer, queuing a second buffer.

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_buffer...
File media/base/audio_buffer_converter.h (right):

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_buffer...
media/base/audio_buffer_converter.h:68: std::list<scoped_refptr<AudioBuffer> >
queued_inputs_;
On 2014/03/27 06:00:45, DaleCurtis wrote:
> deque maybe make a BufferQueue typedef above for both inputs and outputs.

Done.

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_conver...
File media/base/audio_converter.h (right):

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_conver...
media/base/audio_converter.h:93: // How many frames will be requested via a
single ProvideInput() call.
On 2014/03/27 06:00:45, DaleCurtis wrote:
> Seems unused?

Yup, removed.

https://codereview.chromium.org/177333003/diff/420001/media/filters/audio_ren...
File media/filters/audio_renderer_impl.h (right):

https://codereview.chromium.org/177333003/diff/420001/media/filters/audio_ren...
media/filters/audio_renderer_impl.h:207: const AudioHardwareConfig&
hardware_config_;
On 2014/03/27 06:00:45, DaleCurtis wrote:
> storing const& is unusual in CHrome land. I'd check the style guide :)

Changed to a pointer.

[DaleCurtis, 2014-03-27 18:15:22]

Pending the input buffer experiment we discussed offline, this lgtm.

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_buffer...
File media/base/audio_buffer_converter.cc (right):

https://codereview.chromium.org/177333003/diff/420001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:153: std::max(buffer->frame_count(),
On 2014/03/27 17:51:40, rileya wrote:
> On 2014/03/27 06:00:45, DaleCurtis wrote:
> > Does quality seem better if you always use kDefaultRequestSize here? I
wonder
> if
> > alternating buffer sizes might make this weird.
> 
> I'm not sure... I think that'd depend on whether the SincResampler works
better
> with multiple of its kernel size as the request size?

Shouldn't matter, the regions are calculated optimally based on whatever value
you give.

https://codereview.chromium.org/177333003/diff/450001/media/base/audio_buffer...
File media/base/audio_buffer_converter.cc (right):

https://codereview.chromium.org/177333003/diff/450001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:7: #include <list>
No need for this either now?

https://codereview.chromium.org/177333003/diff/450001/media/filters/audio_ren...
File media/filters/audio_renderer_impl.h (right):

https://codereview.chromium.org/177333003/diff/450001/media/filters/audio_ren...
media/filters/audio_renderer_impl.h:207: AudioHardwareConfig* hardware_config_;
const* const?

[DaleCurtis, 2014-03-27 18:17:26]

(needs a more detailed description and TEST= line though)

[rileya (GONE FROM CHROMIUM), 2014-03-27 19:07:11]

Addressed comments, updated description and added a TEST=

I made a unit test (not committing it because it took ~2mins to run...) which
converted ~7hrs of 8KHz data and didn't see any drift issues -- I got exactly as
many output frames as I'd expect given the total input frames and the io ratio.

https://codereview.chromium.org/177333003/diff/450001/media/base/audio_buffer...
File media/base/audio_buffer_converter.cc (right):

https://codereview.chromium.org/177333003/diff/450001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:7: #include <list>
On 2014/03/27 18:15:23, DaleCurtis wrote:
> No need for this either now?

Removed.

https://codereview.chromium.org/177333003/diff/450001/media/filters/audio_ren...
File media/filters/audio_renderer_impl.h (right):

https://codereview.chromium.org/177333003/diff/450001/media/filters/audio_ren...
media/filters/audio_renderer_impl.h:207: AudioHardwareConfig* hardware_config_;
On 2014/03/27 18:15:23, DaleCurtis wrote:
> const* const?

Sounds good, done.

[DaleCurtis, 2014-03-27 20:35:07]

https://codereview.chromium.org/177333003/diff/470001/media/base/audio_buffer...
File media/base/audio_buffer_converter.h (right):

https://codereview.chromium.org/177333003/diff/470001/media/base/audio_buffer...
media/base/audio_buffer_converter.h:25: class AudioBufferConverter : public
AudioConverter::InputCallback {
need MEDIA_EXPORT here.

[rileya (GONE FROM CHROMIUM), 2014-03-27 20:57:31]

On 2014/03/27 20:35:07, DaleCurtis wrote:
>
https://codereview.chromium.org/177333003/diff/470001/media/base/audio_buffer...
> File media/base/audio_buffer_converter.h (right):
> 
>
https://codereview.chromium.org/177333003/diff/470001/media/base/audio_buffer...
> media/base/audio_buffer_converter.h:25: class AudioBufferConverter : public
> AudioConverter::InputCallback {
> need MEDIA_EXPORT here.

Good catch! Added.

[DaleCurtis, 2014-03-27 20:59:54]

https://codereview.chromium.org/177333003/diff/470001/media/filters/audio_ren...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/177333003/diff/470001/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:751:
buffer_converter_->ResetTimestampState();
Need to check expecting_config_changes here.

[rileya (GONE FROM CHROMIUM), 2014-03-27 21:02:35]

Added a DCHECK.

[DaleCurtis, 2014-03-27 22:49:26]

https://codereview.chromium.org/177333003/diff/530001/media/base/audio_buffer...
File media/base/audio_buffer_converter.cc (right):

https://codereview.chromium.org/177333003/diff/530001/media/base/audio_buffer...
media/base/audio_buffer_converter.cc:161: new AudioConverter(input_params_,
output_params_, false));
As discussed offline, flip disable_fifo to true now.

[rileya (GONE FROM CHROMIUM), 2014-03-27 23:02:59]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-03-27 23:05:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rileya@chromium.org/177333003/500026

[commit-bot: I haz the power, 2014-03-28 02:51:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-28 02:51:48]

Retried try job too often on ios_dbg_simulator for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[rileya (GONE FROM CHROMIUM), 2014-03-28 02:53:19]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-03-28 02:54:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rileya@chromium.org/177333003/500026

[commit-bot: I haz the power, 2014-03-28 03:27:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-28 03:27:39]

Retried try job too often on ios_dbg_simulator for step(s) crypto_unittests,
url_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[rileya (GONE FROM CHROMIUM), 2014-03-28 03:36:59]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-03-28 03:38:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rileya@chromium.org/177333003/500026

[rileya1, 2014-03-28 04:10:27]

The CQ bit was unchecked by rileya@google.com

[rileya1, 2014-03-28 04:10:28]

The CQ bit was checked by rileya@google.com

[DaleCurtis, 2014-03-28 04:20:30]

Committed patchset #26 manually as r260071 (presubmit successful).