New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 152780 link

Starred by 6 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2012
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug



Sign in to add a comment

Audio is bad on MacBook Pro with USB headset

Project Member Reported by kjellander@chromium.org, Sep 27 2012

Issue description

Version: 23.0.1271.10
OS: OS X 10.8.2 on a MacBook Pro

What steps will reproduce the problem?
1. Start Chrome on a MacBook Pro 10.8.2 with a Soundblaster Arena USB headset plugged in
2. Go to https://apprtc.appspot.com/?debug=loopback
3. Select the Soundblaster Arena headset microphone and click Allow.
4. The loopback audio is really bad, crackling and can barely be heard. 

What is the expected output? What do you see instead?
The audio should be of decent quality. Instead it's very poor.

Please use labels and text to provide additional information.
It seems to be related to the USB headset, since the quality is OK with the built in mic.
After a few seconds the remote video freezes too (see  issue 152776 )
 
Verified on 159216.

- Audio in media_unitests is OK.
- Audio in content_unittests is OK.

Will dig some more and prepare for Shijing.


Cc: crogers@google.com dalecur...@chromium.org
Status: Started
Shijing: I am able to "solve" the problem by modifying the buffer size in audio_util.cc from the current 128 to 480 (using 48kHz). Then it sounds perfect again.

I am a bit confused here because 128 seems to work in the unit test but not in the apprct demo and the callback stream should be identical in these cases (I am obviously wrong here). Using pass-through mode should not be required for good audio.

Will check the input from kjellander about "It seems to be related to the USB headset" next.

Restored buffer size to 128 again and used the built-in speaker (mic from USB headset) and now the audio was perfect again.

It seems like we have issued with the new FIFO chain for some cases and that the timing is not perfect when we do e.g. 128->480. It will most likely render a callback sequence that is not as "clean" as the pass-through mode.

Dale: we have discussed this earlier; would it be possible to add a new enumerator which will allow clients to override the default buffer settings in audio_util? I don't think the current solution will fly for us and we have full control of the sizes we need. WDYT?

Comment 3 by xians@chromium.org, Sep 28 2012

I was able to reproduce the issue only once with Canary, then it "disappeared".

It is still my strong concern that using 128 samples as buffer size is not a optimal solution for non-webaudio clients, especially for time critical applications like webrtc. I will not be surprised if we have similar issues with flapper with some particular devices or some low performance machines.

Compared with having a new enumerator, I prefer allowing using users' buffer size if it is smaller than 2048. (I guess no low latency client is using a buffer size bigger than 2048)

Dale and Chris, what do you think? If you guys strongly want keeping 128 for Mac, 447 for windows, we can make a new enumerator to make sure webrtc won't be affected by FIFO, otherwise I will allow the users to use their own buffer size if it is smaller than 2048, otherwise it wil use 2048. (note, 2048 might not be the best number for all platform. On Mac, 1024 might be preferred). 

Comment 4 by crogers@google.com, Sep 28 2012

Shijing, one option is to add the enumerator called something like FORCE_BUFFER_SIZE.

But, probably a better option is to implement AudioManagerMac::GetPreferredLowLatencyOutputStreamParameters() where it can make some optimal choices, especially in the case where no resampling is used, it can accept any buffer size up to 2047

In other words, for your case, it will see 480, and pass that value through, using that without the FIFO
I think Chris' solution is sufficient for now as adding that enum won't help us if Flash is having a bad time with 128 as well.

I suspect things are okay with Flash though, since we're drawing from a larger buffer (2048, a multiple of 128 even) to begin with.  On top of that I've observed (what is probably) the AudioUnit API requesting more data than configured if the system is under too much load to fulfill the configured request size.

We must dig more into this issue later even if we can find a short term
fix. The current behavior makes me worried because it works sometimes and
sometimes is sounds bad and it is not trivial to figure out what goes
wrong. Mac has always felt like the most stable platform and we should
ensure that we come back to that state.

