Description was changed from
==========
MediaRecorder: support sampling rate adaption in AudioTrackRecorder
** WIP **
BUG=569089
==========
to
==========
MediaRecorder: support sampling rate adaption in AudioTrackRecorder
This CL adds support for audio coming from the MediaStream
Tracks with a sampling rate not directly supported by Opus
({48k, 24k, 16k, 12k, 8k}samples/s) by adding a AudioConverter
in AudioTrackRecorder::AudioEncoder.
A massive cleanup ensues since
- MSTrack audio comes always in 10ms chunks
- Opus is configured for 10ms encoding as well
- buffering, if any, happens inside AudioConverter
BUG=569089
Tested/verified by:
- unit tests, which are beefed up
- content_browsertests
- demo [1]
- bug's [2] with a tiny correction: s/opus/webm/ for
the mimeType to use for capture. I used a variety of
input wav's, mostly the original sfx5.wav in the bug
and the "miscellaneous" entries in [3]
[1] https://webrtc.github.io/samples/src/content/getusermed
ia/record/
[2] https://dl.dropboxusercontent.com/u/15217362/transcode/audiotranscode.html
[3] http://download.wavetlan.com/SVV/Media/HTTP/http-wav.htm
==========

mcasas

Description was changed from ========== MediaRecorder: support sampling rate adaption in AudioTrackRecorder This CL adds ...

Description was changed from
==========
MediaRecorder: support sampling rate adaption in AudioTrackRecorder
This CL adds support for audio coming from the MediaStream
Tracks with a sampling rate not directly supported by Opus
({48k, 24k, 16k, 12k, 8k}samples/s) by adding a AudioConverter
in AudioTrackRecorder::AudioEncoder.
A massive cleanup ensues since
- MSTrack audio comes always in 10ms chunks
- Opus is configured for 10ms encoding as well
- buffering, if any, happens inside AudioConverter
BUG=569089
Tested/verified by:
- unit tests, which are beefed up
- content_browsertests
- demo [1]
- bug's [2] with a tiny correction: s/opus/webm/ for
the mimeType to use for capture. I used a variety of
input wav's, mostly the original sfx5.wav in the bug
and the "miscellaneous" entries in [3]
[1] https://webrtc.github.io/samples/src/content/getusermed
ia/record/
[2] https://dl.dropboxusercontent.com/u/15217362/transcode/audiotranscode.html
[3] http://download.wavetlan.com/SVV/Media/HTTP/http-wav.htm
==========
to
==========
MediaRecorder: support sampling rate adaption in AudioTrackRecorder
This CL adds support for audio coming from the MediaStream
Tracks with a sampling rate not directly supported by Opus
({48k, 24k, 16k, 12k, 8k}samples/s) by adding a AudioConverter
in AudioTrackRecorder::AudioEncoder.
A massive cleanup ensues since
- MSTrack audio comes always in 10ms chunks
- Opus is configured for 10ms encoding as well
- buffering, if any, happens inside AudioConverter
BUG=569089
TEST =
- unit tests, which are beefed up
- content_browsertests
- demo [1]
- bug's [2] with a tiny correction: s/opus/webm/ for
the mimeType to use for capture. I used a variety of
input wav's, mostly the original sfx5.wav in the bug
and the "miscellaneous" entries in [3]
[1] https://webrtc.github.io/samples/src/content/getusermedia/record/
[2] https://dl.dropboxusercontent.com/u/15217362/transcode/audiotranscode.html
[3] http://download.wavetlan.com/SVV/Media/HTTP/http-wav.htm
==========

