Add generic interfaces for the sinks of the media stream audio track

Add generic interfaces for the sinks of the media stream audio track to get the audio data, and this patch also adds interfaces of AddToAudioTrack and RemoveFromAudioTrack to connect the sink to the audio track.


BUG=322547
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238503

[no longer working on chromium, 2013-11-27 13:36:20]

Guys, it is the sister CL to add the generic interfaces for sinks of audio
track.

I am hoping to land this before my vocation, please take a look.

SX

https://codereview.chromium.org/90743004/diff/1/content/content_renderer.gypi
File content/content_renderer.gypi (right):

https://codereview.chromium.org/90743004/diff/1/content/content_renderer.gypi...
content/content_renderer.gypi:583: 'public/renderer/media_stream_audio_sink.h'
Note, I put them under enable_webrtc==1 otherwise it won't compile because some
of the dependencies are under the flag.

We will try getting rid of the "enable_webrtc" flag for getUserMedia as
follow-ups.

https://codereview.chromium.org/90743004/diff/1/content/public/renderer/media...
File content/public/renderer/media_stream_audio_sink.h (right):

https://codereview.chromium.org/90743004/diff/1/content/public/renderer/media...
content/public/renderer/media_stream_audio_sink.h:14: class CONTENT_EXPORT
MediaStreamAudioSink {
it will inherit MediaStreamSink after Per lands his CL.

https://codereview.chromium.org/90743004/diff/1/content/public/renderer/media...
content/public/renderer/media_stream_audio_sink.h:30: // TODO(xians): Remove
those WebRtc specific variables after moving the
I hope these comments and TODO can explain why we need to keep these webrtc
specific variables for the short term.
And we are working hard to move the APM to chrome now, after this is done, the
callback interface will be much cleaned.

[perkj_chrome, 2013-11-27 14:56:39]

Discussed off line. I suggest this cl adds the sink interface that only include
things that we want it to include and change the current audio track
implementation to accept two sink interfaces, this new one , and the old one.

An experimental cl for this has already been created in another context.

https://codereview.chromium.org/90743004/diff/1/content/public/renderer/media...
File content/public/renderer/media_stream_audio_sink.h (right):

https://codereview.chromium.org/90743004/diff/1/content/public/renderer/media...
content/public/renderer/media_stream_audio_sink.h:20: // |number_of_frames| is
the number of audio freams in the buffer.
spelling

[no longer working on chromium, 2013-11-27 16:27:56]

Hey, Per gave a good proposal to hide those peer connection specific variables
under the sink owner, this can give us a much cleaner sink interface.

I have not got enough time to implement the AddtoAudioTrack and
RemoveFromAudioTrack, they will be addressed either in a follow-up version or
separate CL (if I have time)

PTAL.

SX

https://codereview.chromium.org/90743004/diff/1/content/public/renderer/media...
File content/public/renderer/media_stream_audio_sink.h (right):

https://codereview.chromium.org/90743004/diff/1/content/public/renderer/media...
content/public/renderer/media_stream_audio_sink.h:20: // |number_of_frames| is
the number of audio freams in the buffer.
On 2013/11/27 14:56:40, perkj wrote:
> spelling

Done.

[no longer working on chromium, 2013-11-27 16:32:52]

+ Tommi.

[Jói, 2013-11-27 17:35:01]

Cool stuff.

https://codereview.chromium.org/90743004/diff/60001/content/public/renderer/m...
File content/public/renderer/media_stream_audio_sink.h (right):

https://codereview.chromium.org/90743004/diff/60001/content/public/renderer/m...
content/public/renderer/media_stream_audio_sink.h:25: // Callback on delivering
the audio interleaved data.
audio interleaved data -> interleaved audio data
(unless "audio interleaved data" is what's commonly used)

https://codereview.chromium.org/90743004/diff/60001/content/public/renderer/m...
content/public/renderer/media_stream_audio_sink.h:27: // |sample_rate| is the
sample frequency of audio data.
audio data -> |audio_data|

https://codereview.chromium.org/90743004/diff/60001/content/public/renderer/m...
content/public/renderer/media_stream_audio_sink.h:28: // |number_of_channels| is
the number of channels reflecting the order of
"reflecting the order of surround sound channels" seems a bit vague, what does
it mean?

https://codereview.chromium.org/90743004/diff/60001/content/public/renderer/m...
content/public/renderer/media_stream_audio_sink.h:30: // |number_of_frames| is
the number of audio frames in the buffer.
buffer -> |audio_data|

https://codereview.chromium.org/90743004/diff/60001/content/public/renderer/m...
content/public/renderer/media_stream_audio_sink.h:36: // Callback called when
the forma of the audio stream has changed, and
forma -> format

Suggest period after "changed" then start a new sentence: This is called on the
same thread as OnData().

https://codereview.chromium.org/90743004/diff/60001/content/public/renderer/m...
content/public/renderer/media_stream_audio_sink.h:44: CONTENT_EXPORT void
AddToAudioTrack(MediaStreamAudioSink* sink,
nit: For consistency with RemoveFromAudioTrack, suggest breaking the line the
same here as is done there.

https://codereview.chromium.org/90743004/diff/60001/content/renderer/media/me...
File content/renderer/media/media_stream_audio_sink_owner.cc (right):

https://codereview.chromium.org/90743004/diff/60001/content/renderer/media/me...
content/renderer/media/media_stream_audio_sink_owner.cc:68: bool
MediaStreamAudioSinkOwner::IsEqual(
This will return true if |other| is NULL and media_stream_delegate_ is NULL,
while peer_connection_delegate_ might be the one being used and might be
non-NULL.

Same thing for the other IsEqual implementation.

You need to special case the |other == NULL| case and only return true if both
media_stream_delegate_ and peer_connection_delegate_ are NULL.

https://codereview.chromium.org/90743004/diff/60001/content/renderer/media/me...
File content/renderer/media/media_stream_audio_sink_owner.h (right):

https://codereview.chromium.org/90743004/diff/60001/content/renderer/media/me...
content/renderer/media/media_stream_audio_sink_owner.h:18: // Reference counted
container of MediaStreamAudioSink delegates.
It might be worth mentioning that this is a temporary type while we still have
two different types of sinks?

Also, "container of MediaStreamAudioSink delegates" seems inaccurate since this
is for either that type or for PeerConnectionAudioSink. Perhaps "holder of audio
stream sinks"?

https://codereview.chromium.org/90743004/diff/60001/content/renderer/media/me...
content/renderer/media/media_stream_audio_sink_owner.h:42: // Wrapper which
allows to use std::find_if() when adding and removing
Would these wrappers be needed if we simply had two overloads of operator()
directly on MediaStreamAudioSinkOwner?

[tommi (sloooow) - chröme, 2013-11-28 09:12:10]

I maybe a little bit out of the loop with regards to having two pointers in the
same sink class but documentation should clear that up.
oh and one nit: s/vocation/vacation - unless you really mean you'll be working
:)

https://codereview.chromium.org/90743004/diff/60001/content/public/renderer/m...
File content/public/renderer/media_stream_audio_sink.cc (right):

https://codereview.chromium.org/90743004/diff/60001/content/public/renderer/m...
content/public/renderer/media_stream_audio_sink.cc:19: // TODO(xians): Implement
this.
NOTIMPLEMENTED?

https://codereview.chromium.org/90743004/diff/60001/content/renderer/media/me...
File content/renderer/media/media_stream_audio_sink_owner.h (right):

https://codereview.chromium.org/90743004/diff/60001/content/renderer/media/me...
content/renderer/media/media_stream_audio_sink_owner.h:67: MediaStreamAudioSink*
media_stream_delegate_;
document ownership and how it's guaranteed that these pointers will never be
dangling.

https://codereview.chromium.org/90743004/diff/60001/content/renderer/media/me...
content/renderer/media/media_stream_audio_sink_owner.h:68:
PeerConnectionAudioSink* peer_connection_delegate_;
why do we have these two mutually exclusive pointers in the same class?

[no longer working on chromium, 2013-11-28 17:27:17]

HI Guys,

I have already addressed all the comments, implemented the AddToAudioTrack and
removeFromAudioTrack, and separate the PeerConnection sink with media stream
sink on the sink owner class.

PTAL.
SX

https://codereview.chromium.org/90743004/diff/60001/content/public/renderer/m...
File content/public/renderer/media_stream_audio_sink.cc (right):

https://codereview.chromium.org/90743004/diff/60001/content/public/renderer/m...
content/public/renderer/media_stream_audio_sink.cc:19: // TODO(xians): Implement
this.
On 2013/11/28 09:12:11, tommi wrote:
> NOTIMPLEMENTED?

Implemented now.

https://codereview.chromium.org/90743004/diff/60001/content/public/renderer/m...
File content/public/renderer/media_stream_audio_sink.h (right):

https://codereview.chromium.org/90743004/diff/60001/content/public/renderer/m...
content/public/renderer/media_stream_audio_sink.h:25: // Callback on delivering
the audio interleaved data.
On 2013/11/27 17:35:02, Jói wrote:
> audio interleaved data -> interleaved audio data
> (unless "audio interleaved data" is what's commonly used)

Done.

https://codereview.chromium.org/90743004/diff/60001/content/public/renderer/m...
content/public/renderer/media_stream_audio_sink.h:27: // |sample_rate| is the
sample frequency of audio data.
On 2013/11/27 17:35:02, Jói wrote:
> audio data -> |audio_data|

Done.

https://codereview.chromium.org/90743004/diff/60001/content/public/renderer/m...
content/public/renderer/media_stream_audio_sink.h:27: // |sample_rate| is the
sample frequency of audio data.
On 2013/11/27 17:35:02, Jói wrote:
> audio data -> |audio_data|

Done.

https://codereview.chromium.org/90743004/diff/60001/content/public/renderer/m...
content/public/renderer/media_stream_audio_sink.h:28: // |number_of_channels| is
the number of channels reflecting the order of
On 2013/11/27 17:35:02, Jói wrote:
> "reflecting the order of surround sound channels" seems a bit vague, what does
> it mean?

OK, I just changed it to is the number of audio channels of |audio_data|. hope
it is more clear.

https://codereview.chromium.org/90743004/diff/60001/content/public/renderer/m...
content/public/renderer/media_stream_audio_sink.h:30: // |number_of_frames| is
the number of audio frames in the buffer.
On 2013/11/27 17:35:02, Jói wrote:
> buffer -> |audio_data|

Done.

https://codereview.chromium.org/90743004/diff/60001/content/public/renderer/m...
content/public/renderer/media_stream_audio_sink.h:36: // Callback called when
the forma of the audio stream has changed, and
On 2013/11/27 17:35:02, Jói wrote:
> forma -> format
> 
> Suggest period after "changed" then start a new sentence: This is called on
the
> same thread as OnData().

Done.

https://codereview.chromium.org/90743004/diff/60001/content/public/renderer/m...
content/public/renderer/media_stream_audio_sink.h:44: CONTENT_EXPORT void
AddToAudioTrack(MediaStreamAudioSink* sink,
On 2013/11/27 17:35:02, Jói wrote:
> nit: For consistency with RemoveFromAudioTrack, suggest breaking the line the
> same here as is done there.

Done.

https://codereview.chromium.org/90743004/diff/60001/content/renderer/media/me...
File content/renderer/media/media_stream_audio_sink_owner.cc (right):

https://codereview.chromium.org/90743004/diff/60001/content/renderer/media/me...
content/renderer/media/media_stream_audio_sink_owner.cc:68: bool
MediaStreamAudioSinkOwner::IsEqual(
On 2013/11/27 17:35:02, Jói wrote:
> This will return true if |other| is NULL and media_stream_delegate_ is NULL,
> while peer_connection_delegate_ might be the one being used and might be
> non-NULL.
> 
> Same thing for the other IsEqual implementation.
> 
> You need to special case the |other == NULL| case and only return true if both
> media_stream_delegate_ and peer_connection_delegate_ are NULL.

Added a DCHECK(other);

https://codereview.chromium.org/90743004/diff/60001/content/renderer/media/me...
File content/renderer/media/media_stream_audio_sink_owner.h (right):

https://codereview.chromium.org/90743004/diff/60001/content/renderer/media/me...
content/renderer/media/media_stream_audio_sink_owner.h:18: // Reference counted
container of MediaStreamAudioSink delegates.
On 2013/11/27 17:35:02, Jói wrote:
> It might be worth mentioning that this is a temporary type while we still have
> two different types of sinks?
> 

Added a TODO to remind removing the PeerConnectionAudioSink code.

> Also, "container of MediaStreamAudioSink delegates" seems inaccurate since
this
> is for either that type or for PeerConnectionAudioSink. Perhaps "holder of
audio
> stream sinks"?

Done.

https://codereview.chromium.org/90743004/diff/60001/content/renderer/media/me...
content/renderer/media/media_stream_audio_sink_owner.h:42: // Wrapper which
allows to use std::find_if() when adding and removing
On 2013/11/27 17:35:02, Jói wrote:
> Would these wrappers be needed if we simply had two overloads of operator()
> directly on MediaStreamAudioSinkOwner?

It is a very interesting idea.
I guess you mean adding 
bool operator()(const scoped_refptr<MediaStreamAudioSinkOwner>& owner) {
  if (media_stream_delegate_)
    return owner->IsEqual(media_stream_delegate_);
  if (peer_connection_delegate_)
    return owner->IsEqual(peer_connection_delegate_);
}

The only problem for that is that MediaStreamAudioSinkOwner is reference
counted, so it won't work.

https://codereview.chromium.org/90743004/diff/60001/content/renderer/media/me...
content/renderer/media/media_stream_audio_sink_owner.h:67: MediaStreamAudioSink*
media_stream_delegate_;
On 2013/11/28 09:12:11, tommi wrote:
> document ownership and how it's guaranteed that these pointers will never be
> dangling.

Done.

https://codereview.chromium.org/90743004/diff/60001/content/renderer/media/me...
content/renderer/media/media_stream_audio_sink_owner.h:68:
PeerConnectionAudioSink* peer_connection_delegate_;
On 2013/11/28 09:12:11, tommi wrote:
> why do we have these two mutually exclusive pointers in the same class?

Done with separate the media stream sink owner and peer connection sink owner

[no longer working on chromium, 2013-11-28 19:22:21]

https://codereview.chromium.org/90743004/diff/60001/content/renderer/media/me...
File content/renderer/media/media_stream_audio_sink_owner.cc (right):

https://codereview.chromium.org/90743004/diff/60001/content/renderer/media/me...
content/renderer/media/media_stream_audio_sink_owner.cc:68: bool
MediaStreamAudioSinkOwner::IsEqual(
On 2013/11/28 17:27:18, xians1 wrote:
> On 2013/11/27 17:35:02, Jói wrote:
> > This will return true if |other| is NULL and media_stream_delegate_ is NULL,
> > while peer_connection_delegate_ might be the one being used and might be
> > non-NULL.
> > 
> > Same thing for the other IsEqual implementation.
> > 
> > You need to special case the |other == NULL| case and only return true if
both
> > media_stream_delegate_ and peer_connection_delegate_ are NULL.
> 
> Added a DCHECK(other);


I just realized that I forgot to commit the local change on adding DCHECK here,
but I will do it tomorrow after I get back to the office.

[tommi (sloooow) - chröme, 2013-11-29 11:48:18]

can you ping back when you've uploaded everything?


On Thu, Nov 28, 2013 at 8:22 PM, <xians@chromium.org> wrote:

>
> https://codereview.chromium.org/90743004/diff/60001/
> content/renderer/media/media_stream_audio_sink_owner.cc
> File content/renderer/media/media_stream_audio_sink_owner.cc (right):
>
> https://codereview.chromium.org/90743004/diff/60001/
> content/renderer/media/media_stream_audio_sink_owner.cc#newcode68
> content/renderer/media/media_stream_audio_sink_owner.cc:68: bool
> MediaStreamAudioSinkOwner::IsEqual(
> On 2013/11/28 17:27:18, xians1 wrote:
>
>> On 2013/11/27 17:35:02, Jói wrote:
>> > This will return true if |other| is NULL and media_stream_delegate_
>>
> is NULL,
>
>> > while peer_connection_delegate_ might be the one being used and
>>
> might be
>
>> > non-NULL.
>> >
>> > Same thing for the other IsEqual implementation.
>> >
>> > You need to special case the |other == NULL| case and only return
>>
> true if both
>
>> > media_stream_delegate_ and peer_connection_delegate_ are NULL.
>>
>
>  Added a DCHECK(other);
>>
>
>
> I just realized that I forgot to commit the local change on adding
> DCHECK here, but I will do it tomorrow after I get back to the office.
>
> https://codereview.chromium.org/90743004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[no longer working on chromium, 2013-11-29 12:04:18]

HI guys, now it has everything.

Jochen, may I have your owner stamp for content_renderer.gypi?

Thanks,
SX

[tommi (sloooow) - chröme, 2013-11-29 13:00:20]

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_sink_owner.cc (right):

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/m...
content/renderer/media/media_stream_audio_sink_owner.cc:28:
delegate_->OnData(audio_data,
what about the return value from delegate_->OnData()?

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/m...
content/renderer/media/media_stream_audio_sink_owner.cc:37: void
MediaStreamAudioSinkOwner::OnSetFormat(
which methods do we expect to be called from the callback thread and which from
other threads?  Can we add a thread checker for the "main" thread and make it
clear with a comment when we are on the callback thread?

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/m...
content/renderer/media/media_stream_audio_sink_owner.cc:51: DCHECK(other);
The reason for having this DCHECK doesn't apply now so IsEqual(NULL) is imho a
valid call to see if the delegate pointer is NULL.

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_sink_owner.h (right):

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/m...
content/renderer/media/media_stream_audio_sink_owner.h:17: // Reference counted
holder of MediaStreamAudioSinksinks.
MediaStreamAudioSink sinks.

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/m...
content/renderer/media/media_stream_audio_sink_owner.h:36: virtual bool
IsEqual(const PeerConnectionAudioSink* other) const OVERRIDE {
do you need to implement this?

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/m...
content/renderer/media/media_stream_audio_sink_owner.h:46: // Weak reference to
the delegate, the client need to call Reset() to set the
s/Weak reference/Raw pointer
(since we have WeakPtr and that's a different thing)

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/m...
File content/renderer/media/media_stream_track_extra_data.h (right):

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/m...
content/renderer/media/media_stream_track_extra_data.h:34: bool is_local_track_;
make this const?

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/p...
File content/renderer/media/peer_connection_audio_sink_owner.cc (right):

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/p...
content/renderer/media/peer_connection_audio_sink_owner.cc:41: void
PeerConnectionAudioSinkOwner::OnSetFormat(
can we add thread checks for functions that are expected to be called on the
main thread and document when we're on any other thread?

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/p...
content/renderer/media/peer_connection_audio_sink_owner.cc:55: DCHECK(other);
nit: No need for this dcheck now.

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/p...
File content/renderer/media/peer_connection_audio_sink_owner.h (right):

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/p...
content/renderer/media/peer_connection_audio_sink_owner.h:36: virtual bool
IsEqual(const MediaStreamAudioSink* other) const OVERRIDE{
do you need to override this?

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/p...
content/renderer/media/peer_connection_audio_sink_owner.h:46: // Weak reference
to the delegate, the client need to call Reset() to set the
nit: Raw pointer
s/the client need/the client needs

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_capturer_unittest.cc (right):

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer_unittest.cc:68: class
MockWebRtcAudioCapturerSink : public PeerConnectionAudioSink {
change name to MockPeerConnectionAudioSink.  Alternatively inherit from the
interface instead of an implementation.

[no longer working on chromium, 2013-11-29 15:02:03]

PTAL.

Thanks,
SX

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_sink_owner.cc (right):

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/m...
content/renderer/media/media_stream_audio_sink_owner.cc:28:
delegate_->OnData(audio_data,
On 2013/11/29 13:00:20, tommi wrote:
> what about the return value from delegate_->OnData()?

It is a virtual void OnData(const int16* audio_data,
                            int sample_rate,
                            int number_of_channels,
                            int number_of_frames) = 0;

Because no AGC is needed for the new interface, so no return value is needed.

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/m...
content/renderer/media/media_stream_audio_sink_owner.cc:37: void
MediaStreamAudioSinkOwner::OnSetFormat(
On 2013/11/29 13:00:20, tommi wrote:
> which methods do we expect to be called from the callback thread and which
from
> other threads?  Can we add a thread checker for the "main" thread and make it
> clear with a comment when we are on the callback thread?

OnSetFormat and OnData are called on the same thread, other methods are called
on the main render thread.

As discussed offline, we agreed to not add the thread check for now.

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/m...
content/renderer/media/media_stream_audio_sink_owner.cc:51: DCHECK(other);
On 2013/11/29 13:00:20, tommi wrote:
> The reason for having this DCHECK doesn't apply now so IsEqual(NULL) is imho a
> valid call to see if the delegate pointer is NULL.

why? the caller should make sure other should not be NULL.

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_sink_owner.h (right):

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/m...
content/renderer/media/media_stream_audio_sink_owner.h:17: // Reference counted
holder of MediaStreamAudioSinksinks.
On 2013/11/29 13:00:20, tommi wrote:
> MediaStreamAudioSink sinks.

Done.

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/m...
content/renderer/media/media_stream_audio_sink_owner.h:36: virtual bool
IsEqual(const PeerConnectionAudioSink* other) const OVERRIDE {
On 2013/11/29 13:00:20, tommi wrote:
> do you need to implement this?

I think so, otherwise it will be pure virtual in this class.

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/m...
content/renderer/media/media_stream_audio_sink_owner.h:46: // Weak reference to
the delegate, the client need to call Reset() to set the
On 2013/11/29 13:00:20, tommi wrote:
> s/Weak reference/Raw pointer
> (since we have WeakPtr and that's a different thing)

Done.

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/m...
File content/renderer/media/media_stream_track_extra_data.h (right):

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/m...
content/renderer/media/media_stream_track_extra_data.h:34: bool is_local_track_;
On 2013/11/29 13:00:20, tommi wrote:
> make this const?

Done.

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/p...
File content/renderer/media/peer_connection_audio_sink_owner.cc (right):

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/p...
content/renderer/media/peer_connection_audio_sink_owner.cc:41: void
PeerConnectionAudioSinkOwner::OnSetFormat(
On 2013/11/29 13:00:20, tommi wrote:
> can we add thread checks for functions that are expected to be called on the
> main thread and document when we're on any other thread?

as discussed offline, we can skip this for now since it will touch quite some
existing unittests

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/p...
content/renderer/media/peer_connection_audio_sink_owner.cc:55: DCHECK(other);
On 2013/11/29 13:00:20, tommi wrote:
> nit: No need for this dcheck now.

It is unclear to me, shouldn't the client take care that other won't be NULL ?

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/p...
File content/renderer/media/peer_connection_audio_sink_owner.h (right):

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/p...
content/renderer/media/peer_connection_audio_sink_owner.h:36: virtual bool
IsEqual(const MediaStreamAudioSink* other) const OVERRIDE{
On 2013/11/29 13:00:20, tommi wrote:
> do you need to override this?

Yes. For example, when a new MediaStreamAudioSink sink is added to the track, we
loop through all the MediaStreamAudioTrackSink in the lists and check
IsEqual(const MediaStreamAudioSink*), and those PeerConnectionAudioSinkOwner
should just return false in such case.

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/p...
content/renderer/media/peer_connection_audio_sink_owner.h:46: // Weak reference
to the delegate, the client need to call Reset() to set the
On 2013/11/29 13:00:20, tommi wrote:
> nit: Raw pointer
> s/the client need/the client needs

Done.

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_capturer_unittest.cc (right):

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer_unittest.cc:68: class
MockWebRtcAudioCapturerSink : public PeerConnectionAudioSink {
On 2013/11/29 13:00:20, tommi wrote:
> change name to MockPeerConnectionAudioSink.  Alternatively inherit from the
> interface instead of an implementation.

Done.

[tommi (sloooow) - chröme, 2013-11-29 15:31:25]

lgtm

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_sink_owner.h (right):

https://codereview.chromium.org/90743004/diff/120001/content/renderer/media/m...
content/renderer/media/media_stream_audio_sink_owner.h:36: virtual bool
IsEqual(const PeerConnectionAudioSink* other) const OVERRIDE {
On 2013/11/29 15:02:04, xians1 wrote:
> On 2013/11/29 13:00:20, tommi wrote:
> > do you need to implement this?
> 
> I think so, otherwise it will be pure virtual in this class.

ok, I guess that would do it :)

[Jói, 2013-11-29 15:47:58]

LGTM

https://codereview.chromium.org/90743004/diff/140001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_track_sink.h (right):

https://codereview.chromium.org/90743004/diff/140001/content/renderer/media/m...
content/renderer/media/media_stream_audio_track_sink.h:12: //#include
"content/renderer/media/media_stream_audio_sink_owner.h"
You probably want to remove this and the next line?

[jochen (gone - plz use gerrit), 2013-12-02 10:24:53]

https://codereview.chromium.org/90743004/diff/160001/content/public/renderer/...
File content/public/renderer/media_stream_audio_sink.h (right):

https://codereview.chromium.org/90743004/diff/160001/content/public/renderer/...
content/public/renderer/media_stream_audio_sink.h:23: class CONTENT_EXPORT
MediaStreamAudioSink {
i'm not sure I understand why this interface needs to be in public/?

https://codereview.chromium.org/90743004/diff/160001/content/public/renderer/...
content/public/renderer/media_stream_audio_sink.h:43: CONTENT_EXPORT void
AddToAudioTrack(
why not have those as static methods MediaSreamAudioSink?

[no longer working on chromium, 2013-12-02 10:40:41]

https://codereview.chromium.org/90743004/diff/140001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_track_sink.h (right):

https://codereview.chromium.org/90743004/diff/140001/content/renderer/media/m...
content/renderer/media/media_stream_audio_track_sink.h:12: //#include
"content/renderer/media/media_stream_audio_sink_owner.h"
On 2013/11/29 15:47:59, Jói wrote:
> You probably want to remove this and the next line?

Done.

https://codereview.chromium.org/90743004/diff/160001/content/public/renderer/...
File content/public/renderer/media_stream_audio_sink.h (right):

https://codereview.chromium.org/90743004/diff/160001/content/public/renderer/...
content/public/renderer/media_stream_audio_sink.h:23: class CONTENT_EXPORT
MediaStreamAudioSink {
On 2013/12/02 10:24:55, jochen wrote:
> i'm not sure I understand why this interface needs to be in public/?

This needs to be in public because chrome cast sits in chrome, and it will
implement this interface. And we only have some other implemenations like
webrtc, webaudio in content.

https://codereview.chromium.org/90743004/diff/160001/content/public/renderer/...
content/public/renderer/media_stream_audio_sink.h:43: CONTENT_EXPORT void
AddToAudioTrack(
On 2013/12/02 10:24:55, jochen wrote:
> why not have those as static methods MediaSreamAudioSink?

Do you mean
#1 putting these two methods into MediaStreamAudioSink and make them static?
or #2 put them in a separate class and make them static members there?

For #1, these two methods should not belong to the MediaStreamAudioSink.
For #2, I think it is almost the same as the current approach, and I use the
current approach because it is a also used in other content/public code.

Joi, do you have any comment here? If we are going to change it to static
methods, we probably need to change the sister CL in
https://codereview.chromium.org/83023005/

[Jói, 2013-12-02 10:45:22]

https://codereview.chromium.org/90743004/diff/160001/content/public/renderer/...
File content/public/renderer/media_stream_audio_sink.h (right):

https://codereview.chromium.org/90743004/diff/160001/content/public/renderer/...
content/public/renderer/media_stream_audio_sink.h:43: CONTENT_EXPORT void
AddToAudioTrack(
On 2013/12/02 10:40:42, xians1 wrote:
> On 2013/12/02 10:24:55, jochen wrote:
> > why not have those as static methods MediaSreamAudioSink?
> 
> Do you mean
> #1 putting these two methods into MediaStreamAudioSink and make them static?
> or #2 put them in a separate class and make them static members there?
> 
> For #1, these two methods should not belong to the MediaStreamAudioSink.
> For #2, I think it is almost the same as the current approach, and I use the
> current approach because it is a also used in other content/public code.
> 
> Joi, do you have any comment here? If we are going to change it to static
> methods, we probably need to change the sister CL in
> https://codereview.chromium.org/83023005/

This sink interface will likely be implemented by what you normally think of as
a "sink" i.e. someone receiving data from a track, but also (within content, not
outside) by the tracks themselves to receive data from a source.

If we were exposing the content::MediaStreamTrack concept to embedders, it would
make sense to have these functions as static methods of
content::MediaStreamTrack (or its audio and video specializations). But since
we're not, I think it makes most sense to have these as top-level functions.

[jochen (gone - plz use gerrit), 2013-12-02 10:45:34]

On 2013/12/02 10:40:41, xians1 wrote:
>
https://codereview.chromium.org/90743004/diff/140001/content/renderer/media/m...
> File content/renderer/media/media_stream_audio_track_sink.h (right):
> 
>
https://codereview.chromium.org/90743004/diff/140001/content/renderer/media/m...
> content/renderer/media/media_stream_audio_track_sink.h:12: //#include
> "content/renderer/media/media_stream_audio_sink_owner.h"
> On 2013/11/29 15:47:59, Jói wrote:
> > You probably want to remove this and the next line?
> 
> Done.
> 
>
https://codereview.chromium.org/90743004/diff/160001/content/public/renderer/...
> File content/public/renderer/media_stream_audio_sink.h (right):
> 
>
https://codereview.chromium.org/90743004/diff/160001/content/public/renderer/...
> content/public/renderer/media_stream_audio_sink.h:23: class CONTENT_EXPORT
> MediaStreamAudioSink {
> On 2013/12/02 10:24:55, jochen wrote:
> > i'm not sure I understand why this interface needs to be in public/?
> 
> This needs to be in public because chrome cast sits in chrome, and it will
> implement this interface. And we only have some other implemenations like
> webrtc, webaudio in content.

ok,

> 
>
https://codereview.chromium.org/90743004/diff/160001/content/public/renderer/...
> content/public/renderer/media_stream_audio_sink.h:43: CONTENT_EXPORT void
> AddToAudioTrack(
> On 2013/12/02 10:24:55, jochen wrote:
> > why not have those as static methods MediaSreamAudioSink?
> 
> Do you mean
> #1 putting these two methods into MediaStreamAudioSink and make them static?
> or #2 put them in a separate class and make them static members there?

I meant #1 (also the .cc file doesn't need to be in content/public then)
 
> For #1, these two methods should not belong to the MediaStreamAudioSink.
> For #2, I think it is almost the same as the current approach, and I use the
> current approach because it is a also used in other content/public code.
> 
> Joi, do you have any comment here? If we are going to change it to static
> methods, we probably need to change the sister CL in
> https://codereview.chromium.org/83023005/

[jochen (gone - plz use gerrit), 2013-12-02 10:46:42]

ok, Joi is the API guy, so lgtm :)

[no longer working on chromium, 2013-12-02 11:28:26]

Hi John, could you please take another look at the new content/public interface
to see if it makes sense?
FYI, Per has a sister CL for the video side:
https://codereview.chromium.org/83023005/.

Thanks,
SX

[perkj_chrome, 2013-12-02 14:57:38]

Consider using AddFromAudioTrack and RemoveFromAudioTrack in the
webrtc_local_audio_renderer.

https://codereview.chromium.org/90743004/diff/180001/content/public/renderer/...
File content/public/renderer/media_stream_audio_sink.h (right):

https://codereview.chromium.org/90743004/diff/180001/content/public/renderer/...
content/public/renderer/media_stream_audio_sink.h:30: virtual void OnData(const
int16* audio_data,
Please document on what thread this is called on.

https://codereview.chromium.org/90743004/diff/180001/content/public/renderer/...
content/public/renderer/media_stream_audio_sink.h:37: virtual void
OnSetFormat(const media::AudioParameters& params) = 0;
Are there no overlap between |params| and the parameters in OnData ? 
Ie, would it make more sence to have

virtual void OnData(const int16* audio_data,
                    int number_of_frames,
                    const media::AudioParameters& params) 

and remove OnSetFormat(const media::AudioParameters& params) ?

https://codereview.chromium.org/90743004/diff/180001/content/public/renderer/...
content/public/renderer/media_stream_audio_sink.h:43: CONTENT_EXPORT void
AddToAudioTrack(
Document what thread this need to be called on.

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_sink_owner.h (right):

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/m...
content/renderer/media/media_stream_audio_sink_owner.h:19: : public
MediaStreamAudioTrackSink {
nit: fits on line above

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/m...
content/renderer/media/media_stream_audio_sink_owner.h:23: //
MediaStreamAudioSink implementation.
Nope- this is not from MediaStreamAudioSink. Maybe from
MediaStreamAudioTrackSink?

[Alpha Left Google, 2013-12-02 19:11:37]

https://codereview.chromium.org/90743004/diff/180001/content/public/renderer/...
File content/public/renderer/media_stream_audio_sink.cc (right):

https://codereview.chromium.org/90743004/diff/180001/content/public/renderer/...
content/public/renderer/media_stream_audio_sink.cc:18:
MediaStreamTrackExtraData* extra_data =
track.isNull() can be true and extra_data can also be NULL. It should be checked
here and return a boolean.

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_sink_owner.cc (right):

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/m...
content/renderer/media/media_stream_audio_sink_owner.cc:28:
delegate_->OnData(audio_data,
There should be a TODO to fix this. Delegate can spend long processing time on
the data. Doing this inside a lock is bad. Maybe take the pointer in a lock and
call OnData() outside the lock? I'm not sure but this definitely need a fix.

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/m...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:429:
AddNativeTrackToBlinkTrack(audio_track.get(), track, true);
If I'm looking at this code alone I would have to look further to see what the
extra boolean mean. If the method name is AddLocalTrackToBlinkTrack() then it is
self documenting.

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/m...
File content/renderer/media/media_stream_dependency_factory.h (right):

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.h:152: bool
is_local_track);
I suggest there are two methods to be clear; having a boolean is not very
readable.

e.g.

AddLocalTrackToBlinkTrack(...native_track, ...webrtc_track)  and
AddRemoteTrackToBlinkTrack(...native_track, ...webrtc_track)

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/p...
File content/renderer/media/peer_connection_audio_sink_owner.cc (right):

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/p...
content/renderer/media/peer_connection_audio_sink_owner.cc:27: return
delegate_->OnData(audio_data,
Please add a TODO or fix this. It is unclear whether there is heavy processing
when the lock is acquired.

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/p...
File content/renderer/media/peer_connection_audio_sink_owner.h (right):

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/p...
content/renderer/media/peer_connection_audio_sink_owner.h:17: // Reference
counted holder of PeerConnectionAudioSink.
This class looks very similar to MediaStreamAudioSinkOwner and its sole purpose
is to call the delegate within a lock. Is it possible to combine
MediaStreamAudioSinkOwner and this class?

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/r...
File content/renderer/media/remote_media_stream_impl.cc (right):

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/r...
content/renderer/media/remote_media_stream_impl.cc:56:
content::MediaStreamDependencyFactory::AddNativeTrackToBlinkTrack(
This would be much more readable if this is AddRemoteTrackToBlinkTrack(trac,
*webrtc_track);

[jam, 2013-12-03 00:49:24]

content/public lgtm with nits from https://codereview.chromium.org/83023005/
applied here (moving static methods to interface, and get rid of CONTENT_EXPORT
on interface)

[no longer working on chromium, 2013-12-03 11:45:44]

I have already addressed Alpha, Per and John's comments.

Alpha and Per, could you please take a look at it again?

SX

https://codereview.chromium.org/90743004/diff/180001/content/public/renderer/...
File content/public/renderer/media_stream_audio_sink.cc (right):

https://codereview.chromium.org/90743004/diff/180001/content/public/renderer/...
content/public/renderer/media_stream_audio_sink.cc:18:
MediaStreamTrackExtraData* extra_data =
On 2013/12/02 19:11:38, Alpha wrote:
> track.isNull() can be true and extra_data can also be NULL. It should be
checked
> here and return a boolean.

The sinks are depending on the track, and they should outlive the track by
holding a blink track object, which is a smart pointer. So the track and the
extra data should be always valid. If not, then it is considered to be
programming error and needs to be fixed.

I did some local tests and the current code works. In case the track goes away
before the sink, which is considered as programming error and will crash chrome.
Then we will know and fix the real problem.

https://codereview.chromium.org/90743004/diff/180001/content/public/renderer/...
File content/public/renderer/media_stream_audio_sink.h (right):

https://codereview.chromium.org/90743004/diff/180001/content/public/renderer/...
content/public/renderer/media_stream_audio_sink.h:30: virtual void OnData(const
int16* audio_data,
On 2013/12/02 14:57:39, perkj wrote:
> Please document on what thread this is called on. 

Done.

https://codereview.chromium.org/90743004/diff/180001/content/public/renderer/...
content/public/renderer/media_stream_audio_sink.h:37: virtual void
OnSetFormat(const media::AudioParameters& params) = 0;
On 2013/12/02 14:57:39, perkj wrote:
> Are there no overlap between |params| and the parameters in OnData ? 
> Ie, would it make more sence to have
> 
> virtual void OnData(const int16* audio_data,
>                     int number_of_frames,
>                     const media::AudioParameters& params) 
> 
> and remove OnSetFormat(const media::AudioParameters& params) ? 
>  

We discussed this together in our piranha plant meetings, the problem for such
approach is that the sinks need to check the format for each callback handle the
format change. And we don't want to do this.

https://codereview.chromium.org/90743004/diff/180001/content/public/renderer/...
content/public/renderer/media_stream_audio_sink.h:43: CONTENT_EXPORT void
AddToAudioTrack(
On 2013/12/02 14:57:39, perkj wrote:
> Document what thread this need to be called on.

Done.

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_sink_owner.cc (right):

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/m...
content/renderer/media/media_stream_audio_sink_owner.cc:28:
delegate_->OnData(audio_data,
On 2013/12/02 19:11:38, Alpha wrote:
> There should be a TODO to fix this. Delegate can spend long processing time on
> the data. Doing this inside a lock is bad. Maybe take the pointer in a lock
and
> call OnData() outside the lock? I'm not sure but this definitely need a fix.

This is old code and the change is from renaming:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

There are multiple reasons why we need to hold the lock for the whole section:
#1 When the sink goes away, we call RemoveSink() on the main render thread,
which set the delegate_ to NULL, but OnData() might be ongoing. we need to make
sure no more callback after RemoveSink().
#2 Avoiding too many reference count. We don't want to make all the sinks
reference counted to be able to avoid a lock here.
#3 It is true that delegate might spend some processing time, the drawback is
that it will block the main render thread a bit when the sink is removed from
the track, so it is not a big problem. From the real-time perspective, it is
fine to let the real-time thread to block other thread, but not the other way
around.

Anyway, I added a TODO to make the process quicker, though I am not sure if we
have another better option here.

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_sink_owner.h (right):

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/m...
content/renderer/media/media_stream_audio_sink_owner.h:19: : public
MediaStreamAudioTrackSink {
On 2013/12/02 14:57:39, perkj wrote:
> nit: fits on line above

Done.

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/m...
content/renderer/media/media_stream_audio_sink_owner.h:23: //
MediaStreamAudioSink implementation.
On 2013/12/02 14:57:39, perkj wrote:
> Nope- this is not from MediaStreamAudioSink. Maybe from
> MediaStreamAudioTrackSink?

Done.

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/m...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:429:
AddNativeTrackToBlinkTrack(audio_track.get(), track, true);
On 2013/12/02 19:11:38, Alpha wrote:
> If I'm looking at this code alone I would have to look further to see what the
> extra boolean mean. If the method name is AddLocalTrackToBlinkTrack() then it
is
> self documenting.

Please see my previous comment.

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/m...
File content/renderer/media/media_stream_dependency_factory.h (right):

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.h:152: bool
is_local_track);
On 2013/12/02 19:11:38, Alpha wrote:
> I suggest there are two methods to be clear; having a boolean is not very
> readable.
> 
> e.g.
> 
> AddLocalTrackToBlinkTrack(...native_track, ...webrtc_track)  and
> AddRemoteTrackToBlinkTrack(...native_track, ...webrtc_track)

The reason why we have the is_local_track flag is to distinguish the local audio
track with remote audio track for now, since those AddToAudioTrack()
RemoveFromAudioTrack() methods only support local track.

Adding a flag is Tommi's opinion in order to help dchecking only local track is
supported on those methods.

This solution is deemed to be temporary and the flag will be removed once the
remote audio track can use the same interface as the local audio track.

Also, on the video side, they don't need this at all.

I talked to Per, and he thinks using a flag is the simplest solution and prefers
not adding more temporary method.

Hope it is OK to keep it this way.

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/p...
File content/renderer/media/peer_connection_audio_sink_owner.cc (right):

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/p...
content/renderer/media/peer_connection_audio_sink_owner.cc:27: return
delegate_->OnData(audio_data,
On 2013/12/02 19:11:38, Alpha wrote:
> Please add a TODO or fix this. It is unclear whether there is heavy processing
> when the lock is acquired.

Please see my previous comments, and I am not going to add a TODO here since
this PeerConnectionAudioSinkOwner will be deprecated after peer connection can
use MediaStreamAudioSinkOwner

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/p...
File content/renderer/media/peer_connection_audio_sink_owner.h (right):

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/p...
content/renderer/media/peer_connection_audio_sink_owner.h:17: // Reference
counted holder of PeerConnectionAudioSink.
On 2013/12/02 19:11:38, Alpha wrote:
> This class looks very similar to MediaStreamAudioSinkOwner and its sole
purpose
> is to call the delegate within a lock. Is it possible to combine
> MediaStreamAudioSinkOwner and this class?

I think you want Patch Set 3, where I was asked to separate it into two classes
by Tommi.

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/r...
File content/renderer/media/remote_media_stream_impl.cc (right):

https://codereview.chromium.org/90743004/diff/180001/content/renderer/media/r...
content/renderer/media/remote_media_stream_impl.cc:56:
content::MediaStreamDependencyFactory::AddNativeTrackToBlinkTrack(
On 2013/12/02 19:11:38, Alpha wrote:
> This would be much more readable if this is AddRemoteTrackToBlinkTrack(trac,
> *webrtc_track);

Please see my previous comment.

[perkj_chrome, 2013-12-03 13:38:52]

just nits and a question.

https://codereview.chromium.org/90743004/diff/180001/content/public/renderer/...
File content/public/renderer/media_stream_audio_sink.h (right):

https://codereview.chromium.org/90743004/diff/180001/content/public/renderer/...
content/public/renderer/media_stream_audio_sink.h:37: virtual void
OnSetFormat(const media::AudioParameters& params) = 0;
So should sample_rate and number_of channel be removed from OnData and only
exist in AudioParams?

On 2013/12/03 11:45:45, xians1 wrote:
> On 2013/12/02 14:57:39, perkj wrote:
> > Are there no overlap between |params| and the parameters in OnData ? 
> > Ie, would it make more sence to have
> > 
> > virtual void OnData(const int16* audio_data,
> >                     int number_of_frames,
> >                     const media::AudioParameters& params) 
> > 
> > and remove OnSetFormat(const media::AudioParameters& params) ? 
> >  
> 
> We discussed this together in our piranha plant meetings, the problem for such
> approach is that the sinks need to check the format for each callback handle
the
> format change. And we don't want to do this.

https://codereview.chromium.org/90743004/diff/220001/content/public/renderer/...
File content/public/renderer/media_stream_audio_sink.h (right):

https://codereview.chromium.org/90743004/diff/220001/content/public/renderer/...
content/public/renderer/media_stream_audio_sink.h:49: // This is called on the
same thread as calling OnData().
nit: remove calling

https://codereview.chromium.org/90743004/diff/220001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_sink_owner.cc (right):

https://codereview.chromium.org/90743004/diff/220001/content/renderer/media/m...
content/renderer/media/media_stream_audio_sink_owner.cc:13:
MediaStreamAudioSink* sink)
nit: argument fits on line above.

https://codereview.chromium.org/90743004/diff/220001/content/renderer/media/m...
content/renderer/media/media_stream_audio_sink_owner.cc:27: // TODO(xians):
Investigate on the possibility of not calling out with the
nit: Investigate the possibility to not grab the lock.

https://codereview.chromium.org/90743004/diff/220001/content/renderer/media/p...
File content/renderer/media/peer_connection_audio_sink_owner.h (right):

https://codereview.chromium.org/90743004/diff/220001/content/renderer/media/p...
content/renderer/media/peer_connection_audio_sink_owner.h:19: : public
MediaStreamAudioTrackSink {
nit: fits on line above

[perkj_chrome, 2013-12-03 13:38:57]

just nits and a question.

[no longer working on chromium, 2013-12-03 13:50:28]

Per, PTAL.

Thanks,
SX

https://codereview.chromium.org/90743004/diff/180001/content/public/renderer/...
File content/public/renderer/media_stream_audio_sink.h (right):

https://codereview.chromium.org/90743004/diff/180001/content/public/renderer/...
content/public/renderer/media_stream_audio_sink.h:37: virtual void
OnSetFormat(const media::AudioParameters& params) = 0;
On 2013/12/03 13:38:53, perkj wrote:
> So should sample_rate and number_of channel be removed from OnData and only
> exist in AudioParams?
> 
> On 2013/12/03 11:45:45, xians1 wrote:
> > On 2013/12/02 14:57:39, perkj wrote:
> > > Are there no overlap between |params| and the parameters in OnData ? 
> > > Ie, would it make more sence to have
> > > 
> > > virtual void OnData(const int16* audio_data,
> > >                     int number_of_frames,
> > >                     const media::AudioParameters& params) 
> > > 
> > > and remove OnSetFormat(const media::AudioParameters& params) ? 
> > >  
> > 
> > We discussed this together in our piranha plant meetings, the problem for
such
> > approach is that the sinks need to check the format for each callback handle
> the
> > format change. And we don't want to do this.
> 

No, I am afraid we can't remove them.
They are needed for the following use cases:
# webrtc (peer connection) has no callback interface for OnSetFormat, it expects
format in the data flow.
# We use sample_rate and number_of channel to check the sink clients are always
dealing the data with the right format.

https://codereview.chromium.org/90743004/diff/220001/content/public/renderer/...
File content/public/renderer/media_stream_audio_sink.h (right):

https://codereview.chromium.org/90743004/diff/220001/content/public/renderer/...
content/public/renderer/media_stream_audio_sink.h:49: // This is called on the
same thread as calling OnData().
On 2013/12/03 13:38:53, perkj wrote:
> nit: remove calling

Done.

https://codereview.chromium.org/90743004/diff/220001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_sink_owner.cc (right):

https://codereview.chromium.org/90743004/diff/220001/content/renderer/media/m...
content/renderer/media/media_stream_audio_sink_owner.cc:13:
MediaStreamAudioSink* sink)
On 2013/12/03 13:38:53, perkj wrote:
> nit: argument fits on line above.

Done.

https://codereview.chromium.org/90743004/diff/220001/content/renderer/media/m...
content/renderer/media/media_stream_audio_sink_owner.cc:27: // TODO(xians):
Investigate on the possibility of not calling out with the
On 2013/12/03 13:38:53, perkj wrote:
> nit: Investigate the possibility to not grab the lock. 

I think it is different meaning, we need the lock for atomic operation, and this
TODO is for narrowing the scope of the lock and not calling out.

https://codereview.chromium.org/90743004/diff/220001/content/renderer/media/p...
File content/renderer/media/peer_connection_audio_sink_owner.h (right):

https://codereview.chromium.org/90743004/diff/220001/content/renderer/media/p...
content/renderer/media/peer_connection_audio_sink_owner.h:19: : public
MediaStreamAudioTrackSink {
On 2013/12/03 13:38:53, perkj wrote:
> nit: fits on line above

Done.

[perkj_chrome, 2013-12-03 14:14:34]

lgtm

[Alpha Left Google, 2013-12-03 17:30:56]

Okay. LGTM as the design has been discussed with other reviewers.

[commit-bot: I haz the power, 2013-12-03 17:49:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/90743004/240001

[no longer working on chromium, 2013-12-03 17:54:39]

Thanks guys, I am landing this now and will create a separate CL to inherit the
MediaStreamSInk interface from per's CL.

[commit-bot: I haz the power, 2013-12-03 19:14:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/90743004/240001

[commit-bot: I haz the power, 2013-12-03 23:43:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/90743004/240001

[commit-bot: I haz the power, 2013-12-04 00:46:11]

Change committed as 238503