Initialize/configure video encoders asychronously.

Initialize/configure video encoders asychronously.

Greatly speeds up setRemoteDescription() by moving encoder initialization
off the main worker thread, which is free to move onto gathering ICE
candidates and other tasks while InitEncode() is performed. It also
un-blocks PeerConnection GetStats() which is no longer blocked on
encoder initialization.

BUG=webrtc:5410
R=stefan@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/fb647a67be94bb3c940d8b5fba01972f7ce91a29

[pbos-webrtc, 2016-03-03 16:46:19]

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

[commit-bot: I haz the power, 2016-03-03 16:46:30]

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

[pbos-webrtc, 2016-03-03 16:47:51]

PTAL

[commit-bot: I haz the power, 2016-03-03 18:46:56]

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

[commit-bot: I haz the power, 2016-03-03 18:46:57]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[stefan-webrtc, 2016-03-14 13:14:03]

Nice

https://codereview.webrtc.org/1757313002/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/1757313002/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:590:
ASSERT_TRUE(encoder_factory.WaitForCreatedVideoEncoder());
Maybe we should make WaitForCreatedVideoEncoder take a parameter so we can call:
EXPECT_TRUE(encoder_factory.WaitForCreatedVideoEncoders(3));

or something like that?

https://codereview.webrtc.org/1757313002/diff/1/webrtc/video/video_quality_te...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/1757313002/diff/1/webrtc/video/video_quality_te...
webrtc/video/video_quality_test.cc:510: test_label_.c_str(),
dropped_frames_before_rendering_);
It's a bit weird that we mix PrintResult with printfs. Could you please align
(either way)?

