In screenshare mode, suppress VP8 bitrate overshoot and increase quality

In screenshare mode, suppress VP8 bitrate overshoot and increase quality

This change includes several improvements:

* VP8 configured with new rate control
* Detection of frame dropping, with qp bump for next frame
* Increased target and TL0 bitrates
* Reworked rate control (TL allocation) in screenshare_layers

A note on performance: PSNR and SSIM is expected to get slightly worse with this cl. Frame drops and delays should however improve.

BUG=4171
R=pbos@webrtc.org, stefan@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/2c4c9148191a10c0e82c9a209d454c6b1ebbaf20

[sprang_webrtc, 2015-06-17 12:18:21]

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

[sprang_webrtc, 2015-06-17 12:18:22]

Please take a first pass!
I'm working on an new screenshare_layers_unittest, but please add any comments
you have on the feature code in the meantime, since we're short on time...

[stefan-webrtc, 2015-06-18 09:39:02]

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

https://codereview.webrtc.org/1193513006/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:125: int
max_bitrate_kbit,
Should these say kbps instead?

https://codereview.webrtc.org/1193513006/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:147:
layers_[1].enhanced_max_qp = min_qp_ + (((max_qp_ - min_qp_) * 85) / 100);
80% more in the lower layer and 85% more in the higher layer? Comment

https://codereview.webrtc.org/1193513006/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:166:
layers_[active_layer_].state = TemporalLayer::State::kQualityBoost;
This means we always boost quality after a drop? Doesn't that defeat the purpose
of the drop somehow?

https://codereview.webrtc.org/1193513006/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:206: if
(layers_[0].last_qp == -1 || layers_[1].last_qp == -1)
This means that the first frame should be a sync frame? Might want to comment on
what you want to achieve.

https://codereview.webrtc.org/1193513006/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:220: if
(layers_[active_layer_].state == TemporalLayer::State::kQualityBoost &&
Comment to describe what you are doing with this if-else.

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

https://codereview.webrtc.org/1193513006/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:671: codec_.mode ==
kScreensharing ? 2 : 0);
What does 2 mean?

https://codereview.webrtc.org/1193513006/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:848: vpx_codec_enc_cfg_t
temp_config;
What is temporary with this config? Isn't it a screencast config?

https://codereview.webrtc.org/1193513006/diff/1/webrtc/modules/video_coding/m...
File webrtc/modules/video_coding/main/source/generic_encoder.cc (right):

