Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1579693006:
MediaRecorder: support sampling rate adaption in AudioTrackRecorder (Closed)
|
Created:
4 years, 11 months ago by mcasas Modified:
4 years, 10 months ago Reviewers:
miu CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, cwilso Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMediaRecorder: 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.
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
Committed: https://crrev.com/acc25ac1e26010c501fd98d59738e95e22a7015c
Cr-Commit-Position: refs/heads/master@{#372491}
Patch Set 1 #
Total comments: 15
Patch Set 2 : miu@s comments, rebased (bits per second stuff) #
Total comments: 15
Patch Set 3 : miu@s second round of comments #
Total comments: 16
Patch Set 4 : miu@s third round of comments #
Total comments: 2
Patch Set 5 : miu@s nit #
Messages
Total messages: 29 (17 generated)
Description was changed from ========== MediaRecorder: support sampling rate adpation in AudioTrackRecorder ** WIP ** BUG=569089 ========== to ========== MediaRecorder: support sampling rate adaption in AudioTrackRecorder ** WIP ** BUG=569089 ==========
Patchset #1 (id:1) has been deleted
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
==========
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
==========
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
mcasas@chromium.org changed reviewers: + miu@chromium.org
miu@ PTAL disclaimer: first time doing Audio stuff, expect dragons! :) (I'm particularly uncertain about timestamping)
Patchset #1 (id:60001) has been deleted
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; Opus will produce higher quality audio if encoding buffers of longer duration. How about 2880 (60 ms)? 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). 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). 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. https://codereview.chromium.org/1579693006/diff/80001/content/renderer/media/... content/renderer/media/audio_track_recorder.cc:114: double ProvideInput(media::AudioBus* audio_bus, This should be private, since it's only meant to be called by the internally-owned AudioConverter. 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() * 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." https://codereview.chromium.org/1579693006/diff/80001/content/renderer/media/... content/renderer/media/audio_track_recorder.cc:207: output_params_.bits_per_sample() / 8]); 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()] https://codereview.chromium.org/1579693006/diff/80001/content/renderer/media/... content/renderer/media/audio_track_recorder.cc:248: converter_->Convert(audio_bus.get()); 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. Likewise, you should execute this Convert, the ToInterleaved, and DoEncode in a loop; until the FIFO no longer contains enough frames. 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()?). Also, you need to account for the time delay introduced by the FIFO buffering, and use ConvertWithDelay() here. 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, 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.
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.
Patchset #2 (id:100001) has been deleted
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
==========
https://codereview.chromium.org/1579693006/diff/120001/content/renderer/media... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1579693006/diff/120001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:34: static const int kOpusMaxDataBytes = 4000; Consider making these all enum constants, per recent discussion on chromium-dev@ (from about 2 months ago). enum : int { kOpusMaxDataBytes = 4000, ... }; It's also the standard way of doing things in Chromium media code (e.g., media/base/limits.h). https://codereview.chromium.org/1579693006/diff/120001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:52: (kOpusPreferredSamplingRate / 1000) * kOpusPreferredBufferDurationMs; nit (to avoid rounding error if kOpusPreferredSamplingRate is ever changed): kOpusPreferredSamplingRate * kOpusPreferredBufferDurationMs / base::Time::kMillisecondsPerSecond https://codereview.chromium.org/1579693006/diff/120001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:137: // 48ksamples/s and 480, respectively. Output is 2880 frames per buffer. https://codereview.chromium.org/1579693006/diff/120001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:177: if (input_params_.Equals(input_params)) This will always be false, since you call input_params_.set_frames_per_buffer() below. It seems that you're mixing up the AudioParameters of the audio being pushed via AudioEncoder::OnData() with the AudioParameters of the audio being pushed into |converter_|. https://codereview.chromium.org/1579693006/diff/120001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:187: input_params_.set_frames_per_buffer(input_params_.sample_rate() * These are the AudioParameters for the |converter_| input. Since you're creating AudioConverter without allowing it to use its internal FIFO, it will only attempt to pull-in 10 ms of audio (when it calls ProvideInput()) and there will be 50 ms gaps of silence in the output AudioBus from every Convert() call. You want to set 60 ms worth of frames_per_buffer here. Now, once again, I'll claim you don't need kMediaStreamTrackBufferDurationMs. Just let your |fifo_| fill with whatever-sized AudioBuses are provided. ;-) https://codereview.chromium.org/1579693006/diff/120001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:193: media::GuessChannelLayout(std::min(input_params_.channels(), 2)), Opus supports from 1 to 255 channels. Why restrict that? ;) https://codereview.chromium.org/1579693006/diff/120001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:252: fifo_->Push(input_bus.release()); Memory leak: Don't release the scoped_ptr here. Your call but, instead of using media::AudioFifo, consider just keeping a queue (std::deque<scoped_ptr<AudioBus>>) in this class. This would avoid extra data copies. https://codereview.chromium.org/1579693006/diff/120001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:268: capture_time_of_first_buffer_); After this, you need to increment capture_time_of_first_buffer_ by kOpusPreferredFramesPerBuffer ms. Also, instead of keeping a capture_time_of_first_buffer_ member, which might become invalid or drift (e.g., if EncodeAudio() is not called due to skipping ahead), consider always doing the math based on the |capture_time| argument: base::TimeTicks capture_time_of_first_sample = capture_time - base::TimeDelta::FromMicroseconds(fifo_->frames() * base::Time::kMicrosecondsPerSecond / input_params_.sample_rate());
Patchset #3 (id:140001) has been deleted
miu@ PTAL https://codereview.chromium.org/1579693006/diff/120001/content/renderer/media... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1579693006/diff/120001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:34: static const int kOpusMaxDataBytes = 4000; On 2016/01/27 01:52:58, miu wrote: > Consider making these all enum constants, per recent discussion on chromium-dev@ > (from about 2 months ago). > > enum : int { > kOpusMaxDataBytes = 4000, > ... > }; > > It's also the standard way of doing things in Chromium media code (e.g., > media/base/limits.h). Done for those that are not calculations. https://codereview.chromium.org/1579693006/diff/120001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:52: (kOpusPreferredSamplingRate / 1000) * kOpusPreferredBufferDurationMs; On 2016/01/27 01:52:58, miu wrote: > nit (to avoid rounding error if kOpusPreferredSamplingRate is ever changed): > > kOpusPreferredSamplingRate * kOpusPreferredBufferDurationMs / > base::Time::kMillisecondsPerSecond Done. https://codereview.chromium.org/1579693006/diff/120001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:137: // 48ksamples/s and 480, respectively. On 2016/01/27 01:52:58, miu wrote: > Output is 2880 frames per buffer. Done. https://codereview.chromium.org/1579693006/diff/120001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:187: input_params_.set_frames_per_buffer(input_params_.sample_rate() * On 2016/01/27 01:52:58, miu wrote: > These are the AudioParameters for the |converter_| input. Since you're creating > AudioConverter without allowing it to use its internal FIFO, it will only > attempt to pull-in 10 ms of audio (when it calls ProvideInput()) and there will > be 50 ms gaps of silence in the output AudioBus from every Convert() call. > > You want to set 60 ms worth of frames_per_buffer here. > > Now, once again, I'll claim you don't need kMediaStreamTrackBufferDurationMs. > Just let your |fifo_| fill with whatever-sized AudioBuses are provided. ;-) Done. I still need kMediaStreamTrackBufferDurationMs for other calculations (e.g. kOpusPreferredFramesPerBuffer). Note: I am using the internal FIFO via setting |disable_fifo| = false in l.201l ; configuring input params as 10ms, output params for 60ms. https://codereview.chromium.org/1579693006/diff/120001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:193: media::GuessChannelLayout(std::min(input_params_.channels(), 2)), On 2016/01/27 01:52:59, miu wrote: > Opus supports from 1 to 255 channels. Why restrict that? ;) Maybe Opus does, but our third_party doesn't :( Added a note. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/opus/s... https://codereview.chromium.org/1579693006/diff/120001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:252: fifo_->Push(input_bus.release()); On 2016/01/27 01:52:59, miu wrote: > Memory leak: Don't release the scoped_ptr here. > Done > Your call but, instead of using media::AudioFifo, consider just keeping a queue > (std::deque<scoped_ptr<AudioBus>>) in this class. This would avoid extra data > copies. Added a TODO(), let me know if it's ok or you want that to happen now (will add a bug otherwise). https://codereview.chromium.org/1579693006/diff/120001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:268: capture_time_of_first_buffer_); On 2016/01/27 01:52:58, miu wrote: > After this, you need to increment capture_time_of_first_buffer_ by > kOpusPreferredFramesPerBuffer ms. > > Also, instead of keeping a capture_time_of_first_buffer_ member, which might > become invalid or drift (e.g., if EncodeAudio() is not called due to skipping > ahead), consider always doing the math based on the |capture_time| argument: > > base::TimeTicks capture_time_of_first_sample = capture_time - > base::TimeDelta::FromMicroseconds(fifo_->frames() * > base::Time::kMicrosecondsPerSecond / input_params_.sample_rate()); Done.
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 = nit: No need to use the 'static' keyword inside the anonymous namespace. https://codereview.chromium.org/1579693006/diff/160001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:212: kMaxNumberOfFifoBuffers * input_params_.frames_per_buffer())); 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. ;) 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() * 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. 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 Please delete the second sentence of this comment (no longer applies to the current code). https://codereview.chromium.org/1579693006/diff/160001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:287: if (fifo_->frames() >= audio_bus->frames()) 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; 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. s/OnDaata/OnData/ 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. 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). 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}}; 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.
Patchset #4 (id:180001) has been deleted
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.
lgtm https://codereview.chromium.org/1579693006/diff/200001/content/renderer/media... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1579693006/diff/200001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:52: const size_t kMaxNumberOfFifoBuffers = 2; nit: Now this can be an enum.
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]
- 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
==========
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.
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
==========
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org Link to the patchset: https://codereview.chromium.org/1579693006/#ps220001 (title: "miu@s nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1579693006/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1579693006/220001
Message was sent while issue was closed.
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.
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
==========
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.
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
==========
Message was sent while issue was closed.
Committed patchset #5 (id:220001)
Message was sent while issue was closed.
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.
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
==========
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.
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
Committed: https://crrev.com/acc25ac1e26010c501fd98d59738e95e22a7015c
Cr-Commit-Position: refs/heads/master@{#372491}
==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/acc25ac1e26010c501fd98d59738e95e22a7015c Cr-Commit-Position: refs/heads/master@{#372491}
Message was sent while issue was closed.
publishing old comments https://codereview.chromium.org/1579693006/diff/200001/content/renderer/media... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1579693006/diff/200001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:52: const size_t kMaxNumberOfFifoBuffers = 2; On 2016/01/29 21:37:20, miu wrote: > nit: Now this can be an enum. Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