https://codereview.webrtc.org/1757313002/diff/1/webrtc/video/video_send_strea...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/1757313002/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:178: if (video_codec.codecType ==
kVideoCodecVP8) {
Maybe change this to a switch {}?

https://codereview.webrtc.org/1757313002/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:497: RTC_CHECK_EQ(0,
vie_encoder_.RegisterExternalEncoder(
Is there anything we should DCHECK before this to ensure the encoder process
thread is started after video send stream initialization, somehow? And maybe
thread check.

https://codereview.webrtc.org/1757313002/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:541: rtc::CritScope
lock(&encoder_settings_crit_);
Maybe put this within its own scope?

[pbos-webrtc, 2016-03-14 14:05:23]

rebase

[pbos-webrtc, 2016-03-14 15:12:53]

feedback

[pbos-webrtc, 2016-03-14 15:12:58]

PTAL

https://codereview.webrtc.org/1757313002/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/1757313002/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:590:
ASSERT_TRUE(encoder_factory.WaitForCreatedVideoEncoder());
On 2016/03/14 13:14:03, stefan-webrtc (holmer) wrote:
> Maybe we should make WaitForCreatedVideoEncoder take a parameter so we can
call:
> EXPECT_TRUE(encoder_factory.WaitForCreatedVideoEncoders(3));
> 
> or something like that?

Done.

https://codereview.webrtc.org/1757313002/diff/1/webrtc/video/video_quality_te...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/1757313002/diff/1/webrtc/video/video_quality_te...
webrtc/video/video_quality_test.cc:510: test_label_.c_str(),
dropped_frames_before_rendering_);
On 2016/03/14 13:14:03, stefan-webrtc (holmer) wrote:
> It's a bit weird that we mix PrintResult with printfs. Could you please align
> (either way)?

Done.

https://codereview.webrtc.org/1757313002/diff/1/webrtc/video/video_send_strea...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/1757313002/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:178: if (video_codec.codecType ==
kVideoCodecVP8) {
On 2016/03/14 13:14:03, stefan-webrtc (holmer) wrote:
> Maybe change this to a switch {}?

Done.

https://codereview.webrtc.org/1757313002/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:497: RTC_CHECK_EQ(0,
vie_encoder_.RegisterExternalEncoder(
On 2016/03/14 13:14:03, stefan-webrtc (holmer) wrote:
> Is there anything we should DCHECK before this to ensure the encoder process
> thread is started after video send stream initialization, somehow? And maybe
> thread check.

Don't think we have any good primitives for checking that?

https://codereview.webrtc.org/1757313002/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:541: rtc::CritScope
lock(&encoder_settings_crit_);
On 2016/03/14 13:14:03, stefan-webrtc (holmer) wrote:
> Maybe put this within its own scope?

Done.

[pbos-webrtc, 2016-03-14 15:13:05]

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

[commit-bot: I haz the power, 2016-03-14 15:13:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1757313002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1757313002/40001

[stefan-webrtc, 2016-03-14 15:27:22]

lgtm, but consider my comment.

https://codereview.webrtc.org/1757313002/diff/40001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/1757313002/diff/40001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:192: if (video_codec.codecType ==
kVideoCodecVP8) {
Maybe switch here too?

[pbos-webrtc, 2016-03-14 15:42:43]

merge switch blocks

[pbos-webrtc, 2016-03-14 15:43:11]

https://codereview.webrtc.org/1757313002/diff/40001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/1757313002/diff/40001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:192: if (video_codec.codecType ==
kVideoCodecVP8) {
On 2016/03/14 15:27:22, stefan-webrtc (holmer) wrote:
> Maybe switch here too?

Merged into previous switch, done.

[commit-bot: I haz the power, 2016-03-14 15:59:07]

Description was changed from

==========
Initialize/configure video encoders asychronously.

Greatly speeds up setRemoteDescription() by moving encoder initialization
off the main worker thread, which is free to move onto gathering ICE
candidates and other tasks while InitEncode() is performed. It also
un-blocks PeerConnection GetStats() which is no longer blocked on
encoder initialization.

BUG=webrtc:5410
R=stefan@webrtc.org
==========

to

==========
Initialize/configure video encoders asychronously.

Greatly speeds up setRemoteDescription() by moving encoder initialization
off the main worker thread, which is free to move onto gathering ICE
candidates and other tasks while InitEncode() is performed. It also
un-blocks PeerConnection GetStats() which is no longer blocked on
encoder initialization.

BUG=webrtc:5410
R=stefan@webrtc.org

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

[commit-bot: I haz the power, 2016-03-14 15:59:08]

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

[pbos-webrtc, 2016-03-14 15:59:09]

Description was changed from

==========
Initialize/configure video encoders asychronously.

Greatly speeds up setRemoteDescription() by moving encoder initialization
off the main worker thread, which is free to move onto gathering ICE
candidates and other tasks while InitEncode() is performed. It also
un-blocks PeerConnection GetStats() which is no longer blocked on
encoder initialization.

BUG=webrtc:5410
R=stefan@webrtc.org

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

to

==========
Initialize/configure video encoders asychronously.

Greatly speeds up setRemoteDescription() by moving encoder initialization
off the main worker thread, which is free to move onto gathering ICE
candidates and other tasks while InitEncode() is performed. It also
un-blocks PeerConnection GetStats() which is no longer blocked on
encoder initialization.

BUG=webrtc:5410
R=stefan@webrtc.org

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

[pbos-webrtc, 2016-03-14 15:59:10]

Committed patchset #4 (id:60001) manually as
fb647a67be94bb3c940d8b5fba01972f7ce91a29 (presubmit successful).

[emircan, 2016-03-16 00:37:22]

I am currently hitting a DCHECK(thread_checker) in Chrome ToT, and it looks like
this patch might be related. Note that rtc_video_encoder is used for HW
encoders. It expects InitEncode() to be called from libjingle worker main thread
where it is contructed on. Can you take a look?

[86260:40199:0315/172016:FATAL:rtc_video_encoder.cc(628)] Check failed:
thread_checker_.CalledOnValidThread(). 
0   libbase.dylib                       0x0000000117211ccf
_ZN4base5debug10StackTraceC2Ev + 47
1   libbase.dylib                       0x0000000117211e73
_ZN4base5debug10StackTraceC1Ev + 35
2   libbase.dylib                       0x000000011729dfe0
_ZN7logging10LogMessageD2Ev + 80
3   libbase.dylib                       0x000000011729b913
_ZN7logging10LogMessageD1Ev + 35
4   libcontent.dylib                    0x000000011d8c1ad6
_ZN7content15RTCVideoEncoder10InitEncodeEPKN6webrtc10VideoCodecEim + 566
5   libcontent.dylib                    0x000000011ef38bb9
_ZN6webrtc35VideoEncoderSoftwareFallbackWrapper10InitEncodeEPKNS_10VideoCodecEim
+ 233
6   libcontent.dylib                    0x000000011ef83c27
_ZN6webrtc17VCMGenericEncoder10InitEncodeEPKNS_10VideoCodecEim + 343
7   libcontent.dylib                    0x000000011ef718b5
_ZN6webrtc16VCMCodecDataBase12SetSendCodecEPKNS_10VideoCodecEim + 1909
8   libcontent.dylib                    0x000000011efc258c
_ZN6webrtc3vcm11VideoSender17RegisterSendCodecEPKNS_10VideoCodecEjj + 380
9   libcontent.dylib                    0x000000011efc067e
_ZN6webrtc12_GLOBAL__N_121VideoCodingModuleImpl17RegisterSendCodecEPKNS_10VideoCodecEjj
+ 62
10  libcontent.dylib                    0x000000011ef565a3
_ZN6webrtc10ViEEncoder10SetEncoderERKNS_10VideoCodecEi + 723
11  libcontent.dylib                    0x000000011ef46889
_ZN6webrtc8internal15VideoSendStream14EncoderProcessEv + 697
12  libcontent.dylib                    0x000000011ef44a76
_ZN6webrtc8internal15VideoSendStream21EncoderThreadFunctionEPv + 38
13  libcontent.dylib                    0x000000011e4ae138
_ZN3rtc14PlatformThread3RunEv + 520

[pbos-webrtc, 2016-03-16 13:57:04]

On 2016/03/16 00:37:22, emircan wrote:
> I am currently hitting a DCHECK(thread_checker) in Chrome ToT, and it looks
like
> this patch might be related. Note that rtc_video_encoder is used for HW
> encoders. It expects InitEncode() to be called from libjingle worker main
thread
> where it is contructed on. Can you take a look?
> 
> [86260:40199:0315/172016:FATAL:rtc_video_encoder.cc(628)] Check failed:
> thread_checker_.CalledOnValidThread(). 
> 0   libbase.dylib                       0x0000000117211ccf
> _ZN4base5debug10StackTraceC2Ev + 47
> 1   libbase.dylib                       0x0000000117211e73
> _ZN4base5debug10StackTraceC1Ev + 35
> 2   libbase.dylib                       0x000000011729dfe0
> _ZN7logging10LogMessageD2Ev + 80
> 3   libbase.dylib                       0x000000011729b913
> _ZN7logging10LogMessageD1Ev + 35
> 4   libcontent.dylib                    0x000000011d8c1ad6
> _ZN7content15RTCVideoEncoder10InitEncodeEPKN6webrtc10VideoCodecEim + 566
> 5   libcontent.dylib                    0x000000011ef38bb9
>
_ZN6webrtc35VideoEncoderSoftwareFallbackWrapper10InitEncodeEPKNS_10VideoCodecEim
> + 233
> 6   libcontent.dylib                    0x000000011ef83c27
> _ZN6webrtc17VCMGenericEncoder10InitEncodeEPKNS_10VideoCodecEim + 343
> 7   libcontent.dylib                    0x000000011ef718b5
> _ZN6webrtc16VCMCodecDataBase12SetSendCodecEPKNS_10VideoCodecEim + 1909
> 8   libcontent.dylib                    0x000000011efc258c
> _ZN6webrtc3vcm11VideoSender17RegisterSendCodecEPKNS_10VideoCodecEjj + 380
> 9   libcontent.dylib                    0x000000011efc067e
>
_ZN6webrtc12_GLOBAL__N_121VideoCodingModuleImpl17RegisterSendCodecEPKNS_10VideoCodecEjj
> + 62
> 10  libcontent.dylib                    0x000000011ef565a3
> _ZN6webrtc10ViEEncoder10SetEncoderERKNS_10VideoCodecEi + 723
> 11  libcontent.dylib                    0x000000011ef46889
> _ZN6webrtc8internal15VideoSendStream14EncoderProcessEv + 697
> 12  libcontent.dylib                    0x000000011ef44a76
> _ZN6webrtc8internal15VideoSendStream21EncoderThreadFunctionEPv + 38
> 13  libcontent.dylib                    0x000000011e4ae138
> _ZN3rtc14PlatformThread3RunEv + 520

I posted https://bugs.chromium.org/p/chromium/issues/detail?id=595308, please
help out there if you can.

[pbos-webrtc, 2016-03-22 10:25:14]

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

The reason for reverting is: Breaks RTCVideoEncoder which has incorrect
assumptions on where InitEncode etc. is called from. Temporarily reverting until
RTCVideoEncoder has been updated..