Third installement of low level audio for mac...

Third installement of low level audio for mac
- Finally audio playback wired
- Takes into account initial buffer fill change of last week
- Two 'can you hear this?' unit tests added



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

[cpu_(ooo_6.6-7.5), 2009-04-24 17:05:03]

The only noteworthy thing here is lack of locks between the enqueue function and
the stop function.

[John Grabowski, 2009-04-24 20:39:02]

http://codereview.chromium.org/92131/diff/1/3
File media/audio/mac/audio_output_mac.cc (right):

http://codereview.chromium.org/92131/diff/1/3#newcode32
Line 32: format_.mBytesPerFrame = format_.mBytesPerPacket;
Not sure why you reordered this here.
If reordering is warranted perhaps you can order all these assignments to match
declaration order in the struct so it's easier to see if you missed one.

http://codereview.chromium.org/92131/diff/1/3#newcode39
Line 39: AudioSourceCallback* source = source_;
Why did you add this local?  I would guess because the Stop() call is async
(below) but a comment would be nice.  Multithreaded problems can be hard to
debug; comments really help with understanding, especially for the next person
to come along.

http://codereview.chromium.org/92131/diff/1/3#newcode101
Line 101: source_ = NULL;
R/W from multiple threads, yes?
Perhaps declare source_ volatile in a header.

http://codereview.chromium.org/92131/diff/1/3#newcode127
Line 127: // Note to future hackers of this function: Do not add locks here
because we
Perhaps an overview of what threads you have running, and typical call chains
for them, would really help.

http://codereview.chromium.org/92131/diff/1/3#newcode149
Line 149: if (NULL == queue)
What does it mean if we get more data but have no queue?  Is that valid?  No
error is thrown here.

