Use RC_TIMESTAMP_MODE for OpenH264.

Use RC_TIMESTAMP_MODE for OpenH264.

Performs rate control based on timestamp deltas instead of announced
frame rate.

BUG=webrtc:5855
R=hbos@webrtc.org

Committed: https://crrev.com/c4deee49a3ec42b7fe83c82f750539b36aae1d3f
Cr-Commit-Position: refs/heads/master@{#12611}

[pbos-webrtc, 2016-05-03 16:45:21]

PTAL, I also did some cleanup (there's CHECKs outside that we only ever get
kVideoFrameDelta or kVideoFrameKey in here).

[hbos, 2016-05-03 17:26:07]

On 2016/05/03 16:45:21, pbos-webrtc wrote:
> PTAL, I also did some cleanup (there's CHECKs outside that we only ever get
> kVideoFrameDelta or kVideoFrameKey in here).

"CHECKs that we only ever get kVideoFrameDelta or kVideoFrameKey" is not true,
you CHECK that frame_types is not null and only care about kVideoFrameKey, not
kVideoFrameDelta or other values.
Also, I noticed that vp8_impl.cc and vp9_impl.cc's Encode checks with an if
statement that |frame_types| is not null and ignores it otherwise instead of
crashing. Are you sure DCHECKing is the right thing to do?

[pbos-webrtc, 2016-05-03 17:35:27]

On 2016/05/03 17:26:07, hbos wrote:
> On 2016/05/03 16:45:21, pbos-webrtc wrote:
> > PTAL, I also did some cleanup (there's CHECKs outside that we only ever get
> > kVideoFrameDelta or kVideoFrameKey in here).
> 
> "CHECKs that we only ever get kVideoFrameDelta or kVideoFrameKey" is not true,
> you CHECK that frame_types is not null and only care about kVideoFrameKey, not
> kVideoFrameDelta or other values.
> Also, I noticed that vp8_impl.cc and vp9_impl.cc's Encode checks with an if
> statement that |frame_types| is not null and ignores it otherwise instead of
> crashing. Are you sure DCHECKing is the right thing to do?

https://chromium.googlesource.com/external/webrtc/+/87f8c0d07253a0be5b0c4579d...

I guess I should update those as well (in a follow-up).

[hbos, 2016-05-03 17:44:00]

On 2016/05/03 17:35:27, pbos-webrtc wrote:
> On 2016/05/03 17:26:07, hbos wrote:
> > On 2016/05/03 16:45:21, pbos-webrtc wrote:
> > > PTAL, I also did some cleanup (there's CHECKs outside that we only ever
get
> > > kVideoFrameDelta or kVideoFrameKey in here).
> > 
> > "CHECKs that we only ever get kVideoFrameDelta or kVideoFrameKey" is not
true,
> > you CHECK that frame_types is not null and only care about kVideoFrameKey,
not
> > kVideoFrameDelta or other values.
> > Also, I noticed that vp8_impl.cc and vp9_impl.cc's Encode checks with an if
> > statement that |frame_types| is not null and ignores it otherwise instead of
> > crashing. Are you sure DCHECKing is the right thing to do?
> 
>
https://chromium.googlesource.com/external/webrtc/+/87f8c0d07253a0be5b0c4579d...
> 
> I guess I should update those as well (in a follow-up).

LGTM

[pbos-webrtc, 2016-05-03 17:49:49]

The CQ bit was checked by pbos@webrtc.org

[commit-bot: I haz the power, 2016-05-03 17:50:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1945763002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1945763002/1

[pbos-webrtc, 2016-05-03 18:00:09]

Description was changed from

==========
Use RC_TIMESTAMP_MODE for OpenH264.

Performs rate control based on timestamp deltas instead of announced
frame rate.

BUG=webrtc:5855
R=hbos@webrtc.org
==========

to

==========
Use RC_TIMESTAMP_MODE for OpenH264.

Performs rate control based on timestamp deltas instead of announced
frame rate.

BUG=webrtc:5855
R=hbos@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/c4deee49a3ec42b7fe83c82f7...
==========

[commit-bot: I haz the power, 2016-05-03 18:00:09]

Description was changed from

==========
Use RC_TIMESTAMP_MODE for OpenH264.

Performs rate control based on timestamp deltas instead of announced
frame rate.

BUG=webrtc:5855
R=hbos@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/c4deee49a3ec42b7fe83c82f7...
==========

to

==========
Use RC_TIMESTAMP_MODE for OpenH264.

Performs rate control based on timestamp deltas instead of announced
frame rate.

BUG=webrtc:5855
R=hbos@webrtc.org

Committed: https://crrev.com/c4deee49a3ec42b7fe83c82f750539b36aae1d3f
Cr-Commit-Position: refs/heads/master@{#12611}
==========

[pbos-webrtc, 2016-05-03 18:00:10]

Committed patchset #1 (id:1) manually as
c4deee49a3ec42b7fe83c82f750539b36aae1d3f (presubmit successful).

[commit-bot: I haz the power, 2016-05-03 18:00:10]

Patchset 1 (id:??) landed as
https://crrev.com/c4deee49a3ec42b7fe83c82f750539b36aae1d3f
Cr-Commit-Position: refs/heads/master@{#12611}

[stefan-webrtc, 2016-05-04 07:01:49]

Could you give some background for this change? We initially decided against
RC_TIMESTAMP_MODE because it might bring unexpected behaviors. For instance, if
we don't have any frames to encode for several seconds (frames being dropped or
camera being muted) we wouldn't want the following frame to be very large. That
would defeat the purpose of dropping frames before the encoder.

We have made the same decision for VP8/VP9 wrappers.

[pbos-webrtc, 2016-05-04 14:17:43]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.webrtc.org/1950973002/ by pbos@webrtc.org.

The reason for reverting is: Previous mode aligns with other encoders, and
RC_TIMESTAMP_MODE might have issues with no frames for several seconds..