miu@ PTAL
https://codereview.chromium.org/1579693006/diff/80001/content/renderer/media/...
File content/renderer/media/audio_track_recorder.cc (right):
https://codereview.chromium.org/1579693006/diff/80001/content/renderer/media/...
content/renderer/media/audio_track_recorder.cc:47: static const int
kOpusPreferredFramesPerBuffer = 480;
On 2016/01/22 00:14:53, miu wrote:
> Opus will produce higher quality audio if encoding buffers of longer duration.
> How about 2880 (60 ms)?
Done.
Note that the input will still be chunks of 10ms of audio.
> Also, you may need to increase kOpusMaxDataBytes to
> accommodate this increase (check the libopus code for this, but OTOH, it'd
> probably be something like 4000*6=24000).
Seems to be constant, added doc.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/opus/s...https://codereview.chromium.org/1579693006/diff/80001/content/renderer/media/...
content/renderer/media/audio_track_recorder.cc:81: // |buffer|.
(AudioBus::ToInterleaved() does not support float).
On 2016/01/22 00:14:53, miu wrote:
> It probably should! ;)
>
> This code was originally copy-pasted from media/cast/sender/audio_encoder.cc.
> So, since there are two modules need the same thing, perhaps there should be a
> AudioBus::ToInterleaved() for floats. Consider at least putting a TODO+crbug
> here.
Bug it is. Happy to do it or it can a GoodFirstBug.
https://codereview.chromium.org/1579693006/diff/80001/content/renderer/media/...
content/renderer/media/audio_track_recorder.cc:114: double
ProvideInput(media::AudioBus* audio_bus,
On 2016/01/22 00:14:53, miu wrote:
> This should be private, since it's only meant to be called by the
> internally-owned AudioConverter.
Done.
https://codereview.chromium.org/1579693006/diff/80001/content/renderer/media/...
content/renderer/media/audio_track_recorder.cc:184:
input_params_.set_frames_per_buffer(input_params_.sample_rate() *
On 2016/01/22 00:14:53, miu wrote:
> Shouldn't this be:
>
> input_params_.set_frames_per_buffer(input_params_.sample_rate() *
> kOpusPreferredFramesPerBuffer / kOpusPreferredSamplingRate);
>
> Then, you don't need the "10 ms assumption."
No, input is always 10ms, the input sampling rate varies.
Experimentally (lol) frames_per_buffer() is not set, so
for the purpose of DCHECK() @ OnEncodeAudio(), it's
filled in here.
e.g. 10ms @ 44100 ks/s ==> 441 input frames.
https://codereview.chromium.org/1579693006/diff/80001/content/renderer/media/...
content/renderer/media/audio_track_recorder.cc:199: converter_->AddInput(this);
add here
|converter_->PrimeWithSilence()|
https://codereview.chromium.org/1579693006/diff/80001/content/renderer/media/...
content/renderer/media/audio_track_recorder.cc:207:
output_params_.bits_per_sample() / 8]);
On 2016/01/22 00:14:53, miu wrote:
> bits_per_sample() is erroneous (it really should be removed from AudioParams).
> It doesn't apply for float samples (they are always 32 bits per sample).
>
> Seems like this should be:
>
> new float[output_params_.channels() * output_params_.frames_per_buffer()]
Done.
https://codereview.chromium.org/1579693006/diff/80001/content/renderer/media/...
content/renderer/media/audio_track_recorder.cc:248:
converter_->Convert(audio_bus.get());
On 2016/01/22 00:14:53, miu wrote:
> You can't call convert until you know there are enough frames queued-up in the
> FIFO. Otherwise, zeros will be provided as input to the converter, which will
> cause crackling/gaps in the output audio.
Noted. This is experimented in another unrelated CL
https://crrev.com/1599533003/
thanks for the tip ;)
> Likewise, you should execute this
> Convert, the ToInterleaved, and DoEncode in a loop; until the FIFO no longer
> contains enough frames.
Done.
> You'll have to do the math to reconcile
> frames_per_buffer AND sampling rate differences (or there might be something
in
> AudioConverter to give you this number via a getter method...maybe
> ChunkSize()?).
Since both input and output are multiples of 10ms, the
math is simply an N:1 input-to-output frame count, which
I tried to reflect in constants.
A different problem arises again in the unrelated CL
mentioned before, where input does not come in 10
ms chunks and more calculations are needed for the
buffering.
I don't think ChunkSize() helps here.
>
> Also, you need to account for the time delay introduced by the FIFO buffering,
> and use ConvertWithDelay() here.
IIUC ConvertWithDelay boils down to a provideInput()
call back with a certain delay parameter, but in our
case what we want is a single provideInput() asking
for 6 input chunks, there should be no delay at all.
https://codereview.chromium.org/1579693006/diff/80001/content/renderer/media/...
File content/renderer/media/audio_track_recorder_unittest.cc (right):
https://codereview.chromium.org/1579693006/diff/80001/content/renderer/media/...
content/renderer/media/audio_track_recorder_unittest.cc:83: 22050,
On 2016/01/22 00:14:53, miu wrote:
> Note: Increasing the Opus frame duration (as I mentioned in comments above)
will
> allow you to correctly support 22050 Hz audio, since that would require 20 ms
> frame durations.
Acknowledged.

