Remove VoEVideoSync interface.

Remove VoEVideoSync interface.

The removed tests are covered by cases in call_perf_tests.

BUG=webrtc:4690
Review-Url: https://codereview.webrtc.org/2672583002
Cr-Commit-Position: refs/heads/master@{#16621}
Committed: https://chromium.googlesource.com/external/webrtc/+/08b19dfc671306e37640a18250aeebefd4e6a92a

[the sun, 2017-02-08 20:42:31]

Description was changed from

==========
Remove VoEVideoSync interface.

WIP

BUG=webrtc:4690
==========

to

==========
Remove VoEVideoSync interface.

BUG=webrtc:4690
==========

[the sun, 2017-02-08 20:46:22]

Description was changed from

==========
Remove VoEVideoSync interface.

BUG=webrtc:4690
==========

to

==========
Remove VoEVideoSync interface.

The removed tests are covered by cases in call_perf_tests.

BUG=webrtc:4690
==========

[the sun, 2017-02-08 20:46:23]

solenberg@webrtc.org changed reviewers:
+ kwiberg@webrtc.org, stefan@webrtc.org

[kwiberg-webrtc, 2017-02-09 10:16:40]

https://codereview.webrtc.org/2672583002/diff/60001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2672583002/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:1236: // This needs to be done before |sending|
is set to true.
Is this comment still good? It looks like you're doing it afterwards now...

https://codereview.webrtc.org/2672583002/diff/60001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel_proxy.cc (right):

https://codereview.webrtc.org/2672583002/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel_proxy.cc:137: // Called on worker thread or Call's
module_process_thread_.
Umm... First, can you use thread checkers to verify this? That's pretty much
universally a good idea.

Second, what reason is there to restrict this function to either of two
different threads? You don't protect against concurrent calls that way.

[the sun, 2017-02-11 12:05:54]

https://codereview.webrtc.org/2672583002/diff/60001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2672583002/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:1236: // This needs to be done before |sending|
is set to true.
On 2017/02/09 10:16:40, kwiberg-webrtc wrote:
> Is this comment still good? It looks like you're doing it afterwards now...

Updated the comment to be more clear. AFAICT it still applies since
RTPSender::SetSendingStatus() may otherwise randomize the sequence number.

https://codereview.webrtc.org/2672583002/diff/60001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel_proxy.cc (right):

https://codereview.webrtc.org/2672583002/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel_proxy.cc:137: // Called on worker thread or Call's
module_process_thread_.
On 2017/02/09 10:16:40, kwiberg-webrtc wrote:
> Umm... First, can you use thread checkers to verify this? That's pretty much
> universally a good idea.
> 
> Second, what reason is there to restrict this function to either of two
> different threads? You don't protect against concurrent calls that way.

Are you saying an rtc::RaceChecker would be more appropriate?

[kwiberg-webrtc, 2017-02-11 21:25:25]

https://codereview.webrtc.org/2672583002/diff/60001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2672583002/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:1236: // This needs to be done before |sending|
is set to true.
On 2017/02/11 12:05:51, the sun wrote:
> On 2017/02/09 10:16:40, kwiberg-webrtc wrote:
> > Is this comment still good? It looks like you're doing it afterwards now...
> 
> Updated the comment to be more clear. AFAICT it still applies since
> RTPSender::SetSendingStatus() may otherwise randomize the sequence number.

Oh, OK, it was referring to the _rtpRtcpModule->SetSendingStatus(true) call
below, not the channel_state_.SetSending(true) call.

https://codereview.webrtc.org/2672583002/diff/60001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel_proxy.cc (right):

https://codereview.webrtc.org/2672583002/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel_proxy.cc:137: // Called on worker thread or Call's
module_process_thread_.
On 2017/02/11 12:05:53, the sun wrote:
> On 2017/02/09 10:16:40, kwiberg-webrtc wrote:
> > Umm... First, can you use thread checkers to verify this? That's pretty much
> > universally a good idea.
> > 
> > Second, what reason is there to restrict this function to either of two
> > different threads? You don't protect against concurrent calls that way.
> 
> Are you saying an rtc::RaceChecker would be more appropriate?

I'm just saying that I don't understand what you're doing... or rather, why
you're doing it. AFAICT, this method, which was previously only safe to call
from the thread thread_checker_ has imprinted on, is now safe to call from
either of the two threads you mention in the comment. But you don't explain why
it's safe to allow two threads in. (If only one thread is allowed, there can be
no race, but two threads can certainly race.)

