(Relanding) Restores larger output buffer size when output stream requiring smaller size is closed

(Relanding) Restores larger output buffer size when output stream requiring smaller size is closed

Previous attempt was done at: https://codereview.chromium.org/1903753002/ but it broke an
ASAN bot on Mac. This is the second attempt to land.

Restores a larger output buffer size when output stream requiring smaller size is closed if
no input stream is currently using the same audio device.

BUG=602587
TEST=Manual tests and extra debug logging using a wide mix of audio applications (audio tag, WebAudio and WebRTC).
I have also verified that all tests now passes under ASAN

Committed: https://crrev.com/04fa4fc3ea17b4b93904f79bcf7fb0ced0dfb90b
Cr-Commit-Position: refs/heads/master@{#393261}

[henrika (OOO until Aug 14), 2016-05-11 14:22:14]

Description was changed from

==========
(Relanding) Restores larger output buffer size when output stream requiring
smaller size is closed

BUG=
==========

to

==========
(Relanding) Restores larger output buffer size when output stream requiring
smaller size is closed

Previous attempt was done at: https://codereview.chromium.org/1903753002/ but it
broke an
ASAN bot on Mac. This is the second attempt to land.

Restores a larger output buffer size when output stream requiring smaller size
is closed if
no input stream is currently using the same audio device.

BUG=602587
TEST=Manual tests and extra debug logging using a wide mix of audio applications
(audio tag, WebAudio and WebRTC).
==========

[henrika (OOO until Aug 14), 2016-05-11 16:55:20]

henrika@chromium.org changed reviewers:
+ olka@chromium.org

[henrika (OOO until Aug 14), 2016-05-11 16:55:23]

Olga, PTAL.

I don't do anything new here but only resolve an issue tied to unit tests
running under ASAN. This CL fixes those tests. I tried to come up with a
solution that affected as few files as possible. The current approach creates a
special contract, for Mac only, where a real output stream which is about to be
closed, informs/prepares the audio manager about it. This way, the audio manager
will only try to restore I/O buffer sizes for real streams and no fake streams
used under tests. It avoids invalid casting (detected by ASAN) and works well
for us.

[o1ka, 2016-05-12 11:30:17]

Thanks! Don't see any races. A couple of question:

https://codereview.chromium.org/1973503003/diff/40001/media/audio/mac/audio_a...
File media/audio/mac/audio_auhal_mac.cc (right):

https://codereview.chromium.org/1973503003/diff/40001/media/audio/mac/audio_a...
media/audio/mac/audio_auhal_mac.cc:129:
manager_->PrepareForAttemptToIncreaseIOBufferSize(device_);
Ah ok, I see what you mean. Could we simplify it a bit by just having a
dedicated method ReleaseOutputStream(stream, device) in the manager? Than we can
get rid of both device_id_used_by_closing_output_stream_ and
try_to_increase_io_buffer_size_, and make a single call here.

https://codereview.chromium.org/1973503003/diff/40001/media/audio/mac/audio_m...
File media/audio/mac/audio_manager_mac.cc (right):

https://codereview.chromium.org/1973503003/diff/40001/media/audio/mac/audio_m...
media/audio/mac/audio_manager_mac.cc:872: output_io_buffer_size_map_[device_id]
= buffer_size;
Should we really do it here? (See my comment to l.933)

https://codereview.chromium.org/1973503003/diff/40001/media/audio/mac/audio_m...
media/audio/mac/audio_manager_mac.cc:877: if (desired_buffer_size > buffer_size)
{
shouldn't we just check (output_io_buffer_size_map_[device_id] <
desired_buffer_size_)  here instead of iterating through all the streams?

https://codereview.chromium.org/1973503003/diff/40001/media/audio/mac/audio_m...
media/audio/mac/audio_manager_mac.cc:933: if (!is_input && (result == noErr)) {
if (output_io_buffer_size_map_.count(device_id) == 0), but (result != noErr), it
means you updated that map at l.872 with the value which is not actually set,
isn't it?

[o1ka, 2016-05-12 12:04:15]

https://codereview.chromium.org/1973503003/diff/40001/media/audio/mac/audio_a...
File media/audio/mac/audio_auhal_mac.cc (right):

https://codereview.chromium.org/1973503003/diff/40001/media/audio/mac/audio_a...
media/audio/mac/audio_auhal_mac.cc:129:
manager_->PrepareForAttemptToIncreaseIOBufferSize(device_);
On 2016/05/12 11:30:17, o1ka wrote:
> Ah ok, I see what you mean. Could we simplify it a bit by just having a
> dedicated method ReleaseOutputStream(stream, device) in the manager? Than we
can
> get rid of both device_id_used_by_closing_output_stream_ and
> try_to_increase_io_buffer_size_, and make a single call here.

(Actually, it should be named differently, because overloading is not encouraged
by Chromium if I'm not mistaken)

[henrika (OOO until Aug 14), 2016-05-12 13:02:08]

Thanks. PTAL

https://codereview.chromium.org/1973503003/diff/40001/media/audio/mac/audio_a...
File media/audio/mac/audio_auhal_mac.cc (right):

https://codereview.chromium.org/1973503003/diff/40001/media/audio/mac/audio_a...
media/audio/mac/audio_auhal_mac.cc:129:
manager_->PrepareForAttemptToIncreaseIOBufferSize(device_);
Good idea. Done.

https://codereview.chromium.org/1973503003/diff/40001/media/audio/mac/audio_a...
media/audio/mac/audio_auhal_mac.cc:129:
manager_->PrepareForAttemptToIncreaseIOBufferSize(device_);
Well, that does not seem that strict but OK I can change it ;-)

https://codereview.chromium.org/1973503003/diff/40001/media/audio/mac/audio_m...
File media/audio/mac/audio_manager_mac.cc (right):

https://codereview.chromium.org/1973503003/diff/40001/media/audio/mac/audio_m...
media/audio/mac/audio_manager_mac.cc:872: output_io_buffer_size_map_[device_id]
= buffer_size;
Nahh, you are probably correct. I must agree that storing it here does not feel
OK. Removing.

https://codereview.chromium.org/1973503003/diff/40001/media/audio/mac/audio_m...
media/audio/mac/audio_manager_mac.cc:877: if (desired_buffer_size > buffer_size)
{
If you don't mind I would like to avoid making such a change at this stage. I
did a quick test and found that there were cased when the size would be
increased but it wasn't. Did not spend time on figuring out why.

https://codereview.chromium.org/1973503003/diff/40001/media/audio/mac/audio_m...
media/audio/mac/audio_manager_mac.cc:933: if (!is_input && (result == noErr)) {
Lines 868 - 873 have been  removed.

[henrika (OOO until Aug 14), 2016-05-12 13:03:59]

Thanks. PTAL

[o1ka, 2016-05-12 13:35:20]

Looks fine, I just got one more question

https://codereview.chromium.org/1973503003/diff/60001/media/audio/mac/audio_m...
File media/audio/mac/audio_manager_mac.cc (right):

https://codereview.chromium.org/1973503003/diff/60001/media/audio/mac/audio_m...
media/audio/mac/audio_manager_mac.cc:943: const size_t& actual_size =
output_io_buffer_size_map_[device_id];
I just realized: why do we need to cache buffer sizes in
output_io_buffer_size_map_, if we can take one from AudioUnitGetProperty(
audio_unit, kAudioDevicePropertyBufferFrameSize, ...)?

[henrika (OOO until Aug 14), 2016-05-12 14:25:05]

https://codereview.chromium.org/1973503003/diff/60001/media/audio/mac/audio_m...
File media/audio/mac/audio_manager_mac.cc (right):

https://codereview.chromium.org/1973503003/diff/60001/media/audio/mac/audio_m...
media/audio/mac/audio_manager_mac.cc:943: const size_t& actual_size =
output_io_buffer_size_map_[device_id];
That's a very good question. I wanted to avoid possibly complex Core Audio API
calls as far as possible. Note that it can in fact fail and I have found cases
where it can block for long periods. It feels safer and "lighter" to use the map
instead.

[o1ka, 2016-05-12 14:34:29]

On 2016/05/12 14:25:05, henrika wrote:
>
https://codereview.chromium.org/1973503003/diff/60001/media/audio/mac/audio_m...
> File media/audio/mac/audio_manager_mac.cc (right):
> 
>
https://codereview.chromium.org/1973503003/diff/60001/media/audio/mac/audio_m...
> media/audio/mac/audio_manager_mac.cc:943: const size_t& actual_size =
> output_io_buffer_size_map_[device_id];
> That's a very good question. I wanted to avoid possibly complex Core Audio API
> calls as far as possible. Note that it can in fact fail and I have found cases
> where it can block for long periods. It feels safer and "lighter" to use the
map
> instead.

If your opinion is that caching (and potential bugs related to it) is safer than
Core Audio call (with sometimes unpredictable behavior) - I'm fine with it.
Could you put it into a comment though?
LGTM % comment on why we cache.

[henrika (OOO until Aug 14), 2016-05-12 14:53:56]

Description was changed from

==========
(Relanding) Restores larger output buffer size when output stream requiring
smaller size is closed

Previous attempt was done at: https://codereview.chromium.org/1903753002/ but it
broke an
ASAN bot on Mac. This is the second attempt to land.

Restores a larger output buffer size when output stream requiring smaller size
is closed if
no input stream is currently using the same audio device.

BUG=602587
TEST=Manual tests and extra debug logging using a wide mix of audio applications
(audio tag, WebAudio and WebRTC).
==========

to

==========
(Relanding) Restores larger output buffer size when output stream requiring
smaller size is closed

Previous attempt was done at: https://codereview.chromium.org/1903753002/ but it
broke an
ASAN bot on Mac. This is the second attempt to land.

Restores a larger output buffer size when output stream requiring smaller size
is closed if
no input stream is currently using the same audio device.

BUG=602587
TEST=Manual tests and extra debug logging using a wide mix of audio applications
(audio tag, WebAudio and WebRTC).
I have also verified that all tests now passes under ASAN
==========

[henrika (OOO until Aug 14), 2016-05-12 15:02:47]

The CQ bit was checked by henrika@chromium.org

[henrika (OOO until Aug 14), 2016-05-12 15:02:47]

The patchset sent to the CQ was uploaded after l-g-t-m from olka@chromium.org
Link to the patchset: https://codereview.chromium.org/1973503003/#ps80001
(title: "added comment")

[commit-bot: I haz the power, 2016-05-12 15:03:00]

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

[henrika (OOO until Aug 14), 2016-05-12 15:03:15]

Thanks Olga. Let's hope it sticks this time :-)

[commit-bot: I haz the power, 2016-05-12 16:05:22]

Description was changed from

==========
(Relanding) Restores larger output buffer size when output stream requiring
smaller size is closed

Previous attempt was done at: https://codereview.chromium.org/1903753002/ but it
broke an
ASAN bot on Mac. This is the second attempt to land.

Restores a larger output buffer size when output stream requiring smaller size
is closed if
no input stream is currently using the same audio device.

BUG=602587
TEST=Manual tests and extra debug logging using a wide mix of audio applications
(audio tag, WebAudio and WebRTC).
I have also verified that all tests now passes under ASAN
==========

to

==========
(Relanding) Restores larger output buffer size when output stream requiring
smaller size is closed

Previous attempt was done at: https://codereview.chromium.org/1903753002/ but it
broke an
ASAN bot on Mac. This is the second attempt to land.

Restores a larger output buffer size when output stream requiring smaller size
is closed if
no input stream is currently using the same audio device.

BUG=602587
TEST=Manual tests and extra debug logging using a wide mix of audio applications
(audio tag, WebAudio and WebRTC).
I have also verified that all tests now passes under ASAN
==========

[commit-bot: I haz the power, 2016-05-12 16:05:22]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-12 16:06:29]

Description was changed from

==========
(Relanding) Restores larger output buffer size when output stream requiring
smaller size is closed

Previous attempt was done at: https://codereview.chromium.org/1903753002/ but it
broke an
ASAN bot on Mac. This is the second attempt to land.

Restores a larger output buffer size when output stream requiring smaller size
is closed if
no input stream is currently using the same audio device.

BUG=602587
TEST=Manual tests and extra debug logging using a wide mix of audio applications
(audio tag, WebAudio and WebRTC).
I have also verified that all tests now passes under ASAN
==========

to

==========
(Relanding) Restores larger output buffer size when output stream requiring
smaller size is closed

Previous attempt was done at: https://codereview.chromium.org/1903753002/ but it
broke an
ASAN bot on Mac. This is the second attempt to land.

Restores a larger output buffer size when output stream requiring smaller size
is closed if
no input stream is currently using the same audio device.

BUG=602587
TEST=Manual tests and extra debug logging using a wide mix of audio applications
(audio tag, WebAudio and WebRTC).
I have also verified that all tests now passes under ASAN

Committed: https://crrev.com/04fa4fc3ea17b4b93904f79bcf7fb0ced0dfb90b
Cr-Commit-Position: refs/heads/master@{#393261}
==========

[commit-bot: I haz the power, 2016-05-12 16:06:30]

Patchset 5 (id:??) landed as
https://crrev.com/04fa4fc3ea17b4b93904f79bcf7fb0ced0dfb90b
Cr-Commit-Position: refs/heads/master@{#393261}