Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2709483009:
Fixed Full stack tests to correctly process selected TL and SL while (Closed)
DescriptionFixed Full stack tests to correctly process selected TL and SL while
calculating frame sizes. Added actual_bitrate metric which also accounts
for TL and SL info. Metric encoded_frame_size calculation is cleaned up. Perf alerts should be ignored.
BUG=webrtc:7095
Review-Url: https://codereview.webrtc.org/2709483009
Cr-Commit-Position: refs/heads/master@{#16800}
Committed: https://chromium.googlesource.com/external/webrtc/+/1e7732c3d9c1b8d4d3fa7f7c2c94ba383c50a1e5
Patch Set 1 #Patch Set 2 : fixing mistake where important code is inside DCHECK #
Total comments: 10
Patch Set 3 : Implemented Sprang@ comments. #
Total comments: 2
Patch Set 4 : Implemented Sprang@ nits #Patch Set 5 : rebase #Patch Set 6 : fixing CE #Messages
Total messages: 25 (14 generated)
ilnik@webrtc.org changed reviewers: + sprang@webrtc.org
Description was changed from ========== Fixed Full stack tests to correctly process selected TL and SL while calculating frame sizes. Added actual_bitrate metric which also accounts for TL and SL info. BUG=webrtc:7095 ========== to ========== Fixed Full stack tests to correctly process selected TL and SL while calculating frame sizes. Added actual_bitrate metric which also accounts for TL and SL info. BUG=webrtc:7095 ==========
ilnik@webrtc.org changed reviewers: + brandtr@webrtc.org
https://codereview.webrtc.org/2709483009/diff/20001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2709483009/diff/20001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:328: RtpDepacketizer::Create(is_vp8 ? kRtpVideoVp8 : kRtpVideoVp9)); Can you extract this to a helper method? https://codereview.webrtc.org/2709483009/diff/20001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:363: void EncodedFrameCallback(const EncodedFrame& frame) override { } encoded_frame_size_ isn't being populate any more? I think it'd still be interesting to have. Also note that this can be slightly different than rtp media payload size, since it'd doesn't include and payload specific headers. https://codereview.webrtc.org/2709483009/diff/20001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:843: double GetActualBitrate() { Maybe GetAverageMediaPayloadBitrateBps() ? https://codereview.webrtc.org/2709483009/diff/20001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:846: rtc::kNumMillisecsPerSec; Just to be sure, maybe add a dcheck verifying that last_sending_time_ and first_sending_time_ are not equal. Though a single frame/single packet test should be rare even with the quick mode.
https://codereview.webrtc.org/2709483009/diff/20001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2709483009/diff/20001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:328: RtpDepacketizer::Create(is_vp8 ? kRtpVideoVp8 : kRtpVideoVp9)); On 2017/02/22 16:56:07, språng wrote: > Can you extract this to a helper method? Done. https://codereview.webrtc.org/2709483009/diff/20001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:363: void EncodedFrameCallback(const EncodedFrame& frame) override { } On 2017/02/22 16:56:07, språng wrote: > encoded_frame_size_ isn't being populate any more? > > I think it'd still be interesting to have. Also note that this can be slightly > different than rtp media payload size, since it'd doesn't include and payload > specific headers. No, it is populated in sendRtp to encoded_frame_sizes_, from there it's inserted to Comparison struct and finally, added to statistic in PerformFrameComparison at line 785. It will be a bit different, but it's the only way to separate different streams. Before my changes encoded_frame_size_ was populated in both places. https://codereview.webrtc.org/2709483009/diff/20001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:843: double GetActualBitrate() { On 2017/02/22 16:56:07, språng wrote: > Maybe GetAverageMediaPayloadBitrateBps() ? Done. https://codereview.webrtc.org/2709483009/diff/20001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:846: rtc::kNumMillisecsPerSec; On 2017/02/22 16:56:06, språng wrote: > Just to be sure, maybe add a dcheck verifying that last_sending_time_ and > first_sending_time_ are not equal. Though a single frame/single packet test > should be rare even with the quick mode. Actually, in quick mode there will be exactly one frame. Need to add fix for that. Nice catch.
Description was changed from ========== Fixed Full stack tests to correctly process selected TL and SL while calculating frame sizes. Added actual_bitrate metric which also accounts for TL and SL info. BUG=webrtc:7095 ========== to ========== Fixed Full stack tests to correctly process selected TL and SL while calculating frame sizes. Added actual_bitrate metric which also accounts for TL and SL info. Metric encoded_frame_size calculation is cleaned up. Perf alerts should be ignored. BUG=webrtc:7095 ==========
lgtm with nits https://codereview.webrtc.org/2709483009/diff/20001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2709483009/diff/20001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:363: void EncodedFrameCallback(const EncodedFrame& frame) override { } On 2017/02/23 08:58:29, ilnik wrote: > On 2017/02/22 16:56:07, språng wrote: > > encoded_frame_size_ isn't being populate any more? > > > > I think it'd still be interesting to have. Also note that this can be slightly > > different than rtp media payload size, since it'd doesn't include and payload > > specific headers. > > No, it is populated in sendRtp to encoded_frame_sizes_, from there it's inserted > to Comparison struct and finally, added to statistic in PerformFrameComparison > at line 785. It will be a bit different, but it's the only way to separate > different streams. Before my changes encoded_frame_size_ was populated in both > places. I see. Then I guess you can remove this method, remove the public EncodedFrameObserver inheritance and the registering of this class as pre_decode_callback on the video receive configs. https://codereview.webrtc.org/2709483009/diff/40001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2709483009/diff/40001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:848: return 0; nit: use brackets for if/else if { ... } else { ... }
https://codereview.webrtc.org/2709483009/diff/20001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2709483009/diff/20001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:363: void EncodedFrameCallback(const EncodedFrame& frame) override { } On 2017/02/23 10:46:54, språng wrote: > On 2017/02/23 08:58:29, ilnik wrote: > > On 2017/02/22 16:56:07, språng wrote: > > > encoded_frame_size_ isn't being populate any more? > > > > > > I think it'd still be interesting to have. Also note that this can be > slightly > > > different than rtp media payload size, since it'd doesn't include and > payload > > > specific headers. > > > > No, it is populated in sendRtp to encoded_frame_sizes_, from there it's > inserted > > to Comparison struct and finally, added to statistic in PerformFrameComparison > > at line 785. It will be a bit different, but it's the only way to separate > > different streams. Before my changes encoded_frame_size_ was populated in both > > places. > > I see. > Then I guess you can remove this method, remove the public EncodedFrameObserver > inheritance and the registering of this class as pre_decode_callback on the > video receive configs. Done. https://codereview.webrtc.org/2709483009/diff/40001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2709483009/diff/40001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:848: return 0; On 2017/02/23 10:46:54, språng wrote: > nit: use brackets for if/else > > if { > ... > } else { > ... > } Done.
The CQ bit was checked by ilnik@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/2709483009/#ps60001 (title: "Implemented Sprang@ nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...) ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/17663) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/23117) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/21898)
The CQ bit was checked by ilnik@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/2709483009/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
The CQ bit was checked by ilnik@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/2709483009/#ps100001 (title: "fixing CE")
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": 100001, "attempt_start_ts": 1487851823955810,
"parent_rev": "676e7539e42410550be56ad4ce25302b7016d77e", "commit_rev":
"1e7732c3d9c1b8d4d3fa7f7c2c94ba383c50a1e5"}
Message was sent while issue was closed.
Description was changed from ========== Fixed Full stack tests to correctly process selected TL and SL while calculating frame sizes. Added actual_bitrate metric which also accounts for TL and SL info. Metric encoded_frame_size calculation is cleaned up. Perf alerts should be ignored. BUG=webrtc:7095 ========== to ========== Fixed Full stack tests to correctly process selected TL and SL while calculating frame sizes. Added actual_bitrate metric which also accounts for TL and SL info. Metric encoded_frame_size calculation is cleaned up. Perf alerts should be ignored. BUG=webrtc:7095 Review-Url: https://codereview.webrtc.org/2709483009 Cr-Commit-Position: refs/heads/master@{#16800} Committed: https://chromium.googlesource.com/external/webrtc/+/1e7732c3d9c1b8d4d3fa7f7c2... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/external/webrtc/+/1e7732c3d9c1b8d4d3fa7f7c2... |
