Derive current layer from TL frame config.

Derive current layer from TL frame config.

Partially removes "current frame" state from TemporalLayers, aiming to
tie more state to the frame config being encoded. This is necessary for
having several outstanding frames being encoded.

Also renames several structs, since TemporalReferences contains more
than references it's renamed TemporalLayers::FrameConfig.

BUG=chromium:702017, webrtc:7349
R=sprang@webrtc.org

Review-Url: https://codereview.webrtc.org/2853073004
Cr-Commit-Position: refs/heads/master@{#18012}
Committed: https://chromium.googlesource.com/external/webrtc/+/18ad1d4008ce5a1909310bafdd50897cfefc2de7

[pbos-webrtc, 2017-05-02 10:54:32]

PTAL

[pbos-webrtc, 2017-05-02 10:54:57]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 10:55:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-05-02 11:17:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-02 11:17:52]

Dry run: This issue passed the CQ dry run.

[sprang_webrtc, 2017-05-03 14:12:56]

sprang@webrtc.org changed reviewers:
+ sprang@webrtc.org
- brandt@webrtc.org

[sprang_webrtc, 2017-05-03 14:12:57]

R -brandt@ +me :)

[sprang_webrtc, 2017-05-03 15:18:43]

https://codereview.webrtc.org/2853073004/diff/1/webrtc/modules/video_coding/c...
File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc (right):

https://codereview.webrtc.org/2853073004/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:306: //
TODO(pbos): Name method so that it's obvious that it updates state.
Remove todo?

https://codereview.webrtc.org/2853073004/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:311: int
pattern_idx = ++pattern_idx_;
Is it intentional that we let pattern_idx_ go up to 255 before overflowing and
wrapping around rather than explicitly wrapping it at temporal_pattern_.size() ?
Right now this looks to me like it works only because temporal_pattern_.size()
always happen to be a divisor of 256..?
If it is desired I think a DCHECK and/or comment somewhere would be in order :)

https://codereview.webrtc.org/2853073004/diff/1/webrtc/modules/video_coding/c...
File webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc (right):

https://codereview.webrtc.org/2853073004/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:266: //            
make function const.
reflow comment

https://codereview.webrtc.org/2853073004/diff/1/webrtc/modules/video_coding/c...
File webrtc/modules/video_coding/codecs/vp8/temporal_layers.h (right):

https://codereview.webrtc.org/2853073004/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/temporal_layers.h:90:
TemporalLayers::FrameConfig tl_config,
Since it's a proper struct now, make it const TemporalLayers::FrameConfig&
here, below and in vp8_impl?

[pbos-webrtc, 2017-05-04 10:52:22]

PTAL :)

https://codereview.webrtc.org/2853073004/diff/1/webrtc/modules/video_coding/c...
File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc (right):

https://codereview.webrtc.org/2853073004/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:306: //
TODO(pbos): Name method so that it's obvious that it updates state.
On 2017/05/03 15:18:43, sprang_webrtc wrote:
> Remove todo?

Done.

https://codereview.webrtc.org/2853073004/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:311: int
pattern_idx = ++pattern_idx_;
On 2017/05/03 15:18:43, sprang_webrtc wrote:
> Is it intentional that we let pattern_idx_ go up to 255 before overflowing and
> wrapping around rather than explicitly wrapping it at temporal_pattern_.size()
?
> Right now this looks to me like it works only because temporal_pattern_.size()
> always happen to be a divisor of 256..?
> If it is desired I think a DCHECK and/or comment somewhere would be in order
:)

Did as suggested (explicitly wrap). I added a DCHECK above since pattern_idx is
also used to index into temporal_ids_, and a comment saying that if
temporal_ids_ is ever larger than temporal_patterns_ it needs to wrap at the
largest one.

https://codereview.webrtc.org/2853073004/diff/1/webrtc/modules/video_coding/c...
File webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc (right):

