Initialize VideoEncoder objects asynchronously.

Initialize VideoEncoder objects asynchronously.

Removes blocking from VideoSendStream constructor and
ReconfigureVideoEncoder calls. Moves initialization onto the encoder
thread and performed as soon as possible but without blocking the
calling thread.

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

[pbos-webrtc, 2016-01-18 20:03:56]

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

[commit-bot: I haz the power, 2016-01-18 20:04:07]

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

[pbos-webrtc, 2016-01-18 20:04:27]

Description was changed from

==========
Initialize VideoEncoders asynchronously.

Removes blocking from VideoSendStream constructor and
ReconfigureVideoEncoder calls. Moves initialization onto the encoder
thread and performed as soon as possible but without blocking the
calling thread.

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

to

==========
Initialize VideoEncoder objects asynchronously.

Removes blocking from VideoSendStream constructor and
ReconfigureVideoEncoder calls. Moves initialization onto the encoder
thread and performed as soon as possible but without blocking the
calling thread.

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

[commit-bot: I haz the power, 2016-01-18 20:12:38]

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

[commit-bot: I haz the power, 2016-01-18 20:12:38]

Dry run: Try jobs failed on following builders:
  win_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/12165)

[stefan-webrtc, 2016-01-19 10:24:46]

You seem to have some compile issues still.

Could you update the description with the perf impact of this change (if you
have it?)?

[stefan-webrtc, 2016-01-19 12:02:10]

+mflodman as it would be good to get another set of eyes on this.

https://codereview.webrtc.org/1600973002/diff/1/talk/media/webrtc/fakewebrtcv...
File talk/media/webrtc/fakewebrtcvideoengine.h (right):

https://codereview.webrtc.org/1600973002/diff/1/talk/media/webrtc/fakewebrtcv...
talk/media/webrtc/fakewebrtcvideoengine.h:208: rtc::CritScope lock(&crit_);
Why is this crit_ needed now? Maybe you could document the threads you expect
these methods to be called on?

