Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1752293003:
Add VP9 to chrome_webrtc_video_quality_browsertest.cc. (Closed)
|
Created:
4 years, 9 months ago by asapersson2 Modified:
4 years, 9 months ago Reviewers:
phoglund_chromium CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, tnakamura+watch_chromium.org, phoglund+watch_chromium.org, mcasas+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd VP9 to chrome_webrtc_video_quality_browsertest.cc.
BUG=chromium:554512
Committed: https://crrev.com/34252c4b95fa8fc024dc84cb77f2389da4632c6d
Cr-Commit-Position: refs/heads/master@{#379552}
Patch Set 1 : #
Total comments: 6
Patch Set 2 : #Messages
Total messages: 15 (7 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Add VP9 to chrome_webrtc_video_quality_browsertest.cc. BUG=chromium:554512 ========== to ========== Add VP9 to chrome_webrtc_video_quality_browsertest.cc. BUG=chromium:554512 ==========
asapersson@chromium.org changed reviewers: + phoglund@chromium.org
https://codereview.chromium.org/1752293003/diff/20001/chrome/browser/media/ch... File chrome/browser/media/chrome_webrtc_video_quality_browsertest.cc (right): https://codereview.chromium.org/1752293003/diff/20001/chrome/browser/media/ch... chrome/browser/media/chrome_webrtc_video_quality_browsertest.cc:201: const std::string& video_codec, I don't see why this function needs to know about the video codec; encode the video codec in the test label from the caller instead. https://codereview.chromium.org/1752293003/diff/20001/chrome/browser/media/ch... chrome/browser/media/chrome_webrtc_video_quality_browsertest.cc:311: // analysis Nit: pull up to previous line https://codereview.chromium.org/1752293003/diff/20001/chrome/browser/media/ch... chrome/browser/media/chrome_webrtc_video_quality_browsertest.cc:319: GetWorkingDir().Append(kCapturedYuvFileName))); What about: std::string codec_label = video_codec.empty() ? "" : "_" + video_codec; std::string test_label = base::StringPrintf("%s%s", test_config_.test_name, codec_label.c_str()); ASSERT_TRUE(CompareVideosAndPrintResult( test_label, ...); Consider making a helper function MakeLabel(test_config_.test_name, video_codec) if you think that makes sense.
(+myself to CC :) I'll a make follow-up to also use H264 after this lands)
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/1752293003/diff/20001/chrome/browser/media/ch... File chrome/browser/media/chrome_webrtc_video_quality_browsertest.cc (right): https://codereview.chromium.org/1752293003/diff/20001/chrome/browser/media/ch... chrome/browser/media/chrome_webrtc_video_quality_browsertest.cc:201: const std::string& video_codec, On 2016/03/07 10:57:12, phoglund_chrome wrote: > I don't see why this function needs to know about the video codec; encode the > video codec in the test label from the caller instead. Done. https://codereview.chromium.org/1752293003/diff/20001/chrome/browser/media/ch... chrome/browser/media/chrome_webrtc_video_quality_browsertest.cc:311: // analysis On 2016/03/07 10:57:12, phoglund_chrome wrote: > Nit: pull up to previous line Done. https://codereview.chromium.org/1752293003/diff/20001/chrome/browser/media/ch... chrome/browser/media/chrome_webrtc_video_quality_browsertest.cc:319: GetWorkingDir().Append(kCapturedYuvFileName))); On 2016/03/07 10:57:12, phoglund_chrome wrote: > What about: > > std::string codec_label = video_codec.empty() ? "" : "_" + video_codec; > std::string test_label = base::StringPrintf("%s%s", > test_config_.test_name, codec_label.c_str()); > ASSERT_TRUE(CompareVideosAndPrintResult( > test_label, ...); > > Consider making a helper function MakeLabel(test_config_.test_name, video_codec) > if you think that makes sense. Done.
lgtm
The CQ bit was checked by asapersson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1752293003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1752293003/60001
Message was sent while issue was closed.
Description was changed from ========== Add VP9 to chrome_webrtc_video_quality_browsertest.cc. BUG=chromium:554512 ========== to ========== Add VP9 to chrome_webrtc_video_quality_browsertest.cc. BUG=chromium:554512 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add VP9 to chrome_webrtc_video_quality_browsertest.cc. BUG=chromium:554512 ========== to ========== Add VP9 to chrome_webrtc_video_quality_browsertest.cc. BUG=chromium:554512 Committed: https://crrev.com/34252c4b95fa8fc024dc84cb77f2389da4632c6d Cr-Commit-Position: refs/heads/master@{#379552} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/34252c4b95fa8fc024dc84cb77f2389da4632c6d Cr-Commit-Position: refs/heads/master@{#379552} |
