Combining SetVideoSend and SetSource into one method.

Combining SetVideoSend and SetSource into one method.

This means there's only one thread hop to the worker thread.

At the video engine level, SetOptions and SetSource
are combined into one method (all within the same critical section)
which ensures that no frame will be encoded while SetVideoSend
is only partially finished.

BUG=webrtc:5691
Committed: https://crrev.com/5a4a75ae48382d8a85105d7fcd173718db8501c5
Cr-Commit-Position: refs/heads/master@{#13022}

[Taylor Brandstetter, 2016-03-29 22:30:34]

deadbeef@webrtc.org changed reviewers:
+ pbos@webrtc.org, pthatcher@webrtc.org

[Taylor Brandstetter, 2016-03-29 22:30:35]

pbos: PTAL at webrtc/media/*
pthatcher: PTAL at everything else

https://codereview.webrtc.org/1838413002/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideoengine2.cc (left):

https://codereview.webrtc.org/1838413002/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2.cc:1647: return true;
I got rid of this early return and added "if (capturer_changing && capturer_ !=
nullptr)" below because I thought that makes the method's flow more clear.

[pbos-webrtc, 2016-03-30 12:23:06]

As I understand this is pending/needs to be rebased on top of some refactoring
by nisse@/perkj@ right? Let me know if I should review it yet.

[Taylor Brandstetter, 2016-03-30 16:24:31]

On 2016/03/30 12:23:06, pbos-webrtc wrote:
> As I understand this is pending/needs to be rebased on top of some refactoring
> by nisse@/perkj@ right? Let me know if I should review it yet.

Correct. I'll wait for their CLs to land and then merge; nothing to review for
now.

[Taylor Brandstetter, 2016-03-30 16:24:38]

deadbeef@webrtc.org changed reviewers:
- pbos@webrtc.org, pthatcher@webrtc.org

[pthatcher1, 2016-03-30 17:14:13]

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

[pthatcher1, 2016-03-30 17:14:14]

This looks very good.  I look forward to seeing how it looks once it's merged
with the latest refactoring.

[Taylor Brandstetter, 2016-05-13 21:07:46]

Description was changed from

==========
Combining SetVideoSend and SetCapturer into one method.

This means there's only one thread hop to the worker thread.

At the video engine level, MuteStream, SetOptions and SetCapturer
are combined into one method (all within the same critical section)
which ensures that no frame will be encoded while SetVideoSend
is only partially finished.

Another small optimization: the encoder will not be reconfigured
if the options don't change at all.

BUG=webrtc:5691
==========

to

==========
Combining SetVideoSend and SetCapturer into one method.

This means there's only one thread hop to the worker thread.

At the video engine level, SetOptions and SetCapturer
are combined into one method (all within the same critical section)
which ensures that no frame will be encoded while SetVideoSend
is only partially finished.

BUG=webrtc:5691
==========

[Taylor Brandstetter, 2016-05-13 21:08:01]

Description was changed from

==========
Combining SetVideoSend and SetCapturer into one method.

This means there's only one thread hop to the worker thread.

At the video engine level, SetOptions and SetCapturer
are combined into one method (all within the same critical section)
which ensures that no frame will be encoded while SetVideoSend
is only partially finished.

BUG=webrtc:5691
==========

to

==========
Combining SetVideoSend and SetSource into one method.

This means there's only one thread hop to the worker thread.

At the video engine level, SetOptions and SetSource
are combined into one method (all within the same critical section)
which ensures that no frame will be encoded while SetVideoSend
is only partially finished.

BUG=webrtc:5691
==========

[Taylor Brandstetter, 2016-05-13 21:09:28]

deadbeef@webrtc.org changed reviewers:
+ pbos@webrtc.org

[Taylor Brandstetter, 2016-05-13 21:09:29]

This CL has been refactored; review away

[pbos-webrtc, 2016-05-14 00:42:39]

lgtm, please don't submit before pthatcher@ has reviewed, I'd like a second pair
of eyes on this

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

https://codereview.webrtc.org/1838413002/diff/60001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1033: << ", source = " << (source !=
nullptr ? "(source)" : "NULL")
"nullptr"

https://codereview.webrtc.org/1838413002/diff/60001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1041: LOG(LS_ERROR) << "No sending
stream on ssrc " << ssrc;
This log will never happen, do RTC_CHECK(...) <<

https://codereview.webrtc.org/1838413002/diff/60001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1647: if (source_changing && source_
!= nullptr) {
&& source_

[Taylor Brandstetter, 2016-05-16 19:03:00]

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

https://codereview.webrtc.org/1838413002/diff/60001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1033: << ", source = " << (source !=
nullptr ? "(source)" : "NULL")
On 2016/05/14 00:42:39, pbos-webrtc wrote:
> "nullptr"

Done.

https://codereview.webrtc.org/1838413002/diff/60001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1041: LOG(LS_ERROR) << "No sending
stream on ssrc " << ssrc;
On 2016/05/14 00:42:39, pbos-webrtc wrote:
> This log will never happen, do RTC_CHECK(...) <<

I believe it actually can happen due to this issue:
https://bugs.chromium.org/p/webrtc/issues/detail?id=4908

Hence the workaround to only RTC_CHECK if the source is not nullptr.

https://codereview.webrtc.org/1838413002/diff/60001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1647: if (source_changing && source_
!= nullptr) {
On 2016/05/14 00:42:39, pbos-webrtc wrote:
> && source_

Done.

[pbos-webrtc, 2016-05-16 19:04:50]

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

https://codereview.webrtc.org/1838413002/diff/60001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1041: LOG(LS_ERROR) << "No sending
stream on ssrc " << ssrc;
On 2016/05/16 19:03:00, Taylor Brandstetter wrote:
> On 2016/05/14 00:42:39, pbos-webrtc wrote:
> > This log will never happen, do RTC_CHECK(...) <<
> 
> I believe it actually can happen due to this issue:
> https://bugs.chromium.org/p/webrtc/issues/detail?id=4908
> 
> Hence the workaround to only RTC_CHECK if the source is not nullptr.

Oh, totally misread this. Carry on. :)

[Taylor Brandstetter, 2016-06-01 19:21:18]

Pinging Peter; no rush, but just in case you missed it, this is ready for review
again.

[pthatcher1, 2016-06-01 22:18:04]

https://codereview.webrtc.org/1838413002/diff/80001/webrtc/api/rtpsender.cc
File webrtc/api/rtpsender.cc (right):

https://codereview.webrtc.org/1838413002/diff/80001/webrtc/api/rtpsender.cc#n...
webrtc/api/rtpsender.cc:307: provider_->SetVideoSend(ssrc_, false, nullptr,
nullptr);
It might be more readable to make a private method called ClearVideoSend() or
StopVideoSend() that does this line, and then use it in 3 places.

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

https://codereview.webrtc.org/1838413002/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1023: // method.
If we're going to remove that argument, should we call the method SetVideoSource
instead of SetVideoSend?

And should SetVideoPlayout be SetVideoSink?

https://codereview.webrtc.org/1838413002/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1605: bool source_changing = source_
!= source;
This might be a little more clear this options_changing, like so:

bool source_changing = source_ != source;
bool options_changing = (enable && options);
if (source_changing) {
  // This can and should be done before taking the lock.
  DisconnectSource();
}

if (source_changing || options_changing) {
  rtc::CritScope cs(&lock_);
  if (options_changing) {
    ... options changing part ...
  }
}

...

[Taylor Brandstetter, 2016-06-02 19:12:52]

https://codereview.webrtc.org/1838413002/diff/80001/webrtc/api/rtpsender.cc
File webrtc/api/rtpsender.cc (right):

https://codereview.webrtc.org/1838413002/diff/80001/webrtc/api/rtpsender.cc#n...
webrtc/api/rtpsender.cc:307: provider_->SetVideoSend(ssrc_, false, nullptr,
nullptr);
On 2016/06/01 22:18:03, pthatcher1 wrote:
> It might be more readable to make a private method called ClearVideoSend() or
> StopVideoSend() that does this line, and then use it in 3 places.

Done.

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

https://codereview.webrtc.org/1838413002/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1023: // method.
On 2016/06/01 22:18:03, pthatcher1 wrote:
> If we're going to remove that argument, should we call the method
SetVideoSource
> instead of SetVideoSend?
> 
> And should SetVideoPlayout be SetVideoSink?

There's also options though. SetVideoSourceAndOptions? I don't think this needs
to prevent landing the CL though, I can remove "enable" later.

https://codereview.webrtc.org/1838413002/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1605: bool source_changing = source_
!= source;
On 2016/06/01 22:18:04, pthatcher1 wrote:
> This might be a little more clear this options_changing, like so:
> 
> bool source_changing = source_ != source;
> bool options_changing = (enable && options);
> if (source_changing) {
>   // This can and should be done before taking the lock.
>   DisconnectSource();
> }
> 
> if (source_changing || options_changing) {
>   rtc::CritScope cs(&lock_);
>   if (options_changing) {
>     ... options changing part ...
>   }
> }
> 
> ... 

"enable && options" doesn't mean the options are changing though. And we can't
check if they're changing outside of the lock. I did what you suggested, I just
renamed the variable to "options_present".

[pthatcher1, 2016-06-02 22:22:51]

lgtm

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

https://codereview.webrtc.org/1838413002/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1023: // method.
On 2016/06/02 19:12:51, Taylor Brandstetter wrote:
> On 2016/06/01 22:18:03, pthatcher1 wrote:
> > If we're going to remove that argument, should we call the method
> SetVideoSource
> > instead of SetVideoSend?
> > 
> > And should SetVideoPlayout be SetVideoSink?
> 
> There's also options though. SetVideoSourceAndOptions? I don't think this
needs
> to prevent landing the CL though, I can remove "enable" later.

We're moving all of the remaining options to be either part of the source, or a
part of RtpParameters.  So eventually there won't be any options.  So I'm fine
calling it SetVideoSource.

I agree it doesn't block the CL, though.

[Taylor Brandstetter, 2016-06-02 22:36:41]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-06-02 22:36:41]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1838413002/#ps140001 (title:
"Adding TODO.")

[commit-bot: I haz the power, 2016-06-02 22:36:51]

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

[commit-bot: I haz the power, 2016-06-02 23:23:43]

Description was changed from

==========
Combining SetVideoSend and SetSource into one method.

This means there's only one thread hop to the worker thread.

At the video engine level, SetOptions and SetSource
are combined into one method (all within the same critical section)
which ensures that no frame will be encoded while SetVideoSend
is only partially finished.

BUG=webrtc:5691
==========

to

==========
Combining SetVideoSend and SetSource into one method.

This means there's only one thread hop to the worker thread.

At the video engine level, SetOptions and SetSource
are combined into one method (all within the same critical section)
which ensures that no frame will be encoded while SetVideoSend
is only partially finished.

BUG=webrtc:5691
==========

[commit-bot: I haz the power, 2016-06-02 23:23:44]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-06-02 23:23:55]

Description was changed from

==========
Combining SetVideoSend and SetSource into one method.

This means there's only one thread hop to the worker thread.

At the video engine level, SetOptions and SetSource
are combined into one method (all within the same critical section)
which ensures that no frame will be encoded while SetVideoSend
is only partially finished.

BUG=webrtc:5691
==========

to

==========
Combining SetVideoSend and SetSource into one method.

This means there's only one thread hop to the worker thread.

At the video engine level, SetOptions and SetSource
are combined into one method (all within the same critical section)
which ensures that no frame will be encoded while SetVideoSend
is only partially finished.

BUG=webrtc:5691

Committed: https://crrev.com/5a4a75ae48382d8a85105d7fcd173718db8501c5
Cr-Commit-Position: refs/heads/master@{#13022}
==========

[commit-bot: I haz the power, 2016-06-02 23:23:57]

Patchset 8 (id:??) landed as
https://crrev.com/5a4a75ae48382d8a85105d7fcd173718db8501c5
Cr-Commit-Position: refs/heads/master@{#13022}