https://codereview.webrtc.org/1193513006/diff/1/webrtc/modules/video_coding/m...
webrtc/modules/video_coding/main/source/generic_encoder.cc:121: if
(is_screenshare_ && result == WEBRTC_VIDEO_CODEC_OK &&
This is really hard to understand. What Does it do, and why? Seems to rely on
the underlying encoder being synchronous and finish encoding/return a frame
before it the Encode() call returns?

[sprang_webrtc, 2015-06-18 13:05:03]

Addressed comments. Some tests added, more on the way.

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

https://codereview.webrtc.org/1193513006/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:125: int
max_bitrate_kbit,
On 2015/06/18 09:39:02, stefan-webrtc (holmer) wrote:
> Should these say kbps instead?

Done.

https://codereview.webrtc.org/1193513006/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:147:
layers_[1].enhanced_max_qp = min_qp_ + (((max_qp_ - min_qp_) * 85) / 100);
On 2015/06/18 09:39:02, stefan-webrtc (holmer) wrote:
> 80% more in the lower layer and 85% more in the higher layer? Comment

Done.

https://codereview.webrtc.org/1193513006/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:166:
layers_[active_layer_].state = TemporalLayer::State::kQualityBoost;
On 2015/06/18 09:39:02, stefan-webrtc (holmer) wrote:
> This means we always boost quality after a drop? Doesn't that defeat the
purpose
> of the drop somehow?

The rate control will be reset to start at max qp, so the quality will start out
_very_ low, and then ramp up, after a drop.

https://codereview.webrtc.org/1193513006/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:206: if
(layers_[0].last_qp == -1 || layers_[1].last_qp == -1)
On 2015/06/18 09:39:02, stefan-webrtc (holmer) wrote:
> This means that the first frame should be a sync frame? Might want to comment
on
> what you want to achieve.

Done.

https://codereview.webrtc.org/1193513006/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:220: if
(layers_[active_layer_].state == TemporalLayer::State::kQualityBoost &&
On 2015/06/18 09:39:02, stefan-webrtc (holmer) wrote:
> Comment to describe what you are doing with this if-else.

Done.

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

https://codereview.webrtc.org/1193513006/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:671: codec_.mode ==
kScreensharing ? 2 : 0);
On 2015/06/18 09:39:02, stefan-webrtc (holmer) wrote:
> What does 2 mean?

Commented

https://codereview.webrtc.org/1193513006/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:848: vpx_codec_enc_cfg_t
temp_config;
On 2015/06/18 09:39:02, stefan-webrtc (holmer) wrote:
> What is temporary with this config? Isn't it a screencast config?

It's not stored in configurations_ and will be reset on the next update. Added
comment.

https://codereview.webrtc.org/1193513006/diff/1/webrtc/modules/video_coding/m...
File webrtc/modules/video_coding/main/source/generic_encoder.cc (right):

https://codereview.webrtc.org/1193513006/diff/1/webrtc/modules/video_coding/m...
webrtc/modules/video_coding/main/source/generic_encoder.cc:121: if
(is_screenshare_ && result == WEBRTC_VIDEO_CODEC_OK &&
On 2015/06/18 09:39:02, stefan-webrtc (holmer) wrote:
> This is really hard to understand. What Does it do, and why? Seems to rely on
> the underlying encoder being synchronous and finish encoding/return a frame
> before it the Encode() call returns?

Yes, this was an ad-hoc way to discover that the encoder had dropped a frame a
we needed to re-encode it. I've changed it so the encoder returns a special code
for this case instead.

[sprang_webrtc, 2015-06-23 13:57:02]

Ping?

[stefan-webrtc, 2015-06-23 14:28:33]

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

https://codereview.webrtc.org/1193513006/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:671: codec_.mode ==
kScreensharing ? 2 : 0);
On 2015/06/18 13:05:02, språng wrote:
> On 2015/06/18 09:39:02, stefan-webrtc (holmer) wrote:
> > What does 2 mean?
> 
> Commented

Still doesn't say what 2 means

https://codereview.webrtc.org/1193513006/diff/1/webrtc/modules/video_coding/m...
File webrtc/modules/video_coding/main/source/generic_encoder.cc (right):

https://codereview.webrtc.org/1193513006/diff/1/webrtc/modules/video_coding/m...
webrtc/modules/video_coding/main/source/generic_encoder.cc:121: if
(is_screenshare_ && result == WEBRTC_VIDEO_CODEC_OK &&
On 2015/06/18 13:05:02, språng wrote:
> On 2015/06/18 09:39:02, stefan-webrtc (holmer) wrote:
> > This is really hard to understand. What Does it do, and why? Seems to rely
on
> > the underlying encoder being synchronous and finish encoding/return a frame
> > before it the Encode() call returns?
> 
> Yes, this was an ad-hoc way to discover that the encoder had dropped a frame a
> we needed to re-encode it. I've changed it so the encoder returns a special
code
> for this case instead.

I'm wondering if it would be better to have this logic in vp8_impl.cc instead?
Or is there a reason to have it generically?

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

https://codereview.webrtc.org/1193513006/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:24: static const
int kMaxQpDeltaForSync = 8;
It's a bit strange to call this Max, since it can be overridden by the time
check against kMaxTimeBetweenSyncs.

https://codereview.webrtc.org/1193513006/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:112: if
(last_timestamp_ == 0) {
last_timestamp_ can become 0 mid stream, so it would probably be better to use
-1 as init value.

https://codereview.webrtc.org/1193513006/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:222: // First frame
in TL1 should only depend on TL0 since there are not
there are no

https://codereview.webrtc.org/1193513006/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:227: DCHECK_NE(0u,
last_sync_timestamp_);
Can't this be 0 mid stream?

https://codereview.webrtc.org/1193513006/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/screenshare_layers.h (right):

https://codereview.webrtc.org/1193513006/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.h:92: } layers_[2];
We should name the constant 2 as it's used in more than one place (also in the
.cc)

[sprang_webrtc, 2015-06-23 15:07:31]

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

https://codereview.webrtc.org/1193513006/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:671: codec_.mode ==
kScreensharing ? 2 : 0);
On 2015/06/23 14:28:32, stefan-webrtc (holmer) wrote:
> On 2015/06/18 13:05:02, språng wrote:
> > On 2015/06/18 09:39:02, stefan-webrtc (holmer) wrote:
> > > What does 2 mean?
> > 
> > Commented
> 
> Still doesn't say what 2 means

You mean you miss the actual 2? :)

https://codereview.webrtc.org/1193513006/diff/1/webrtc/modules/video_coding/m...
File webrtc/modules/video_coding/main/source/generic_encoder.cc (right):

