Don't re-randomize picture_id/tl0_pic_idx when re-initializing internal encoders.

Don't re-randomize picture_id/tl0_pic_idx when re-initializing internal encoders.

TESTED=video_loopback and AppRTCMobile with forced encoder reinits every 30 frames.
BUG=webrtc:7475
Review-Url: https://codereview.webrtc.org/2833493003
Cr-Commit-Position: refs/heads/master@{#17984}
Committed: https://chromium.googlesource.com/external/webrtc/+/080830c513023df6dbf11b42e92c9df59fc8c049

[brandtr, 2017-04-20 08:28:43]

Patchset #1 (id:1) has been deleted

[brandtr, 2017-04-20 08:33:11]

Patchset #1 (id:20001) has been deleted

[brandtr, 2017-04-20 08:59:10]

brandtr@webrtc.org changed reviewers:
+ asapersson@webrtc.org, magjed@webrtc.org, stefan@webrtc.org

[brandtr, 2017-04-20 08:59:11]

Please take a look.

asapersson: General review.
holmer: OWNER.
magjed: Android encoder.

https://codereview.webrtc.org/2833493003/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc (right):

https://codereview.webrtc.org/2833493003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:198: return
tl0_pic_idx_;
If the first frame after a reinit always is a tl0 frame (it should be, since
it's a key frame), then |tl0_pic_idx_| will be incremented here:
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/....

So we should return and initialize with the current value, not the current+1
value.

Please help me verify that I don't introduce an off-by-one error here :)

[stefan-webrtc, 2017-04-20 09:28:09]

What's your opinion about moving the randomization out to the rtp_rtcp instead?
That's where rtp timestamps are randomized, and that class already has logic to
retain the rtp state.

https://codereview.webrtc.org/2833493003/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc (right):

https://codereview.webrtc.org/2833493003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:198: return
tl0_pic_idx_;
On 2017/04/20 08:59:11, brandtr wrote:
> If the first frame after a reinit always is a tl0 frame (it should be, since
> it's a key frame), then |tl0_pic_idx_| will be incremented here:
>
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/....
> 
> So we should return and initialize with the current value, not the current+1
> value.
> 
> Please help me verify that I don't introduce an off-by-one error here :)

Maybe we can add a test in vp8_impl_unittests.cc to verify this?

https://codereview.webrtc.org/2833493003/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc (right):

https://codereview.webrtc.org/2833493003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:136:
random_(rtc::TimeMicros()),
Mostly FYI, but we discovered today that TimeMicros gives us millisecond
granularity on Windows.

[brandtr, 2017-04-20 09:38:51]

On 2017/04/20 09:28:09, stefan-webrtc wrote:
> What's your opinion about moving the randomization out to the rtp_rtcp
instead?
> That's where rtp timestamps are randomized, and that class already has logic
to
> retain the rtp state.

I think that would make sense long term. Then we might want to move the setting
of the picture_id to the packetization altogether. I'm not sure if this would be
possible for the tl0_pic_idx however, i.e., if that depends on information that
only the encoder has.

>
https://codereview.webrtc.org/2833493003/diff/40001/webrtc/modules/video_codi...
> File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc
(right):
> 
>
https://codereview.webrtc.org/2833493003/diff/40001/webrtc/modules/video_codi...
> webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:198: return
> tl0_pic_idx_;
> On 2017/04/20 08:59:11, brandtr wrote:
> > If the first frame after a reinit always is a tl0 frame (it should be, since
> > it's a key frame), then |tl0_pic_idx_| will be incremented here:
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/....
> > 
> > So we should return and initialize with the current value, not the current+1
> > value.
> > 
> > Please help me verify that I don't introduce an off-by-one error here :)
> 
> Maybe we can add a test in vp8_impl_unittests.cc to verify this?

Yes, that's a good idea. I'll have to extend the unit test to support temporal
layers then.

>
https://codereview.webrtc.org/2833493003/diff/40001/webrtc/modules/video_codi...
> File webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc (right):
> 
>
https://codereview.webrtc.org/2833493003/diff/40001/webrtc/modules/video_codi...
> webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:136:
> random_(rtc::TimeMicros()),
> Mostly FYI, but we discovered today that TimeMicros gives us millisecond
> granularity on Windows.

I was going to come over and ask terelius/nisse about this, but you were in a
meeting. I think ms granularity is fine, and if that is more robust I might as
well change to that.

[magjed_webrtc, 2017-04-20 12:39:12]

https://codereview.webrtc.org/2833493003/diff/40001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc (right):

https://codereview.webrtc.org/2833493003/diff/40001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc:336:
picture_id_(random_.Rand<uint16_t>() & 0x7FFF),
Is it possible to pass in the start |picture_id_| and |tl0_pic_idx_| to this
class instead of using a random number generator?

[stefan-webrtc, 2017-04-20 13:45:36]

Just to make sure we don't forget about my comment at lunch today:

We should make sure we handle the case where a HW encoder fails and we fall back
to SW, so that the SW doesn't start at a different random picture id.

[brandtr, 2017-04-21 08:29:31]

https://codereview.webrtc.org/2833493003/diff/40001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc (right):

https://codereview.webrtc.org/2833493003/diff/40001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc:336:
picture_id_(random_.Rand<uint16_t>() & 0x7FFF),
On 2017/04/20 12:39:12, magjed_webrtc wrote:
> Is it possible to pass in the start |picture_id_| and |tl0_pic_idx_| to this
> class instead of using a random number generator?

That is something that we maybe want to do long-term. Then we would treat
PictureID and Tl0PicIdx as part of the RTP state, which is retained at the Call
level. Making that change would be a medium-sized refactoring however, including
changing the encoder interface.

For now, I think the change in this CL is an improvement over the current
behaviour however. Together with https://codereview.webrtc.org/2830793005/, we
will stop re-randomizing PictureID during a call, thus making live easier for
the jitter buffer.

According to https://tools.ietf.org/html/rfc7741#section-4.2, we don't
necessarily have to randomize the initial PictureID at all. I'm not sure if
there is any benefit from not randomizing it, except for not having to
instantiate a PRNG here.

[magjed_webrtc, 2017-04-21 15:11:19]

androidmediaencoder_jni.cc lgtm

https://codereview.webrtc.org/2833493003/diff/40001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc (right):

https://codereview.webrtc.org/2833493003/diff/40001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc:297: webrtc::Random
random_;
I would like to not keep this as a member variable since it's only used in the
ctor, so I prefer to move the initialization of picture_id_ and tl0_pic_idx_
into the ctor body and having random_ as a local variable.

https://codereview.webrtc.org/2833493003/diff/40001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc:336:
picture_id_(random_.Rand<uint16_t>() & 0x7FFF),
On 2017/04/21 08:29:31, brandtr wrote:
> On 2017/04/20 12:39:12, magjed_webrtc wrote:
> > Is it possible to pass in the start |picture_id_| and |tl0_pic_idx_| to this
> > class instead of using a random number generator?
> 
> That is something that we maybe want to do long-term. Then we would treat
> PictureID and Tl0PicIdx as part of the RTP state, which is retained at the
Call
> level. Making that change would be a medium-sized refactoring however,
including
> changing the encoder interface.
> 
> For now, I think the change in this CL is an improvement over the current
> behaviour however. Together with https://codereview.webrtc.org/2830793005/, we
> will stop re-randomizing PictureID during a call, thus making live easier for
> the jitter buffer.
> 
> According to https://tools.ietf.org/html/rfc7741#section-4.2, we don't
> necessarily have to randomize the initial PictureID at all. I'm not sure if
> there is any benefit from not randomizing it, except for not having to
> instantiate a PRNG here.

Acknowledged.

[brandtr, 2017-04-24 07:53:47]

https://codereview.webrtc.org/2833493003/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc (right):

https://codereview.webrtc.org/2833493003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:136:
random_(rtc::TimeMicros()),
On 2017/04/20 09:28:09, stefan-webrtc wrote:
> Mostly FYI, but we discovered today that TimeMicros gives us millisecond
> granularity on Windows.

Changed to TimeMillis(). That should be granular enough.

https://codereview.webrtc.org/2833493003/diff/40001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc (right):

https://codereview.webrtc.org/2833493003/diff/40001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc:297: webrtc::Random
random_;
On 2017/04/21 15:11:18, magjed_webrtc wrote:
> I would like to not keep this as a member variable since it's only used in the
> ctor, so I prefer to move the initialization of picture_id_ and tl0_pic_idx_
> into the ctor body and having random_ as a local variable.

Good idea. Done.

[brandtr, 2017-04-24 12:25:31]

On 2017/04/20 13:45:36, stefan-webrtc wrote:
> Just to make sure we don't forget about my comment at lunch today:
> 
> We should make sure we handle the case where a HW encoder fails and we fall
back
> to SW, so that the SW doesn't start at a different random picture id.

This is a good idea, so I've added it to my TODO list. In general, we need to
consider how this RTP state interacts with out mix of adapters, such as SW
fallback and SimulcastEncoderAdapter.

The current state, before this CL is landed, is that we _will_ randomize the
picture_id when falling back to SW VP8 or SW VP9. After this CL, we will still
randomize the picture_id when falling back. So the behaviour stays the same. So
I think making this change belongs in a larger refactoring effort, where we move
the picture_id/tl0_pic_idx state out of the encoder and into some intermediary
layer.

[brandtr, 2017-04-26 11:38:28]

Patchset #3 (id:80001) has been deleted

[brandtr, 2017-04-26 11:57:01]

holmer: Added unit tests, as requested.

[åsapersson, 2017-04-27 12:09:39]

lgtm with comment fixed

https://codereview.webrtc.org/2833493003/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc (right):

https://codereview.webrtc.org/2833493003/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc:343:
encoder_->Encode(*input_frame_, nullptr, nullptr));
increase timestamp for new frames (since it is used to determine tl0_pic_idx_)

[brandtr, 2017-04-27 13:00:55]

On 2017/04/27 12:09:39, åsapersson wrote:
> lgtm with comment fixed
> 
>
https://codereview.webrtc.org/2833493003/diff/100001/webrtc/modules/video_cod...
> File webrtc/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc (right):
> 
>
https://codereview.webrtc.org/2833493003/diff/100001/webrtc/modules/video_cod...
> webrtc/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc:343:
> encoder_->Encode(*input_frame_, nullptr, nullptr));
> increase timestamp for new frames (since it is used to determine tl0_pic_idx_)

Good catch, thanks! The tests in the previous PS didn't fail since I only ever
pushed through two frames. Now I'm incrementing the timestamps and pushing
through four frames.

[stefan-webrtc, 2017-05-02 07:21:50]

lgtm, but consider my comments

https://codereview.webrtc.org/2833493003/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/test/video_codec_test.h (right):

https://codereview.webrtc.org/2833493003/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/video_codec_test.h:81: VideoCodec
codec_inst_;
codec_inst_ has always been a strange name to me. Should we use codec_settings_
or something else instead? :)

https://codereview.webrtc.org/2833493003/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc (right):

https://codereview.webrtc.org/2833493003/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc:218: void
ExpectFrame(int16_t picture_id, int tl0_pic_idx, uint8_t temporal_idx) {
ExpectFrameWith() might be a better name

https://codereview.webrtc.org/2833493003/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp9/test/vp9_impl_unittest.cc (right):

https://codereview.webrtc.org/2833493003/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/test/vp9_impl_unittest.cc:35: void
ExpectFrame(int16_t picture_id, int tl0_pic_idx, uint8_t temporal_idx) {
ExpectFrameWith here too perhaps

[brandtr, 2017-05-02 07:24:47]

Rebase.

[brandtr, 2017-05-02 07:40:14]

Patchset #6 (id:160001) has been deleted

[brandtr, 2017-05-02 07:48:50]

https://codereview.webrtc.org/2833493003/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/test/video_codec_test.h (right):

https://codereview.webrtc.org/2833493003/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/video_codec_test.h:81: VideoCodec
codec_inst_;
On 2017/05/02 07:21:50, stefan-webrtc wrote:
> codec_inst_ has always been a strange name to me. Should we use
codec_settings_
> or something else instead? :)

Sounds good to me. I have never understood what |inst_| means either...
Instance?

https://codereview.webrtc.org/2833493003/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc (right):

https://codereview.webrtc.org/2833493003/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc:218: void
ExpectFrame(int16_t picture_id, int tl0_pic_idx, uint8_t temporal_idx) {
On 2017/05/02 07:21:50, stefan-webrtc wrote:
> ExpectFrameWith() might be a better name

Done.

https://codereview.webrtc.org/2833493003/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp9/test/vp9_impl_unittest.cc (right):

https://codereview.webrtc.org/2833493003/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/test/vp9_impl_unittest.cc:35: void
ExpectFrame(int16_t picture_id, int tl0_pic_idx, uint8_t temporal_idx) {
On 2017/05/02 07:21:50, stefan-webrtc wrote:
> ExpectFrameWith here too perhaps

Done.

[brandtr, 2017-05-02 07:48:59]

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

[commit-bot: I haz the power, 2017-05-02 07:49:10]

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 08:19:45]

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

[commit-bot: I haz the power, 2017-05-02 08:19:46]

Dry run: This issue passed the CQ dry run.

[brandtr, 2017-05-03 07:00:48]

Most other uses of webrtc::Random uses microsecond granularity for the seed, so
I changed back to using that here as well. Should probably not matter, but might
as well be consistent.

Will land during the day.

[brandtr, 2017-05-03 08:53:27]

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

[commit-bot: I haz the power, 2017-05-03 08:53:37]

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-03 09:15:37]

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

[commit-bot: I haz the power, 2017-05-03 09:15:38]

Dry run: This issue passed the CQ dry run.

[brandtr, 2017-05-03 10:23:05]

The CQ bit was checked by brandtr@webrtc.org

[brandtr, 2017-05-03 10:23:06]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
asapersson@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2833493003/#ps200001 (title:
"Change back TimeMills() -> TimeMicros().")

[commit-bot: I haz the power, 2017-05-03 10:23:18]

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-03 10:25:53]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1493806985667940,
"parent_rev": "4ed18da9903f6fdb4a502def3e0e6f0408463940", "commit_rev":
"080830c513023df6dbf11b42e92c9df59fc8c049"}

[commit-bot: I haz the power, 2017-05-03 10:25:57]

Description was changed from

==========
Don't re-randomize picture_id/tl0_pic_idx when re-initializing internal
encoders.

TESTED=video_loopback and AppRTCMobile with forced encoder reinits every 30
frames.
BUG=webrtc:7475
==========

to

==========
Don't re-randomize picture_id/tl0_pic_idx when re-initializing internal
encoders.

TESTED=video_loopback and AppRTCMobile with forced encoder reinits every 30
frames.
BUG=webrtc:7475

Review-Url: https://codereview.webrtc.org/2833493003
Cr-Commit-Position: refs/heads/master@{#17984}
Committed:
https://chromium.googlesource.com/external/webrtc/+/080830c513023df6dbf11b42e...
==========

[commit-bot: I haz the power, 2017-05-03 10:25:58]

Committed patchset #7 (id:200001) as
https://chromium.googlesource.com/external/webrtc/+/080830c513023df6dbf11b42e...