Comment 7 by xians@chromium.org, Sep 28 2012

Thanks Chris and Dale. Implementing GetPreferredLowLatencyOutputStreamParameters() is exactly what I want to do. What I will do is that when the client' buffer size is <= 2047, we will use the client's buffer size to open the soundcard. I will do this for all platforms to have a consistent behavior, hope it makes sense to you.

Dale, I am sure that audio on Mac will work well, as it does for webaudio. But we don't want to consume too much CPU to avoid starving other threads. : )

Henrik, I quite agree with you. Even though we workaround this problem by skipping FIFO, but it does not say that the problem won't happen when resampler is used. We should continue to investigate on it unless we are sure that the problem is fixed in any case.

Comment 8 by crogers@google.com, Sep 28 2012

This is the case *only* if the sample-rates match.


No, please don't change the other platforms!  We have
this specifically tuned to work as well as possible for each platform.  For
example, on Windows with WASAPI it *really* needs to use the recommended
buffer size.  We rely on this behavior for Flash.

That's why GetPreferredLowLatencyOutputSt**reamParameters() is a virtual
method and has *different* implementations for each OS.

Comment 9 by crogers@google.com, Sep 28 2012

This change should *only* be made if the sample-rates match.

No, please don't change the other platforms!  We have this specifically tuned to work as well as possible for each platform.  For example, on Windows with WASAPI it *really* needs to use the recommended buffer size.  We rely on this behavior for Flash.

That's why GetPreferredLowLatencyOutputStreamParameters() is a virtual method and has *different* implementations for each OS, because the behavior of each OS is different.

Comment 10 by xians@chromium.org, Sep 28 2012

Then it is very likely the audio is choppy when the resampler is needed, if it is true, we need to find another fix and merge them to branch.

Linux has been the case, so the question is on windows. On windows, it is using a buffer size exactly equivalent to 10ms data.  Chris, could you please explain a bit why this is important tor flash, is it related to jitter that you talked about in the email thread?
For webrtc, we have a drawback of using this number when the native sample rate is 44100, where GetAudioHardwareBufferSize returns 448, but the most optimal number for webrtc is 440. This might not be critical since it should only add complexity but hurt nothing else. Henrik, what do you think about this?
Project Member

Comment 11 by bugdroid1@chromium.org, Oct 2 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=159666

------------------------------------------------------------------------
r159666 | xians@chromium.org | 2012-10-02T09:51:32.513842Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/media/audio/mac/audio_manager_mac.cc?r1=159666&r2=159665&pathrev=159666
   M http://src.chromium.org/viewvc/chrome/trunk/src/media/audio/mac/audio_manager_mac.h?r1=159666&r2=159665&pathrev=159666

Use clients' preferred buffer size when the sample rates match and it is a number smaller than 2047.

BUG= 152780 
TEST=apprtc.appspot.com/?debug=loopback, no choppy audio

Review URL: https://chromiumcodereview.appspot.com/11014015
------------------------------------------------------------------------
Labels: Merge-Requested
Status: Fixed

Comment 14 by kareng@google.com, Oct 3 2012

does this work on canary?

Status: Verified
Yes, it is verified on both canary and latest code.

Comment 16 by kareng@google.com, Oct 4 2012

Labels: -Merge-Requested Merge-Approved

Comment 17 by kareng@google.com, Oct 4 2012

Labels: -Merge-Approved Merge-Merged
Committed revision 160266 i merged it for u.
Project Member

Comment 18 by bugdroid1@chromium.org, Oct 4 2012

Labels: merge-merged-1271
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=160266

------------------------------------------------------------------------
r160266 | karen@chromium.org | 2012-10-04T23:32:36.245797Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1271/src/media/audio/mac/audio_manager_mac.cc?r1=160266&r2=160265&pathrev=160266
   M http://src.chromium.org/viewvc/chrome/branches/1271/src/media/audio/mac/audio_manager_mac.h?r1=160266&r2=160265&pathrev=160266