https://codereview.webrtc.org/1600973002/diff/1/talk/media/webrtc/webrtcvideo...
File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/1600973002/diff/1/talk/media/webrtc/webrtcvideo...
talk/media/webrtc/webrtcvideoengine2_unittest.cc:388:
ASSERT_TRUE(encoder_factory.WaitForCreatedVideoEncoder());
Any chance that the encoder has already been recreated before you call
WaitFor... and then end up timing out?

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
File webrtc/video/video_capture_input.cc (right):

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
webrtc/video/video_capture_input.cc:142: if (encoder_registered_) {
Maybe set this to false after the external encoder has been de-registered?

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
webrtc/video/video_capture_input.cc:164: // gap between the first and second
frame encoded with the new settings.
I don't understand this last long sentence.

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
webrtc/video/video_capture_input.cc:169: if (replace_codec_settings) {
I don't see how this can ever be true?

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
webrtc/video/video_capture_input.cc:176: // Optional only for tests that don't
instantiate a full VideoSendStream.
Are you saying the following code is only executed by tests? Or that it doesn't
have to be executed by tests? How?

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
webrtc/video/video_capture_input.cc:177: // TODO(pbos): Mock VideoSendStream.
I think this TODO should be more detailed. You shouldn't use mocks in production
code?

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
webrtc/video/video_capture_input.cc:179:
video_send_stream_->SetSendCodec(codec_settings);
Shouldn't this call directly into the vcm? Seems a bit odd that it calls the vcm
directly for some things and then goes via video_send_stream_ for other. I get
the feeling like we're mixing layers here somehow. Should the VideoCaptureInput
be allowed to talk to the vcm at all?

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
webrtc/video/video_capture_input.cc:180: if (config_.suspend_below_min_bitrate)
Doesn't this mean that suspend below min bitrate is enabled as a feature? Seems
like the line below actually suspends the video? Maybe just a bad API name :/

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

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream_tests.cc:1426:
EXPECT_TRUE(init_encode_event_.Wait(kDefaultTimeoutMs));
Any risk for this to be racy?

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/vie_channel.cc
File webrtc/video/vie_channel.cc (right):

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/vie_channel.cc#n...
webrtc/video/vie_channel.cc:204: }
Could you enlighten why this was necessary to change in this CL?

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/vie_channel.cc#n...
webrtc/video/vie_channel.cc:371: void ViEChannel::SetSendCodec(const VideoCodec&
video_codec) {
Should we check what thread this is being called on?

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/vie_channel.cc#n...
webrtc/video/vie_channel.cc:431: }
When reading the changes for this method I can't see much that actually did
change. Can you please summarize what you have done here? Did you only remove
registration/deregistration of rtp modules?

[pbos-webrtc, 2016-01-19 15:27:30]

comments + rebase

[pbos-webrtc, 2016-01-19 15:27:58]

https://codereview.webrtc.org/1600973002/diff/1/talk/media/webrtc/fakewebrtcv...
File talk/media/webrtc/fakewebrtcvideoengine.h (right):

https://codereview.webrtc.org/1600973002/diff/1/talk/media/webrtc/fakewebrtcv...
talk/media/webrtc/fakewebrtcvideoengine.h:208: rtc::CritScope lock(&crit_);
On 2016/01/19 12:02:09, stefan-webrtc (holmer) wrote:
> Why is this crit_ needed now? Maybe you could document the threads you expect
> these methods to be called on?

Because the encoder factory can be called from
SimulcastEncoderAdapter::InitEncode, which is now on the encoder thread. As far
as I can tell this is fine with the current implementations
(RTCVideoEncoderFactory, MediaCodecVideoEncoderFactory).

https://codereview.webrtc.org/1600973002/diff/1/talk/media/webrtc/webrtcvideo...
File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/1600973002/diff/1/talk/media/webrtc/webrtcvideo...
talk/media/webrtc/webrtcvideoengine2_unittest.cc:388:
ASSERT_TRUE(encoder_factory.WaitForCreatedVideoEncoder());
On 2016/01/19 12:02:09, stefan-webrtc (holmer) wrote:
> Any chance that the encoder has already been recreated before you call
> WaitFor... and then end up timing out?

No, Set() remains set until Wait() catches it.

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
File webrtc/video/video_capture_input.cc (right):

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
webrtc/video/video_capture_input.cc:142: if (encoder_registered_) {
On 2016/01/19 12:02:09, stefan-webrtc (holmer) wrote:
> Maybe set this to false after the external encoder has been de-registered?

We're only doing this on a shutdown, so we should never run EncoderProcess again
after this point.

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
webrtc/video/video_capture_input.cc:164: // gap between the first and second
frame encoded with the new settings.
On 2016/01/19 12:02:09, stefan-webrtc (holmer) wrote:
> I don't understand this last long sentence.

Hope this is better. :)

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
webrtc/video/video_capture_input.cc:169: if (replace_codec_settings) {
On 2016/01/19 12:02:09, stefan-webrtc (holmer) wrote:
> I don't see how this can ever be true?

Line 155 makes it true if TriggerSetSendCodec have been called.

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
webrtc/video/video_capture_input.cc:176: // Optional only for tests that don't
instantiate a full VideoSendStream.
On 2016/01/19 12:02:09, stefan-webrtc (holmer) wrote:
> Are you saying the following code is only executed by tests? Or that it
doesn't
> have to be executed by tests? How?

Not executed by tests. Comment updated.

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
webrtc/video/video_capture_input.cc:177: // TODO(pbos): Mock VideoSendStream.
On 2016/01/19 12:02:09, stefan-webrtc (holmer) wrote:
> I think this TODO should be more detailed. You shouldn't use mocks in
production
> code?

Updated TODO

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
webrtc/video/video_capture_input.cc:179:
video_send_stream_->SetSendCodec(codec_settings);
On 2016/01/19 12:02:09, stefan-webrtc (holmer) wrote:
> Shouldn't this call directly into the vcm? Seems a bit odd that it calls the
vcm
> directly for some things and then goes via video_send_stream_ for other. I get
> the feeling like we're mixing layers here somehow. Should the
VideoCaptureInput
> be allowed to talk to the vcm at all?

It needs to configure vie_channel, vie_channel and stats_proxy which
VideoSendStream already has. Is this too ugly, should I move vie_encoder and
vie_channel down here? Then I'll have to have both frame_callback (for testing)
and vie_encoder for the actual object (when not testing).

I'm not sure I like the alternative right now.

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
webrtc/video/video_capture_input.cc:180: if (config_.suspend_below_min_bitrate)
On 2016/01/19 12:02:09, stefan-webrtc (holmer) wrote:
> Doesn't this mean that suspend below min bitrate is enabled as a feature?
Seems
> like the line below actually suspends the video? Maybe just a bad API name :/

Bad API name, it turns on the option.

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

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream_tests.cc:1426:
EXPECT_TRUE(init_encode_event_.Wait(kDefaultTimeoutMs));
On 2016/01/19 12:02:09, stefan-webrtc (holmer) wrote:
> Any risk for this to be racy?

So long as ::CreateVideoSendStream causes one InitEncode and
ReconfigureVideoEncoder causes another, we're fine. We wait for the first init
before doing ReconfigureVideoEncoder, which is good.

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/vie_channel.cc
File webrtc/video/vie_channel.cc (right):

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/vie_channel.cc#n...
webrtc/video/vie_channel.cc:204: }
On 2016/01/19 12:02:10, stefan-webrtc (holmer) wrote:
> Could you enlighten why this was necessary to change in this CL?

I can only add/remove modules from the construction thread, but I need to turn
on/off RTP modules on the fly from the encoder thread.

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/vie_channel.cc#n...
webrtc/video/vie_channel.cc:371: void ViEChannel::SetSendCodec(const VideoCodec&
video_codec) {
On 2016/01/19 12:02:10, stefan-webrtc (holmer) wrote:
> Should we check what thread this is being called on?

It'd be interesting to inject thread checkers from VideoSendStream that'd be
shared across objects (encoder_thread_checker). I think that sounds like a
follow-up though?

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/vie_channel.cc#n...
webrtc/video/vie_channel.cc:431: }
On 2016/01/19 12:02:10, stefan-webrtc (holmer) wrote:
> When reading the changes for this method I can't see much that actually did
> change. Can you please summarize what you have done here? Did you only remove
> registration/deregistration of rtp modules?

Yes, and move crit_ to cover the entire function (so that it remains
synchronized with StartSend and StopSend).

[pbos-webrtc, 2016-01-19 15:38:28]

use rtc::Optional for replacement settings, fixes win compile issue

[pbos-webrtc, 2016-01-19 15:46:57]

PTAL

[pbos-webrtc, 2016-01-19 15:47:02]

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

[commit-bot: I haz the power, 2016-01-19 15:47:06]

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

[commit-bot: I haz the power, 2016-01-19 19:29:45]

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

[commit-bot: I haz the power, 2016-01-19 19:29:47]

Dry run: Try jobs failed on following builders:
  win_baremetal on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/8481)