Also, even if allowing those two threads and no others is a good idea, it's
probably best to enforce that with more than just a comment. I think you could
do something like

  RTC_DCHECK(thread_checker_1_.CalledOnValidThread()
             || thread_checker_2_.CalledOnValidThread());

(note: I've never tried this!) followed by a race checker test to assert that
the two threads are in fact not racing.

[the sun, 2017-02-11 23:24:54]

https://codereview.webrtc.org/2672583002/diff/60001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel_proxy.cc (right):

https://codereview.webrtc.org/2672583002/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel_proxy.cc:137: // Called on worker thread or Call's
module_process_thread_.
On 2017/02/11 21:25:25, kwiberg-webrtc wrote:
> On 2017/02/11 12:05:53, the sun wrote:
> > On 2017/02/09 10:16:40, kwiberg-webrtc wrote:
> > > Umm... First, can you use thread checkers to verify this? That's pretty
much
> > > universally a good idea.
> > > 
> > > Second, what reason is there to restrict this function to either of two
> > > different threads? You don't protect against concurrent calls that way.
> > 
> > Are you saying an rtc::RaceChecker would be more appropriate?
> 
> I'm just saying that I don't understand what you're doing... or rather, why
> you're doing it. AFAICT, this method, which was previously only safe to call
> from the thread thread_checker_ has imprinted on, is now safe to call from
> either of the two threads you mention in the comment. But you don't explain
why
> it's safe to allow two threads in. (If only one thread is allowed, there can
be
> no race, but two threads can certainly race.)
> 
> Also, even if allowing those two threads and no others is a good idea, it's
> probably best to enforce that with more than just a comment. I think you could
> do something like
> 
>   RTC_DCHECK(thread_checker_1_.CalledOnValidThread()
>              || thread_checker_2_.CalledOnValidThread());
> 
> (note: I've never tried this!) followed by a race checker test to assert that
> the two threads are in fact not racing.

Ah, well. The underlying code (voe::Channel) is sprinkled with locks to handle
all calling scenarios the VoE APIs allow. There are basically no restrictions on
which thread(s) call the APIs.

ChannelProxy adds an outer shell on voe::Channel and uses thread checker to try
and *add* some restrictions so that someday down the line, we can step into a
new, blissful and lockless world. So the thread checkers here function as
best-effort documentation of current behavior. In the cases where the threading
model is dubious, I've chosen to leave a comment rather than add multiple
checkers. The DeliverRtp/DeliverRtcp methods on AudioReceiveStream are similar
(test code and production code have different threading models).

Probably, the best way to deal with the situation is to pull out the aspects
from voe::Channel that are accessed on different threads and establish
non-ambiguous ownership. I think both thread checkers and comments helps pin
pointing where effort is required.

I've actually grown less fond of thread checkers as I've used them. Most of the
time we just want to enforce single-threaded behavior - that methods on an
object are called in sequence. What actual thread they are accessed from doesn't
matter, and enforcing a specific thread precludes e.g. "legal" transfer of
ownership from one thread to another (there was an issue in Chromium with just
that).

A design where everything is single-threaded and concurrency is supported by the
use of channels, would be easier to reason about.

[kwiberg-webrtc, 2017-02-12 00:58:37]

https://codereview.webrtc.org/2672583002/diff/60001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel_proxy.cc (right):

https://codereview.webrtc.org/2672583002/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel_proxy.cc:137: // Called on worker thread or Call's
module_process_thread_.
On 2017/02/11 23:24:53, the sun wrote:
> On 2017/02/11 21:25:25, kwiberg-webrtc wrote:
> > On 2017/02/11 12:05:53, the sun wrote:
> > > On 2017/02/09 10:16:40, kwiberg-webrtc wrote:
> > > > Umm... First, can you use thread checkers to verify this? That's pretty
> much
> > > > universally a good idea.
> > > > 
> > > > Second, what reason is there to restrict this function to either of two
> > > > different threads? You don't protect against concurrent calls that way.
> > > 
> > > Are you saying an rtc::RaceChecker would be more appropriate?
> > 
> > I'm just saying that I don't understand what you're doing... or rather, why
> > you're doing it. AFAICT, this method, which was previously only safe to call
> > from the thread thread_checker_ has imprinted on, is now safe to call from
> > either of the two threads you mention in the comment. But you don't explain
> why
> > it's safe to allow two threads in. (If only one thread is allowed, there can
> be
> > no race, but two threads can certainly race.)
> > 
> > Also, even if allowing those two threads and no others is a good idea, it's
> > probably best to enforce that with more than just a comment. I think you
could
> > do something like
> > 
> >   RTC_DCHECK(thread_checker_1_.CalledOnValidThread()
> >              || thread_checker_2_.CalledOnValidThread());
> > 
> > (note: I've never tried this!) followed by a race checker test to assert
that
> > the two threads are in fact not racing.
> 
> Ah, well. The underlying code (voe::Channel) is sprinkled with locks to handle
> all calling scenarios the VoE APIs allow. There are basically no restrictions
on
> which thread(s) call the APIs.
> 
> ChannelProxy adds an outer shell on voe::Channel and uses thread checker to
try
> and *add* some restrictions so that someday down the line, we can step into a
> new, blissful and lockless world. So the thread checkers here function as
> best-effort documentation of current behavior. In the cases where the
threading
> model is dubious, I've chosen to leave a comment rather than add multiple
> checkers. The DeliverRtp/DeliverRtcp methods on AudioReceiveStream are similar
> (test code and production code have different threading models).
> 
> Probably, the best way to deal with the situation is to pull out the aspects
> from voe::Channel that are accessed on different threads and establish
> non-ambiguous ownership. I think both thread checkers and comments helps pin
> pointing where effort is required.
> 
> I've actually grown less fond of thread checkers as I've used them. Most of
the
> time we just want to enforce single-threaded behavior - that methods on an
> object are called in sequence. What actual thread they are accessed from
doesn't
> matter, and enforcing a specific thread precludes e.g. "legal" transfer of
> ownership from one thread to another (there was an issue in Chromium with just
> that).
> 
> A design where everything is single-threaded and concurrency is supported by
the
> use of channels, would be easier to reason about.

OK. Then it sounds like you want a race checker---just an assertion of
non-concurrency, without specifying what threads are allowed.

Both thread checkers and race checkers are designed to apply to all accesses of
a member variable, though, and I see most methods here use thread checkers. It
may get icky trying to mix and match.

[the sun, 2017-02-12 10:18:13]

Added better thread checking

[the sun, 2017-02-12 10:20:22]

On 2017/02/12 00:58:37, kwiberg-webrtc wrote:
>
https://codereview.webrtc.org/2672583002/diff/60001/webrtc/voice_engine/chann...
> File webrtc/voice_engine/channel_proxy.cc (right):
> 
>
https://codereview.webrtc.org/2672583002/diff/60001/webrtc/voice_engine/chann...
> webrtc/voice_engine/channel_proxy.cc:137: // Called on worker thread or Call's
> module_process_thread_.
> On 2017/02/11 23:24:53, the sun wrote:
> > On 2017/02/11 21:25:25, kwiberg-webrtc wrote:
> > > On 2017/02/11 12:05:53, the sun wrote:
> > > > On 2017/02/09 10:16:40, kwiberg-webrtc wrote:
> > > > > Umm... First, can you use thread checkers to verify this? That's
pretty
> > much
> > > > > universally a good idea.
> > > > > 
> > > > > Second, what reason is there to restrict this function to either of
two
> > > > > different threads? You don't protect against concurrent calls that
way.
> > > > 
> > > > Are you saying an rtc::RaceChecker would be more appropriate?
> > > 
> > > I'm just saying that I don't understand what you're doing... or rather,
why
> > > you're doing it. AFAICT, this method, which was previously only safe to
call
> > > from the thread thread_checker_ has imprinted on, is now safe to call from
> > > either of the two threads you mention in the comment. But you don't
explain
> > why
> > > it's safe to allow two threads in. (If only one thread is allowed, there
can
> > be
> > > no race, but two threads can certainly race.)
> > > 
> > > Also, even if allowing those two threads and no others is a good idea,
it's
> > > probably best to enforce that with more than just a comment. I think you
> could
> > > do something like
> > > 
> > >   RTC_DCHECK(thread_checker_1_.CalledOnValidThread()
> > >              || thread_checker_2_.CalledOnValidThread());
> > > 
> > > (note: I've never tried this!) followed by a race checker test to assert
> that
> > > the two threads are in fact not racing.
> > 
> > Ah, well. The underlying code (voe::Channel) is sprinkled with locks to
handle
> > all calling scenarios the VoE APIs allow. There are basically no
restrictions
> on
> > which thread(s) call the APIs.
> > 
> > ChannelProxy adds an outer shell on voe::Channel and uses thread checker to
> try
> > and *add* some restrictions so that someday down the line, we can step into
a
> > new, blissful and lockless world. So the thread checkers here function as
> > best-effort documentation of current behavior. In the cases where the
> threading
> > model is dubious, I've chosen to leave a comment rather than add multiple
> > checkers. The DeliverRtp/DeliverRtcp methods on AudioReceiveStream are
similar
> > (test code and production code have different threading models).
> > 
> > Probably, the best way to deal with the situation is to pull out the aspects
> > from voe::Channel that are accessed on different threads and establish
> > non-ambiguous ownership. I think both thread checkers and comments helps pin
> > pointing where effort is required.
> > 
> > I've actually grown less fond of thread checkers as I've used them. Most of
> the
> > time we just want to enforce single-threaded behavior - that methods on an
> > object are called in sequence. What actual thread they are accessed from
> doesn't
> > matter, and enforcing a specific thread precludes e.g. "legal" transfer of
> > ownership from one thread to another (there was an issue in Chromium with
just
> > that).
> > 
> > A design where everything is single-threaded and concurrency is supported by
> the
> > use of channels, would be easier to reason about.
> 
> OK. Then it sounds like you want a race checker---just an assertion of
> non-concurrency, without specifying what threads are allowed.
> 
> Both thread checkers and race checkers are designed to apply to all accesses
of
> a member variable, though, and I see most methods here use thread checkers. It
> may get icky trying to mix and match.

The interface *is* used by multiple threads, so yes, it is icky. :)

