Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12806009:
Add OpenSL configurations (Closed)
|
Created:
7 years, 9 months ago by leozwang1 Modified:
7 years, 5 months ago Reviewers:
tommi (sloooow) - chröme, Ami GONE FROM CHROMIUM, qinmin, Raymond Toy, no longer working on chromium, wjia(left Chromium) CC:
chromium-reviews, feature-media-reviews_chromium.org, hellner1 Base URL:
https://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionAdd OpenSL configurations
This patch has been landed in https://src.chromium.org/viewvc/chrome?view=rev&revision=207921
Descritpion:
Both input and output need to be configured to use webrtc.
However, it could affect other elements that use opensl as input or output,
for example, webaudio, but it's not clear to me that how much audio will
be degraded. A better solution could be that configuring OpenSL based on
use cases.
Please let me know your thoughts on this matter.
BUG=222393
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 28
Patch Set 3 : #Patch Set 4 : fix one compliation error #Patch Set 5 : change included header files #Patch Set 6 : cleanup header files #Patch Set 7 : add opensles_outtput.h #
Total comments: 33
Patch Set 8 : address some comments, working on the rest #Patch Set 9 : Addressed comments #
Total comments: 1
Patch Set 10 : moved macro #
Messages
Total messages: 30 (0 generated)
please review it.
https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... File media/audio/android/opensles_input.cc (right): https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... media/audio/android/opensles_input.cc:218: // CreateAudioRecorder and specify SL_IID_ANDROIDCONFIGURATION. create AudioRecorder https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... media/audio/android/opensles_input.cc:223: DCHECK_EQ(SL_RESULT_SUCCESS, err); the DCHECK seems redundant with the if statement below https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... media/audio/android/opensles_input.cc:232: DCHECK_EQ(SL_RESULT_SUCCESS, err); ditto https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... File media/audio/android/opensles_input.h (right): https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... media/audio/android/opensles_input.h:8: #include <SLES/OpenSLES.h> move these to cc https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... File media/audio/android/opensles_output.cc (right): https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... media/audio/android/opensles_output.cc:218: // CreateAudioPlayer and specify SL_IID_ANDROIDCONFIGURATION. white space in CreateAudioPlayer https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... media/audio/android/opensles_output.cc:223: DCHECK_EQ(SL_RESULT_SUCCESS, err); ditto https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... media/audio/android/opensles_output.cc:231: DCHECK_EQ(SL_RESULT_SUCCESS, err); ditto
https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... File media/audio/android/opensles_input.cc (right): https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... media/audio/android/opensles_input.cc:196: const SLuint32 number_of_interfaces = 2; what about: const SLInterfaceID interface_id[] = { SL_IID_ANDROIDSIMPLEBUFFERQUEUE, SL_IID_ANDROIDCONFIGURATION }; const SLuint32 number_of_interfaces = arraysize(interface_id); https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... media/audio/android/opensles_input.cc:223: DCHECK_EQ(SL_RESULT_SUCCESS, err); This DCHECK and the if() that follows are contradictory according to how we use DCHECK in chrome code. The DCHECK means that this should never happen. If that's true, then you don't need the if(). If on the other hand you think this could happen, then log an error instead. Something like: if (err != SL_RESULT_SUCCESS) { DLOG(ERROR) << "GetInterface(SL_IID_ANDROIDCONFIGURATION): " << err; return false; } https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... media/audio/android/opensles_input.cc:232: DCHECK_EQ(SL_RESULT_SUCCESS, err); same here. https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... media/audio/android/opensles_input.cc:238: DCHECK_EQ(SL_RESULT_SUCCESS, err); can you fix these (and below) as well? https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... File media/audio/android/opensles_output.cc (right): https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... media/audio/android/opensles_output.cc:194: const SLuint32 number_of_interfaces = 3; could use arraysize() here. https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... media/audio/android/opensles_output.cc:223: DCHECK_EQ(SL_RESULT_SUCCESS, err); same thing for DCHECK vs if()
Please test the effect on webaudio and <audio> output. The linked bug 500s for me.
Mainly replaced DCHECK. https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... File media/audio/android/opensles_input.cc (right): https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... media/audio/android/opensles_input.cc:196: const SLuint32 number_of_interfaces = 2; On 2013/03/21 14:56:44, tommi wrote: > what about: > const SLInterfaceID interface_id[] = { > SL_IID_ANDROIDSIMPLEBUFFERQUEUE, > SL_IID_ANDROIDCONFIGURATION > }; > > const SLuint32 number_of_interfaces = arraysize(interface_id); Done. https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... media/audio/android/opensles_input.cc:218: // CreateAudioRecorder and specify SL_IID_ANDROIDCONFIGURATION. On 2013/03/21 14:56:19, qinmin wrote: > create AudioRecorder Done. https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... media/audio/android/opensles_input.cc:223: DCHECK_EQ(SL_RESULT_SUCCESS, err); On 2013/03/21 14:56:19, qinmin wrote: > the DCHECK seems redundant with the if statement below Done. https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... media/audio/android/opensles_input.cc:223: DCHECK_EQ(SL_RESULT_SUCCESS, err); On 2013/03/21 14:56:44, tommi wrote: > This DCHECK and the if() that follows are contradictory according to how we use > DCHECK in chrome code. The DCHECK means that this should never happen. If > that's true, then you don't need the if(). > If on the other hand you think this could happen, then log an error instead. > Something like: > > if (err != SL_RESULT_SUCCESS) { > DLOG(ERROR) << "GetInterface(SL_IID_ANDROIDCONFIGURATION): " << err; > return false; > } Done. https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... media/audio/android/opensles_input.cc:232: DCHECK_EQ(SL_RESULT_SUCCESS, err); On 2013/03/21 14:56:19, qinmin wrote: > ditto Done. https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... media/audio/android/opensles_input.cc:232: DCHECK_EQ(SL_RESULT_SUCCESS, err); On 2013/03/21 14:56:44, tommi wrote: > same here. Done. https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... media/audio/android/opensles_input.cc:238: DCHECK_EQ(SL_RESULT_SUCCESS, err); On 2013/03/21 14:56:44, tommi wrote: > can you fix these (and below) as well? Done. https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... File media/audio/android/opensles_input.h (right): https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... media/audio/android/opensles_input.h:8: #include <SLES/OpenSLES.h> On 2013/03/21 14:56:19, qinmin wrote: > move these to cc Done. https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... File media/audio/android/opensles_output.cc (right): https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... media/audio/android/opensles_output.cc:194: const SLuint32 number_of_interfaces = 3; On 2013/03/21 14:56:44, tommi wrote: > could use arraysize() here. Done. https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... media/audio/android/opensles_output.cc:218: // CreateAudioPlayer and specify SL_IID_ANDROIDCONFIGURATION. On 2013/03/21 14:56:19, qinmin wrote: > white space in CreateAudioPlayer Done. https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... media/audio/android/opensles_output.cc:223: DCHECK_EQ(SL_RESULT_SUCCESS, err); On 2013/03/21 14:56:19, qinmin wrote: > ditto Done. https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... media/audio/android/opensles_output.cc:223: DCHECK_EQ(SL_RESULT_SUCCESS, err); On 2013/03/21 14:56:44, tommi wrote: > same thing for DCHECK vs if() Done. https://chromiumcodereview.appspot.com/12806009/diff/7002/media/audio/android... media/audio/android/opensles_output.cc:231: DCHECK_EQ(SL_RESULT_SUCCESS, err); On 2013/03/21 14:56:19, qinmin wrote: > ditto Done.
I defer my part to Qinmin and Ami.
Hi Ami and Raymond, is there a test set I should run through? I tried some <audio> websites, but want to follow the standard QA procedure to see the impact on webaudio and <audio>.
On 2013/03/21 16:00:13, leozwang1 wrote: > Hi Ami and Raymond, is there a test set I should run through? > I tried some <audio> websites, but want to follow the standard QA procedure to > see the impact on webaudio and <audio>. You can't run webaudio because it's not in the sources yet. I will try this patch on my local webaudio build and let you know how it goes.
Hi Raymond, please also test with Revision 189210 which we must have for webrtc.
https://codereview.chromium.org/12806009/diff/7002/media/audio/android/opensl... File media/audio/android/opensles_input.h (right): https://codereview.chromium.org/12806009/diff/7002/media/audio/android/opensl... media/audio/android/opensles_input.h:8: #include <SLES/OpenSLES.h> Cannot move these to cc because header file uses SLES defines.
On 2013/03/21 16:00:13, leozwang1 wrote: > Hi Ami and Raymond, is there a test set I should run through? > I tried some <audio> websites, but want to follow the standard QA procedure to > see the impact on webaudio and <audio>. yihongg@ should have information for you on what to test for <audio>.
https://codereview.chromium.org/12806009/diff/7002/media/audio/android/opensl... File media/audio/android/opensles_input.h (right): https://codereview.chromium.org/12806009/diff/7002/media/audio/android/opensl... media/audio/android/opensles_input.h:8: #include <SLES/OpenSLES.h> only OpenSLES_Android.h is used I suppose, and the other 2 can be moved. On 2013/03/21 17:43:38, leozwang1 wrote: > Cannot move these to cc because header file uses SLES defines. >
SLAndroidSimpleBufferQueueItf requires OpenSLES_Android.h which depends on OpenSLES.h. OpenSLES_AndroidConfiguration.h is included in OpenSLES_Android.h, I removed it in patch 6.
Removing myself as it looks like you have sufficient coverage in reviewers
Raymond, any updates about webaudio test?
No, I didn't try it. I was trying to my webaudio code building with the latest chromium version. If you're ready to commit, just go ahead. If it does something bad on webaudio, we'll fix it then. On Thu, Mar 21, 2013 at 6:35 PM, <leozwang@chromium.org> wrote: > Raymond, any updates about webaudio test? > > https://chromiumcodereview.**appspot.com/12806009/<https://chromiumcodereview... >
Raymond, yes, it's very important for webrtc, if this patch doesn't affect <audio>, I will commit it after review. Ami: Tomorrow, I will let QA to verify if this patch affects <audio>, at the same time, can you please take a quick at the code?
There are a bunch of nits/optional comments here, but the main thing is the next-to-last comment (search for "heart"). https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... File media/audio/android/opensles_input.cc (right): https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_input.cc:87: if (SL_RESULT_SUCCESS != err) { I hope you can make a pass (in a different CL) cleaning up the order of comparisons in these files. https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_input.cc:87: if (SL_RESULT_SUCCESS != err) { What's the deal with the removal of the DCHECKs everywhere in this CL? Under what conditions can these SL calls fail? By line-count, these changes (removal of DCHECKs and addition of DLOGs) is by far the greater part of this CL, yet they are not mentioned in the CL description. Please make sure the description explains *why* you make the changes in the CL. https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_input.cc:89: } FYI braces are optional in the C++ style guide for single-line bodies. (this is a difference to the Java style guide, which requires braces even for single-line bodies) https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_input.cc:203: const SLuint32 number_of_interfaces = arraysize(interface_id); can inline into the CreateAudioRecorder call. https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_input.cc:204: const SLboolean interface_required[number_of_interfaces] = { number_of_interface is unnecessary. https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_input.cc:220: // Create AudioRecorder and specify SL_IID_ANDROIDCONFIGURATION. "Create AudioRecorder" comment is a bit late here, no? (it was actually created at l.208 above) https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_input.cc:230: SLint32 stream_type = SL_ANDROID_RECORDING_PRESET_VOICE_COMMUNICATION; this variable is obscuring what the call below actually does. please inline the value into the call. https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... File media/audio/android/opensles_output.cc (right): https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_output.cc:83: DLOG(WARNING) << "SetPlayState() failed to start playing"; Why is this a WARNING when the rest of the failure logs are ERROR? https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_output.cc:144: DLOG(ERROR) << "CreatePlayer slCreateEngine: " << err; When there is this much duplicated code, it's usually a good idea to do something about it. One idea is to do something like: #define LOG_ON_FAILURE_AND_RETURN(op, retvalue) \ do { \ SLresult err = (op); \ if (err != SL_RESULT_SUCCESS) { \ DLOG(ERROR) << #op << " failed: " << err; \ return retvalue; \ } \ } while (0) Then callsites can replace stanzas like: err = slCreateEngine(engine_object_.Receive(), 1, option, 0, NULL, NULL); if (SL_RESULT_SUCCESS != err) { DLOG(ERROR) << "CreatePlayer slCreateEngine: " << err; return false; } with: LOG_ON_FAILURE_AND_RETURN(slCreateEngine(engine_object_.Receive(), 1, option, 0, NULL, NULL), false); The benefits of doing this are: - consistency in error handling (don't get some as WARNINGs others as ERRORs) - less clutter makes code more readable - what's actually being done is easier to see when it is not obscured by log statements. I don't really care if this is done in this CL or not, but since you're touching all these callsites it seems like an easy choice. https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_output.cc:161: DLOG(ERROR) << "GetInterface(SL_IID_ENGINE): " << err; These log messages are pretty inconsistent; some have CreatePlayer in them, some don't. Again, a macro would help with this too (as you wouldn't have to hand-craft the log message). https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_output.cc:203: const SLuint32 number_of_interfaces = arraysize(interface_id); inline into call below. https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_output.cc:204: const SLboolean interface_required[number_of_interfaces] = { drop number_of_interfaces https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_output.cc:231: SLint32 stream_type = SL_ANDROID_STREAM_VOICE; this variable is obscuring what the call below actually does. please inline the value into the call. https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_output.cc:231: SLint32 stream_type = SL_ANDROID_STREAM_VOICE; This seems to be the heart of the potential problem with this CL (and I wish you'd called it out explicitly as such). If there is a difference in the system implementation between VOICE and MUSIC then it is likely to be a bad idea to do this unconditionally and apply VOICE to <audio> generally. What is the audible effect of commenting out l.231-238 vs. not? What is the audible effect of using VOICE vs. MUSIC for webrtc? What is the audible effect of using VOICE vs. MUSIC for go/ocnyf? What this comes down to is that I don't know enough about the implementations of this feature to make a call in the abstract. If you find someone who does, that's great. If you can experimentally show that VOICE is always better for MUSIC for our usecases, then great. https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... File media/audio/android/opensles_output.h (right): https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_output.h:9: #include <SLES/OpenSLES_Android.h> nit: Are these includes really necessary? Aren't they part of the contract of opensles_util.h below, and thus can be assumed to be included anyway?
https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... File media/audio/android/opensles_input.cc (right): https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_input.cc:87: if (SL_RESULT_SUCCESS != err) { sure I will. https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_input.cc:87: if (SL_RESULT_SUCCESS != err) { Both Tommi and Qinmin suggested to remove DCHECK and add DLOG. I haven't experienced any opensl fail so far, however, there are some reports say opensl could fail, for example, on emulator. https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_input.cc:203: const SLuint32 number_of_interfaces = arraysize(interface_id); On 2013/03/22 16:24:41, Ami Fischman wrote: > can inline into the CreateAudioRecorder call. Done. https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_input.cc:204: const SLboolean interface_required[number_of_interfaces] = { On 2013/03/22 16:24:41, Ami Fischman wrote: > number_of_interface is unnecessary. Done. https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_input.cc:220: // Create AudioRecorder and specify SL_IID_ANDROIDCONFIGURATION. On 2013/03/22 16:24:41, Ami Fischman wrote: > "Create AudioRecorder" comment is a bit late here, no? (it was actually created > at l.208 above) Done. https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... File media/audio/android/opensles_output.cc (right): https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_output.cc:204: const SLboolean interface_required[number_of_interfaces] = { On 2013/03/22 16:24:41, Ami Fischman wrote: > drop number_of_interfaces Done. https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_output.cc:231: SLint32 stream_type = SL_ANDROID_STREAM_VOICE; It's my concern too, we're going to do some tests. The current design of input/output doesn't consider android use case, we may need to change it in future. Basically on android, audioplayer and audiorecor need to what type of stream they're going to play. Any comments on it? https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... File media/audio/android/opensles_output.h (right): https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_output.h:9: #include <SLES/OpenSLES_Android.h> We need these two. opensles_util.h #include <SLES/OpenSLES.h>.
https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... File media/audio/android/opensles_output.cc (right): https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_output.cc:231: SLint32 stream_type = SL_ANDROID_STREAM_VOICE; On 2013/03/22 21:37:09, leozwang1 wrote: > It's my concern too, we're going to do some tests. > The current design of input/output doesn't consider android use case, we may > need to change it in future. Basically on android, audioplayer and audiorecor > need to what type of stream they're going to play. Any comments on it? I don't think you'd want to make the stream contain its purpose. Possibly the best move here would be to expose a global state button (mirroring the android-side SDK surface, which is a global state button) to set/unset voice mode. The real question is when would you push/unpush the button. I suspect that would have to be driven by webrtc code sensing activation/deactivation of a call. https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... File media/audio/android/opensles_output.h (right): https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_output.h:9: #include <SLES/OpenSLES_Android.h> On 2013/03/22 21:37:09, leozwang1 wrote: > We need these two. opensles_util.h #include <SLES/OpenSLES.h>. Isn't this (<SLES/OpenSLES.h>) one of the two here??
https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... File media/audio/android/opensles_output.cc (right): https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_output.cc:231: SLint32 stream_type = SL_ANDROID_STREAM_VOICE; can you teach me how this "global state button (mirroring the android-side SDK surface, which is a global state button)" work? Is there sample code or doc? https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... File media/audio/android/opensles_output.h (right): https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_output.h:9: #include <SLES/OpenSLES_Android.h> If I comment out #include <SLES/OpenSLES.h>, I will get error: /usr/local/google/home/leozwang/src/chromium/src/third_party/android_tools/ndk//platforms/android-14/arch-arm/usr/include/SLES/OpenSLES_Android.h:31:9: error: 'sl_int64_t' does not name a type /usr/local/google/home/leozwang/src/chromium/src/third_party/android_tools/ndk//platforms/android-14/arch-arm/usr/include/SLES/OpenSLES_Android.h:33:9: error: 'sl_uint64_t' does not name a type
https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... File media/audio/android/opensles_output.cc (right): https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_output.cc:231: SLint32 stream_type = SL_ANDROID_STREAM_VOICE; On 2013/03/22 22:44:48, leozwang1 wrote: > can you teach me how this "global state button (mirroring the android-side SDK > surface, which is a global state button)" work? Is there sample code or doc? I just mean that the toggle of what global mode to use shouldn't be tied to a stream. Instead it should be a method on AudioManager that renderers can call via IPC. https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... File media/audio/android/opensles_output.h (right): https://codereview.chromium.org/12806009/diff/42001/media/audio/android/opens... media/audio/android/opensles_output.h:9: #include <SLES/OpenSLES_Android.h> On 2013/03/22 22:44:48, leozwang1 wrote: > If I comment out #include <SLES/OpenSLES.h>, I will get error: > /usr/local/google/home/leozwang/src/chromium/src/third_party/android_tools/ndk//platforms/android-14/arch-arm/usr/include/SLES/OpenSLES_Android.h:31:9: > error: 'sl_int64_t' does not name a type > /usr/local/google/home/leozwang/src/chromium/src/third_party/android_tools/ndk//platforms/android-14/arch-arm/usr/include/SLES/OpenSLES_Android.h:33:9: > error: 'sl_uint64_t' does not name a type OIC the problem is that you're including OpenSLES_Android.h before opensles_util.h so you can't rely on its includes. fine.
PTAL. The answer to your audio quality question is still unknown. https://chromiumcodereview.appspot.com/12806009/diff/42001/media/audio/androi... File media/audio/android/opensles_input.cc (right): https://chromiumcodereview.appspot.com/12806009/diff/42001/media/audio/androi... media/audio/android/opensles_input.cc:87: if (SL_RESULT_SUCCESS != err) { On 2013/03/22 16:24:41, Ami Fischman wrote: > I hope you can make a pass (in a different CL) cleaning up the order of > comparisons in these files. Done. https://chromiumcodereview.appspot.com/12806009/diff/42001/media/audio/androi... media/audio/android/opensles_input.cc:87: if (SL_RESULT_SUCCESS != err) { On 2013/03/22 16:24:41, Ami Fischman wrote: > What's the deal with the removal of the DCHECKs everywhere in this CL? > Under what conditions can these SL calls fail? > > By line-count, these changes (removal of DCHECKs and addition of DLOGs) is by > far the greater part of this CL, yet they are not mentioned in the CL > description. Please make sure the description explains *why* you make the > changes in the CL. Done. https://chromiumcodereview.appspot.com/12806009/diff/42001/media/audio/androi... File media/audio/android/opensles_output.cc (right): https://chromiumcodereview.appspot.com/12806009/diff/42001/media/audio/androi... media/audio/android/opensles_output.cc:144: DLOG(ERROR) << "CreatePlayer slCreateEngine: " << err; On 2013/03/22 16:24:41, Ami Fischman wrote: > When there is this much duplicated code, it's usually a good idea to do > something about it. One idea is to do something like: > #define LOG_ON_FAILURE_AND_RETURN(op, retvalue) \ > do { \ > SLresult err = (op); \ > if (err != SL_RESULT_SUCCESS) { \ > DLOG(ERROR) << #op << " failed: " << err; \ > return retvalue; \ > } \ > } while (0) > > Then callsites can replace stanzas like: > > err = slCreateEngine(engine_object_.Receive(), 1, option, 0, NULL, NULL); > if (SL_RESULT_SUCCESS != err) { > DLOG(ERROR) << "CreatePlayer slCreateEngine: " << err; > return false; > } > > with: > > LOG_ON_FAILURE_AND_RETURN(slCreateEngine(engine_object_.Receive(), 1, option, 0, > NULL, NULL), false); > > The benefits of doing this are: > - consistency in error handling (don't get some as WARNINGs others as ERRORs) > - less clutter makes code more readable - what's actually being done is easier > to see when it is not obscured by log statements. > > I don't really care if this is done in this CL or not, but since you're touching > all these callsites it seems like an easy choice. Done. https://chromiumcodereview.appspot.com/12806009/diff/42001/media/audio/androi... media/audio/android/opensles_output.cc:161: DLOG(ERROR) << "GetInterface(SL_IID_ENGINE): " << err; On 2013/03/22 16:24:41, Ami Fischman wrote: > These log messages are pretty inconsistent; some have CreatePlayer in them, some > don't. > Again, a macro would help with this too (as you wouldn't have to hand-craft the > log message). Done.
Modulo moving the macro into the .cc files (have two copies of it), this is fine, assuming it: 1) does something useful for webrtc; and 2) doesn't regress <audio> How did this CL come about, if you don't at least know #1 above? The concrete version of this is these questions which I asked earlier in the review and which I didn't see an answer to: What is the audible effect of commenting out l.231-238 vs. not? What is the audible effect of using VOICE vs. MUSIC for webrtc? What is the audible effect of using VOICE vs. MUSIC for go/ocnyf? https://chromiumcodereview.appspot.com/12806009/diff/61001/media/audio/androi... File media/audio/android/opensles_util.h (right): https://chromiumcodereview.appspot.com/12806009/diff/61001/media/audio/androi... media/audio/android/opensles_util.h:12: #define LOG_ON_FAILURE_AND_RETURN(op, ...) \ This can't live in a header. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Prepro...
PS#10 addressed all my coding concerns. Audio quality questions are still being discussed out of reviewlog.
On 2013/03/24 20:10:39, Ami Fischman wrote: > PS#10 addressed all my coding concerns. Audio quality questions are still being > discussed out of reviewlog. Any news?
The last email on the subject was from me to Leo and pasted below (slightly edited). Leo's moved to another project since. @hellner do you want to take over the last bits of verification needed for this CL to show that it is an improvement (or at least not a regression) and take it over in a new rietveld issue? On Tue, Apr 30, 2013 at 10:45 PM, Ami Fischman <fischman@google.com> wrote: > I think I finally understand what you're saying. [...] > The actual effect of the CL (excluding code cleanup) is to set the SL_IID_ANDROIDCONFIGURATION's interface: > - Input: key SL_ANDROID_KEY_RECORDING_PRESET gets value SL_ANDROID_RECORDING_PRESET_VOICE_COMMUNICATION > - Output: key SL_ANDROID_KEY_STREAM_TYPE gets value SL_ANDROID_STREAM_VOICE > It does these things so that the system's volume settings for voice > communications (in each direction) will take effect when the streams are > used in chrome. > Please rewrite the CL description so that it says what it does. > >> > What is the audible effect of commenting out l.231-238 vs. not? >> Without this patch, it's not meaningful to test webrtc because >> application cannot control volume. The bug is >> https://code.google.com/p/chromium/issues/detail?id=233736 > The way I read the bug is that clank+apprtc should be unusable without > headphones (because volume is too low). My impression was different, though > I tend to quickly mute or plug in headphones because of feedback. Is that > really what the bug says or does it only happen sometimes? > >> >> > What is the audible effect of using VOICE vs. MUSIC for webrtc? >> > What is the audible effect of using VOICE vs. MUSIC for go/ocnyf? [...] > [what] I'm asking is to confirm > that this change doesn't make playback of all media files through the web > browser broken.
If you start a new cl, please add qinmin@ and rtoy@ as the reviewers who should know the impact of the change.
Ami, I have tested this patch and it allows user to adjust volume. I think we'd have this patch landed.
On 2013/06/17 22:40:44, wjia wrote: > Ami, > > I have tested this patch and it allows user to adjust volume. I think we'd have > this patch landed. Wei, See my comment #27 in the reviewlog. If you want to take over this CL to land it you'll need to upload it to a new rietveld issue (see Leo's request in #28). Please make sure to rewrite the CL description so it says what it does and how it was tested. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
