Switch OnMoreData() to use AudioBus.

Switch OnMoreData() to use AudioBus.

As titled, with this change we're now piping float data around the pipeline from
end to end. This change is in preparation for browser side channel remixing and
resampling.

As a consequence of this change the shared memory now represents the
contents of an AudioBus object, which is essentially audio data in a float
planar format.

BUG=114700
TEST=Should be no audible change. Ran all existing tests. Compiled ran
WebAudio/HTML5/WebRTC on all platforms and PPAPI on Linux.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154951

[DaleCurtis, 2012-08-22 23:09:21]

scherkus: all,
crogers: mac changes,
henrika: windows changes,
nfullagar/brettw: ppapi changes.

This requires the following to land first:
http://codereview.chromium.org/10826296/
http://codereview.chromium.org/10855086/

http://codereview.chromium.org/10832285/diff/23001/media/audio/mac/audio_outp...
File media/audio/mac/audio_output_mac_unittest.cc (left):

http://codereview.chromium.org/10832285/diff/23001/media/audio/mac/audio_outp...
media/audio/mac/audio_output_mac_unittest.cc:35: TEST(MacAudioTest,
SineWaveAudio16MonoTest) {
Duplicate test from simple sources. Copy/paste remnant?

http://codereview.chromium.org/10832285/diff/23001/media/audio/simple_sources.h
File media/audio/simple_sources.h (left):

http://codereview.chromium.org/10832285/diff/23001/media/audio/simple_sources...
media/audio/simple_sources.h:45: class MEDIA_EXPORT PushAudioOutput {
Replaced with CapSamples/Reset API on SineWaveAudioSource.

http://codereview.chromium.org/10832285/diff/23001/media/audio/win/audio_outp...
File media/audio/win/audio_output_win_unittest.cc (left):

http://codereview.chromium.org/10832285/diff/23001/media/audio/win/audio_outp...
media/audio/win/audio_output_win_unittest.cc:82: class TestSourceTripleBuffer :
public TestSourceBasic {
Misleading test to begin with since it's trying to infer information about AOS
via parameters. Test would pass on AlsaOutputDevice since it always gave a new
buffer for OnMoreData.

Not possible to test this way since AudioBus is shared for the entire AOS.

http://codereview.chromium.org/10832285/diff/23001/media/audio/win/audio_outp...
File media/audio/win/audio_output_win_unittest.cc (right):

http://codereview.chromium.org/10832285/diff/23001/media/audio/win/audio_outp...
media/audio/win/audio_output_win_unittest.cc:404: oas->Start(&source);
Test was broken before, it never started the AOS.

http://codereview.chromium.org/10832285/diff/23001/media/audio/win/audio_outp...
media/audio/win/audio_output_win_unittest.cc:548: break;
Not sure how this ever passed without this break;

[henrika (OOO until Aug 14), 2012-08-23 14:42:50]

LGTM for the Windows part.

FYI - I must admit that I have never really understood the purpose of some of
the tests in win/audio_output_win_unittest.cc. I see that you had questions here
as well but I am unable to answer. Perhaps you could check with some of the
original authors.

http://codereview.chromium.org/10832285/diff/23001/media/audio/win/audio_low_...
File media/audio/win/audio_low_latency_output_win.cc (right):

http://codereview.chromium.org/10832285/diff/23001/media/audio/win/audio_low_...
media/audio/win/audio_low_latency_output_win.cc:829: audio_bus_->ToInterleaved(
Have you verified that WASAPIAudioOutputStreamTest::ReadFromStereoFile works
(run with --gtest_also_run_disabled_tests)? I guess you must go from interleaved
to planar (non-interleaved) in the test as well to match now; right?

http://codereview.chromium.org/10832285/diff/23001/media/audio/win/audio_low_...
File media/audio/win/audio_low_latency_output_win_unittest.cc (right):

http://codereview.chromium.org/10832285/diff/23001/media/audio/win/audio_low_...
media/audio/win/audio_low_latency_output_win_unittest.cc:50: 
nit, why empty line here?

http://codereview.chromium.org/10832285/diff/23001/media/audio/win/audio_low_...
media/audio/win/audio_low_latency_output_win_unittest.cc:134:
audio_bus->FromInterleaved(
Thx ;-)

http://codereview.chromium.org/10832285/diff/23001/media/audio/win/audio_outp...
File media/audio/win/audio_output_win_unittest.cc (left):

http://codereview.chromium.org/10832285/diff/23001/media/audio/win/audio_outp...
media/audio/win/audio_output_win_unittest.cc:82: class TestSourceTripleBuffer :
public TestSourceBasic {
ALSA on Windows :-)

[Chris Rogers, 2012-08-24 20:20:26]

Dale, thanks for doing this.

http://codereview.chromium.org/10832285/diff/23001/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

http://codereview.chromium.org/10832285/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/media/audio_renderer_host.cc:203:
DCHECK_GT(buffer_size, 0U);
Does this DCHECK make sense?  Since uint32 is unsigned it will always be
positive.  On the other hand we probably want some kind of sanity check against
wildly huge values.

http://codereview.chromium.org/10832285/diff/23001/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

http://codereview.chromium.org/10832285/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/media/audio_sync_reader.cc:23: audio_bus_ =
media::AudioBus::WrapMemory(params, shared_memory->memory());
I think we need to have a sanity check here that the |params| size matches the
actual |shared_memory| size.  It *should*, but it's very important to not make a
mistake here.

http://codereview.chromium.org/10832285/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/media/audio_sync_reader.cc:73: //
TODO(dalecurtis): Do we need to zero out remaining frames?
Please do, I remember specifically that we had a bug where this was causing
problems.

http://codereview.chromium.org/10832285/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/media/audio_sync_reader.cc:76: //
TODO(dalecurtis): This is probably unnecessary.
It seems like it should be unnecessary, and in normal running conditions it is
not needed.  But, it handles the degenerate case when audio starts glitching
because the renderer can't render fast enough (for example in Web Audio API if
the audio rendering becomes too heavy).  We're already in catastrophic failure
mode at this point because we're glitching, but if we fail to zero out the
memory here, then the glitching anomaly becomes even worse since it causes
repeated buffers (potentially quite loud ones!) to be played back end-end which
sounds a lot worse.

http://codereview.chromium.org/10832285/diff/23001/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_sync_reader.h (right):

http://codereview.chromium.org/10832285/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/media/audio_sync_reader.h:33: virtual int
Read(media::AudioBus* audio_bus) OVERRIDE;
Please add comment for Read() explaining the meaning of the return value (it now
returns frames instead of bytes).

http://codereview.chromium.org/10832285/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/media/audio_sync_reader.h:60: int packet_size_;
It would be nice to get away from the word "packet" which is usually used more
on the codec end and call this something like "max_buffer_size_"

http://codereview.chromium.org/10832285/diff/23001/media/audio/audio_device_t...
File media/audio/audio_device_thread.cc (right):

http://codereview.chromium.org/10832285/diff/23001/media/audio/audio_device_t...
media/audio/audio_device_thread.cc:200: audio_bus_ =
AudioBus::WrapMemory(audio_parameters_, shared_memory_.memory());
I think we should have a sanity check that the audio_parameters_ reflects the
actual size of the shared_memory_ (and not just accept it on faith from what the
browser gives back to us)

http://codereview.chromium.org/10832285/diff/23001/media/audio/mac/audio_low_...
File media/audio/mac/audio_low_latency_output_mac.cc (right):

http://codereview.chromium.org/10832285/diff/23001/media/audio/mac/audio_low_...
media/audio/mac/audio_low_latency_output_mac.cc:240: // volume adjust should use
SSE optimized vector_fmul() prior to interleave.
I think we're still going to need some platform-specific final channel ordering
adjustments like the Swizzle below.  We will have some notion of cross-platform
channel ordering for 5.1, etc.  But that may be different than the OS-level
channel ordering.

http://codereview.chromium.org/10832285/diff/23001/media/audio/mac/audio_outp...
File media/audio/mac/audio_output_mac.cc (right):

http://codereview.chromium.org/10832285/diff/23001/media/audio/mac/audio_outp...
media/audio/mac/audio_output_mac.cc:419: base::AutoLock
lock(audio_stream->audio_bus_lock_);
Are you sure this is right?  If this function is re-entrant then we don't want a
lock since we'll get dead-lock (see comment on line 374).  I'm also pretty sure
that this method won't be called from multiple threads in the same time window
so the lock shouldn't be needed.

http://codereview.chromium.org/10832285/diff/23001/media/audio/mac/audio_outp...
media/audio/mac/audio_output_mac.cc:420: 
You use "audio_stream->audio_bus_" three times below.  It would be nice to use a
local AudioBus*

http://codereview.chromium.org/10832285/diff/23001/media/audio/mac/audio_outp...
File media/audio/mac/audio_output_mac.h (right):

http://codereview.chromium.org/10832285/diff/23001/media/audio/mac/audio_outp...
media/audio/mac/audio_output_mac.h:110: base::Lock audio_bus_lock_;
I don't think the lock is needed.

http://codereview.chromium.org/10832285/diff/23001/ppapi/native_client/src/sh...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_audio.h (right):

http://codereview.chromium.org/10832285/diff/23001/ppapi/native_client/src/sh...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_audio.h:67:
scoped_ptr<uint8_t> audio_buffer_;
I think this should be scoped_array<> ??

http://codereview.chromium.org/10832285/diff/23001/ppapi/shared_impl/ppb_audi...
File ppapi/shared_impl/ppb_audio_shared.h (right):

http://codereview.chromium.org/10832285/diff/23001/ppapi/shared_impl/ppb_audi...
ppapi/shared_impl/ppb_audio_shared.h:110: scoped_ptr<uint8_t> audio_buffer_;
scoped_array<> ??

[DaleCurtis, 2012-08-24 23:53:11]

https://chromiumcodereview.appspot.com/10832285/diff/23001/content/browser/re...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://chromiumcodereview.appspot.com/10832285/diff/23001/content/browser/re...
content/browser/renderer_host/media/audio_renderer_host.cc:203:
DCHECK_GT(buffer_size, 0U);
On 2012/08/24 20:20:26, Chris Rogers wrote:
> Does this DCHECK make sense?  Since uint32 is unsigned it will always be
> positive.  On the other hand we probably want some kind of sanity check
against
> wildly huge values.

It does make sense since buffer_size could still be 0 which is wrong. I've added
a new check based on media::limits::kMaxPacketSizeBytes now.

https://chromiumcodereview.appspot.com/10832285/diff/23001/content/browser/re...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://chromiumcodereview.appspot.com/10832285/diff/23001/content/browser/re...
content/browser/renderer_host/media/audio_sync_reader.cc:23: audio_bus_ =
media::AudioBus::WrapMemory(params, shared_memory->memory());
On 2012/08/24 20:20:26, Chris Rogers wrote:
> I think we need to have a sanity check here that the |params| size matches the
> actual |shared_memory| size.  It *should*, but it's very important to not make
a
> mistake here.

Done.

https://chromiumcodereview.appspot.com/10832285/diff/23001/content/browser/re...
content/browser/renderer_host/media/audio_sync_reader.cc:73: //
TODO(dalecurtis): Do we need to zero out remaining frames?
On 2012/08/24 20:20:26, Chris Rogers wrote:
> Please do, I remember specifically that we had a bug where this was causing
> problems.

Done.

https://chromiumcodereview.appspot.com/10832285/diff/23001/content/browser/re...
content/browser/renderer_host/media/audio_sync_reader.cc:76: //
TODO(dalecurtis): This is probably unnecessary.
On 2012/08/24 20:20:26, Chris Rogers wrote:
> It seems like it should be unnecessary, and in normal running conditions it is
> not needed.  But, it handles the degenerate case when audio starts glitching
> because the renderer can't render fast enough (for example in Web Audio API if
> the audio rendering becomes too heavy).  We're already in catastrophic failure
> mode at this point because we're glitching, but if we fail to zero out the
> memory here, then the glitching anomaly becomes even worse since it causes
> repeated buffers (potentially quite loud ones!) to be played back end-end
which
> sounds a lot worse.

Thanks for the information, left alone.

https://chromiumcodereview.appspot.com/10832285/diff/23001/content/browser/re...
File content/browser/renderer_host/media/audio_sync_reader.h (right):

https://chromiumcodereview.appspot.com/10832285/diff/23001/content/browser/re...
content/browser/renderer_host/media/audio_sync_reader.h:33: virtual int
Read(media::AudioBus* audio_bus) OVERRIDE;
On 2012/08/24 20:20:26, Chris Rogers wrote:
> Please add comment for Read() explaining the meaning of the return value (it
now
> returns frames instead of bytes).

That's done in AudioOutputController::SyncReader.

https://chromiumcodereview.appspot.com/10832285/diff/23001/content/browser/re...
content/browser/renderer_host/media/audio_sync_reader.h:60: int packet_size_;
On 2012/08/24 20:20:26, Chris Rogers wrote:
> It would be nice to get away from the word "packet" which is usually used more
> on the codec end and call this something like "max_buffer_size_"

I'll put out another CL for this since there's a whole CL worth of terminology
clean up in audio_util (now shared_memory_util) to get everyone using the
correct terms.  As it is shared_memory_util uses packet_size and
shared_memory_size interchangeably and has an overloaded
"shared_memory_created_size".

Then I'll rename this once we agree on terms in that CL.

https://chromiumcodereview.appspot.com/10832285/diff/23001/media/audio/audio_...
File media/audio/audio_device_thread.cc (right):

https://chromiumcodereview.appspot.com/10832285/diff/23001/media/audio/audio_...
media/audio/audio_device_thread.cc:200: audio_bus_ =
AudioBus::WrapMemory(audio_parameters_, shared_memory_.memory());
On 2012/08/24 20:20:26, Chris Rogers wrote:
> I think we should have a sanity check that the audio_parameters_ reflects the
> actual size of the shared_memory_ (and not just accept it on faith from what
the
> browser gives back to us)

Done.

https://chromiumcodereview.appspot.com/10832285/diff/23001/media/audio/mac/au...
File media/audio/mac/audio_low_latency_output_mac.cc (right):

https://chromiumcodereview.appspot.com/10832285/diff/23001/media/audio/mac/au...
media/audio/mac/audio_low_latency_output_mac.cc:240: // volume adjust should use
SSE optimized vector_fmul() prior to interleave.
On 2012/08/24 20:20:26, Chris Rogers wrote:
> I think we're still going to need some platform-specific final channel
ordering
> adjustments like the Swizzle below.  We will have some notion of
cross-platform
> channel ordering for 5.1, etc.  But that may be different than the OS-level
> channel ordering.

I'm hoping we can have each AOS return the matrix necessary for channel remixing
to the desired layout given a ChannelLayout.  We need to discuss this and
resampling in more detail next week.

https://chromiumcodereview.appspot.com/10832285/diff/23001/media/audio/mac/au...
File media/audio/mac/audio_output_mac.cc (right):

https://chromiumcodereview.appspot.com/10832285/diff/23001/media/audio/mac/au...
media/audio/mac/audio_output_mac.cc:419: base::AutoLock
lock(audio_stream->audio_bus_lock_);
On 2012/08/24 20:20:26, Chris Rogers wrote:
> Are you sure this is right?  If this function is re-entrant then we don't want
a
> lock since we'll get dead-lock (see comment on line 374).  I'm also pretty
sure
> that this method won't be called from multiple threads in the same time window
> so the lock shouldn't be needed.

That indicates that a lock shouldn't be held across AudioQueue / OS API calls,
within the calling range of OnMoreData() I believe this is safe (otherwise no
function all the way down OnMoreData could hold a lock).

Without the lock if multiple OnMoreData() calls occur the shared audio_bus may
be trampled as locks further down the OnMoreData chain cycle to allow the next
pass through.

https://chromiumcodereview.appspot.com/10832285/diff/23001/media/audio/mac/au...
media/audio/mac/audio_output_mac.cc:420: 
On 2012/08/24 20:20:26, Chris Rogers wrote:
> You use "audio_stream->audio_bus_" three times below.  It would be nice to use
a
> local AudioBus*

Done. Presumably PCMQueueOutAudioOutputStream won't be destroyed while we're in
the middle of this call?

https://chromiumcodereview.appspot.com/10832285/diff/23001/media/audio/win/au...
File media/audio/win/audio_low_latency_output_win.cc (right):

https://chromiumcodereview.appspot.com/10832285/diff/23001/media/audio/win/au...
media/audio/win/audio_low_latency_output_win.cc:829: audio_bus_->ToInterleaved(
On 2012/08/23 14:42:53, henrika wrote:
> Have you verified that WASAPIAudioOutputStreamTest::ReadFromStereoFile works
> (run with --gtest_also_run_disabled_tests)? I guess you must go from
interleaved
> to planar (non-interleaved) in the test as well to match now; right?

It seems to work as is without modification. Is that what you expected? I'm a
little unclear on your comment.

https://chromiumcodereview.appspot.com/10832285/diff/23001/media/audio/win/au...
File media/audio/win/audio_low_latency_output_win_unittest.cc (right):

https://chromiumcodereview.appspot.com/10832285/diff/23001/media/audio/win/au...
media/audio/win/audio_low_latency_output_win_unittest.cc:50: 
On 2012/08/23 14:42:53, henrika wrote:
> nit, why empty line here?

Didn't seem to fit with the lines directly above. Looking at it now it goes
nicely with the first block though.

https://chromiumcodereview.appspot.com/10832285/diff/23001/ppapi/native_clien...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_audio.h (right):

https://chromiumcodereview.appspot.com/10832285/diff/23001/ppapi/native_clien...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_audio.h:67:
scoped_ptr<uint8_t> audio_buffer_;
On 2012/08/24 20:20:26, Chris Rogers wrote:
> I think this should be scoped_array<> ??

Done.

https://chromiumcodereview.appspot.com/10832285/diff/23001/ppapi/shared_impl/...
File ppapi/shared_impl/ppb_audio_shared.h (right):

https://chromiumcodereview.appspot.com/10832285/diff/23001/ppapi/shared_impl/...
ppapi/shared_impl/ppb_audio_shared.h:110: scoped_ptr<uint8_t> audio_buffer_;
On 2012/08/24 20:20:26, Chris Rogers wrote:
> scoped_array<> ??

Done.

[scherkus (not reviewing), 2012-08-27 20:46:54]

lgtm w/ nit

nice work!!

http://codereview.chromium.org/10832285/diff/32001/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_sync_reader.h (right):

http://codereview.chromium.org/10832285/diff/32001/content/browser/renderer_h...
content/browser/renderer_host/media/audio_sync_reader.h:56: // Shared memory
wrapper used for transfering audio data to Read() callers.
s/transfering/transferring

[nfullagar, 2012-08-27 21:45:00]

From a NaCl perspective, some concerns.  We need to be extra clear that
AudioBus' data_ is potentially a shared memory window, and that the client on
the other end is potentially evil, and will be able to write anything it wants
from any thread at any time into the shm.  When copying data out of the shm in
trusted code, it must be validated during the copy - checking for NaNs,
denormals, etc.  These scenarios should be clear in audio_bus.h so others who
modify audio_bus.h are aware.  There should also be validation of the contents
of the shm included in this CL since it is turning on AudioBus & floating point
data for NaCl clients.  (It isn't clear to me that FromInterleaved in
audio_bus.cc won't stop propagation of bad floats deeper into the trusted
software stack)  Someone more security minded than me probably ought to take a
look :)

http://codereview.chromium.org/10832285/diff/32001/ppapi/native_client/src/sh...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_audio.h (right):

http://codereview.chromium.org/10832285/diff/32001/ppapi/native_client/src/sh...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_audio.h:66: int
audio_buffer_size_;
audio_buffer_size_in_bytes_ for extra clarification

http://codereview.chromium.org/10832285/diff/32001/ppapi/shared_impl/ppb_audi...
File ppapi/shared_impl/ppb_audio_shared.h (right):

http://codereview.chromium.org/10832285/diff/32001/ppapi/shared_impl/ppb_audi...
ppapi/shared_impl/ppb_audio_shared.h:109: int audio_buffer_size_;
audio_buffer_size_in_bytes_ for extra clarification

[DaleCurtis, 2012-08-27 22:02:15]

nfullagar: From our previous discussions the only concern seemed to be handing
the hardware invalid float values.  As we're only outputting integer audio right
now, ToInterleaved() clips values before output.  

Per some spec info I found, http://c0x.coding-guidelines.com/6.3.1.4.html
converting to integer behavior is undefined, so we'll likely get garbage values
which we'll then clip to appropriate values.

If in the future we output float audio, we'll need to sanitize the values before
output, (but we'd need to do that anyways since the mixers will be generating
values > 1.0 and < -1.0)

+cevans for some security perspective. Should I file a security review bug for
this? Any special concerns about inf/nan floats propagating across the shared
memory?

[nfullagar, 2012-08-27 22:25:17]

Ok, unless cevans has deeper concerns, most of my concerns can be addressed by
adding appropriate comments so others working on this code are also aware.
- add a warning to audio_bus.h that what data_ points to can be written at any
time from untrusted code when AudioBus is used from NaCl, and thus must be
validated/sanitized as appropriate.
- add a warning that if in the future we output float, to sanitize data before
passing on to the output device.

On 2012/08/27 22:02:15, DaleCurtis wrote:
> nfullagar: From our previous discussions the only concern seemed to be handing
> the hardware invalid float values.  As we're only outputting integer audio
right
> now, ToInterleaved() clips values before output.  
> 
> Per some spec info I found, http://c0x.coding-guidelines.com/6.3.1.4.html
> converting to integer behavior is undefined, so we'll likely get garbage
values
> which we'll then clip to appropriate values.
> 
> If in the future we output float audio, we'll need to sanitize the values
before
> output, (but we'd need to do that anyways since the mixers will be generating
> values > 1.0 and < -1.0)
> 
> +cevans for some security perspective. Should I file a security review bug for
> this? Any special concerns about inf/nan floats propagating across the shared
> memory?

[Chris Rogers, 2012-08-28 22:32:05]

LGTM with nits

http://codereview.chromium.org/10832285/diff/32001/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

http://codereview.chromium.org/10832285/diff/32001/content/browser/renderer_h...
content/browser/renderer_host/media/audio_sync_reader.cc:67:
DCHECK_EQ(static_cast<size_t>(size) % sizeof(*audio_bus_->channel(0)), 0U);
It would be nice to have a little more whitespace (a blank line or two) in
between 66:67, 67:68, and have a comment explaining the DCHECK_EQ on 67 in plain
English.

http://codereview.chromium.org/10832285/diff/32001/content/browser/renderer_h...
content/browser/renderer_host/media/audio_sync_reader.cc:75: // caller's
AudioBus to point to the shared memory.
Up to you, but I would remove this TODO since the code is hard enough to follow
even without the extra micro-optimization TODO.  I think that this optimization
is the least of our worries considering the grand scheme of cleanup we need to
make.

http://codereview.chromium.org/10832285/diff/32001/media/audio/audio_output_d...
File media/audio/audio_output_device.cc (right):

http://codereview.chromium.org/10832285/diff/32001/media/audio/audio_output_d...
media/audio/audio_output_device.cc:278: // these methods to
Set/GetActualFrameCount().
With any luck, we'll be able to get rid of the need for
SetActualDataSizeInBytes() before too long.

In the meantime, since you have a TODO here, please add a matching TODO in
AudioSyncReader::Read() explaining how these correspond.

http://codereview.chromium.org/10832285/diff/32001/media/audio/mac/audio_outp...
File media/audio/mac/audio_output_mac.cc (right):

http://codereview.chromium.org/10832285/diff/32001/media/audio/mac/audio_outp...
media/audio/mac/audio_output_mac.cc:419: base::AutoLock
lock(audio_stream->audio_bus_lock_);
Please remove lock - it's not necessary from a thread-safety standpoint and
makes it harder to read the code.

[Chris Rogers, 2012-08-28 22:36:38]

http://codereview.chromium.org/10832285/diff/32001/media/audio/mac/audio_outp...
File media/audio/mac/audio_output_mac.cc (right):

http://codereview.chromium.org/10832285/diff/32001/media/audio/mac/audio_outp...
media/audio/mac/audio_output_mac.cc:419: base::AutoLock
lock(audio_stream->audio_bus_lock_);
To elaborate about removing the lock, the audio_bus_ is only used within the
RenderCallback() which itself is invoked on a single-thread.

[DaleCurtis, 2012-08-29 04:34:43]

All comments should be addressed.  I'll do some more testing on all platforms
tomorrow.  Hopefully we can get this landed in the coming days. Thanks!

nfullagar, brettw: I was able to get the actual frame count from PPAPI instead
of trying to deduce it from the shared memory size (which can't be done due to
padding).  Please ensure I'm not doing anything crazy here.

https://chromiumcodereview.appspot.com/10832285/diff/32001/content/browser/re...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://chromiumcodereview.appspot.com/10832285/diff/32001/content/browser/re...
content/browser/renderer_host/media/audio_sync_reader.cc:67:
DCHECK_EQ(static_cast<size_t>(size) % sizeof(*audio_bus_->channel(0)), 0U);
On 2012/08/28 22:32:05, Chris Rogers wrote:
> It would be nice to have a little more whitespace (a blank line or two) in
> between 66:67, 67:68, and have a comment explaining the DCHECK_EQ on 67 in
plain
> English.

Done.

https://chromiumcodereview.appspot.com/10832285/diff/32001/content/browser/re...
content/browser/renderer_host/media/audio_sync_reader.cc:75: // caller's
AudioBus to point to the shared memory.
On 2012/08/28 22:32:05, Chris Rogers wrote:
> Up to you, but I would remove this TODO since the code is hard enough to
follow
> even without the extra micro-optimization TODO.  I think that this
optimization
> is the least of our worries considering the grand scheme of cleanup we need to
> make.

Done.

https://chromiumcodereview.appspot.com/10832285/diff/32001/content/browser/re...
File content/browser/renderer_host/media/audio_sync_reader.h (right):

https://chromiumcodereview.appspot.com/10832285/diff/32001/content/browser/re...
content/browser/renderer_host/media/audio_sync_reader.h:56: // Shared memory
wrapper used for transfering audio data to Read() callers.
On 2012/08/27 20:46:55, scherkus wrote:
> s/transfering/transferring

Done.

https://chromiumcodereview.appspot.com/10832285/diff/32001/media/audio/audio_...
File media/audio/audio_output_device.cc (right):

https://chromiumcodereview.appspot.com/10832285/diff/32001/media/audio/audio_...
media/audio/audio_output_device.cc:278: // these methods to
Set/GetActualFrameCount().
On 2012/08/28 22:32:05, Chris Rogers wrote:
> With any luck, we'll be able to get rid of the need for
> SetActualDataSizeInBytes() before too long.
> 
> In the meantime, since you have a TODO here, please add a matching TODO in
> AudioSyncReader::Read() explaining how these correspond.

Done.

https://chromiumcodereview.appspot.com/10832285/diff/32001/media/audio/mac/au...
File media/audio/mac/audio_output_mac.cc (right):

https://chromiumcodereview.appspot.com/10832285/diff/32001/media/audio/mac/au...
media/audio/mac/audio_output_mac.cc:419: base::AutoLock
lock(audio_stream->audio_bus_lock_);
On 2012/08/28 22:36:39, Chris Rogers wrote:
> To elaborate about removing the lock, the audio_bus_ is only used within the
> RenderCallback() which itself is invoked on a single-thread.

Done.

https://chromiumcodereview.appspot.com/10832285/diff/32001/ppapi/native_clien...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_audio.h (right):

https://chromiumcodereview.appspot.com/10832285/diff/32001/ppapi/native_clien...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_audio.h:66: int
audio_buffer_size_;
On 2012/08/27 21:45:00, nfullagar wrote:
> audio_buffer_size_in_bytes_ for extra clarification

Done.

https://chromiumcodereview.appspot.com/10832285/diff/32001/ppapi/shared_impl/...
File ppapi/shared_impl/ppb_audio_shared.h (right):

https://chromiumcodereview.appspot.com/10832285/diff/32001/ppapi/shared_impl/...
ppapi/shared_impl/ppb_audio_shared.h:109: int audio_buffer_size_;
On 2012/08/27 21:45:00, nfullagar wrote:
> audio_buffer_size_in_bytes_ for extra clarification

Done.

https://chromiumcodereview.appspot.com/10832285/diff/33003/content/browser/re...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://chromiumcodereview.appspot.com/10832285/diff/33003/content/browser/re...
content/browser/renderer_host/media/audio_sync_reader.cc:64: if (!DataReady()) {
I ran into this issue while testing debug builds. OnMoreData() happens too
quickly so we end up with a junk GetActualDataSizeInBytes value.

I'm not sure if this is the right solution or if we should use the Windows
solution on all platforms. Thoughts?

[Chris Rogers, 2012-08-29 04:59:17]

https://chromiumcodereview.appspot.com/10832285/diff/33003/content/browser/re...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://chromiumcodereview.appspot.com/10832285/diff/33003/content/browser/re...
content/browser/renderer_host/media/audio_sync_reader.cc:64: if (!DataReady()) {
Please don't add this.  At one time, when this code was first added it was
enabled for all platforms and caused problems on Mac OS X.  We know that
OnMoreData() happens too quickly on the waveOut back-end.  The correct solution
is not using this technique.

Let's try to keep this CL targeted to just switching the shared-memory format
over to AudioBus, so we can stage changes in small pieces.  This change is
unrelated.

On 2012/08/29 04:34:43, DaleCurtis wrote:
> I ran into this issue while testing debug builds. OnMoreData() happens too
> quickly so we end up with a junk GetActualDataSizeInBytes value.
> 
> I'm not sure if this is the right solution or if we should use the Windows
> solution on all platforms. Thoughts?

[DaleCurtis, 2012-08-29 05:17:56]

http://codereview.chromium.org/10832285/diff/33003/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

http://codereview.chromium.org/10832285/diff/33003/content/browser/renderer_h...
content/browser/renderer_host/media/audio_sync_reader.cc:64: if (!DataReady()) {
On 2012/08/29 04:59:17, Chris Rogers wrote:
> Please don't add this.  At one time, when this code was first added it was
> enabled for all platforms and caused problems on Mac OS X.  We know that
> OnMoreData() happens too quickly on the waveOut back-end.  The correct
solution
> is not using this technique.
> 
> Let's try to keep this CL targeted to just switching the shared-memory format
> over to AudioBus, so we can stage changes in small pieces.  This change is
> unrelated.
> 
> On 2012/08/29 04:34:43, DaleCurtis wrote:
> > I ran into this issue while testing debug builds. OnMoreData() happens too
> > quickly so we end up with a junk GetActualDataSizeInBytes value.
> > 
> > I'm not sure if this is the right solution or if we should use the Windows
> > solution on all platforms. Thoughts?
> 

This is not unrelated as since we're wrapping the shared memory on the renderer
side you can't access the shared memory until DataReady() is true or you'll get
partial / in processing audio.  Also, without this the CHECK() on frames below
fails too since GetActualDataSizeInBytes == UnknownDataSize == UINT32_MAX - 1.

I don't see any harm in the change I've made, otherwise we're reading
uninitialized data (or data in the process of being initialized) and returning
it to the output device which seems much worse.

[DaleCurtis, 2012-08-29 17:03:42]

http://codereview.chromium.org/10832285/diff/33003/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

http://codereview.chromium.org/10832285/diff/33003/content/browser/renderer_h...
content/browser/renderer_host/media/audio_sync_reader.cc:64: if (!DataReady()) {
On 2012/08/29 05:17:57, DaleCurtis wrote:
> On 2012/08/29 04:59:17, Chris Rogers wrote:
> > Please don't add this.  At one time, when this code was first added it was
> > enabled for all platforms and caused problems on Mac OS X.  We know that
> > OnMoreData() happens too quickly on the waveOut back-end.  The correct
> solution
> > is not using this technique.
> > 
> > Let's try to keep this CL targeted to just switching the shared-memory
format
> > over to AudioBus, so we can stage changes in small pieces.  This change is
> > unrelated.
> > 
> > On 2012/08/29 04:34:43, DaleCurtis wrote:
> > > I ran into this issue while testing debug builds. OnMoreData() happens too
> > > quickly so we end up with a junk GetActualDataSizeInBytes value.
> > > 
> > > I'm not sure if this is the right solution or if we should use the Windows
> > > solution on all platforms. Thoughts?
> > 
> 
> This is not unrelated as since we're wrapping the shared memory on the
renderer
> side you can't access the shared memory until DataReady() is true or you'll
get
> partial / in processing audio.  Also, without this the CHECK() on frames below
> fails too since GetActualDataSizeInBytes == UnknownDataSize == UINT32_MAX - 1.
> 
> I don't see any harm in the change I've made, otherwise we're reading
> uninitialized data (or data in the process of being initialized) and returning
> it to the output device which seems much worse.

Chris explained his concerns offline. I'll switch the CHECKs to instead clip the
size, frame values as necessary.

[nfullagar, 2012-08-29 18:38:22]

http://codereview.chromium.org/10832285/diff/33003/ppapi/native_client/src/sh...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_audio.cc (right):

http://codereview.chromium.org/10832285/diff/33003/ppapi/native_client/src/sh...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_audio.cc:117:
audio->audio_bus_->channels());
Perhaps cleaner if AudioBus had a member func to return size in bytes of the
buffer (sans padding?)  Or a descriptive temp var?
  media::SetActualDataSizeInBytes(
      audio->shm_buffer_, audio->shm_size_,
      audio->audio_bus_->buffer_size_in_bytes());
...or...
  size_t audio_bus_buffer_size_in_bytes =
      audio->audio_bus_->frames() *
      sizeof(*(audio->audio_bus_->channel(0))) *
      audio->audio_bus_->channels());
  media::SetActualDataSizeInBytes(
      audio->shm_buffer_, audio->shm_size_,
      audio_bus_buffer_size_in_bytes);
...or?

http://codereview.chromium.org/10832285/diff/33003/ppapi/shared_impl/ppb_audi...
File ppapi/shared_impl/ppb_audio_shared.cc (right):

http://codereview.chromium.org/10832285/diff/33003/ppapi/shared_impl/ppb_audi...
ppapi/shared_impl/ppb_audio_shared.cc:89: audio_buffer_size_bytes_ =
audio_bus and audio_buffer are starting to blur together (for me) when reading
through the code.  Is there a more distinctive name that could better
distinguish between the two?  I like |audio_bus_| as it is, but the other could 
be |audio_client_buffer_| or |client_buffer_| or ..?

http://codereview.chromium.org/10832285/diff/33003/ppapi/shared_impl/ppb_audi...
ppapi/shared_impl/ppb_audio_shared.cc:175: sizeof(*audio_bus_->channel(0)) *
audio_bus_->channels());
same comment from other proxy re: audio_bus_ buffer size in bytes

[DaleCurtis, 2012-08-29 18:52:22]

http://codereview.chromium.org/10832285/diff/33003/ppapi/native_client/src/sh...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_audio.cc (right):

http://codereview.chromium.org/10832285/diff/33003/ppapi/native_client/src/sh...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_audio.cc:117:
audio->audio_bus_->channels());
On 2012/08/29 18:38:22, nfullagar wrote:
> Perhaps cleaner if AudioBus had a member func to return size in bytes of the
> buffer (sans padding?)  Or a descriptive temp var?
>   media::SetActualDataSizeInBytes(
>       audio->shm_buffer_, audio->shm_size_,
>       audio->audio_bus_->buffer_size_in_bytes());
> ...or...
>   size_t audio_bus_buffer_size_in_bytes =
>       audio->audio_bus_->frames() *
>       sizeof(*(audio->audio_bus_->channel(0))) *
>       audio->audio_bus_->channels());
>   media::SetActualDataSizeInBytes(
>       audio->shm_buffer_, audio->shm_size_,
>       audio_bus_buffer_size_in_bytes);
> ...or?

Per the TODO this will just be temporary until I rename the
Get/SetActualDataSize methods to Set/GetActualFrameCount() at which point all
that mess will disappear for SetActualFrameCount(audio->shm_buffer_,
audio->shm_size_, audio_bus->frames());

I'll add a local "bytes_per_frame" temp value for now.

http://codereview.chromium.org/10832285/diff/33003/ppapi/shared_impl/ppb_audi...
File ppapi/shared_impl/ppb_audio_shared.cc (right):

http://codereview.chromium.org/10832285/diff/33003/ppapi/shared_impl/ppb_audi...
ppapi/shared_impl/ppb_audio_shared.cc:89: audio_buffer_size_bytes_ =
On 2012/08/29 18:38:22, nfullagar wrote:
> audio_bus and audio_buffer are starting to blur together (for me) when reading
> through the code.  Is there a more distinctive name that could better
> distinguish between the two?  I like |audio_bus_| as it is, but the other
could 
> be |audio_client_buffer_| or |client_buffer_| or ..?

Since audio is already in the class name I switched it to |client_buffer_|

http://codereview.chromium.org/10832285/diff/33003/ppapi/shared_impl/ppb_audi...
ppapi/shared_impl/ppb_audio_shared.cc:175: sizeof(*audio_bus_->channel(0)) *
audio_bus_->channels());
On 2012/08/29 18:38:22, nfullagar wrote:
> same comment from other proxy re: audio_bus_ buffer size in bytes

