Remove VoEVideoSync interface.

webrtc/voice_engine/channel_proxy.cc

Index: webrtc/voice_engine/channel_proxy.cc
diff --git a/webrtc/voice_engine/channel_proxy.cc b/webrtc/voice_engine/channel_proxy.cc
index a58b2e19557790c9248ae0133a714fa4d50729ae..85602cbf47f8ca6cce0f47faceb8fd915cdaea4d 100644
--- a/webrtc/voice_engine/channel_proxy.cc
+++ b/webrtc/voice_engine/channel_proxy.cc
@@ -134,7 +134,7 @@ int32_t ChannelProxy::GetSpeechOutputLevelFullRange() const {
 }
 
 uint32_t ChannelProxy::GetDelayEstimate() const {
-  RTC_DCHECK(thread_checker_.CalledOnValidThread());
+  // Called on worker thread or Call's module_process_thread_.

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

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:53]

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

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

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:53]

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]

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.

   return channel()->GetDelayEstimate();
 }
 
@@ -267,16 +267,6 @@ void ChannelProxy::GetRtpRtcp(RtpRtcp** rtp_rtcp,
   RTC_DCHECK_EQ(0, error);
 }
 
-void ChannelProxy::GetDelayEstimate(int* jitter_buffer_delay_ms,
-                                    int* playout_buffer_delay_ms) const {
-  // Called on Call's module_process_thread_.
-  RTC_DCHECK(jitter_buffer_delay_ms);
-  RTC_DCHECK(playout_buffer_delay_ms);
-  bool error = channel()->GetDelayEstimate(jitter_buffer_delay_ms,
-                                           playout_buffer_delay_ms);
-  RTC_DCHECK(error);
-}
-
 uint32_t ChannelProxy::GetPlayoutTimestamp() const {
   // Called on video capture thread.
   unsigned int timestamp = 0;