Move temporal-layer properties to FrameConfig.

Move temporal-layer properties to FrameConfig.

Removes keying on pattern_idx inside TemporalLayers implementations for
several properties that are different between screencast temporal layers
and normal/default temporal layers.

This is a step towards sharing PopulateCodecSpecific between the layer
patterns and code deduplication to longer term be able to separate the
packetizer step from encoder settings, so that temporal patterns can be
used for asynchronous hardware encoders where there may be outstanding
frames.

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

Review-Url: https://codereview.webrtc.org/2924993002
Cr-Commit-Position: refs/heads/master@{#19097}
Committed: https://chromium.googlesource.com/external/webrtc/+/1777c5fec5f14b7ecc882e359e36cfe89903cbfb

[pbos-webrtc, 2017-06-07 01:31:35]

PTAL, I think this makes the code simpler (removes pattern_idx as an opaque
handle).

[pbos-webrtc, 2017-06-07 01:31:43]

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

[commit-bot: I haz the power, 2017-06-07 01:31:52]

Dry run: CQ is trying da patch.

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

[pbos-webrtc, 2017-06-07 01:35:29]

Description was changed from

==========
Move temporal-layer properties to FrameConfig.

Removes keying on pattern_idx inside TemporalLayers implementations for
several properties that are different between screencast temporal layers
and normal/default temporal layers.

This is a step towards sharing PopulateCodecSpecific between the layer
patterns and code deduplication to longer term be able to separate the
packetizer step from encoder settings, so that temporal patterns can be
used for asynchronous hardware encoders where there may be outstanding
frames.

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

to

==========
Move temporal-layer properties to FrameConfig.

Removes keying on pattern_idx inside TemporalLayers implementations for
several properties that are different between screencast temporal layers
and normal/default temporal layers.

This is a step towards sharing PopulateCodecSpecific between the layer
patterns and code deduplication to longer term be able to separate the
packetizer step from encoder settings, so that temporal patterns can be
used for asynchronous hardware encoders where there may be outstanding
frames.

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

[pbos-webrtc, 2017-06-07 01:35:29]

pbos@webrtc.org changed reviewers:
+ brandtr@webrtc.org
- brandt@webrtc.org

[commit-bot: I haz the power, 2017-06-07 01:47:46]

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

[commit-bot: I haz the power, 2017-06-07 01:47:47]

Dry run: Try jobs failed on following builders:
  linux_msan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/21016)

[pbos-webrtc, 2017-06-07 20:50:53]

remove timestamp check, input frame doesn't repeat

[pbos-webrtc, 2017-06-07 20:51:16]

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

[commit-bot: I haz the power, 2017-06-07 20:51:23]

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-06-07 20:53:32]

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

[commit-bot: I haz the power, 2017-06-07 20:53:33]

Dry run: Try jobs failed on following builders:
  win_clang_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_dbg/builds/14344)
  win_x64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/26154)

[pbos-webrtc, 2017-06-07 21:49:42]

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

[commit-bot: I haz the power, 2017-06-07 21:49:48]

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-06-07 22:08:10]

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

[commit-bot: I haz the power, 2017-06-07 22:08:11]

Dry run: This issue passed the CQ dry run.

[pbos-webrtc, 2017-06-08 23:20:48]

On 2017/06/07 20:51:10, pbos-webrtc wrote:

ping!

[brandtr, 2017-06-12 15:15:52]

brandtr@webrtc.org changed reviewers:
+ sprang@webrtc.org

[brandtr, 2017-06-12 15:15:53]

I think this looks good. But let Erik have a look too, since he is way more
familiar with this code than me.

https://codereview.webrtc.org/2924993002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc (left):

