Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(12)

Issue 2753543010: WebRTC: Use the MediaStream Recording API for the audio_quality_browsertest. (Closed)

Created:
1 year, 8 months ago by ehmaldonado_chromium
Modified:
1 year, 8 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.

Description

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/+/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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+169 lines, -211 lines) Patch
M chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc View 1 2 3 4 5 6 7 18 chunks +106 lines, -210 lines 0 comments Download
A chrome/test/data/webrtc/audio_extraction.js View 1 2 3 4 5 6 1 chunk +62 lines, -0 lines 0 comments Download
D chrome/test/data/webrtc/resources/force_mic_volume_max.exe.sha1 View 1 1 chunk +0 lines, -1 line 0 comments Download
M chrome/test/data/webrtc/webrtc_audio_quality_test.html View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 42 (18 generated)
ehmaldonado_chromium
Please take a look. Do you know who else can review this?
1 year, 8 months ago (2017-03-16 15:29:54 UTC) #2
ehmaldonado_chromium
hbos: Are you the right person to review this?
1 year, 8 months ago (2017-03-17 15:17:46 UTC) #4
hbos_chromium
On 2017/03/17 15:17:46, ehmaldonado_chromium wrote: > hbos: Are you the right person to review this? ...
1 year, 8 months ago (2017-03-20 11:08:21 UTC) #6
kjellander_chromium
Sorry for the delay https://codereview.chromium.org/2753543010/diff/1/chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc File chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc (left): https://codereview.chromium.org/2753543010/diff/1/chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc#oldcode289 chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc:289: FILE_PATH_LITERAL("force_mic_volume_max.exe"))); I guess you can ...
1 year, 8 months ago (2017-03-21 11:09:01 UTC) #9
ehmaldonado_chromium
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/webrtc_audio_quality_browsertest.cc > File ...
1 year, 8 months ago (2017-03-21 12:56:17 UTC) #10
kjellander_chromium
https://codereview.chromium.org/2753543010/diff/1/chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc File chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc (right): https://codereview.chromium.org/2753543010/diff/1/chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc#newcode93 chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc:93: // 5. (For Chrome bots) Ensure sox and rec ...
1 year, 8 months ago (2017-03-21 13:29:23 UTC) #11
ehmaldonado_chromium
> https://codereview.chromium.org/2753543010/diff/20001/chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc > File chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc (right): > > https://codereview.chromium.org/2753543010/diff/20001/chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc#newcode538 > chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc:538: int > polling_interval_msec = ...
1 year, 8 months ago (2017-03-21 14:05:24 UTC) #12
kjellander_chromium
lgtm with my comment addressed. You might want to have another reviewer for the MediaRecorder ...
1 year, 8 months ago (2017-03-21 14:07:26 UTC) #13
ehmaldonado_chromium
Hi Miguel, are you the right person to review the usage of MediaStream in this ...
1 year, 8 months ago (2017-03-21 14:26:30 UTC) #15
mcasas
https://codereview.chromium.org/2753543010/diff/60001/chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc File chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc (right): https://codereview.chromium.org/2753543010/diff/60001/chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc#newcode161 chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc:161: const base::FilePath& recording, |recording| is confusing, what about s/recording/record_output_path/ ...
1 year, 8 months ago (2017-03-21 16:54:22 UTC) #16
ehmaldonado_chromium
I'll address the other comments shortly :) https://codereview.chromium.org/2753543010/diff/60001/chrome/test/data/webrtc/audio_extraction.js File chrome/test/data/webrtc/audio_extraction.js (right): https://codereview.chromium.org/2753543010/diff/60001/chrome/test/data/webrtc/audio_extraction.js#newcode23 chrome/test/data/webrtc/audio_extraction.js:23: var gAudioBase64 ...
1 year, 8 months ago (2017-03-21 16:59:55 UTC) #17
mcasas
On 2017/03/21 16:59:55, ehmaldonado_chromium wrote: > I'll address the other comments shortly :) > > ...
1 year, 8 months ago (2017-03-21 17:10:14 UTC) #18
ehmaldonado_chromium
On 2017/03/21 17:10:14, mcasas wrote: > On 2017/03/21 16:59:55, ehmaldonado_chromium wrote: > > I'll address ...
1 year, 8 months ago (2017-03-22 16:33:54 UTC) #19
mcasas
lgtm with some comments/suggestions. https://codereview.chromium.org/2753543010/diff/80001/chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc File chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc (right): https://codereview.chromium.org/2753543010/diff/80001/chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc#newcode9 chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc:9: #include "base/base64.h" Probably not needed ...
1 year, 8 months ago (2017-03-22 17:04:55 UTC) #20
ehmaldonado_chromium
PTAL. I had to modify a couple of other files.
1 year, 8 months ago (2017-03-22 17:41:13 UTC) #21
mcasas
https://codereview.chromium.org/2753543010/diff/100001/chrome/browser/media/webrtc/webrtc_browsertest_common.cc File chrome/browser/media/webrtc/webrtc_browsertest_common.cc (right): https://codereview.chromium.org/2753543010/diff/100001/chrome/browser/media/webrtc/webrtc_browsertest_common.cc#newcode192 chrome/browser/media/webrtc/webrtc_browsertest_common.cc:192: } This code is very similar to l.129-163. Either ...
1 year, 8 months ago (2017-03-22 17:54:14 UTC) #22
ehmaldonado_chromium
PTAL I decided to just call PollingWaitUntil(..., "true", ...) directly.
1 year, 8 months ago (2017-03-22 18:06:54 UTC) #23
mcasas
On 2017/03/22 18:06:54, ehmaldonado_chromium wrote: > PTAL > I decided to just call PollingWaitUntil(..., "true", ...
1 year, 8 months ago (2017-03-22 18:07:46 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2753543010/120001
1 year, 8 months ago (2017-03-22 18:10:16 UTC) #27
commit-bot: I haz the power
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/192036)
1 year, 8 months ago (2017-03-22 18:49:43 UTC) #29
kjellander_chromium
On 2017/03/22 18:49:43, commit-bot: I haz the power wrote: > Try jobs failed on following ...
1 year, 8 months ago (2017-03-23 08:00:45 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2753543010/140001
1 year, 8 months ago (2017-03-23 11:51:22 UTC) #38
commit-bot: I haz the power
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/a0a47bfebb016ec468bc6d1b4be9b1acb22a4075
1 year, 8 months ago (2017-03-23 11:55:42 UTC) #41
ehmaldonado_chromium
1 year, 8 months ago (2017-03-23 18:52:38 UTC) #42
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.

Powered by Google App Engine
This is Rietveld 408576698