Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1406113002:
Add AudioTrackRecorder for audio component of MediaStream recording. (Closed)
DescriptionAdd AudioTrackRecorder for audio component of MediaStream recording.
This is the first of ~three CLs for adding the audio component of
MediaStreamRecording. These CLs will be:
1) Add AudioTrackRecorder, a MediaStreamAudioSink which will be owned by MediaRecorderHandler
2) Update WebmMuxer to mux Opus output of AudioTrackRecorder
3) Update MediaRecorderHandler to use AudioTrackRecorder
BUG=528519
Committed: https://crrev.com/9b6d878b661f617cdbfc027a8c9b9b444aab948c
Cr-Commit-Position: refs/heads/master@{#360108}
Patch Set 1 : #
Total comments: 33
Patch Set 2 : address mcasas' comments #Patch Set 3 : trybots #
Total comments: 44
Patch Set 4 : address comments #
Total comments: 20
Patch Set 5 : mcasas' comments #Patch Set 6 : forward declaration #
Total comments: 17
Patch Set 7 : address comments, trybots #
Total comments: 12
Patch Set 8 : address comments #
Total comments: 8
Patch Set 9 : miu's comments #
Total comments: 12
Patch Set 10 : comments #
Total comments: 30
Patch Set 11 : minyue@'s comments #
Total comments: 8
Patch Set 12 : minyue@'s comments #
Total comments: 6
Patch Set 13 : comments #
Total comments: 10
Patch Set 14 : comments and format #Patch Set 15 : rebase #Patch Set 16 : fix asan #
Total comments: 2
Patch Set 17 : miu's comment #
Messages
Total messages: 74 (32 generated)
ajose@chromium.org changed reviewers: + mcasas@chromium.org
PTAL at just the AudioTrackRecorder files, will remove others from this CL ASAP.
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Add AudioTrackRecorder for audio component of MediaStream recording. This is the first of three CLs for adding the audio component of MediaStreamRecording. BUG=528519 ========== to ========== Add AudioTrackRecorder for audio component of MediaStream recording. This is the first of ~three CLs for adding the audio component of MediaStreamRecording. These CLs will consist of: 1) Add AudioTrackRecorder, a MediaStreamAudioSink which will be owned by MediaRecorderHandler 2) Update WebmMuxer to mux Opus output of AudioTrackRecorder 3) Update MediaRecorderHandler to use AudioTrackRecorder BUG=528519 ==========
Description was changed from ========== Add AudioTrackRecorder for audio component of MediaStream recording. This is the first of ~three CLs for adding the audio component of MediaStreamRecording. These CLs will consist of: 1) Add AudioTrackRecorder, a MediaStreamAudioSink which will be owned by MediaRecorderHandler 2) Update WebmMuxer to mux Opus output of AudioTrackRecorder 3) Update MediaRecorderHandler to use AudioTrackRecorder BUG=528519 ========== to ========== Add AudioTrackRecorder for audio component of MediaStream recording. This is the first of ~three CLs for adding the audio component of MediaStreamRecording. These CLs will be: 1) Add AudioTrackRecorder, a MediaStreamAudioSink which will be owned by MediaRecorderHandler 2) Update WebmMuxer to mux Opus output of AudioTrackRecorder 3) Update MediaRecorderHandler to use AudioTrackRecorder BUG=528519 ==========
Looks pretty good % opus things, that I don't know about. Couple of comments. Turn TODO()s into actions or add bugs for them please. https://chromiumcodereview.appspot.com/1406113002/diff/40001/content/renderer... File content/renderer/media/audio_track_recorder.cc (right): https://chromiumcodereview.appspot.com/1406113002/diff/40001/content/renderer... content/renderer/media/audio_track_recorder.cc:40: AudioEncoder(const OnEncodedAudioCB& on_encoded_audio_cb) Do not inline large methods such as this ctor and dtor below. https://chromiumcodereview.appspot.com/1406113002/diff/40001/content/renderer... content/renderer/media/audio_track_recorder.cc:57: bool IsInitialized() { return initialized_; } You can also turn |audio_params_| into a scoped_ptr() and redirect this method to testing it. https://chromiumcodereview.appspot.com/1406113002/diff/40001/content/renderer... content/renderer/media/audio_track_recorder.cc:92: scoped_ptr<base::Thread> encoding_thread_; Maybe miu@ will have something to say about the necessity of this thread down the line, for the time being I'll skip it. I'd say that perhaps the extra delay+jitter of the thread jump offsets the benefits of not loading the IO thread. https://chromiumcodereview.appspot.com/1406113002/diff/40001/content/renderer... content/renderer/media/audio_track_recorder.cc:137: opus_encoder_ = reinterpret_cast<OpusEncoder*>(encoder_memory_.get()); Wow this is black magic! Seriously though, do we actually need |encoder_memory_|? can't we allocate and cast to |opus_encoder_|, or would that miss one (necessary) destructor? https://chromiumcodereview.appspot.com/1406113002/diff/40001/content/renderer... content/renderer/media/audio_track_recorder.cc:146: !IsValidFrameDuration(frame_duration)) { I'd move the check for IsValidFrameDuration() to right after initializing it in l.131 (and make |frame_duration| const). https://chromiumcodereview.appspot.com/1406113002/diff/40001/content/renderer... content/renderer/media/audio_track_recorder.cc:147: DVLOG(1) << __FUNCTION__ << ": bad inputs."; Make this msg more meaningful or remove it. https://chromiumcodereview.appspot.com/1406113002/diff/40001/content/renderer... content/renderer/media/audio_track_recorder.cc:153: DVLOG(1) << __FUNCTION__ << ": couldn't initialize opus encoder."; What about caching the result of opus_encoder_init() and showing some info error using opus_strerror() (see below). https://chromiumcodereview.appspot.com/1406113002/diff/40001/content/renderer... content/renderer/media/audio_track_recorder.cc:162: bitrate = OPUS_AUTO; |bitrate| is initalized to kDefaultAudioEncoderBitrate, so I fail to see the usefulness of this if(). Suggest some const initing (with the comment) in the line of: const int bitrate = (w00t) ? OPUS_AUTO : kDefaultAudioEncoderBitrate; https://chromiumcodereview.appspot.com/1406113002/diff/40001/content/renderer... content/renderer/media/audio_track_recorder.cc:183: DCHECK_EQ(audio_bus->channels(), audio_params_.channels()); If this doesn't change as |src_pos| moves along, move it out of the loop. https://chromiumcodereview.appspot.com/1406113002/diff/40001/content/renderer... content/renderer/media/audio_track_recorder.cc:231: << ": Error code from opus_encode_float(): " << result; Suggestion: Use opus_strerror() [1] [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/opus/s... https://chromiumcodereview.appspot.com/1406113002/diff/40001/content/renderer... content/renderer/media/audio_track_recorder.cc:235: // one byte means the packet does not need to be transmitted. nit: remove "byte" https://chromiumcodereview.appspot.com/1406113002/diff/40001/content/renderer... content/renderer/media/audio_track_recorder.cc:273: DCHECK(encoder_->IsInitialized()); Thread? Or a comment about it for the method. https://chromiumcodereview.appspot.com/1406113002/diff/40001/content/renderer... content/renderer/media/audio_track_recorder.cc:282: encoder_->InsertAudio(audio_data.Pass(), estimated_capture_time); Is a bit confusing that we copy the data from |audio_bus| at this stage, since we define InsertAudio(). Shouldn't we do that inside that method instead? IOW I would see this method as a simple trampoline. https://chromiumcodereview.appspot.com/1406113002/diff/40001/content/renderer... content/renderer/media/audio_track_recorder.cc:292: // TODO(ajose): consider only storing params in ATR _or_ encoder, not both. I was thinking the same, and by preference I'd say inside ATR::AE, no need to cache it up here. https://chromiumcodereview.appspot.com/1406113002/diff/40001/content/renderer... File content/renderer/media/audio_track_recorder.h (right): https://chromiumcodereview.appspot.com/1406113002/diff/40001/content/renderer... content/renderer/media/audio_track_recorder.h:50: scoped_refptr<AudioEncoder> encoder_; const ? (not 100% sure since you reset() it explicitly in dtor). https://chromiumcodereview.appspot.com/1406113002/diff/40001/content/renderer... File content/renderer/media/audio_track_recorder_unittest.cc (right): https://chromiumcodereview.appspot.com/1406113002/diff/40001/content/renderer... content/renderer/media/audio_track_recorder_unittest.cc:51: public EncodedAudioHandlerInterface { Actually you don't need to define this interface/ derive from it to MOCK its method (this, I was confused about), you can MOCK a non-virtual method (but you can't eclipse it). https://chromiumcodereview.appspot.com/1406113002/diff/40001/content/renderer... content/renderer/media/audio_track_recorder_unittest.cc:138: audio_track_recorder_->OnSetFormat(params1_); What's the point of this? Suggestion: You can add methods to sniff inside a class' inner state and make then test only by suffixing them ForTesting(). Every TEST_x is run in a public subclass of AudioTrackRecorderTest, so the idea woud be test -> accessor in AudioTrackRecorderTest --> blaForTesting() fixture in ATR::AudioEncoder. SG?
ajose@chromium.org changed reviewers: + miu@chromium.org
Thanks for the comments! I'll make sure to convert TODOs to bugs after a few more rounds of comments (hoping most of them will go away). miu@ - mind taking a look, particularly at opus stuff? Should encoding happen on a separate thread? Should there be more buffering of audio data? https://codereview.chromium.org/1406113002/diff/40001/content/renderer/media/... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1406113002/diff/40001/content/renderer/media/... content/renderer/media/audio_track_recorder.cc:40: AudioEncoder(const OnEncodedAudioCB& on_encoded_audio_cb) On 2015/10/19 20:02:08, mcasas wrote: > Do not inline large methods such as this ctor and dtor below. Done. https://codereview.chromium.org/1406113002/diff/40001/content/renderer/media/... content/renderer/media/audio_track_recorder.cc:57: bool IsInitialized() { return initialized_; } On 2015/10/19 20:02:08, mcasas wrote: > You can also turn |audio_params_| into a scoped_ptr() > and redirect this method to testing it. Done. https://codereview.chromium.org/1406113002/diff/40001/content/renderer/media/... content/renderer/media/audio_track_recorder.cc:137: opus_encoder_ = reinterpret_cast<OpusEncoder*>(encoder_memory_.get()); On 2015/10/19 20:02:08, mcasas wrote: > Wow this is black magic! > Seriously though, do we actually need |encoder_memory_|? > can't we allocate and cast to |opus_encoder_|, or would that > miss one (necessary) destructor? Switched to opus_encoder_create https://codereview.chromium.org/1406113002/diff/40001/content/renderer/media/... content/renderer/media/audio_track_recorder.cc:146: !IsValidFrameDuration(frame_duration)) { On 2015/10/19 20:02:08, mcasas wrote: > I'd move the check for IsValidFrameDuration() to right after > initializing it in l.131 (and make |frame_duration| const). Done. https://codereview.chromium.org/1406113002/diff/40001/content/renderer/media/... content/renderer/media/audio_track_recorder.cc:147: DVLOG(1) << __FUNCTION__ << ": bad inputs."; On 2015/10/19 20:02:09, mcasas wrote: > Make this msg more meaningful or remove it. Done. https://codereview.chromium.org/1406113002/diff/40001/content/renderer/media/... content/renderer/media/audio_track_recorder.cc:153: DVLOG(1) << __FUNCTION__ << ": couldn't initialize opus encoder."; On 2015/10/19 20:02:08, mcasas wrote: > What about caching the result of opus_encoder_init() and > showing some info error using opus_strerror() (see below). Done. https://codereview.chromium.org/1406113002/diff/40001/content/renderer/media/... content/renderer/media/audio_track_recorder.cc:162: bitrate = OPUS_AUTO; On 2015/10/19 20:02:08, mcasas wrote: > |bitrate| is initalized to kDefaultAudioEncoderBitrate, so I fail > to see the usefulness of this if(). Suggest some const initing > (with the comment) in the line of: > > const int bitrate = (w00t) ? OPUS_AUTO : kDefaultAudioEncoderBitrate; Was considering letting user set bitrate but will probably just let opus always choose for now. https://codereview.chromium.org/1406113002/diff/40001/content/renderer/media/... content/renderer/media/audio_track_recorder.cc:183: DCHECK_EQ(audio_bus->channels(), audio_params_.channels()); On 2015/10/19 20:02:08, mcasas wrote: > If this doesn't change as |src_pos| moves along, move it > out of the loop. Done. https://codereview.chromium.org/1406113002/diff/40001/content/renderer/media/... content/renderer/media/audio_track_recorder.cc:231: << ": Error code from opus_encode_float(): " << result; On 2015/10/19 20:02:08, mcasas wrote: > Suggestion: Use opus_strerror() [1] > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/opus/s... Nice https://codereview.chromium.org/1406113002/diff/40001/content/renderer/media/... content/renderer/media/audio_track_recorder.cc:235: // one byte means the packet does not need to be transmitted. On 2015/10/19 20:02:08, mcasas wrote: > nit: remove "byte" Done. https://codereview.chromium.org/1406113002/diff/40001/content/renderer/media/... content/renderer/media/audio_track_recorder.cc:273: DCHECK(encoder_->IsInitialized()); On 2015/10/19 20:02:08, mcasas wrote: > Thread? Or a comment about it for the method. Looking into this. https://codereview.chromium.org/1406113002/diff/40001/content/renderer/media/... content/renderer/media/audio_track_recorder.cc:282: encoder_->InsertAudio(audio_data.Pass(), estimated_capture_time); On 2015/10/19 20:02:08, mcasas wrote: > Is a bit confusing that we copy the data from |audio_bus| > at this stage, since we define InsertAudio(). Shouldn't we > do that inside that method instead? IOW I would see this > method as a simple trampoline. Done. https://codereview.chromium.org/1406113002/diff/40001/content/renderer/media/... content/renderer/media/audio_track_recorder.cc:292: // TODO(ajose): consider only storing params in ATR _or_ encoder, not both. On 2015/10/19 20:02:08, mcasas wrote: > I was thinking the same, and by preference I'd say inside ATR::AE, > no need to cache it up here. Done. https://codereview.chromium.org/1406113002/diff/40001/content/renderer/media/... File content/renderer/media/audio_track_recorder.h (right): https://codereview.chromium.org/1406113002/diff/40001/content/renderer/media/... content/renderer/media/audio_track_recorder.h:50: scoped_refptr<AudioEncoder> encoder_; On 2015/10/19 20:02:09, mcasas wrote: > const ? > (not 100% sure since you reset() it explicitly in dtor). Done. https://codereview.chromium.org/1406113002/diff/40001/content/renderer/media/... File content/renderer/media/audio_track_recorder_unittest.cc (right): https://codereview.chromium.org/1406113002/diff/40001/content/renderer/media/... content/renderer/media/audio_track_recorder_unittest.cc:51: public EncodedAudioHandlerInterface { On 2015/10/19 20:02:09, mcasas wrote: > Actually you don't need to define this interface/ derive from it to > MOCK its method (this, I was confused about), you can MOCK > a non-virtual method (but you can't eclipse it). Done. https://codereview.chromium.org/1406113002/diff/40001/content/renderer/media/... content/renderer/media/audio_track_recorder_unittest.cc:138: audio_track_recorder_->OnSetFormat(params1_); On 2015/10/19 20:02:09, mcasas wrote: > What's the point of this? > Suggestion: You can add methods to sniff inside a class' > inner state and make then test only by suffixing them > ForTesting(). Every TEST_x is run in a public subclass > of AudioTrackRecorderTest, so the idea woud be > test -> accessor in AudioTrackRecorderTest --> > blaForTesting() fixture in ATR::AudioEncoder. SG? Acknowledged.
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
A few more comments. https://chromiumcodereview.appspot.com/1406113002/diff/140001/content/rendere... File content/renderer/media/audio_track_recorder.cc (right): https://chromiumcodereview.appspot.com/1406113002/diff/140001/content/rendere... content/renderer/media/audio_track_recorder.cc:35: : on_encoded_audio_cb_(on_encoded_audio_cb), Don't inline complex ctor. https://chromiumcodereview.appspot.com/1406113002/diff/140001/content/rendere... content/renderer/media/audio_track_recorder.cc:69: const scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_; I think this fellow is not used now, perhaps you wanted to use it for DCHECK()ing? If the latter is the case, you can sandwich it in an #if DCHECK_IS_ON(). https://chromiumcodereview.appspot.com/1406113002/diff/140001/content/rendere... content/renderer/media/audio_track_recorder.cc:79: OpusEncoder* opus_encoder_; Hmm this smells like a base::WeakPtr<>. However, there's another problem: |opus_encoder_| is created and destroyed on Main thread, but is used on EncodeFromFilledBuffer() on |origin_task_runner_| thread. How's that not a race? I'd suggest following a pattern like VideoTrackRecorder with the ScopedVpxCodecCtxPtr [1], then from ~AudioEncoder(), send |opus_encoder_| to be destroyed in |origin_task_runner_| via DeleteSoon(). [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... https://chromiumcodereview.appspot.com/1406113002/diff/140001/content/rendere... content/renderer/media/audio_track_recorder.cc:131: const media::AudioParameters& params) { Thread check plz, here and elsewhere. https://chromiumcodereview.appspot.com/1406113002/diff/140001/content/rendere... content/renderer/media/audio_track_recorder.cc:134: if (audio_params_.get() && audio_params_->Equals(params)) How can |audio_params_.get()| be nullptr if it's a reference? https://chromiumcodereview.appspot.com/1406113002/diff/140001/content/rendere... content/renderer/media/audio_track_recorder.cc:152: sampling_rate % samples_per_frame_ != 0) { I see what you do here but is more customary to check values as close as possible to the assignments. Also, you do a DCHECK(params.IsValid()) on l.132 but then act upon (part of) its tests (num_channels <= 0). I'd suggest, instead of l.132 if (!params.IsValid()) { DLOG(ERROR) << "Invalid params: " << params.AsHumanReadableString(); return false(); } DCHECK_EQ(params.bits_per_sample(), 16); // etc And check only for specific validity on every assignment afterwards, either via DCHECK or if(badstuff) { DLOG(ERROR); return false; } https://chromiumcodereview.appspot.com/1406113002/diff/140001/content/rendere... content/renderer/media/audio_track_recorder.cc:169: DVLOG(1) << __FUNCTION__ << ": couldn't initialize opus encoder: " There's no real need for __FUNCTION__ here, and I'd write DLOG(ERROR) <<... https://chromiumcodereview.appspot.com/1406113002/diff/140001/content/rendere... content/renderer/media/audio_track_recorder.cc:179: // later versions. Could we codify this assumption? int bitrate; DCHECK(opus_encoder_ctl(enc, OPUS_GET_BITRATE(&bitrate)) == OPUS_OK && bitrate == kWhateverBitrateIsAssumed); https://chromiumcodereview.appspot.com/1406113002/diff/140001/content/rendere... content/renderer/media/audio_track_recorder.cc:180: CHECK_EQ(opus_encoder_ctl(opus_encoder_, OPUS_SET_BITRATE(OPUS_AUTO)), Here you say: crash the renderer tab if opus_encoder_ctl() can't set the bitrate, isn't that a bit drastic? Perhaps we could make InitOpus() return a boolean and act upon it on AudioTrackRecorder()? https://chromiumcodereview.appspot.com/1406113002/diff/140001/content/rendere... content/renderer/media/audio_track_recorder.cc:206: reinterpret_cast<uint8*>(string_as_array(out)), kOpusMaxPayloadSize); What about s/reinterpret_cast<uint8*>(string_as_array(out))/out->data()/ ? https://chromiumcodereview.appspot.com/1406113002/diff/140001/content/rendere... content/renderer/media/audio_track_recorder.cc:218: } What about: if (result > 1) { out->resize(result); return true; } // Comment on possible <=1 return values... DLOG_IF(ERROR, result < 0) << "Error! resistance is futile" << ...; return false; https://chromiumcodereview.appspot.com/1406113002/diff/140001/content/rendere... content/renderer/media/audio_track_recorder.cc:224: // See https://tools.ietf.org/html/rfc6716#section-2.1.4 Maybe also mention opus.h? [1] [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/opus/s... https://chromiumcodereview.appspot.com/1406113002/diff/140001/content/rendere... content/renderer/media/audio_track_recorder.cc:230: duration == base::TimeDelta::FromMilliseconds(60); It sucks that we have to check for this duration ourselves! Is there no (perhaps obscure) opus function for that? (I realize the answer might be "no!" ). https://chromiumcodereview.appspot.com/1406113002/diff/140001/content/rendere... File content/renderer/media/audio_track_recorder.h (right): https://chromiumcodereview.appspot.com/1406113002/diff/140001/content/rendere... content/renderer/media/audio_track_recorder.h:22: class CONTENT_EXPORT AudioTrackRecorder Need to write documentation here.
Structural issues: https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:17: const int kDefaultFramesPerSecond = 100; Consider using enums for these constants rather than global ints (per recent discussion about a month ago on chromium-dev@): enum { DEFAULT_FRAMES_PER_SECOND = 100, OPUS_MAX_PAYLOAD_SIZE = 4000 }; https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:22: // Note: Whereas other RTP implementations do not, the cast library is This comment seems....out of place? ;-) https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:31: class AudioTrackRecorder::AudioEncoder General statement about the entire AudioEncoder class: Can you make it so most (or all) of the code is being run on the same thread (i.e., an encoder thread owned by AudioTrackRecorder)? This would make creation/destruction much cleaner and less error-prone. https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:34: AudioEncoder(const OnEncodedAudioCB& on_encoded_audio_cb) Need explicit keyword here. https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:35: : on_encoded_audio_cb_(on_encoded_audio_cb), On 2015/10/21 19:19:51, mcasas wrote: > Don't inline complex ctor. It doesn't matter for classes private to .cc modules. ;-) https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:74: scoped_ptr<media::AudioParameters> audio_params_; No need for separate heap allocation here. Just: media::AudioParams params_; https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:102: origin_task_runner_ = base::MessageLoop::current()->task_runner(); This doesn't seem safe to me. Usually encoding should happen on its own thread. (See comment below.) https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:166: opus_encoder_ = opus_encoder_create(sampling_rate, num_channels, Can this InitOpus() method be called multiple times? (IIUC, the answer is 'yes' since the audio format could change via calls to OnSetFormat().) Therefore, you should check whether |opus_encoder_| is not null, and destroy the old one first (or keep the current instance and change its configuration, if possible). https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:237: encoder_(new AudioEncoder(on_encoded_audio_cb)), Note: If AudioEncoder was only used on a separate encode thread, you could wrap this callback like so: new AudioEncoder(media::BindToCurrentLoop(on_encoded_audio_cb)) This would let the encoder thread trampoline the run of the callback to the current thread. https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:256: encoder_->EncodeAudio(audio_bus, estimated_capture_time); Is this method being called on a real-time audio thread? If so, you should not be doing audio encoding on this thread. Instead, a separate encoder thread should be used.
Patchset #4 (id:160001) has been deleted
Thanks for the comments! Hopefully the threading is sane, let me know what you think. https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:17: const int kDefaultFramesPerSecond = 100; On 2015/10/21 20:35:38, miu wrote: > Consider using enums for these constants rather than global ints (per recent > discussion about a month ago on chromium-dev@): > > enum { > DEFAULT_FRAMES_PER_SECOND = 100, > OPUS_MAX_PAYLOAD_SIZE = 4000 > }; Done. https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:22: // Note: Whereas other RTP implementations do not, the cast library is On 2015/10/21 20:35:38, miu wrote: > This comment seems....out of place? ;-) Whoops... https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:31: class AudioTrackRecorder::AudioEncoder On 2015/10/21 20:35:38, miu wrote: > General statement about the entire AudioEncoder class: Can you make it so most > (or all) of the code is being run on the same thread (i.e., an encoder thread > owned by AudioTrackRecorder)? This would make creation/destruction much cleaner > and less error-prone. Done. https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:34: AudioEncoder(const OnEncodedAudioCB& on_encoded_audio_cb) On 2015/10/21 20:35:38, miu wrote: > Need explicit keyword here. Done. https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:35: : on_encoded_audio_cb_(on_encoded_audio_cb), On 2015/10/21 20:35:38, miu wrote: > On 2015/10/21 19:19:51, mcasas wrote: > > Don't inline complex ctor. > > It doesn't matter for classes private to .cc modules. ;-) Done. https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:35: : on_encoded_audio_cb_(on_encoded_audio_cb), On 2015/10/21 19:19:51, mcasas wrote: > Don't inline complex ctor. Acknowledged. https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:69: const scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_; On 2015/10/21 19:19:52, mcasas wrote: > I think this fellow is not used now, perhaps you wanted to > use it for DCHECK()ing? If the latter is the case, you can > sandwich it in an #if DCHECK_IS_ON(). ThreadChecker already does something similar to this [1]. Will change if you'd prefer it. [1] https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr... https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:74: scoped_ptr<media::AudioParameters> audio_params_; On 2015/10/21 20:35:38, miu wrote: > No need for separate heap allocation here. Just: > > media::AudioParams params_; Done. https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:79: OpusEncoder* opus_encoder_; On 2015/10/21 19:19:52, mcasas wrote: > Hmm this smells like a base::WeakPtr<>. However, there's another > problem: |opus_encoder_| is created and destroyed on Main > thread, but is used on EncodeFromFilledBuffer() on > |origin_task_runner_| thread. How's that not a race? > > I'd suggest following a pattern like VideoTrackRecorder with the > ScopedVpxCodecCtxPtr [1], then from ~AudioEncoder(), send > |opus_encoder_| to be destroyed in |origin_task_runner_| via > DeleteSoon(). > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... Think this is resolved with new structure - AudioEncoder is created and destroyed on main, but all AudioEncoder work should happen on |encoding_thread_| (with tasks potentialy being posted from main and audio capture thread). WDYT? https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:102: origin_task_runner_ = base::MessageLoop::current()->task_runner(); On 2015/10/21 20:35:38, miu wrote: > This doesn't seem safe to me. Usually encoding should happen on its own thread. > (See comment below.) Done. https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:131: const media::AudioParameters& params) { On 2015/10/21 19:19:52, mcasas wrote: > Thread check plz, here and elsewhere. Done. https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:152: sampling_rate % samples_per_frame_ != 0) { On 2015/10/21 19:19:52, mcasas wrote: > I see what you do here but is more customary to > check values as close as possible to the assignments. > > Also, you do a DCHECK(params.IsValid()) on l.132 > but then act upon (part of) its tests (num_channels <= 0). > > I'd suggest, instead of l.132 > if (!params.IsValid()) { > DLOG(ERROR) << "Invalid params: " << params.AsHumanReadableString(); > return false(); > } > DCHECK_EQ(params.bits_per_sample(), 16); > // etc > > And check only for specific validity on every assignment afterwards, > either via DCHECK or if(badstuff) { DLOG(ERROR); return false; } Might have gone overboard... https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:166: opus_encoder_ = opus_encoder_create(sampling_rate, num_channels, On 2015/10/21 20:35:38, miu wrote: > Can this InitOpus() method be called multiple times? (IIUC, the answer is 'yes' > since the audio format could change via calls to OnSetFormat().) Therefore, you > should check whether |opus_encoder_| is not null, and destroy the old one first > (or keep the current instance and change its configuration, if possible). Done. https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:169: DVLOG(1) << __FUNCTION__ << ": couldn't initialize opus encoder: " On 2015/10/21 19:19:52, mcasas wrote: > There's no real need for __FUNCTION__ here, and I'd write DLOG(ERROR) <<... Done. https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:180: CHECK_EQ(opus_encoder_ctl(opus_encoder_, OPUS_SET_BITRATE(OPUS_AUTO)), On 2015/10/21 19:19:51, mcasas wrote: > Here you say: crash the renderer tab if opus_encoder_ctl() can't set > the bitrate, isn't that a bit drastic? Perhaps we could make > InitOpus() return a boolean and act upon it on AudioTrackRecorder()? Done. https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:206: reinterpret_cast<uint8*>(string_as_array(out)), kOpusMaxPayloadSize); On 2015/10/21 19:19:52, mcasas wrote: > What about > s/reinterpret_cast<uint8*>(string_as_array(out))/out->data()/ ? out->data() is const char*, need unsigned char* but the internet says const_cast might be finicky - worth changing? https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:218: } On 2015/10/21 19:19:52, mcasas wrote: > What about: > > if (result > 1) { > out->resize(result); > return true; > } > // Comment on possible <=1 return values... > DLOG_IF(ERROR, result < 0) << "Error! resistance is futile" << ...; > return false; Done. https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:230: duration == base::TimeDelta::FromMilliseconds(60); On 2015/10/21 19:19:52, mcasas wrote: > It sucks that we have to check for this duration ourselves! > Is there no (perhaps obscure) opus function for that? (I > realize the answer might be "no!" ). Not that I can find :( https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:237: encoder_(new AudioEncoder(on_encoded_audio_cb)), On 2015/10/21 20:35:38, miu wrote: > Note: If AudioEncoder was only used on a separate encode thread, you could wrap > this callback like so: > > new AudioEncoder(media::BindToCurrentLoop(on_encoded_audio_cb)) > > This would let the encoder thread trampoline the run of the callback to the > current thread. Nice! https://codereview.chromium.org/1406113002/diff/140001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:256: encoder_->EncodeAudio(audio_bus, estimated_capture_time); On 2015/10/21 20:35:38, miu wrote: > Is this method being called on a real-time audio thread? If so, you should not > be doing audio encoding on this thread. Instead, a separate encoder thread > should be used. Done.
Patchset #5 (id:200001) has been deleted
Haven't looked into ownerships and destruction sequences in detail, but here's some comments. (I'm sure miu@ will focus on those anyway ;) ) https://chromiumcodereview.appspot.com/1406113002/diff/180001/content/rendere... File content/renderer/media/audio_track_recorder.cc (right): https://chromiumcodereview.appspot.com/1406113002/diff/180001/content/rendere... content/renderer/media/audio_track_recorder.cc:45: bool IsInitialized() { return initialized_; } Since IsInitialized() is only used for DCHECK()s, there's no need for |initialized_|, you can relay to e.g. |audio_params_.IsValid()|, WDYT? And it should be private. https://chromiumcodereview.appspot.com/1406113002/diff/180001/content/rendere... content/renderer/media/audio_track_recorder.cc:90: AudioTrackRecorder::AudioEncoder::~AudioEncoder() { Order of method definition should follow the declaration order [1], please reshuffle. [1] http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... https://chromiumcodereview.appspot.com/1406113002/diff/180001/content/rendere... content/renderer/media/audio_track_recorder.cc:101: encoder_thread_checker_.DetachFromThread(); Hmm this would reset the checker every time OnSetFormat() is called. Is it guaranteed that would happen only once, and/or that we'd get an OnSetFormat() before EncodeAudio()? I'd say, move this DetachFromThread() to AudioEncode(). First time CalledOnValidThread() is called, it will peg itself to the current thread, and that'd would also catch (potential) OnSetFormat() calls from other threads. In those lines, perhaps also add here an: DCHECK(!IsInitialized()); https://chromiumcodereview.appspot.com/1406113002/diff/180001/content/rendere... content/renderer/media/audio_track_recorder.cc:104: DLOG(ERROR) << "Couldn't initialize opus."; No need for more DLOG()s here, InitOpus() is informational enough. OnSetFormat() is returning void but InitOpus() returns a boolean. Is there no way to signal an initialization error upwards (to MediaRecorderHandler I presume)? https://chromiumcodereview.appspot.com/1406113002/diff/180001/content/rendere... content/renderer/media/audio_track_recorder.cc:193: TransferSamplesIntoBuffer(audio_bus.get(), src_pos, buffer_fill_end_, AudioBus has some method called ToInterleaved() [1] which could be handy here (or not). [1] https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_b... https://chromiumcodereview.appspot.com/1406113002/diff/180001/content/rendere... content/renderer/media/audio_track_recorder.cc:240: out->resize(result); This is not your fault, but are we allocating an array of |OPUS_MAX_PAYLOAD_SIZE| and, after encoding, resizing it to the appropriate size, for every single frame? That doesn't sound very efficient :( https://chromiumcodereview.appspot.com/1406113002/diff/180001/content/rendere... content/renderer/media/audio_track_recorder.cc:252: bool AudioTrackRecorder::AudioEncoder::IsValidFrameDuration( Move this method to an anonymous namespace and out of the class (smaller footprint). https://chromiumcodereview.appspot.com/1406113002/diff/180001/content/rendere... content/renderer/media/audio_track_recorder.cc:285: track_.reset(); I don't think you need to reset() it explicitly since is anyway going out of scope [1]. With that, you could make |track_| const. (This could apply to VideoTrackRecorder too) [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://chromiumcodereview.appspot.com/1406113002/diff/180001/content/rendere... File content/renderer/media/audio_track_recorder.h (right): https://chromiumcodereview.appspot.com/1406113002/diff/180001/content/rendere... content/renderer/media/audio_track_recorder.h:15: #include "media/audio/audio_parameters.h" |media::AudioParameters| can be forward declared right? https://chromiumcodereview.appspot.com/1406113002/diff/180001/content/rendere... content/renderer/media/audio_track_recorder.h:63: // |encoder_| should be initialized before |encoder_thread_| s.t. what's |s.t.| standing for?
Patchset #5 (id:220001) has been deleted
Thanks for the comments :0) Saving some of the more substantial changes for another CL. Still working on a memory leak in test. https://codereview.chromium.org/1406113002/diff/180001/content/renderer/media... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1406113002/diff/180001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:45: bool IsInitialized() { return initialized_; } On 2015/10/26 18:56:25, mcasas wrote: > Since IsInitialized() is only used for DCHECK()s, > there's no need for |initialized_|, you can relay > to e.g. |audio_params_.IsValid()|, WDYT? > And it should be private. I kinda like explicit initialization (doesn't hide state in AudioParameters, which might be confusing to future maintainers) but will change if you prefer. https://codereview.chromium.org/1406113002/diff/180001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:90: AudioTrackRecorder::AudioEncoder::~AudioEncoder() { On 2015/10/26 18:56:25, mcasas wrote: > Order of method definition should follow the declaration order [1], > please reshuffle. > > [1] > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... Done. https://codereview.chromium.org/1406113002/diff/180001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:101: encoder_thread_checker_.DetachFromThread(); On 2015/10/26 18:56:25, mcasas wrote: > Hmm this would reset the checker every time OnSetFormat() is called. > Is it guaranteed that would happen only once, and/or that we'd get > an OnSetFormat() before EncodeAudio()? I'd say, move this > DetachFromThread() to AudioEncode(). First time CalledOnValidThread() > is called, it will peg itself to the current thread, and that'd would > also catch (potential) OnSetFormat() calls from other threads. > In those lines, perhaps also add here an: > DCHECK(!IsInitialized()); Done. https://codereview.chromium.org/1406113002/diff/180001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:104: DLOG(ERROR) << "Couldn't initialize opus."; On 2015/10/26 18:56:25, mcasas wrote: > No need for more DLOG()s here, InitOpus() is informational enough. > OnSetFormat() is returning void but InitOpus() returns a boolean. > Is there no way to signal an initialization error upwards (to > MediaRecorderHandler I presume)? Hmm, tricky because ATR::OnSetFormat() is calling AudioEncoder::OnSetFormat() asynchronously, and also ATR::OnSetFormat() is called by media::MediaStreamAudioSinkOwner which we don't control in MediaRecorderHandler. Any ideas? https://codereview.chromium.org/1406113002/diff/180001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:193: TransferSamplesIntoBuffer(audio_bus.get(), src_pos, buffer_fill_end_, On 2015/10/26 18:56:25, mcasas wrote: > AudioBus has some method called ToInterleaved() [1] which > could be handy here (or not). > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_b... I'll look into it, added a bug. https://codereview.chromium.org/1406113002/diff/180001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:240: out->resize(result); On 2015/10/26 18:56:25, mcasas wrote: > This is not your fault, but are we allocating an array of > |OPUS_MAX_PAYLOAD_SIZE| and, after encoding, > resizing it to the appropriate size, for every single frame? > That doesn't sound very efficient :( I'll look into it, added a bug. https://codereview.chromium.org/1406113002/diff/180001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:252: bool AudioTrackRecorder::AudioEncoder::IsValidFrameDuration( On 2015/10/26 18:56:25, mcasas wrote: > Move this method to an anonymous namespace and out of the > class (smaller footprint). Done. https://codereview.chromium.org/1406113002/diff/180001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:285: track_.reset(); On 2015/10/26 18:56:25, mcasas wrote: > I don't think you need to reset() it explicitly since is anyway > going out of scope [1]. With that, you could make |track_| const. > (This could apply to VideoTrackRecorder too) > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Done. https://codereview.chromium.org/1406113002/diff/180001/content/renderer/media... File content/renderer/media/audio_track_recorder.h (right): https://codereview.chromium.org/1406113002/diff/180001/content/renderer/media... content/renderer/media/audio_track_recorder.h:15: #include "media/audio/audio_parameters.h" On 2015/10/26 18:56:25, mcasas wrote: > |media::AudioParameters| can be forward declared right? Don't think so, I switched it back to being a member https://codereview.chromium.org/1406113002/diff/180001/content/renderer/media... content/renderer/media/audio_track_recorder.h:63: // |encoder_| should be initialized before |encoder_thread_| s.t. On 2015/10/26 18:56:25, mcasas wrote: > what's |s.t.| standing for? "such that", I'll expand it
https://codereview.chromium.org/1406113002/diff/260001/content/renderer/media... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1406113002/diff/260001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:73: bool IsInitialized() { return initialized_; } You don't need the |initialized_| bool. Instead, just return !!opus_encoder_; https://codereview.chromium.org/1406113002/diff/260001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:115: InitOpus(params); The return value is being ignored here. Looking at the code some more, it seems like you don't need a separate InitOpus() method. Just put all that code into this method. https://codereview.chromium.org/1406113002/diff/260001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:161: if (params.bits_per_sample() != 16) { IMO, this is irrelevant. The AudioBuses always provide floats which, technically, are quasi-32 bits per sample. https://codereview.chromium.org/1406113002/diff/260001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:178: if (frame_duration == base::TimeDelta() || No need for zero frame_duration check since IsValidOpusFrameDuration() will return false in this case. https://codereview.chromium.org/1406113002/diff/260001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:189: DestroyOpus(); This should go at the top of the method, before any of the "early return" statements. https://codereview.chromium.org/1406113002/diff/260001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:303: // TODO(ajose): When will audio_bus be deleted? Looks like the closure (on line 310) or the AudioEncoder::EncodeAudio method will delete it automatically when the scoped_ptr<> goes out-of-scope. https://codereview.chromium.org/1406113002/diff/260001/content/renderer/media... File content/renderer/media/audio_track_recorder.h (right): https://codereview.chromium.org/1406113002/diff/260001/content/renderer/media... content/renderer/media/audio_track_recorder.h:73: const scoped_ptr<base::Thread> encoder_thread_; nit: No need for separate heap allocation (scoped_ptr<>) here, just this: base::Thread encoder_thread_; https://codereview.chromium.org/1406113002/diff/260001/content/renderer/media... File content/renderer/media/audio_track_recorder_unittest.cc (right): https://codereview.chromium.org/1406113002/diff/260001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:29: const int kNumChannels = 1; Consider testing more than one channel (e.g., to test the sample-interleaving code). https://codereview.chromium.org/1406113002/diff/260001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:97: scoped_ptr<AudioTrackRecorder> audio_track_recorder_; nit: Don't need separate heap allocation here, just: AudioTrackRecorder audio_track_recorder_;
Patchset #7 (id:280001) has been deleted
Patchset #7 (id:300001) has been deleted
Thanks for the comments! Still wondering about framerate - it's currently a constant, but that seems wrong. It would be better to calculate the framerate from AudioParameter's sample_rate and GetBytesPerFrame, right? Asan trybot should be passing now. https://codereview.chromium.org/1406113002/diff/260001/content/renderer/media... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1406113002/diff/260001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:73: bool IsInitialized() { return initialized_; } On 2015/10/26 22:41:04, miu wrote: > You don't need the |initialized_| bool. Instead, just return !!opus_encoder_; Done. https://codereview.chromium.org/1406113002/diff/260001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:115: InitOpus(params); On 2015/10/26 22:41:04, miu wrote: > The return value is being ignored here. Looking at the code some more, it seems > like you don't need a separate InitOpus() method. Just put all that code into > this method. Done. https://codereview.chromium.org/1406113002/diff/260001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:161: if (params.bits_per_sample() != 16) { On 2015/10/26 22:41:04, miu wrote: > IMO, this is irrelevant. The AudioBuses always provide floats which, > technically, are quasi-32 bits per sample. Done. https://codereview.chromium.org/1406113002/diff/260001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:178: if (frame_duration == base::TimeDelta() || On 2015/10/26 22:41:04, miu wrote: > No need for zero frame_duration check since IsValidOpusFrameDuration() will > return false in this case. Done. https://codereview.chromium.org/1406113002/diff/260001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:189: DestroyOpus(); On 2015/10/26 22:41:04, miu wrote: > This should go at the top of the method, before any of the "early return" > statements. Done. https://codereview.chromium.org/1406113002/diff/260001/content/renderer/media... File content/renderer/media/audio_track_recorder.h (right): https://codereview.chromium.org/1406113002/diff/260001/content/renderer/media... content/renderer/media/audio_track_recorder.h:73: const scoped_ptr<base::Thread> encoder_thread_; On 2015/10/26 22:41:04, miu wrote: > nit: No need for separate heap allocation (scoped_ptr<>) here, just this: > > base::Thread encoder_thread_; Done. https://codereview.chromium.org/1406113002/diff/260001/content/renderer/media... File content/renderer/media/audio_track_recorder_unittest.cc (right): https://codereview.chromium.org/1406113002/diff/260001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:29: const int kNumChannels = 1; On 2015/10/26 22:41:04, miu wrote: > Consider testing more than one channel (e.g., to test the sample-interleaving > code). Done. https://codereview.chromium.org/1406113002/diff/260001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:97: scoped_ptr<AudioTrackRecorder> audio_track_recorder_; On 2015/10/26 22:41:04, miu wrote: > nit: Don't need separate heap allocation here, just: > > AudioTrackRecorder audio_track_recorder_; Moving ATR to the initialization list of the tests ctr (and changing PrepareBlinkTrackOfType() to return a blink::WebMediaStreamTrack) is giving me segfaults I haven't been able to resolve. I can keep looking at it.
Patchset #7 (id:320001) has been deleted
Patchset #7 (id:340001) has been deleted
Patchset #7 (id:360001) has been deleted
Almost there. https://codereview.chromium.org/1406113002/diff/380001/content/renderer/media... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1406113002/diff/380001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:168: DCHECK(IsInitialized()); Actually, since there's no way for OnSetFormat() to inform of initialisation errors, this should be a real if(!IsInitialized()) return; otherwise we might get some uncontrolled crash below. https://codereview.chromium.org/1406113002/diff/380001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:210: // TODO(ajose): Replace with AudioBus::ToInterleaved()? http://crbug.com/547918 This line is > 80 characters :) https://codereview.chromium.org/1406113002/diff/380001/content/renderer/media... File content/renderer/media/audio_track_recorder.h (right): https://codereview.chromium.org/1406113002/diff/380001/content/renderer/media... content/renderer/media/audio_track_recorder.h:45: class AudioBus; move to l.16: namespace media { class AudioBus; } // namespace media https://codereview.chromium.org/1406113002/diff/380001/content/renderer/media... File content/renderer/media/audio_track_recorder_unittest.cc (right): https://codereview.chromium.org/1406113002/diff/380001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:73: EXPECT_LE(num_channels, 2); why not EXPECT(num_channels == 1 || num_channels == 2) ? https://codereview.chromium.org/1406113002/diff/380001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:79: : stereo_source_.OnMoreData(bus.get(), 0); I understand the code but I think it's the first time I see this in Chromium. Can you find other examples of it in the code base? Otherwise revert to if-else plz. https://codereview.chromium.org/1406113002/diff/380001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:147: // Need to figure out what to do about framerate. TODO(s) need to have an associated crbug.com. Or they are just comments. Consider then a crbug.com to "improve ATRTest" for different framerates and/or inspecting encoded results., and label it GoodFirstBug :)
Thanks for the comments :) Will add more crbugs ASAP if they're not resolved soon. https://codereview.chromium.org/1406113002/diff/380001/content/renderer/media... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1406113002/diff/380001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:168: DCHECK(IsInitialized()); On 2015/10/28 20:03:00, mcasas wrote: > Actually, since there's no way for OnSetFormat() to > inform of initialisation errors, this should be a real > if(!IsInitialized()) > return; > > otherwise we might get some uncontrolled > crash below. Done. https://codereview.chromium.org/1406113002/diff/380001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:210: // TODO(ajose): Replace with AudioBus::ToInterleaved()? http://crbug.com/547918 On 2015/10/28 20:03:00, mcasas wrote: > This line is > 80 characters :) I was hoping nobody would notice... https://codereview.chromium.org/1406113002/diff/380001/content/renderer/media... File content/renderer/media/audio_track_recorder.h (right): https://codereview.chromium.org/1406113002/diff/380001/content/renderer/media... content/renderer/media/audio_track_recorder.h:45: class AudioBus; On 2015/10/28 20:03:00, mcasas wrote: > move to l.16: > > namespace media { > class AudioBus; > } // namespace media Done. https://codereview.chromium.org/1406113002/diff/380001/content/renderer/media... File content/renderer/media/audio_track_recorder_unittest.cc (right): https://codereview.chromium.org/1406113002/diff/380001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:73: EXPECT_LE(num_channels, 2); On 2015/10/28 20:03:00, mcasas wrote: > why not > EXPECT(num_channels == 1 || num_channels == 2) > ? Done. https://codereview.chromium.org/1406113002/diff/380001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:79: : stereo_source_.OnMoreData(bus.get(), 0); On 2015/10/28 20:03:00, mcasas wrote: > I understand the code but I think it's the first time > I see this in Chromium. Can you find other examples of > it in the code base? Otherwise revert to if-else plz. Done. https://codereview.chromium.org/1406113002/diff/380001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:147: // Need to figure out what to do about framerate. On 2015/10/28 20:03:00, mcasas wrote: > TODO(s) need to have an associated http://crbug.com. > Or they are just comments. Consider then a > http://crbug.com to "improve ATRTest" for different > framerates and/or inspecting encoded results., > and label it GoodFirstBug :) SGTM. Will add a bug for the framerate stuff if it's not resolved soon.
Ah, I see the issue around the DEFAULT_FRAMES_PER_SECOND. Addressed this in my comments: https://codereview.chromium.org/1406113002/diff/400001/content/renderer/media... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1406113002/diff/400001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:20: DEFAULT_FRAMES_PER_SECOND = 100, Actually, the purpose of this is to define the duration of one frame for the purposes of Opus encoding. It doesn't really depend on anything external. I suggest you just hard-code the Opus frame duration to 10 ms (with caveat; see comment below). https://codereview.chromium.org/1406113002/diff/400001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:31: static bool IsValidOpusFrameDuration(base::TimeDelta duration) { This function isn't necessary. Just fix the Opus frame duration to 10 ms. https://codereview.chromium.org/1406113002/diff/400001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:71: bool IsInitialized() { return !!opus_encoder_; } style: Inline accessors are in lower case, and this one should be const: bool is_initialized() const { return !!opus_encoder_; } https://codereview.chromium.org/1406113002/diff/400001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:122: if (samples_per_frame_ <= 0 || Hmm...If you were to hard-code frame durations to 10 ms, this check here would only error-out on a few of the possible sample rates (e.g., 22,050 Hz). So, you may need to have some code that tries to use 10 ms frame durations and checks that the sample rate modulo 100 is zero. If it is not, it should try falling-back to 20 ms frame durations (where the sample rate modulo 50 is zero). Note: 100 FPS = 1000 ms / 10 ms 50 FPS = 1000 ms / 20 ms (BTW--If you want to be really pedantic, you could also use 40 ms and 60 ms as additional "fall-back" frame durations, since Opus supports them.)
Thanks for the comments, miu. I ended up adding a function to find a valid duration rather than hard-coding, would be glad to change. https://codereview.chromium.org/1406113002/diff/400001/content/renderer/media... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1406113002/diff/400001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:20: DEFAULT_FRAMES_PER_SECOND = 100, On 2015/10/29 01:26:05, miu wrote: > Actually, the purpose of this is to define the duration of one frame for the > purposes of Opus encoding. It doesn't really depend on anything external. I > suggest you just hard-code the Opus frame duration to 10 ms (with caveat; see > comment below). Done. https://codereview.chromium.org/1406113002/diff/400001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:31: static bool IsValidOpusFrameDuration(base::TimeDelta duration) { On 2015/10/29 01:26:05, miu wrote: > This function isn't necessary. Just fix the Opus frame duration to 10 ms. Done. https://codereview.chromium.org/1406113002/diff/400001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:71: bool IsInitialized() { return !!opus_encoder_; } On 2015/10/29 01:26:05, miu wrote: > style: Inline accessors are in lower case, and this one should be const: > > bool is_initialized() const { return !!opus_encoder_; } Done. https://codereview.chromium.org/1406113002/diff/400001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:122: if (samples_per_frame_ <= 0 || On 2015/10/29 01:26:05, miu wrote: > Hmm...If you were to hard-code frame durations to 10 ms, this check here would > only error-out on a few of the possible sample rates (e.g., 22,050 Hz). So, you > may need to have some code that tries to use 10 ms frame durations and checks > that the sample rate modulo 100 is zero. If it is not, it should try > falling-back to 20 ms frame durations (where the sample rate modulo 50 is zero). > > Note: > > 100 FPS = 1000 ms / 10 ms > 50 FPS = 1000 ms / 20 ms > > (BTW--If you want to be really pedantic, you could also use 40 ms and 60 ms as > additional "fall-back" frame durations, since Opus supports them.) Done.
Patchset #9 (id:420001) has been deleted
LGTM with minor nits. Good job. https://codereview.chromium.org/1406113002/diff/440001/content/renderer/media... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1406113002/diff/440001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:206: // called from the dtor (main thread) or from OnSetForamt (render thread); nit: OnSetFormat() https://codereview.chromium.org/1406113002/diff/440001/content/renderer/media... File content/renderer/media/audio_track_recorder_unittest.cc (right): https://codereview.chromium.org/1406113002/diff/440001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:80: // two channels Nit: remove this comment and with that, also the curly brackets. https://codereview.chromium.org/1406113002/diff/440001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:105: media::AudioParameters params1_; nit: I think |params1|, |params2|, |mono_source|| and |stereo_source_| can be const.
https://codereview.chromium.org/1406113002/diff/440001/content/renderer/media... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1406113002/diff/440001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:28: static bool GetOpusFrameDuration(int sample_rate, int* frame_duration) { nit: You could simplify by not taking an int* output argument. It might allow the compiler to do more optimization too. Suggestion: // Returns the Opus frame duration in milliseconds, or zero if none will work for the given |sample_rate|. static int GetOpusFrameDuration(int sample_rate) { ... } https://codereview.chromium.org/1406113002/diff/440001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:37: if (sample_rate % (1000 / possible_duration) == 0) { 1000 does not divide evenly by 60. Suggest you change this expression to: if ((sample_rate * possible_duration) % 1000 == 0) { https://codereview.chromium.org/1406113002/diff/440001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:136: samples_per_frame_ = params.sample_rate() / (1000 / frame_duration); Like above, this math could break if 1000 is not evenly divisible by |frame_duration|.
ajose@chromium.org changed reviewers: + sergeyu@chromium.org
Thanks for the comments! sergeyu@: PTAL for third_party/opus use, when you get a chance. https://codereview.chromium.org/1406113002/diff/440001/content/renderer/media... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1406113002/diff/440001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:28: static bool GetOpusFrameDuration(int sample_rate, int* frame_duration) { On 2015/10/29 23:16:42, miu wrote: > nit: You could simplify by not taking an int* output argument. It might allow > the compiler to do more optimization too. Suggestion: > > // Returns the Opus frame duration in milliseconds, or zero if none will work > for the given |sample_rate|. > static int GetOpusFrameDuration(int sample_rate) { > ... > } Done. https://codereview.chromium.org/1406113002/diff/440001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:37: if (sample_rate % (1000 / possible_duration) == 0) { On 2015/10/29 23:16:42, miu wrote: > 1000 does not divide evenly by 60. > > Suggest you change this expression to: > > if ((sample_rate * possible_duration) % 1000 == 0) { Done. https://codereview.chromium.org/1406113002/diff/440001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:136: samples_per_frame_ = params.sample_rate() / (1000 / frame_duration); On 2015/10/29 23:16:42, miu wrote: > Like above, this math could break if 1000 is not evenly divisible by > |frame_duration|. Done. https://codereview.chromium.org/1406113002/diff/440001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:206: // called from the dtor (main thread) or from OnSetForamt (render thread); On 2015/10/29 22:58:21, mcasas wrote: > nit: OnSetFormat() Done. https://codereview.chromium.org/1406113002/diff/440001/content/renderer/media... File content/renderer/media/audio_track_recorder_unittest.cc (right): https://codereview.chromium.org/1406113002/diff/440001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:80: // two channels On 2015/10/29 22:58:21, mcasas wrote: > Nit: remove this comment and with that, also the curly brackets. Done. https://codereview.chromium.org/1406113002/diff/440001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:105: media::AudioParameters params1_; On 2015/10/29 22:58:21, mcasas wrote: > nit: I think |params1|, |params2|, |mono_source|| and |stereo_source_| can > be const. media::SineWaveAudioSource doesn't like being const because of OnMoreData() not being const.
lgtm
ajose@chromium.org changed reviewers: + henrika@chromium.org
henrika@, sergeyu@: would appreciate owner's RS for content/renderer/media/DEPS change to include third_party/opus.
henrika@chromium.org changed reviewers: + tlegrand@chromium.org
LGTM (cc: tlegrand for Opus just in case).
ajose@chromium.org changed reviewers: + minyue@chromium.org - tlegrand@chromium.org
Adding minyue@ on tlegrand@'s advice. minyue@: Mind taking a look at Opus usage in AudioTrackRecorder::AudioEncoder in content/renderer/media/audio_track_recorder.cc?
On 2015/11/10 19:24:09, ajose wrote: > Adding minyue@ on tlegrand@'s advice. > > minyue@: Mind taking a look at Opus usage in AudioTrackRecorder::AudioEncoder in > content/renderer/media/audio_track_recorder.cc? Hi Thanks for the work! See my comments inline. Two slightly important points are 1. The encoder class here share very similar codes with class AudioEncoder::OpusImpl for cast. is there a chance to merge the two (or let AudioTrackRecorder use AudioEncoder::OpusImpl) 2. We do not have any use or test about decoding. I do not know if the use of std::string to store payload guarantees decodability?
https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:7: #include "content/renderer/media/audio_track_recorder.h" I know this should go before all other include as a separate line https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:11: I don't know if there should be a empty line https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:24: // Support for max sampling rate of 48KHz, 2 channels, 100 ms duration. where dose 100 ms duration come from? https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:30: static int GetOpusFrameDuration(int sample_rate) { Like it or not, the naming convention in Chrome media uses frame to mean one multi-channel sample See https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_b... What is then for a trunk of frames? I am not sure. It is usually implicit. Or you may use num_frames (or frames). I hope you follow that convention, since audio_track_recorder resides in media. https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:38: for (auto possible_duration : opus_valid_frame_durations_ms) { This says that if all are valid, we'd use the shortest, right? From compression point of view, longer frame usually gives better performance. What are we supposed to optimize for? https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:120: // Check for and destroy previous OpusEncoder, if necessary. "if necessary" seems unnecessary. We seem to delete the encoder always. https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:138: if (samples_per_frame_ <= 0 || how can "samples_per_frame_ <= 0" happen? https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:141: MAX_SAMPLES_x_CHANNELS_PER_FRAME) { is MAX_SAMPLES_x_CHANNELS_PER_FRAME a good name? the lower case x makes it look unconventional. Also consider my suggestion on "samples x channels" -> "frames" https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:163: if (opus_encoder_ctl(opus_encoder_, OPUS_SET_BITRATE(OPUS_AUTO)) != OPUS_OK) { Auto bit rate can be ok. but why not giving AudioTrackRecorder a way to set the bit rate? https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:181: // Encode all audio in |audio_bus| into zero or more frames. "frames"->"packets" https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:239: out->resize(OPUS_MAX_PAYLOAD_SIZE); Unless memory size is very critical, it would be good to fix the size of out. I don't know if this solves 547918. https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... File content/renderer/media/audio_track_recorder.h (right): https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder.h:36: scoped_ptr<std::string> encoded_data, Em.. why using a std::string for encoded_data? https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... File content/renderer/media/audio_track_recorder_unittest.cc (right): https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:42: class AudioTrackRecorderTest : public testing::Test { A slightly bigger problem is that we have no test that decodes the recording.
Thanks for the comments, minyue! > Is there a chance to merge ATR and cast::AudioEncoder? I was initially hoping to do this, but I think the owners of AudioEncoder prefer ATR to be separate. > Test decoding? Response inline. > Does std::string guarantee decodability? Do you have a theory for why it would not? https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:7: #include "content/renderer/media/audio_track_recorder.h" On 2015/11/11 13:40:10, minyue wrote: > I know this should go before all other include as a separate line Done. https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:11: On 2015/11/11 13:40:10, minyue wrote: > I don't know if there should be a empty line Done. https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:24: // Support for max sampling rate of 48KHz, 2 channels, 100 ms duration. On 2015/11/11 13:40:10, minyue wrote: > where dose 100 ms duration come from? Lifted this from cast::AudioEncoder, didn't make Opus-specific. Thanks for catching. https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:30: static int GetOpusFrameDuration(int sample_rate) { On 2015/11/11 13:40:10, minyue wrote: > Like it or not, the naming convention in Chrome media uses > > frame to mean one multi-channel sample > > See > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_b... > > What is then for a trunk of frames? I am not sure. It is usually implicit. Or > you may use num_frames (or frames). > > I hope you follow that convention, since audio_track_recorder resides in media. Good point, I had unintentionally been using both meanings of "frame" here. I've switched the "chunk of samples" definition to being called "buffer" instead (even though this is a bit misleading in its own way) to be consistent with media::AudioParameters [1]. Hopefully more consistent now. [1] https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/audio_... https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:38: for (auto possible_duration : opus_valid_frame_durations_ms) { On 2015/11/11 13:40:10, minyue wrote: > This says that if all are valid, we'd use the shortest, right? From compression > point of view, longer frame usually gives better performance. What are we > supposed to optimize for? Switched to descending. https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:120: // Check for and destroy previous OpusEncoder, if necessary. On 2015/11/11 13:40:10, minyue wrote: > "if necessary" seems unnecessary. We seem to delete the encoder always. Done. https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:138: if (samples_per_frame_ <= 0 || On 2015/11/11 13:40:10, minyue wrote: > how can "samples_per_frame_ <= 0" happen? Should be covered by params.IsValid(). https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:141: MAX_SAMPLES_x_CHANNELS_PER_FRAME) { On 2015/11/11 13:40:10, minyue wrote: > is MAX_SAMPLES_x_CHANNELS_PER_FRAME a good name? the lower case x makes it look > unconventional. > > Also consider my suggestion on "samples x channels" -> "frames" Changed to |MAX_SAMPLES_PER_BUFFER| to fit new naming scheme. https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:163: if (opus_encoder_ctl(opus_encoder_, OPUS_SET_BITRATE(OPUS_AUTO)) != OPUS_OK) { On 2015/11/11 13:40:10, minyue wrote: > Auto bit rate can be ok. but why not giving AudioTrackRecorder a way to set the > bit rate? This will likely be added in the near future. https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:181: // Encode all audio in |audio_bus| into zero or more frames. On 2015/11/11 13:40:10, minyue wrote: > "frames"->"packets" Done. https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:239: out->resize(OPUS_MAX_PAYLOAD_SIZE); On 2015/11/11 13:40:10, minyue wrote: > Unless memory size is very critical, it would be good to fix the size of out. I > don't know if this solves 547918. Could you clarify this? Won't calling out->resize(SIZE) "fix the size" in the sense that it will consistently be |SIZE|? Or are you suggesting that |out| should be passed in with a fixed, known size so that this call to resize() is unnecessary? https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... File content/renderer/media/audio_track_recorder.h (right): https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder.h:36: scoped_ptr<std::string> encoded_data, On 2015/11/11 13:40:10, minyue wrote: > Em.. why using a std::string for encoded_data? I believe this choice was made to simplify data movement across threads, as std::string owns its memory (as compared to C-style strings or base::StringPiece, which depend on external ownership). This choice was also made in VideoTrackRecorder. Do you have suggestions for an alternative? https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... File content/renderer/media/audio_track_recorder_unittest.cc (right): https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:42: class AudioTrackRecorderTest : public testing::Test { On 2015/11/11 13:40:10, minyue wrote: > A slightly bigger problem is that we have no test that decodes the recording. Would this be redundant with Opus's tests? Do you have recommendations for how to verify the data has been decoded properly? Does Opus have a "lossless encoding" mode so that the decoded data would be the same as the input?
Patchset #11 (id:480001) has been deleted
Patchset #11 (id:500001) has been deleted
Thanks for the changes! It is quite good now, just a couple of small issues. https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:163: if (opus_encoder_ctl(opus_encoder_, OPUS_SET_BITRATE(OPUS_AUTO)) != OPUS_OK) { On 2015/11/12 00:10:40, ajose wrote: > On 2015/11/11 13:40:10, minyue wrote: > > Auto bit rate can be ok. but why not giving AudioTrackRecorder a way to set > the > > bit rate? > > This will likely be added in the near future. Acknowledged. https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:239: out->resize(OPUS_MAX_PAYLOAD_SIZE); On 2015/11/12 00:10:40, ajose wrote: > On 2015/11/11 13:40:10, minyue wrote: > > Unless memory size is very critical, it would be good to fix the size of out. > I > > don't know if this solves 547918. > > Could you clarify this? Won't calling out->resize(SIZE) "fix the size" in the > sense that it will consistently be |SIZE|? Or are you suggesting that |out| > should be passed in with a fixed, known size so that this call to resize() is > unnecessary? Ah, I see why there is a confusion. I just focused on reducing resize() as said in the bug. But of course, we want to know the actual size on the returning of |out|. Without returning the actual payload size explicitly, it would be hard to circumvent using resize(). You may just leave it as it is. https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... File content/renderer/media/audio_track_recorder.h (right): https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder.h:36: scoped_ptr<std::string> encoded_data, On 2015/11/12 00:10:41, ajose wrote: > On 2015/11/11 13:40:10, minyue wrote: > > Em.. why using a std::string for encoded_data? > > I believe this choice was made to simplify data movement across threads, as > std::string owns its memory (as compared to C-style strings or > base::StringPiece, which depend on external ownership). This choice was also > made in VideoTrackRecorder. Do you have suggestions for an alternative? Yes, indeed, it is quite often used in VPx. Good, that gives me a lot of confidence. Thanks. https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... File content/renderer/media/audio_track_recorder_unittest.cc (right): https://codereview.chromium.org/1406113002/diff/460001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:42: class AudioTrackRecorderTest : public testing::Test { On 2015/11/12 00:10:41, ajose wrote: > On 2015/11/11 13:40:10, minyue wrote: > > A slightly bigger problem is that we have no test that decodes the recording. > > Would this be redundant with Opus's tests? Do you have recommendations for how > to verify the data has been decoded properly? Does Opus have a "lossless > encoding" mode so that the decoded data would be the same as the input? I think it is just fine to try to decode a stream and see if the decoder gives no error and current number of output samples. Do not need to care about how similar the output signal is to the input. I basically would like to see that the payload is decodable. https://codereview.chromium.org/1406113002/diff/520001/content/content_render... File content/content_renderer.gypi (right): https://codereview.chromium.org/1406113002/diff/520001/content/content_render... content/content_renderer.gypi:111: 'renderer/android/synchronous_compositor_filter.cc', I suppose these come from rebasing. A trick for future is that you may do rebase in separate patch sets. https://codereview.chromium.org/1406113002/diff/520001/content/renderer/media... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1406113002/diff/520001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:47: sample_rate % (sample_rate * possible_duration / 1000) == 0) { ah, my bad arithmetic. Could you give me an example that passes the first condition but not the second? https://codereview.chromium.org/1406113002/diff/520001/content/renderer/media... File content/renderer/media/audio_track_recorder_unittest.cc (right): https://codereview.chromium.org/1406113002/diff/520001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:139: TEST_F(AudioTrackRecorderTest, OnData) { also try an invalid sampling frequency and see if the result is expected.
ajose@chromium.org changed reviewers: + avi@chromium.org
avi@: mind RS LGTM content/test/BUILD.gn? Thanks for the comments, minyue! > add decoding test I talked to mcasas about this and he mentioned that we'll be exercising this in upcoming integration tests. I'll save it for there if you're alright with it. https://codereview.chromium.org/1406113002/diff/520001/content/content_render... File content/content_renderer.gypi (right): https://codereview.chromium.org/1406113002/diff/520001/content/content_render... content/content_renderer.gypi:111: 'renderer/android/synchronous_compositor_filter.cc', On 2015/11/12 16:52:31, minyue wrote: > I suppose these come from rebasing. > > A trick for future is that you may do rebase in separate patch sets. Good point. I'm in the bad habit of deleting every patch set with no comments on it, but rebases really should be separate. https://codereview.chromium.org/1406113002/diff/520001/content/renderer/media... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1406113002/diff/520001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:47: sample_rate % (sample_rate * possible_duration / 1000) == 0) { On 2015/11/12 16:52:31, minyue wrote: > ah, my bad arithmetic. Could you give me an example that passes the first > condition but not the second? My previous math was incomplete. Trying to address your comments made me understand things better :) Say we have |sample_rate| = 480000. We will first check |possible_duration| = 60. |sample_rate| * |possible_duration| % 1000 == 0, in this case, so this function would have returned 60 previously (when only the first condition existed). But this is basically just checking that |frames_per_buffer| will be an integer, where |frames_per_buffer| = |sample_rate| / (1000ms / |buffer_duration_in_ms|) = |sample_rate| * |buffer_duration_in_ms| / 1000 What we actually want to be sure of is that |sample_rate| is evenly divisible by |frames_per_buffer|. So, with a |buffer_duration| of 60, we have: |sample_rate| % |frames_per_buffer| = 48000 % 2880 = 1920 != 0. So we would return a bad |buffer_duration|. Adding in the second check prevents this. The first check is still necessary to ensure |frames_per_buffer| is an integer. https://codereview.chromium.org/1406113002/diff/520001/content/renderer/media... File content/renderer/media/audio_track_recorder_unittest.cc (right): https://codereview.chromium.org/1406113002/diff/520001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:139: TEST_F(AudioTrackRecorderTest, OnData) { On 2015/11/12 16:52:31, minyue wrote: > also try an invalid sampling frequency and see if the result is expected. I've added a comment to the test-improvement bug here: http://crbug.com/548856. In the meantime I parameterized these tests.
Still LGTM with a nit. https://codereview.chromium.org/1406113002/diff/540001/content/renderer/media... File content/renderer/media/audio_track_recorder_unittest.cc (right): https://codereview.chromium.org/1406113002/diff/540001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:56: 48000, /* sample rate */ Use |kDefaultSamplingRate|; the same in l.60 and |kDefaultSamplingRate| where applicable in these entries.
I don't think the duration calculation is right. Correct me if I am mistaken.
About decoding test, I do not know your actual plan. I think what you mean by
"integration test" includes AudioTrackRecorder, some packaging, i.e., adding
headers to the output of the decoder and packing payloads together, then some
de-packaging and then decoding. That sounds a rather complicated system. and if
anything went wrong, you may probably spend a lot of time debugging. Things that
can be easily captured by unittests should better be done in unittests.
Also, adding a decoder is trivial. Just in you
AudioTrackRecorderTest:OnEncodedAudio(...) add
EXPECT_EQ(sample_rate * duration,
opus_decode_float(decoder_, // created in ctor
data,
reinterpret_cast<uint8*>(string_as_array(encoded_data)),
encoded_data.size(), // buffer for output
out_size, // size of output buffer
0));
Also print |out| yourself, and make sure that it looks like a sine wave. No need
to write test for it.
https://codereview.chromium.org/1406113002/diff/520001/content/renderer/media...
File content/renderer/media/audio_track_recorder.cc (right):
https://codereview.chromium.org/1406113002/diff/520001/content/renderer/media...
content/renderer/media/audio_track_recorder.cc:47: sample_rate % (sample_rate *
possible_duration / 1000) == 0) {
On 2015/11/13 00:22:44, ajose wrote:
> On 2015/11/12 16:52:31, minyue wrote:
> > ah, my bad arithmetic. Could you give me an example that passes the first
> > condition but not the second?
>
> My previous math was incomplete. Trying to address your comments made me
> understand things better :) Say we have |sample_rate| = 480000. We will first
> check |possible_duration| = 60.
>
> |sample_rate| * |possible_duration| % 1000 == 0, in this case, so this
function
> would have returned 60 previously (when only the first condition existed).
>
> But this is basically just checking that |frames_per_buffer| will be an
integer,
> where |frames_per_buffer|
> = |sample_rate| / (1000ms / |buffer_duration_in_ms|)
> = |sample_rate| * |buffer_duration_in_ms| / 1000
> What we actually want to be sure of is that |sample_rate| is evenly divisible
by
> |frames_per_buffer|.
>
> So, with a |buffer_duration| of 60, we have:
> |sample_rate| % |frames_per_buffer| = 48000 % 2880 = 1920 != 0. So we would
> return a bad |buffer_duration|. Adding in the second check prevents this. The
> first check is still necessary to ensure |frames_per_buffer| is an integer.
Interesting, but why would you limit |sample_rate| / |frames_per_buffer| to be
integer?
which essentially says frame duration is divisible by 1 second, right?
60 will never pass.
Or am I mistaken?
https://codereview.chromium.org/1406113002/diff/520001/content/renderer/media...
File content/renderer/media/audio_track_recorder_unittest.cc (right):
https://codereview.chromium.org/1406113002/diff/520001/content/renderer/media...
content/renderer/media/audio_track_recorder_unittest.cc:139:
TEST_F(AudioTrackRecorderTest, OnData) {
On 2015/11/13 00:22:44, ajose wrote:
> On 2015/11/12 16:52:31, minyue wrote:
> > also try an invalid sampling frequency and see if the result is expected.
>
> I've added a comment to the test-improvement bug here:
http://crbug.com/548856.
> In the meantime I parameterized these tests.
Acknowledged.
content/test/BUILD.gn lgtm fwiw
https://codereview.chromium.org/1406113002/diff/540001/content/renderer/media... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1406113002/diff/540001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:200: capture_time); This is the wrong |capture_time|. The first sample in the buffer that was encoded would be from a prior call to EncodeAudio, which had an earlier |capture_time| argument. This is the reason for the extra code that manages the |frame_capture_time_| member in the media::cast::AudioEncoder: https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/sender/...
https://codereview.chromium.org/1406113002/diff/540001/content/renderer/media... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1406113002/diff/540001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:47: sample_rate % (sample_rate * possible_duration / 1000) == 0) { What's this second expression checking? Why is the LHS of the logical-AND not enough?
Thanks all for the comments. minyue: I added decoding to the unittest, and can verify that the output looks sinusoidal. You are also right about |sample_rate| / |frames_per_buffer| not needing to be an integer, I had been assuming this incorrectly. https://codereview.chromium.org/1406113002/diff/540001/content/renderer/media... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1406113002/diff/540001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:47: sample_rate % (sample_rate * possible_duration / 1000) == 0) { On 2015/11/13 22:08:42, miu wrote: > What's this second expression checking? Why is the LHS of the logical-AND not > enough? Thanks for clearing this up with me. I had been assuming |sample_rate| needed to be evenly divisible by |frames_per_buffer|. https://codereview.chromium.org/1406113002/diff/540001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:200: capture_time); On 2015/11/13 21:54:33, miu wrote: > This is the wrong |capture_time|. The first sample in the buffer that was > encoded would be from a prior call to EncodeAudio, which had an earlier > |capture_time| argument. > > This is the reason for the extra code that manages the |frame_capture_time_| > member in the media::cast::AudioEncoder: > https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/sender/... Done. https://codereview.chromium.org/1406113002/diff/540001/content/renderer/media... File content/renderer/media/audio_track_recorder_unittest.cc (right): https://codereview.chromium.org/1406113002/diff/540001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:56: 48000, /* sample rate */ On 2015/11/13 00:25:46, mcasas wrote: > Use |kDefaultSamplingRate|; the same > in l.60 and |kDefaultSamplingRate| where > applicable in these entries. Done.
Hi, It is quite there. Just a couple of issues on the changes to the test. https://codereview.chromium.org/1406113002/diff/560001/content/renderer/media... File content/renderer/media/audio_track_recorder.h (right): https://codereview.chromium.org/1406113002/diff/560001/content/renderer/media... content/renderer/media/audio_track_recorder.h:48: int BufferDurationForTesting(int sample_rate); You probably don't need this function. here can be 3 options 1. use |buffer_duration_| or |frames_per_buffer_|. AudioTrackRecorderTest is a friend and should be able to access it. 2. Make GetOpusBufferDuration a member method. 3. Copy GetOpusBufferDuration to the test https://codereview.chromium.org/1406113002/diff/560001/content/renderer/media... File content/renderer/media/audio_track_recorder_unittest.cc (right): https://codereview.chromium.org/1406113002/diff/560001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:18: #include "third_party/opus/src/include/opus.h" move up before WebKit/public/web/WebHeap... https://codereview.chromium.org/1406113002/diff/560001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:94: decoder_memory_( I think the use of decoder_memory_ is not necessary, you can possibly opus_decoder_(opus_decoder_create(...)) https://codereview.chromium.org/1406113002/diff/560001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:147: if (params.Equals(first_params_)) { I hope you add a function to reset decoder so that you can test what to do after format change. That can illustrate what to do if one reset encoder. https://codereview.chromium.org/1406113002/diff/560001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:231: audio_track_recorder_->OnSetFormat(second_params_); After this, call a decoder reset that I proposed.
Thanks for the comments, PTAL https://codereview.chromium.org/1406113002/diff/560001/content/renderer/media... File content/renderer/media/audio_track_recorder.h (right): https://codereview.chromium.org/1406113002/diff/560001/content/renderer/media... content/renderer/media/audio_track_recorder.h:48: int BufferDurationForTesting(int sample_rate); On 2015/11/16 11:57:32, minyue wrote: > You probably don't need this function. > > here can be 3 options > > 1. use |buffer_duration_| or |frames_per_buffer_|. AudioTrackRecorderTest is a > friend and should be able to access it. > > 2. Make GetOpusBufferDuration a member method. > > 3. Copy GetOpusBufferDuration to the test Done. https://codereview.chromium.org/1406113002/diff/560001/content/renderer/media... File content/renderer/media/audio_track_recorder_unittest.cc (right): https://codereview.chromium.org/1406113002/diff/560001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:18: #include "third_party/opus/src/include/opus.h" On 2015/11/16 11:57:33, minyue wrote: > move up before WebKit/public/web/WebHeap... Hmm, seems the capital 'W' throws off emacs sort-lines https://codereview.chromium.org/1406113002/diff/560001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:94: decoder_memory_( On 2015/11/16 11:57:32, minyue wrote: > I think the use of decoder_memory_ is not necessary, you can possibly > > opus_decoder_(opus_decoder_create(...)) Done. https://codereview.chromium.org/1406113002/diff/560001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:147: if (params.Equals(first_params_)) { On 2015/11/16 11:57:33, minyue wrote: > I hope you add a function to reset decoder so that you can test what to do after > format change. That can illustrate what to do if one reset encoder. Done. https://codereview.chromium.org/1406113002/diff/560001/content/renderer/media... content/renderer/media/audio_track_recorder_unittest.cc:231: audio_track_recorder_->OnSetFormat(second_params_); On 2015/11/16 11:57:32, minyue wrote: > After this, call a decoder reset that I proposed. Done.
lgtm
Patchset #15 (id:600001) has been deleted
The CQ bit was checked by ajose@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org, henrika@chromium.org, avi@chromium.org, mcasas@chromium.org, minyue@chromium.org Link to the patchset: https://codereview.chromium.org/1406113002/#ps620001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1406113002/620001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1406113002/620001
The CQ bit was unchecked by ajose@chromium.org
Patchset #16 (id:640001) has been deleted
https://codereview.chromium.org/1406113002/diff/660001/content/renderer/media... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1406113002/diff/660001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:90: base::TimeTicks buffer_capture_time_; This shouldn't be a class member since there is no need to persist the value across calls to EncodeAudio(). Just make it a local variable in EncodeAudio().
Sorry miu, I'll wait for LGTM before trying to commit again. https://codereview.chromium.org/1406113002/diff/660001/content/renderer/media... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1406113002/diff/660001/content/renderer/media... content/renderer/media/audio_track_recorder.cc:90: base::TimeTicks buffer_capture_time_; On 2015/11/17 04:18:01, miu wrote: > This shouldn't be a class member since there is no need to persist the value > across calls to EncodeAudio(). Just make it a local variable in EncodeAudio(). Done.
lgtm
The CQ bit was checked by ajose@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from minyue@chromium.org, avi@chromium.org, mcasas@chromium.org, henrika@chromium.org Link to the patchset: https://codereview.chromium.org/1406113002/#ps680001 (title: "miu's comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1406113002/680001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1406113002/680001
Message was sent while issue was closed.
Committed patchset #17 (id:680001)
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/9b6d878b661f617cdbfc027a8c9b9b444aab948c Cr-Commit-Position: refs/heads/master@{#360108} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