I've added a separate thread checker for the things that are currently accessed
on the ModuleProcessThread. Also another race checker for the one method
accessed on the video capture thread.

[the sun, 2017-02-12 10:21:08]

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

[commit-bot: I haz the power, 2017-02-12 10:21:20]

Dry run: 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-02-12 11:03:08]

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

[commit-bot: I haz the power, 2017-02-12 11:03:09]

Dry run: This issue passed the CQ dry run.

[kwiberg-webrtc, 2017-02-13 13:58:05]

Sorry if I'm being difficult, but the concurrency situation with this class
seems to be a bit of a mess... and you're trying to use four mutual exclusion
thingies of two different types.

If you have one thread checker protecting a method, that also protects against
concurrent access. But when you require only one of two thread checkers to OK
the method call, you must additionally protect against concurrent access by the
two threads in some other way, e.g. with a race checker.

In general, these matters are *greatly* simplified if instead of considering
each method, you can divide the member variables into groups that should each be
protected by a mutex-like thingy. It is mutable data, not code, that needs
protecting. But given that this class essentially has just one member variable
with a very complex set of protection requirements, that sort of thinking may
not be possible...

https://codereview.webrtc.org/2672583002/diff/140001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel_proxy.cc (right):

https://codereview.webrtc.org/2672583002/diff/140001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel_proxy.cc:140:
module_process_thread_checker_.CalledOnValidThread());
No race checker here?

