[Audio] Add support of SLAndroidConfigurationItf into backend of Cubeb + OpenSLES

RESOLVED FIXED in B2G C4 (2jan on)

Status

Firefox OS
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mchen, Assigned: mchen)

Tracking

unspecified
B2G C4 (2jan on)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

5 years ago
Currently we already setup the stream type from media element -> audiostream -> sydney -> audiotrack. Now we need to support it on cubed -> openSLES -> audiotrack.
(Assignee)

Comment 1

5 years ago
After a rough implementation (function working), I found that openSLES from Android doesn't set SL_ANDROID_STREAM_XXX for AudioSystem::ENFORCED_AUDIBLE. But we have an audio channel called publicnotification which will be mapped to ENFORCED_AUDIBLE (even there is no one use it and originally used by camera shutter sound.)

So currently we can ignore this mapping then try to add it into upstream.
(Assignee)

Comment 2

5 years ago
Created attachment 715380 [details] [diff] [review]
Patch v1

This patch tried to apply AudioChannelType into Gecko into stream type in AudioFlinger and through openSLES interface.

Currently openSLES library doesn't support stream type of ENFORCED_AUDIABLE (map to Gecko is publicnotification). And B2G now doesn't use it too.
Attachment #715380 - Flags: review?(mwu)
(Assignee)

Comment 3

5 years ago
Created attachment 715887 [details] [diff] [review]
patch v2

1. Add define of MOZ_B2G to protect Andorid configuration.
2. Transfer to original reviewer of this file.
Attachment #715380 - Attachment is obsolete: true
Attachment #715380 - Flags: review?(mwu)
Attachment #715887 - Flags: review?(kinetik)
Summary: [Audio] Add support of SLAndroidConfigurationItf into backend of Cubed + OpenSLES → [Audio] Add support of SLAndroidConfigurationItf into backend of Cubeb + OpenSLES
Comment on attachment 715887 [details] [diff] [review]
patch v2

Review of attachment 715887 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks good.  I need to think about the naming of the stream type enums a little more, but everything else is great.

::: content/media/AudioStream.cpp
@@ +177,5 @@
>        return SA_STREAM_TYPE_MAX;
>    }
>  }
>  
> +static cube_stream_type ConvertChannelToCUBEDType(dom::AudioChannelType aType)

Cubeb.

(It's named after a type of pepper).

@@ +192,5 @@
> +    case dom::AUDIO_CHANNEL_TELEPHONY:
> +      return CUBEB_STREAM_TYPE_VOICE_CALL;
> +    case dom::AUDIO_CHANNEL_RINGER:
> +      return CUBEB_STREAM_TYPE_RING;
> +    // Currently Android openSLES library doesn't support FORCE_AUDIABLE yet.

Typo in AUDIBLE.

::: media/libcubeb/include/cubeb.h
@@ +114,5 @@
> +    CUBEB_STREAM_TYPE_TTS = 9,
> +    CUBEB_STREAM_TYPE_FM = 10,
> +
> +    CUBEB_STREAM_TYPE_MAX
> +} cube_stream_type;

I think I want to use different names for these, but I'll get back to you on that later.

@@ +123,5 @@
>                                     #cubeb_sample_format. */
>    unsigned int rate;          /**< Requested sample rate.  Valid range is [1, 192000]. */
>    unsigned int channels;      /**< Requested channel count.  Valid range is [1, 32]. */
> +
> +  cube_stream_type streamType;

stream_type.

::: media/libcubeb/src/cubeb_opensl.c
@@ +77,5 @@
>      }
>    }
>  }
>  
> +static SLuint32 ConvertStreamTypeToSLStream(cube_stream_type streamType)

New line after SLUint32.  And cubeb's naming convention is all lowercase with underscores separating words.

@@ +273,2 @@
>    SLresult res = (*ctx->eng)->CreateAudioPlayer(ctx->eng, &stm->playerObj,
> +                                                &source, &sink, 2, ids, req);

I think we want to introduce an NELEMS define and replace "2" with NELEMS(ids).

(And maybe add an assert that NELEMS(ids) == NELEMS(req), but that might be too paranoid).

@@ +276,5 @@
>      cubeb_stream_destroy(stm);
>      return CUBEB_ERROR;
>    }
>  
> +  #ifdef MOZ_B2G

cubeb's a standalone library, so we can't use MOZ_ defines.  I think it's fine to compile this unconditionally, anyway.

@@ +277,5 @@
>      return CUBEB_ERROR;
>    }
>  
> +  #ifdef MOZ_B2G
> +  SLint32 streamType = ConvertStreamTypeToSLStream(stream_params.streamType);

SLuint32 and stream_type.