Done.

[DaleCurtis, 2012-08-30 00:28:04]

Discussed with cevans who pointed me to jschuh who didn't have any unique
concerns regarding float data across the shared memory so long as the browser
side is expecting it.

His main concern was that it has test and fuzzing coverage before it ships to
stable.

Currently, we've got plenty of test coverage on the media side.  I'm not sure
what the NaCl testing side looks like though.  I've just been testing with
Bastion and NaClBox per previous discussions with nfullagar.

nfullagar: Regarding fuzzing, aside from ClusterFuzz what kind of fuzzing does
NaCl have? Is there anything I should add?

[nfullagar, 2012-09-04 20:35:19]

> 
> nfullagar: Regarding fuzzing, aside from ClusterFuzz what kind of fuzzing does
> NaCl have? Is there anything I should add?

AFAIK we don't have any fuzzing tests for PPAPI wrt to NaCl.  For the shm, we
haven't in the past as it started off as just a fixed block of int data.  When
the count written field was added, it wasn't fuzzed.

[nfullagar, 2012-09-04 20:46:32]

lgtm

[brettw, 2012-09-04 21:40:53]

lgtm

http://codereview.chromium.org/10832285/diff/60002/ppapi/shared_impl/ppb_audi...
File ppapi/shared_impl/ppb_audio_shared.cc (right):