https://codereview.webrtc.org/2672583002/diff/140001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel_proxy.cc:158:
module_process_thread_checker_.CalledOnValidThread());
Race checker too?

https://codereview.webrtc.org/2672583002/diff/140001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel_proxy.h (right):

https://codereview.webrtc.org/2672583002/diff/140001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel_proxy.h:126: rtc::RaceChecker
video_capture_thread_race_checker_;
Hmm. How does it make sense to name race checkers after threads? I would have
thought that like mutexes, race checkers should probably be named after the
stuff they protect (because they work just like mutexes except that they explode
in case of contention).

[the sun, 2017-02-13 19:43:17]

On 2017/02/13 13:58:05, kwiberg-webrtc wrote:
> Sorry if I'm being difficult, but the concurrency situation with this class
> seems to be a bit of a mess... and you're trying to use four mutual exclusion
> thingies of two different types.
> 
> If you have one thread checker protecting a method, that also protects against
> concurrent access. But when you require only one of two thread checkers to OK
> the method call, you must additionally protect against concurrent access by
the
> two threads in some other way, e.g. with a race checker.
> 
> In general, these matters are *greatly* simplified if instead of considering
> each method, you can divide the member variables into groups that should each
be
> protected by a mutex-like thingy. It is mutable data, not code, that needs
> protecting. But given that this class essentially has just one member variable
> with a very complex set of protection requirements, that sort of thinking may
> not be possible...
> 
>
https://codereview.webrtc.org/2672583002/diff/140001/webrtc/voice_engine/chan...
> File webrtc/voice_engine/channel_proxy.cc (right):
> 
>
https://codereview.webrtc.org/2672583002/diff/140001/webrtc/voice_engine/chan...
> webrtc/voice_engine/channel_proxy.cc:140:
> module_process_thread_checker_.CalledOnValidThread());
> No race checker here?
> 
>
https://codereview.webrtc.org/2672583002/diff/140001/webrtc/voice_engine/chan...
> webrtc/voice_engine/channel_proxy.cc:158:
> module_process_thread_checker_.CalledOnValidThread());
> Race checker too?
> 
>
https://codereview.webrtc.org/2672583002/diff/140001/webrtc/voice_engine/chan...
> File webrtc/voice_engine/channel_proxy.h (right):
> 
>
https://codereview.webrtc.org/2672583002/diff/140001/webrtc/voice_engine/chan...
> webrtc/voice_engine/channel_proxy.h:126: rtc::RaceChecker
> video_capture_thread_race_checker_;
> Hmm. How does it make sense to name race checkers after threads? I would have
> thought that like mutexes, race checkers should probably be named after the
> stuff they protect (because they work just like mutexes except that they
explode
> in case of contention).