@@ +281,5 @@
> +  SLint32 streamType = ConvertStreamTypeToSLStream(stream_params.streamType);
> +  if (streamType != 0xFFFFFFFF) {
> +    SLAndroidConfigurationItf playerConfig;
> +    res = (*stm->playerObj)->GetInterface(stm->playerObj,
> +      ctx->SL_IID_ANDROIDCONFIGURATION, &playerConfig);

Align the "ctx..." line with the opening brace of the line above.

@@ +283,5 @@
> +    SLAndroidConfigurationItf playerConfig;
> +    res = (*stm->playerObj)->GetInterface(stm->playerObj,
> +      ctx->SL_IID_ANDROIDCONFIGURATION, &playerConfig);
> +    res = (*playerConfig)->SetConfiguration(playerConfig,
> +      SL_ANDROID_KEY_STREAM_TYPE, &streamType, sizeof(SLint32));

And same here.
Attachment #715887 - Flags: review?(kinetik) → review+
(Assignee)

Comment 5

5 years ago
Created attachment 717020 [details] [diff] [review]
patch v3

To address the comment from reviewer.

And don't we really use any define to protect Android specific codes?
Ex: OpenSLES_Android.h

I am not sure that this header will be on other OS.
Thanks.
Attachment #715887 - Attachment is obsolete: true
Attachment #717020 - Flags: review+
(Assignee)

Updated

5 years ago
Flags: needinfo?(kinetik)
(In reply to Marco Chen [:mchen] from comment #5)
> And don't we really use any define to protect Android specific codes?
> Ex: OpenSLES_Android.h

Is there a non-Gecko define we can use? cubeb needs to be able to be built as a standalone library outside of Gecko.  Maybe #ifdef __ANDROID__?  Otherwise, I'm happy to leave it as-is for now and deal with it later if we ever need to build it on a non-Android platform.


+static cube_stream_type ConvertChannelToCubedType(dom::AudioChannelType aType)

Latest patch still has a typo here: should be Cubeb.


+  cube_stream_type stream_type;

Can you add a comment for this like the other struct members have? Use #cubeb_stream_type to link the docs to the enum.
Flags: needinfo?(kinetik)
(Assignee)

Comment 7

5 years ago
Created attachment 720585 [details] [diff] [review]
Patch v4

1. fix the name to cubeb.
2. Add comment to cubeb_stream in cubeb_stream_params
3. Add __ANDROID__ to protect Android specific code.
Attachment #717020 - Attachment is obsolete: true
Attachment #720585 - Flags: review?(kinetik)
Comment on attachment 720585 [details] [diff] [review]
Patch v4

Review of attachment 720585 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

I think we might as well land this as-is and revisit the stream_type stuff later.  I can't find good documentation to work out exactly what the intentions of the various Android stream types are, and I don't have time to dig through the AudioFlinger source and work it out right now.

::: media/libcubeb/src/Makefile.in
@@ +28,5 @@
>                cubeb_opensl.c \
>                $(NULL)
>  endif
>  # No Android implementation of libcubeb yet.
> +CFLAGS += -D__ANDROID__

Isn't this defined by default when building on Android/B2G? That was the reason I chose it... cpp -dM /dev/null with an Android GCC reports it's already set, at least.

::: media/libcubeb/src/cubeb_opensl.c
@@ +85,5 @@
>  }
>  
> +#ifdef __ANDROID__
> +static SLuint32
> +convert_stream_type_to_slStream(cubeb_stream_type stream_type)

This code doesn't use CamelCase, so rename to convert_stream_type_to_sl_stream.
Attachment #720585 - Flags: review?(kinetik) → review+
(Assignee)

Comment 9

5 years ago
Created attachment 722112 [details] [diff] [review]
Patch v5

1. Remove the define of __ANDROID__ from make file.
2. Fix the naming of convert_stream_type_to_sl_stream.

Wait for result of try.
Attachment #720585 - Attachment is obsolete: true
Attachment #722112 - Flags: review+
You probably remembered, but just in case: please send a github pull request and use update.sh to update the in-tree copy of cubeb from git.
(Assignee)

Comment 11

5 years ago
Yes, I remember that. Thanks for the reminding.
Blocks: 848581
(Assignee)

Comment 12

5 years ago
Created attachment 723820 [details] [diff] [review]
Patch Checkin-Version

Result of try.
https://hg.mozilla.org/try/rev/f9e9c652f5c7

1. Add define of __ANDROID__ in AudioStream.cpp to protect android specific thing on cubeb plus opensl backend.
Attachment #722112 - Attachment is obsolete: true
Attachment #723820 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/c69ac844a778
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c69ac844a778
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → B2G C4 (2jan on)
You need to log in before you can comment on or make changes to this bug.