[stefan-webrtc, 2016-01-20 12:22:48]

https://codereview.webrtc.org/1600973002/diff/1/talk/media/webrtc/webrtcvideo...
File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/1600973002/diff/1/talk/media/webrtc/webrtcvideo...
talk/media/webrtc/webrtcvideoengine2_unittest.cc:388:
ASSERT_TRUE(encoder_factory.WaitForCreatedVideoEncoder());
On 2016/01/19 15:27:58, pbos-webrtc wrote:
> On 2016/01/19 12:02:09, stefan-webrtc (holmer) wrote:
> > Any chance that the encoder has already been recreated before you call
> > WaitFor... and then end up timing out?
> 
> No, Set() remains set until Wait() catches it.

Thanks!

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
File webrtc/video/video_capture_input.cc (right):

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
webrtc/video/video_capture_input.cc:142: if (encoder_registered_) {
On 2016/01/19 15:27:58, pbos-webrtc wrote:
> On 2016/01/19 12:02:09, stefan-webrtc (holmer) wrote:
> > Maybe set this to false after the external encoder has been de-registered?
> 
> We're only doing this on a shutdown, so we should never run EncoderProcess
again
> after this point.

Right, I was mostly thinking if some other method start using
encoder_registered_. It is probably good to keep it in sync.

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
webrtc/video/video_capture_input.cc:164: // gap between the first and second
frame encoded with the new settings.
On 2016/01/19 15:27:58, pbos-webrtc wrote:
> On 2016/01/19 12:02:09, stefan-webrtc (holmer) wrote:
> > I don't understand this last long sentence.
> 
> Hope this is better. :)