Description was changed from
==========
MediaRecorder: support sampling rate adaption in AudioTrackRecorder
This CL adds support for audio coming from the MediaStream
Tracks with a sampling rate not directly supported by Opus
({48k, 24k, 16k, 12k, 8k}samples/s) by adding a AudioConverter
in AudioTrackRecorder::AudioEncoder.
A massive cleanup ensues since
- MSTrack audio comes always in 10ms chunks
- Opus is configured for 10ms encoding as well
- buffering, if any, happens inside AudioConverter
BUG=569089
TEST =
- unit tests, which are beefed up
- content_browsertests
- demo [1]
- bug's [2] with a tiny correction: s/opus/webm/ for
the mimeType to use for capture. I used a variety of
input wav's, mostly the original sfx5.wav in the bug
and the "miscellaneous" entries in [3]
[1] https://webrtc.github.io/samples/src/content/getusermedia/record/
[2] https://dl.dropboxusercontent.com/u/15217362/transcode/audiotranscode.html
[3] http://download.wavetlan.com/SVV/Media/HTTP/http-wav.htm
==========
to
==========
MediaRecorder: support sampling rate adaption in AudioTrackRecorder
This CL adds support for audio coming from the MediaStream
Tracks with a sampling rate not directly supported by Opus
({48k, 24k, 16k, 12k, 8k}samples/s) by adding a AudioConverter
in AudioTrackRecorder::AudioEncoder.
A massive cleanup ensues since
- MSTrack audio comes always in 10ms chunks
- Opus is configured for 10ms encoding as well
- buffering, if any, happens inside AudioConverter
BUG=569089
TEST =
- unit tests, which are beefed up
- content_browsertests
- demo [1]
- corrected bug's file [2]
(correction being: s/opus/webm/ for the mimeType to
use for capture.)
I used a variety of input wav's, mostly the original
sfx5.wav in the bug and the "miscellaneous" entries in [3]
[1] https://webrtc.github.io/samples/src/content/getusermedia/record/
[2] https://rawgit.com/Miguelao/demos/master/audiotranscode.html
[3] http://download.wavetlan.com/SVV/Media/HTTP/http-wav.htm
==========

