Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2747123005:
Simplify default temporal layers. (Closed)
|
Created:
3 years, 9 months ago by pbos-webrtc Modified:
3 years, 9 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhengzhonghou_agora.io, video-team_agora.io, stefan-webrtc, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionSimplify default temporal layers.
Moves towards separating which layers may be referenced instead of
referencing libvpx flags directly. This will make strategies easier to
extract and usable from hardware encoders (RTCVideoEncoder, for
instance).
BUG=chromium:702017, webrtc:7349
R=brandtr@webrtc.org, marpan@webrtc.org. sprang@webrtc.org
Review-Url: https://codereview.webrtc.org/2747123005
Cr-Commit-Position: refs/heads/master@{#17349}
Committed: https://chromium.googlesource.com/external/webrtc/+/e6fe2a9a7f489ae6cf2a1f9df214394221965369
Patch Set 1 #Patch Set 2 : simplify TemporalReferences #Patch Set 3 : move GetTemporalPattern out of class #
Total comments: 16
Patch Set 4 : nits + add comments #
Total comments: 1
Patch Set 5 : update comment #Patch Set 6 : updated comment #Patch Set 7 : update comments so that it doesn't look like updates are optional #
Messages
Total messages: 31 (14 generated)
PTAL, I'd love some ideas for better naming here specifically. If you agree we can also remove last/golden/arf naming and call them buffer[0,1,2]. WDYT?
Description was changed from ========== Simplify default temporal layers. Moves towards separating which layers may be referenced instead of referencing libvpx flags directly. This will make strategies easier to extract and usable from hardware encoders (RTCVideoEncoder). BUG=chromium:702017, webrtc:7349 R=marpan@webrtc.org ========== to ========== Simplify default temporal layers. Moves towards separating which layers may be referenced instead of referencing libvpx flags directly. This will make strategies easier to extract and usable from hardware encoders (RTCVideoEncoder, for instance). BUG=chromium:702017, webrtc:7349 R=marpan@webrtc.org ==========
simplify TemporalReferences
PTAL, I think this made it a bit easier now. Could probably still use some help naming things. :)
move GetTemporalPattern out of class
The CQ bit was checked by pbos@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/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
pbos@webrtc.org changed reviewers: + brandtr@webrtc.org
Rasmus can you take a look as well? :)
Description was changed from ========== Simplify default temporal layers. Moves towards separating which layers may be referenced instead of referencing libvpx flags directly. This will make strategies easier to extract and usable from hardware encoders (RTCVideoEncoder, for instance). BUG=chromium:702017, webrtc:7349 R=marpan@webrtc.org ========== to ========== Simplify default temporal layers. Moves towards separating which layers may be referenced instead of referencing libvpx flags directly. This will make strategies easier to extract and usable from hardware encoders (RTCVideoEncoder, for instance). BUG=chromium:702017, webrtc:7349 R=brandtr@webrtc.org, marpan@webrtc.org ==========
https://codereview.webrtc.org/2747123005/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.h (right): https://codereview.webrtc.org/2747123005/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.h:40: const bool reference_last; The idea is that these bools here are easier to understand and translate to hardware encoder parameters. They would have to do conversion similar to ::EncodeFlags() in the .cc file.
Nice refactoring! I think there's opportunity to make this logic even easier to understand, by adding some visual cues. See comments. Do we feel confident in the coverage provided by default_temporal_layers_unittest.cc? In terms of reference/update flags, it looks fairly comprehensive to me. It's not checking ts_target_bitrate and ts_rate_decimator however. https://codereview.webrtc.org/2747123005/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc (right): https://codereview.webrtc.org/2747123005/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:58: namespace { nit: New line between l58 and l59. (Or remove space on l137.) https://codereview.webrtc.org/2747123005/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:67: case 4: Might be good to add a diagram showing these IDs visually? Something like https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/.... Could be combined with the next comment. https://codereview.webrtc.org/2747123005/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:81: return {TemporalReferences(kReferenceAndUpdate, kReference, kReference)}; It would be great with some simple ASCII art here, showing the reference dependency graphs for the different layer setups. Then it would also be fairly easy to understand what reference buffers need to be updated for the different layers. (The latter information is described by the now deleted comments here: https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/...) https://codereview.webrtc.org/2747123005/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.h (right): https://codereview.webrtc.org/2747123005/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.h:43: const bool update_golden; I like these names instead of "buffer 0, 1, 2". What would the benefit of naming them with numbers be? That we can generalize this to other codecs later on?
sprang@webrtc.org changed reviewers: + sprang@webrtc.org
looks good, this is a lot cleaner than before! Agree some basic ascii art would be appreciated though :) https://codereview.webrtc.org/2747123005/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc (right): https://codereview.webrtc.org/2747123005/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:67: case 4: On 2017/03/21 12:20:09, brandtr wrote: > Might be good to add a diagram showing these IDs visually? Something like > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/.... > > Could be combined with the next comment. Do we ever use 4 temporal layers in practice? https://codereview.webrtc.org/2747123005/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:212: int patternIdx = ++pattern_idx_ % temporal_pattern_.size(); Why not just update pattern_idx_ itself? https://codereview.webrtc.org/2747123005/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:237: assert(num_layers_ > 0); nit: RTC_[D]CHECK_GT
nits + add comments
PTAL https://codereview.webrtc.org/2747123005/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc (right): https://codereview.webrtc.org/2747123005/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:58: namespace { On 2017/03/21 12:20:09, brandtr wrote: > nit: New line between l58 and l59. (Or remove space on l137.) Done. https://codereview.webrtc.org/2747123005/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:67: case 4: On 2017/03/21 13:50:38, språng wrote: > On 2017/03/21 12:20:09, brandtr wrote: > > Might be good to add a diagram showing these IDs visually? Something like > > > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/.... > > > > Could be combined with the next comment. > > Do we ever use 4 temporal layers in practice? No, but they are supported in RTP, hardware encoders can support them and if we expose temporal layers to javascript we might want to support up to four layers. https://codereview.webrtc.org/2747123005/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:67: case 4: On 2017/03/21 12:20:09, brandtr wrote: > Might be good to add a diagram showing these IDs visually? Something like > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/.... > > Could be combined with the next comment. Done. https://codereview.webrtc.org/2747123005/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:81: return {TemporalReferences(kReferenceAndUpdate, kReference, kReference)}; On 2017/03/21 12:20:09, brandtr wrote: > It would be great with some simple ASCII art here, showing the reference > dependency graphs for the different layer setups. Then it would also be fairly > easy to understand what reference buffers need to be updated for the different > layers. (The latter information is described by the now deleted comments here: > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/...) Since this contains all references drawing this is really hard. I put in some descriptive comments instead, PTAL. https://codereview.webrtc.org/2747123005/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:212: int patternIdx = ++pattern_idx_ % temporal_pattern_.size(); On 2017/03/21 13:50:38, språng wrote: > Why not just update pattern_idx_ itself? pattern_idx_ is used for both temporal_pattern_.size() and temporal_ids_.size() which are different lengths. https://codereview.webrtc.org/2747123005/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:237: assert(num_layers_ > 0); On 2017/03/21 13:50:38, språng wrote: > nit: RTC_[D]CHECK_GT Done. https://codereview.webrtc.org/2747123005/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.h (right): https://codereview.webrtc.org/2747123005/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.h:43: const bool update_golden; On 2017/03/21 12:20:09, brandtr wrote: > I like these names instead of "buffer 0, 1, 2". What would the benefit of naming > them with numbers be? That we can generalize this to other codecs later on? Potentially that the buffer used doesn't really matter, but I think I like keeping them too. https://codereview.webrtc.org/2747123005/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc (right): https://codereview.webrtc.org/2747123005/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:147: TemporalReferences(kReference, kReference, kReference, marpan@: This does not look like a proper layer sync, it references previous TL2 and TL1 buffers. Should this not be kReference, kNone, kNone) ? This pattern is the same as the one before and verified in https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/... where kTemporalUpdateNone references all buffers but update none of them.
update comment
Description was changed from ========== Simplify default temporal layers. Moves towards separating which layers may be referenced instead of referencing libvpx flags directly. This will make strategies easier to extract and usable from hardware encoders (RTCVideoEncoder, for instance). BUG=chromium:702017, webrtc:7349 R=brandtr@webrtc.org, marpan@webrtc.org ========== to ========== Simplify default temporal layers. Moves towards separating which layers may be referenced instead of referencing libvpx flags directly. This will make strategies easier to extract and usable from hardware encoders (RTCVideoEncoder, for instance). BUG=chromium:702017, webrtc:7349 R=brandtr@webrtc.org, marpan@webrtc.org. sprang@webrtc.org ==========
updated comment
The lists of TemporalReferences look compatible with the comments, so this lgtm. https://codereview.webrtc.org/2747123005/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc (right): https://codereview.webrtc.org/2747123005/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:81: return {TemporalReferences(kReferenceAndUpdate, kReference, kReference)}; On 2017/03/21 17:37:46, pbos-webrtc wrote: > On 2017/03/21 12:20:09, brandtr wrote: > > It would be great with some simple ASCII art here, showing the reference > > dependency graphs for the different layer setups. Then it would also be fairly > > easy to understand what reference buffers need to be updated for the different > > layers. (The latter information is described by the now deleted comments here: > > > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/...) > > Since this contains all references drawing this is really hard. I put in some > descriptive comments instead, PTAL. Much easier to read through the lists now, thanks.
marpan@google.com changed reviewers: + marpan@google.com
lgtm
update comments so that it doesn't look like updates are optional
The CQ bit was checked by pbos@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org, marpan@google.com Link to the patchset: https://codereview.webrtc.org/2747123005/#ps120001 (title: "update comments so that it doesn't look like updates are optional")
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": 120001, "attempt_start_ts": 1490213819865080,
"parent_rev": "9342e0a9c256be6d40498f1b1868cc5fe0874ec4", "commit_rev":
"e6fe2a9a7f489ae6cf2a1f9df214394221965369"}
Message was sent while issue was closed.
Description was changed from ========== Simplify default temporal layers. Moves towards separating which layers may be referenced instead of referencing libvpx flags directly. This will make strategies easier to extract and usable from hardware encoders (RTCVideoEncoder, for instance). BUG=chromium:702017, webrtc:7349 R=brandtr@webrtc.org, marpan@webrtc.org. sprang@webrtc.org ========== to ========== Simplify default temporal layers. Moves towards separating which layers may be referenced instead of referencing libvpx flags directly. This will make strategies easier to extract and usable from hardware encoders (RTCVideoEncoder, for instance). BUG=chromium:702017, webrtc:7349 R=brandtr@webrtc.org, marpan@webrtc.org. sprang@webrtc.org Review-Url: https://codereview.webrtc.org/2747123005 Cr-Commit-Position: refs/heads/master@{#17349} Committed: https://chromium.googlesource.com/external/webrtc/+/e6fe2a9a7f489ae6cf2a1f9df... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/e6fe2a9a7f489ae6cf2a1f9df... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
