Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2577513002:
Disable Android VEA for Media Recorder (Closed)
DescriptionDisable Android VEA for Media Recorder
HW codec use to be disabled for all on Android. Now we enable H264 HW
encoding on Android for RTC in crbug/664652. Meantime MediaRecoder
wants to use it too. But as shown in crbug/653864, the
WebRtcMediaRecorderTest.PeerConnection will fail with HW encoder on
some devices. So we have to disable VEA for Media Recorder at present
until we figure out and solve the problam. (Another decision should be
made is if we want to enable VP8 HW encoding to media recorder too, as
Media Recorder ignores switches for RTC.)
BUG=653864
Committed: https://crrev.com/353fe353d4f1159a1dafbc4cc13f52829c7c8ed8
Cr-Commit-Position: refs/heads/master@{#439271}
Patch Set 1 #Patch Set 2 : rebase #
Messages
Total messages: 31 (17 generated)
The CQ bit was checked by braveyao@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 ========== Disable Android VEA for Media Recorder HW codec use to be disabled for all on Android. Now we enable H264 HW encoding on Android for RTC in crbug/664652. Meantime MediaRecoder wants to use it too. But as shown in crbug/653864, the MediaRecorder will fail with HW encoder on some devices. So we have to disable VEA for Media Recorder at present until we figure out and solve the problam.(Another decision should be made is if we want to enable VP8 HW encoding to media recorder too.) BUG=653864 ========== to ========== Disable Android VEA for Media Recorder HW codec use to be disabled for all on Android. Now we enable H264 HW encoding on Android for RTC in crbug/664652. Meantime MediaRecoder wants to use it too. But as shown in crbug/653864, the WebRtcMediaRecorderTest.PeerConnection will fail with HW encoder on some devices. So we have to disable VEA for Media Recorder at present until we figure out and solve the problam. (Another decision should be made is if we want to enable VP8 HW encoding to media recorder too, as Media Recorder ignores switches for RTC.) BUG=653864 ==========
Description was changed from ========== Disable Android VEA for Media Recorder HW codec use to be disabled for all on Android. Now we enable H264 HW encoding on Android for RTC in crbug/664652. Meantime MediaRecoder wants to use it too. But as shown in crbug/653864, the WebRtcMediaRecorderTest.PeerConnection will fail with HW encoder on some devices. So we have to disable VEA for Media Recorder at present until we figure out and solve the problam. (Another decision should be made is if we want to enable VP8 HW encoding to media recorder too, as Media Recorder ignores switches for RTC.) BUG=653864 ========== to ========== Disable Android VEA for Media Recorder HW codec use to be disabled for all on Android. Now we enable H264 HW encoding on Android for RTC in crbug/664652. Meantime MediaRecoder wants to use it too. But as shown in crbug/653864, the WebRtcMediaRecorderTest.PeerConnection will fail with HW encoder on some devices. So we have to disable VEA for Media Recorder at present until we figure out and solve the problam. (Another decision should be made is if we want to enable VP8 HW encoding to media recorder too, as Media Recorder ignores switches for RTC.) BUG=653864 ==========
braveyao@chromium.org changed reviewers: + perkj@chromium.org
Hi perkj@, please take a look when you get a chance.
braveyao@chromium.org changed reviewers: + emircan@chromium.org
+ emircan@
On 2016/12/13 21:48:08, braveyao wrote: > + emircan@ lgtm We need to get the test running again. Note that VEA use case in MediaRecorder is the same as WebRTC use case. If MediaRecorder test fails on a device, it is very likely that WebRTC use is broken too. However, since WebRTC has software fallback, we might not be aware of it being broken. I suggest not having code divergence between VEA client(webrtc, cast, media recorder). We should have a common blacklist covering all, i.e. N5 case broken here can give enough confidence to disable it in the common blacklist.
On 2016/12/14 01:48:25, emircan wrote: > On 2016/12/13 21:48:08, braveyao wrote: > > + emircan@ > > lgtm > > We need to get the test running again. Note that VEA use case in MediaRecorder > is the same as WebRTC use case. If MediaRecorder test fails on a device, it is > very likely that WebRTC use is broken too. However, since WebRTC has software > fallback, we might not be aware of it being broken. I suggest not having code > divergence between VEA client(webrtc, cast, media recorder). We should have a > common blacklist covering all, i.e. N5 case broken here can give enough > confidence to disable it in the common blacklist. Thanks for the talking yesterday, emircan@. That leads to more thinking to the VEA use case in MediaRecorder. There are some difference between VEA use case in MediaRecorder and WebRTC as I can think about right now: - Will MediaRecoder utilize VP8 HW codec? WebRTC focuses only on H264 currently, VP8 is not well tested yet. So if MediaRecorder wants to use VP8 HW codec, more tests should be done first. - WebRTC cares more about performance, e.g. BPS/FPS real time control, while it's not a concern of MediaRecorder. So is it really good to share BlackList between them, because apparently WebRTC will blacklist more devices than MediaRecorder. And there may be more to think about. Before these are clarified, it's better to keep VEA disabled to MediaRecorder temporally for now.
Hi perkj@, needs owner's review to these two files :)
I am no longer actively working in Chrome. Since you guys in Mtv actually wrote the recorder, can you please make sure you also have at least one owner of those files in Mtv? lgtm
On 2016/12/16 08:17:02, perkj_chrome wrote: > I am no longer actively working in Chrome. > Since you guys in Mtv actually wrote the recorder, can you please make sure you > also have at least one owner of those files in Mtv? > > lgtm Yes, that's why I included emircan@ here who actually works on MediaRecorder now.
The CQ bit was checked by braveyao@chromium.org
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by braveyao@chromium.org
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
Failed to apply patch for
content/browser/webrtc/webrtc_media_recorder_browsertest.cc:
While running git apply --index -p1;
error: patch failed:
content/browser/webrtc/webrtc_media_recorder_browsertest.cc:141
error: content/browser/webrtc/webrtc_media_recorder_browsertest.cc: patch does
not apply
Patch: content/browser/webrtc/webrtc_media_recorder_browsertest.cc
Index: content/browser/webrtc/webrtc_media_recorder_browsertest.cc
diff --git a/content/browser/webrtc/webrtc_media_recorder_browsertest.cc
b/content/browser/webrtc/webrtc_media_recorder_browsertest.cc
index
40b0613197d1f157feb7a659bc982fecc168d621..e60cffa8decdb367a497fcf1f4d7d476395b5ded
100644
--- a/content/browser/webrtc/webrtc_media_recorder_browsertest.cc
+++ b/content/browser/webrtc/webrtc_media_recorder_browsertest.cc
@@ -141,13 +141,7 @@ IN_PROC_BROWSER_TEST_F(WebRtcMediaRecorderTest,
kMediaRecorderHtmlFile);
}
-// http://crbug.com/673126, flaky on Android.
-#if defined(OS_ANDROID)
-#define MAYBE_PeerConnection DISABLED_PeerConnection
-#else
-#define MAYBE_PeerConnection PeerConnection
-#endif
-IN_PROC_BROWSER_TEST_P(WebRtcMediaRecorderTest, MAYBE_PeerConnection) {
+IN_PROC_BROWSER_TEST_P(WebRtcMediaRecorderTest, PeerConnection) {
MaybeForceDisableEncodeAccelerator(GetParam().disable_accelerator);
MakeTypicalCall(base::StringPrintf("testRecordRemotePeerConnection(\"%s\");",
GetParam().video_codec.c_str()),
The CQ bit was checked by braveyao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from perkj@chromium.org, emircan@chromium.org Link to the patchset: https://codereview.chromium.org/2577513002/#ps20001 (title: "rebase")
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": 20001, "attempt_start_ts": 1481932086440670,
"parent_rev": "529ed2b04ed914493e1ab3accefc69c34e61b028", "commit_rev":
"faf4b52319667766c73de0a5ce37e2cae5b19d60"}
Message was sent while issue was closed.
Description was changed from ========== Disable Android VEA for Media Recorder HW codec use to be disabled for all on Android. Now we enable H264 HW encoding on Android for RTC in crbug/664652. Meantime MediaRecoder wants to use it too. But as shown in crbug/653864, the WebRtcMediaRecorderTest.PeerConnection will fail with HW encoder on some devices. So we have to disable VEA for Media Recorder at present until we figure out and solve the problam. (Another decision should be made is if we want to enable VP8 HW encoding to media recorder too, as Media Recorder ignores switches for RTC.) BUG=653864 ========== to ========== Disable Android VEA for Media Recorder HW codec use to be disabled for all on Android. Now we enable H264 HW encoding on Android for RTC in crbug/664652. Meantime MediaRecoder wants to use it too. But as shown in crbug/653864, the WebRtcMediaRecorderTest.PeerConnection will fail with HW encoder on some devices. So we have to disable VEA for Media Recorder at present until we figure out and solve the problam. (Another decision should be made is if we want to enable VP8 HW encoding to media recorder too, as Media Recorder ignores switches for RTC.) BUG=653864 Review-Url: https://codereview.chromium.org/2577513002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Disable Android VEA for Media Recorder HW codec use to be disabled for all on Android. Now we enable H264 HW encoding on Android for RTC in crbug/664652. Meantime MediaRecoder wants to use it too. But as shown in crbug/653864, the WebRtcMediaRecorderTest.PeerConnection will fail with HW encoder on some devices. So we have to disable VEA for Media Recorder at present until we figure out and solve the problam. (Another decision should be made is if we want to enable VP8 HW encoding to media recorder too, as Media Recorder ignores switches for RTC.) BUG=653864 Review-Url: https://codereview.chromium.org/2577513002 ========== to ========== Disable Android VEA for Media Recorder HW codec use to be disabled for all on Android. Now we enable H264 HW encoding on Android for RTC in crbug/664652. Meantime MediaRecoder wants to use it too. But as shown in crbug/653864, the WebRtcMediaRecorderTest.PeerConnection will fail with HW encoder on some devices. So we have to disable VEA for Media Recorder at present until we figure out and solve the problam. (Another decision should be made is if we want to enable VP8 HW encoding to media recorder too, as Media Recorder ignores switches for RTC.) BUG=653864 Committed: https://crrev.com/353fe353d4f1159a1dafbc4cc13f52829c7c8ed8 Cr-Commit-Position: refs/heads/master@{#439271} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/353fe353d4f1159a1dafbc4cc13f52829c7c8ed8 Cr-Commit-Position: refs/heads/master@{#439271} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
