Support for remote audio into tracks

Support for unmixed remote audio into tracks.

BUG=chromium:121673
R=solenberg@webrtc.org

Committed: https://crrev.com/f888bb58da04c5095759b5ec7ce2e1fa2cd414fd
Cr-Commit-Position: refs/heads/master@{#10995}

[tommi, 2015-12-08 22:26:41]

simplify creating a remote audio track

[tommi, 2015-12-08 22:35:03]

Description was changed from

==========
Support for remote audio into tracks

BUG=chromium:121673
==========

to

==========
Support for unmixed remote audio into tracks.

BUG=chromium:121673
==========

[tommi, 2015-12-09 21:33:52]

Fix remote source teardown, implement state, add checks and documentation

[tommi, 2015-12-09 22:20:57]

Rebase

[tommi, 2015-12-09 22:24:29]

Fix fwd decl

[tommi, 2015-12-10 01:02:31]

Change when we fire callbacks for external media

[tommi, 2015-12-10 01:09:54]

tommi@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org, perkj@webrtc.org

[tommi, 2015-12-10 01:09:54]

please take a look.

Henrik - *voice_engine*, *voiceengine*, *voe*
Per - everything else

[tommi, 2015-12-10 12:05:52]

tommi@webrtc.org changed reviewers:
+ solenberg@webrtc.org
- henrik.lundin@webrtc.org

[tommi, 2015-12-10 12:05:52]

Swapping Henrik out for The Sun!

[perkj_webrtc, 2015-12-10 12:24:05]

A few once to start with...

https://codereview.webrtc.org/1505253004/diff/100001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1505253004/diff/100001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.cc:1597: ssrc, session_.get(), stream, track_id);
The remote track can outlive the session since the track is ref counted. Is that
a problem?

https://codereview.webrtc.org/1505253004/diff/100001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/1505253004/diff/100001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface_unittest.cc:37: #include
"talk/app/webrtc/remoteaudiosource.h"
unused?

https://codereview.webrtc.org/1505253004/diff/100001/talk/app/webrtc/remoteau...
File talk/app/webrtc/remoteaudiosource.cc (right):

https://codereview.webrtc.org/1505253004/diff/100001/talk/app/webrtc/remoteau...
talk/app/webrtc/remoteaudiosource.cc:142: void RemoteAudioSource::OnClose() {
What triggers this?

https://codereview.webrtc.org/1505253004/diff/100001/talk/app/webrtc/remoteau...
talk/app/webrtc/remoteaudiosource.cc:143: main_thread_->Post(new
MessageHandler(this));
use async_invoker instead. You don't need the MessageHandler since
RemoteAudioSource is also ref counted.

https://codereview.webrtc.org/1505253004/diff/100001/talk/app/webrtc/remoteau...
File talk/app/webrtc/remoteaudiotrack.cc (right):

https://codereview.webrtc.org/1505253004/diff/100001/talk/app/webrtc/remoteau...
talk/app/webrtc/remoteaudiotrack.cc:68: scoped_refptr<AudioProcessorInterface>
RemoteAudioTrack::GetAudioProcessor() {
Are these not used for remote audio? Only local?

https://codereview.webrtc.org/1505253004/diff/100001/talk/app/webrtc/rtprecei...
File talk/app/webrtc/rtpreceiver.h (right):

https://codereview.webrtc.org/1505253004/diff/100001/talk/app/webrtc/rtprecei...
talk/app/webrtc/rtpreceiver.h:74: AudioProviderInterface* provider_;  // Set to
null in Stop().
Can you make sure |provider_| in remote source is also set to null then to
ensure it can no longer be used?

https://codereview.webrtc.org/1505253004/diff/100001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1505253004/diff/100001/talk/session/media/chann...
talk/session/media/channel.cc:1380: worker_thread()->Invoke<void>(
Use InvokeOnWorker same as the others.

[the sun, 2015-12-10 12:36:32]

Here are first few comments for VoE, assuming you'll add test cases once the
right structure is in place.

https://codereview.webrtc.org/1505253004/diff/100001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoe.h (right):

https://codereview.webrtc.org/1505253004/diff/100001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoe.h:101: external_media_(engine_) {}
I'm working towards deprecating the old VoE interfaces, so I'd REALLY rather not
add new dependencies on them. Please remove and solve in a different way... see
next comment.

https://codereview.webrtc.org/1505253004/diff/100001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1505253004/diff/100001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1258: int err =
external_media->RegisterExternalMediaProcessing(
Add an API on AudioReceiveStream instead: SetRawAudioSink(AudioRenderer::Sink*)
and either of:

a) Use the external media processing interface from the AudioReceiveStream
implementation. (less work for you but I'll have to clean it up later).
b) Add the appropriate call in VoE::Channel (and ChannelProxy) to take and
AudioRenderer::Sink all the way down (this is the way we want to go regardless).

https://codereview.webrtc.org/1505253004/diff/100001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/1505253004/diff/100001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:565: // applying scaling/panning, as that applies
to the mix operation.
Does this break any of our tests? It might break 3rd party code using the
processing API and relying on scaling etc being applied.

If you pick option b) in the previous comment you'd keep the external media code
in its original place for now, and add a new hook for AudioRenderer::Sink in
this place.

[tommi, 2015-12-10 22:36:52]

Address Per's comments

[tommi, 2015-12-10 22:37:25]

Addressed Per's comments.

Now to fix the sink.

https://codereview.webrtc.org/1505253004/diff/100001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1505253004/diff/100001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.cc:1597: ssrc, session_.get(), stream, track_id);
On 2015/12/10 12:24:05, perkj1 wrote:
> The remote track can outlive the session since the track is ref counted. Is
that
> a problem?

That should be handled in the remote track implementation.  When the session
goes away, the track clears its pointer to the session.  However, since you
pointed it out, there is probably a chance that we have a problem still. I'll
look at it but basically if we pass a session pointer here and do not use it in
order to get the OnClose notification, we could have a UAF.  In the other case,
we do handle the session going out of scope.

https://codereview.webrtc.org/1505253004/diff/100001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/1505253004/diff/100001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface_unittest.cc:37: #include
"talk/app/webrtc/remoteaudiosource.h"
On 2015/12/10 12:24:05, perkj1 wrote:
> unused?

ah, will remove.

https://codereview.webrtc.org/1505253004/diff/100001/talk/app/webrtc/remoteau...
File talk/app/webrtc/remoteaudiosource.cc (right):

https://codereview.webrtc.org/1505253004/diff/100001/talk/app/webrtc/remoteau...
talk/app/webrtc/remoteaudiosource.cc:142: void RemoteAudioSource::OnClose() {
On 2015/12/10 12:24:05, perkj1 wrote:
> What triggers this?

Deletion of the session.  It tears down the voice channels which tears down
sinks to the channels, which deletes the sink that sits between this place and
the channel, which calls OnClose on destruction.  Happens on libjingle's worker
thread.

https://codereview.webrtc.org/1505253004/diff/100001/talk/app/webrtc/remoteau...
talk/app/webrtc/remoteaudiosource.cc:143: main_thread_->Post(new
MessageHandler(this));
On 2015/12/10 12:24:05, perkj1 wrote:
> use async_invoker instead. You don't need the MessageHandler since
> RemoteAudioSource is also ref counted.

I started doing that but I'm hesitating.  Have you seen how much code that is
(even without the Bind() dependency)?  If we need more advanced message passing,
I think we could add an async invoker member variable, but as is, I think it's
overkill.

Regarding the RemoteAudioSource being ref counted, yes I know...  Perhaps the
name of this class is confusing.  This is RemoteAudioSource::MessageHandler, not
the MessageHandler interface.  See at the top of this file.

https://codereview.webrtc.org/1505253004/diff/100001/talk/app/webrtc/remoteau...
File talk/app/webrtc/remoteaudiotrack.cc (right):

https://codereview.webrtc.org/1505253004/diff/100001/talk/app/webrtc/remoteau...
talk/app/webrtc/remoteaudiotrack.cc:68: scoped_refptr<AudioProcessorInterface>
RemoteAudioTrack::GetAudioProcessor() {
On 2015/12/10 12:24:05, perkj1 wrote:
> Are these not used for remote audio? Only local? 

Local only. There's a todo to remove it but it's being used in a few places
still.  There's no need to override it here, so I removed it.

https://codereview.webrtc.org/1505253004/diff/100001/talk/app/webrtc/rtprecei...
File talk/app/webrtc/rtpreceiver.h (right):

https://codereview.webrtc.org/1505253004/diff/100001/talk/app/webrtc/rtprecei...
talk/app/webrtc/rtpreceiver.h:74: AudioProviderInterface* provider_;  // Set to
null in Stop().
On 2015/12/10 12:24:05, perkj1 wrote:
> Can you make sure |provider_| in remote source is also set to null then to
> ensure it can no longer be used? 

As discussed offline, it's handled in OnClose() (also see other comment).

https://codereview.webrtc.org/1505253004/diff/100001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1505253004/diff/100001/talk/session/media/chann...
talk/session/media/channel.cc:1380: worker_thread()->Invoke<void>(
On 2015/12/10 12:24:05, perkj1 wrote:
> Use InvokeOnWorker same as the others.

InvokeOnWorker unfortunately only works for methods that return bool.  This is
how InvokeOnWorker is implemented and how it's done elsewhere.  E.g.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

[tommi, 2015-12-10 22:52:28]

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

[commit-bot: I haz the power, 2015-12-10 22:52:33]

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

[commit-bot: I haz the power, 2015-12-10 23:06:47]

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

[commit-bot: I haz the power, 2015-12-10 23:06:47]

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

[tommi, 2015-12-11 13:09:16]

New AudioSink added

[tommi, 2015-12-11 13:15:04]

Revert unwanted VoE changes

[tommi, 2015-12-11 13:25:55]

tommi@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org

[tommi, 2015-12-11 13:25:56]

OK, ready for the next round.  I switched to a new audio sink instead of using
the voice engine APIs that Fredrik told me are going away.
I think Fredrik is OOO today, so switching *voe* reviews back over to hlundin.

[tommi, 2015-12-11 13:28:04]

On 2015/12/11 13:25:56, tommi (webrtc) wrote:
> OK, ready for the next round.  I switched to a new audio sink instead of using
> the voice engine APIs that Fredrik told me are going away.
> I think Fredrik is OOO today, so switching *voe* reviews back over to hlundin.

oh, the inttypes.h build errors on windows can be ignored for now. I'll probably
need to do some #include shuffling for inttypes.

[the sun, 2015-12-11 13:52:01]

For webrtc/audio, webrtc/voice_engine:
Structure looks good; the AudioSink::Data type will for sure be changed a bit
going forward but it does the job now.
Please add test cases to exercise the new code paths. If it isn't tested, it
didn't happen.

https://codereview.chromium.org/1505253004/diff/160001/webrtc/audio/audio_sink.h
File webrtc/audio/audio_sink.h (right):

https://codereview.chromium.org/1505253004/diff/160001/webrtc/audio/audio_sin...
webrtc/audio/audio_sink.h:20: class AudioSink {
I thought the style guide said interface classes must end in "Interface"?

https://codereview.chromium.org/1505253004/diff/160001/webrtc/audio/audio_sin...
webrtc/audio/audio_sink.h:37: int bits_per_sample = 16;    // Bits per sample
(currently always 16bit).
Either this should be an enum (float, int16, int32, interleave, deinterleave...)
or, more likely, we should just pass floats, always. Make bits_per_sample const
or hard code in your AudioSink->AudioTrackSinkInterface adapter.

https://codereview.chromium.org/1505253004/diff/160001/webrtc/voice_engine/ch...
File webrtc/voice_engine/channel_proxy.cc (right):

https://codereview.chromium.org/1505253004/diff/160001/webrtc/voice_engine/ch...
webrtc/voice_engine/channel_proxy.cc:139: void
ChannelProxy::SetSink(rtc::scoped_ptr<AudioSink> sink) {
Thread check please.

[tommi, 2015-12-11 14:10:20]

Rename AudioSink->AudioSinkInterface

[tommi, 2015-12-11 14:21:07]

Add thread check, Remove bits_per_sample and use int16_t.

[tommi (sloooow) - chröme, 2015-12-11 14:22:56]

tommi@chromium.org changed reviewers:
+ tommi@chromium.org

[tommi (sloooow) - chröme, 2015-12-11 14:22:57]

Renamed, added check and removed bits_per_sample.

I'm currently using Chrome to test and verify and would like to ask permission
to land the tests in webrtc in a separate CL? :D

https://codereview.chromium.org/1505253004/diff/160001/webrtc/audio/audio_sink.h
File webrtc/audio/audio_sink.h (right):

https://codereview.chromium.org/1505253004/diff/160001/webrtc/audio/audio_sin...
webrtc/audio/audio_sink.h:20: class AudioSink {
On 2015/12/11 13:52:01, the sun wrote:
> I thought the style guide said interface classes must end in "Interface"?

Done.

https://codereview.chromium.org/1505253004/diff/160001/webrtc/audio/audio_sin...
webrtc/audio/audio_sink.h:37: int bits_per_sample = 16;    // Bits per sample
(currently always 16bit).
On 2015/12/11 13:52:01, the sun wrote:
> Either this should be an enum (float, int16, int32, interleave,
deinterleave...)
> or, more likely, we should just pass floats, always. Make bits_per_sample
const
> or hard code in your AudioSink->AudioTrackSinkInterface adapter.

I'm mimicking what's being done in Chromium here but maybe I should just skip
that and have audio data fixed as int16_t* for now.  I agree that it would be
better to switch to floats and have a better way to represent the format of the
audio, but I think it's a little bit outside the scope of this cl.

https://codereview.chromium.org/1505253004/diff/160001/webrtc/voice_engine/ch...
File webrtc/voice_engine/channel_proxy.cc (right):

https://codereview.chromium.org/1505253004/diff/160001/webrtc/voice_engine/ch...
webrtc/voice_engine/channel_proxy.cc:139: void
ChannelProxy::SetSink(rtc::scoped_ptr<AudioSink> sink) {
On 2015/12/11 13:52:01, the sun wrote:
> Thread check please.

Done.

[tommi (sloooow) - chröme, 2015-12-11 14:26:15]

Here's the Chromium side CL btw:
https://codereview.chromium.org/1514143003/

[the sun, 2015-12-11 16:32:05]

Took a better look now, at 
talk/media
webrtc/

Before I give a thumbs up, fix the nits, de-fake the MockAudioProvider and, if
you are serious about adding 439 new lines of code without a single automated
test, add a link to the follow-up CL that pays off that technical debt. ;)

https://codereview.chromium.org/1505253004/diff/160001/webrtc/audio/audio_sink.h
File webrtc/audio/audio_sink.h (right):

https://codereview.chromium.org/1505253004/diff/160001/webrtc/audio/audio_sin...
webrtc/audio/audio_sink.h:37: int bits_per_sample = 16;    // Bits per sample
(currently always 16bit).
On 2015/12/11 14:22:57, tommi wrote:
> On 2015/12/11 13:52:01, the sun wrote:
> > Either this should be an enum (float, int16, int32, interleave,
> deinterleave...)
> > or, more likely, we should just pass floats, always. Make bits_per_sample
> const
> > or hard code in your AudioSink->AudioTrackSinkInterface adapter.
> 
> I'm mimicking what's being done in Chromium here but maybe I should just skip
> that and have audio data fixed as int16_t* for now.  I agree that it would be
> better to switch to floats and have a better way to represent the format of
the
> audio, but I think it's a little bit outside the scope of this cl.

Agreed.

https://codereview.chromium.org/1505253004/diff/200001/talk/app/webrtc/remote...
File talk/app/webrtc/remoteaudiosource.h (right):

https://codereview.chromium.org/1505253004/diff/200001/talk/app/webrtc/remote...
talk/app/webrtc/remoteaudiosource.h:87: rtc::CriticalSection sink_lock_;
Add thread checkers to document the expected usage by different threads here.

https://codereview.chromium.org/1505253004/diff/200001/talk/app/webrtc/rtpsen...
File talk/app/webrtc/rtpsenderreceiver_unittest.cc (right):

https://codereview.chromium.org/1505253004/diff/200001/talk/app/webrtc/rtpsen...
talk/app/webrtc/rtpsenderreceiver_unittest.cc:74: void SetRawAudioSink(uint32_t,
No. You just turned the mock into a fake. Please add a MOCK_METHOD2 for
SetRawAudioSink and make the appropriate ON_CALL or EXPECT_CALL setup in the
test.

https://codereview.chromium.org/1505253004/diff/200001/talk/app/webrtc/webrtc...
File talk/app/webrtc/webrtcsession.h (right):

https://codereview.chromium.org/1505253004/diff/200001/talk/app/webrtc/webrtc...
talk/app/webrtc/webrtcsession.h:45: #include "webrtc/base/thread_checker.h"
AFAICT no thread checker is used here?

https://codereview.chromium.org/1505253004/diff/200001/talk/media/base/fakeme...
File talk/media/base/fakemediaengine.h (right):

https://codereview.chromium.org/1505253004/diff/200001/talk/media/base/fakeme...
talk/media/base/fakemediaengine.h:38: #include "talk/media/base/audiorenderer.h"
one too many

https://codereview.chromium.org/1505253004/diff/200001/talk/media/base/fakeme...
talk/media/base/fakemediaengine.h:353: sink_ = std::move(sink);
If you're doing tests in a separate CL, can you skip adding this infrastructure
until then?

https://codereview.chromium.org/1505253004/diff/200001/talk/media/base/mediac...
File talk/media/base/mediachannel.h (right):

https://codereview.chromium.org/1505253004/diff/200001/talk/media/base/mediac...
talk/media/base/mediachannel.h:38: #include "webrtc/audio/audio_sink.h"
forward declare instead?

https://codereview.chromium.org/1505253004/diff/200001/talk/media/webrtc/fake...
File talk/media/webrtc/fakewebrtccall.h (right):

https://codereview.chromium.org/1505253004/diff/200001/talk/media/webrtc/fake...
talk/media/webrtc/fakewebrtccall.h:114:
rtc::scoped_ptr<webrtc::AudioSinkInterface> sink_;
again, I'd rather not add any extra gunk unless it is used or you have a CL
already in the pipe to use it.

https://codereview.chromium.org/1505253004/diff/200001/talk/media/webrtc/webr...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.chromium.org/1505253004/diff/200001/talk/media/webrtc/webr...
talk/media/webrtc/webrtcvoiceengine.cc:2420:
RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
Nit: add
LOG(LS_VERBOSE) << "WebRtcVoiceMediaChannel::SetRawAudioSink";
after the thread check.

https://codereview.chromium.org/1505253004/diff/200001/talk/session/media/cha...
File talk/session/media/channel.h (right):

https://codereview.chromium.org/1505253004/diff/200001/talk/session/media/cha...
talk/session/media/channel.h:371: rtc::scoped_ptr<webrtc::AudioSinkInterface>
sink);
This file should forward declare AudioSinkInterface, keeping with "include what
you use"

https://codereview.chromium.org/1505253004/diff/200001/webrtc/audio_receive_s...
File webrtc/audio_receive_stream.h (right):

https://codereview.chromium.org/1505253004/diff/200001/webrtc/audio_receive_s...
webrtc/audio_receive_stream.h:106: // Sets an audio sync that receives unmixed
audio from the receive stream.
s/sync/sink

I think it is worth to mention that audio must still be pulled through the
AudioTransport interface and is only tapped here as a side effect.

[the sun, 2015-12-11 16:33:23]

On 2015/12/11 16:32:05, the sun wrote:
> Took a better look now, at 
> talk/media
> webrtc/
> 
> Before I give a thumbs up, fix the nits, de-fake the MockAudioProvider and, if
> you are serious about adding 439 new lines of code without a single automated
> test, add a link to the follow-up CL that pays off that technical debt. ;)

To avoid any misunderstanding, I don't expect the CL to be done, just that it
has been started on.

>
https://codereview.chromium.org/1505253004/diff/160001/webrtc/audio/audio_sink.h
> File webrtc/audio/audio_sink.h (right):
> 
>
https://codereview.chromium.org/1505253004/diff/160001/webrtc/audio/audio_sin...
> webrtc/audio/audio_sink.h:37: int bits_per_sample = 16;    // Bits per sample
> (currently always 16bit).
> On 2015/12/11 14:22:57, tommi wrote:
> > On 2015/12/11 13:52:01, the sun wrote:
> > > Either this should be an enum (float, int16, int32, interleave,
> > deinterleave...)
> > > or, more likely, we should just pass floats, always. Make bits_per_sample
> > const
> > > or hard code in your AudioSink->AudioTrackSinkInterface adapter.
> > 
> > I'm mimicking what's being done in Chromium here but maybe I should just
skip
> > that and have audio data fixed as int16_t* for now.  I agree that it would
be
> > better to switch to floats and have a better way to represent the format of
> the
> > audio, but I think it's a little bit outside the scope of this cl.
> 
> Agreed.
> 
>
https://codereview.chromium.org/1505253004/diff/200001/talk/app/webrtc/remote...
> File talk/app/webrtc/remoteaudiosource.h (right):
> 
>
https://codereview.chromium.org/1505253004/diff/200001/talk/app/webrtc/remote...
> talk/app/webrtc/remoteaudiosource.h:87: rtc::CriticalSection sink_lock_;
> Add thread checkers to document the expected usage by different threads here.
> 
>
https://codereview.chromium.org/1505253004/diff/200001/talk/app/webrtc/rtpsen...
> File talk/app/webrtc/rtpsenderreceiver_unittest.cc (right):
> 
>
https://codereview.chromium.org/1505253004/diff/200001/talk/app/webrtc/rtpsen...
> talk/app/webrtc/rtpsenderreceiver_unittest.cc:74: void
SetRawAudioSink(uint32_t,
> No. You just turned the mock into a fake. Please add a MOCK_METHOD2 for
> SetRawAudioSink and make the appropriate ON_CALL or EXPECT_CALL setup in the
> test.
> 
>
https://codereview.chromium.org/1505253004/diff/200001/talk/app/webrtc/webrtc...
> File talk/app/webrtc/webrtcsession.h (right):
> 
>
https://codereview.chromium.org/1505253004/diff/200001/talk/app/webrtc/webrtc...
> talk/app/webrtc/webrtcsession.h:45: #include "webrtc/base/thread_checker.h"
> AFAICT no thread checker is used here?
> 
>
https://codereview.chromium.org/1505253004/diff/200001/talk/media/base/fakeme...
> File talk/media/base/fakemediaengine.h (right):
> 
>
https://codereview.chromium.org/1505253004/diff/200001/talk/media/base/fakeme...
> talk/media/base/fakemediaengine.h:38: #include
"talk/media/base/audiorenderer.h"
> one too many
> 
>
https://codereview.chromium.org/1505253004/diff/200001/talk/media/base/fakeme...
> talk/media/base/fakemediaengine.h:353: sink_ = std::move(sink);
> If you're doing tests in a separate CL, can you skip adding this
infrastructure
> until then?
> 
>
https://codereview.chromium.org/1505253004/diff/200001/talk/media/base/mediac...
> File talk/media/base/mediachannel.h (right):
> 
>
https://codereview.chromium.org/1505253004/diff/200001/talk/media/base/mediac...
> talk/media/base/mediachannel.h:38: #include "webrtc/audio/audio_sink.h"
> forward declare instead?
> 
>
https://codereview.chromium.org/1505253004/diff/200001/talk/media/webrtc/fake...
> File talk/media/webrtc/fakewebrtccall.h (right):
> 
>
https://codereview.chromium.org/1505253004/diff/200001/talk/media/webrtc/fake...
> talk/media/webrtc/fakewebrtccall.h:114:
> rtc::scoped_ptr<webrtc::AudioSinkInterface> sink_;
> again, I'd rather not add any extra gunk unless it is used or you have a CL
> already in the pipe to use it.
> 
>
https://codereview.chromium.org/1505253004/diff/200001/talk/media/webrtc/webr...
> File talk/media/webrtc/webrtcvoiceengine.cc (right):
> 
>
https://codereview.chromium.org/1505253004/diff/200001/talk/media/webrtc/webr...
> talk/media/webrtc/webrtcvoiceengine.cc:2420:
> RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
> Nit: add
> LOG(LS_VERBOSE) << "WebRtcVoiceMediaChannel::SetRawAudioSink";
> after the thread check.
> 
>
https://codereview.chromium.org/1505253004/diff/200001/talk/session/media/cha...
> File talk/session/media/channel.h (right):
> 
>
https://codereview.chromium.org/1505253004/diff/200001/talk/session/media/cha...
> talk/session/media/channel.h:371: rtc::scoped_ptr<webrtc::AudioSinkInterface>
> sink);
> This file should forward declare AudioSinkInterface, keeping with "include
what
> you use"
> 
>
https://codereview.chromium.org/1505253004/diff/200001/webrtc/audio_receive_s...
> File webrtc/audio_receive_stream.h (right):
> 
>
https://codereview.chromium.org/1505253004/diff/200001/webrtc/audio_receive_s...
> webrtc/audio_receive_stream.h:106: // Sets an audio sync that receives unmixed
> audio from the receive stream.
> s/sync/sink
> 
> I think it is worth to mention that audio must still be pulled through the
> AudioTransport interface and is only tapped here as a side effect.

[tommi, 2015-12-11 16:58:23]

Fix format_macros.h on windows

[tommi (sloooow) - chröme, 2015-12-11 16:59:01]

The CQ bit was checked by tommi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-11 16:59:12]

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

[commit-bot: I haz the power, 2015-12-11 16:59:15]

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

[commit-bot: I haz the power, 2015-12-11 16:59:16]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[tommi (sloooow) - chröme, 2015-12-11 17:51:18]

https://codereview.chromium.org/1505253004/diff/200001/talk/app/webrtc/remote...
File talk/app/webrtc/remoteaudiosource.h (right):

https://codereview.chromium.org/1505253004/diff/200001/talk/app/webrtc/remote...
talk/app/webrtc/remoteaudiosource.h:87: rtc::CriticalSection sink_lock_;
On 2015/12/11 16:32:04, the sun wrote:
> Add thread checkers to document the expected usage by different threads here.

I'm using main_thread_ as my thread checker and there's a thread check in almost
every method. For the ones where there isn't, I'll either add one or add more
documentation.

https://codereview.chromium.org/1505253004/diff/200001/talk/app/webrtc/rtpsen...
File talk/app/webrtc/rtpsenderreceiver_unittest.cc (right):

https://codereview.chromium.org/1505253004/diff/200001/talk/app/webrtc/rtpsen...
talk/app/webrtc/rtpsenderreceiver_unittest.cc:74: void SetRawAudioSink(uint32_t,
On 2015/12/11 16:32:04, the sun wrote:
> No. You just turned the mock into a fake. Please add a MOCK_METHOD2 for
> SetRawAudioSink and make the appropriate ON_CALL or EXPECT_CALL setup in the
> test.

There is some functionality here though and it's the one that applies to all the
test below.  The lifetime guarantees that come with setting the sink is
basically what this does.  So, it depends on what 'fake' means, but this is not
a no-op.

The mock macros do currently not work with this type of owner passing, so using
MOCK_METHOD2 does not work.  However, I could add a new method that can be
mocked for the sake of doing something with the sink.  It would look something
like:

void SetRawAudioSink(uint32_t ssrc, rtc::scoped_ptr<AudioSinkInterface> sink)
override {
  SetRawAudioSink(ssrc, &sink);
}

MOCK_METHOD2(SetRawAudioSink, void(uint32_t ssrc,
rtc::scoped_ptr<AudioSinkInterface>* sink));

In order to take ownership of the object and move it into a local, some gtest
macro magic is required. I currently don't see any WithArg() handling of
arguments in this code and figured it would make it unnecessarily complex.

So, in this case all the tests need the same functionality, MOCK doesn't work,
jumping through several gtest hoops is required to get the same functionality as
MockAudioProvider provides now (lifetime guarantees), so I took the easy way
out.

https://codereview.chromium.org/1505253004/diff/200001/talk/app/webrtc/webrtc...
File talk/app/webrtc/webrtcsession.h (right):

https://codereview.chromium.org/1505253004/diff/200001/talk/app/webrtc/webrtc...
talk/app/webrtc/webrtcsession.h:45: #include "webrtc/base/thread_checker.h"
On 2015/12/11 16:32:04, the sun wrote:
> AFAICT no thread checker is used here?

Removed

https://codereview.chromium.org/1505253004/diff/200001/talk/media/base/fakeme...
File talk/media/base/fakemediaengine.h (right):

https://codereview.chromium.org/1505253004/diff/200001/talk/media/base/fakeme...
talk/media/base/fakemediaengine.h:38: #include "talk/media/base/audiorenderer.h"
On 2015/12/11 16:32:04, the sun wrote:
> one too many

Acknowledged.

https://codereview.chromium.org/1505253004/diff/200001/talk/media/base/fakeme...
talk/media/base/fakemediaengine.h:353: sink_ = std::move(sink);
On 2015/12/11 16:32:04, the sun wrote:
> If you're doing tests in a separate CL, can you skip adding this
infrastructure
> until then?

This runs in existing tests.  The code isn't _completely_ without test coverage
:)  Anything that tests a remote track, runs through most of these layers. 
Audio is even sent to the track.  However, there isn't a test that tests the
libjingle interfaces (basically verifying that audio comes out of
AudioTrackSinkInterface).

https://codereview.chromium.org/1505253004/diff/200001/talk/media/base/mediac...
File talk/media/base/mediachannel.h (right):

https://codereview.chromium.org/1505253004/diff/200001/talk/media/base/mediac...
talk/media/base/mediachannel.h:34: #include "talk/media/base/audiorenderer.h"
also reverted this change

https://codereview.chromium.org/1505253004/diff/200001/talk/media/base/mediac...
talk/media/base/mediachannel.h:38: #include "webrtc/audio/audio_sink.h"
On 2015/12/11 16:32:04, the sun wrote:
> forward declare instead?

Done.
(be warned that about 10 more file were added to the CL with a +1 line)

https://codereview.chromium.org/1505253004/diff/200001/talk/media/webrtc/fake...
File talk/media/webrtc/fakewebrtccall.h (right):

https://codereview.chromium.org/1505253004/diff/200001/talk/media/webrtc/fake...
talk/media/webrtc/fakewebrtccall.h:114:
rtc::scoped_ptr<webrtc::AudioSinkInterface> sink_;
On 2015/12/11 16:32:04, the sun wrote:
> again, I'd rather not add any extra gunk unless it is used or you have a CL
> already in the pipe to use it.

Already used in existing tests.  Before, there was a class, AudioTrack, which
was used for both local and remote tracks (albeit close to no functionality for
the remote case).  I've split that up into AudioTrack (which continues to do
very little) and RemoteAudioTrack, which does a lot more.  All tests that
exercise the 'remote audio track' code path, do run this code, and it's several
tests.

https://codereview.chromium.org/1505253004/diff/200001/talk/media/webrtc/webr...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.chromium.org/1505253004/diff/200001/talk/media/webrtc/webr...
talk/media/webrtc/webrtcvoiceengine.cc:2420:
RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
On 2015/12/11 16:32:05, the sun wrote:
> Nit: add
> LOG(LS_VERBOSE) << "WebRtcVoiceMediaChannel::SetRawAudioSink";
> after the thread check.

Done.

https://codereview.chromium.org/1505253004/diff/200001/talk/session/media/cha...
File talk/session/media/channel.h (right):

https://codereview.chromium.org/1505253004/diff/200001/talk/session/media/cha...
talk/session/media/channel.h:371: rtc::scoped_ptr<webrtc::AudioSinkInterface>
sink);
On 2015/12/11 16:32:05, the sun wrote:
> This file should forward declare AudioSinkInterface, keeping with "include
what
> you use"

Added fwd decl.  Just so that I'm not misunderstanding, is there an include that
shouldn't be in this file or is missing?

https://codereview.chromium.org/1505253004/diff/200001/webrtc/audio_receive_s...
File webrtc/audio_receive_stream.h (right):

https://codereview.chromium.org/1505253004/diff/200001/webrtc/audio_receive_s...
webrtc/audio_receive_stream.h:106: // Sets an audio sync that receives unmixed
audio from the receive stream.
On 2015/12/11 16:32:05, the sun wrote:
> s/sync/sink

Thanks.

> I think it is worth to mention that audio must still be pulled through the
> AudioTransport interface and is only tapped here as a side effect.

Yes good point. Added documentation.

[tommi (sloooow) - chröme, 2015-12-11 17:51:40]

tommi@chromium.org changed reviewers:
- tommi@chromium.org

[tommi, 2015-12-11 17:52:13]

Address comments

[tommi, 2015-12-11 17:52:39]

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

[commit-bot: I haz the power, 2015-12-11 17:52:46]

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

[the sun, 2015-12-11 19:46:29]

LGTM

A few nits still, and one misunderstanding about the forward decls that you
probably want to address.

Thanks for making the effort to future-proof the VoE/Call structure!

https://codereview.chromium.org/1505253004/diff/200001/talk/app/webrtc/remote...
File talk/app/webrtc/remoteaudiosource.h (right):

https://codereview.chromium.org/1505253004/diff/200001/talk/app/webrtc/remote...
talk/app/webrtc/remoteaudiosource.h:87: rtc::CriticalSection sink_lock_;
On 2015/12/11 17:51:18, tommi wrote:
> On 2015/12/11 16:32:04, the sun wrote:
> > Add thread checkers to document the expected usage by different threads
here.
> 
> I'm using main_thread_ as my thread checker and there's a thread check in
almost
> every method. For the ones where there isn't, I'll either add one or add more
> documentation.

Acknowledged.

https://codereview.chromium.org/1505253004/diff/200001/talk/app/webrtc/rtpsen...
File talk/app/webrtc/rtpsenderreceiver_unittest.cc (right):

https://codereview.chromium.org/1505253004/diff/200001/talk/app/webrtc/rtpsen...
talk/app/webrtc/rtpsenderreceiver_unittest.cc:74: void SetRawAudioSink(uint32_t,
On 2015/12/11 17:51:18, tommi wrote:
> On 2015/12/11 16:32:04, the sun wrote:
> > No. You just turned the mock into a fake. Please add a MOCK_METHOD2 for
> > SetRawAudioSink and make the appropriate ON_CALL or EXPECT_CALL setup in the
> > test.
> 
> There is some functionality here though and it's the one that applies to all
the
> test below.  The lifetime guarantees that come with setting the sink is
> basically what this does.  So, it depends on what 'fake' means, but this is
not
> a no-op.
> 
> The mock macros do currently not work with this type of owner passing, so
using
> MOCK_METHOD2 does not work.  However, I could add a new method that can be
> mocked for the sake of doing something with the sink.  It would look something
> like:
> 
> void SetRawAudioSink(uint32_t ssrc, rtc::scoped_ptr<AudioSinkInterface> sink)
> override {
>   SetRawAudioSink(ssrc, &sink);
> }
> 
> MOCK_METHOD2(SetRawAudioSink, void(uint32_t ssrc,
> rtc::scoped_ptr<AudioSinkInterface>* sink));
> 
> In order to take ownership of the object and move it into a local, some gtest
> macro magic is required. I currently don't see any WithArg() handling of
> arguments in this code and figured it would make it unnecessarily complex.
> 
> So, in this case all the tests need the same functionality, MOCK doesn't work,
> jumping through several gtest hoops is required to get the same functionality
as
> MockAudioProvider provides now (lifetime guarantees), so I took the easy way
> out.

Does it work to add in SetUp() or CreateAudioRtpSender something like:
    EXPECT_CALL(audio_provider_, SetRawAudioSink(kSsrc, _))
        .WillOnce(Invoke([this](uint32_t ssrc,
rtc::scoped_ptr<AudioSinkInterface> sink) {
          sink_ = std::move(sink);
        }));
and keep the sink_ in the test class? Is it a problem with how the scoped_ptr is
forwarded to the lambda?

If that doesn't work I guess your chosen solution is the best trade off.

https://codereview.chromium.org/1505253004/diff/200001/talk/media/base/fakeme...
File talk/media/base/fakemediaengine.h (right):

https://codereview.chromium.org/1505253004/diff/200001/talk/media/base/fakeme...
talk/media/base/fakemediaengine.h:353: sink_ = std::move(sink);
On 2015/12/11 17:51:18, tommi wrote:
> On 2015/12/11 16:32:04, the sun wrote:
> > If you're doing tests in a separate CL, can you skip adding this
> infrastructure
> > until then?
> 
> This runs in existing tests.  The code isn't _completely_ without test
coverage
> :)  Anything that tests a remote track, runs through most of these layers. 
> Audio is even sent to the track.  However, there isn't a test that tests the
> libjingle interfaces (basically verifying that audio comes out of
> AudioTrackSinkInterface).

Ah, thanks for explaining. I hadn't looked at the upper libjingle layers in too
much detail.

https://codereview.chromium.org/1505253004/diff/200001/talk/media/base/mediac...
File talk/media/base/mediachannel.h (right):

https://codereview.chromium.org/1505253004/diff/200001/talk/media/base/mediac...
talk/media/base/mediachannel.h:34: #include "talk/media/base/audiorenderer.h"
On 2015/12/11 17:51:18, tommi wrote:
> also reverted this change

Acknowledged.

https://codereview.chromium.org/1505253004/diff/200001/talk/media/base/mediac...
talk/media/base/mediachannel.h:38: #include "webrtc/audio/audio_sink.h"
On 2015/12/11 17:51:18, tommi wrote:
> On 2015/12/11 16:32:04, the sun wrote:
> > forward declare instead?
> 
> Done.
> (be warned that about 10 more file were added to the CL with a +1 line)

Ah, oh. Hmm. Well, that didn't turn out too well then. Is that because anyone
including a file declaring a interface with a by-value scoped_ptr will need to
know the full declaration of AudioSinkInterface? In that case, I guess this was
correct originally and should be changed back. Sorry.

The reason I brought it up was that you used a forward declaration in the
webrtc/audio_receive_stream.h, so I figured it would work and should be
consistent. Again, sorry, please revert.

https://codereview.chromium.org/1505253004/diff/200001/talk/session/media/cha...
File talk/session/media/channel.h (right):

https://codereview.chromium.org/1505253004/diff/200001/talk/session/media/cha...
talk/session/media/channel.h:371: rtc::scoped_ptr<webrtc::AudioSinkInterface>
sink);
On 2015/12/11 17:51:18, tommi wrote:
> On 2015/12/11 16:32:05, the sun wrote:
> > This file should forward declare AudioSinkInterface, keeping with "include
> what
> > you use"
> 
> Added fwd decl.  Just so that I'm not misunderstanding, is there an include
that
> shouldn't be in this file or is missing?

The inclusion of AudioSinkInterface must have been transient somehow.

https://codereview.chromium.org/1505253004/diff/240001/talk/app/webrtc/remote...
File talk/app/webrtc/remoteaudiosource.cc (right):

https://codereview.chromium.org/1505253004/diff/240001/talk/app/webrtc/remote...
talk/app/webrtc/remoteaudiosource.cc:43: MessageHandler(RemoteAudioSource*
source) : source_(source) {}
explicit

https://codereview.chromium.org/1505253004/diff/240001/talk/app/webrtc/remote...
talk/app/webrtc/remoteaudiosource.cc:54:
RTC_DISALLOW_COPY_AND_ASSIGN(MessageHandler);
DISALLOW_IMPLICIT...

https://codereview.chromium.org/1505253004/diff/240001/talk/app/webrtc/remote...
talk/app/webrtc/remoteaudiosource.cc:59: Sink(RemoteAudioSource* source) :
source_(source) {}
explicit

https://codereview.chromium.org/1505253004/diff/240001/talk/app/webrtc/remote...
talk/app/webrtc/remoteaudiosource.cc:68: const
rtc::scoped_refptr<RemoteAudioSource> source_;
DISALLOW_IMPLICIT...

https://codereview.chromium.org/1505253004/diff/240001/talk/app/webrtc/remote...
File talk/app/webrtc/remoteaudiotrack.cc (right):

https://codereview.chromium.org/1505253004/diff/240001/talk/app/webrtc/remote...
talk/app/webrtc/remoteaudiotrack.cc:58: default:
RTC_NOTREACHED()

https://codereview.chromium.org/1505253004/diff/240001/talk/app/webrtc/remote...
File talk/app/webrtc/remoteaudiotrack.h (right):

https://codereview.chromium.org/1505253004/diff/240001/talk/app/webrtc/remote...
talk/app/webrtc/remoteaudiotrack.h:71: const
rtc::scoped_refptr<RemoteAudioSource> audio_source_;
DISALLOW_IMPLICIT...

https://codereview.chromium.org/1505253004/diff/240001/webrtc/audio_receive_s...
File webrtc/audio_receive_stream.h (right):

https://codereview.chromium.org/1505253004/diff/240001/webrtc/audio_receive_s...
webrtc/audio_receive_stream.h:106: // Sets an audio sinc that receives unmixed
audio from the receive stream.
nit: a "sinc" in signal processing context is something quite different
(https://en.wikipedia.org/wiki/Sinc_function). :)

https://codereview.chromium.org/1505253004/diff/240001/webrtc/base/format_mac...
File webrtc/base/format_macros.h (right):

https://codereview.chromium.org/1505253004/diff/240001/webrtc/base/format_mac...
webrtc/base/format_macros.h:76: #include <inttypes.h>
hmmm, why?

[the sun, 2015-12-11 19:48:44]

[snip]
>
https://codereview.chromium.org/1505253004/diff/200001/talk/session/media/cha...
> talk/session/media/channel.h:371: rtc::scoped_ptr<webrtc::AudioSinkInterface>
> sink);
> On 2015/12/11 17:51:18, tommi wrote:
> > On 2015/12/11 16:32:05, the sun wrote:
> > > This file should forward declare AudioSinkInterface, keeping with "include
> > what
> > > you use"
> > 
> > Added fwd decl.  Just so that I'm not misunderstanding, is there an include
> that
> > shouldn't be in this file or is missing?
> 
> The inclusion of AudioSinkInterface must have been transient somehow.

transient->transitive

[commit-bot: I haz the power, 2015-12-11 19:53:00]

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

[commit-bot: I haz the power, 2015-12-11 19:53:00]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[tommi, 2015-12-11 22:34:14]

Address comments

[tommi, 2015-12-12 00:33:11]

https://codereview.chromium.org/1505253004/diff/200001/talk/media/base/mediac...
File talk/media/base/mediachannel.h (right):

https://codereview.chromium.org/1505253004/diff/200001/talk/media/base/mediac...
talk/media/base/mediachannel.h:38: #include "webrtc/audio/audio_sink.h"
On 2015/12/11 19:46:28, the sun wrote:
> On 2015/12/11 17:51:18, tommi wrote:
> > On 2015/12/11 16:32:04, the sun wrote:
> > > forward declare instead?
> > 
> > Done.
> > (be warned that about 10 more file were added to the CL with a +1 line)
> 
> Ah, oh. Hmm. Well, that didn't turn out too well then. Is that because anyone
> including a file declaring a interface with a by-value scoped_ptr will need to
> know the full declaration of AudioSinkInterface? In that case, I guess this
was
> correct originally and should be changed back. Sorry.
> 
> The reason I brought it up was that you used a forward declaration in the
> webrtc/audio_receive_stream.h, so I figured it would work and should be
> consistent. Again, sorry, please revert.

This didn't make sense to me either, so I went through the files again and
realized that there was a missing include in fakemediaengine.h.  All of the
implementation there is inline and I was sure I had added an include there, but
alas that was what was causing all of this.  So I've reverted the includes I
added but kept the fwd decl like you asked and also fixed other places where
there was no implementation in the header.

https://codereview.chromium.org/1505253004/diff/240001/talk/app/webrtc/remote...
File talk/app/webrtc/remoteaudiosource.cc (right):

https://codereview.chromium.org/1505253004/diff/240001/talk/app/webrtc/remote...
talk/app/webrtc/remoteaudiosource.cc:43: MessageHandler(RemoteAudioSource*
source) : source_(source) {}
On 2015/12/11 19:46:28, the sun wrote:
> explicit

Done.

https://codereview.chromium.org/1505253004/diff/240001/talk/app/webrtc/remote...
talk/app/webrtc/remoteaudiosource.cc:54:
RTC_DISALLOW_COPY_AND_ASSIGN(MessageHandler);
On 2015/12/11 19:46:29, the sun wrote:
> DISALLOW_IMPLICIT...

Done.

https://codereview.chromium.org/1505253004/diff/240001/talk/app/webrtc/remote...
talk/app/webrtc/remoteaudiosource.cc:59: Sink(RemoteAudioSource* source) :
source_(source) {}
On 2015/12/11 19:46:28, the sun wrote:
> explicit

Done.

https://codereview.chromium.org/1505253004/diff/240001/talk/app/webrtc/remote...
talk/app/webrtc/remoteaudiosource.cc:68: const
rtc::scoped_refptr<RemoteAudioSource> source_;
On 2015/12/11 19:46:28, the sun wrote:
> DISALLOW_IMPLICIT...

Done.

https://codereview.chromium.org/1505253004/diff/240001/talk/app/webrtc/remote...
File talk/app/webrtc/remoteaudiotrack.cc (right):

https://codereview.chromium.org/1505253004/diff/240001/talk/app/webrtc/remote...
talk/app/webrtc/remoteaudiotrack.cc:58: default:
On 2015/12/11 19:46:29, the sun wrote:
> RTC_NOTREACHED()

this is actually intended (i.e. kInitializing).  Added a comment to clarify.

https://codereview.chromium.org/1505253004/diff/240001/talk/app/webrtc/remote...
File talk/app/webrtc/remoteaudiotrack.h (right):

https://codereview.chromium.org/1505253004/diff/240001/talk/app/webrtc/remote...
talk/app/webrtc/remoteaudiotrack.h:71: const
rtc::scoped_refptr<RemoteAudioSource> audio_source_;
On 2015/12/11 19:46:29, the sun wrote:
> DISALLOW_IMPLICIT...

Done.

https://codereview.chromium.org/1505253004/diff/240001/webrtc/audio_receive_s...
File webrtc/audio_receive_stream.h (right):

https://codereview.chromium.org/1505253004/diff/240001/webrtc/audio_receive_s...
webrtc/audio_receive_stream.h:106: // Sets an audio sinc that receives unmixed
audio from the receive stream.
On 2015/12/11 19:46:29, the sun wrote:
> nit: a "sinc" in signal processing context is something quite different
> (https://en.wikipedia.org/wiki/Sinc_function). :)

Thanks! typo^2 :-/  started out typing 'sync' out of habit and ended up with
'sinc' after fixing only the 'y'.  Silver lining is that TIL ;)
Correct spelling is important.

https://codereview.chromium.org/1505253004/diff/240001/webrtc/base/format_mac...
File webrtc/base/format_macros.h (right):

https://codereview.chromium.org/1505253004/diff/240001/webrtc/base/format_mac...
webrtc/base/format_macros.h:76: #include <inttypes.h>
On 2015/12/11 19:46:29, the sun wrote:
> hmmm, why?

If you look a couple of patch sets back, you'll see the windows trybots turn red
and fail with errors like this:

e:\b\depot_tools\win_toolchain\vs2013_files\vc\include\inttypes.h(126) : warning
C4005: 'PRId64' : macro redefinition
        e:\b\build\slave\win\build\src\webrtc\base\format_macros.h(77) : see
previous definition of 'PRId64'

The reason is that there was a bug here that audio_sinks.h triggered.
audio_sinks.h includes #include <inttypes.h>, but this file was being included
via transmit_mixer.cc _before_ audio_sink.h got included.
That caused the below definitions to be defined and they conflict with the ones
in <inittypes.h>

If you look a few lines up, you can see that we already do this for WEBRTC_POSIX
and we should be doing it for Windows as well.
See e.g. the file in Chromium:
https://code.google.com/p/chromium/codesearch#chromium/src/base/format_macros...

Maybe it's time to update this file.

[tommi, 2015-12-12 00:33:29]

The CQ bit was checked by tommi@webrtc.org

[tommi, 2015-12-12 00:33:30]

The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org
Link to the patchset: https://codereview.chromium.org/1505253004/#ps250001
(title: "Address comments")

[commit-bot: I haz the power, 2015-12-12 00:33:37]

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

[commit-bot: I haz the power, 2015-12-12 00:34:39]

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

[commit-bot: I haz the power, 2015-12-12 00:34:40]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[commit-bot: I haz the power, 2015-12-12 00:37:18]

Description was changed from

==========
Support for unmixed remote audio into tracks.

BUG=chromium:121673
==========

to

==========
Support for unmixed remote audio into tracks.

BUG=chromium:121673
R=solenberg@webrtc.org

Committed: https://crrev.com/f888bb58da04c5095759b5ec7ce2e1fa2cd414fd
Cr-Commit-Position: refs/heads/master@{#10995}
==========

[tommi, 2015-12-12 00:37:19]

Description was changed from

==========
Support for unmixed remote audio into tracks.

BUG=chromium:121673
R=solenberg@webrtc.org

Committed: https://crrev.com/f888bb58da04c5095759b5ec7ce2e1fa2cd414fd
Cr-Commit-Position: refs/heads/master@{#10995}
==========

to

==========
Support for unmixed remote audio into tracks.

BUG=chromium:121673
R=solenberg@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/f888bb58da04c5095759b5ec7...
==========

[commit-bot: I haz the power, 2015-12-12 00:37:20]

Patchset 14 (id:??) landed as
https://crrev.com/f888bb58da04c5095759b5ec7ce2e1fa2cd414fd
Cr-Commit-Position: refs/heads/master@{#10995}

[tommi, 2015-12-12 00:37:21]

Committed patchset #14 (id:250001) manually as
f888bb58da04c5095759b5ec7ce2e1fa2cd414fd (presubmit successful).

[juberti, 2015-12-12 00:43:08]

juberti@google.com changed reviewers:
+ juberti@google.com

[juberti, 2015-12-12 00:43:09]

https://codereview.webrtc.org/1505253004/diff/250001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.h (right):

https://codereview.webrtc.org/1505253004/diff/250001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.h:253: void SetRawAudioSink(uint32_t ssrc,
Is there a reason why this doesn't use cricket::AudioRenderer (similar to
SetAudioSend above and SetVideoPlayout below)? It would be nice to maintain
symmetry between audio and video.

[tommi, 2015-12-12 01:09:11]

https://codereview.webrtc.org/1505253004/diff/250001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.h (right):

https://codereview.webrtc.org/1505253004/diff/250001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.h:253: void SetRawAudioSink(uint32_t ssrc,
On 2015/12/12 00:43:09, juberti wrote:
> Is there a reason why this doesn't use cricket::AudioRenderer (similar to
> SetAudioSend above and SetVideoPlayout below)? It would be nice to maintain
> symmetry between audio and video.

I started out with that but Fredrik and I ended up with this approach.  Here are
some of the reasons:

* There are then three layers of sinks.  AudioTrackSinkInterface,
AudioRenderer::Sink and the sink inside of WebRTC, I was using VoEMediaProcess. 
Now there are two layers (internal/external)

* Fredrik is refactoring the voice engine and the last interface may well be
going away.  So there's a desire to have one that replaces it and ideally has
some of the things we haven't had, such as timestamp.  So, we decided to add
this new sink and leave the other alone.  Since it's a part of the new
sanctioned voe code, it makes sense to use it from the new libjingle code
(mediastreamprovider.h) which is already using related types such as
AudioReceiveStream.

* Lifetime management is slightly less confusing with the current approach. 
Since the provider and consumer of the audio data will have different lifetimes,
they need to both be aware of each other and either can terminate the sink. 
AudioRenderer::Sink has an OnClose method for the provider to notify the
consumer and the consumer would typically disconnect a sink by calling
AudioRenderer->SetSink(nullptr) or comparable method, to clear it. This kind of
worked.  That is until I realized I had layers of implementations of SetSink and
all of the implementations of SetSink, had different lifetimes.  So the source
might have a pointer to a WebRtcSession (which implements
AudioProviderInterface) but the the actual sink pointer could be sent down
further layers to a different object that actually provides the data and has a
longer lifetime than the session object.  This would cause OnClose() to be
called at the right time, but when the session died, there was no way of knowing
it for the source object, so if it tried to terminate the sink, there would be a
UAF.
  So I decided that it would be better to adapt a model where holding a pointer
back to the provider wasn't necessary and that the way a sink would be notified
that data will no longer be provided, would be by deletion of the sink.  This
forces the consumer to have a separate sink implementation which takes care of
the notifications for both sides and its implementation is ultimately up to the
consumer.
  OnClose() isn't necessary in that model so I didn't see much point in using
AudioRenderer::Sink differently than it is intended to be used and also outside
of the context of AudioRenderer.

* AudioRenderer is in cricket which adds one more namespace into the mix (there
are one or two other types from cricket in the PeerConnection API but the intent
is to migrate types into webrtc/rtc).

* There may be more properties that will accompany the audio data in the future,
so we opted for a struct to hold the data for slightly improved future
compatibility instead of changing function prototypes.

[perkj_webrtc, 2015-12-13 19:26:48]

lgtm with nits.

https://codereview.webrtc.org/1505253004/diff/140001/talk/media/base/fakemedi...
File talk/media/base/fakemediaengine.h (right):

https://codereview.webrtc.org/1505253004/diff/140001/talk/media/base/fakemedi...
talk/media/base/fakemediaengine.h:38: #include "talk/media/base/audiorenderer.h"
nit: remove extra

https://codereview.webrtc.org/1505253004/diff/140001/talk/media/base/mediacha...
File talk/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1505253004/diff/140001/talk/media/base/mediacha...
talk/media/base/mediachannel.h:34: #include "talk/media/base/audiorenderer.h"
nit , no need since it is still used as a pointer.

https://codereview.webrtc.org/1505253004/diff/250001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1505253004/diff/250001/talk/session/media/chann...
talk/session/media/channel.cc:48: bool SetRawAudioSink_w(VoiceMediaChannel*
channel,
nit: Since this file contain implementation for audio, video and data I would
have prefered this to be implemented as VoiceChannel::SetRawAudioSink_w just to
keep the relevant code close to each other.

[tommi, 2015-12-13 19:46:34]

see https://codereview.webrtc.org/1523603002 for followup

https://codereview.webrtc.org/1505253004/diff/140001/talk/media/base/fakemedi...
File talk/media/base/fakemediaengine.h (right):

https://codereview.webrtc.org/1505253004/diff/140001/talk/media/base/fakemedi...
talk/media/base/fakemediaengine.h:38: #include "talk/media/base/audiorenderer.h"
On 2015/12/13 19:26:47, perkj1 wrote:
> nit: remove extra

Done in the last patch set

https://codereview.webrtc.org/1505253004/diff/140001/talk/media/base/mediacha...
File talk/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1505253004/diff/140001/talk/media/base/mediacha...
talk/media/base/mediachannel.h:34: #include "talk/media/base/audiorenderer.h"
On 2015/12/13 19:26:48, perkj1 wrote:
> nit , no need since it is still used as a pointer.  

Done.

https://codereview.webrtc.org/1505253004/diff/250001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1505253004/diff/250001/talk/session/media/chann...
talk/session/media/channel.cc:48: bool SetRawAudioSink_w(VoiceMediaChannel*
channel,
On 2015/12/13 19:26:48, perkj1 wrote:
> nit: Since this file contain implementation for audio, video and data I would
> have prefered this to be implemented as VoiceChannel::SetRawAudioSink_w just
to
> keep the relevant code close to each other.

As is it's not a member function, so I figured I wouldn't expose it in the
header and it also requires less code.  I agree that it would be a good idea to
split voice, video and data up into separate files.