http://codereview.chromium.org/92131/diff/1/3#newcode154
Line 154: if (err == -66632) {
constant name for this error?

http://codereview.chromium.org/92131/diff/1/2
File media/audio/mac/audio_output_mac_unittest.cc (right):

http://codereview.chromium.org/92131/diff/1/2#newcode65
Line 65: if (!audio_man->HasAudioDevices())
Hmm.  If no audio devices, return happily?  When would that happen and be OK?

http://codereview.chromium.org/92131/diff/1/2#newcode82
Line 82: 
Idea for an extra test: try setting the callback frequency REAL small and doing
start/stop/start/stop to see if you can trigger a problem.

[cpu_(ooo_6.6-7.5), 2009-04-30 22:31:20]

code updated, please check again.

http://codereview.chromium.org/92131/diff/1/3
File media/audio/mac/audio_output_mac.cc (right):

http://codereview.chromium.org/92131/diff/1/3#newcode32
Line 32: format_.mBytesPerFrame = format_.mBytesPerPacket;
On 2009/04/24 20:39:02, John Grabowski wrote:
> Not sure why you reordered this here.
> If reordering is warranted perhaps you can order all these assignments to
match
> declaration order in the struct so it's easier to see if you missed one.

That was my original approach but if you recall I reordered as per mmentovai
request. The change here is because that reordering introduced a bug :(
mBytesPerFrame was being set to 0 regardless.  Unit test saved the bacon.

http://codereview.chromium.org/92131/diff/1/3#newcode39
Line 39: AudioSourceCallback* source = source_;
On 2009/04/24 20:39:02, John Grabowski wrote:
> Why did you add this local?  I would guess because the Stop() call is async
> (below) but a comment would be nice.  Multithreaded problems can be hard to
> debug; comments really help with understanding, especially for the next person
> to come along.

Done.

http://codereview.chromium.org/92131/diff/1/3#newcode101
Line 101: source_ = NULL;
On 2009/04/24 20:39:02, John Grabowski wrote:
> R/W from multiple threads, yes?
> Perhaps declare source_ volatile in a header.

I tried, but gcc insists in marking volatile the functions so it balloned into
changes everywhere specially in the interface :(
Given that afaik that decoration actually doing for multi-thread correctness I
abandoned that approach.

http://codereview.chromium.org/92131/diff/1/3#newcode127
Line 127: // Note to future hackers of this function: Do not add locks here
because we
On 2009/04/24 20:39:02, John Grabowski wrote:
> Perhaps an overview of what threads you have running, and typical call chains
> for them, would really help.

Done.

http://codereview.chromium.org/92131/diff/1/3#newcode154
Line 154: if (err == -66632) {
On 2009/04/24 20:39:02, John Grabowski wrote:
> constant name for this error?

Not known :)  made one up. Maybe when snow comes.

http://codereview.chromium.org/92131/diff/1/2
File media/audio/mac/audio_output_mac_unittest.cc (right):

http://codereview.chromium.org/92131/diff/1/2#newcode65
Line 65: if (!audio_man->HasAudioDevices())
On 2009/04/24 20:39:02, John Grabowski wrote:
> Hmm.  If no audio devices, return happily?  When would that happen and be OK?
> 

In the bots. It is conceivable that no audio is available.
The mock audio tests (upcming) are the best we can do there.

http://codereview.chromium.org/92131/diff/1/2#newcode82
Line 82: 
On 2009/04/24 20:39:02, John Grabowski wrote:
> Idea for an extra test: try setting the callback frequency REAL small and
doing
> start/stop/start/stop to see if you can trigger a problem.

Sure, let me do that in the next drop.

[cpu_(ooo_6.6-7.5), 2009-04-30 22:49:15]

Updated again with the SL constant.

On 2009/04/30 22:31:20, cpu wrote:
> code updated, please check again.
> 
> http://codereview.chromium.org/92131/diff/1/3
> File media/audio/mac/audio_output_mac.cc (right):
> 
> http://codereview.chromium.org/92131/diff/1/3#newcode32
> Line 32: format_.mBytesPerFrame = format_.mBytesPerPacket;
> On 2009/04/24 20:39:02, John Grabowski wrote:
> > Not sure why you reordered this here.
> > If reordering is warranted perhaps you can order all these assignments to
> match
> > declaration order in the struct so it's easier to see if you missed one.
> 
> That was my original approach but if you recall I reordered as per mmentovai
> request. The change here is because that reordering introduced a bug :(
> mBytesPerFrame was being set to 0 regardless.  Unit test saved the bacon.
> 
> http://codereview.chromium.org/92131/diff/1/3#newcode39
> Line 39: AudioSourceCallback* source = source_;
> On 2009/04/24 20:39:02, John Grabowski wrote:
> > Why did you add this local?  I would guess because the Stop() call is async
> > (below) but a comment would be nice.  Multithreaded problems can be hard to
> > debug; comments really help with understanding, especially for the next
person
> > to come along.
> 
> Done.
> 
> http://codereview.chromium.org/92131/diff/1/3#newcode101
> Line 101: source_ = NULL;
> On 2009/04/24 20:39:02, John Grabowski wrote:
> > R/W from multiple threads, yes?
> > Perhaps declare source_ volatile in a header.
> 
> I tried, but gcc insists in marking volatile the functions so it balloned into
> changes everywhere specially in the interface :(
> Given that afaik that decoration actually doing for multi-thread correctness I
> abandoned that approach.
> 
> http://codereview.chromium.org/92131/diff/1/3#newcode127
> Line 127: // Note to future hackers of this function: Do not add locks here
> because we
> On 2009/04/24 20:39:02, John Grabowski wrote:
> > Perhaps an overview of what threads you have running, and typical call
chains
> > for them, would really help.
> 
> Done.
> 
> http://codereview.chromium.org/92131/diff/1/3#newcode154
> Line 154: if (err == -66632) {
> On 2009/04/24 20:39:02, John Grabowski wrote:
> > constant name for this error?
> 
> Not known :)  made one up. Maybe when snow comes.
> 
> http://codereview.chromium.org/92131/diff/1/2
> File media/audio/mac/audio_output_mac_unittest.cc (right):
> 
> http://codereview.chromium.org/92131/diff/1/2#newcode65
> Line 65: if (!audio_man->HasAudioDevices())
> On 2009/04/24 20:39:02, John Grabowski wrote:
> > Hmm.  If no audio devices, return happily?  When would that happen and be
OK?
> > 
> 
> In the bots. It is conceivable that no audio is available.
> The mock audio tests (upcming) are the best we can do there.
> 
> http://codereview.chromium.org/92131/diff/1/2#newcode82
> Line 82: 
> On 2009/04/24 20:39:02, John Grabowski wrote:
> > Idea for an extra test: try setting the callback frequency REAL small and
> doing
> > start/stop/start/stop to see if you can trigger a problem.
> 
> Sure, let me do that in the next drop.

[John Grabowski, 2009-04-30 23:16:24]

LGTM
Thanks for following up.

http://codereview.chromium.org/92131/diff/1/3
File media/audio/mac/audio_output_mac.cc (right):

http://codereview.chromium.org/92131/diff/1/3#newcode32
Line 32: format_.mBytesPerFrame = format_.mBytesPerPacket;
On 2009/04/30 22:31:20, cpu wrote:
> On 2009/04/24 20:39:02, John Grabowski wrote:
> > Not sure why you reordered this here.
> > If reordering is warranted perhaps you can order all these assignments to
> match
> > declaration order in the struct so it's easier to see if you missed one.
> 
> That was my original approach but if you recall I reordered as per mmentovai
> request. The change here is because that reordering introduced a bug :(
> mBytesPerFrame was being set to 0 regardless.  Unit test saved the bacon.
> 

Ah... I see it.
So glad to hear you have unit tests!!!

http://codereview.chromium.org/92131/diff/2002/2004#newcode11
Line 11: PCMQueueOutAudioOutputStream::PCMQueueOutAudioOutputStream(
Thanks for doing this.

http://codereview.chromium.org/92131/diff/2002/2004#newcode28
Line 28: format_.mBitsPerChannel = bits_per_sample;
More correctly, "when we switch to the Snow Leopard SDK"