Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3003823003:
Fix FrameConfigs used for VP8 with four temporal layers. (Closed)
|
Created:
3 years, 3 months ago by sprang_webrtc Modified:
3 years, 2 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFix FrameConfigs used for VP8 with four temporal layers.
sync flag should only be true if:
* temporal layer > 0
* frame predics _only_ from tl0 (last)
Furthermore, flags should not predict from a buffer unless it has been
previously updated in the current iteration for the pattern.
I also added an experiment with an alternative pattern for a three
layer setup, where loose some efficieny by halving the pattern but
gain a little bit by updating arf for the top layer. The theory is that
this will cause fewer frame drops since we don't have as much
dependency on previous frames in the upper layer (which might not be
retransmitted).
BUG=webrtc:8162
Review-Url: https://codereview.webrtc.org/3003823003
Cr-Commit-Position: refs/heads/master@{#19716}
Committed: https://chromium.googlesource.com/external/webrtc/+/ff19d35bae3cb2567ad39f82dda9e1f9ded107c9
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fix unit tests, add full stack tests #
Total comments: 6
Patch Set 3 : Added generalized frame config test. Cleanup. #
Total comments: 4
Patch Set 4 : typos #Patch Set 5 : Fixed int to uint cast in test #
Messages
Total messages: 37 (19 generated)
sprang@webrtc.org changed reviewers: + marpan@webrtc.org, pbos@webrtc.org
sprang@webrtc.org changed reviewers: + ilnik@webrtc.org
On 2017/08/29 08:34:03, sprang_webrtc wrote: lgtm
The CQ bit was checked by sprang@webrtc.org
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: linux_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/29114)
https://codereview.webrtc.org/3003823003/diff/1/webrtc/modules/video_coding/c... File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc (right): https://codereview.webrtc.org/3003823003/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:99: if (field_trial::IsEnabled("WebRTC-AltTemporalLayers")) { Since this is a subset of the first, can you create the first and then resize it to four? https://codereview.webrtc.org/3003823003/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:165: if (field_trial::IsEnabled("WebRTC-AltTemporalLayers")) { Since this is a subset of the first, can you create the vector first and then resize it to four instead of returning two different-looking initializer lists? https://codereview.webrtc.org/3003823003/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:165: if (field_trial::IsEnabled("WebRTC-AltTemporalLayers")) { Can you add a test that covers this case? You can set field trials in tests.
The CQ bit was checked by sprang@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
updated and added tests. ptal https://codereview.webrtc.org/3003823003/diff/1/webrtc/modules/video_coding/c... File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc (right): https://codereview.webrtc.org/3003823003/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:99: if (field_trial::IsEnabled("WebRTC-AltTemporalLayers")) { On 2017/08/29 15:46:42, pbos-webrtc wrote: > Since this is a subset of the first, can you create the first and then resize it > to four? Done. https://codereview.webrtc.org/3003823003/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:165: if (field_trial::IsEnabled("WebRTC-AltTemporalLayers")) { On 2017/08/29 15:46:42, pbos-webrtc wrote: > Since this is a subset of the first, can you create the vector first and then > resize it to four instead of returning two different-looking initializer lists? They are different. In this version we update altref with frames in TL2, instead of just referencing TL0, TL1, and the previous key frame. https://codereview.webrtc.org/3003823003/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:165: if (field_trial::IsEnabled("WebRTC-AltTemporalLayers")) { On 2017/08/29 15:46:42, pbos-webrtc wrote: > Can you add a test that covers this case? You can set field trials in tests. Test added.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
pbos@google.com changed reviewers: + pbos@google.com
https://codereview.webrtc.org/3003823003/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc (right): https://codereview.webrtc.org/3003823003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:101: if (field_trial::IsEnabled("WebRTC-AltTemporalLayers")) Actually if this is not supposed to be a subset below pattern I prefer if you use the old path of just returning {false, true, true, false}. https://codereview.webrtc.org/3003823003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:101: if (field_trial::IsEnabled("WebRTC-AltTemporalLayers")) Also do you think we could generate GetTemporalLayerSync from std::vector<TemporalLayers::FrameConfig> from GetTemporalPattern() instead of having this pattern have to match below? https://codereview.webrtc.org/3003823003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:165: if (field_trial::IsEnabled("WebRTC-AltTemporalLayers")) { Can you put down here why this is meaningfully different from the other longer pattern? What's this trying to investigate? Also, can you make the field trial name more meaningful? Not sure if that's possible, but maybe something like "WebRTC-UseShortTL3Pattern" is more meaningful? FWIW it would've been meaningful to keep the pattern bug fixes and the experiment separate, suggest doing so in the future. (The bug fixes would've been submittable straight away.)
https://codereview.webrtc.org/3003823003/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc (right): https://codereview.webrtc.org/3003823003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:101: if (field_trial::IsEnabled("WebRTC-AltTemporalLayers")) On 2017/08/31 19:54:38, pbos1 wrote: > Actually if this is not supposed to be a subset below pattern I prefer if you > use the old path of just returning {false, true, true, false}. Done. https://codereview.webrtc.org/3003823003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:101: if (field_trial::IsEnabled("WebRTC-AltTemporalLayers")) On 2017/08/31 19:54:38, pbos1 wrote: > Also do you think we could generate GetTemporalLayerSync from > std::vector<TemporalLayers::FrameConfig> from GetTemporalPattern() instead of > having this pattern have to match below? Potentially, but it's not super straight forward. I've added a generalized test. I'm thinking that in a follow-up CL we can modify that to generate the sync bit instead, or even refactor the test into a class the can validate the frame configs actually going out of the encoder in say debug mode. That way we could have found (in eg full stack tests) the bug that sometimes caused incorrect sync bit for screenshare. wdyt? https://codereview.webrtc.org/3003823003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:165: if (field_trial::IsEnabled("WebRTC-AltTemporalLayers")) { On 2017/08/31 19:54:38, pbos1 wrote: > Can you put down here why this is meaningfully different from the other longer > pattern? What's this trying to investigate? Done. > Also, can you make the field trial name more meaningful? Not sure if that's > possible, but maybe something like "WebRTC-UseShortTL3Pattern" is > more meaningful? Done. > FWIW it would've been meaningful to keep the pattern bug fixes and the > experiment separate, suggest doing so in the future. (The bug fixes would've > been submittable straight away.) True, that's what I normally do. This CL started out investigating the experiment, I just happened to find the bugs along the way and was lazy...
lgtm, I'd like marpan@ to sign off on the new pattern though. https://codereview.webrtc.org/3003823003/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc (right): https://codereview.webrtc.org/3003823003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:165: // This field trial is intended to check if it is worth using a shorted shortened/shorter https://codereview.webrtc.org/3003823003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:174: // frame until the next sync frame. So we expect a noticeably decrease noticeable
https://codereview.webrtc.org/3003823003/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc (right): https://codereview.webrtc.org/3003823003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:165: // This field trial is intended to check if it is worth using a shorted On 2017/09/01 18:07:57, pbos-webrtc wrote: > shortened/shorter Done. https://codereview.webrtc.org/3003823003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:174: // frame until the next sync frame. So we expect a noticeably decrease On 2017/09/01 18:07:57, pbos-webrtc wrote: > noticeable Done.
sprang@webrtc.org changed reviewers: - pbos@google.com
Got approval from marpan2 out of band, gonna land this. Feel free to lgtm after the fact.
The CQ bit was checked by sprang@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from ilnik@webrtc.org, pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/3003823003/#ps60001 (title: "typos")
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: win_msvc_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_msvc_rel/builds/632) win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/29451) win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/12276)
The CQ bit was checked by sprang@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org, ilnik@webrtc.org Link to the patchset: https://codereview.webrtc.org/3003823003/#ps80001 (title: "Fixed int to uint cast in test")
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: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/27483)
The CQ bit was checked by sprang@webrtc.org
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": 80001, "attempt_start_ts": 1504706402663950,
"parent_rev": "35713eaf565c0fef07c8afc158d7b8fdf7ec3d78", "commit_rev":
"ff19d35bae3cb2567ad39f82dda9e1f9ded107c9"}
Message was sent while issue was closed.
Description was changed from ========== Fix FrameConfigs used for VP8 with four temporal layers. sync flag should only be true if: * temporal layer > 0 * frame predics _only_ from tl0 (last) Furthermore, flags should not predict from a buffer unless it has been previously updated in the current iteration for the pattern. I also added an experiment with an alternative pattern for a three layer setup, where loose some efficieny by halving the pattern but gain a little bit by updating arf for the top layer. The theory is that this will cause fewer frame drops since we don't have as much dependency on previous frames in the upper layer (which might not be retransmitted). BUG=webrtc:8162 ========== to ========== Fix FrameConfigs used for VP8 with four temporal layers. sync flag should only be true if: * temporal layer > 0 * frame predics _only_ from tl0 (last) Furthermore, flags should not predict from a buffer unless it has been previously updated in the current iteration for the pattern. I also added an experiment with an alternative pattern for a three layer setup, where loose some efficieny by halving the pattern but gain a little bit by updating arf for the top layer. The theory is that this will cause fewer frame drops since we don't have as much dependency on previous frames in the upper layer (which might not be retransmitted). BUG=webrtc:8162 Review-Url: https://codereview.webrtc.org/3003823003 Cr-Commit-Position: refs/heads/master@{#19716} Committed: https://chromium.googlesource.com/external/webrtc/+/ff19d35bae3cb2567ad39f82d... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/external/webrtc/+/ff19d35bae3cb2567ad39f82d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
