Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2753543010:
WebRTC: Use the MediaStream Recording API for the audio_quality_browsertest. (Closed)
|
Created:
3 years, 9 months ago by ehmaldonado_chromium Modified:
3 years, 9 months ago CC:
chromium-reviews, chfremer+watch_chromium.org, phoglund+watch_chromium.org, feature-media-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionWebRTC: Use the MediaStream Recording API for the audio_quality_browsertest.
This CL uses the MediaStream Recording API to record the audio received
by the right tag. It saves it as a webm file that is later converted to a
wav file using ffmpeg.
This might cause a performance regression because we're using webm+ffmpeg
to generate the wav file. I set a, hopefully high enough, bitrate to
mitigate this
BUG=chromium:681577
NOTRY=True
Review-Url: https://codereview.chromium.org/2753543010
Cr-Commit-Position: refs/heads/master@{#459050}
Committed: https://chromium.googlesource.com/chromium/src/+/a0a47bfebb016ec468bc6d1b4be9b1acb22a4075
Patch Set 1 #
Total comments: 9
Patch Set 2 : Address comments. #
Total comments: 1
Patch Set 3 : Addressed comments. #
Total comments: 1
Patch Set 4 : Removed obsolete comments. #
Total comments: 10
Patch Set 5 : Download recording to file. #
Total comments: 3
Patch Set 6 : Addressed comments. #
Total comments: 1
Patch Set 7 : Addressed Comments. #Patch Set 8 : Fix windows issues. #
Messages
Total messages: 42 (18 generated)
ehmaldonado@chromium.org changed reviewers: + kjellander@chromium.org
Please take a look. Do you know who else can review this?
ehmaldonado@chromium.org changed reviewers: + hbos@webrtc.org
hbos: Are you the right person to review this?
hbos@chromium.org changed reviewers: + hbos@chromium.org
On 2017/03/17 15:17:46, ehmaldonado_chromium wrote: > hbos: Are you the right person to review this? I am not familiar with the old or new binary and their command line options, or the MediaRecorder API being used, but I am familiar with browser_tests and it LGTM. It might be appropriate to clean up webrtc_audio_quality_browsertest.cc a bit to put ExecuteJavascript calls into helper functions similar to how https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/webrtc_brows... contains helper functions for doing ExecuteJavascript, but I think that is beyond the scope of this CL which follows the existing pattern. https://codereview.chromium.org/2753543010/diff/1/chrome/test/data/webrtc/web... File chrome/test/data/webrtc/webrtc_audio_quality_test.html (right): https://codereview.chromium.org/2753543010/diff/1/chrome/test/data/webrtc/web... chrome/test/data/webrtc/webrtc_audio_quality_test.html:19: <td><audio id="remote-view" autoplay onplay="startAudioCapture(25)"> This makes any audio quality test (loading this .html and) receiving audio be captured. It feels a bit "implicit" to me to put it in the .html since all other non-layout logic is in .cc and .js files. Could we make it an "explicit" call to invoke startAudioCapture(arg) from MAYBE_WebRtcAudioQualityBrowserTest::SetupAndRecordAudioCall instead unless this approach has benefits I'm missing?
Description was changed from ========== WebRTC: Use the MediaStream Recording API for the audio_quality_browsertest. This CL uses the MediaStream Recording API to record the audio received by the right tag. It saves it as a webm file that is later converted to a wav file using ffmpeg. This might cause a performance regression because we're using webm+ffmpeg to generate the wav file. I set a, hopefully high enough, bitrate to mitigate this BUG=chromium:681577 ========== to ========== WebRTC: Use the MediaStream Recording API for the audio_quality_browsertest. This CL uses the MediaStream Recording API to record the audio received by the right tag. It saves it as a webm file that is later converted to a wav file using ffmpeg. This might cause a performance regression because we're using webm+ffmpeg to generate the wav file. I set a, hopefully high enough, bitrate to mitigate this BUG=chromium:681577 ==========
hbos@chromium.org changed reviewers: - hbos@webrtc.org
Sorry for the delay https://codereview.chromium.org/2753543010/diff/1/chrome/browser/media/webrtc... File chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc (left): https://codereview.chromium.org/2753543010/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc:289: FILE_PATH_LITERAL("force_mic_volume_max.exe"))); I guess you can drop this file as well in this CL: https://cs.chromium.org/chromium/src/chrome/test/data/webrtc/resources/force_... https://codereview.chromium.org/2753543010/diff/1/chrome/browser/media/webrtc... File chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc (right): https://codereview.chromium.org/2753543010/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc:93: // 5. (For Chrome bots) Ensure sox and rec are reachable from the env the test Update this to remove the 'rec' parts. https://codereview.chromium.org/2753543010/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc:183: base::CommandLine MakeSoxCommandLine() { Can you talk to Oleh about this? Since he's written code about removing silence in https://codereview.webrtc.org/2694203002/. Ideally we'd create a stand-alone binary that can be used instead, then we could use it in both places. https://codereview.chromium.org/2753543010/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc:241: // Runs ffmpeg on the captured webm video and writes it to a yuv video file. To a Wave file, right? https://codereview.chromium.org/2753543010/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc:250: // Set up ffmpeg to output at a certain resolution (-s) and bitrate (-b:v). Update this comment, you're no longer passing -s and -b:v as in https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/webrtc_video... https://codereview.chromium.org/2753543010/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc:607: DVLOG(0) << "Started recording to " << recording.value() << std::endl; This is duplicated with line 541-547. Can you move into a helper function? https://codereview.chromium.org/2753543010/diff/1/chrome/test/data/webrtc/web... File chrome/test/data/webrtc/webrtc_audio_quality_test.html (right): https://codereview.chromium.org/2753543010/diff/1/chrome/test/data/webrtc/web... chrome/test/data/webrtc/webrtc_audio_quality_test.html:19: <td><audio id="remote-view" autoplay onplay="startAudioCapture(25)"> On 2017/03/20 11:08:21, hbos_chromium wrote: > This makes any audio quality test (loading this .html and) receiving audio be > captured. It feels a bit "implicit" to me to put it in the .html since all other > non-layout logic is in .cc and .js files. > > Could we make it an "explicit" call to invoke startAudioCapture(arg) from > MAYBE_WebRtcAudioQualityBrowserTest::SetupAndRecordAudioCall instead unless this > approach has benefits I'm missing? It sounds like a good idea, but not strictly necessary as this file is only used by https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/webrtc_audio... so I think it's fine. If other tests wants to use it as well, they could to that job at that point.
On 2017/03/21 11:09:01, kjellander_chromium wrote: > Sorry for the delay > > https://codereview.chromium.org/2753543010/diff/1/chrome/browser/media/webrtc... > File chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc (left): > > https://codereview.chromium.org/2753543010/diff/1/chrome/browser/media/webrtc... > chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc:289: > FILE_PATH_LITERAL("force_mic_volume_max.exe"))); > I guess you can drop this file as well in this CL: > https://cs.chromium.org/chromium/src/chrome/test/data/webrtc/resources/force_... > > https://codereview.chromium.org/2753543010/diff/1/chrome/browser/media/webrtc... > File chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc (right): > > https://codereview.chromium.org/2753543010/diff/1/chrome/browser/media/webrtc... > chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc:93: // 5. (For > Chrome bots) Ensure sox and rec are reachable from the env the test > Update this to remove the 'rec' parts. > > https://codereview.chromium.org/2753543010/diff/1/chrome/browser/media/webrtc... > chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc:183: > base::CommandLine MakeSoxCommandLine() { > Can you talk to Oleh about this? Since he's written code about removing silence > in https://codereview.webrtc.org/2694203002/. > Ideally we'd create a stand-alone binary that can be used instead, then we could > use it in both places. > > https://codereview.chromium.org/2753543010/diff/1/chrome/browser/media/webrtc... > chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc:241: // Runs > ffmpeg on the captured webm video and writes it to a yuv video file. > To a Wave file, right? > > https://codereview.chromium.org/2753543010/diff/1/chrome/browser/media/webrtc... > chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc:250: // Set up > ffmpeg to output at a certain resolution (-s) and bitrate (-b:v). > Update this comment, you're no longer passing -s and -b:v as in > https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/webrtc_video... > > https://codereview.chromium.org/2753543010/diff/1/chrome/browser/media/webrtc... > chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc:607: DVLOG(0) << > "Started recording to " << recording.value() << std::endl; > This is duplicated with line 541-547. Can you move into a helper function? > > https://codereview.chromium.org/2753543010/diff/1/chrome/test/data/webrtc/web... > File chrome/test/data/webrtc/webrtc_audio_quality_test.html (right): > > https://codereview.chromium.org/2753543010/diff/1/chrome/test/data/webrtc/web... > chrome/test/data/webrtc/webrtc_audio_quality_test.html:19: <td><audio > id="remote-view" autoplay onplay="startAudioCapture(25)"> > On 2017/03/20 11:08:21, hbos_chromium wrote: > > This makes any audio quality test (loading this .html and) receiving audio be > > captured. It feels a bit "implicit" to me to put it in the .html since all > other > > non-layout logic is in .cc and .js files. > > > > Could we make it an "explicit" call to invoke startAudioCapture(arg) from > > MAYBE_WebRtcAudioQualityBrowserTest::SetupAndRecordAudioCall instead unless > this > > approach has benefits I'm missing? > > It sounds like a good idea, but not strictly necessary as this file is only used > by > https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/webrtc_audio... > so I think it's fine. If other tests wants to use it as well, they could to that > job at that point. PTAL
https://codereview.chromium.org/2753543010/diff/1/chrome/browser/media/webrtc... File chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc (right): https://codereview.chromium.org/2753543010/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc:93: // 5. (For Chrome bots) Ensure sox and rec are reachable from the env the test On 2017/03/21 11:09:00, kjellander_chromium wrote: > Update this to remove the 'rec' parts. You missed this one. rec is no longer needed. https://codereview.chromium.org/2753543010/diff/20001/chrome/browser/media/we... File chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc (right): https://codereview.chromium.org/2753543010/diff/20001/chrome/browser/media/we... chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc:538: int polling_interval_msec = 1000; Move into a constant in the fixture instead?
> https://codereview.chromium.org/2753543010/diff/20001/chrome/browser/media/we... > File chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc (right): > > https://codereview.chromium.org/2753543010/diff/20001/chrome/browser/media/we... > chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc:538: int > polling_interval_msec = 1000; > Move into a constant in the fixture instead? PTAL. I added it as just a constant.
lgtm with my comment addressed. You might want to have another reviewer for the MediaRecorder stuff though. https://codereview.chromium.org/2753543010/diff/40001/chrome/browser/media/we... File chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc (right): https://codereview.chromium.org/2753543010/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc:99: // sudo ln -s /opt/local/bin/rec /usr/local/bin/rec Remove this line too :)
ehmaldonado@chromium.org changed reviewers: + mcasas@chromium.org
Hi Miguel, are you the right person to review the usage of MediaStream in this CL?
https://codereview.chromium.org/2753543010/diff/60001/chrome/browser/media/we... File chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc (right): https://codereview.chromium.org/2753543010/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc:161: const base::FilePath& recording, |recording| is confusing, what about s/recording/record_output_path/ or similar? https://codereview.chromium.org/2753543010/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc:232: base::FilePath path_to_ffmpeg = test::GetToolForPlatform("ffmpeg"); nit: const here and in l.252, l.486, l.493, 495 etc. Essentially everything not being modified, plz make const. https://codereview.chromium.org/2753543010/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc:543: DVLOG(0) << "Done recording to " << recording.value() << std::endl; s/recording.value()/recording.MaybeAsASCII()/ and you don't need std::endl; Same in l.601. https://codereview.chromium.org/2753543010/diff/60001/chrome/test/data/webrtc... File chrome/test/data/webrtc/audio_extraction.js (right): https://codereview.chromium.org/2753543010/diff/60001/chrome/test/data/webrtc... chrome/test/data/webrtc/audio_extraction.js:2: * Copyright (c) 2017 The Chromium Authors. All rights reserved. No (c): https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2753543010/diff/60001/chrome/test/data/webrtc... chrome/test/data/webrtc/audio_extraction.js:11: var gAudioBitsPerSecond = 3000000; gVarName for global variables is atypical in javascript-in-Chromium. Instead, use NAMES_LIKE_THIS for such variables [1] and make them const (since here we don't have to think about Internet Explorer). [1] https://google.github.io/styleguide/javascriptguide.xml?showone=Constants#Con... https://codereview.chromium.org/2753543010/diff/60001/chrome/test/data/webrtc... chrome/test/data/webrtc/audio_extraction.js:17: var gCaptureDuration = 0; s/gCaptureDuration/captureDurationMs/ but for its usage below, I think we don't need a file-scope variable and you can just define it in l.52 as const capture_duration_ms = duration * 1000 /* ms per second */;; https://codereview.chromium.org/2753543010/diff/60001/chrome/test/data/webrtc... chrome/test/data/webrtc/audio_extraction.js:23: var gAudioBase64 = ''; s/gAudioBase64/audio_as_base64/ This can be a huge string (slow test, flaky?), and anyway you'll be writing it to disk in the .cpp file, so perhaps you could consider writing it to disk directly (might need some tweaking to the test http server). https://codereview.chromium.org/2753543010/diff/60001/chrome/test/data/webrtc... chrome/test/data/webrtc/audio_extraction.js:41: function startAudioCapture(duration) { s/duration/duration_seconds/ or similar? https://codereview.chromium.org/2753543010/diff/60001/chrome/test/data/webrtc... chrome/test/data/webrtc/audio_extraction.js:102: } It's pervasive in Chromium to not have {} when the bodies are one-liners. Haven't found the equivalent in the JS style guide, but I think it's reasonable, plz consider it.
I'll address the other comments shortly :) https://codereview.chromium.org/2753543010/diff/60001/chrome/test/data/webrtc... File chrome/test/data/webrtc/audio_extraction.js (right): https://codereview.chromium.org/2753543010/diff/60001/chrome/test/data/webrtc... chrome/test/data/webrtc/audio_extraction.js:23: var gAudioBase64 = ''; On 2017/03/21 16:54:22, mcasas wrote: > s/gAudioBase64/audio_as_base64/ > > This can be a huge string (slow test, flaky?), and > anyway you'll be writing it to disk in the .cpp file, > so perhaps you could consider writing it to disk > directly (might need some tweaking to the test > http server). Are there any examples of this already?
On 2017/03/21 16:59:55, ehmaldonado_chromium wrote: > I'll address the other comments shortly :) > > https://codereview.chromium.org/2753543010/diff/60001/chrome/test/data/webrtc... > File chrome/test/data/webrtc/audio_extraction.js (right): > > https://codereview.chromium.org/2753543010/diff/60001/chrome/test/data/webrtc... > chrome/test/data/webrtc/audio_extraction.js:23: var gAudioBase64 = ''; > On 2017/03/21 16:54:22, mcasas wrote: > > s/gAudioBase64/audio_as_base64/ > > > > This can be a huge string (slow test, flaky?), and > > anyway you'll be writing it to disk in the .cpp file, > > so perhaps you could consider writing it to disk > > directly (might need some tweaking to the test > > http server). > > Are there any examples of this already? Yeah! The second example in the Spec [1] links to the function download() in Web Fundamentals entry [2], also implemented in my demo [3] and in the codepen [4]. [1] https://w3c.github.io/mediacapture-record/MediaRecorder.html#example2 [2] https://developers.google.com/web/updates/2016/01/mediarecorder [3] https://github.com/miguelao/demos/blob/master/mediarecorder.html#L99 [4] http://codepen.io/miguelao/pen/bZPKwP?editors=0010
On 2017/03/21 17:10:14, mcasas wrote: > On 2017/03/21 16:59:55, ehmaldonado_chromium wrote: > > I'll address the other comments shortly :) > > > > > https://codereview.chromium.org/2753543010/diff/60001/chrome/test/data/webrtc... > > File chrome/test/data/webrtc/audio_extraction.js (right): > > > > > https://codereview.chromium.org/2753543010/diff/60001/chrome/test/data/webrtc... > > chrome/test/data/webrtc/audio_extraction.js:23: var gAudioBase64 = ''; > > On 2017/03/21 16:54:22, mcasas wrote: > > > s/gAudioBase64/audio_as_base64/ > > > > > > This can be a huge string (slow test, flaky?), and > > > anyway you'll be writing it to disk in the .cpp file, > > > so perhaps you could consider writing it to disk > > > directly (might need some tweaking to the test > > > http server). > > > > Are there any examples of this already? > > Yeah! The second example in the Spec [1] links to > the function download() in Web Fundamentals entry [2], > also implemented in my demo [3] and in the codepen [4]. > > [1] https://w3c.github.io/mediacapture-record/MediaRecorder.html#example2 > [2] https://developers.google.com/web/updates/2016/01/mediarecorder > [3] https://github.com/miguelao/demos/blob/master/mediarecorder.html#L99 > [4] http://codepen.io/miguelao/pen/bZPKwP?editors=0010 PTAL :)
lgtm with some comments/suggestions. https://codereview.chromium.org/2753543010/diff/80001/chrome/browser/media/we... File chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc (right): https://codereview.chromium.org/2753543010/diff/80001/chrome/browser/media/we... chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc:9: #include "base/base64.h" Probably not needed anymore. https://codereview.chromium.org/2753543010/diff/80001/chrome/test/data/webrtc... File chrome/test/data/webrtc/audio_extraction.js (right): https://codereview.chromium.org/2753543010/diff/80001/chrome/test/data/webrtc... chrome/test/data/webrtc/audio_extraction.js:13: var CAPTURE_STATUS = 'not-started'; This is a true variable, so s/CAPTURE_STATUS/captureStatus/ https://codereview.chromium.org/2753543010/diff/80001/chrome/test/data/webrtc... chrome/test/data/webrtc/audio_extraction.js:62: returnToTest(CAPTURE_STATUS); getCaptureStatus() uses only compare the returned value with 'done-capturing', and usually having a string around makes the test hard to maintain in the long run (because the code has hidden data dependencies). I suggest changing CAPTURE_STATUS (or captureStatus) to a boolean usage, renaming it to e.g. doneRecording or similar, and s/getCaptureStatus()/isRecordingFinished()/ .
PTAL. I had to modify a couple of other files.
https://codereview.chromium.org/2753543010/diff/100001/chrome/browser/media/w... File chrome/browser/media/webrtc/webrtc_browsertest_common.cc (right): https://codereview.chromium.org/2753543010/diff/100001/chrome/browser/media/w... chrome/browser/media/webrtc/webrtc_browsertest_common.cc:192: } This code is very similar to l.129-163. Either refactor the common parts or do something like : bool PollingWaitUntil(const std::string& javascript, content::WebContents* tab_contents, int poll_interval_msec) { return PollingWaitUntil(javascript, "true", tab_contents, poll_interval_msec); } (since you can't have default value for parameters in the middle of the parameter list). You might need to return doneCapturing.toString() in audio_extraction.js l.61.
PTAL I decided to just call PollingWaitUntil(..., "true", ...) directly.
On 2017/03/22 18:06:54, ehmaldonado_chromium wrote: > PTAL > I decided to just call PollingWaitUntil(..., "true", ...) directly. still lgtm, ship it !
The CQ bit was checked by ehmaldonado@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hbos@chromium.org, kjellander@chromium.org Link to the patchset: https://codereview.chromium.org/2753543010/#ps120001 (title: "Addressed Comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
On 2017/03/22 18:49:43, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > win_clang on master.tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...) Don't forget to notify perf sheriffs when this has landed, as I imagine perf numbers might be affected.
The CQ bit was checked by ehmaldonado@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== WebRTC: Use the MediaStream Recording API for the audio_quality_browsertest. This CL uses the MediaStream Recording API to record the audio received by the right tag. It saves it as a webm file that is later converted to a wav file using ffmpeg. This might cause a performance regression because we're using webm+ffmpeg to generate the wav file. I set a, hopefully high enough, bitrate to mitigate this BUG=chromium:681577 ========== to ========== WebRTC: Use the MediaStream Recording API for the audio_quality_browsertest. This CL uses the MediaStream Recording API to record the audio received by the right tag. It saves it as a webm file that is later converted to a wav file using ffmpeg. This might cause a performance regression because we're using webm+ffmpeg to generate the wav file. I set a, hopefully high enough, bitrate to mitigate this BUG=chromium:681577 NOTRY=True ==========
The CQ bit was checked by ehmaldonado@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@chromium.org, mcasas@chromium.org, hbos@chromium.org Link to the patchset: https://codereview.chromium.org/2753543010/#ps140001 (title: "Fix windows issues.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490269871793870,
"parent_rev": "5ea4533d1c12a7cbf21c6f4471946765f73b7270", "commit_rev":
"a0a47bfebb016ec468bc6d1b4be9b1acb22a4075"}
Message was sent while issue was closed.
Description was changed from ========== WebRTC: Use the MediaStream Recording API for the audio_quality_browsertest. This CL uses the MediaStream Recording API to record the audio received by the right tag. It saves it as a webm file that is later converted to a wav file using ffmpeg. This might cause a performance regression because we're using webm+ffmpeg to generate the wav file. I set a, hopefully high enough, bitrate to mitigate this BUG=chromium:681577 NOTRY=True ========== to ========== WebRTC: Use the MediaStream Recording API for the audio_quality_browsertest. This CL uses the MediaStream Recording API to record the audio received by the right tag. It saves it as a webm file that is later converted to a wav file using ffmpeg. This might cause a performance regression because we're using webm+ffmpeg to generate the wav file. I set a, hopefully high enough, bitrate to mitigate this BUG=chromium:681577 NOTRY=True Review-Url: https://codereview.chromium.org/2753543010 Cr-Commit-Position: refs/heads/master@{#459050} Committed: https://chromium.googlesource.com/chromium/src/+/a0a47bfebb016ec468bc6d1b4be9... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/a0a47bfebb016ec468bc6d1b4be9...
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/2773803002/ by ehmaldonado@chromium.org. The reason for reverting is: This might be causing failures in Win7 bots: https://build.chromium.org/p/chromium.webrtc/builders/Win7%20Tester/builds/34106. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