A lot better!

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
webrtc/video/video_capture_input.cc:169: if (replace_codec_settings) {
On 2016/01/19 15:27:58, pbos-webrtc wrote:
> On 2016/01/19 12:02:09, stefan-webrtc (holmer) wrote:
> > I don't see how this can ever be true?
> 
> Line 155 makes it true if TriggerSetSendCodec have been called.

Ah, I misread line 158 to be "replace_codec_settings = false". Maybe call it
old_replace_codec_settings or something to make it a bit easier to read?

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
webrtc/video/video_capture_input.cc:177: // TODO(pbos): Mock VideoSendStream.
On 2016/01/19 15:27:58, pbos-webrtc wrote:
> On 2016/01/19 12:02:09, stefan-webrtc (holmer) wrote:
> > I think this TODO should be more detailed. You shouldn't use mocks in
> production
> > code?
> 
> Updated TODO

Maybe it actually makes sense to do this now so that we can properly test this
class. I assume that now all tests are passing a nullptr as video_send_stream,
which means we actually don't test this path?

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
webrtc/video/video_capture_input.cc:179:
video_send_stream_->SetSendCodec(codec_settings);
On 2016/01/19 15:27:58, pbos-webrtc wrote:
> On 2016/01/19 12:02:09, stefan-webrtc (holmer) wrote:
> > Shouldn't this call directly into the vcm? Seems a bit odd that it calls the
> vcm
> > directly for some things and then goes via video_send_stream_ for other. I
get
> > the feeling like we're mixing layers here somehow. Should the
> VideoCaptureInput
> > be allowed to talk to the vcm at all?
> 
> It needs to configure vie_channel, vie_channel and stats_proxy which
> VideoSendStream already has. Is this too ugly, should I move vie_encoder and
> vie_channel down here? Then I'll have to have both frame_callback (for
testing)
> and vie_encoder for the actual object (when not testing).
> 
> I'm not sure I like the alternative right now.

I guess one of the vie_channel you mentioned should be vie_encoder?

Not sure what is best. Maybe we should accept a dependency from
VideoCaptureInput on VideoSendStream? I dislike it because of the loop we get,
where both depend on each other.

What you really want is to post the call to SetSendCodec onto the encoder
thread. If that would have been possible that would have been your preferred
solution, right? Or would you still have involved VideoCaptureInput?

I'm mostly trying to understand how this needs to work.

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
webrtc/video/video_capture_input.cc:180: if (config_.suspend_below_min_bitrate)
On 2016/01/19 15:27:58, pbos-webrtc wrote:
> On 2016/01/19 12:02:09, stefan-webrtc (holmer) wrote:
> > Doesn't this mean that suspend below min bitrate is enabled as a feature?
> Seems
> > like the line below actually suspends the video? Maybe just a bad API name
:/
> 
> Bad API name, it turns on the option.

Acknowledged.

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/vie_channel.cc
File webrtc/video/vie_channel.cc (right):

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/vie_channel.cc#n...
webrtc/video/vie_channel.cc:204: }
On 2016/01/19 15:27:58, pbos-webrtc wrote:
> On 2016/01/19 12:02:10, stefan-webrtc (holmer) wrote:
> > Could you enlighten why this was necessary to change in this CL?
> 
> I can only add/remove modules from the construction thread, but I need to turn
> on/off RTP modules on the fly from the encoder thread.

I don't think I fully understand. How is the process thread involved with the
construction thread and the encoder thread?

Is it because you want to avoid having to register/deregister modules on the
process thread for some reason? Seems like we are basically duplicating code
that the ProcessThread aldready has?

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/vie_channel.cc#n...
webrtc/video/vie_channel.cc:371: void ViEChannel::SetSendCodec(const VideoCodec&
video_codec) {
On 2016/01/19 15:27:58, pbos-webrtc wrote:
> On 2016/01/19 12:02:10, stefan-webrtc (holmer) wrote:
> > Should we check what thread this is being called on?
> 
> It'd be interesting to inject thread checkers from VideoSendStream that'd be
> shared across objects (encoder_thread_checker). I think that sounds like a
> follow-up though?

Can't you have a separate thread checker?

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/vie_channel.cc#n...
webrtc/video/vie_channel.cc:431: }
On 2016/01/19 15:27:58, pbos-webrtc wrote:
> On 2016/01/19 12:02:10, stefan-webrtc (holmer) wrote:
> > When reading the changes for this method I can't see much that actually did
> > change. Can you please summarize what you have done here? Did you only
remove
> > registration/deregistration of rtp modules?
> 
> Yes, and move crit_ to cover the entire function (so that it remains
> synchronized with StartSend and StopSend).