As discussed off-line, added comments about how the Thread/Race checkers are
used in ChannelProxy. PTAL.

[the sun, 2017-02-14 10:37:43]

solenberg@webrtc.org changed reviewers:
- stefan@webrtc.org

[the sun, 2017-02-14 10:37:44]

Removing Stefan - this was more an FYI for you.

[kwiberg-webrtc, 2017-02-14 13:40:17]

Those comments make things much clearer. lgtm!

[the sun, 2017-02-14 16:21:16]

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

[commit-bot: I haz the power, 2017-02-14 16:21:18]

Dry run: 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-02-14 16:33:13]

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

[commit-bot: I haz the power, 2017-02-14 16:33:15]

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

[the sun, 2017-02-14 17:05:29]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2017-02-14 17:05:29]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2672583002/#ps180001 (title:
"rebase")

[commit-bot: I haz the power, 2017-02-14 17:05:38]

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-02-14 17:15:16]

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

[commit-bot: I haz the power, 2017-02-14 17:15:17]

Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/21311)

[the sun, 2017-02-15 08:18:16]

The CQ bit was checked by solenberg@webrtc.org

[commit-bot: I haz the power, 2017-02-15 08:18:22]

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-02-15 08:42:32]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1487146696860900,
"parent_rev": "0ac1904e3902f65708549b739625cf3e9f921dbd", "commit_rev":
"08b19dfc671306e37640a18250aeebefd4e6a92a"}

[commit-bot: I haz the power, 2017-02-15 08:42:34]

Description was changed from

==========
Remove VoEVideoSync interface.

The removed tests are covered by cases in call_perf_tests.

BUG=webrtc:4690
==========

to

==========
Remove VoEVideoSync interface.

The removed tests are covered by cases in call_perf_tests.

BUG=webrtc:4690

Review-Url: https://codereview.webrtc.org/2672583002
Cr-Commit-Position: refs/heads/master@{#16621}
Committed:
https://chromium.googlesource.com/external/webrtc/+/08b19dfc671306e37640a1825...
==========

[commit-bot: I haz the power, 2017-02-15 08:42:36]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/external/webrtc/+/08b19dfc671306e37640a1825...