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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Marco Chen [:mchen]]

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

Comment 1

[Marco Chen [:mchen]]

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.

Comment 2

[Marco Chen [:mchen]]

Attachment: 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.

Comment 3

[Marco Chen [:mchen]]

Attachment: patch v2

1. Add define of MOZ_B2G to protect Andorid configuration.
2. Transfer to original reviewer of this file.

Comment 4

[Matthew Gregan [:kinetik]]

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.

Comment 5

[Marco Chen [:mchen]]

Attachment: 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.

Comment 6

[Matthew Gregan [: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.

Comment 7

[Marco Chen [:mchen]]

Attachment: 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.

Comment 8

[Matthew Gregan [: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.

Comment 9

[Marco Chen [:mchen]]

Attachment: 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.

Comment 10

[Matthew Gregan [:kinetik]]

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.

Comment 11

[Marco Chen [:mchen]]

Yes, I remember that. Thanks for the reminding.

Comment 12

[Marco Chen [:mchen]]

Attachment: 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.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c69ac844a778

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c69ac844a778