Merge 159666 - Use clients' preferred buffer size when the sample rates match and it is a number smaller than 2047.

BUG= 152780 
TEST=apprtc.appspot.com/?debug=loopback, no choppy audio

Review URL: https://chromiumcodereview.appspot.com/11014015

TBR=xians@chromium.org
Review URL: https://codereview.chromium.org/11040050
------------------------------------------------------------------------
Thanks Karen.
 Issue webrtc:912  has been merged into this issue.
 Issue webrtc:912  has been merged into this issue.

Comment 22 by kareng@google.com, Oct 8 2012

Labels: ReleaseBlock-Stable
Status: Assigned
reverted this from m23 caused a crash. http://code.google.com/p/chromium/issues/detail?id=154352
Project Member

Comment 23 by bugdroid1@chromium.org, Oct 8 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=160688

------------------------------------------------------------------------
r160688 | karen@chromium.org | 2012-10-08T20:12:08.872730Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1271/src/media/audio/mac/audio_manager_mac.cc?r1=160688&r2=160687&pathrev=160688
   M http://src.chromium.org/viewvc/chrome/branches/1271/src/media/audio/mac/audio_manager_mac.h?r1=160688&r2=160687&pathrev=160688

Revert 160266 - Merge 159666 - Use clients' preferred buffer size when the sample rates match and it is a number smaller than 2047.

BUG= 152780 
TEST=apprtc.appspot.com/?debug=loopback, no choppy audio

Review URL: https://chromiumcodereview.appspot.com/11014015

TBR=xians@chromium.org
Review URL: https://codereview.chromium.org/11040050

TBR=karen@chromium.org
Review URL: https://codereview.chromium.org/11096006
------------------------------------------------------------------------
Project Member

Comment 24 by bugdroid1@chromium.org, Oct 9 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=160820

------------------------------------------------------------------------
r160820 | xians@chromium.org | 2012-10-09T11:28:42.730216Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/media/audio/mac/audio_manager_mac.cc?r1=160820&r2=160819&pathrev=160820
   M http://src.chromium.org/viewvc/chrome/trunk/src/media/audio/mac/audio_manager_mac.h?r1=160820&r2=160819&pathrev=160820

Revert 159666 - Use clients' preferred buffer size when the sample rates match and it is a number smaller than 2047.
To be in par with M23.

BUG= 152780 
TEST=apprtc.appspot.com/?debug=loopback, no choppy audio

Review URL: https://chromiumcodereview.appspot.com/11014015

TBR=xians@chromium.org
Review URL: https://codereview.chromium.org/11028092
------------------------------------------------------------------------
no issues on 24.0.1290.1 on Mac OSX 10.8.2

Comment 26 by xians@chromium.org, Oct 11 2012

Cc: xians@chromium.org
Owner: henrika@chromium.org
Assign the bug to Henrik.
Mergedinto: 154352
Status: Duplicate

Comment 29 by xians@chromium.org, Oct 15 2012

Mergedinto: 154352

Comment 30 by xians@chromium.org, Oct 15 2012

Labels: -merge-merged-1271 Merge-Requested
Mergedinto:
Status: Started

Comment 31 by kareng@google.com, Oct 15 2012

Labels: -Merge-Requested Merge-Approved
ok, approved for:

http://src.chromium.org/viewvc/chrome?view=rev&revision=160759
http://src.chromium.org/viewvc/chrome?view=rev&revision=161328
http://src.chromium.org/viewvc/chrome?view=rev&revision=161847

PLS MAKE SURE there are no new crashes in canary :)
Searched latest Canaries for traces of old crashes but was not able to find any.
Example: https://chromecrash.corp.google.com/browse?q=product.name%20CONTAINS%20'Chrome'%20AND%20custom_data.ChromeCrashProto.magic_signature_1.file_path%20CONTAINS%20'media%2Faudio%2Fmac'

shows that there are no crashes in media/audio/mac after 24.0.1294.0.
Doing merge now in the above order.
Done. Adding condensed log just in case:

D:\drover>drover --merge 160759 --branch 1271
------------------------------------------------------------------------
r160759 | dalecurtis@chromium.org | 2012-10-09 03:12:04 +0200 (Tue, 09 Oct 2012) | 9 lines
Changed paths:
   M /trunk/src/media/audio/mac/audio_low_latency_input_mac.cc
   M /trunk/src/media/audio/mac/audio_low_latency_output_mac.cc
   M /trunk/src/media/base/audio_bus.cc

Prevent AudioLowLatencyInputMac from changing frame sizes.

Otherwise we'll hit a CHECK() later since the shared memory isn't configured for the new frame size.

BUG= 154352 
TEST=mac build no longer crashes.


Review URL: https://chromiumcodereview.appspot.com/11086009
------------------------------------------------------------------------
Is this the correct revision? [y|n]:

Sending        media\audio\mac\audio_low_latency_input_mac.cc
Sending        media\audio\mac\audio_low_latency_output_mac.cc
Sending        media\base\audio_bus.cc
Transmitting file data ...
Committed revision 162098.

D:\drover>drover --merge 161328 --branch 1271
------------------------------------------------------------------------
r161328 | henrika@chromium.org | 2012-10-11 13:54:31 +0200 (Thu, 11 Oct 2012) | 11 lines
Changed paths:
   M /trunk/src/media/audio/mac/audio_low_latency_input_mac.cc
   M /trunk/src/media/audio/mac/audio_low_latency_input_mac.h
   M /trunk/src/media/audio/mac/audio_low_latency_input_mac_unittest.cc
   M /trunk/src/media/audio/mac/audio_low_latency_output_mac.cc

Ensures that we always run the low-latency audio capture at natively 128 audio frames. A FIFO is used to adapt to the buffer size requested by the client.

Tested with WebRTC clients in Chrome as well.

Added media_unittests as well for different sample rates.

BUG= 154352 
TEST=content_unittests --v=1 --gtest_filter=WebRTC*


Review URL: https://chromiumcodereview.appspot.com/11099013
------------------------------------------------------------------------
Is this the correct revision? [y|n]:

Sending        media\audio\mac\audio_low_latency_input_mac.cc
Sending        media\audio\mac\audio_low_latency_input_mac.h
Sending        media\audio\mac\audio_low_latency_input_mac_unittest.cc
Sending        media\audio\mac\audio_low_latency_output_mac.cc
Transmitting file data ....
Committed revision 162099.

------------------------------------------------------------------------
r161847 | henrika@chromium.org | 2012-10-15 12:24:56 +0200 (Mon, 15 Oct 2012) | 8 lines
Changed paths:
   M /trunk/src/media/audio/mac/audio_low_latency_output_mac.cc

Removed CHECK to avoid browser crash for Mac OS X.

Reverts part  of http://src.chromium.org/viewvc/chrome?view=rev&revision=161328.

TBR=dalecurtis
BUG= 155608 

Review URL: https://codereview.chromium.org/11143011
------------------------------------------------------------------------
Is this the correct revision? [y|n]:y

Sending        media\audio\mac\audio_low_latency_output_mac.cc
Transmitting file data .
Committed revision 162100.

Comment 34 by kareng@google.com, Oct 16 2012

Labels: -Merge-Approved
ty henrika!

Comment 35 by kareng@google.com, Oct 16 2012

Status: Fixed
closing this so QA can verify.
Cc: jeffreyc@chromium.org
Project Member

Comment 37 by bugdroid1@chromium.org, Mar 10 2013

Labels: -Area-Internals -Feature-WebRTC -Mstone-23 Cr-Internals-WebRTC Cr-Internals M-23
Project Member

Comment 38 by bugdroid1@chromium.org, May 24 2013

Labels: -Cr-Internals-WebRTC Cr-Blink-WebRTC

Comment 39 by laforge@google.com, Jul 24 2013

Cc: -jeffreyc@chromium.org

Sign in to add a comment