Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1215113003:
Added FullStack performance test for screensharing with VP9 (Closed)
|
Created:
5 years, 5 months ago by ivica Modified:
5 years, 4 months ago Reviewers:
pbos-webrtc CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), stefan-webrtc, tterriberry_mozilla.com, andresp, perkj_webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdded FullStack performance test for screensharing with VP9
Committed: https://crrev.com/028cf488284735f949ff1edf88a5b9bf731eb184
Cr-Commit-Position: refs/heads/master@{#9657}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Removing the comments regarding the payload_type #Patch Set 3 : Putting payload_type outside the if-else #Patch Set 4 : Increasing timeout for Android FullStack test #Patch Set 5 : Fixing compile errors... #
Total comments: 1
Patch Set 6 : Replacing ifdefs with kLongTimeoutMs #Patch Set 7 : Fixing formatting #Patch Set 8 : Disabling VP9 performance test on Android #Patch Set 9 : Fixing test names #Patch Set 10 : Changing test names and labels #
Total comments: 2
Patch Set 11 : editing comments and reverting timeout #
Messages
Total messages: 43 (24 generated)
ivica@webrtc.org changed reviewers: + pbos@webrtc.org
In video/full_stack.cc, which payload_types should we use for VP8 and VP9 codecs? Currently, 124 is used, but that's different than VCM_VP8_PAYLOAD_TYPE and VCM_VP9_PAYLOAD_TYPE.
lgtm, feel free to put payload_type outside the if-else https://codereview.webrtc.org/1215113003/diff/1/webrtc/video/full_stack.cc File webrtc/video/full_stack.cc (right): https://codereview.webrtc.org/1215113003/diff/1/webrtc/video/full_stack.cc#ne... webrtc/video/full_stack.cc:452: 124; // this is VCM_I420_PAYLOAD_TYPE (?) Remove comment. https://codereview.webrtc.org/1215113003/diff/1/webrtc/video/full_stack.cc#ne... webrtc/video/full_stack.cc:459: 124; // should I put VCM_VP9_PAYLOAD_TYPE? Remove comment, any payload_type is fine. If you want to you can set 124 outside the if-else.
The CQ bit was checked by ivica@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/1215113003/#ps40001 (title: "Putting payload_type outside the if-else")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1215113003/40001
The CQ bit was unchecked by ivica@webrtc.org
The CQ bit was checked by ivica@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/1215113003/#ps60001 (title: "Increasing timeout for Android FullStack test")
The CQ bit was unchecked by ivica@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1215113003/60001
The CQ bit was checked by ivica@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/1215113003/#ps80001 (title: "Fixing compile errors...")
The CQ bit was unchecked by ivica@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1215113003/80001
https://codereview.webrtc.org/1215113003/diff/80001/webrtc/video/full_stack.cc File webrtc/video/full_stack.cc (right): https://codereview.webrtc.org/1215113003/diff/80001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:211: #ifdef WEBRTC_ANDROID not lgtm, can you use kLongTimeoutMs for both instead?
The CQ bit was checked by ivica@webrtc.org
The CQ bit was unchecked by ivica@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1215113003/120001
The CQ bit was unchecked by commit-bot@chromium.org
A disapproval has been posted by following reviewers: pbos@webrtc.org. Please make sure to get positive review before another CQ attempt.
lgtm
The CQ bit was checked by ivica@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1215113003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android/builds/6784) (exceeded global retry quota)
The CQ bit was checked by ivica@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/1215113003/#ps140001 (title: "Disabling VP9 performance test on Android")
The CQ bit was unchecked by ivica@webrtc.org
The CQ bit was checked by ivica@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1215113003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1215113003/140001
The CQ bit was unchecked by ivica@webrtc.org
I disabled the VP9 performance test on Android the same way it was disabled for VP8.
The CQ bit was checked by ivica@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/1215113003/#ps160001 (title: "Fixing test names")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1215113003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1215113003/160001
The CQ bit was unchecked by ivica@webrtc.org
https://codereview.webrtc.org/1215113003/diff/180001/webrtc/video/full_stack.cc File webrtc/video/full_stack.cc (right): https://codereview.webrtc.org/1215113003/diff/180001/webrtc/video/full_stack.... webrtc/video/full_stack.cc:743: "screenshare_slides", // keep the old label remove comment, or put "// No suffix to preserve historic data.", but I think you can remove the comment. https://codereview.webrtc.org/1215113003/diff/180001/webrtc/video/full_stack.... webrtc/video/full_stack.cc:756: // probably the same issue as "Disabled on Android along with VP8 screenshare above."
lgtm
The CQ bit was checked by ivica@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1215113003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1215113003/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/028cf488284735f949ff1edf88a5b9bf731eb184 Cr-Commit-Position: refs/heads/master@{#9657} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
