Add experimental simulcast screen content mode

Add experimental simulcast screen content mode

BUG=webrtc:4172
Review-Url: https://codereview.webrtc.org/2636443002
Cr-Commit-Position: refs/heads/master@{#16135}
Committed: https://chromium.googlesource.com/external/webrtc/+/a28e971e3bc8a06f366f0ef82cc7168b05a83b59

[sprang_webrtc, 2017-01-13 08:41:41]

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

[perkj_webrtc, 2017-01-13 10:14:11]

Description was changed from

==========
Add experimental simulcast screen content mode

BUG=webrtc:4172
==========

to

==========
Add experimental simulcast screen content mode

BUG=webrtc:4172
==========

[perkj_webrtc, 2017-01-13 10:14:12]

perkj@webrtc.org changed reviewers:
+ asapersson@webrtc.org

[perkj_webrtc, 2017-01-13 10:14:31]

On 2017/01/13 08:41:42, språng wrote:

Adding Åsa. Can you help review this?

[åsapersson, 2017-01-16 13:43:50]

https://codereview.webrtc.org/2636443002/diff/1/webrtc/media/engine/simulcast.cc
File webrtc/media/engine/simulcast.cc (right):

https://codereview.webrtc.org/2636443002/diff/1/webrtc/media/engine/simulcast...
webrtc/media/engine/simulcast.cc:230: FindSimulcastMaxBitrateBps(width, height,
num_simulcast_layers + 1);
now + 1? (and below)

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

https://codereview.webrtc.org/2636443002/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:75:
number_of_temporal_layers_(std::min(2, num_temporal_layers)),
maybe add a constant for 2

[sprang_webrtc, 2017-01-16 16:39:11]

https://codereview.webrtc.org/2636443002/diff/1/webrtc/media/engine/simulcast.cc
File webrtc/media/engine/simulcast.cc (right):

https://codereview.webrtc.org/2636443002/diff/1/webrtc/media/engine/simulcast...
webrtc/media/engine/simulcast.cc:230: FindSimulcastMaxBitrateBps(width, height,
num_simulcast_layers + 1);
On 2017/01/16 13:43:50, åsapersson wrote:
> now + 1? (and below)

Oops, that was left from experimentation. Though it seems the parameter isn't
even used, so removing it instead.

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

https://codereview.webrtc.org/2636443002/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:75:
number_of_temporal_layers_(std::min(2, num_temporal_layers)),
On 2017/01/16 13:43:50, åsapersson wrote:
> maybe add a constant for 2

Done.

[sprang_webrtc, 2017-01-17 11:30:26]

Fixed non-conference mode bug, added test. ptal

[åsapersson, 2017-01-17 12:01:35]

lgtm

[sprang_webrtc, 2017-01-17 12:03:02]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2017-01-17 12:03:07]

The CQ bit was unchecked by sprang@webrtc.org

[sprang_webrtc, 2017-01-17 12:04:24]

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