http://codereview.chromium.org/10832285/diff/60002/ppapi/shared_impl/ppb_audi...
ppapi/shared_impl/ppb_audio_shared.cc:12: // TODO(???): PPAPI shouldn't hard
code these values for all clients.
Can you fill your name in here? At least then there's somebody to ask about it,
even if you're not planning on doing this in the near future.

[DaleCurtis, 2012-09-05 09:12:41]

http://codereview.chromium.org/10832285/diff/60002/ppapi/shared_impl/ppb_audi...
File ppapi/shared_impl/ppb_audio_shared.cc (right):

http://codereview.chromium.org/10832285/diff/60002/ppapi/shared_impl/ppb_audi...
ppapi/shared_impl/ppb_audio_shared.cc:12: // TODO(???): PPAPI shouldn't hard
code these values for all clients.
On 2012/09/04 21:40:53, brettw wrote:
> Can you fill your name in here? At least then there's somebody to ask about
it,
> even if you're not planning on doing this in the near future.

Done.

[commit-bot: I haz the power, 2012-09-05 09:12:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10832285/59006

[commit-bot: I haz the power, 2012-09-05 13:39:24]

Change committed as 154951

[scherkus (not reviewing), 2012-09-05 15:06:49]

On 2012/09/05 13:39:24, I haz the power (commit-bot) wrote:
> Change committed as 154951

\o/