[PPAPI] Pepper MediaStream API audio track implementation and example.

[PPAPI] Pepper MediaStream API audio track implementation and example.

TBR=jamesr@chromium.org

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

[Peng, 2014-01-16 21:58:38]

Hi, The Audio track implementation is ready. Please take a look. Thanks.

[dmichael (off chromium), 2014-01-16 22:34:20]

Not a comprehensive review, but some early feedback...

https://codereview.chromium.org/140783004/diff/190001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/190001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:94: int32_t size
= sizeof(ppapi::MediaStreamFrame::Audio) + frame_data_size_;
I don't know if we care, but this size calculation comes out bigger than you
need, because the data part of MediaStreamFrame::Audio has non-zero size. Maybe
just comment. Probably not worth the effort to shave bytes here.

(PS: Same is true for Video, I think)

https://codereview.chromium.org/140783004/diff/190001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:96:
main_message_loop_proxy_->PostTask(
What thread is this coming in on? Do you know the calling thread for all of
these functions? This looks a little unusual here, at least in the absence of
more documentation.

The Chromium approach is usually to have each class run on a designated thread.
So if you're dealing with two or more threads, it may make sense to split
functionality. At the very least, it needs to be obvious for each function what
thread(s) it runs on.

https://codereview.chromium.org/140783004/diff/190001/ppapi/shared_impl/media...
File ppapi/shared_impl/media_stream_frame.h (right):

https://codereview.chromium.org/140783004/diff/190001/ppapi/shared_impl/media...
ppapi/shared_impl/media_stream_frame.h:33: // Nacl code and renderer code.
nit: s/Nacl/NaCl

https://codereview.chromium.org/140783004/diff/190001/ppapi/shared_impl/media...
File ppapi/shared_impl/media_stream_frame_buffer.h (right):

https://codereview.chromium.org/140783004/diff/190001/ppapi/shared_impl/media...
ppapi/shared_impl/media_stream_frame_buffer.h:81: mutable base::Lock mutex_;
Can you explain more about why you've done this? We try to avoid explicit
locking as much as possible in Chromium. FWIW, on the plugin side of the proxy,
there is already a lock around everything.

On the host side, we should find a different approach. Usually it involves
posting tasks to an appropriate thread. If you can outline which functions are
on which threads currently in the host side, maybe we can help you come up with
a good approach for dealing with the threading.

[Peng, 2014-01-17 00:23:49]

https://codereview.chromium.org/140783004/diff/190001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/190001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:94: int32_t size
= sizeof(ppapi::MediaStreamFrame::Audio) + frame_data_size_;
On 2014/01/16 22:34:20, dmichael wrote:
> I don't know if we care, but this size calculation comes out bigger than you
> need, because the data part of MediaStreamFrame::Audio has non-zero size.
Maybe
> just comment. Probably not worth the effort to shave bytes here.
> 
> (PS: Same is true for Video, I think)

Done.

https://codereview.chromium.org/140783004/diff/190001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:96:
main_message_loop_proxy_->PostTask(
On 2014/01/16 22:34:20, dmichael wrote:
> What thread is this coming in on? Do you know the calling thread for all of
> these functions? This looks a little unusual here, at least in the absence of
> more documentation.
> 
> The Chromium approach is usually to have each class run on a designated
thread.
> So if you're dealing with two or more threads, it may make sense to split
> functionality. At the very least, it needs to be obvious for each function
what
> thread(s) it runs on.

OnData() & OnSetFormat() are called from an Audio Thread. Because the
InitFrames() will do some sync IPC, so have to do it in main thread.:(

I add some document here. Not sure if it is enough. Done

https://codereview.chromium.org/140783004/diff/190001/ppapi/shared_impl/media...
File ppapi/shared_impl/media_stream_frame.h (right):

https://codereview.chromium.org/140783004/diff/190001/ppapi/shared_impl/media...
ppapi/shared_impl/media_stream_frame.h:33: // Nacl code and renderer code.
On 2014/01/16 22:34:20, dmichael wrote:
> nit: s/Nacl/NaCl

Done.

https://codereview.chromium.org/140783004/diff/190001/ppapi/shared_impl/media...
File ppapi/shared_impl/media_stream_frame_buffer.h (right):

https://codereview.chromium.org/140783004/diff/190001/ppapi/shared_impl/media...
ppapi/shared_impl/media_stream_frame_buffer.h:81: mutable base::Lock mutex_;
On 2014/01/16 22:34:20, dmichael wrote:
> Can you explain more about why you've done this? We try to avoid explicit
> locking as much as possible in Chromium. FWIW, on the plugin side of the
proxy,
> there is already a lock around everything.
> 
> On the host side, we should find a different approach. Usually it involves
> posting tasks to an appropriate thread. If you can outline which functions are
> on which threads currently in the host side, maybe we can help you come up
with
> a good approach for dealing with the threading.

All audio frames are received from an audio thread (|OnData()|). I know we could
post it to main thread. But it will add some latency.:( And samples data passed
to OnData is not refcounted. If we want to post task to main thread |OnData()|,
we also need find a way how to handle it (copied it to a temp buffer, or call
Dequeue() to get a free frame ready in main thread before OnData() is called).

FYI, This Lock protects operations on the frame_queue_. Most operations are
enqueue and dequeue. They are not big time consuming operations.
The lock's impact is minor I think. 

I have add more inline comments on AudioTrackHost, please take a look.

[dmichael (off chromium), 2014-01-17 23:45:05]

Sorry I didn't have a chance to comment sooner today...  I still think we need
to come up with a way to follow a more idiomatic approach for the threads.

https://codereview.chromium.org/140783004/diff/340001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/340001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:82:
SendEnqueueFrameMessageToPlugin(index);
The way that ChannelProxy makes it OK to send from any thread is by doing
PostTask on to the IO thread. So there's always a thread hop. I wonder if you
could make the Host live on the IO thread, and PostTask from here to a host
operation on the IO thread. Then there would probably be a way to avoid another
"PostTask" from there...

You could implement the MediaStreamAudioSink stuff in a separate delegate class
that lives on the audio thread.

I really think we should avoid using the lock the way you have, because it's
unusual in Chrome. It's also unusual and confusing here that this class gets
created on the main thread but called on the audio thread for most operations
here, and called on the main thread for base class operations. I'm not 100% sure
if there's a clean design for separating this without messing up your use of a
base class for the host, but I think the current locking approach plus the mix
of two different calling threads for methods in this Host hierarchy is too
different from usual Chromium practice.

https://codereview.chromium.org/140783004/diff/340001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:98: // The size
is slight bigger for containing a frame, because 8 extra bytes are
nit: "slight bigger"->"slightly bigger than necessary"

[Peng, 2014-01-20 22:05:41]

https://codereview.chromium.org/140783004/diff/340001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/340001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:82:
SendEnqueueFrameMessageToPlugin(index);

On 2014/01/17 23:45:06, dmichael wrote:
> The way that ChannelProxy makes it OK to send from any thread is by doing
> PostTask on to the IO thread. So there's always a thread hop. I wonder if you
> could make the Host live on the IO thread, and PostTask from here to a host
> operation on the IO thread. Then there would probably be a way to avoid
another
> "PostTask" from there...

Do we have existing host which lives on the IO thread? I don't know how to do
it.

> 
> You could implement the MediaStreamAudioSink stuff in a separate delegate
class
> that lives on the audio thread.
> 
> I really think we should avoid using the lock the way you have, because it's
> unusual in Chrome. It's also unusual and confusing here that this class gets
> created on the main thread but called on the audio thread for most operations
> here, and called on the main thread for base class operations. I'm not 100%
sure
> if there's a clean design for separating this without messing up your use of a
> base class for the host, but I think the current locking approach plus the mix
> of two different calling threads for methods in this Host hierarchy is too
> different from usual Chromium practice.

There are some difficulties if lock is not used:

1. The argument audio_data of OnData() is owned by caller, if we want to post a
task to main thread to create and send frames, we have to copy it to a temporary
memory in the audio thread first, and then copy the data from the temporary
memory to the frame buffer in the main thread later. :(

2. If we want to fill the frame in the audio thread, we have to maintain the
|frame_qeue_| in the audio thread. So when we receive the HostEnqueueFrame
message, we need post a task to the audio thread to add frame into the
|frame_queue_|. But the audio thread does not have a message loop.  So this way
does not work. :(

3. I think an atomic integer can resolve issue. We could use 32 (or 64) bits of
the atomic integer to mark which frames are free. So we can access this integer
in the audio and main threads without lock. But unfortunately, base/atomicops.h
does not support bits operations now. We need implement it. :(

Any idea? What's your suggestion?

https://codereview.chromium.org/140783004/diff/340001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:98: // The size
is slight bigger for containing a frame, because 8 extra bytes are
On 2014/01/17 23:45:06, dmichael wrote:
> nit: "slight bigger"->"slightly bigger than necessary"

Done.

[dmichael (off chromium), 2014-01-21 23:22:53]

I'm having trouble coming up with ideas that are clearly better than what you
have, given the current architecture, but see if you like any of them.

That said...  have you thought at all about a whole different architecture,
where we don't bother passing the audio through the renderer? The audio is
actually being captured in the Browser, which sends it to all listening
processes via a CancellableSyncSocket. It should be possible to plumb this stuff
from the browser to the plugin. In that case, we wouldn't use the shared memory
buffer at all...  the plugin would have an audio thread that reads the
CancellableSyncSocket. That thread would probably stick the data in a circular
buffer that is *not* shared (i.e., is just normal memory in the plugin process),
and run the callback.

The plumbing to get the audio data there might be tricky, but the actual plugin
stuff would be simpler, and we'd eliminate a whole process hop's worth of
latency.

https://codereview.chromium.org/140783004/diff/340001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/340001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:69:
&(frame_buffer()->GetFramePointer(index)->audio);
It's not obvious that it's safe to call frame_buffer() or
SendEnqueueFrameMessage inside OnData. (I think it is technically safe, but I
first thought there was a bug here in the current implementation. I.e., that the
PepperMediaStreamAudioTrackHost could be destroyed after this point, and so
|frame| and |this| might become invalid.)

The thing that protects you (I think!) is that the destructor calls OnClose,
which calls RemoveFromAudioTrack, which ultimately calls
MediaStreamAudioSinkOwner::Reset, which tries to acquire the same lock that is
held while OnData is running:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

So...  the destructor in that case actually blocks, and so |this| stays valid
until after OnData has finished.

But that's all pretty subtle. (Aside: It makes me kind of sad that the
MediaStreamAudio code already has at least a couple of locks, and we're adding
another. I'm not so much worried about performance as I am our ability to reason
about it and get it right with no race conditions or deadlocks.)

I'm thinking that you should define a separate "AudioSinkDelegate" class (name
up to you) that implements MediaStreamAudioSink. The
PepperMediaStreamAudioTrackHost is responsible for creating it and calling the
Add/RemoveFromAudioTrack functions. It would probably have to have some
callbacks to the Host to relay back the "WroteFrame(index)" and "InitFrames"
events. It doesn't actually simplify anything, but at least there would be some
distinction about which class runs on which threads, which is more in keeping
with Chromium practice, so more readable to a Chromium developer, probably. The
delegate would probably just have a raw pointer to the buffer. (So the downside
there is we still have to make the same assumptions about lifespan. But it still
might be cleaner, and we'd have convenient places to document our assumptions.. 
like, right after we call RemoveFromAudioTrack and right before we delete the
Delegate.)

WDYT?

By the way, I'm having trouble thinking of any nice way to avoid the lock in the
buffer :-/.

https://codereview.chromium.org/140783004/diff/340001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:82:
SendEnqueueFrameMessageToPlugin(index);
On 2014/01/20 22:05:44, Peng wrote:
> 
> On 2014/01/17 23:45:06, dmichael wrote:
> > The way that ChannelProxy makes it OK to send from any thread is by doing
> > PostTask on to the IO thread. So there's always a thread hop. I wonder if
you
> > could make the Host live on the IO thread, and PostTask from here to a host
> > operation on the IO thread. Then there would probably be a way to avoid
> another
> > "PostTask" from there...
> 
> Do we have existing host which lives on the IO thread? I don't know how to do
> it.
We have "Filters" instead, which are (IIRC) created and destroyed on the IO
thread. Mostly in the browser side.
https://code.google.com/p/chromium/codesearch#search/&q=Pepper.*Filter&sq=pac...
Note how in OverrideTaskRunnerForMessage, they can specify which messages are
handled on which threads. You could The tricky bit there is that you have a
shared Host base class, which might not work right if you transition to a
Filter.

So...  I'm not sure it's worth the trouble. It would still be hard to avoid
another PostTask when sending from the IO thread, I think.

> 
> > 
> > You could implement the MediaStreamAudioSink stuff in a separate delegate
> class
> > that lives on the audio thread.
> > 
> > I really think we should avoid using the lock the way you have, because it's
> > unusual in Chrome. It's also unusual and confusing here that this class gets
> > created on the main thread but called on the audio thread for most
operations
> > here, and called on the main thread for base class operations. I'm not 100%
> sure
> > if there's a clean design for separating this without messing up your use of
a
> > base class for the host, but I think the current locking approach plus the
mix
> > of two different calling threads for methods in this Host hierarchy is too
> > different from usual Chromium practice.
> 
> There are some difficulties if lock is not used:
> 
> 1. The argument audio_data of OnData() is owned by caller, if we want to post
a
> task to main thread to create and send frames, we have to copy it to a
temporary
> memory in the audio thread first, and then copy the data from the temporary
> memory to the frame buffer in the main thread later. :(
> 
> 2. If we want to fill the frame in the audio thread, we have to maintain the
> |frame_qeue_| in the audio thread. So when we receive the HostEnqueueFrame
> message, we need post a task to the audio thread to add frame into the
> |frame_queue_|. But the audio thread does not have a message loop.  So this
way
> does not work. :(
> 
> 3. I think an atomic integer can resolve issue. We could use 32 (or 64) bits
of
> the atomic integer to mark which frames are free. So we can access this
integer
> in the audio and main threads without lock. But unfortunately,
base/atomicops.h
> does not support bits operations now. We need implement it. :(
> 
> Any idea? What's your suggestion?
My best alternative I can think of right now is having yet another buffer, sort
of a simple (slightly modified) producer/consumer queue for talking to the audio
thread. When the Host gets an Enqueue message, it immediately dequeues and
pushes it in to the audio thread's queue. Inside the audio thread, there would
be a call on that little queue like ...TryDequeue, which grabs a lock inside the
producer/consumer queue, if there's a frame, pops it, and returns either failure
or the frame. I'm not sure if this is any better. It separates the concerns of
cross-thread from cross-process, so it might actually be clearer.  But it means
implementing a whole separate (though simpler) queue, so that's kind of bad. And
it still requires locking.

[Peng, 2014-01-22 20:13:31]

https://codereview.chromium.org/140783004/diff/340001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/340001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:69:
&(frame_buffer()->GetFramePointer(index)->audio);
On 2014/01/21 23:22:53, dmichael wrote:
> It's not obvious that it's safe to call frame_buffer() or
> SendEnqueueFrameMessage inside OnData. (I think it is technically safe, but I
> first thought there was a bug here in the current implementation. I.e., that
the
> PepperMediaStreamAudioTrackHost could be destroyed after this point, and so
> |frame| and |this| might become invalid.)
> 
> The thing that protects you (I think!) is that the destructor calls OnClose,
> which calls RemoveFromAudioTrack, which ultimately calls
> MediaStreamAudioSinkOwner::Reset, which tries to acquire the same lock that is
> held while OnData is running:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
> 
> So...  the destructor in that case actually blocks, and so |this| stays valid
> until after OnData has finished.
> 
> But that's all pretty subtle. (Aside: It makes me kind of sad that the
> MediaStreamAudio code already has at least a couple of locks, and we're adding
> another. I'm not so much worried about performance as I am our ability to
reason
> about it and get it right with no race conditions or deadlocks.)
> 
> I'm thinking that you should define a separate "AudioSinkDelegate" class (name
> up to you) that implements MediaStreamAudioSink. The
> PepperMediaStreamAudioTrackHost is responsible for creating it and calling the
> Add/RemoveFromAudioTrack functions. It would probably have to have some
> callbacks to the Host to relay back the "WroteFrame(index)" and "InitFrames"
> events. It doesn't actually simplify anything, but at least there would be
some
> distinction about which class runs on which threads, which is more in keeping
> with Chromium practice, so more readable to a Chromium developer, probably.
The
> delegate would probably just have a raw pointer to the buffer. (So the
downside
> there is we still have to make the same assumptions about lifespan. But it
still
> might be cleaner, and we'd have convenient places to document our
assumptions.. 
> like, right after we call RemoveFromAudioTrack and right before we delete the
> Delegate.)
> 
> WDYT?

Done

> 
> By the way, I'm having trouble thinking of any nice way to avoid the lock in
the
> buffer :-/.

https://codereview.chromium.org/140783004/diff/340001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:82:
SendEnqueueFrameMessageToPlugin(index);
On 2014/01/21 23:22:53, dmichael wrote:
> On 2014/01/20 22:05:44, Peng wrote:
> > 
> > On 2014/01/17 23:45:06, dmichael wrote:
> > > The way that ChannelProxy makes it OK to send from any thread is by doing
> > > PostTask on to the IO thread. So there's always a thread hop. I wonder if
> you
> > > could make the Host live on the IO thread, and PostTask from here to a
host
> > > operation on the IO thread. Then there would probably be a way to avoid
> > another
> > > "PostTask" from there...
> > 
> > Do we have existing host which lives on the IO thread? I don't know how to
do
> > it.
> We have "Filters" instead, which are (IIRC) created and destroyed on the IO
> thread. Mostly in the browser side.
>
https://code.google.com/p/chromium/codesearch#search/&q=Pepper.*Filter&sq=pac...
> Note how in OverrideTaskRunnerForMessage, they can specify which messages are
> handled on which threads. You could The tricky bit there is that you have a
> shared Host base class, which might not work right if you transition to a
> Filter.
> 
> So...  I'm not sure it's worth the trouble. It would still be hard to avoid
> another PostTask when sending from the IO thread, I think.

Leave it. Maybe revisit it in future, if we find any performance issue.

> 
> > 
> > > 
> > > You could implement the MediaStreamAudioSink stuff in a separate delegate
> > class
> > > that lives on the audio thread.
> > > 
> > > I really think we should avoid using the lock the way you have, because
it's
> > > unusual in Chrome. It's also unusual and confusing here that this class
gets
> > > created on the main thread but called on the audio thread for most
> operations
> > > here, and called on the main thread for base class operations. I'm not
100%
> > sure
> > > if there's a clean design for separating this without messing up your use
of
> a
> > > base class for the host, but I think the current locking approach plus the
> mix
> > > of two different calling threads for methods in this Host hierarchy is too
> > > different from usual Chromium practice.
> > 
> > There are some difficulties if lock is not used:
> > 
> > 1. The argument audio_data of OnData() is owned by caller, if we want to
post
> a
> > task to main thread to create and send frames, we have to copy it to a
> temporary
> > memory in the audio thread first, and then copy the data from the temporary
> > memory to the frame buffer in the main thread later. :(
> > 
> > 2. If we want to fill the frame in the audio thread, we have to maintain the
> > |frame_qeue_| in the audio thread. So when we receive the HostEnqueueFrame
> > message, we need post a task to the audio thread to add frame into the
> > |frame_queue_|. But the audio thread does not have a message loop.  So this
> way
> > does not work. :(
> > 
> > 3. I think an atomic integer can resolve issue. We could use 32 (or 64) bits
> of
> > the atomic integer to mark which frames are free. So we can access this
> integer
> > in the audio and main threads without lock. But unfortunately,
> base/atomicops.h
> > does not support bits operations now. We need implement it. :(
> > 
> > Any idea? What's your suggestion?
> My best alternative I can think of right now is having yet another buffer,
sort
> of a simple (slightly modified) producer/consumer queue for talking to the
audio
> thread. When the Host gets an Enqueue message, it immediately dequeues and
> pushes it in to the audio thread's queue. Inside the audio thread, there would
> be a call on that little queue like ...TryDequeue, which grabs a lock inside
the
> producer/consumer queue, if there's a frame, pops it, and returns either
failure
> or the frame. I'm not sure if this is any better. It separates the concerns of
> cross-thread from cross-process, so it might actually be clearer.  But it
means
> implementing a whole separate (though simpler) queue, so that's kind of bad.
And
> it still requires locking.

Done.

[yzshen1, 2014-01-22 21:05:47]

Sorry for late reply.

A high-level comment: It might be good to do some tests to see whether the
performance of the current approach (shared_mem + IPC notifications for buffer
readiness/consumption) is good enough for audio data.
As David said, we previously used a different approach to transfer audio data
between the browser and the plugin process (PPB_Audio and PPB_AudioInput_Dev). I
think the purpose is to have smaller and more consistent latency. It would be
nice if we have some data to compare the two approaches.

What do you think?

[bbudge, 2014-01-22 22:06:36]

https://codereview.chromium.org/140783004/diff/450003/content/renderer/pepper...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/450003/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:39: 
I don't understand your use of atomics here. Between the NoBarrier_Load and the
NoBarrier_AtomicIncrement, it seems the value could change.

Is this method called by more than one thread? If so, it seems like it won't be
thread safe.

[Peng, 2014-01-23 01:14:21]

https://codereview.chromium.org/140783004/diff/450003/content/renderer/pepper...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/450003/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:39: 
On 2014/01/22 22:06:37, bbudge wrote:
> I don't understand your use of atomics here. Between the NoBarrier_Load and
the
> NoBarrier_AtomicIncrement, it seems the value could change.
> 
> Is this method called by more than one thread? If so, it seems like it won't
be
> thread safe.

This method is only called from the main thread. And OnData() and OnSetFormat()
will be only called from the audio thread.

In this function, the NoBarrier_Load() is not necessary at all. Actually it is
only used for below DCHECK() for debugging only. Probably I should make them
into one line. Like:

DCHECK(!(base::subtle::NoBarrier_Load(&frames_) & (1 << index)));

[Peng, 2014-01-24 17:39:22]

On 2014/01/22 21:05:47, yzshen1 wrote:
> Sorry for late reply.
> 
> A high-level comment: It might be good to do some tests to see whether the
> performance of the current approach (shared_mem + IPC notifications for buffer
> readiness/consumption) is good enough for audio data.
> As David said, we previously used a different approach to transfer audio data
> between the browser and the plugin process (PPB_Audio and PPB_AudioInput_Dev).
I
> think the purpose is to have smaller and more consistent latency. It would be
> nice if we have some data to compare the two approaches.
> 
> What do you think?

I used chrome tracing on a Z620. And I got some latency data for AudioTrack.

Latency for total 5556 AudioFrames (10ms duration per AudioFrames) : Max:0.934ms
Min:0.078ms Average:0.247ms

The latency is between AudioTrackHost::AudioSink::OnData() being called in the
audio thread in renderer and the AudioFrame being received in plugin.

[yzshen1, 2014-01-24 17:44:01]

On 2014/01/24 17:39:22, Peng wrote:
> On 2014/01/22 21:05:47, yzshen1 wrote:
> > Sorry for late reply.
> > 
> > A high-level comment: It might be good to do some tests to see whether the
> > performance of the current approach (shared_mem + IPC notifications for
buffer
> > readiness/consumption) is good enough for audio data.
> > As David said, we previously used a different approach to transfer audio
data
> > between the browser and the plugin process (PPB_Audio and
PPB_AudioInput_Dev).
> I
> > think the purpose is to have smaller and more consistent latency. It would
be
> > nice if we have some data to compare the two approaches.
> > 
> > What do you think?
> 
> I used chrome tracing on a Z620. And I got some latency data for AudioTrack.
> 
> Latency for total 5556 AudioFrames (10ms duration per AudioFrames) :
Max:0.934ms
> Min:0.078ms Average:0.247ms
> 
> The latency is between AudioTrackHost::AudioSink::OnData() being called in the
> audio thread in renderer and the AudioFrame being received in plugin.

Could you please also try it out on some lower performance machines? Such as a
slow windows laptop?
While you are playing with some other tabs (resource consuming sites such as
docs, gmail, youtube)?

And it would be nice if we have a document recording the data you get. :)

Thanks!

[dmichael (off chromium), 2014-01-24 18:28:53]

Thank you for collecting some data!

I think it would also be interesting to know the latency between the
browser and renderer, if you can measure it. That would give us a rough
idea of what latency would look like going directly to the plugin.


On Fri, Jan 24, 2014 at 10:44 AM, <yzshen@chromium.org> wrote:

> On 2014/01/24 17:39:22, Peng wrote:
>
>> On 2014/01/22 21:05:47, yzshen1 wrote:
>> > Sorry for late reply.
>> >
>> > A high-level comment: It might be good to do some tests to see whether
>> the
>> > performance of the current approach (shared_mem + IPC notifications for
>>
> buffer
>
>> > readiness/consumption) is good enough for audio data.
>> > As David said, we previously used a different approach to transfer audio
>>
> data
>
>> > between the browser and the plugin process (PPB_Audio and
>>
> PPB_AudioInput_Dev).
>
>> I
>> > think the purpose is to have smaller and more consistent latency. It
>> would
>>
> be
>
>> > nice if we have some data to compare the two approaches.
>> >
>> > What do you think?
>>
>
>  I used chrome tracing on a Z620. And I got some latency data for
>> AudioTrack.
>>
>
>  Latency for total 5556 AudioFrames (10ms duration per AudioFrames) :
>>
> Max:0.934ms
>
>> Min:0.078ms Average:0.247ms
>>
>
>  The latency is between AudioTrackHost::AudioSink::OnData() being called
>> in the
>> audio thread in renderer and the AudioFrame being received in plugin.
>>
>
> Could you please also try it out on some lower performance machines? Such
> as a
> slow windows laptop?
> While you are playing with some other tabs (resource consuming sites such
> as
> docs, gmail, youtube)?
>
> And it would be nice if we have a document recording the data you get. :)
>
> Thanks!
>
>
>
> https://codereview.chromium.org/140783004/
>

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

[Peng, 2014-01-24 20:28:17]

https://docs.google.com/a/google.com/spreadsheets/d/1gRD3ugeFZJvQgnj2_Za__w8D...

I created a google sheets to compare latency between MediaStreamAudioTrack and
AudioInput. PTAL.

MediaStreamAudioTrack latency between audio sample being received in Host and
Plugin resource.
AudioInput latency is between audio samples being received in browser process
and plugin process.

AudioInput has less latency. But it uses an audio threads in plugin to deliver
audio samples. If we use similar implementation, it is difficult to use similar
interface for both audio and video track.

What's your suggestions? Do you think the latency in MediaStreamAudioTrack is
acceptable?  How do we reduce the latency? Does chrome support prioritized IPC
messages? So some messages with higher priority can be delivered and processed
before other messages.


On 2014/01/24 18:28:53, dmichael wrote:
> Thank you for collecting some data!
> 
> I think it would also be interesting to know the latency between the
> browser and renderer, if you can measure it. That would give us a rough
> idea of what latency would look like going directly to the plugin.
> 
> 
> On Fri, Jan 24, 2014 at 10:44 AM, <mailto:yzshen@chromium.org> wrote:
> 
> > On 2014/01/24 17:39:22, Peng wrote:
> >
> >> On 2014/01/22 21:05:47, yzshen1 wrote:
> >> > Sorry for late reply.
> >> >
> >> > A high-level comment: It might be good to do some tests to see whether
> >> the
> >> > performance of the current approach (shared_mem + IPC notifications for
> >>
> > buffer
> >
> >> > readiness/consumption) is good enough for audio data.
> >> > As David said, we previously used a different approach to transfer audio
> >>
> > data
> >
> >> > between the browser and the plugin process (PPB_Audio and
> >>
> > PPB_AudioInput_Dev).
> >
> >> I
> >> > think the purpose is to have smaller and more consistent latency. It
> >> would
> >>
> > be
> >
> >> > nice if we have some data to compare the two approaches.
> >> >
> >> > What do you think?
> >>
> >
> >  I used chrome tracing on a Z620. And I got some latency data for
> >> AudioTrack.
> >>
> >
> >  Latency for total 5556 AudioFrames (10ms duration per AudioFrames) :
> >>
> > Max:0.934ms
> >
> >> Min:0.078ms Average:0.247ms
> >>
> >
> >  The latency is between AudioTrackHost::AudioSink::OnData() being called
> >> in the
> >> audio thread in renderer and the AudioFrame being received in plugin.
> >>
> >
> > Could you please also try it out on some lower performance machines? Such
> > as a
> > slow windows laptop?
> > While you are playing with some other tabs (resource consuming sites such
> > as
> > docs, gmail, youtube)?
> >
> > And it would be nice if we have a document recording the data you get. :)
> >
> > Thanks!
> >
> >
> >
> > https://codereview.chromium.org/140783004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[dmichael (off chromium), 2014-01-24 22:17:22]

On 2014/01/24 20:28:17, Peng wrote:
>
https://docs.google.com/a/google.com/spreadsheets/d/1gRD3ugeFZJvQgnj2_Za__w8D...
> 
> I created a google sheets to compare latency between MediaStreamAudioTrack and
> AudioInput. PTAL.
> 
> MediaStreamAudioTrack latency between audio sample being received in Host and
> Plugin resource.
> AudioInput latency is between audio samples being received in browser process
> and plugin process.
> 
> AudioInput has less latency. But it uses an audio threads in plugin to deliver
> audio samples. If we use similar implementation, it is difficult to use
similar
> interface for both audio and video track.
> 
> What's your suggestions? Do you think the latency in MediaStreamAudioTrack is
> acceptable?  How do we reduce the latency? Does chrome support prioritized IPC
> messages? So some messages with higher priority can be delivered and processed
> before other messages.
> 
Hmm, that's a pretty big difference. I think if the CL was more straightforward
in its thread model, I'd be willing to take the hit and optimize later. But I
think with it being that much slower, and also kind of complex, it might be
worth spending the time to try to find out how hard it would be to make the
browser talk directly to the plugin. Maybe it means shipping one end of the
SyncSocket over to the plugin somehow, or maybe it's some more complicated thing
where we need a host to live in the browser.

I agree the API is unavoidably slower. Is there any good way for you to measure
how much latency is due to the extra IPC hop and how much is due to task
switching? E.g., you could add something in the plugin side of the AudioInput
API to do a PostTask to the main plugin thread, and measure how much time that
would add. That could give us a rough idea of the best latency we could get with
the new API.

[Peng, 2014-01-24 23:37:48]

On 2014/01/24 22:17:22, dmichael wrote:
> On 2014/01/24 20:28:17, Peng wrote:
> >
>
https://docs.google.com/a/google.com/spreadsheets/d/1gRD3ugeFZJvQgnj2_Za__w8D...
> > 
> > I created a google sheets to compare latency between MediaStreamAudioTrack
and
> > AudioInput. PTAL.
> > 
> > MediaStreamAudioTrack latency between audio sample being received in Host
and
> > Plugin resource.
> > AudioInput latency is between audio samples being received in browser
process
> > and plugin process.
> > 
> > AudioInput has less latency. But it uses an audio threads in plugin to
deliver
> > audio samples. If we use similar implementation, it is difficult to use
> similar
> > interface for both audio and video track.
> > 
> > What's your suggestions? Do you think the latency in MediaStreamAudioTrack
is
> > acceptable?  How do we reduce the latency? Does chrome support prioritized
IPC
> > messages? So some messages with higher priority can be delivered and
processed
> > before other messages.
> > 
> Hmm, that's a pretty big difference. I think if the CL was more
straightforward
> in its thread model, I'd be willing to take the hit and optimize later. But I
> think with it being that much slower, and also kind of complex, it might be
> worth spending the time to try to find out how hard it would be to make the
> browser talk directly to the plugin. Maybe it means shipping one end of the
> SyncSocket over to the plugin somehow, or maybe it's some more complicated
thing
> where we need a host to live in the browser.

In my test, the audio data comes from browser process (mic). But with current
implementation,
we don't forbid sending an audio track which data comes from network or another
plugin to a NaCl
plugin. So in those case, data is not from browser directly. So AudioInput_Dev's
way does not work.
So we have to have two code paths, if we optimize mic input with
AudioInput_Dev's solution.

> 
> I agree the API is unavoidably slower. Is there any good way for you to
measure
> how much latency is due to the extra IPC hop and how much is due to task
> switching? E.g., you could add something in the plugin side of the AudioInput
> API to do a PostTask to the main plugin thread, and measure how much time that
> would add. That could give us a rough idea of the best latency we could get
with
> the new API.

Sure. Next week, I will do more research to find out which part adds more
latency.

[Peng, 2014-01-27 17:50:49]

On 2014/01/24 23:37:48, Peng wrote:
> On 2014/01/24 22:17:22, dmichael wrote:
> > On 2014/01/24 20:28:17, Peng wrote:
> > >
> >
>
https://docs.google.com/a/google.com/spreadsheets/d/1gRD3ugeFZJvQgnj2_Za__w8D...
> > > 
> > > I created a google sheets to compare latency between MediaStreamAudioTrack
> and
> > > AudioInput. PTAL.
> > > 
> > > MediaStreamAudioTrack latency between audio sample being received in Host
> and
> > > Plugin resource.
> > > AudioInput latency is between audio samples being received in browser
> process
> > > and plugin process.
> > > 
> > > AudioInput has less latency. But it uses an audio threads in plugin to
> deliver
> > > audio samples. If we use similar implementation, it is difficult to use
> > similar
> > > interface for both audio and video track.
> > > 
> > > What's your suggestions? Do you think the latency in MediaStreamAudioTrack
> is
> > > acceptable?  How do we reduce the latency? Does chrome support prioritized
> IPC
> > > messages? So some messages with higher priority can be delivered and
> processed
> > > before other messages.
> > > 
> > Hmm, that's a pretty big difference. I think if the CL was more
> straightforward
> > in its thread model, I'd be willing to take the hit and optimize later. But
I
> > think with it being that much slower, and also kind of complex, it might be
> > worth spending the time to try to find out how hard it would be to make the
> > browser talk directly to the plugin. Maybe it means shipping one end of the
> > SyncSocket over to the plugin somehow, or maybe it's some more complicated
> thing
> > where we need a host to live in the browser.
> 
> In my test, the audio data comes from browser process (mic). But with current
> implementation,
> we don't forbid sending an audio track which data comes from network or
another
> plugin to a NaCl
> plugin. So in those case, data is not from browser directly. So
AudioInput_Dev's
> way does not work.
> So we have to have two code paths, if we optimize mic input with
> AudioInput_Dev's solution.
> 
> > 
> > I agree the API is unavoidably slower. Is there any good way for you to
> measure
> > how much latency is due to the extra IPC hop and how much is due to task
> > switching? E.g., you could add something in the plugin side of the
AudioInput
> > API to do a PostTask to the main plugin thread, and measure how much time
that
> > would add. That could give us a rough idea of the best latency we could get
> with
> > the new API.
> 
> Sure. Next week, I will do more research to find out which part adds more
> latency.

Hi, I updated the sheets to include the detail in the MediaStreamAudioTrack
latency.

I think the thread hops adds a lot of latency. Probably we could improve it by
using a separate PIPE to signal plugin for a new frame.

[yzshen1, 2014-01-27 18:01:31]

> Hi, I updated the sheets to include the detail in the MediaStreamAudioTrack
> latency.
> 
> I think the thread hops adds a lot of latency. Probably we could improve it by
> using a separate PIPE to signal plugin for a new frame.

If the plugin wants to receive notifications on the main thread (by calling
GetFrame() on the main thread), there is not much that we can do, we have to
dispatch messages to the main thread anyway.

Otherwise, we may be able to do smart things to directly dispatch messages to
the target thread (instead of io->main->target, we do io->target directly).

That may be simpler than introducing a separate IO thread for the audio traffic.

I did similar things in https://codereview.chromium.org/46433002

In the case of sending unsolicited message, because it is not a reply, we need
another way to figure out the target thread, maybe we can give the renderer
process an (opaque) ID, and the renderer can give it back to the plugin when
calling SendUnsolicitedReply().

[Peng, 2014-01-29 19:01:18]

On 2014/01/27 18:01:31, yzshen1 wrote:
> > Hi, I updated the sheets to include the detail in the MediaStreamAudioTrack
> > latency.
> > 
> > I think the thread hops adds a lot of latency. Probably we could improve it
by
> > using a separate PIPE to signal plugin for a new frame.
> 
> If the plugin wants to receive notifications on the main thread (by calling
> GetFrame() on the main thread), there is not much that we can do, we have to
> dispatch messages to the main thread anyway.
> 
> Otherwise, we may be able to do smart things to directly dispatch messages to
> the target thread (instead of io->main->target, we do io->target directly).
> 
> That may be simpler than introducing a separate IO thread for the audio
traffic.
> 
> I did similar things in https://codereview.chromium.org/46433002
> 
> In the case of sending unsolicited message, because it is not a reply, we need
> another way to figure out the target thread, maybe we can give the renderer
> process an (opaque) ID, and the renderer can give it back to the plugin when
> calling SendUnsolicitedReply().

After discussing with Xians, we think the MediaStream API can not get data from
browser directly
without involving renderer process. (I wrote down those reasons in the doc).
So what we can do is optimizing the data path between renderer and plugin
process.
1, The first step is using ResourceReplyThreadRegistrar to avoid an extra thread
hopping (main -> target thread). I added a TODO for it. Will do it in a separate
CL.
2, If it still can not meet performance requirements, we will use a dedicated
audio thread and IO channel.

The CL is updated. PTAL. Thanks.

[yzshen1, 2014-01-29 21:28:05]

I haven't looked at the example.

Thanks!

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:38:
DCHECK_GE(index, 0);
Please also also DCHECK that this is called on the correct thread.

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:44:
base::subtle::NoBarrier_AtomicIncrement(&frames_, 1 << index);
This kind of code is, as the name suggested, subtle.
If there is no evidence that this is performance bottleneck, we should consider
using Lock. 

I haven't looked into details, but without barrier, how is memory visibility?

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:82: index ++;
no space in the middle, please.

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:87:
&(host_->frame_buffer()->GetFramePointer(index)->audio);
We are accessing the frame buffer from multiple threads without a lock. I saw
your comments on 73-76, but it has an implicit assumption about how frame buffer
is implemented.

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:99:
base::subtle::NoBarrier_AtomicIncrement(&frames_, -(1 << index));
what if index == 31?

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:104:
host_->SendEnqueueFrameMessageToPlugin(index);
Unless we said explicitly comment at the declaration that this method is thread
safe.

But I really don't think this is a good idea.

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:133: // sync
IPC. So we have to call |InitFrames()| from the main thread.
I have no problem with running InitFrames() on the main thread, but can we avoid
sync IPCs?

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:153: OnClose();
(I haven't read the relevant backend code, but want to make sure you have
considered that.)
Can we be sure that at this point, audio_sink_ is not used on the audio thread?
I think that requires MediaStreamAudioSink::RemoveFromAudioTrack() to be
blocking if there is any ongoing activity on the audio thread.

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_stream_audio_track_host.h (right):

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:33: public
base::SupportsWeakPtr<AudioSink> {
Please comment about on which thread weak pointers are dereferenced /
invalidates. (You probably have read the thread-safty note in weak_ptr.h)

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:38: void
EnqueueFrame(int32_t index);
On which thread it is called?

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:51: // Unowned
host which is available during the AudioSink's lifespan.
Please add comment about on which thread the member variables are accessed.

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:62: // The size
of frame pixels in bytes.
audio doesn't have pixels, right?

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:67:
base::subtle::Atomic32 frames_;
Do you mean the current implementation only supports up to 32 free frames?

https://codereview.chromium.org/140783004/diff/890001/ppapi/shared_impl/media...
File ppapi/shared_impl/media_stream_frame_buffer.h (right):

https://codereview.chromium.org/140783004/diff/890001/ppapi/shared_impl/media...
ppapi/shared_impl/media_stream_frame_buffer.h:44: virtual bool
OnNewFramePreEnqueued(int32_t index);
BeforeNewFrameEnqueued() may be more clearer.

That being said, I think the old way is easier to understand: we always enqueue
the frame index, and send a OnNewFrameEqueued() notification.
Why do we need such a change?

https://codereview.chromium.org/140783004/diff/890001/ppapi/thunk/ppb_audio_f...
File ppapi/thunk/ppb_audio_frame_api.h (right):

https://codereview.chromium.org/140783004/diff/890001/ppapi/thunk/ppb_audio_f...
ppapi/thunk/ppb_audio_frame_api.h:29: // Private APIs:
In Pepper, Private API has a different meaning. Maybe we could rephrase the
comment as "Methods used by Pepper internal implementation only."

[Peng, 2014-01-31 18:54:43]

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:38:
DCHECK_GE(index, 0);
On 2014/01/29 21:28:06, yzshen1 wrote:
> Please also also DCHECK that this is called on the correct thread.

Done.

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:44:
base::subtle::NoBarrier_AtomicIncrement(&frames_, 1 << index);
On 2014/01/29 21:28:06, yzshen1 wrote:
> This kind of code is, as the name suggested, subtle.
> If there is no evidence that this is performance bottleneck, we should
consider
> using Lock. 

Atomic's performance is slightly better than lock. And David said we should try
to 
avoid using lock. So I think Atomic is a good candidate. I am OK to use lock as
well.


> 
> I haven't looked into details, but without barrier, how is memory visibility?

As I understand, memory barrier is for forbidding out-of-order accessing. So it
is not necessary here.

See the example in http://en.wikipedia.org/wiki/Memory_barrier

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:82: index ++;
On 2014/01/29 21:28:06, yzshen1 wrote:
> no space in the middle, please.

Done.

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:87:
&(host_->frame_buffer()->GetFramePointer(index)->audio);
On 2014/01/29 21:28:06, yzshen1 wrote:
> We are accessing the frame buffer from multiple threads without a lock. I saw
> your comments on 73-76, but it has an implicit assumption about how frame
buffer
> is implemented.

Yes. After the frame_buffer() being initialized, the frame pointers will not be
changed.
So it is save to call GetFramePointer() in multiple threads. Any suggestion to
make
the code looks better?

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:99:
base::subtle::NoBarrier_AtomicIncrement(&frames_, -(1 << index));
On 2014/01/29 21:28:06, yzshen1 wrote:
> what if index == 31?

I think (1 << 32) will be 0x80000000. Because it is 32 bits integer, -(1 << 32)
will be 0x80000000 too. frames_ + 0x80000000 == frames_ - 0x80000000, when
frame_'s 31th bit is 1.

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:104:
host_->SendEnqueueFrameMessageToPlugin(index);
On 2014/01/29 21:28:06, yzshen1 wrote:
> Unless we said explicitly comment at the declaration that this method is
thread
> safe.
> 
> But I really don't think this is a good idea.

I understand. But I really don't have any good idea how to make it better.
Or PostTask to main thread to add another thread hopping.

WDYT?

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:133: // sync
IPC. So we have to call |InitFrames()| from the main thread.
On 2014/01/29 21:28:06, yzshen1 wrote:
> I have no problem with running InitFrames() on the main thread, but can we
avoid
> sync IPCs?

Currently, InitFrames() uses
content::RenderThread::HostAllocateSharedMemoryBuffer(size).
It is a sync function. Do you know how to do it in async way? Maybe it is better
to do
it in a separate CL as well. I added a TODO in
pepper_media_stream_track_host_base.cc.

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:153: OnClose();
On 2014/01/29 21:28:06, yzshen1 wrote:
> (I haven't read the relevant backend code, but want to make sure you have
> considered that.)
> Can we be sure that at this point, audio_sink_ is not used on the audio
thread?
> I think that requires MediaStreamAudioSink::RemoveFromAudioTrack() to be
> blocking if there is any ongoing activity on the audio thread.

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
I do see a lock is used in AddSink(), RemoveSink(), OnData() and OnSetFromant().
I assume it is thread safe. I think xians@ may give us more detail.

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_stream_audio_track_host.h (right):

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:33: public
base::SupportsWeakPtr<AudioSink> {
On 2014/01/29 21:28:06, yzshen1 wrote:
> Please comment about on which thread weak pointers are dereferenced /
> invalidates. (You probably have read the thread-safty note in weak_ptr.h) 

Done.

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:38: void
EnqueueFrame(int32_t index);
On 2014/01/29 21:28:06, yzshen1 wrote:
> On which thread it is called?

Done.

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:51: // Unowned
host which is available during the AudioSink's lifespan.
On 2014/01/29 21:28:06, yzshen1 wrote:
> Please add comment about on which thread the member variables are accessed.

I added comment for members which will be only accessed in the audio thread.

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:62: // The size
of frame pixels in bytes.
On 2014/01/29 21:28:06, yzshen1 wrote:
> audio doesn't have pixels, right?

Done.

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:67:
base::subtle::Atomic32 frames_;
On 2014/01/29 21:28:06, yzshen1 wrote:
> Do you mean the current implementation only supports up to 32 free frames?
Yes. I changed to Atomic64. I think it should be enough.

Currently, one audio frame is 10ms. with Atomic64, we could support up to 640ms
latency.
In my test, 1 buffer is enough for 99% cases and 3 buffers is good for all
cases.

https://codereview.chromium.org/140783004/diff/890001/ppapi/shared_impl/media...
File ppapi/shared_impl/media_stream_frame_buffer.h (right):

https://codereview.chromium.org/140783004/diff/890001/ppapi/shared_impl/media...
ppapi/shared_impl/media_stream_frame_buffer.h:44: virtual bool
OnNewFramePreEnqueued(int32_t index);
On 2014/01/29 21:28:06, yzshen1 wrote:
> BeforeNewFrameEnqueued() may be more clearer.
> 
> That being said, I think the old way is easier to understand: we always
enqueue
> the frame index, and send a OnNewFrameEqueued() notification.
> Why do we need such a change?

I though the performance is slightly better by avoiding an extra enqueue and
dequeue. Change it back. Done

https://codereview.chromium.org/140783004/diff/890001/ppapi/thunk/ppb_audio_f...
File ppapi/thunk/ppb_audio_frame_api.h (right):

https://codereview.chromium.org/140783004/diff/890001/ppapi/thunk/ppb_audio_f...
ppapi/thunk/ppb_audio_frame_api.h:29: // Private APIs:
On 2014/01/29 21:28:06, yzshen1 wrote:
> In Pepper, Private API has a different meaning. Maybe we could rephrase the
> comment as "Methods used by Pepper internal implementation only."

Done.

[yzshen1, 2014-01-31 19:50:25]

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:44:
base::subtle::NoBarrier_AtomicIncrement(&frames_, 1 << index);
On 2014/01/31 18:54:43, Peng wrote:
> On 2014/01/29 21:28:06, yzshen1 wrote:
> > This kind of code is, as the name suggested, subtle.
> > If there is no evidence that this is performance bottleneck, we should
> consider
> > using Lock. 
> 
> Atomic's performance is slightly better than lock. And David said we should
try
> to 
> avoid using lock. So I think Atomic is a good candidate. I am OK to use lock
as
> well.

IMO, the biggest reason that we avoid lock is to make things more intuitive and
less error prone. We use message loops + posting tasks to achieve that.
Here avoiding lock only makes the code harder to understand or modify. :/ If
performance is not an issue, I personally think using lock may be a good idea.

What do you think, Peng and David?

> > I haven't looked into details, but without barrier, how is memory
visibility?
> 
> As I understand, memory barrier is for forbidding out-of-order accessing. So
it
> is not necessary here.
> 
> See the example in http://en.wikipedia.org/wiki/Memory_barrier

I thought without memory barrier, theoretically it is possible that the other
threads could not observe the change for quite a while. (Imagine that the data
is cached separately for different cores.)

But my understanding of memory model is poor, so I am probably talking nonsense.
:)

[dmichael (off chromium), 2014-01-31 20:14:27]

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:44:
base::subtle::NoBarrier_AtomicIncrement(&frames_, 1 << index);
Yuzhu's exactly right; I was only concerned about Lock because it's usually
against Chromium style. I'm sorry I wasn't more clear.

See:
http://www.chromium.org/developers/design-documents/threading
(and also
http://www.chromium.org/developers/design-documents/threading/suble-threading...
)
The performance of lock is almost inconsequential here; the thread hop for
sending messages on the IO thread is much more expensive. We should aim for
"obviously correct".

In this case, after the discussions we've had, I think making Buffer a
thread-safe object is appropriate. The right way to do that is using base::Lock.

Again, sorry I was unclear before.

[bbudge, 2014-01-31 20:29:34]

Partial review, since it appears you may switch from atomics to locks.

https://codereview.chromium.org/140783004/diff/900001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/900001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:21: //
TODO(penghuang): make it configurable.
s/it/this

https://codereview.chromium.org/140783004/diff/900001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:32:
frames_(0ll),
This is confusing at first. Can we use 0LL?

https://codereview.chromium.org/140783004/diff/900001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:46: // Set the
bit for the frame index. Automic does not support and/or
s/Automic/Atomic

https://codereview.chromium.org/140783004/diff/900001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:48:
base::subtle::NoBarrier_AtomicIncrement(&frames_, 1 << index);
Using AtomicIncrement here makes me nervous, since if the bit is already set
somehow, we'd get a carry that could potentially propagate through several
frames. Even if this is correct, it is hard to understand, leading to bugs if
other developers change the code.

If you really want to use atomics, it might be better to use
NoBarrier_CompareAndSwap.

However, I agree with Yuzhu (and now David) that lock would be so much easier to
understand, and very fast if there is no contention, which seems likely.

https://codereview.chromium.org/140783004/diff/900001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_stream_audio_track_host.h (right):

https://codereview.chromium.org/140783004/diff/900001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:35: // This class
is created and destroyed in the plugin main thread.
s/in/on

https://codereview.chromium.org/140783004/diff/900001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:39: // Enquque a
frame in the main thread.
s/Enquque/Enqueue
s/in/on

https://codereview.chromium.org/140783004/diff/900001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:46: // Those
functions are called from the audio thread.
// These functions should be called on the audio thread.

https://codereview.chromium.org/140783004/diff/900001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:57: // It is
accessed in the audio thread.
// Access only on the audio thread.
and below.

https://codereview.chromium.org/140783004/diff/900001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:93: // True if it
has been added to |blink::WebMediaStreamTrack| as a sink.
it is a ambiguous here. Is it the host or the AudioSink?

[Peng, 2014-01-31 21:09:24]

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:44:
base::subtle::NoBarrier_AtomicIncrement(&frames_, 1 << index);
On 2014/01/31 20:14:28, dmichael wrote:
> Yuzhu's exactly right; I was only concerned about Lock because it's usually
> against Chromium style. I'm sorry I wasn't more clear.
> 
> See:
> http://www.chromium.org/developers/design-documents/threading
> (and also
>
http://www.chromium.org/developers/design-documents/threading/suble-threading...
> )
> The performance of lock is almost inconsequential here; the thread hop for
> sending messages on the IO thread is much more expensive. We should aim for
> "obviously correct".
> 
> In this case, after the discussions we've had, I think making Buffer a
> thread-safe object is appropriate. The right way to do that is using
base::Lock.
> 
> Again, sorry I was unclear before.

Done.

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:44:
base::subtle::NoBarrier_AtomicIncrement(&frames_, 1 << index);
On 2014/01/31 19:50:26, yzshen1 wrote:
> On 2014/01/31 18:54:43, Peng wrote:
> > On 2014/01/29 21:28:06, yzshen1 wrote:
> > > This kind of code is, as the name suggested, subtle.
> > > If there is no evidence that this is performance bottleneck, we should
> > consider
> > > using Lock. 
> > 
> > Atomic's performance is slightly better than lock. And David said we should
> try
> > to 
> > avoid using lock. So I think Atomic is a good candidate. I am OK to use lock
> as
> > well.
> 
> IMO, the biggest reason that we avoid lock is to make things more intuitive
and
> less error prone. We use message loops + posting tasks to achieve that.
> Here avoiding lock only makes the code harder to understand or modify. :/ If
> performance is not an issue, I personally think using lock may be a good idea.
> 
> What do you think, Peng and David?

Fixed by using base::Lock. Done

> 
> > > I haven't looked into details, but without barrier, how is memory
> visibility?
> > 
> > As I understand, memory barrier is for forbidding out-of-order accessing. So
> it
> > is not necessary here.
> > 
> > See the example in http://en.wikipedia.org/wiki/Memory_barrier
> 
> I thought without memory barrier, theoretically it is possible that the other
> threads could not observe the change for quite a while. (Imagine that the data
> is cached separately for different cores.)
> 
> But my understanding of memory model is poor, so I am probably talking
nonsense.
> :)

https://codereview.chromium.org/140783004/diff/900001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/900001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:21: //
TODO(penghuang): make it configurable.
On 2014/01/31 20:29:35, bbudge wrote:
> s/it/this

Done.

https://codereview.chromium.org/140783004/diff/900001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:32:
frames_(0ll),
On 2014/01/31 20:29:35, bbudge wrote:
> This is confusing at first. Can we use 0LL?

Done.

https://codereview.chromium.org/140783004/diff/900001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:46: // Set the
bit for the frame index. Automic does not support and/or
On 2014/01/31 20:29:35, bbudge wrote:
> s/Automic/Atomic

Done.

https://codereview.chromium.org/140783004/diff/900001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:48:
base::subtle::NoBarrier_AtomicIncrement(&frames_, 1 << index);
On 2014/01/31 20:29:35, bbudge wrote:
> Using AtomicIncrement here makes me nervous, since if the bit is already set
> somehow, we'd get a carry that could potentially propagate through several
> frames. Even if this is correct, it is hard to understand, leading to bugs if
> other developers change the code.
> 
> If you really want to use atomics, it might be better to use
> NoBarrier_CompareAndSwap.
> 
> However, I agree with Yuzhu (and now David) that lock would be so much easier
to
> understand, and very fast if there is no contention, which seems likely.

Fixed by using base::Lock

https://codereview.chromium.org/140783004/diff/900001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_stream_audio_track_host.h (right):

https://codereview.chromium.org/140783004/diff/900001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:35: // This class
is created and destroyed in the plugin main thread.
On 2014/01/31 20:29:35, bbudge wrote:
> s/in/on

Done.

https://codereview.chromium.org/140783004/diff/900001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:39: // Enquque a
frame in the main thread.
On 2014/01/31 20:29:35, bbudge wrote:
> s/Enquque/Enqueue
> s/in/on

Removed 'in the main thread', because I added a lock in AudioSink, so it is
thread safe now.

https://codereview.chromium.org/140783004/diff/900001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:46: // Those
functions are called from the audio thread.
On 2014/01/31 20:29:35, bbudge wrote:
> // These functions should be called on the audio thread.

Done.

https://codereview.chromium.org/140783004/diff/900001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:57: // It is
accessed in the audio thread.
On 2014/01/31 20:29:35, bbudge wrote:
> // Access only on the audio thread.
> and below.

Done.

https://codereview.chromium.org/140783004/diff/900001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:93: // True if it
has been added to |blink::WebMediaStreamTrack| as a sink.
On 2014/01/31 20:29:35, bbudge wrote:
> it is a ambiguous here. Is it the host or the AudioSink?

Done.

[bbudge, 2014-01-31 22:13:00]

Some comments. Otherwise, LGTM

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:46: uint32_t
number_of_frames, uint32_t frame_size) {
DCHECK_EQ(main_message_loop_proxy_, base::MessageLoopProxy::current());
?

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:115: int32_t
size = sizeof(ppapi::MediaStreamFrame::Audio) + frame_data_size_;
since sizeof is a size_t, this may not compile on all platforms.

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_audio_track_host.h (right):

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:72: // A mutex to
portect the index queue |frames_|.
s/portect/protect

Can we name this lock_ to be more consistent with other Chromium usages?

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:75: // A queue
for free frame indexes.
s/indexes/indices

https://codereview.chromium.org/140783004/diff/1000001/ppapi/examples/media_s...
File ppapi/examples/media_stream_audio/media_stream_audio.cc (right):

https://codereview.chromium.org/140783004/diff/1000001/ppapi/examples/media_s...
ppapi/examples/media_stream_audio/media_stream_audio.cc:119:
*image.GetAddr32(pp::Point(x, y)) = 0xff202020;
It might be nice to define constants to indicate what colors these represent
e.g. kDarkGray.

https://codereview.chromium.org/140783004/diff/1000001/ppapi/examples/media_s...
ppapi/examples/media_stream_audio/media_stream_audio.cc:135: x++, i +=
channel_count_) {
Could we draw all the channels?

https://codereview.chromium.org/140783004/diff/1000001/ppapi/examples/media_s...
ppapi/examples/media_stream_audio/media_stream_audio.cc:144: // Callback that is
invoked when new frames are recevied.
s/recevied/received

[no longer working on chromium, 2014-02-01 21:28:08]

Driven-by.

Regarding to the MediaStreamAudioSink thread safe, after calling
MediaStreamAudioSink::RemoveFromAudioTrack(), there won't be more callback to
the AudioSink.

SX

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:38: void
PepperMediaStreamAudioTrackHost::AudioSink::EnqueueFrame(int32_t index) {
The thread model is very complicated in this class, please add thread check to
all methods.
For the audio thread, you can capture_thread_checker_.DetachFromThread() on
PepperMediaStreamAudioTrackHost::AudioSink::OnSetFormat, and
DCHECK(capture_thread_checker_.CalledOnValidThread()) on
PepperMediaStreamAudioTrackHost::AudioSink::OnData

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:46: uint32_t
number_of_frames, uint32_t frame_size) {
The number_of_frames here and  number_of_frames in AudioSink::OnData mean
different things. Can you change the name to remove the confusion, for example,
number_of_buffers,  buffer_size

Also do you really need uint32_t? how about size_t?

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:76:
ppapi::MediaStreamFrame::Audio* frame =
in OnSetFormat() you post a task InitFramesOnMainThread on the main render
thread and call InitFrames there, but before the task is executed, Ondata with
the new format might have come. Is it a problem if host_->frame_buffer() is
still using the old format?

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:92: timestamp_
+= frame_duration_;
nit, you don't need this frame_duration_ member, frame_duration_ equals to
number_of_frames in the callback.

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:118: // sync
IPC. So we have to call |InitFrames()| from the main thread.
I think you need some synchronization between OnSetFormat and
InitFramesOnMainThread. Probably you should drop the audio buffer until the
buffer in the main thread is setup properly.
For example, empty the frames_ here, and set it up in InitFramesOnMainThread

[Peng, 2014-02-02 02:11:25]

CL is updated. PTAL. Thanks.

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:38: void
PepperMediaStreamAudioTrackHost::AudioSink::EnqueueFrame(int32_t index) {
On 2014/02/01 21:28:09, xians1 wrote:
> The thread model is very complicated in this class, please add thread check to
> all methods.
> For the audio thread, you can capture_thread_checker_.DetachFromThread() on
> PepperMediaStreamAudioTrackHost::AudioSink::OnSetFormat, and
> DCHECK(capture_thread_checker_.CalledOnValidThread()) on
> PepperMediaStreamAudioTrackHost::AudioSink::OnData

Done.

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:46: uint32_t
number_of_frames, uint32_t frame_size) {
On 2014/01/31 22:13:01, bbudge wrote:
> DCHECK_EQ(main_message_loop_proxy_, base::MessageLoopProxy::current());
> ?

Done.

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:46: uint32_t
number_of_frames, uint32_t frame_size) {
On 2014/02/01 21:28:09, xians1 wrote:
> The number_of_frames here and  number_of_frames in AudioSink::OnData mean
> different things. Can you change the name to remove the confusion, for
example,
> number_of_buffers,  buffer_size

Because the base class is used for both Audio and Video Track. So we decided use
name AudioFrame and VideoFrame. So everywhere in audio and video tracks, we use
frame. But after reading MediaStreamAudioSink. I understand buffer is better
name than frame here. So I think it is better to rename InitFrames of the base
class to InitBuffers, and AudioFrame to AudioBuffer, but keep VideoFrame, etc.
But I would like to do it in a separate CL, because it need change base classes
as well. WDYT?


> 
> Also do you really need uint32_t? how about size_t?
I changed it to int32_t, because this function call base class's InitFrames()
which takes int32_t.

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:76:
ppapi::MediaStreamFrame::Audio* frame =
On 2014/02/01 21:28:09, xians1 wrote:
> in OnSetFormat() you post a task InitFramesOnMainThread on the main render
> thread and call InitFrames there, but before the task is executed, Ondata with
> the new format might have come. Is it a problem if host_->frame_buffer() is
> still using the old format?

I remember you told me the OnSetFormat will only be called once. So before
InitFrameOnMianThead() being finished, the index will be always -1, So we will
simply drop all data until underlying buffer is ready.
It should not have any problem, right?

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:92: timestamp_
+= frame_duration_;
On 2014/02/01 21:28:09, xians1 wrote:
> nit, you don't need this frame_duration_ member, frame_duration_ equals to
> number_of_frames in the callback.

I think we need calculate duration from the number_of_frames base on the sample
rate.
I think the number_of_frames is always a same value. So probably using an extra
can has slightly better performance. ?

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:115: int32_t
size = sizeof(ppapi::MediaStreamFrame::Audio) + frame_data_size_;
On 2014/01/31 22:13:01, bbudge wrote:
> since sizeof is a size_t, this may not compile on all platforms.

Done.

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:118: // sync
IPC. So we have to call |InitFrames()| from the main thread.
On 2014/02/01 21:28:09, xians1 wrote:
> I think you need some synchronization between OnSetFormat and
> InitFramesOnMainThread. Probably you should drop the audio buffer until the
> buffer in the main thread is setup properly.
> For example, empty the frames_ here, and set it up in InitFramesOnMainThread

I assume this function will be only called once. And InitFrameOnMianThread()
uses a lock
to protect the frame queue |frames_|. and the |frames_| will be always empty
before InitFrameOnMainThread is finished. So it should be OK.

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_audio_track_host.h (right):

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:72: // A mutex to
portect the index queue |frames_|.
On 2014/01/31 22:13:01, bbudge wrote:
> s/portect/protect
> 
> Can we name this lock_ to be more consistent with other Chromium usages?

Done.

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:75: // A queue
for free frame indexes.
On 2014/01/31 22:13:01, bbudge wrote:
> s/indexes/indices

Done.

https://codereview.chromium.org/140783004/diff/1000001/ppapi/examples/media_s...
File ppapi/examples/media_stream_audio/media_stream_audio.cc (right):

https://codereview.chromium.org/140783004/diff/1000001/ppapi/examples/media_s...
ppapi/examples/media_stream_audio/media_stream_audio.cc:119:
*image.GetAddr32(pp::Point(x, y)) = 0xff202020;
On 2014/01/31 22:13:01, bbudge wrote:
> It might be nice to define constants to indicate what colors these represent
> e.g. kDarkGray.

Done.

https://codereview.chromium.org/140783004/diff/1000001/ppapi/examples/media_s...
ppapi/examples/media_stream_audio/media_stream_audio.cc:135: x++, i +=
channel_count_) {
On 2014/01/31 22:13:01, bbudge wrote:
> Could we draw all the channels?

Done.

https://codereview.chromium.org/140783004/diff/1000001/ppapi/examples/media_s...
ppapi/examples/media_stream_audio/media_stream_audio.cc:144: // Callback that is
invoked when new frames are recevied.
On 2014/01/31 22:13:01, bbudge wrote:
> s/recevied/received

Done.

[no longer working on chromium, 2014-02-03 09:50:43]

lgtm to the audio track sink code with a few nits, please address them.

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:46: uint32_t
number_of_frames, uint32_t frame_size) {
On 2014/02/02 02:11:26, Peng wrote:
> On 2014/02/01 21:28:09, xians1 wrote:
> > The number_of_frames here and  number_of_frames in AudioSink::OnData mean
> > different things. Can you change the name to remove the confusion, for
> example,
> > number_of_buffers,  buffer_size
> 
> Because the base class is used for both Audio and Video Track. So we decided
use
> name AudioFrame and VideoFrame. So everywhere in audio and video tracks, we
use
> frame. But after reading MediaStreamAudioSink. I understand buffer is better
> name than frame here. So I think it is better to rename InitFrames of the base
> class to InitBuffers, and AudioFrame to AudioBuffer, but keep VideoFrame, etc.
> But I would like to do it in a separate CL, because it need change base
classes
> as well. WDYT?
> 

sgtm.


> 
> > 
> > Also do you really need uint32_t? how about size_t?
> I changed it to int32_t, because this function call base class's InitFrames()
> which takes int32_t.
>

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:46: uint32_t
number_of_frames, uint32_t frame_size) {
On 2014/02/02 02:11:26, Peng wrote:
> On 2014/02/01 21:28:09, xians1 wrote:
> > The number_of_frames here and  number_of_frames in AudioSink::OnData mean
> > different things. Can you change the name to remove the confusion, for
> example,
> > number_of_buffers,  buffer_size
> 
> Because the base class is used for both Audio and Video Track. So we decided
use
> name AudioFrame and VideoFrame. So everywhere in audio and video tracks, we
use
> frame. But after reading MediaStreamAudioSink. I understand buffer is better
> name than frame here. So I think it is better to rename InitFrames of the base
> class to InitBuffers, and AudioFrame to AudioBuffer, but keep VideoFrame, etc.
> But I would like to do it in a separate CL, because it need change base
classes
> as well. WDYT?
> 
> 
> > 
> > Also do you really need uint32_t? how about size_t?
> I changed it to int32_t, because this function call base class's InitFrames()
> which takes int32_t.
> 

Curiously, does the overall pepper code prefer int32_t over int?

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:76:
ppapi::MediaStreamFrame::Audio* frame =
On 2014/02/02 02:11:26, Peng wrote:
> On 2014/02/01 21:28:09, xians1 wrote:
> > in OnSetFormat() you post a task InitFramesOnMainThread on the main render
> > thread and call InitFrames there, but before the task is executed, Ondata
with
> > the new format might have come. Is it a problem if host_->frame_buffer() is
> > still using the old format?
> 
> I remember you told me the OnSetFormat will only be called once. So before
> InitFrameOnMianThead() being finished, the index will be always -1, So we will
> simply drop all data until underlying buffer is ready.
> It should not have any problem, right?
> 

No, today this can be called more than once. Everytime the format of the audio
data has changed, it will trigger OnSetFormat. But OnSetFormat won't be called
frequently.

Probably in the future we can make sure OnSetFormat can be called once, but not
now.

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:92: timestamp_
+= frame_duration_;
On 2014/02/02 02:11:26, Peng wrote:
> On 2014/02/01 21:28:09, xians1 wrote:
> > nit, you don't need this frame_duration_ member, frame_duration_ equals to
> > number_of_frames in the callback.
> 
> I think we need calculate duration from the number_of_frames base on the
sample
> rate.
> I think the number_of_frames is always a same value. So probably using an
extra
> can has slightly better performance. ?
> 

OK, it is fine to use the member then.

https://codereview.chromium.org/140783004/diff/1060001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/1060001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:38: void
PepperMediaStreamAudioTrackHost::AudioSink::EnqueueFrame(int32_t index) {
thread check for this method as well?

https://codereview.chromium.org/140783004/diff/1060001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_audio_track_host.h (right):

https://codereview.chromium.org/140783004/diff/1060001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:1: // Copyright
(c) 2014 The Chromium Authors. All rights reserved.
nit, %s/Copyright (c) 2014/Copyright 2014/g

https://codereview.chromium.org/140783004/diff/1060001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:42: void
EnqueueFrame(int32_t index);
please add comment to explain how this method is used.

[Peng, 2014-02-03 14:38:53]

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:46: uint32_t
number_of_frames, uint32_t frame_size) {
On 2014/02/03 09:50:44, xians1 wrote:
> On 2014/02/02 02:11:26, Peng wrote:
> > On 2014/02/01 21:28:09, xians1 wrote:
> > > The number_of_frames here and  number_of_frames in AudioSink::OnData mean
> > > different things. Can you change the name to remove the confusion, for
> > example,
> > > number_of_buffers,  buffer_size
> > 
> > Because the base class is used for both Audio and Video Track. So we decided
> use
> > name AudioFrame and VideoFrame. So everywhere in audio and video tracks, we
> use
> > frame. But after reading MediaStreamAudioSink. I understand buffer is better
> > name than frame here. So I think it is better to rename InitFrames of the
base
> > class to InitBuffers, and AudioFrame to AudioBuffer, but keep VideoFrame,
etc.
> > But I would like to do it in a separate CL, because it need change base
> classes
> > as well. WDYT?
> > 
> > 
> > > 
> > > Also do you really need uint32_t? how about size_t?
> > I changed it to int32_t, because this function call base class's
InitFrames()
> > which takes int32_t.
> > 
> 
> Curiously, does the overall pepper code prefer int32_t over int?

I think so. I did not find size_t in ppapi/api.

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:76:
ppapi::MediaStreamFrame::Audio* frame =
On 2014/02/03 09:50:44, xians1 wrote:
> On 2014/02/02 02:11:26, Peng wrote:
> > On 2014/02/01 21:28:09, xians1 wrote:
> > > in OnSetFormat() you post a task InitFramesOnMainThread on the main render
> > > thread and call InitFrames there, but before the task is executed, Ondata
> with
> > > the new format might have come. Is it a problem if host_->frame_buffer()
is
> > > still using the old format?
> > 
> > I remember you told me the OnSetFormat will only be called once. So before
> > InitFrameOnMianThead() being finished, the index will be always -1, So we
will
> > simply drop all data until underlying buffer is ready.
> > It should not have any problem, right?
> > 
> 
> No, today this can be called more than once. Everytime the format of the audio
> data has changed, it will trigger OnSetFormat. But OnSetFormat won't be called
> frequently.
> 
> Probably in the future we can make sure OnSetFormat can be called once, but
not
> now.

That is a bad news. Could you give me some real cases for the change?

Right now, the base class does not support re-allocating underlying shared
memory. I am think could we allocate the buffer with bigger frame size which is
big enough for all kinds of formats. So we can avoid re-allocating shared
memory.

https://codereview.chromium.org/140783004/diff/1060001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/1060001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:38: void
PepperMediaStreamAudioTrackHost::AudioSink::EnqueueFrame(int32_t index) {
On 2014/02/03 09:50:44, xians1 wrote:
> thread check for this method as well?

Done.

https://codereview.chromium.org/140783004/diff/1060001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_audio_track_host.h (right):

https://codereview.chromium.org/140783004/diff/1060001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:1: // Copyright
(c) 2014 The Chromium Authors. All rights reserved.
On 2014/02/03 09:50:44, xians1 wrote:
> nit, %s/Copyright (c) 2014/Copyright 2014/g

Done.

https://codereview.chromium.org/140783004/diff/1060001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:42: void
EnqueueFrame(int32_t index);
On 2014/02/03 09:50:44, xians1 wrote:
> please add comment to explain how this method is used.

Done.

[yzshen1, 2014-02-03 18:14:35]

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:133: // sync
IPC. So we have to call |InitFrames()| from the main thread.
Ah, I see. :)

I thought you meant that you added new sync IPCs. I am fine with
HostAllocateSharedMemoryBuffer().

On 2014/01/31 18:54:43, Peng wrote:
> On 2014/01/29 21:28:06, yzshen1 wrote:
> > I have no problem with running InitFrames() on the main thread, but can we
> avoid
> > sync IPCs?
> 
> Currently, InitFrames() uses
> content::RenderThread::HostAllocateSharedMemoryBuffer(size).
> It is a sync function. Do you know how to do it in async way? Maybe it is
better
> to do
> it in a separate CL as well. I added a TODO in
> pepper_media_stream_track_host_base.cc.
>

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_stream_audio_track_host.h (right):

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:33: public
base::SupportsWeakPtr<AudioSink> {
I couldn't find the corresponding change.
On 2014/01/31 18:54:43, Peng wrote:
> On 2014/01/29 21:28:06, yzshen1 wrote:
> > Please comment about on which thread weak pointers are dereferenced /
> > invalidates. (You probably have read the thread-safty note in weak_ptr.h) 
> 
> Done.

https://codereview.chromium.org/140783004/diff/1450001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/1450001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:88:
memcpy(frame->data, audio_data, frame_data_size_);
Can we check that |frame->data| is big enough to store the data of size
|frame_data_size_|? We need to fail gracefully or die directly. DCHECK on line
50 is not sufficient.

(Do we need to also check whether |audio_data| has that much data?)

https://codereview.chromium.org/140783004/diff/1450001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:90: // This
function is called from the audio thread, but
We cannot assume SendEnqueueFrameMessageToPlugin() is thread safe.
For example, we don't guarantee that the IPC::Sender interface that PpapiHost
uses is thread safe.
The current impl "happens to" be safe doesn't mean we can rely on it, IMO. If we
want to go down this direction, we need to make it explicit and formally
supported.

I cannot think up a better way than asking the main thread to do the send. :/

https://codereview.chromium.org/140783004/diff/1450001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_audio_track_host.h (right):

https://codereview.chromium.org/140783004/diff/1450001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:42: // Enqueue a
free frame index into |frames_| which will be used for
Enqueue*s*

https://codereview.chromium.org/140783004/diff/1450001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:58:
PepperMediaStreamAudioTrackHost* host_;
This should only be accessed from the main thread, right?

https://codereview.chromium.org/140783004/diff/1450001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:83:
base::ThreadChecker capture_thread_checker_;
Is capture_thread == audio thread mentioned above? It seems better to make the
name consistent.

[Peng, 2014-02-03 19:21:06]

CL is updated. PTAL. Thanks.

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_stream_audio_track_host.h (right):

https://codereview.chromium.org/140783004/diff/890001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:33: public
base::SupportsWeakPtr<AudioSink> {
On 2014/02/03 18:14:36, yzshen1 wrote:
> I couldn't find the corresponding change.
> On 2014/01/31 18:54:43, Peng wrote:
> > On 2014/01/29 21:28:06, yzshen1 wrote:
> > > Please comment about on which thread weak pointers are dereferenced /
> > > invalidates. (You probably have read the thread-safty note in weak_ptr.h) 
> > 
> > Done.
> 

I added comments about the constructor and the destructor.
'This class is created and destroyed on the plugin main thread.' so the weak
pointers will be dereferenced and invalidated on the plugin main thread too,
right?

Move the comment to class level.

https://codereview.chromium.org/140783004/diff/1450001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/1450001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:88:
memcpy(frame->data, audio_data, frame_data_size_);
On 2014/02/03 18:14:36, yzshen1 wrote:
> Can we check that |frame->data| is big enough to store the data of size
> |frame_data_size_|? We need to fail gracefully or die directly. DCHECK on line
> 50 is not sufficient.
> 
> (Do we need to also check whether |audio_data| has that much data?)

Add DCHECK_GE(host_->frame_buffer()->frame_size(), frame_size) in
InitFramesOnMainThread().
Done

https://codereview.chromium.org/140783004/diff/1450001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:90: // This
function is called from the audio thread, but
On 2014/02/03 18:14:36, yzshen1 wrote:
> We cannot assume SendEnqueueFrameMessageToPlugin() is thread safe.
> For example, we don't guarantee that the IPC::Sender interface that PpapiHost
> uses is thread safe.
> The current impl "happens to" be safe doesn't mean we can rely on it, IMO. If
we
> want to go down this direction, we need to make it explicit and formally
> supported.
> 
> I cannot think up a better way than asking the main thread to do the send. :/

Done.

https://codereview.chromium.org/140783004/diff/1450001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_audio_track_host.h (right):

https://codereview.chromium.org/140783004/diff/1450001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:42: // Enqueue a
free frame index into |frames_| which will be used for
On 2014/02/03 18:14:36, yzshen1 wrote:
> Enqueue*s*

Done.

https://codereview.chromium.org/140783004/diff/1450001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:58:
PepperMediaStreamAudioTrackHost* host_;
On 2014/02/03 18:14:36, yzshen1 wrote:
> This should only be accessed from the main thread, right?

Except for reading frame_buffer(), to get frame_size, frame_pointer, etc.

https://codereview.chromium.org/140783004/diff/1450001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:83:
base::ThreadChecker capture_thread_checker_;
On 2014/02/03 18:14:36, yzshen1 wrote:
> Is capture_thread == audio thread mentioned above? It seems better to make the
> name consistent.

Done.

[no longer working on chromium, 2014-02-03 20:23:59]

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:76:
ppapi::MediaStreamFrame::Audio* frame =
On 2014/02/03 14:38:54, Peng wrote:
> On 2014/02/03 09:50:44, xians1 wrote:
> > On 2014/02/02 02:11:26, Peng wrote:
> > > On 2014/02/01 21:28:09, xians1 wrote:
> > > > in OnSetFormat() you post a task InitFramesOnMainThread on the main
render
> > > > thread and call InitFrames there, but before the task is executed,
Ondata
> > with
> > > > the new format might have come. Is it a problem if host_->frame_buffer()
> is
> > > > still using the old format?
> > > 
> > > I remember you told me the OnSetFormat will only be called once. So before
> > > InitFrameOnMianThead() being finished, the index will be always -1, So we
> will
> > > simply drop all data until underlying buffer is ready.
> > > It should not have any problem, right?
> > > 
> > 
> > No, today this can be called more than once. Everytime the format of the
audio
> > data has changed, it will trigger OnSetFormat. But OnSetFormat won't be
called
> > frequently.
> > 
> > Probably in the future we can make sure OnSetFormat can be called once, but
> not
> > now.
> 
> That is a bad news. Could you give me some real cases for the change?
> 
> Right now, the base class does not support re-allocating underlying shared
> memory. I am think could we allocate the buffer with bigger frame size which
is
> big enough for all kinds of formats. So we can avoid re-allocating shared
> memory.

Each time if a WebRtcAudioCapturer::SetCapturerSource is called, it will trigger
the SetFormat.
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

You can work around the problem if you can allocate a buffer of 10ms data, that
is the the maximum buffer size of a packet that the sink will get.
I would suggest you add a CHECK here to make sure it is the case.

In the future, I am going to refactor the code to make sure SetFormat will be
called on once.

[Peng, 2014-02-03 21:57:46]

On 2014/02/03 20:23:59, xians1 wrote:
>
https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
> File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):
> 
>
https://codereview.chromium.org/140783004/diff/1000001/content/renderer/peppe...
> content/renderer/pepper/pepper_media_stream_audio_track_host.cc:76:
> ppapi::MediaStreamFrame::Audio* frame =
> On 2014/02/03 14:38:54, Peng wrote:
> > On 2014/02/03 09:50:44, xians1 wrote:
> > > On 2014/02/02 02:11:26, Peng wrote:
> > > > On 2014/02/01 21:28:09, xians1 wrote:
> > > > > in OnSetFormat() you post a task InitFramesOnMainThread on the main
> render
> > > > > thread and call InitFrames there, but before the task is executed,
> Ondata
> > > with
> > > > > the new format might have come. Is it a problem if
host_->frame_buffer()
> > is
> > > > > still using the old format?
> > > > 
> > > > I remember you told me the OnSetFormat will only be called once. So
before
> > > > InitFrameOnMianThead() being finished, the index will be always -1, So
we
> > will
> > > > simply drop all data until underlying buffer is ready.
> > > > It should not have any problem, right?
> > > > 
> > > 
> > > No, today this can be called more than once. Everytime the format of the
> audio
> > > data has changed, it will trigger OnSetFormat. But OnSetFormat won't be
> called
> > > frequently.
> > > 
> > > Probably in the future we can make sure OnSetFormat can be called once,
but
> > not
> > > now.
> > 
> > That is a bad news. Could you give me some real cases for the change?
> > 
> > Right now, the base class does not support re-allocating underlying shared
> > memory. I am think could we allocate the buffer with bigger frame size which
> is
> > big enough for all kinds of formats. So we can avoid re-allocating shared
> > memory.
> 
> Each time if a WebRtcAudioCapturer::SetCapturerSource is called, it will
trigger
> the SetFormat.
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
> 
> You can work around the problem if you can allocate a buffer of 10ms data,
that
> is the the maximum buffer size of a packet that the sink will get.
> I would suggest you add a CHECK here to make sure it is the case.
> 
> In the future, I am going to refactor the code to make sure SetFormat will be
> called on once.

xians told me offline the |OnSetFormat()| could be called more than once. But
only the duration of audio buffer can be changed, and the max value of duration
is 10ms. So I updated the CL to always allocate buffer with 10ms duration. If
OnSetFormat() is called again, we will reused allocated underlying buffer
instead of re-allocate a new one.

PTAL. Thanks.

[dmichael (off chromium), 2014-02-03 22:18:33]

lgtm, thank you!

https://codereview.chromium.org/140783004/diff/1750001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/1750001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:114:
COMPILE_ASSERT(AudioParameters::kTelephoneSampleRate == 8000,
Would it be better to compare to PP_AUDIOFRAME_SAMPLERATE_8000 and
PP_AUDIOFRAME_SAMPLERATE_44100?

https://codereview.chromium.org/140783004/diff/1750001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_audio_track_host.h (right):

https://codereview.chromium.org/140783004/diff/1750001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:37: public
base::SupportsWeakPtr<AudioSink> {
It looks like you're only getting AudioSink WeakPtrs from within AudioSink
itself. Please use WeakPtrFactory instead, to avoid exposing WeakPtrs beyond
where they are needed.

https://codereview.chromium.org/140783004/diff/1750001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:44: void
EnqueueFrame(int32_t index);
please note what thread(s) this is callable on

https://codereview.chromium.org/140783004/diff/1780001/ppapi/examples/media_s...
File ppapi/examples/media_stream_audio/media_stream_audio.cc (right):

https://codereview.chromium.org/140783004/diff/1780001/ppapi/examples/media_s...
ppapi/examples/media_stream_audio/media_stream_audio.cc:28: 
How about a high-level comment about what the example does?

https://codereview.chromium.org/140783004/diff/1780001/ppapi/examples/media_s...
ppapi/examples/media_stream_audio/media_stream_audio.cc:183: int16_t* samples_;
std::vector?

https://codereview.chromium.org/140783004/diff/1780001/ppapi/proxy/media_stre...
File ppapi/proxy/media_stream_audio_track_resource.cc (right):

https://codereview.chromium.org/140783004/diff/1780001/ppapi/proxy/media_stre...
ppapi/proxy/media_stream_audio_track_resource.cc:87:
DCHECK(frame_resource->GetFrameBufferIndex() >= 0);
Why not DCHECK_GE?

https://codereview.chromium.org/140783004/diff/1780001/ppapi/proxy/ppapi_mess...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/140783004/diff/1780001/ppapi/proxy/ppapi_mess...
ppapi/proxy/ppapi_messages.h:1442: // Message for init frames. It also takes a
shared memory handle.
Might be clearer to explain that the shared memory handle is put in the outer
ResourceReplyMessage

[Peng, 2014-02-03 22:50:29]

https://codereview.chromium.org/140783004/diff/1750001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/1750001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:114:
COMPILE_ASSERT(AudioParameters::kTelephoneSampleRate == 8000,
On 2014/02/03 22:18:34, dmichael wrote:
> Would it be better to compare to PP_AUDIOFRAME_SAMPLERATE_8000 and
> PP_AUDIOFRAME_SAMPLERATE_44100?
There are build errors for comparing two enums. So I used

https://codereview.chromium.org/140783004/diff/1750001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_audio_track_host.h (right):

https://codereview.chromium.org/140783004/diff/1750001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:37: public
base::SupportsWeakPtr<AudioSink> {
On 2014/02/03 22:18:34, dmichael wrote:
> It looks like you're only getting AudioSink WeakPtrs from within AudioSink
> itself. Please use WeakPtrFactory instead, to avoid exposing WeakPtrs beyond
> where they are needed.

Done.

https://codereview.chromium.org/140783004/diff/1750001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:44: void
EnqueueFrame(int32_t index);
On 2014/02/03 22:18:34, dmichael wrote:
> please note what thread(s) this is callable on

Done.

https://codereview.chromium.org/140783004/diff/1780001/ppapi/examples/media_s...
File ppapi/examples/media_stream_audio/media_stream_audio.cc (right):

https://codereview.chromium.org/140783004/diff/1780001/ppapi/examples/media_s...
ppapi/examples/media_stream_audio/media_stream_audio.cc:28: 
On 2014/02/03 22:18:34, dmichael wrote:
> How about a high-level comment about what the example does?

Done.

https://codereview.chromium.org/140783004/diff/1780001/ppapi/examples/media_s...
ppapi/examples/media_stream_audio/media_stream_audio.cc:183: int16_t* samples_;
On 2014/02/03 22:18:34, dmichael wrote:
> std::vector?

Done.

https://codereview.chromium.org/140783004/diff/1780001/ppapi/proxy/media_stre...
File ppapi/proxy/media_stream_audio_track_resource.cc (right):

https://codereview.chromium.org/140783004/diff/1780001/ppapi/proxy/media_stre...
ppapi/proxy/media_stream_audio_track_resource.cc:87:
DCHECK(frame_resource->GetFrameBufferIndex() >= 0);
On 2014/02/03 22:18:34, dmichael wrote:
> Why not DCHECK_GE?

Done.

https://codereview.chromium.org/140783004/diff/1780001/ppapi/proxy/ppapi_mess...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/140783004/diff/1780001/ppapi/proxy/ppapi_mess...
ppapi/proxy/ppapi_messages.h:1442: // Message for init frames. It also takes a
shared memory handle.
On 2014/02/03 22:18:34, dmichael wrote:
> Might be clearer to explain that the shared memory handle is put in the outer
> ResourceReplyMessage

Done.

[Peng, 2014-02-04 18:20:58]

https://codereview.chromium.org/140783004/diff/1750001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/1750001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:114:
COMPILE_ASSERT(AudioParameters::kTelephoneSampleRate == 8000,
On 2014/02/03 22:50:31, Peng wrote:
> On 2014/02/03 22:18:34, dmichael wrote:
> > Would it be better to compare to PP_AUDIOFRAME_SAMPLERATE_8000 and
> > PP_AUDIOFRAME_SAMPLERATE_44100?
> There are build errors for comparing two enums. So I used
> 

Done.

[yzshen1, 2014-02-04 18:26:12]

Only a few more nits. Thanks!

https://codereview.chromium.org/140783004/diff/1450001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_audio_track_host.h (right):

https://codereview.chromium.org/140783004/diff/1450001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:58:
PepperMediaStreamAudioTrackHost* host_;
On 2014/02/03 19:21:08, Peng wrote:
> On 2014/02/03 18:14:36, yzshen1 wrote:
> > This should only be accessed from the main thread, right?
> 
> Except for reading frame_buffer(), to get frame_size, frame_pointer, etc.

Please explicitly talk about it here and at the declaration of frame_buffer in
pepper_media_stream_track_host_base, explaining what is done and why it is okay.

This is something that won't be obvious for those who need to read / maintain
the code later.

https://codereview.chromium.org/140783004/diff/1880001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/1880001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:55:
DCHECK_GE(host_->frame_buffer()->frame_size(), frame_size);
DCHECK is probably not sufficient: if somehow InitFrames() fails in release
builds, the code may continue running while frame_buffer()->frame_size() <
frame_size, and then we have buffer overflow on line 100.

As I said, we should either fail gracefully or die directly.

https://codereview.chromium.org/140783004/diff/2180001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_audio_track_host.h (right):

https://codereview.chromium.org/140783004/diff/2180001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:90:
base::WeakPtrFactory<AudioSink> weak_factory_;
nit: it is better to place |weak_factory_| as the last member of the class.

https://codereview.chromium.org/140783004/diff/2180001/ppapi/examples/media_s...
File ppapi/examples/media_stream_audio/media_stream_audio.cc (right):

https://codereview.chromium.org/140783004/diff/2180001/ppapi/examples/media_s...
ppapi/examples/media_stream_audio/media_stream_audio.cc:35: uint32_t kColorRed =
0xFFFF0000;
nit: please use const.

https://codereview.chromium.org/140783004/diff/2180001/ppapi/examples/media_s...
ppapi/examples/media_stream_audio/media_stream_audio.cc:159: void
OnGetFrame(int32_t result, pp::AudioFrame frame) {
nit: You could also use pp::AudioFrame&.

https://codereview.chromium.org/140783004/diff/2180001/ppapi/examples/media_s...
ppapi/examples/media_stream_audio/media_stream_audio.cc:164: if
(samples_.empty()) {
Do we need to consider that frames may change configuration in the middle of the
stream?

Because developers might read the example and assume that the configuration
never changes. We should at least comment about it.

https://codereview.chromium.org/140783004/diff/2180001/ppapi/examples/media_s...
ppapi/examples/media_stream_audio/media_stream_audio.cc:175: sample_count_ *
channel_count_ * sizeof(int16_t));
Do we need to make sure GetSampleSize() agrees with int16_t?

[Peng, 2014-02-04 19:57:30]

CL is updated. PTAL. Thanks.

https://codereview.chromium.org/140783004/diff/1450001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_audio_track_host.h (right):

https://codereview.chromium.org/140783004/diff/1450001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:58:
PepperMediaStreamAudioTrackHost* host_;
On 2014/02/04 18:26:13, yzshen1 wrote:
> On 2014/02/03 19:21:08, Peng wrote:
> > On 2014/02/03 18:14:36, yzshen1 wrote:
> > > This should only be accessed from the main thread, right?
> > 
> > Except for reading frame_buffer(), to get frame_size, frame_pointer, etc.
> 
> Please explicitly talk about it here and at the declaration of frame_buffer in
> pepper_media_stream_track_host_base, explaining what is done and why it is
okay.
> 
> This is something that won't be obvious for those who need to read / maintain
> the code later.

Done.

https://codereview.chromium.org/140783004/diff/1880001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/140783004/diff/1880001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:55:
DCHECK_GE(host_->frame_buffer()->frame_size(), frame_size);
On 2014/02/04 18:26:13, yzshen1 wrote:
> DCHECK is probably not sufficient: if somehow InitFrames() fails in release
> builds, the code may continue running while frame_buffer()->frame_size() <
> frame_size, and then we have buffer overflow on line 100.
> 
> As I said, we should either fail gracefully or die directly.

Sorry. I misunderstood your comment. I changed DCHECK to CHECK and added a TODO.

https://codereview.chromium.org/140783004/diff/2180001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_audio_track_host.h (right):

https://codereview.chromium.org/140783004/diff/2180001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:90:
base::WeakPtrFactory<AudioSink> weak_factory_;
On 2014/02/04 18:26:13, yzshen1 wrote:
> nit: it is better to place |weak_factory_| as the last member of the class.

Done.

https://codereview.chromium.org/140783004/diff/2180001/ppapi/examples/media_s...
File ppapi/examples/media_stream_audio/media_stream_audio.cc (right):

https://codereview.chromium.org/140783004/diff/2180001/ppapi/examples/media_s...
ppapi/examples/media_stream_audio/media_stream_audio.cc:35: uint32_t kColorRed =
0xFFFF0000;
On 2014/02/04 18:26:13, yzshen1 wrote:
> nit: please use const.

Done.

https://codereview.chromium.org/140783004/diff/2180001/ppapi/examples/media_s...
ppapi/examples/media_stream_audio/media_stream_audio.cc:159: void
OnGetFrame(int32_t result, pp::AudioFrame frame) {
On 2014/02/04 18:26:13, yzshen1 wrote:
> nit: You could also use pp::AudioFrame&.

Tried pp::AudioFrame&, but it does not compile.

https://codereview.chromium.org/140783004/diff/2180001/ppapi/examples/media_s...
ppapi/examples/media_stream_audio/media_stream_audio.cc:164: if
(samples_.empty()) {
On 2014/02/04 18:26:13, yzshen1 wrote:
> Do we need to consider that frames may change configuration in the middle of
the
> stream?
> 
> Because developers might read the example and assume that the configuration
> never changes. We should at least comment about it.

Done

https://codereview.chromium.org/140783004/diff/2180001/ppapi/examples/media_s...
ppapi/examples/media_stream_audio/media_stream_audio.cc:175: sample_count_ *
channel_count_ * sizeof(int16_t));
On 2014/02/04 18:26:13, yzshen1 wrote:
> Do we need to make sure GetSampleSize() agrees with int16_t?

Add a PP_DCHECK at the beginning of this function.

[yzshen1, 2014-02-04 20:19:58]

LGTM

Please wait for other reviews' LGs. Thanks for the great work!

https://codereview.chromium.org/140783004/diff/2460001/ppapi/examples/media_s...
File ppapi/examples/media_stream_audio/media_stream_audio.cc (right):

https://codereview.chromium.org/140783004/diff/2460001/ppapi/examples/media_s...
ppapi/examples/media_stream_audio/media_stream_audio.cc:168: if (channel_count_
== channels || sample_count_ != samples) {
is it channel_count_ != channel?

https://codereview.chromium.org/140783004/diff/2460001/ppapi/examples/media_s...
ppapi/examples/media_stream_audio/media_stream_audio.cc:175: timer_interval_ =
(sample_count_ * 1000) / frame.GetSampleRate() + 5;
It seems better to move this line out of if(), and before setting
timer_internal_, you set a boolean indicating whether we need to schedule next
timer.

[Peng, 2014-02-04 20:33:40]

Thanks.

https://codereview.chromium.org/140783004/diff/2460001/ppapi/examples/media_s...
File ppapi/examples/media_stream_audio/media_stream_audio.cc (right):

https://codereview.chromium.org/140783004/diff/2460001/ppapi/examples/media_s...
ppapi/examples/media_stream_audio/media_stream_audio.cc:168: if (channel_count_
== channels || sample_count_ != samples) {
On 2014/02/04 20:19:59, yzshen1 wrote:
> is it channel_count_ != channel?

Oops! Done

https://codereview.chromium.org/140783004/diff/2460001/ppapi/examples/media_s...
ppapi/examples/media_stream_audio/media_stream_audio.cc:175: timer_interval_ =
(sample_count_ * 1000) / frame.GetSampleRate() + 5;
On 2014/02/04 20:19:59, yzshen1 wrote:
> It seems better to move this line out of if(), and before setting
> timer_internal_, you set a boolean indicating whether we need to schedule next
> timer.

Done.

[Peng, 2014-02-04 20:39:00]

Hi Justin, Could you please review ppapi/proxy/ppapi_messages.h? Thanks.

[jschuh, 2014-02-05 21:44:02]

ipc security lgtm: string as a simple opaque identifier.

[Peng, 2014-02-05 21:45:48]

The CQ bit was checked by penghuang@chromium.org

[commit-bot: I haz the power, 2014-02-05 21:49:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/140783004/2470001

[commit-bot: I haz the power, 2014-02-06 02:50:27]

Change committed as 249245