[commit-bot: I haz the power, 2017-01-17 12:04:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[perkj_webrtc, 2017-01-17 13:09:03]

https://codereview.webrtc.org/2636443002/diff/40001/webrtc/media/engine/simul...
File webrtc/media/engine/simulcast.cc (right):

https://codereview.webrtc.org/2636443002/diff/40001/webrtc/media/engine/simul...
webrtc/media/engine/simulcast.cc:171: std::vector<webrtc::VideoStream>
GetSimulcastConfig(size_t max_streams,
I think you will need  one version of this method that keep the old argument
list as well to support downstream projects. Default is_screencast to false.

https://codereview.webrtc.org/2636443002/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2636443002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1730: if
(FindMatchingCodec(internal_encoder_factory_->supported_codecs(), codec)) {
internal_encoder_factory_ does not seem to be used by this class after this
function. 
Can who ever needs internal_encoder_factory_ after this method return own it?
create a std::unique_ptr and move it to where its needed.

[sprang_webrtc, 2017-01-17 13:23:28]

https://codereview.webrtc.org/2636443002/diff/40001/webrtc/media/engine/simul...
File webrtc/media/engine/simulcast.cc (right):

https://codereview.webrtc.org/2636443002/diff/40001/webrtc/media/engine/simul...
webrtc/media/engine/simulcast.cc:171: std::vector<webrtc::VideoStream>
GetSimulcastConfig(size_t max_streams,
On 2017/01/17 13:09:03, perkj_webrtc wrote:
> I think you will need  one version of this method that keep the old argument
> list as well to support downstream projects. Default is_screencast to false.

I (temporarily) made is_screencast a default parameter.

https://codereview.webrtc.org/2636443002/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2636443002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1730: if
(FindMatchingCodec(internal_encoder_factory_->supported_codecs(), codec)) {
On 2017/01/17 13:09:03, perkj_webrtc wrote:
> internal_encoder_factory_ does not seem to be used by this class after this
> function. 
> Can who ever needs internal_encoder_factory_ after this method return own it?
> create a std::unique_ptr and move it to where its needed.

It's the SimulcastEncoderAdapter that uses this reference, and it's used in the
InitEncode() call which happens on a separate thread. 

Unfortunately the adapter normally takes a pointer to an external encoder
factory, which we don't want to delete. So in that case it would have to keep
track of if the instance should be deleted or not :(

[perkj_webrtc, 2017-01-17 13:24:44]

rs lgtm then.

[sprang_webrtc, 2017-01-17 13:25:14]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2017-01-17 13:25:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
asapersson@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2636443002/#ps60001 (title:
"Add default param so we won't break downstream")

[commit-bot: I haz the power, 2017-01-17 13:25:19]

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-01-17 14:51:35]

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

[commit-bot: I haz the power, 2017-01-17 14:51:36]

Failed to apply patch for webrtc/media/engine/webrtcvideoengine2.h:
While running git apply --index -p1;
  error: patch failed: webrtc/media/engine/webrtcvideoengine2.h:347
  error: webrtc/media/engine/webrtcvideoengine2.h: patch does not apply

Patch:       webrtc/media/engine/webrtcvideoengine2.h
Index: webrtc/media/engine/webrtcvideoengine2.h
diff --git a/webrtc/media/engine/webrtcvideoengine2.h
b/webrtc/media/engine/webrtcvideoengine2.h
index
1fb794eece5f78cf3b1801532a0560db0ef8d414..8f25408c065ed360de2bf2db383a7f55d4ec31e5
100644
--- a/webrtc/media/engine/webrtcvideoengine2.h
+++ b/webrtc/media/engine/webrtcvideoengine2.h
@@ -347,6 +347,8 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public
webrtc::Transport {
         ACCESS_ON(&thread_checker_);
     WebRtcVideoEncoderFactory* const external_encoder_factory_
         ACCESS_ON(&thread_checker_);
+    const std::unique_ptr<WebRtcVideoEncoderFactory> internal_encoder_factory_
+        ACCESS_ON(&thread_checker_);
 
     rtc::CriticalSection lock_;
     webrtc::VideoSendStream* stream_ ACCESS_ON(&thread_checker_);

[sprang_webrtc, 2017-01-18 07:22:56]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2017-01-18 07:22:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
asapersson@webrtc.org, perkj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2636443002/#ps80001 (title:
"Rebase, again")

[commit-bot: I haz the power, 2017-01-18 07:22:59]

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-01-18 08:36:32]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484724176124430,
"parent_rev": "b3dc2b7b1e02970c49064a61a4bd0fc941249c93", "commit_rev":
"a28e971e3bc8a06f366f0ef82cc7168b05a83b59"}

[commit-bot: I haz the power, 2017-01-18 08:36:35]

Description was changed from

==========
Add experimental simulcast screen content mode

BUG=webrtc:4172
==========

to

==========
Add experimental simulcast screen content mode

BUG=webrtc:4172

Review-Url: https://codereview.webrtc.org/2636443002
Cr-Commit-Position: refs/heads/master@{#16135}
Committed:
https://chromium.googlesource.com/external/webrtc/+/a28e971e3bc8a06f366f0ef82...
==========

[commit-bot: I haz the power, 2017-01-18 08:36:36]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/a28e971e3bc8a06f366f0ef82...

[sprang_webrtc, 2017-01-18 13:18:57]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.webrtc.org/2643763002/ by sprang@webrtc.org.

The reason for reverting is: Breaks chromium..