Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1407083006:
Update MediaRecorderHandler to use AudioTrackRecorder (Closed)
DescriptionUpdate MediaRecorderHandler to use AudioTrackRecorder
This is the third 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/a664ea923f135c74b9494acb36ee524e4c9f8448
Cr-Commit-Position: refs/heads/master@{#360704}
Patch Set 1 : #Patch Set 2 : add layout tests #
Total comments: 23
Patch Set 3 : remove layouttests #Patch Set 4 : get tests passing #Patch Set 5 : trybots #
Total comments: 16
Patch Set 6 : mcasas@'s comments #
Total comments: 2
Patch Set 7 : mcasas@'s comments #
Messages
Total messages: 22 (9 generated)
Patchset #1 (id:1) has been deleted
ajose@chromium.org changed reviewers: + mcasas@chromium.org
Worth a first look?
Some comments. Nothing major. https://codereview.chromium.org/1407083006/diff/40001/content/public/renderer... File content/public/renderer/media_stream_api.cc (right): https://codereview.chromium.org/1407083006/diff/40001/content/public/renderer... content/public/renderer/media_stream_api.cc:80: // params, ? https://codereview.chromium.org/1407083006/diff/40001/content/public/renderer... content/public/renderer/media_stream_api.cc:97: 48000, // audio_config.rtp_timebase, // sampling rate All these comments "confuse and scare me" :) I think the style would prefer sth like: 48000 /* param_name */ or 48000, // Explanation of the value (two spaces bw ',' and '//') Since this is testing infra, there'd be no need for extracting constants though. https://codereview.chromium.org/1407083006/diff/40001/content/public/renderer... File content/public/renderer/media_stream_api.h (right): https://codereview.chromium.org/1407083006/diff/40001/content/public/renderer... content/public/renderer/media_stream_api.h:48: // const media::AudioParameters& params, ? https://codereview.chromium.org/1407083006/diff/40001/content/renderer/media/... File content/renderer/media/media_recorder_handler.cc (right): https://codereview.chromium.org/1407083006/diff/40001/content/renderer/media/... content/renderer/media/media_recorder_handler.cc:187: } No {} https://codereview.chromium.org/1407083006/diff/40001/content/renderer/media/... File content/renderer/media/media_recorder_handler_unittest.cc (right): https://codereview.chromium.org/1407083006/diff/40001/content/renderer/media/... content/renderer/media/media_recorder_handler_unittest.cc:147: EXPECT_FALSE(hasAudioRecorders()); Why not EXPECT_TRUE()? Oh I see, the |kMediaRecorderTestParams| is only Video. Please rename accordingly both the array and the test, or extend them. https://codereview.chromium.org/1407083006/diff/40001/content/renderer/media/... content/renderer/media/media_recorder_handler_unittest.cc:222: // media::VideoFrame::CreateBlackFrame(gfx::Size(160, 80)); ? https://codereview.chromium.org/1407083006/diff/40001/content/renderer/media/... File content/renderer/media/mock_media_stream_registry.cc (right): https://codereview.chromium.org/1407083006/diff/40001/content/renderer/media/... content/renderer/media/mock_media_stream_registry.cc:61: // TODO: CONSIDER CHANGING THIS TO NEW WAY IN WebmMuxerTest No shouting :) https://codereview.chromium.org/1407083006/diff/40001/content/renderer/media/... content/renderer/media/mock_media_stream_registry.cc:62: StreamDeviceInfo device_info(MEDIA_DEVICE_AUDIO_CAPTURE, "Mock device", const? https://codereview.chromium.org/1407083006/diff/40001/content/shell/renderer/... File content/shell/renderer/layout_test/blink_test_runner.cc (right): https://codereview.chromium.org/1407083006/diff/40001/content/shell/renderer/... content/shell/renderer/layout_test/blink_test_runner.cc:250: // audio_params_ = params; clean. https://codereview.chromium.org/1407083006/diff/40001/content/shell/renderer/... content/shell/renderer/layout_test/blink_test_runner.cc:252: LOG(INFO) << "params.AsHumanReadableString()"; Sure you don't mean: LOG(INFO) << "params: " << params.AsHumanReadableString(); ? https://codereview.chromium.org/1407083006/diff/40001/content/shell/renderer/... content/shell/renderer/layout_test/blink_test_runner.cc:785: // media::AudioParameters(), Clean up this plz. https://codereview.chromium.org/1407083006/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-basic-audio.html (right): https://codereview.chromium.org/1407083006/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-basic-audio.html:62: console.log("test?"); ? https://codereview.chromium.org/1407083006/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType-expected.txt (right): https://codereview.chromium.org/1407083006/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType-expected.txt:6: PASS check MediaRecorder.canRecordMimeType() with audio/opus Actually when a test passes 100%, there's no need for the -expected.txt, so you can remove this file.
Patchset #3 (id:60001) has been deleted
Patchset #4 (id:100001) has been deleted
Sorry for the long delay. https://codereview.chromium.org/1407083006/diff/40001/content/renderer/media/... File content/renderer/media/media_recorder_handler.cc (right): https://codereview.chromium.org/1407083006/diff/40001/content/renderer/media/... content/renderer/media/media_recorder_handler.cc:187: } On 2015/11/10 18:18:58, mcasas wrote: > No {} Done. https://codereview.chromium.org/1407083006/diff/40001/content/renderer/media/... File content/renderer/media/media_recorder_handler_unittest.cc (right): https://codereview.chromium.org/1407083006/diff/40001/content/renderer/media/... content/renderer/media/media_recorder_handler_unittest.cc:147: EXPECT_FALSE(hasAudioRecorders()); On 2015/11/10 18:18:59, mcasas wrote: > Why not EXPECT_TRUE()? > Oh I see, the |kMediaRecorderTestParams| is only Video. Please > rename accordingly both the array and the test, or extend them. Done. https://codereview.chromium.org/1407083006/diff/40001/content/renderer/media/... content/renderer/media/media_recorder_handler_unittest.cc:222: // media::VideoFrame::CreateBlackFrame(gfx::Size(160, 80)); On 2015/11/10 18:18:58, mcasas wrote: > ? Done. https://codereview.chromium.org/1407083006/diff/40001/content/renderer/media/... File content/renderer/media/mock_media_stream_registry.cc (right): https://codereview.chromium.org/1407083006/diff/40001/content/renderer/media/... content/renderer/media/mock_media_stream_registry.cc:61: // TODO: CONSIDER CHANGING THIS TO NEW WAY IN WebmMuxerTest On 2015/11/10 18:18:59, mcasas wrote: > No shouting :) FINE https://codereview.chromium.org/1407083006/diff/40001/content/renderer/media/... content/renderer/media/mock_media_stream_registry.cc:62: StreamDeviceInfo device_info(MEDIA_DEVICE_AUDIO_CAPTURE, "Mock device", On 2015/11/10 18:18:59, mcasas wrote: > const? Done. https://codereview.chromium.org/1407083006/diff/40001/content/shell/renderer/... File content/shell/renderer/layout_test/blink_test_runner.cc (right): https://codereview.chromium.org/1407083006/diff/40001/content/shell/renderer/... content/shell/renderer/layout_test/blink_test_runner.cc:250: // audio_params_ = params; On 2015/11/10 18:18:59, mcasas wrote: > clean. Done. https://codereview.chromium.org/1407083006/diff/40001/content/shell/renderer/... content/shell/renderer/layout_test/blink_test_runner.cc:252: LOG(INFO) << "params.AsHumanReadableString()"; On 2015/11/10 18:18:59, mcasas wrote: > Sure you don't mean: > LOG(INFO) << "params: " << params.AsHumanReadableString(); > ? Done. https://codereview.chromium.org/1407083006/diff/40001/content/shell/renderer/... content/shell/renderer/layout_test/blink_test_runner.cc:785: // media::AudioParameters(), On 2015/11/10 18:18:59, mcasas wrote: > Clean up this plz. Done. https://codereview.chromium.org/1407083006/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-basic-audio.html (right): https://codereview.chromium.org/1407083006/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-basic-audio.html:62: console.log("test?"); On 2015/11/10 18:18:59, mcasas wrote: > ? Done. https://codereview.chromium.org/1407083006/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType-expected.txt (right): https://codereview.chromium.org/1407083006/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType-expected.txt:6: PASS check MediaRecorder.canRecordMimeType() with audio/opus On 2015/11/10 18:18:59, mcasas wrote: > Actually when a test passes 100%, there's no need > for the -expected.txt, so you can remove this file. Awesome.
Patchset #5 (id:140001) has been deleted
almost there. https://codereview.chromium.org/1407083006/diff/160001/content/renderer/media... File content/renderer/media/media_recorder_handler.cc (right): https://codereview.chromium.org/1407083006/diff/160001/content/renderer/media... content/renderer/media/media_recorder_handler.cc:45: // TODO(ajose): Not sure about some of these... See my comment below. https://codereview.chromium.org/1407083006/diff/160001/content/renderer/media... content/renderer/media/media_recorder_handler.cc:49: mimeType.utf8().compare("video/opus") == 0 || "video/opus" ? Also, what if MIME was "video/vp8,audio/opus", would it pass this filter? https://codereview.chromium.org/1407083006/diff/160001/content/renderer/media... File content/renderer/media/media_recorder_handler_unittest.cc (right): https://codereview.chromium.org/1407083006/diff/160001/content/renderer/media... content/renderer/media/media_recorder_handler_unittest.cc:131: TEST_P(MediaRecorderHandlerTest, CanSupportMimeType) { Hmm this should not be parameterised, right? Keep it as TEST_F. https://codereview.chromium.org/1407083006/diff/160001/content/renderer/media... content/renderer/media/media_recorder_handler_unittest.cc:146: } Guess this test should at least try an OK combination, e.g. "video/vp8,audio/opus". https://codereview.chromium.org/1407083006/diff/160001/content/renderer/media... content/renderer/media/media_recorder_handler_unittest.cc:164: EXPECT_FALSE(hasVideoRecorders()); EXPECT_TRUE(hasVideoRecorders() || !GetParam().has_video); ? Same below. https://codereview.chromium.org/1407083006/diff/160001/content/renderer/media... content/renderer/media/media_recorder_handler_unittest.cc:184: if (GetParam().has_audio) { no {} https://codereview.chromium.org/1407083006/diff/160001/content/renderer/media... content/renderer/media/media_recorder_handler_unittest.cc:245: } no {} https://codereview.chromium.org/1407083006/diff/160001/content/renderer/media... File content/renderer/media/mock_media_stream_registry.cc (right): https://codereview.chromium.org/1407083006/diff/160001/content/renderer/media... content/renderer/media/mock_media_stream_registry.cc:75: nullptr)); nit: add the parameter names in a comment /**/ for those few that are hardcoded. (l.455 should do that too...)
Patchset #6 (id:180001) has been deleted
Thanks for the comments https://codereview.chromium.org/1407083006/diff/160001/content/renderer/media... File content/renderer/media/media_recorder_handler.cc (right): https://codereview.chromium.org/1407083006/diff/160001/content/renderer/media... content/renderer/media/media_recorder_handler.cc:45: // TODO(ajose): Not sure about some of these... On 2015/11/19 00:49:52, mcasas wrote: > See my comment below. Done. https://codereview.chromium.org/1407083006/diff/160001/content/renderer/media... content/renderer/media/media_recorder_handler.cc:49: mimeType.utf8().compare("video/opus") == 0 || On 2015/11/19 00:49:52, mcasas wrote: > "video/opus" ? > > Also, what if MIME was "video/vp8,audio/opus", would it pass > this filter? Changed to use tokenizer. https://codereview.chromium.org/1407083006/diff/160001/content/renderer/media... File content/renderer/media/media_recorder_handler_unittest.cc (right): https://codereview.chromium.org/1407083006/diff/160001/content/renderer/media... content/renderer/media/media_recorder_handler_unittest.cc:131: TEST_P(MediaRecorderHandlerTest, CanSupportMimeType) { On 2015/11/19 00:49:52, mcasas wrote: > Hmm this should not be parameterised, right? > Keep it as TEST_F. Done. https://codereview.chromium.org/1407083006/diff/160001/content/renderer/media... content/renderer/media/media_recorder_handler_unittest.cc:146: } On 2015/11/19 00:49:52, mcasas wrote: > Guess this test should at least try an OK combination, > e.g. "video/vp8,audio/opus". Done. https://codereview.chromium.org/1407083006/diff/160001/content/renderer/media... content/renderer/media/media_recorder_handler_unittest.cc:164: EXPECT_FALSE(hasVideoRecorders()); On 2015/11/19 00:49:52, mcasas wrote: > EXPECT_TRUE(hasVideoRecorders() || !GetParam().has_video); > ? > > Same below. Done. https://codereview.chromium.org/1407083006/diff/160001/content/renderer/media... content/renderer/media/media_recorder_handler_unittest.cc:184: if (GetParam().has_audio) { On 2015/11/19 00:49:52, mcasas wrote: > no {} Done. https://codereview.chromium.org/1407083006/diff/160001/content/renderer/media... content/renderer/media/media_recorder_handler_unittest.cc:245: } On 2015/11/19 00:49:52, mcasas wrote: > no {} Done. https://codereview.chromium.org/1407083006/diff/160001/content/renderer/media... File content/renderer/media/mock_media_stream_registry.cc (right): https://codereview.chromium.org/1407083006/diff/160001/content/renderer/media... content/renderer/media/mock_media_stream_registry.cc:75: nullptr)); On 2015/11/19 00:49:52, mcasas wrote: > nit: add the parameter names in a comment /**/ for those > few that are hardcoded. (l.455 should do that too...) Done.
LGTM w/ suggestion https://codereview.chromium.org/1407083006/diff/200001/content/renderer/media... File content/renderer/media/media_recorder_handler.cc (right): https://codereview.chromium.org/1407083006/diff/200001/content/renderer/media... content/renderer/media/media_recorder_handler.cc:55: token.compare("video/vp8") != 0 && what about if (base::EqualsCaseInsensitiveASCII(token, "video/vp8") || ... ) return true; (positive-logic, early out, also case-insensitive).
https://codereview.chromium.org/1407083006/diff/200001/content/renderer/media... File content/renderer/media/media_recorder_handler.cc (right): https://codereview.chromium.org/1407083006/diff/200001/content/renderer/media... content/renderer/media/media_recorder_handler.cc:55: token.compare("video/vp8") != 0 && On 2015/11/19 22:44:00, mcasas wrote: > what about > if (base::EqualsCaseInsensitiveASCII(token, "video/vp8") || ... ) > return true; > > (positive-logic, early out, also case-insensitive). But what if another one of the tokens is unsupported? I don't think we can return true until we've looked at all of them. I could add a flag or something if you'd like.
On 2015/11/19 22:59:01, ajose wrote: > https://codereview.chromium.org/1407083006/diff/200001/content/renderer/media... > File content/renderer/media/media_recorder_handler.cc (right): > > https://codereview.chromium.org/1407083006/diff/200001/content/renderer/media... > content/renderer/media/media_recorder_handler.cc:55: token.compare("video/vp8") > != 0 && > On 2015/11/19 22:44:00, mcasas wrote: > > what about > > if (base::EqualsCaseInsensitiveASCII(token, "video/vp8") || ... ) > > return true; > > > > (positive-logic, early out, also case-insensitive). > > But what if another one of the tokens is unsupported? I don't think we can > return true until we've looked at all of them. I could add a flag or something > if you'd like. still LGTM.
The CQ bit was checked by ajose@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1407083006/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1407083006/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) ios_rel_device_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1407083006/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1407083006/220001
Message was sent while issue was closed.
Committed patchset #7 (id:220001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/a664ea923f135c74b9494acb36ee524e4c9f8448 Cr-Commit-Position: refs/heads/master@{#360704} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