https://codereview.webrtc.org/2853073004/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:266: //            
make function const.
On 2017/05/03 15:18:43, sprang_webrtc wrote:
> reflow comment

Oops, done, thanks. :)

https://codereview.webrtc.org/2853073004/diff/1/webrtc/modules/video_coding/c...
File webrtc/modules/video_coding/codecs/vp8/temporal_layers.h (right):

https://codereview.webrtc.org/2853073004/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/temporal_layers.h:90:
TemporalLayers::FrameConfig tl_config,
On 2017/05/03 15:18:43, sprang_webrtc wrote:
> Since it's a proper struct now, make it const TemporalLayers::FrameConfig&
> here, below and in vp8_impl?

(FWIW: I didn't because I believe the struct is small enough to be faster to
copy than to pass pointer and dereference, though I agree that style consistency
is more important here.)

 Done.

[pbos-webrtc, 2017-05-04 10:52:22]

feedback

[pbos-webrtc, 2017-05-04 10:55:40]

Description was changed from

==========
Derive current layer from TL frame config.

Partially removes "current frame" state from TemporalLayers, aiming to
tie more state to the frame config being encoded. This is necessary for
having several outstanding frames being encoded.

Also renames several structs, since TemporalReferences contains more
than references it's renamed TemporalLayers::FrameConfig.

BUG=chromium:702017, webrtc:7349
R=brandt@webrtc.org
==========

to

==========
Derive current layer from TL frame config.

Partially removes "current frame" state from TemporalLayers, aiming to
tie more state to the frame config being encoded. This is necessary for
having several outstanding frames being encoded.

Also renames several structs, since TemporalReferences contains more
than references it's renamed TemporalLayers::FrameConfig.

BUG=chromium:702017, webrtc:7349
R=sprang@webrtc.org
==========

[pbos-webrtc, 2017-05-04 10:55:47]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 10:55:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-05-04 10:57:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-04 10:57:21]

Dry run: 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_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/20061)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/25375)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/24252)
  mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/25391)

[sprang_webrtc, 2017-05-04 11:30:16]

lgtm

[pbos-webrtc, 2017-05-04 11:44:20]

rebase

[pbos-webrtc, 2017-05-04 11:45:18]

The CQ bit was checked by pbos@webrtc.org

[pbos-webrtc, 2017-05-04 11:45:19]

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/2853073004/#ps40001 (title:
"rebase")

[commit-bot: I haz the power, 2017-05-04 11:45:24]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-05-04 12:04:46]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493898318732030,
"parent_rev": "173658968e80cc25e98038feb05efd750bdb8e9d", "commit_rev":
"18ad1d4008ce5a1909310bafdd50897cfefc2de7"}

[commit-bot: I haz the power, 2017-05-04 12:04:50]

Description was changed from

==========
Derive current layer from TL frame config.

Partially removes "current frame" state from TemporalLayers, aiming to
tie more state to the frame config being encoded. This is necessary for
having several outstanding frames being encoded.

Also renames several structs, since TemporalReferences contains more
than references it's renamed TemporalLayers::FrameConfig.

BUG=chromium:702017, webrtc:7349
R=sprang@webrtc.org
==========

to

==========
Derive current layer from TL frame config.

Partially removes "current frame" state from TemporalLayers, aiming to
tie more state to the frame config being encoded. This is necessary for
having several outstanding frames being encoded.

Also renames several structs, since TemporalReferences contains more
than references it's renamed TemporalLayers::FrameConfig.

BUG=chromium:702017, webrtc:7349
R=sprang@webrtc.org

Review-Url: https://codereview.webrtc.org/2853073004
Cr-Commit-Position: refs/heads/master@{#18012}
Committed:
https://chromium.googlesource.com/external/webrtc/+/18ad1d4008ce5a1909310bafd...
==========

[commit-bot: I haz the power, 2017-05-04 12:04:51]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/18ad1d4008ce5a1909310bafd...