Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15217002:
Using native sampling rate and optimal buffer size for audio on Android. (Closed)
|
Created:
7 years, 7 months ago by leozwang1 Modified:
7 years, 6 months ago Reviewers:
no longer working on chromium, tommi (sloooow) - chröme, qinmin, Raymond Toy, Raymond Toy (Google) CC:
chromium-reviews, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, jam, darin-cc_chromium.org Base URL:
https://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionUsing native sampling rate and optimal buffer size for audio on Android.
We want to use native sampling rate and optimial buffer size which is returned by
framework on Android to achieve high audio quality.
BUG=https://code.google.com/p/webrtc/issues/detail?id=1669
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202464
Patch Set 1 #
Total comments: 2
Patch Set 2 : rebase #
Total comments: 4
Patch Set 3 : size -> frame #Patch Set 4 : take max #
Total comments: 4
Patch Set 5 : address Qinmin's comments #
Total comments: 24
Patch Set 6 : Address Shijing's comment #
Total comments: 5
Patch Set 7 : change const name #
Total comments: 6
Patch Set 8 : change buffer_size #Patch Set 9 : rebase #
Total comments: 8
Patch Set 10 : bytes->frames and address Min's comments #
Total comments: 6
Patch Set 11 : change func name #
Messages
Total messages: 40 (0 generated)
I cannot update https://chromiumcodereview.appspot.com/14430003/ I created this one which also added your patch which correctly calculate framesize.
lgtm, with a small comment in audio_manager_android. Do you know if the computed buffer size will be ok for webrtc? https://chromiumcodereview.appspot.com/15217002/diff/1/media/audio/android/au... File media/audio/android/audio_manager_android.cc (right): https://chromiumcodereview.appspot.com/15217002/diff/1/media/audio/android/au... media/audio/android/audio_manager_android.cc:129: return GetMinOutputBufferSize(sample_rate, channels); I wonder if we should return the max of GetMinOutputBufferSize and kTargetFrameSize, in case kTargetFrameSize is too small?
https://chromiumcodereview.appspot.com/15217002/diff/6001/media/audio/android... File media/audio/android/audio_manager_android.cc (right): https://chromiumcodereview.appspot.com/15217002/diff/6001/media/audio/android... media/audio/android/audio_manager_android.cc:187: int AudioManagerAndroid::GetMinInputBufferSize(int rate, int channels) { Should this be changed to return frames like GetMinOutputBufferSize (GetMinOutputFrameSize) to be consistent? I think the AudioParameter objects work in frames. https://chromiumcodereview.appspot.com/15217002/diff/6001/media/audio/android... media/audio/android/audio_manager_android.cc:193: int AudioManagerAndroid::GetMinOutputBufferSize(int rate, int channels) { Should this be named GetMinOutputFrameSize instead of BufferSize, since we're returning the number of frames?
https://chromiumcodereview.appspot.com/15217002/diff/1/media/audio/android/au... File media/audio/android/audio_manager_android.cc (right): https://chromiumcodereview.appspot.com/15217002/diff/1/media/audio/android/au... media/audio/android/audio_manager_android.cc:129: return GetMinOutputBufferSize(sample_rate, channels); All devices I have tested have "LowLatency", this else path has not been well tested. form my experience, this return value might be small for Chromium. https://chromiumcodereview.appspot.com/15217002/diff/6001/media/audio/android... File media/audio/android/audio_manager_android.cc (right): https://chromiumcodereview.appspot.com/15217002/diff/6001/media/audio/android... media/audio/android/audio_manager_android.cc:187: int AudioManagerAndroid::GetMinInputBufferSize(int rate, int channels) { On 2013/05/16 19:30:01, rtoy wrote: > Should this be changed to return frames like GetMinOutputBufferSize > (GetMinOutputFrameSize) to be consistent? I think the AudioParameter objects > work in frames. Done. https://chromiumcodereview.appspot.com/15217002/diff/6001/media/audio/android... media/audio/android/audio_manager_android.cc:193: int AudioManagerAndroid::GetMinOutputBufferSize(int rate, int channels) { On 2013/05/16 19:30:01, rtoy wrote: > Should this be named GetMinOutputFrameSize instead of BufferSize, since we're > returning the number of frames? Done.
Hi Qinmin and Tommi, please review this CL.
I don't know the Android code that well and I'm definitely not qualified to review the java code :) Can you ask Shijing to review the C++ code? I'm not sure who best to ask for the java part.
https://chromiumcodereview.appspot.com/15217002/diff/15001/media/base/android... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://chromiumcodereview.appspot.com/15217002/diff/15001/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:111: if (channels == 1) java coding style need {} for if statements https://chromiumcodereview.appspot.com/15217002/diff/15001/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:124: if (channels == 1) ditto
Hi Min and Raymond, pls take a look at patch 5. Tommi, thanks. Shijing, please review this patch. https://chromiumcodereview.appspot.com/15217002/diff/15001/media/base/android... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://chromiumcodereview.appspot.com/15217002/diff/15001/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:111: if (channels == 1) On 2013/05/17 16:25:23, qinmin wrote: > java coding style need {} for if statements Done. https://chromiumcodereview.appspot.com/15217002/diff/15001/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:124: if (channels == 1) On 2013/05/17 16:25:23, qinmin wrote: > ditto Done.
https://codereview.chromium.org/15217002/diff/22002/media/audio/android/audio... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/15217002/diff/22002/media/audio/android/audio... media/audio/android/audio_manager_android.cc:60: int size = GetMinInputFrameSize(GetNativeOutputSampleRate(), 2); nit, size -> buffer_size https://codereview.chromium.org/15217002/diff/22002/media/audio/android/audio... media/audio/android/audio_manager_android.cc:120: int AudioManagerAndroid::GetAudioOptimalOutputFrameSize(int sample_rate, nit, s/GetAudioOptimalOutputFrameSize/GetOptimalOutputFrameSize/g https://codereview.chromium.org/15217002/diff/22002/media/audio/android/audio... media/audio/android/audio_manager_android.cc:122: const int kTargetFrameSize = 2048; kTargetFrameSize is the same as kDefaultBufferSize, move kDefaultBufferSize to global and use it instead https://codereview.chromium.org/15217002/diff/22002/media/audio/android/audio... media/audio/android/audio_manager_android.cc:127: return ((kTargetFrameSize + frame_size / 2) / frame_size) * frame_size; I have to admit that I am not familiar with PROPERTY_OUTPUT_FRAMES_PER_BUFFER, not sure why it is "optimal" to be a multiple of the number, could you please elaborate the reasons of doing so and why kTargetFrameSize is 2048? More explicitly, why not do it this way? const int kNumberBuffers = 4; // or whatever, decided from testing. reutrn kNumberBuffers * frame_size; https://codereview.chromium.org/15217002/diff/22002/media/audio/android/audio... media/audio/android/audio_manager_android.cc:128: } else nit, add { } https://codereview.chromium.org/15217002/diff/22002/media/audio/android/audio... media/audio/android/audio_manager_android.cc:147: buffer_size = buffer_size <= 0 ? kDefaultBufferSize : buffer_size; how buffer_size can be <= 0? https://codereview.chromium.org/15217002/diff/22002/media/audio/android/audio... media/audio/android/audio_manager_android.cc:182: int AudioManagerAndroid::GetNativeOutputSampleRate() { Does GetNativeOutputSampleRate() get the native sample rate based on the device currently being used? Or it is some number used by the audioflinger? In case it is the sample rate used by the audioflinger, cache the value and only query the hardware at the first times. https://codereview.chromium.org/15217002/diff/22002/media/audio/android/audio... media/audio/android/audio_manager_android.cc:188: int AudioManagerAndroid::GetMinInputFrameSize(int rate, int channels) { could you please add comments here, to explain why the buffer size needs to divide /2 / channels? https://codereview.chromium.org/15217002/diff/22002/media/audio/android/audio... media/audio/android/audio_manager_android.cc:190: base::android::AttachCurrentThread(), rate, channels) / 2 / channels; nit, use sizeof(int16) instead of 2 https://codereview.chromium.org/15217002/diff/22002/media/audio/android/audio... media/audio/android/audio_manager_android.cc:199: bool AudioManagerAndroid::IsAudioLowLatencySupported() { I don't think you need a separate IsAudioLowLatencySupported(), merge it to GetAudioLowLatencyFrameSize https://codereview.chromium.org/15217002/diff/22002/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/15217002/diff/22002/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:29: private static final int DEFAULT_FRAME_PER_BUFFER = 256; why 256 is the default one? Add comment please.
https://codereview.chromium.org/15217002/diff/22002/media/audio/android/audio... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/15217002/diff/22002/media/audio/android/audio... media/audio/android/audio_manager_android.cc:60: int size = GetMinInputFrameSize(GetNativeOutputSampleRate(), 2); On 2013/05/20 09:59:26, xians1 wrote: > nit, size -> buffer_size Done. https://codereview.chromium.org/15217002/diff/22002/media/audio/android/audio... media/audio/android/audio_manager_android.cc:120: int AudioManagerAndroid::GetAudioOptimalOutputFrameSize(int sample_rate, On 2013/05/20 09:59:26, xians1 wrote: > nit, s/GetAudioOptimalOutputFrameSize/GetOptimalOutputFrameSize/g Done. https://codereview.chromium.org/15217002/diff/22002/media/audio/android/audio... media/audio/android/audio_manager_android.cc:122: const int kTargetFrameSize = 2048; On 2013/05/20 09:59:26, xians1 wrote: > kTargetFrameSize is the same as kDefaultBufferSize, move kDefaultBufferSize to > global and use it instead Done. https://codereview.chromium.org/15217002/diff/22002/media/audio/android/audio... media/audio/android/audio_manager_android.cc:127: return ((kTargetFrameSize + frame_size / 2) / frame_size) * frame_size; In order to achieve to "low latency", the buffer size must be a multiple of the buffer size returned by GetAudioLowLatencyFrameSize(). I think audiofligner implements in this way to achieve low latency. Probably it's not the best answer to your Q, but let me know if you need more info or we can discuss offline. https://codereview.chromium.org/15217002/diff/22002/media/audio/android/audio... media/audio/android/audio_manager_android.cc:128: } else On 2013/05/20 09:59:26, xians1 wrote: > nit, add { } Done. https://codereview.chromium.org/15217002/diff/22002/media/audio/android/audio... media/audio/android/audio_manager_android.cc:147: buffer_size = buffer_size <= 0 ? kDefaultBufferSize : buffer_size; good catch, it was latency code, removed it. https://codereview.chromium.org/15217002/diff/22002/media/audio/android/audio... media/audio/android/audio_manager_android.cc:182: int AudioManagerAndroid::GetNativeOutputSampleRate() { Then we could also cache all return value from GetBufferSize apis. Because these apis are not experience, maybe we can keep them can call them as needed? https://codereview.chromium.org/15217002/diff/22002/media/audio/android/audio... media/audio/android/audio_manager_android.cc:188: int AudioManagerAndroid::GetMinInputFrameSize(int rate, int channels) { On 2013/05/20 09:59:26, xians1 wrote: > could you please add comments here, to explain why the buffer size needs to > divide /2 / channels? Done. https://codereview.chromium.org/15217002/diff/22002/media/audio/android/audio... media/audio/android/audio_manager_android.cc:190: base::android::AttachCurrentThread(), rate, channels) / 2 / channels; On 2013/05/20 09:59:26, xians1 wrote: > nit, use sizeof(int16) instead of 2 Done. https://codereview.chromium.org/15217002/diff/22002/media/audio/android/audio... media/audio/android/audio_manager_android.cc:199: bool AudioManagerAndroid::IsAudioLowLatencySupported() { Currently it's called once, but it's just wrapper of java api, we might need to call it in different use cases, for example, if IsLowLentencyDevice we do some specially. I prefer to keep it a separated api for now. https://codereview.chromium.org/15217002/diff/22002/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/15217002/diff/22002/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:29: private static final int DEFAULT_FRAME_PER_BUFFER = 256; On 2013/05/20 09:59:26, xians1 wrote: > why 256 is the default one? > Add comment please. Done.
ping
https://chromiumcodereview.appspot.com/15217002/diff/32001/media/audio/androi... File media/audio/android/audio_manager_android.cc (right): https://chromiumcodereview.appspot.com/15217002/diff/32001/media/audio/androi... media/audio/android/audio_manager_android.cc:61: static const int kDefaultBufferSize = 1024; This is kind of confusing since it has the same name but different value from the definition at line 25. Should the name be changed, either here or line 25? https://chromiumcodereview.appspot.com/15217002/diff/32001/media/audio/androi... media/audio/android/audio_manager_android.cc:125: const int kDefaultBufferSize = 2048; Shouldn't this be removed now?
we used to share one const parameter, I created two now. https://codereview.chromium.org/15217002/diff/32001/media/audio/android/audio... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/15217002/diff/32001/media/audio/android/audio... media/audio/android/audio_manager_android.cc:61: static const int kDefaultBufferSize = 1024; On 2013/05/21 16:30:38, rtoy wrote: > This is kind of confusing since it has the same name but different value from > the definition at line 25. Should the name be changed, either here or line 25? Done. https://codereview.chromium.org/15217002/diff/32001/media/audio/android/audio... media/audio/android/audio_manager_android.cc:125: const int kDefaultBufferSize = 2048; On 2013/05/21 16:30:38, rtoy wrote: > Shouldn't this be removed now? Done. https://codereview.chromium.org/15217002/diff/32001/media/audio/android/audio... media/audio/android/audio_manager_android.cc:125: const int kDefaultBufferSize = 2048; On 2013/05/21 16:30:38, rtoy wrote: > Shouldn't this be removed now? Done.
https://codereview.chromium.org/15217002/diff/22002/media/audio/android/audio... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/15217002/diff/22002/media/audio/android/audio... media/audio/android/audio_manager_android.cc:127: return ((kTargetFrameSize + frame_size / 2) / frame_size) * frame_size; On 2013/05/20 17:41:05, leozwang1 wrote: > In order to achieve to "low latency", the buffer size must be a multiple of the > buffer size returned by GetAudioLowLatencyFrameSize(). I think audiofligner > implements in this way to achieve low latency. Probably it's not the best answer > to your Q, but let me know if you need more info or we can discuss offline. I understand that it needs to be a multiple of GetAudioLowLatencyFrameSize(), but my question is why not doing it this way? const int kLowLatencyNumberBuffers = 4; // or whatever. return kLowLatencyNumberBuffers * frame_size; https://codereview.chromium.org/15217002/diff/42001/media/audio/android/audio... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/15217002/diff/42001/media/audio/android/audio... media/audio/android/audio_manager_android.cc:124: int channels) { indentation. https://codereview.chromium.org/15217002/diff/42001/media/audio/android/audio... media/audio/android/audio_manager_android.cc:133: GetMinOutputFrameSize(sample_rate, channels)); why the output code is different from the way of input buffer size? shouldn't it be: int buffer_size = GetMinOutputFrameSize(sample_rate, channels); return buffer_size > 0 ? buffer_size : kDefaultOutputBufferSize https://codereview.chromium.org/15217002/diff/42001/media/audio/android/audio... media/audio/android/audio_manager_android.cc:141: int buffer_size = GetOptimalOutputFrameSize(sample_rate, 2); This is not correct. In case input_params.IsValid() below, we will open the device with input_params.channel_layout(), it might be mono or stereo. Move the code below, and get the buffer size based on the correct channel layout.
https://codereview.chromium.org/15217002/diff/22002/media/audio/android/audio... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/15217002/diff/22002/media/audio/android/audio... media/audio/android/audio_manager_android.cc:127: return ((kTargetFrameSize + frame_size / 2) / frame_size) * frame_size; On 2013/05/21 21:52:32, xians1 wrote: > On 2013/05/20 17:41:05, leozwang1 wrote: > > In order to achieve to "low latency", the buffer size must be a multiple of > the > > buffer size returned by GetAudioLowLatencyFrameSize(). I think audiofligner > > implements in this way to achieve low latency. Probably it's not the best > answer > > to your Q, but let me know if you need more info or we can discuss offline. > > I understand that it needs to be a multiple of GetAudioLowLatencyFrameSize(), > but my question is why not doing it this way? > const int kLowLatencyNumberBuffers = 4; // or whatever. > return kLowLatencyNumberBuffers * frame_size; > The low latency frame size varies quite a bit depending on the device: 144 for Galaxy Nexus, 256 for Nexus 7(?), and 800 or so for Nexus S. A fixed number of buffers would probably make some devices have too large or too small a size. This is a compromise so that all devices get a size of about 2048 frames.
https://codereview.chromium.org/15217002/diff/42001/media/audio/android/audio... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/15217002/diff/42001/media/audio/android/audio... media/audio/android/audio_manager_android.cc:133: GetMinOutputFrameSize(sample_rate, channels)); The return value of GetMinOutputFrameSize is small on some android devices, we take max of these two variables. https://codereview.chromium.org/15217002/diff/42001/media/audio/android/audio... media/audio/android/audio_manager_android.cc:141: int buffer_size = GetOptimalOutputFrameSize(sample_rate, 2); I was puzzled by the original logic, from what you said, if input_params.IsValid(), the code always use kDefaultBufferSize regardless channel_layout(), is it correct? Just want to make sure I understand the problem before I upload a patch.
PTAL
https://codereview.chromium.org/15217002/diff/42001/media/audio/android/audio... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/15217002/diff/42001/media/audio/android/audio... media/audio/android/audio_manager_android.cc:141: int buffer_size = GetOptimalOutputFrameSize(sample_rate, 2); On 2013/05/22 06:33:19, leozwang1 wrote: > I was puzzled by the original logic, from what you said, if > input_params.IsValid(), the code always use kDefaultBufferSize regardless > channel_layout(), is it correct? Just want to make sure I understand the problem > before I upload a patch. The value we are setting here is frames_per_buffer, which can be interpreted as the numbers of samples per channel when getting a packet. You can check the details in audio_parameters.h To answer your question more explicitly, previously we are using kDefaultBufferSize as the buffer size for 1 channel. If it is in stereo mode, the total buffer size will be 2*kDefaultBufferSize https://codereview.chromium.org/15217002/diff/53001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/15217002/diff/53001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:112: private static int getMinInputBufSize(int sampleRate, int channels) { I am wondering if you really need the |channels| to get the buffer size, I would guess that you will get the same result regardless channels ==1 or channels == 2 here. https://codereview.chromium.org/15217002/diff/53001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:126: private static int getMinOutputBufSize(int sampleRate, int channels) { the same here, could you please verify it?
https://codereview.chromium.org/15217002/diff/53001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/15217002/diff/53001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:112: private static int getMinInputBufSize(int sampleRate, int channels) { java api takes number of channel as the input parameter and indeed adjusts the size based on the input.
https://codereview.chromium.org/15217002/diff/53001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/15217002/diff/53001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:112: private static int getMinInputBufSize(int sampleRate, int channels) { On 2013/05/22 21:03:41, leozwang1 wrote: > java api takes number of channel as the input parameter and indeed adjusts the > size based on the input. I know, but please note that you divide the return value by the channels later, for example : Java_AudioManagerAndroid_getMinInputBufSize( base::android::AttachCurrentThread(), rate, channels) / sizeof(int16) / channels
https://codereview.chromium.org/15217002/diff/53001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/15217002/diff/53001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:112: private static int getMinInputBufSize(int sampleRate, int channels) { which converts size in bytes to frames. I think I have comments about it.
https://codereview.chromium.org/15217002/diff/53001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/15217002/diff/53001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:112: private static int getMinInputBufSize(int sampleRate, int channels) { On 2013/05/22 21:27:42, leozwang1 wrote: > which converts size in bytes to frames. I think I have comments about it. Right, that is exactly what I am trying to say, we are getting the same value in terms of frames for mono and stereo. For example, for mono, we are getting 512 as the returned value of getMinInputBufSize(sample_rate, 1) here, and AudioManagerAndroid::GetMinInputFrameSize() will return 256. For stereo, we will get 1024 from getMinInputBufSize(sample_rate, 2), but the return value of AudioManagerAndroid::GetMinInputFrameSize() is still 256.
Here I think you assume the input and output have linear correlation. without looking at source code to get details how this api calculates buffer size, let's use the java api as what is described in doc. What I can do is, change in Java from getBufferSize to getFrameSize, which is OK with me, but I don't prefer.
On 2013/05/22 21:51:35, leozwang1 wrote: > Here I think you assume the input and output have linear correlation. without > looking at source code to get details how this api calculates buffer size, let's > use the java api as what is described in doc. > What I can do is, change in Java from getBufferSize to getFrameSize, which is OK > with me, but I don't prefer. I think it is a reasonable assumption and that is why I asked for a verification from some tests. If you are strongly reluctant to verify it and just want to stick to the JAVA api, it is OK for me. lgtm
vendor needs to implements HAL and returns to framework, so the return value is vendor/hardware specific which takes number of channels as the one of inputs. Again, my thoughs is in AudioManagerAndroid.java instead of having getMinInputBufSize, create getMinInpuFrameSize, as the result, we can avoid buffer_size->frame_size conversation in native code. I think it's a good compromise and work around. What do you think?
lgtm%nits https://chromiumcodereview.appspot.com/15217002/diff/53001/media/base/android... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://chromiumcodereview.appspot.com/15217002/diff/53001/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:95: " Board: " + Build.BOARD + " Device: " + Build.DEVICE + nit: fix the indentation
https://chromiumcodereview.appspot.com/15217002/diff/53001/media/base/android... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://chromiumcodereview.appspot.com/15217002/diff/53001/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:95: " Board: " + Build.BOARD + " Device: " + Build.DEVICE + On 2013/05/23 16:27:39, qinmin wrote: > nit: fix the indentation Done.
ping reviewers again.
https://chromiumcodereview.appspot.com/15217002/diff/70001/media/base/android... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://chromiumcodereview.appspot.com/15217002/diff/70001/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:127: return AudioRecord.getMinBufferSize( Is there an inconsistency here? The method name is getMinInputFrameSize, implying the result is in frames, but AudioRecord.getMinBufferSize returns the buffer size in bytes. https://chromiumcodereview.appspot.com/15217002/diff/70001/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:147: return AudioTrack.getMinBufferSize( Similar inconsistency as for getMinInputFrameSize. This method is getMinOutputFrameSize, but it seems that AudioTrack.getMinBufferSize is in bytes.
https://chromiumcodereview.appspot.com/15217002/diff/70001/media/base/android... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://chromiumcodereview.appspot.com/15217002/diff/70001/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:127: return AudioRecord.getMinBufferSize( AudioRecord.getMinBufferSize returns buffer size in bytes, however, AudioManagerAndroid.getMinInputFrameSize() is a wrapper which returns buffer size in FRAMES which does divide (2*channels). My initial plan was use it as a bypass api, however, since Shijing doesn't like buffer size in bytes<->frames conversation in native code, I changed it in java to make native code more clean. At the point, I'm ok with either approach.
https://chromiumcodereview.appspot.com/15217002/diff/70001/media/base/android... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://chromiumcodereview.appspot.com/15217002/diff/70001/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:127: return AudioRecord.getMinBufferSize( On 2013/05/24 17:30:39, leozwang1 wrote: > AudioRecord.getMinBufferSize returns buffer size in bytes, however, > AudioManagerAndroid.getMinInputFrameSize() is a wrapper which returns buffer > size in FRAMES which does divide (2*channels). My initial plan was use it as a > bypass api, however, since Shijing doesn't like buffer size in bytes<->frames > conversation in native code, I changed it in java to make native code more > clean. > At the point, I'm ok with either approach. > Oh, sorry! My window size was such that I didn't see the division by 2*channels. Everything is fine then.
ping Shijing again! please.
On 2013/05/23 16:23:41, leozwang1 wrote: > vendor needs to implements HAL and returns to framework, so the return value is > vendor/hardware specific which takes number of channels as the one of inputs. > > Again, my thoughs is in AudioManagerAndroid.java instead of having > getMinInputBufSize, create getMinInpuFrameSize, as the result, we can avoid > buffer_size->frame_size conversation in native code. I think it's a good > compromise and work around. What do you think? Sorry, I thought we were done here. It sounds more symmetric to have getMinInpuFrameSize, since Chrome is actually dealing with frame size. I will suggest you do that change and hide the code in AudioManagerAndroid.java
please check patch 9 and I need owner's stamp too.
almost there, one minor question https://chromiumcodereview.appspot.com/15217002/diff/70001/media/audio/androi... File media/audio/android/audio_manager_android.cc (right): https://chromiumcodereview.appspot.com/15217002/diff/70001/media/audio/androi... media/audio/android/audio_manager_android.cc:201: int AudioManagerAndroid::GetAudioLowLatencyFrameSize() { Can this GetAudioLowLatencyFrameSize be used by both input and output?
https://chromiumcodereview.appspot.com/15217002/diff/70001/media/audio/androi... File media/audio/android/audio_manager_android.cc (right): https://chromiumcodereview.appspot.com/15217002/diff/70001/media/audio/androi... media/audio/android/audio_manager_android.cc:201: int AudioManagerAndroid::GetAudioLowLatencyFrameSize() { this low latency mode is for output only now.
can we change the name to GetLowLatencyOutputFrameSize ? I am ooo now, lgtm to this cl regardless the naming. On 2013/05/27 16:11:37, leozwang1 wrote: > https://chromiumcodereview.appspot.com/15217002/diff/70001/media/audio/androi... > File media/audio/android/audio_manager_android.cc (right): > > https://chromiumcodereview.appspot.com/15217002/diff/70001/media/audio/androi... > media/audio/android/audio_manager_android.cc:201: int > AudioManagerAndroid::GetAudioLowLatencyFrameSize() { > this low latency mode is for output only now.
will commit patch 10.thanks all.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leozwang@chromium.org/15217002/84001
Message was sent while issue was closed.
Change committed as 202464 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
