Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2027323003:
MediaRecorder: parameterise certain browsertests by video codec and/or encode accelerator (Closed)
|
Created:
4 years, 6 months ago by mcasas Modified:
4 years, 6 months ago CC:
chromium-reviews, jam, tnakamura+watch_chromium.org, phoglund+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMediaRecorder: parameterise certain browsertests by video codec and/or encode accelerator
Extended certain test to use VP8, VP9 and H264, with and without
video encode accelerator.
BUG=614235
Committed: https://crrev.com/441d717084666c0028d5b7a6225f4b319ccd8df4
Cr-Commit-Position: refs/heads/master@{#397630}
Patch Set 1 #Patch Set 2 : adding parameterisation support for enable/disable encode accelerator #
Total comments: 4
Messages
Total messages: 22 (10 generated)
mcasas@chromium.org changed reviewers: + emircan@chromium.org
emircan@ PTAL or fwd appropriately.
mcasas@chromium.org changed reviewers: + phoglund@chromium.org
phoglund@ PTAL/Owners RS
lgtm
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
On 2016/06/01 23:44:30, emircan wrote: > lgtm Emircan@, please take a look at PS2 after follow-up of our offline conversation on adding coverage for with/without encode acceleration.
On 2016/06/02 00:39:46, mcasas wrote: > On 2016/06/01 23:44:30, emircan wrote: > > lgtm > > Emircan@, please take a look at PS2 after > follow-up of our offline conversation on > adding coverage for with/without encode > acceleration. Thanks. We will cover test cases of all codecswith SW and HW with this. Regarding HW coverage, I would disable HW encode for CrOS for now as we are seeing issue in H264 especially. We can quickly add that platform check in one of these places. https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
Patchset #2 (id:60001) has been deleted
On 2016/06/02 00:46:24, emircan wrote: > On 2016/06/02 00:39:46, mcasas wrote: > > On 2016/06/01 23:44:30, emircan wrote: > > > lgtm > > > > Emircan@, please take a look at PS2 after > > follow-up of our offline conversation on > > adding coverage for with/without encode > > acceleration. > > Thanks. We will cover test cases of all codecswith SW and HW with this. > > Regarding HW coverage, I would disable HW encode for CrOS for now as we are > seeing issue in H264 especially. We can quickly add that platform check in one > of these places. > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... I will address those changes in https://codereview.chromium.org/2035593002/.
lgtm with nits https://codereview.chromium.org/2027323003/diff/80001/content/browser/media/w... File content/browser/media/webrtc/webrtc_media_recorder_browsertest.cc (right): https://codereview.chromium.org/2027323003/diff/80001/content/browser/media/w... content/browser/media/webrtc/webrtc_media_recorder_browsertest.cc:52: if (!disable) You may as well do the GetParam in here, no? Seems like it will read better and be less clutter. https://codereview.chromium.org/2027323003/diff/80001/content/browser/media/w... content/browser/media/webrtc/webrtc_media_recorder_browsertest.cc:176: INSTANTIATE_TEST_CASE_P(, Hmm, this looks strange, but if it works I'm ok with it.
https://codereview.chromium.org/2027323003/diff/80001/content/browser/media/w... File content/browser/media/webrtc/webrtc_media_recorder_browsertest.cc (right): https://codereview.chromium.org/2027323003/diff/80001/content/browser/media/w... content/browser/media/webrtc/webrtc_media_recorder_browsertest.cc:52: if (!disable) On 2016/06/02 08:46:23, phoglund_chrome wrote: > You may as well do the GetParam in here, no? Seems like it will read better and > be less clutter. GetParam() only works if the test case has been instantiated with an _P() macro, otherwise it hits https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl... https://codereview.chromium.org/2027323003/diff/80001/content/browser/media/w... content/browser/media/webrtc/webrtc_media_recorder_browsertest.cc:176: INSTANTIATE_TEST_CASE_P(, On 2016/06/02 08:46:23, phoglund_chrome wrote: > Hmm, this looks strange, but if it works I'm ok with it. Acknowledged.
On 2016/06/02 14:57:30, mcasas wrote: > https://codereview.chromium.org/2027323003/diff/80001/content/browser/media/w... > File content/browser/media/webrtc/webrtc_media_recorder_browsertest.cc (right): > > https://codereview.chromium.org/2027323003/diff/80001/content/browser/media/w... > content/browser/media/webrtc/webrtc_media_recorder_browsertest.cc:52: if > (!disable) > On 2016/06/02 08:46:23, phoglund_chrome wrote: > > You may as well do the GetParam in here, no? Seems like it will read better > and > > be less clutter. > > GetParam() only works if the test case has been > instantiated with an _P() macro, otherwise it hits > https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl... > > https://codereview.chromium.org/2027323003/diff/80001/content/browser/media/w... > content/browser/media/webrtc/webrtc_media_recorder_browsertest.cc:176: > INSTANTIATE_TEST_CASE_P(, > On 2016/06/02 08:46:23, phoglund_chrome wrote: > > Hmm, this looks strange, but if it works I'm ok with it. > > Acknowledged. https://crrev.com/2035593002/ has landed ==> sending this CL now.
Description was changed from ========== MediaRecorder: parameterise certain browsertests by video codec Extended certain test to use VP8, VP9 and H264. BUG=614235 ========== to ========== MediaRecorder: parameterise certain browsertests by video codec and/or encode accelerator Extended certain test to use VP8, VP9 and H264, with and without video encode accelerator. BUG=614235 ==========
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emircan@chromium.org Link to the patchset: https://codereview.chromium.org/2027323003/#ps80001 (title: "adding parameterisation support for enable/disable encode accelerator")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2027323003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2027323003/80001
Message was sent while issue was closed.
Description was changed from ========== MediaRecorder: parameterise certain browsertests by video codec and/or encode accelerator Extended certain test to use VP8, VP9 and H264, with and without video encode accelerator. BUG=614235 ========== to ========== MediaRecorder: parameterise certain browsertests by video codec and/or encode accelerator Extended certain test to use VP8, VP9 and H264, with and without video encode accelerator. BUG=614235 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== MediaRecorder: parameterise certain browsertests by video codec and/or encode accelerator Extended certain test to use VP8, VP9 and H264, with and without video encode accelerator. BUG=614235 ========== to ========== MediaRecorder: parameterise certain browsertests by video codec and/or encode accelerator Extended certain test to use VP8, VP9 and H264, with and without video encode accelerator. BUG=614235 Committed: https://crrev.com/441d717084666c0028d5b7a6225f4b319ccd8df4 Cr-Commit-Position: refs/heads/master@{#397630} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/441d717084666c0028d5b7a6225f4b319ccd8df4 Cr-Commit-Position: refs/heads/master@{#397630} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