Ok!

[pbos-webrtc, 2016-01-20 18:56:55]

skip counting frames dropped before rendering

[pbos-webrtc, 2016-01-20 19:04:45]

WDYT w/ removing the encoder thread to VideoSendStream?

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
File webrtc/video/video_capture_input.cc (right):

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
webrtc/video/video_capture_input.cc:169: if (replace_codec_settings) {
On 2016/01/20 12:22:48, stefan-webrtc (holmer) wrote:
> On 2016/01/19 15:27:58, pbos-webrtc wrote:
> > On 2016/01/19 12:02:09, stefan-webrtc (holmer) wrote:
> > > I don't see how this can ever be true?
> > 
> > Line 155 makes it true if TriggerSetSendCodec have been called.
> 
> Ah, I misread line 158 to be "replace_codec_settings = false". Maybe call it
> old_replace_codec_settings or something to make it a bit easier to read?

Calling it new_codec_settings, they're still the new ones that need to be
applied.

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
webrtc/video/video_capture_input.cc:177: // TODO(pbos): Mock VideoSendStream.
On 2016/01/20 12:22:48, stefan-webrtc (holmer) wrote:
> On 2016/01/19 15:27:58, pbos-webrtc wrote:
> > On 2016/01/19 12:02:09, stefan-webrtc (holmer) wrote:
> > > I think this TODO should be more detailed. You shouldn't use mocks in
> > production
> > > code?
> > 
> > Updated TODO
> 
> Maybe it actually makes sense to do this now so that we can properly test this
> class. I assume that now all tests are passing a nullptr as video_send_stream,
> which means we actually don't test this path?

All quality tests and otherwise will start failing if this doesn't hold, since
we won't reconfigure the encoder.

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
webrtc/video/video_capture_input.cc:179:
video_send_stream_->SetSendCodec(codec_settings);
On 2016/01/20 12:22:48, stefan-webrtc (holmer) wrote:
> On 2016/01/19 15:27:58, pbos-webrtc wrote:
> > On 2016/01/19 12:02:09, stefan-webrtc (holmer) wrote:
> > > Shouldn't this call directly into the vcm? Seems a bit odd that it calls
the
> > vcm
> > > directly for some things and then goes via video_send_stream_ for other. I
> get
> > > the feeling like we're mixing layers here somehow. Should the
> > VideoCaptureInput
> > > be allowed to talk to the vcm at all?
> > 
> > It needs to configure vie_channel, vie_channel and stats_proxy which
> > VideoSendStream already has. Is this too ugly, should I move vie_encoder and
> > vie_channel down here? Then I'll have to have both frame_callback (for
> testing)
> > and vie_encoder for the actual object (when not testing).
> > 
> > I'm not sure I like the alternative right now.
> 
> I guess one of the vie_channel you mentioned should be vie_encoder?
> 
> Not sure what is best. Maybe we should accept a dependency from
> VideoCaptureInput on VideoSendStream? I dislike it because of the loop we get,
> where both depend on each other.
> 
> What you really want is to post the call to SetSendCodec onto the encoder
> thread. If that would have been possible that would have been your preferred
> solution, right? Or would you still have involved VideoCaptureInput?
> 
> I'm mostly trying to understand how this needs to work.

I still think that VideoCaptureInput should be aware of whether the codec is
initialized or not. Maybe the encoder thread should be broken out of
VideoCaptureInput and put in VideoSendStream?

[pbos-webrtc, 2016-01-20 19:05:22]

rename new_codec_settings

[pbos-webrtc, 2016-01-26 17:31:28]

ping :)