https://codereview.webrtc.org/1193513006/diff/1/webrtc/modules/video_coding/m...
webrtc/modules/video_coding/main/source/generic_encoder.cc:121: if
(is_screenshare_ && result == WEBRTC_VIDEO_CODEC_OK &&
On 2015/06/23 14:28:32, stefan-webrtc (holmer) wrote:
> On 2015/06/18 13:05:02, språng wrote:
> > On 2015/06/18 09:39:02, stefan-webrtc (holmer) wrote:
> > > This is really hard to understand. What Does it do, and why? Seems to rely
> on
> > > the underlying encoder being synchronous and finish encoding/return a
frame
> > > before it the Encode() call returns?
> > 
> > Yes, this was an ad-hoc way to discover that the encoder had dropped a frame
a
> > we needed to re-encode it. I've changed it so the encoder returns a special
> code
> > for this case instead.
> 
> I'm wondering if it would be better to have this logic in vp8_impl.cc instead?
> Or is there a reason to have it generically?

It's more difficult since we don't know until after GetEncodedPartitions if this
happpened or not. We would need to wrap Encode inside a method just for this or
risk using recursion.
While not needed for any other impl, I think it is not inconceivable that we
would like to use this elsewhere?

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

https://codereview.webrtc.org/1193513006/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:24: static const
int kMaxQpDeltaForSync = 8;
On 2015/06/23 14:28:32, stefan-webrtc (holmer) wrote:
> It's a bit strange to call this Max, since it can be overridden by the time
> check against kMaxTimeBetweenSyncs.

kQpDeltaThersholdForSync?

https://codereview.webrtc.org/1193513006/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:112: if
(last_timestamp_ == 0) {
On 2015/06/23 14:28:32, stefan-webrtc (holmer) wrote:
> last_timestamp_ can become 0 mid stream, so it would probably be better to use
> -1 as init value.

Done.
Also switched to using rtc::TimestampWrapAroundHandler.

https://codereview.webrtc.org/1193513006/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:222: // First frame
in TL1 should only depend on TL0 since there are not
On 2015/06/23 14:28:32, stefan-webrtc (holmer) wrote:
> there are no

Done.

https://codereview.webrtc.org/1193513006/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:227: DCHECK_NE(0u,
last_sync_timestamp_);
On 2015/06/23 14:28:32, stefan-webrtc (holmer) wrote:
> Can't this be 0 mid stream?

Updated to use time unwrapped to int64_t and -1 for not available.

https://codereview.webrtc.org/1193513006/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/screenshare_layers.h (right):

https://codereview.webrtc.org/1193513006/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.h:92: } layers_[2];
On 2015/06/23 14:28:33, stefan-webrtc (holmer) wrote:
> We should name the constant 2 as it's used in more than one place (also in the
> .cc)

Done.

[stefan-webrtc, 2015-06-23 15:45:13]

LG, but there are still some tests failing.

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

https://codereview.webrtc.org/1193513006/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:24: static const
int kMaxQpDeltaForSync = 8;
On 2015/06/23 15:07:30, språng wrote:
> On 2015/06/23 14:28:32, stefan-webrtc (holmer) wrote:
> > It's a bit strange to call this Max, since it can be overridden by the time
> > check against kMaxTimeBetweenSyncs.
> 
> kQpDeltaThersholdForSync?

No, but kQpDeltaThresholdForSync sounds good. :)

[sprang_webrtc, 2015-06-24 07:53:43]

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

https://codereview.webrtc.org/1193513006/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:24: static const
int kMaxQpDeltaForSync = 8;
On 2015/06/23 15:45:13, stefan-webrtc (holmer) wrote:
> On 2015/06/23 15:07:30, språng wrote:
> > On 2015/06/23 14:28:32, stefan-webrtc (holmer) wrote:
> > > It's a bit strange to call this Max, since it can be overridden by the
time
> > > check against kMaxTimeBetweenSyncs.
> > 
> > kQpDeltaThersholdForSync?
> 
> No, but kQpDeltaThresholdForSync sounds good. :)

Done.

[stefan-webrtc, 2015-06-24 08:04:11]

lgtm

[sprang_webrtc, 2015-06-24 08:06:37]

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

[sprang_webrtc, 2015-06-24 08:06:38]

+mflodman for talk/media/webrtc/

[sprang_webrtc, 2015-06-24 08:43:12]

sprang@webrtc.org changed reviewers:
+ pbos@webrtc.org
- mflodman@webrtc.org

[sprang_webrtc, 2015-06-24 08:43:13]

-mflodman +pbos for talk/media/webrtc/

[pbos-webrtc, 2015-06-24 09:12:08]

rs-lgtm for talk/

[sprang_webrtc, 2015-06-24 09:24:56]

Committed patchset #9 (id:160001) manually as
2c4c9148191a10c0e82c9a209d454c6b1ebbaf20 (presubmit successful).