PTAL
https://codereview.chromium.org/1579693006/diff/160001/content/renderer/media...
File content/renderer/media/audio_track_recorder.cc (right):
https://codereview.chromium.org/1579693006/diff/160001/content/renderer/media...
content/renderer/media/audio_track_recorder.cc:54: static const int
kOpusPreferredFramesPerBuffer =
On 2016/01/29 02:37:15, miu wrote:
> nit: No need to use the 'static' keyword inside the anonymous namespace.
Done.
https://codereview.chromium.org/1579693006/diff/160001/content/renderer/media...
content/renderer/media/audio_track_recorder.cc:212: kMaxNumberOfFifoBuffers *
input_params_.frames_per_buffer()));
On 2016/01/29 02:37:15, miu wrote:
> I think the second argument to the ctor here is simply:
>
> 2 * input_params_.frames_per_buffer()
>
> Then, you can delete all of the following constants: kMaxNumberOfFifoBuffers,
> kRatioInputToOutputBuffers, and kMediaStreamTrackBufferDurationMs. This is
what
> I was getting at in the last round of comments. ;)
I'm leaving the constant kMaxNumberOfFifoBuffers for
obvious reasons, the others are gone ;)
https://codereview.chromium.org/1579693006/diff/160001/content/renderer/media...
content/renderer/media/audio_track_recorder.cc:248:
DCHECK_EQ(input_bus->frames(), input_params_.sample_rate() *
On 2016/01/29 02:37:15, miu wrote:
> You don't need this DCHECK(). AudioFifo::Push() will happily accept any
number
> of frames. Overflow can only occur if input_bus->frames() is greater than
> input_params_.frames_per_buffer(), which it should never be, and
> AudioFifo::Push() has its own checks for that.
Done.
https://codereview.chromium.org/1579693006/diff/160001/content/renderer/media...
content/renderer/media/audio_track_recorder.cc:262: // conversion. Luckily here
there is an integerkRatioInputToOutputBuffers:1
On 2016/01/29 02:37:15, miu wrote:
> Please delete the second sentence of this comment (no longer applies to the
> current code).
Done.
https://codereview.chromium.org/1579693006/diff/160001/content/renderer/media...
content/renderer/media/audio_track_recorder.cc:287: if (fifo_->frames() >=
audio_bus->frames())
On 2016/01/29 02:37:15, miu wrote:
> AudioFifo will CHECK that there are sufficient frames when Consume() is
called.
> I think, because of your while-loop expression above (line 264), it would be a
> bug if there ever were not enough frames. So, I think this entire method
should
> probably just be:
>
> fifo_->Consume(...);
> return 1.0;
Done.
https://codereview.chromium.org/1579693006/diff/160001/content/renderer/media...
File content/renderer/media/audio_track_recorder_unittest.cc (right):
https://codereview.chromium.org/1579693006/diff/160001/content/renderer/media...
content/renderer/media/audio_track_recorder_unittest.cc:252: // Send more audio.
Expect the first OnDaata() timestamp.
On 2016/01/29 02:37:15, miu wrote:
> s/OnDaata/OnData/
Done. Actually, comment removed.
https://codereview.chromium.org/1579693006/diff/160001/content/renderer/media...
File content/renderer/media/media_recorder_handler_unittest.cc (right):
https://codereview.chromium.org/1579693006/diff/160001/content/renderer/media...
content/renderer/media/media_recorder_handler_unittest.cc:44: // MediaStream
Audio Track always provides audio in chunks of 10 ms.
On 2016/01/29 02:37:15, miu wrote:
> This is not a global assumption. The current Chrome implementation may do
this,
> but that's not something we want to hard-code everywhere. It's fine if you
want
> to use 10 ms chunks for these tests, but please don't write comments or var
> names to indicate that it is a global assumption.
>
> FYI, this code change will break that assumption:
> https://codereview.chromium.org/1647773002/
>
> ...and there's been a long-standing TODO to fix the code that forces WebAudio
to
> use 10 ms chunks. Chris Jones, the original author and spec editor for
WebAudio
> was always asking Chrome Eng to use minimally-sized buffers (on the order of 2
> ms or less).
Removed the comment. For these tests is fine to use 10ms.
https://codereview.chromium.org/1579693006/diff/160001/content/renderer/media...
content/renderer/media/media_recorder_handler_unittest.cc:66: {false, true,
"video/webm", "vp8", 0, 0, 996, 746}};
On 2016/01/29 02:37:15, miu wrote:
> It's a bit brittle to require the tests pass based on the number of output
bytes
> from the encoder. For instance, if libvpx is updated with some kind of
> improvement that changes the number of bytes, you really don't want these unit
> tests to start failing, right? Instead, I'd suggest just checking for an
empty
> or non-empty result to confirm "the plumbing" is good.
My opinion is that a potential libvpx roller should
have to deal (personally or not) with the
consequences of a roll, including potentially
the breaking these tests, but I understand this is
a judgement call so I'll follow your advice: removed
the whole parameter set in favour of a single
threshold.