Remove VoEVideoSync interface.

webrtc/voice_engine/channel_proxy.cc

 /*
  *  Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 116 matching lines @@
 
 int32_t ChannelProxy::GetSpeechOutputLevelFullRange() const {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
   uint32_t level = 0;
   int error = channel()->GetSpeechOutputLevelFullRange(level);
   RTC_DCHECK_EQ(0, error);
   return static_cast<int32_t>(level);
 }
 
 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();
 }
 
 bool ChannelProxy::SetSendTelephoneEventPayloadType(int payload_type,
                                                     int payload_frequency) {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
   return channel()->SetSendTelephoneEventPayloadType(payload_type,
                                                      payload_frequency) == 0;
 }
 
@@ skipping 112 matching lines @@
 
 void ChannelProxy::GetRtpRtcp(RtpRtcp** rtp_rtcp,
                               RtpReceiver** rtp_receiver) const {
   // Called on Call's module_process_thread_.
   RTC_DCHECK(rtp_rtcp);
   RTC_DCHECK(rtp_receiver);
   int error = channel()->GetRtpRtcp(rtp_rtcp, rtp_receiver);
   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;
   int error = channel()->GetPlayoutTimestamp(timestamp);
   RTC_DCHECK(!error || timestamp == 0);
   return timestamp;
 }
 
 void ChannelProxy::SetMinimumPlayoutDelay(int delay_ms) {
   // Called on Call's module_process_thread_.
@@ skipping 77 matching lines @@
   return channel()->SetSendCNPayloadType(type, frequency) == 0;
 }
 
 Channel* ChannelProxy::channel() const {
   RTC_DCHECK(channel_owner_.channel());
   return channel_owner_.channel();
 }
 
 }  // namespace voe
 }  // namespace webrtc