[stefan-webrtc, 2016-01-27 09:06:43]

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
File webrtc/video/video_capture_input.cc (right):

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
webrtc/video/video_capture_input.cc:177: // TODO(pbos): Mock VideoSendStream.
On 2016/01/20 19:04:45, pbos-webrtc wrote:
> On 2016/01/20 12:22:48, stefan-webrtc (holmer) wrote:
> > On 2016/01/19 15:27:58, pbos-webrtc wrote:
> > > On 2016/01/19 12:02:09, stefan-webrtc (holmer) wrote:
> > > > I think this TODO should be more detailed. You shouldn't use mocks in
> > > production
> > > > code?
> > > 
> > > Updated TODO
> > 
> > Maybe it actually makes sense to do this now so that we can properly test
this
> > class. I assume that now all tests are passing a nullptr as
video_send_stream,
> > which means we actually don't test this path?
> 
> All quality tests and otherwise will start failing if this doesn't hold, since
> we won't reconfigure the encoder.

Ok, good, but if we think it's worth adding a todo here, isn't it worth taking
the time to actually add the test? Seems worthwhile to me to add tests when we
anyway are making a rather big change.

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/video_capture_in...
webrtc/video/video_capture_input.cc:179:
video_send_stream_->SetSendCodec(codec_settings);
On 2016/01/20 19:04:45, pbos-webrtc wrote:
> On 2016/01/20 12:22:48, stefan-webrtc (holmer) wrote:
> > On 2016/01/19 15:27:58, pbos-webrtc wrote:
> > > On 2016/01/19 12:02:09, stefan-webrtc (holmer) wrote:
> > > > Shouldn't this call directly into the vcm? Seems a bit odd that it calls
> the
> > > vcm
> > > > directly for some things and then goes via video_send_stream_ for other.
I
> > get
> > > > the feeling like we're mixing layers here somehow. Should the
> > > VideoCaptureInput
> > > > be allowed to talk to the vcm at all?
> > > 
> > > It needs to configure vie_channel, vie_channel and stats_proxy which
> > > VideoSendStream already has. Is this too ugly, should I move vie_encoder
and
> > > vie_channel down here? Then I'll have to have both frame_callback (for
> > testing)
> > > and vie_encoder for the actual object (when not testing).
> > > 
> > > I'm not sure I like the alternative right now.
> > 
> > I guess one of the vie_channel you mentioned should be vie_encoder?
> > 
> > Not sure what is best. Maybe we should accept a dependency from
> > VideoCaptureInput on VideoSendStream? I dislike it because of the loop we
get,
> > where both depend on each other.
> > 
> > What you really want is to post the call to SetSendCodec onto the encoder
> > thread. If that would have been possible that would have been your preferred
> > solution, right? Or would you still have involved VideoCaptureInput?
> > 
> > I'm mostly trying to understand how this needs to work.
> 
> I still think that VideoCaptureInput should be aware of whether the codec is
> initialized or not. Maybe the encoder thread should be broken out of
> VideoCaptureInput and put in VideoSendStream?

Yes, that might be an option? I think I would like that better. Let's here what
Magnus thinks.

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/vie_channel.cc
File webrtc/video/vie_channel.cc (right):

https://codereview.webrtc.org/1600973002/diff/1/webrtc/video/vie_channel.cc#n...
webrtc/video/vie_channel.cc:204: }
On 2016/01/20 12:22:48, stefan-webrtc (holmer) wrote:
> On 2016/01/19 15:27:58, pbos-webrtc wrote:
> > On 2016/01/19 12:02:10, stefan-webrtc (holmer) wrote:
> > > Could you enlighten why this was necessary to change in this CL?
> > 
> > I can only add/remove modules from the construction thread, but I need to
turn
> > on/off RTP modules on the fly from the encoder thread.
> 
> I don't think I fully understand. How is the process thread involved with the
> construction thread and the encoder thread?
> 
> Is it because you want to avoid having to register/deregister modules on the
> process thread for some reason? Seems like we are basically duplicating code
> that the ProcessThread aldready has?

Ping

[mflodman, 2016-01-27 09:38:40]

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

[mflodman, 2016-01-27 09:38:41]

Sorry for the late reply, but I have a few high level comments for this.

- I'd prefer to break out this in smaller CLs, like the list to vector change
doesn't really have to be in this CL and ads a bit of noise.

- Can the new logic in VideoCaptureInput instead be done in ViEEncoder? IMHO It
would be nice to avoid VideoSendStream, VideoSendStream::Config and ViEEncoder
pointers in VideoCaptureInput. This CL spreads out the logic a bit in code we're
trying to refactor, code that already today is no the easiest code to follow.