https://codereview.webrtc.org/2924993002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:365: if
(vp8_info->temporalIdx == 0 && timestamp != timestamp_) {
How about keeping timestamp != timestamp_ as a DCHECK?

https://codereview.webrtc.org/2924993002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc (right):

https://codereview.webrtc.org/2924993002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:119:
TemporalLayerState::kDrop;
revert?

https://codereview.webrtc.org/2924993002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:176:
TemporalLayers::FrameConfig(kReferenceAndUpdate, kNone, kNone);
Add tl_config.packetizer_temporal_idx = 0; explicitly?

[sprang_webrtc, 2017-06-13 11:39:23]

Looks good, but I think adding unit test could be in order.

https://codereview.webrtc.org/2924993002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/temporal_layers.h (right):

https://codereview.webrtc.org/2924993002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/temporal_layers.h:51: int
packetizer_temporal_idx;
Please add a comment explaining what the difference between these are and why
this hack exists. Maybe also a todo about cleaning this up when we (hopefully)
nuke screenshare_layers

[pbos-webrtc, 2017-06-15 21:59:09]

Done, which tests do you suggest I move into the common space? This seemed
fairly covered by existing default/screenshare layer tests.

https://codereview.webrtc.org/2924993002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc (left):

https://codereview.webrtc.org/2924993002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:365: if
(vp8_info->temporalIdx == 0 && timestamp != timestamp_) {
On 2017/06/12 15:15:53, brandtr wrote:
> How about keeping timestamp != timestamp_ as a DCHECK?

I believe that timestamp == timestamp_ is OK if this wraps around during a
pause, so technically it's correct to not check timestamp != timestamp_, so long
as we don't get repeat input frames? The int64_t increasing timestamp check is
handled in the capturer input.

Not sure if interesting corner case, also not sure this'll likely catch more
bugs?

https://codereview.webrtc.org/2924993002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc (right):

https://codereview.webrtc.org/2924993002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:119:
TemporalLayerState::kDrop;
On 2017/06/12 15:15:53, brandtr wrote:
> revert?

Done.

https://codereview.webrtc.org/2924993002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:176:
TemporalLayers::FrameConfig(kReferenceAndUpdate, kNone, kNone);
On 2017/06/12 15:15:53, brandtr wrote:
> Add tl_config.packetizer_temporal_idx = 0; explicitly?

Done.

https://codereview.webrtc.org/2924993002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/temporal_layers.h (right):

https://codereview.webrtc.org/2924993002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/temporal_layers.h:51: int
packetizer_temporal_idx;
On 2017/06/13 11:39:22, sprang_webrtc wrote:
> Please add a comment explaining what the difference between these are and why
> this hack exists. Maybe also a todo about cleaning this up when we (hopefully)
> nuke screenshare_layers

Done.

[brandtr, 2017-06-19 06:54:19]

lgtm

https://codereview.webrtc.org/2924993002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc (left):

https://codereview.webrtc.org/2924993002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:365: if
(vp8_info->temporalIdx == 0 && timestamp != timestamp_) {
On 2017/06/15 21:59:09, pbos-webrtc wrote:
> On 2017/06/12 15:15:53, brandtr wrote:
> > How about keeping timestamp != timestamp_ as a DCHECK?
> 
> I believe that timestamp == timestamp_ is OK if this wraps around during a
> pause, so technically it's correct to not check timestamp != timestamp_, so
long
> as we don't get repeat input frames? The int64_t increasing timestamp check is
> handled in the capturer input.
> 
> Not sure if interesting corner case, also not sure this'll likely catch more
> bugs?

Hmm, my idea was to guard against repeat input frames to avoid any confusion
with the tl0_pic_idx. But looking again, I guess it's more robust to not do the
check at all.

[sprang_webrtc, 2017-06-19 09:20:45]

On 2017/06/15 21:59:09, pbos-webrtc wrote:
> Done, which tests do you suggest I move into the common space? This seemed
> fairly covered by existing default/screenshare layer tests.
> 

I was thinking a simple test verifying FrameConfig.encoder_layer_id (which
doesn't seem to be directly touched by any unit test) vs temporal idx.

lgtm with that

[pbos-webrtc, 2017-07-19 21:40:50]

rebase

[pbos-webrtc, 2017-07-19 21:54:09]

fix operator

[pbos-webrtc, 2017-07-19 22:58:50]

simple layer_sync setter

[pbos-webrtc, 2017-07-19 23:31:02]

improve unittests

[pbos-webrtc, 2017-07-19 23:32:03]

The CQ bit was checked by pbos@webrtc.org

[pbos-webrtc, 2017-07-19 23:32:03]

The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org,
sprang@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2924993002/#ps120001 (title:
"improve unittests")

[commit-bot: I haz the power, 2017-07-19 23:32:09]

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-07-20 00:04:03]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1500507123010710,
"parent_rev": "c40e1d3fc78d734a4e261d7b0ce1e51aaa8bdc25", "commit_rev":
"1777c5fec5f14b7ecc882e359e36cfe89903cbfb"}

[commit-bot: I haz the power, 2017-07-20 00:04:08]

Description was changed from

==========
Move temporal-layer properties to FrameConfig.

Removes keying on pattern_idx inside TemporalLayers implementations for
several properties that are different between screencast temporal layers
and normal/default temporal layers.

This is a step towards sharing PopulateCodecSpecific between the layer
patterns and code deduplication to longer term be able to separate the
packetizer step from encoder settings, so that temporal patterns can be
used for asynchronous hardware encoders where there may be outstanding
frames.

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

to

==========
Move temporal-layer properties to FrameConfig.

Removes keying on pattern_idx inside TemporalLayers implementations for
several properties that are different between screencast temporal layers
and normal/default temporal layers.

This is a step towards sharing PopulateCodecSpecific between the layer
patterns and code deduplication to longer term be able to separate the
packetizer step from encoder settings, so that temporal patterns can be
used for asynchronous hardware encoders where there may be outstanding
frames.

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

Review-Url: https://codereview.webrtc.org/2924993002
Cr-Commit-Position: refs/heads/master@{#19097}
Committed:
https://chromium.googlesource.com/external/webrtc/+/1777c5fec5f14b7ecc882e359...
==========

[commit-bot: I haz the power, 2017-07-20 00:04:09]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/external/webrtc/+/1777c5fec5f14b7ecc882e359...