Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1903753002:
Restores larger output buffer size when output stream requiring smaller size is closed (Closed)
|
Created:
4 years, 8 months ago by henrika (OOO until Aug 14) Modified:
4 years, 7 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, tommi (sloooow) - chröme Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRestores 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).
Committed: https://crrev.com/2f3180327f0c71452bb38e853c048c43945047b7
Cr-Commit-Position: refs/heads/master@{#390638}
Patch Set 1 #Patch Set 2 : First working version #Patch Set 3 : Improved comments #Patch Set 4 : nit #Patch Set 5 : Improvements based on tests where default output device was changed #
Total comments: 13
Patch Set 6 : Improvements suggested by olka@ #Patch Set 7 : nit #
Total comments: 3
Patch Set 8 : One more round based on feedback from olka@ #
Total comments: 21
Patch Set 9 : Final round perhaps #Patch Set 10 : nit #
Total comments: 7
Patch Set 11 : Removed potential race conditions #
Total comments: 6
Patch Set 12 : Final changes ;-) #Patch Set 13 : rebased #
Total comments: 8
Patch Set 14 : nit #
Messages
Total messages: 33 (8 generated)
Description was changed from ========== rebased BUG= ========== to ========== Restores larger output buffer size when output stream requiring smaller size is closed *** NOT READY YET *** BUG=602587 ==========
Description was changed from ========== Restores larger output buffer size when output stream requiring smaller size is closed *** NOT READY YET *** BUG=602587 ========== to ========== 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@chromium.org changed reviewers: + grunell@chromium.org, olka@webrtc.org
Olga, PTAL ;-)
olka@chromium.org changed reviewers: + olka@chromium.org - olka@webrtc.org
Nice! Could you clarify some things for me? https://codereview.chromium.org/1903753002/diff/80001/media/audio/mac/audio_m... File media/audio/mac/audio_manager_mac.cc (right): https://codereview.chromium.org/1903753002/diff/80001/media/audio/mac/audio_m... media/audio/mac/audio_manager_mac.cc:865: // Do NOT set the buffer size if there is another output stream using May we probably skip those checks when called from IncreaseIOBufferSizeIfPossible? They n^2 complexity if called for each IncreaseIOBufferSizeIfPossible() call. (Though, n should be small so it probably does not matter; depends on what are the requirements for ReleaseOutputStream) https://codereview.chromium.org/1903753002/diff/80001/media/audio/mac/audio_m... media/audio/mac/audio_manager_mac.cc:931: bool AudioManagerMac::IncreaseIOBufferSizeIfPossible(AudioDeviceID device_id) { Add a thread check for clarity? https://codereview.chromium.org/1903753002/diff/80001/media/audio/mac/audio_m... media/audio/mac/audio_manager_mac.cc:940: size_map[stream->requested_buffer_size()] = stream->actual_buffer_size(); Shouldn't it be that all the streams have the same actual buffer size? I really like this map trick, but I'm not sure if it is needed. https://codereview.chromium.org/1903753002/diff/80001/media/audio/mac/audio_m... media/audio/mac/audio_manager_mac.cc:1006: void AudioManagerMac::ReleaseOutputStream(AudioOutputStream* stream) { I assume it can't race with MakeLowLatencyOutputStream(), am I correct? How is it guaranteed? Can we have a thread check? https://codereview.chromium.org/1903753002/diff/80001/media/audio/mac/audio_m... File media/audio/mac/audio_manager_mac.h (right): https://codereview.chromium.org/1903753002/diff/80001/media/audio/mac/audio_m... media/audio/mac/audio_manager_mac.h:12: #include <map> move to .cc? https://codereview.chromium.org/1903753002/diff/80001/media/audio/mac/audio_m... media/audio/mac/audio_manager_mac.h:130: bool IncreaseIOBufferSizeIfPossible(AudioDeviceID device_id); Protected?
Thanks Olga! Done changes as you suggested. PTAL https://codereview.chromium.org/1903753002/diff/80001/media/audio/mac/audio_m... File media/audio/mac/audio_manager_mac.cc (right): https://codereview.chromium.org/1903753002/diff/80001/media/audio/mac/audio_m... media/audio/mac/audio_manager_mac.cc:865: // Do NOT set the buffer size if there is another output stream using Latest implementation only calls it once for all streams sharing the same device id. IMO, we can keep as is. https://codereview.chromium.org/1903753002/diff/80001/media/audio/mac/audio_m... media/audio/mac/audio_manager_mac.cc:931: bool AudioManagerMac::IncreaseIOBufferSizeIfPossible(AudioDeviceID device_id) { On 2016/04/22 12:24:24, o1ka wrote: > Add a thread check for clarity? Done. https://codereview.chromium.org/1903753002/diff/80001/media/audio/mac/audio_m... media/audio/mac/audio_manager_mac.cc:940: size_map[stream->requested_buffer_size()] = stream->actual_buffer_size(); Now changed. https://codereview.chromium.org/1903753002/diff/80001/media/audio/mac/audio_m... media/audio/mac/audio_manager_mac.cc:1006: void AudioManagerMac::ReleaseOutputStream(AudioOutputStream* stream) { Added thread checks. https://codereview.chromium.org/1903753002/diff/80001/media/audio/mac/audio_m... File media/audio/mac/audio_manager_mac.h (right): https://codereview.chromium.org/1903753002/diff/80001/media/audio/mac/audio_m... media/audio/mac/audio_manager_mac.h:12: #include <map> Used a map as member first. Then forgot to remove. Gone now. Thanks. https://codereview.chromium.org/1903753002/diff/80001/media/audio/mac/audio_m... media/audio/mac/audio_manager_mac.h:130: bool IncreaseIOBufferSizeIfPossible(AudioDeviceID device_id); My bad. Now even private.
Thank you! DCHECKs clarify things a lot. A couple more questions. https://codereview.chromium.org/1903753002/diff/80001/media/audio/mac/audio_m... File media/audio/mac/audio_manager_mac.cc (right): https://codereview.chromium.org/1903753002/diff/80001/media/audio/mac/audio_m... media/audio/mac/audio_manager_mac.cc:865: // Do NOT set the buffer size if there is another output stream using On 2016/04/25 13:54:13, henrika wrote: > Latest implementation only calls it once for all streams sharing the same device > id. IMO, we can keep as is. Acknowledged. https://codereview.chromium.org/1903753002/diff/120001/media/audio/mac/audio_... File media/audio/mac/audio_manager_mac.cc (right): https://codereview.chromium.org/1903753002/diff/120001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:945: // In addition, build a list of output streams using |device_id|. It does not seem to be used - am I missing something? https://codereview.chromium.org/1903753002/diff/120001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:980: for (auto it = requested_sizes.begin(); it != requested_sizes.end(); ++it) { Do I understand it correctly that this information is important for diagnostics, and otherwise we would just find out the minimum requested buffer size without keeping a list and sorting it? Can we probably just print this information at l.956 and keep there track of a minimum requested size found so far, instead of keeping a vector? In the end it looks like only the minimum value is used.
Done. Thanks! PTAL ;-) https://codereview.chromium.org/1903753002/diff/120001/media/audio/mac/audio_... File media/audio/mac/audio_manager_mac.cc (right): https://codereview.chromium.org/1903753002/diff/120001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:945: // In addition, build a list of output streams using |device_id|. I actually only use the first stream in the list. Will change data structure.
https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... File media/audio/mac/audio_auhal_mac.cc (right): https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... media/audio/mac/audio_auhal_mac.cc:228: actual_io_buffer_frame_size_ = number_of_frames; Is the intention that only the first time the requested number of frames differ from what we request should update |actual_io_buffer_frame_size_|? That's what will happen now.
lgtm (please fix the nits) https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... File media/audio/mac/audio_manager_mac.cc (right): https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:953: // All active output streams uses the same actual I/O buffer size given nit: uses->use https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:954: // a unique device ID. Hence, it is sufficient to store one value once. nit: "one value" -> "the value" https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:957: output_stream = stream; nit: all you need is audio_unit, so you can probably store it instead, and check for non-zero actual_size at l.952 and l.966 https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:975: if (min_requested_size == actual_size) { add DCHECK_GE(min_requested_size, actual_size)? https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:985: const size_t increased_io_buffer_frame_size = min_requested_size; const size_t& Actually, I think min_requested_size is a reasonable name and renaming is not necessary. https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:993: DCHECK_EQ(io_buffer_frame_size, increased_io_buffer_frame_size); DCHECK size_was_changed as well?
PTAL https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... File media/audio/mac/audio_auhal_mac.cc (right): https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... media/audio/mac/audio_auhal_mac.cc:228: actual_io_buffer_frame_size_ = number_of_frames; Good point. I can move it up one scope instead. That should do it. But on the other hand, the existing implementation is broken for this case anyhow since the FIFO will not be updated. So, not sure if my change helps. https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... File media/audio/mac/audio_manager_mac.cc (right): https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:953: // All active output streams uses the same actual I/O buffer size given On 2016/04/27 09:00:38, o1ka wrote: > nit: uses->use Done. https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:954: // a unique device ID. Hence, it is sufficient to store one value once. On 2016/04/27 09:00:38, o1ka wrote: > nit: "one value" -> "the value" Done. https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:957: output_stream = stream; I can't really see any benefit by doing that. https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:975: if (min_requested_size == actual_size) { Sorry but where do you want me to add that check? https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:985: const size_t increased_io_buffer_frame_size = min_requested_size; On 2016/04/27 09:00:38, o1ka wrote: > const size_t& > Actually, I think min_requested_size is a reasonable name and renaming is not > necessary. Done. https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:993: DCHECK_EQ(io_buffer_frame_size, increased_io_buffer_frame_size); Actually, Yes. I could not do it earlier but now when only one stream is changed I can.
Oh wait, I've got a question https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... File media/audio/mac/audio_manager_mac.cc (right): https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:957: output_stream = stream; On 2016/04/27 12:12:31, henrika wrote: > I can't really see any benefit by doing that. Clearer intentions of what you really need? https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:975: if (min_requested_size == actual_size) { On 2016/04/27 12:12:30, henrika wrote: > Sorry but where do you want me to add that check? Right above this line. Makes sense? https://codereview.chromium.org/1903753002/diff/180001/media/audio/mac/audio_... File media/audio/mac/audio_auhal_mac.cc (right): https://codereview.chromium.org/1903753002/diff/180001/media/audio/mac/audio_... media/audio/mac/audio_auhal_mac.cc:223: base::subtle::Release_Store(&actual_io_buffer_frame_size_, What is the guarantee that actual size we get in IncreaseIOBufferSizeIfPossible() from the first output stream is up-to-date? Isn't it a race?
No changes yet, only feedback to latest input from olka@. https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... File media/audio/mac/audio_manager_mac.cc (right): https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:957: output_stream = stream; I would like to keep it as is if possible. More simple to check for nullptr below. The AudioUnit is a Mac specific structure which can ne null checked as well but it does not feel as safe. https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:975: if (min_requested_size == actual_size) { OK, above makes sense. I figured you meant here after the check. https://codereview.chromium.org/1903753002/diff/180001/media/audio/mac/audio_... File media/audio/mac/audio_auhal_mac.cc (right): https://codereview.chromium.org/1903753002/diff/180001/media/audio/mac/audio_... media/audio/mac/audio_auhal_mac.cc:223: base::subtle::Release_Store(&actual_io_buffer_frame_size_, Sorry, don't understand. Race between what?
https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... File media/audio/mac/audio_manager_mac.cc (right): https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:957: output_stream = stream; On 2016/04/27 13:36:43, henrika wrote: > I would like to keep it as is if possible. More simple to check for nullptr > below. The AudioUnit is a Mac specific structure which can ne null checked as > well but it does not feel as safe. You don't need to check for null, you can just check that actual_size is zero (see my initial comment). But it's not that important, so - up to you. https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:975: if (min_requested_size == actual_size) { On 2016/04/27 13:36:43, henrika wrote: > OK, above makes sense. I figured you meant here after the check. Acknowledged. https://codereview.chromium.org/1903753002/diff/180001/media/audio/mac/audio_... File media/audio/mac/audio_auhal_mac.cc (right): https://codereview.chromium.org/1903753002/diff/180001/media/audio/mac/audio_... media/audio/mac/audio_auhal_mac.cc:223: base::subtle::Release_Store(&actual_io_buffer_frame_size_, On 2016/04/27 13:36:43, henrika wrote: > Sorry, don't understand. Race between what? See my question: what is the guarantee that we updated _all_ the streams here in Render() before executing IncreaseIOBufferSizeIfPossible()? Otherwise we may end up with an outdated actual size there.
https://codereview.chromium.org/1903753002/diff/180001/media/audio/mac/audio_... File media/audio/mac/audio_auhal_mac.cc (right): https://codereview.chromium.org/1903753002/diff/180001/media/audio/mac/audio_... media/audio/mac/audio_auhal_mac.cc:223: base::subtle::Release_Store(&actual_io_buffer_frame_size_, On 2016/04/27 15:04:31, o1ka wrote: > On 2016/04/27 13:36:43, henrika wrote: > > Sorry, don't understand. Race between what? > > See my question: what is the guarantee that we updated _all_ the streams here in > Render() before executing IncreaseIOBufferSizeIfPossible()? Otherwise we may end > up with an outdated actual size there. I'm not following either. Olga, do you mean that the buffer size the OS requests could change when a stream is removed? Maybe we can discuss offline. https://codereview.chromium.org/1903753002/diff/180001/media/audio/mac/audio_... media/audio/mac/audio_auhal_mac.cc:529: // TODO(henrika): it might be possible to remove this part. What is required to do to figure out if this part is needed? Could it be done now? Iiuc, doing this will (hopefully) ensure we have the correct actual size as early as possible. Does that matter? Would be nice to not have code if it doesn't add any value.
CL is now very much simplified. Streams no longer stores actual size, only audio manager does and maps it against device ID. Should not be racy any longer. No atomics. Also removed redundant code. PTAL https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... File media/audio/mac/audio_manager_mac.cc (right): https://codereview.chromium.org/1903753002/diff/140001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:957: output_stream = stream; Your word is my law ;-) Changed. https://codereview.chromium.org/1903753002/diff/180001/media/audio/mac/audio_... File media/audio/mac/audio_auhal_mac.cc (right): https://codereview.chromium.org/1903753002/diff/180001/media/audio/mac/audio_... media/audio/mac/audio_auhal_mac.cc:223: base::subtle::Release_Store(&actual_io_buffer_frame_size_, Redesigned. Stream no longer contains actual buffer size, hence no race. Instead the audio manager keeps track of actual size per device id. Much better now IMHO. Thanks ;-) https://codereview.chromium.org/1903753002/diff/180001/media/audio/mac/audio_... media/audio/mac/audio_auhal_mac.cc:529: // TODO(henrika): it might be possible to remove this part. Removed now ;-)
https://codereview.chromium.org/1903753002/diff/200001/media/audio/mac/audio_... File media/audio/mac/audio_manager_mac.h (right): https://codereview.chromium.org/1903753002/diff/200001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.h:145: bool IncreaseIOBufferSizeIfPossible(AudioDeviceID device_id); Would it be possible to merge this function with MaybeChangeBufferSize()? Just one general function that maybe changes the size to the optimal one.
https://codereview.chromium.org/1903753002/diff/200001/media/audio/mac/audio_... File media/audio/mac/audio_manager_mac.h (right): https://codereview.chromium.org/1903753002/diff/200001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.h:145: bool IncreaseIOBufferSizeIfPossible(AudioDeviceID device_id); I would like to avoid that at this stage since it would make it even more complicated. This new method is only designed for output. Maybe if input side is included as well. At this stage I can't see any benefit by doing such a merge.
https://codereview.chromium.org/1903753002/diff/200001/media/audio/mac/audio_... File media/audio/mac/audio_manager_mac.cc (right): https://codereview.chromium.org/1903753002/diff/200001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:934: output_io_buffer_size_map_[device_id] = buffer_size; You update it even in case of error; is it correct? Also, you overwritten the previous value at l.872, which means that if we should not update it in case of an error, we have an irrelevant value in the map. https://codereview.chromium.org/1903753002/diff/200001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:958: actual_size = output_io_buffer_size_map_[device_id]; nit: Now when you use a map it does not look quite nice to assign actual_size in the cycle. I would probably use (|min_requested_size| < maxint) as another indicator that |audio_unit| is initialized. (That's up to you though) (And if you initialize actual_size outside the cycle, it can be just a const ref to the map value)
Thanks. PTAL. https://codereview.chromium.org/1903753002/diff/200001/media/audio/mac/audio_... File media/audio/mac/audio_manager_mac.cc (right): https://codereview.chromium.org/1903753002/diff/200001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:934: output_io_buffer_size_map_[device_id] = buffer_size; Thanks. Now fixed. https://codereview.chromium.org/1903753002/diff/200001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:958: actual_size = output_io_buffer_size_map_[device_id]; On 2016/04/28 12:57:40, o1ka wrote: > nit: Now when you use a map it does not look quite nice to assign actual_size in > the cycle. I would probably use (|min_requested_size| < maxint) as another > indicator that |audio_unit| is initialized. > (That's up to you though) > (And if you initialize actual_size outside the cycle, it can be just a const ref > to the map value) Done.
On 2016/04/28 14:40:23, henrika wrote: > Thanks. PTAL. > > https://codereview.chromium.org/1903753002/diff/200001/media/audio/mac/audio_... > File media/audio/mac/audio_manager_mac.cc (right): > > https://codereview.chromium.org/1903753002/diff/200001/media/audio/mac/audio_... > media/audio/mac/audio_manager_mac.cc:934: output_io_buffer_size_map_[device_id] > = buffer_size; > Thanks. Now fixed. > > https://codereview.chromium.org/1903753002/diff/200001/media/audio/mac/audio_... > media/audio/mac/audio_manager_mac.cc:958: actual_size = > output_io_buffer_size_map_[device_id]; > On 2016/04/28 12:57:40, o1ka wrote: > > nit: Now when you use a map it does not look quite nice to assign actual_size > in > > the cycle. I would probably use (|min_requested_size| < maxint) as another > > indicator that |audio_unit| is initialized. > > (That's up to you though) > > (And if you initialize actual_size outside the cycle, it can be just a const > ref > > to the map value) > > Done. Thank you! lgtm. Since there are no unit tests, I think we need some description of manual testing in the CL. WDYT?
Just two questions and a little comment suggestion. https://codereview.chromium.org/1903753002/diff/240001/media/audio/mac/audio_... File media/audio/mac/audio_manager_mac.cc (right): https://codereview.chromium.org/1903753002/diff/240001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:870: output_io_buffer_size_map_[device_id] = buffer_size; We'll set it here to what's actually used in the AudioUnit. Isn't that what already should be in the map? Or can the actual buffer size change outside of our control? https://codereview.chromium.org/1903753002/diff/240001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:993: DCHECK(size_was_changed); Since it is expected that we actually changed the buffer size, do we have to go through the whole algorithm in MaybeChangeBufferSize()? I guess it is for determining the size to set it to? https://codereview.chromium.org/1903753002/diff/240001/media/audio/mac/audio_... File media/audio/mac/audio_manager_mac.h (right): https://codereview.chromium.org/1903753002/diff/240001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.h:138: // system resources. Explain what is returned.
PTAL https://codereview.chromium.org/1903753002/diff/240001/media/audio/mac/audio_... File media/audio/mac/audio_manager_mac.cc (right): https://codereview.chromium.org/1903753002/diff/240001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:870: output_io_buffer_size_map_[device_id] = buffer_size; It is only to cover for the case when this method returns at 873 and no change is done. I don't think it will even happen but adding it just in case. It can be considered as a safety initialization. https://codereview.chromium.org/1903753002/diff/240001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:993: DCHECK(size_was_changed); It contains all the tools needed to modify the size and it also serves as an extra error check to ensure that I have not missed any state combinations here. In addition, it contains code to cap the size to a valid size range. Hence, a new method called ChangeBufferSize() would be almost identical (perhaps a few lines less code). https://codereview.chromium.org/1903753002/diff/240001/media/audio/mac/audio_... File media/audio/mac/audio_manager_mac.h (right): https://codereview.chromium.org/1903753002/diff/240001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.h:138: // system resources. On 2016/04/29 10:05:01, Henrik Grunell wrote: > Explain what is returned. Done.
lgtm. Thanks Henrik! https://codereview.chromium.org/1903753002/diff/240001/media/audio/mac/audio_... File media/audio/mac/audio_manager_mac.cc (right): https://codereview.chromium.org/1903753002/diff/240001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:870: output_io_buffer_size_map_[device_id] = buffer_size; On 2016/04/29 12:08:27, henrika wrote: > It is only to cover for the case when this method returns at 873 and no change > is done. I don't think it will even happen but adding it just in case. It can be > considered as a safety initialization. Acknowledged. https://codereview.chromium.org/1903753002/diff/240001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:993: DCHECK(size_was_changed); On 2016/04/29 12:08:27, henrika wrote: > It contains all the tools needed to modify the size and it also serves as an > extra error check to ensure that I have not missed any state combinations here. > In addition, it contains code to cap the size to a valid size range. Hence, a > new method called ChangeBufferSize() would be almost identical (perhaps a few > lines less code). Acknowledged.
The CQ bit was checked by henrika@chromium.org
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/1903753002/#ps260001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1903753002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1903753002/260001
Message was sent while issue was closed.
Description was changed from ========== 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 ========== 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). ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:260001) has been created in https://codereview.chromium.org/1936553002/ by dvadym@chromium.org. The reason for reverting is: This CL breaks Asan tests on Mac 10.9: https://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests....
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/2f3180327f0c71452bb38e853c048c43945047b7 Cr-Commit-Position: refs/heads/master@{#390638}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== 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). Committed: https://crrev.com/2f3180327f0c71452bb38e853c048c43945047b7 Cr-Commit-Position: refs/heads/master@{#390638} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
