Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2534203004:
Start gathering perf data for VP8 + FlexFEC. (Closed)
|
Created:
4 years ago by brandtr Modified:
4 years ago Reviewers:
stefan-webrtc CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, tterriberry_mozilla.com, mflodman Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionStart gathering perf data for VP8 + FlexFEC.
This is to assess the performance penalty of the (current)
lack of integration with FlexFEC and BWE.
This CL also enables send-side BWE for the following tests:
- foreman_cif_net_delay_0_0_plr_0_VP9
- foreman_cif_net_delay_0_0_plr_0_H264
- foreman_cif_delay_50_0_plr_5_VP9
- foreman_cif_delay_50_0_plr_5_H264
- foreman_cif_delay_50_0_plr_5_H264_flexfec
- foreman_cif_delay_50_0_plr_5_H264_ulpfec
Perf alerts on these tests are therefore expected.
R=stefan@webrtc.org
BUG=webrtc:5654
Committed: https://crrev.com/93c5d030fcb18ec31f7a667d2107223f3cccbc5d
Cr-Commit-Position: refs/heads/master@{#15339}
Patch Set 1 #Patch Set 2 : Remove helper functions and enable send-side BWE for some tests. #Patch Set 3 : Make disabling of send-side BWE a little more explicit. #Patch Set 4 : Disambiguate foreman_cif_500kbps_100ms_32pkts_queue test metric names. #Messages
Total messages: 19 (6 generated)
Local runs show lower (?) media bitrates for VP8+ULPFEC than for VP8+FlexFEC. It would be good to gather some more reliable statistics from the buildbots however. These tests could probably be removed when we add BWE integration to FlexFEC. Overall, we should probably also go through the full stack tests and harmonize the code and settings. For example, some tests use send-side BWE, and some don't, without showing this in the test names.
lgtm
On 2016/11/30 11:41:53, brandtr wrote: > Local runs show lower (?) media bitrates for VP8+ULPFEC than for VP8+FlexFEC. It > would be good to gather some more reliable statistics from the buildbots > however. > > These tests could probably be removed when we add BWE integration to FlexFEC. > Overall, we should probably also go through the full stack tests and harmonize > the code and settings. For example, some tests use send-side BWE, and some > don't, without showing this in the test names. Yes, that'd be good to do. I think it only makes sense to run a few tests with receive-side BWE. If I remember correctly I did that some time ago?
On 2016/11/30 12:48:22, stefan-webrtc (holmer) wrote: > On 2016/11/30 11:41:53, brandtr wrote: > > Local runs show lower (?) media bitrates for VP8+ULPFEC than for VP8+FlexFEC. > It > > would be good to gather some more reliable statistics from the buildbots > > however. > > > > These tests could probably be removed when we add BWE integration to FlexFEC. > > Overall, we should probably also go through the full stack tests and harmonize > > the code and settings. For example, some tests use send-side BWE, and some > > don't, without showing this in the test names. > > Yes, that'd be good to do. I think it only makes sense to run a few tests with > receive-side BWE. If I remember correctly I did that some time ago? Right. Looking at the end of the full_stack.cc file, there is actually one test that uses receive BWE, and is appropriately named. At the beginning of the file, there is an sort of unclear mix of it being on and off though. I'll make a separate patch set where I clean this up, and then you can have a look again :) (This will lead to perf alerts, as the send-side BWE seems to give smoother curves and the old BWEs.)
Description was changed from ========== Start gathering perf data for VP8 + FlexFEC. This is to assess the performance penalty of the (current) lack of integration with FlexFEC and BWE. R=stefan@webrtc.org BUG=webrtc:5654 ========== to ========== Start gathering perf data for VP8 + FlexFEC. This is to assess the performance penalty of the (current) lack of integration with FlexFEC and BWE. This CL also enables send-side BWE for the following tests: - foreman_cif_net_delay_0_0_plr_0_VP9 - foreman_cif_net_delay_0_0_plr_0_H264 - foreman_cif_delay_50_0_plr_5_VP9 - foreman_cif_delay_50_0_plr_5_H264 - foreman_cif_delay_50_0_plr_5_H264_flexfec - foreman_cif_delay_50_0_plr_5_H264_ulpfec Perf alerts on these tests are therefore expected. R=stefan@webrtc.org BUG=webrtc:5654 ==========
PTAL :)
On 2016/11/30 13:16:48, brandtr wrote: > PTAL :) Now only one test should be run without send-side BWE. And it is explicitly named so. [ The test, not the metric :( ] When looking at it, there is actually two tests returning the same metric: https://cs.chromium.org/chromium/src/third_party/webrtc/video/full_stack.cc?l... https://cs.chromium.org/chromium/src/third_party/webrtc/video/full_stack.cc?l... Is this intended?
lgtm
On 2016/11/30 13:41:21, stefan-webrtc (holmer) wrote: > lgtm Should I also update the name of one of the two "foreman_cif_500kbps_100ms_32pkts_queue" tests, or is it OK as is?
On 2016/11/30 13:53:40, brandtr wrote: > On 2016/11/30 13:41:21, stefan-webrtc (holmer) wrote: > > lgtm > > Should I also update the name of one of the two > "foreman_cif_500kbps_100ms_32pkts_queue" tests, or is it OK as is? What do you want the names to be? I'm OK as is.
Added "_recv_bwe" to one of the metrics.
The CQ bit was checked by brandtr@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2534203004/#ps60001 (title: "Disambiguate foreman_cif_500kbps_100ms_32pkts_queue test metric names.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480518757319660,
"parent_rev": "dfd59b93b0a96d808e205daa21ac95c8ba6b5f7e", "commit_rev":
"d0bca119e16ea3edae310ebc2f64e6b3d1bcaae8"}
Message was sent while issue was closed.
Description was changed from ========== Start gathering perf data for VP8 + FlexFEC. This is to assess the performance penalty of the (current) lack of integration with FlexFEC and BWE. This CL also enables send-side BWE for the following tests: - foreman_cif_net_delay_0_0_plr_0_VP9 - foreman_cif_net_delay_0_0_plr_0_H264 - foreman_cif_delay_50_0_plr_5_VP9 - foreman_cif_delay_50_0_plr_5_H264 - foreman_cif_delay_50_0_plr_5_H264_flexfec - foreman_cif_delay_50_0_plr_5_H264_ulpfec Perf alerts on these tests are therefore expected. R=stefan@webrtc.org BUG=webrtc:5654 ========== to ========== Start gathering perf data for VP8 + FlexFEC. This is to assess the performance penalty of the (current) lack of integration with FlexFEC and BWE. This CL also enables send-side BWE for the following tests: - foreman_cif_net_delay_0_0_plr_0_VP9 - foreman_cif_net_delay_0_0_plr_0_H264 - foreman_cif_delay_50_0_plr_5_VP9 - foreman_cif_delay_50_0_plr_5_H264 - foreman_cif_delay_50_0_plr_5_H264_flexfec - foreman_cif_delay_50_0_plr_5_H264_ulpfec Perf alerts on these tests are therefore expected. R=stefan@webrtc.org BUG=webrtc:5654 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Start gathering perf data for VP8 + FlexFEC. This is to assess the performance penalty of the (current) lack of integration with FlexFEC and BWE. This CL also enables send-side BWE for the following tests: - foreman_cif_net_delay_0_0_plr_0_VP9 - foreman_cif_net_delay_0_0_plr_0_H264 - foreman_cif_delay_50_0_plr_5_VP9 - foreman_cif_delay_50_0_plr_5_H264 - foreman_cif_delay_50_0_plr_5_H264_flexfec - foreman_cif_delay_50_0_plr_5_H264_ulpfec Perf alerts on these tests are therefore expected. R=stefan@webrtc.org BUG=webrtc:5654 ========== to ========== Start gathering perf data for VP8 + FlexFEC. This is to assess the performance penalty of the (current) lack of integration with FlexFEC and BWE. This CL also enables send-side BWE for the following tests: - foreman_cif_net_delay_0_0_plr_0_VP9 - foreman_cif_net_delay_0_0_plr_0_H264 - foreman_cif_delay_50_0_plr_5_VP9 - foreman_cif_delay_50_0_plr_5_H264 - foreman_cif_delay_50_0_plr_5_H264_flexfec - foreman_cif_delay_50_0_plr_5_H264_ulpfec Perf alerts on these tests are therefore expected. R=stefan@webrtc.org BUG=webrtc:5654 Committed: https://crrev.com/93c5d030fcb18ec31f7a667d2107223f3cccbc5d Cr-Commit-Position: refs/heads/master@{#15339} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/93c5d030fcb18ec31f7a667d2107223f3cccbc5d Cr-Commit-Position: refs/heads/master@{#15339} |
