Closed Bug 1007552 Opened 5 years ago Closed 5 years ago

[Madai] 'publicnotification' channel should be fixed for camera shutter

Categories

(Firefox OS Graveyard :: AudioChannel, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.0 S3 (6june)

People

(Reporter: oedipus31, Assigned: scheng)

Details

(Whiteboard: [m+])

Attachments

(2 files, 5 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140314220517

Steps to reproduce:

changed camera shutter to 'publicnotification' channel

Sounds.prototype.createAudio = function(url) {
  var audio = new Audio(url);
  // MADAI ONLY. Shutter shound should be always played
  audio.mozAudioChannelType = 'publicnotification'; // notification
  return audio;
};

I checked stream type in AudioTrack.cpp


Actual results:

Currently AudioTrack`s streamType is AUDIO_STREAM_SYSTEM

01-12 19:24:45.959 V/AudioTrack(24964): sampleRate 48000, channelMask 0x1, format 1
01-12 19:24:45.959 V/AudioTrack(24964): streamType 1
01-12 19:24:45.959 V/AudioTrack(24964): set() streamType 1 frameCount 0 flags 0004



Expected results:

The shutter sound should be enforced for carrier/country spec. 
If audio chnnel is 'publicnotification' , it should call 'AUDIO_STREAM_ENFORCED_AUDIBLE' stream type. 

/* Audio stream types */
typedef enum {
    AUDIO_STREAM_DEFAULT          = -1,
    AUDIO_STREAM_VOICE_CALL       = 0,
    AUDIO_STREAM_SYSTEM           = 1,
    AUDIO_STREAM_RING             = 2,
    AUDIO_STREAM_MUSIC            = 3,
    AUDIO_STREAM_ALARM            = 4,
    AUDIO_STREAM_NOTIFICATION     = 5,
    AUDIO_STREAM_BLUETOOTH_SCO    = 6,
    AUDIO_STREAM_ENFORCED_AUDIBLE = 7, /* Sounds that cannot be muted by user and must be routed to speaker */
    AUDIO_STREAM_DTMF             = 8,
    AUDIO_STREAM_TTS              = 9,
    AUDIO_STREAM_INCALL_MUSIC     = 10,
    AUDIO_STREAM_CNT,
    AUDIO_STREAM_MAX              = AUDIO_STREAM_CNT - 1,
} audio_stream_type_t;
OS: All → Gonk (Firefox OS)
Hardware: All → ARM
blocking-b2g: --- → 1.4?
NI :wayne to get more information here if this blocking madai as this does not look like a valid bug on our code. Could be a POVB ? We were also not sure of the spec that is mentioned in the description.
blocking-b2g: 1.4? → ---
Flags: needinfo?(wchang)
In FireFox OS, the "Publicnotification" audio channel type is not supported. In the ConvertChannelToCubebType(), the case dom::AudioChannel::Publicnotification should return CUBEB_STREAM_TYPE_ENFORCED_AUDIBLE to meet the definition of audio stream type of AudioSystem.

static cubeb_stream_type ConvertChannelToCubebType(dom::AudioChannel aChannel)
{
  switch(aChannel) {
    (...)

    // Currently Android openSLES library doesn't support FORCE_AUDIBLE yet.
    case dom::AudioChannel::Publicnotification:
    default:
      NS_ERROR("The value of AudioChannel is invalid");
      return CUBEB_STREAM_TYPE_MAX;
  }
}


So the 'Normal' audio channel is used instead of 'Publicnotification' audio channel. As you saw the log in comment 0:

01-12 19:24:45.959 V/AudioTrack(24964): streamType 1
Assignee: nobody → scheng
marking with [m+] tracker first.

Have spoken to Star, who's now investigating this. Will revisit when some results are available.
Flags: needinfo?(wchang)
Whiteboard: [m+]
Attached patch bug-1007552-fix.patch (obsolete) — Splinter Review
support notification audio channel type
(In reply to Seil Park from comment #0)
> User Agent: Mozilla/5.0 (Windows NT 6.1; rv:28.0) Gecko/20100101
> Firefox/28.0 (Beta/Release)
> Build ID: 20140314220517
> 
> Steps to reproduce:
> 
> changed camera shutter to 'publicnotification' channel
> 
> Sounds.prototype.createAudio = function(url) {
>   var audio = new Audio(url);
>   // MADAI ONLY. Shutter shound should be always played
>   audio.mozAudioChannelType = 'publicnotification'; // notification
>   return audio;
> };
> 
> I checked stream type in AudioTrack.cpp
> 
> 
> Actual results:
> 
> Currently AudioTrack`s streamType is AUDIO_STREAM_SYSTEM
> 
> 01-12 19:24:45.959 V/AudioTrack(24964): sampleRate 48000, channelMask 0x1,
> format 1
> 01-12 19:24:45.959 V/AudioTrack(24964): streamType 1
> 01-12 19:24:45.959 V/AudioTrack(24964): set() streamType 1 frameCount 0
> flags 0004
> 
> 
> 
> Expected results:
> 
> The shutter sound should be enforced for carrier/country spec. 
> If audio chnnel is 'publicnotification' , it should call
> 'AUDIO_STREAM_ENFORCED_AUDIBLE' stream type. 
> 
> /* Audio stream types */
> typedef enum {
>     AUDIO_STREAM_DEFAULT          = -1,
>     AUDIO_STREAM_VOICE_CALL       = 0,
>     AUDIO_STREAM_SYSTEM           = 1,
>     AUDIO_STREAM_RING             = 2,
>     AUDIO_STREAM_MUSIC            = 3,
>     AUDIO_STREAM_ALARM            = 4,
>     AUDIO_STREAM_NOTIFICATION     = 5,
>     AUDIO_STREAM_BLUETOOTH_SCO    = 6,
>     AUDIO_STREAM_ENFORCED_AUDIBLE = 7, /* Sounds that cannot be muted by
> user and must be routed to speaker */
>     AUDIO_STREAM_DTMF             = 8,
>     AUDIO_STREAM_TTS              = 9,
>     AUDIO_STREAM_INCALL_MUSIC     = 10,
>     AUDIO_STREAM_CNT,
>     AUDIO_STREAM_MAX              = AUDIO_STREAM_CNT - 1,
> } audio_stream_type_t;



Could you try this patch (attachment 8423578 [details] [diff] [review])?  It returns enforced audiable stream type for 'Puliciation' aduio channel type.
Flags: needinfo?(oedipus31)
(In reply to Star Cheng [:scheng] from comment #4)
> Created attachment 8423578 [details] [diff] [review]
> bug-1007552-fix.patch
> 
> 
> support notification audio channel type

In the meantime, I will add publicNotification audio channel for AudioManager in order to controlling volume.
Attached patch bug-1007552-fix.patch (obsolete) — Splinter Review
1.support publicnotification audio channel
2.add enforced audiable stream type for opensl
Attachment #8423578 - Attachment is obsolete: true
(In reply to Star Cheng [:scheng] from comment #5)
> (In reply to Seil Park from comment #0)
> > User Agent: Mozilla/5.0 (Windows NT 6.1; rv:28.0) Gecko/20100101
> > Firefox/28.0 (Beta/Release)
> > Build ID: 20140314220517
> >     AUDIO_STREAM_BLUETOOTH_SCO    = 6,
> >     AUDIO_STREAM_ENFORCED_AUDIBLE = 7, /* Sounds that cannot be muted by
> > user and must be routed to speaker */
> >     AUDIO_STREAM_DTMF             = 8,
> >     AUDIO_STREAM_TTS              = 9,
> >     AUDIO_STREAM_INCALL_MUSIC     = 10,
> >     AUDIO_STREAM_CNT,
> >     AUDIO_STREAM_MAX              = AUDIO_STREAM_CNT - 1,
> > } audio_stream_type_t;
> 
> 
> 
> Could you try this patch (attachment 8423578 [details] [diff] [review])?  It
> returns enforced audiable stream type for 'Puliciation' aduio channel type.

I try to enforced audiable stream type for opensl, and I am not sure it is able to work. Please try this patch (attachment 8423693 [details] [diff] [review]).
(In reply to Star Cheng [:scheng] from comment #8)
> (In reply to Star Cheng [:scheng] from comment #5)
> > (In reply to Seil Park from comment #0)
> > > User Agent: Mozilla/5.0 (Windows NT 6.1; rv:28.0) Gecko/20100101
> > > Firefox/28.0 (Beta/Release)
> > > Build ID: 20140314220517
> > >     AUDIO_STREAM_BLUETOOTH_SCO    = 6,
> > >     AUDIO_STREAM_ENFORCED_AUDIBLE = 7, /* Sounds that cannot be muted by
> > > user and must be routed to speaker */
> > >     AUDIO_STREAM_DTMF             = 8,
> > >     AUDIO_STREAM_TTS              = 9,
> > >     AUDIO_STREAM_INCALL_MUSIC     = 10,
> > >     AUDIO_STREAM_CNT,
> > >     AUDIO_STREAM_MAX              = AUDIO_STREAM_CNT - 1,
> > > } audio_stream_type_t;
> > 
> > 
> > 
> > Could you try this patch (attachment 8423578 [details] [diff] [review])?  It
> > returns enforced audiable stream type for 'Puliciation' aduio channel type.
> 
> I try to enforced audiable stream type for opensl, and I am not sure it is
> able to work. Please try this patch (attachment 8423693 [details] [diff] [review]
> [review]).


The SL_ANDROID_STREAM_ENFORCED_AUDIBLE is not supported in opensl ndk lib. 
http://androidxref.com/4.1.1/xref/frameworks/wilhelm/src/android/AudioPlayer_to_android.cpp#464
Flags: needinfo?(oedipus31)
Attachment #8423693 - Attachment is obsolete: true
(In reply to Star Cheng [:scheng] from comment #9)
> (In reply to Star Cheng [:scheng] from comment #8)
> > (In reply to Star Cheng [:scheng] from comment #5)
> > > (In reply to Seil Park from comment #0)
> > > > User Agent: Mozilla/5.0 (Windows NT 6.1; rv:28.0) Gecko/20100101
> > > > Firefox/28.0 (Beta/Release)
> > > > Build ID: 20140314220517
> > > >     AUDIO_STREAM_BLUETOOTH_SCO    = 6,
> > > >     AUDIO_STREAM_ENFORCED_AUDIBLE = 7, /* Sounds that cannot be muted by
> > > > user and must be routed to speaker */
> > > >     AUDIO_STREAM_DTMF             = 8,
> > > >     AUDIO_STREAM_TTS              = 9,
> > > >     AUDIO_STREAM_INCALL_MUSIC     = 10,
> > > >     AUDIO_STREAM_CNT,
> > > >     AUDIO_STREAM_MAX              = AUDIO_STREAM_CNT - 1,
> > > > } audio_stream_type_t;
> > > 
> > > 
> > > 
> > > Could you try this patch (attachment 8423578 [details] [diff] [review])?  It
> > > returns enforced audiable stream type for 'Puliciation' aduio channel type.
> > 
> > I try to enforced audiable stream type for opensl, and I am not sure it is
> > able to work. Please try this patch (attachment 8423693 [details] [diff] [review]
> > [review]).
> 
> 
> The SL_ANDROID_STREAM_ENFORCED_AUDIBLE is not supported in opensl ndk lib. 
> http://androidxref.com/4.1.1/xref/frameworks/wilhelm/src/android/
> AudioPlayer_to_android.cpp#464

Hi, Michael

Can we add a new stream type for opensl DNK lib?
Flags: needinfo?(mwu)
> > The SL_ANDROID_STREAM_ENFORCED_AUDIBLE is not supported in opensl ndk lib. 
> > http://androidxref.com/4.1.1/xref/frameworks/wilhelm/src/android/
> > AudioPlayer_to_android.cpp#464
> 
> Hi, Michael
> 
> Can we add a new stream type for opensl DNK lib?

In the audioPlayer_setStreamType() @ frameworks\wilhelm\src\android\ path. Add a new stream type :

    case SL_ANDROID_STREAM_ENFORCED_AUDIBLE:
        newStreamType = AUDIO_STREAM_ENFORCED_AUDIBLE;
        break;

And set the CUBEB_STREAM_TYPE_ENFORCED_AUDIBLE as stream type for testing in AudioStream.cpp

  // fake audiable  stream type
  params.stream_type = CUBEB_STREAM_TYPE_ENFORCED_AUDIBLE;

See the log: we got the stream type "7".

V/AudioPolicyManagerBase(  175): getOutput() stream 7, samplingRate 44100, format 1, channelMask 3, flags 4
V/AudioPolicyManagerBase(  175): getOutput() returns output 2
Flags: needinfo?(mwu)
Attachment #8423693 - Attachment is obsolete: false
add a enforeced audiable stream type for OPENSL ndk lib.
Hi, 

Could you try these 2 patched?  (attachment 8425369 [details] [diff] [review] + Attachment 8423693 [details] [diff])
Flags: needinfo?(oedipus31)
(In reply to Star Cheng [:scheng] from comment #12)
> Created attachment 8425369 [details] [diff] [review]
> 0001-bug-1007552-add-ENFORCED_AUDIBLE-stream-type-for-cam.patch
> 
> 
> add a enforeced audiable stream type for OPENSL ndk lib.

Hi, Michael

We have to add "ENFORCED_AUDIABLE" stream type for opensl ndk lib in order to map "PublicNotification" audio channel type in Gecko. And this modification occurs in AOSP code base. 

Could you tell me how to create a git repository of platform/frameworks/wilhelm (..\frameworks\wilhelm\) in mozilla github b2g?
Flags: needinfo?(mwu)
I can create a fork, but patching system libraries requires coordination with our SoC vendor friends. Otherwise the change will only affect our AOSP based devices (emulator and nexus devices). You should also attempt to push the patch upstream.
Flags: needinfo?(mwu)
https://github.com/mozilla-b2g/platform_frameworks_wilhelm created.

What android base versions do you want to support this on?
(In reply to Michael Wu [:mwu] from comment #15)
> I can create a fork, but patching system libraries requires coordination
> with our SoC vendor friends. Otherwise the change will only affect our AOSP
> based devices (emulator and nexus devices). You should also attempt to push
> the patch upstream.

Thank you help to create the fork and remind these things.
(In reply to Michael Wu [:mwu] from comment #16)
> https://github.com/mozilla-b2g/platform_frameworks_wilhelm created.
> 
> What android base versions do you want to support this on?


This feature (provide publicnotification audio channel for camera shutter) is for Madai, and it's gonk is Android KK. So I want to support it on android-4.4.2_r1. Do you have any suggestion?
Flags: needinfo?(mwu)
Attachment #8423693 - Attachment is obsolete: true
Attachment #8425369 - Attachment is obsolete: true
I will provide patch for this bug, and remove the WIP patch and ni? tag.
Flags: needinfo?(oedipus31)
Attached patch bug-1007552-fix.patch (obsolete) — Splinter Review
Add fourced audiable stream type in gecko.
Attachment #8426842 - Flags: review?(mwu)
I`m sorry for the delay in my response. I`m following up another project. 
I checked these patches and It is fine. 
https://bug1007552.bugzilla.mozilla.org/attachment.cgi?id=8425369
https://bug1007552.bugzilla.mozilla.org/attachment.cgi?id=8426842
After releasing these patchese, I will review shutter sound scenario with gaia member.
thanks.
Attachment #8426842 - Flags: review?(mwu) → review?(kinetik)
Flags: needinfo?(mwu)
Comment on attachment 8426842 [details] [diff] [review]
bug-1007552-fix.patch

Happy to take the patch with the issues I mentioned addressed.
Attachment #8426842 - Flags: review?(kinetik) → review-
(In reply to Matthew Gregan [:kinetik] from comment #22)
> Comment on attachment 8426842 [details] [diff] [review]
> bug-1007552-fix.patch
> 
> Happy to take the patch with the issues I mentioned addressed.


Hi,  Matthew


Thank your review. And do you mind to tell me what is your concern and what I have to modify. I will try to fix it.
Flags: needinfo?(kinetik)
Sorry, I wrote comments in the review UI and they seem to have disappeared. :-(

From memory:
- rename ENFORCED_AUDIBLE to SYSTEM_ENFORCED as this is the name used in AudioManager.java.
- remove the comment line about KitKat

Thanks!
Flags: needinfo?(kinetik)
Attachment #8426842 - Attachment is obsolete: true
Attached patch bug-1007552-fix.patch (obsolete) — Splinter Review
1.rename ENFORCED_AUDIBLE to SYSTEM_ENFORCED in sles_definitions.h as this is the name used in AudioManager.java.
2.remove the comment line about KitKat in cubeb_opensl.c
Attachment #8430497 - Flags: review?(kinetik)
(In reply to Star Cheng [:scheng] from comment #18)
> (In reply to Michael Wu [:mwu] from comment #16)
> > https://github.com/mozilla-b2g/platform_frameworks_wilhelm created.
> > 
> > What android base versions do you want to support this on?
> 
> 
> This feature (provide publicnotification audio channel for camera shutter)
> is for Madai, and it's gonk is Android KK. So I want to support it on
> android-4.4.2_r1. Do you have any suggestion?


Hi, Michael

I have pushed these modification in repository of my gitHub [1].  But I can't pull request to Tag (android-4.4.2_r1) of Mozilla git hub. Could you help to create a branch of android-4.4.2_r1 for upstream the modification of opensl lib? 


[1] https://github.com/littlestar0425/platform_frameworks_wilhelm/commit/e1c695dd3184dd87ddbdb4a72fb6996aab2d820c
Flags: needinfo?(mwu)
Comment on attachment 8430497 [details] [diff] [review]
bug-1007552-fix.patch

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

r+ with comments addressed

::: content/media/AudioStream.cpp
@@ +136,2 @@
>      case dom::AudioChannel::Publicnotification:
> +      return CUBEB_STREAM_TYPE_ENFORCED_AUDIBLE;

Can you please rename the cubeb constant too?  The names were originally taken from Android, so I'd like to keep them in sync.
Attachment #8430497 - Flags: review?(kinetik) → review+
(In reply to Star Cheng [:scheng] from comment #26)
> Could you
> help to create a branch of android-4.4.2_r1 for upstream the modification of
> opensl lib? 
>

Done. b2g-4.4.2_r1 generated.
Flags: needinfo?(mwu)
1.To add SYSTEM_ENFORCED type for OpenSL ndk lib in wilhelm. 
2.This is only for whilhelm in 4.4..2_r1 branch of AOSP.
Attachment #8432969 - Flags: review?(kinetik)
Oh, so it *is* called ENFORCED_AUDIBLE some places inside the Android code:

+        newStreamType = AUDIO_STREAM_ENFORCED_AUDIBLE;

Confusing. :-/

This looks fine to be, but not sure if I can r+ mozilla-b2g stuff.
(In reply to Matthew Gregan [:kinetik] from comment #30)
> Oh, so it *is* called ENFORCED_AUDIBLE some places inside the Android code:
> 
> +        newStreamType = AUDIO_STREAM_ENFORCED_AUDIBLE;
> 
> Confusing. :-/
> 
> This looks fine to be, but not sure if I can r+ mozilla-b2g stuff.

Oh yeh! AudioManager use STREAM_SYSTEM_ENFORCED, and AUDIO_STREAM_ENFORCED_AUDIBLE is used in Audio.h instead :(


[1] http://androidxref.com/4.4.2_r2/xref/frameworks/base/media/java/android/media/AudioManager.java#216
[2] http://androidxref.com/4.4.2_r2/xref/system/core/include/system/audio.h#49
Attachment #8432969 - Flags: review?(kinetik) → review+
1.TPBL result : https://tbpl.mozilla.org/?tree=Try&rev=17d1a96652a2
2.add reviewer
Attachment #8430497 - Attachment is obsolete: true

There are 2 patches for this issue. One is for Gecko, the other is for Mozilla B2G GitHub

1. attachment 8433874 [details] [diff] [review] : This patch is for Gecko
2. Attachment 8432969 [details] : This patch is for B2G GitHub
Keywords: checkin-needed
(In reply to Star Cheng [:scheng] from comment #32)
> Created attachment 8433874 [details] [diff] [review]
> bug-1007552-fix-v2.patch
> 
> 
> 1.TPBL result : https://tbpl.mozilla.org/?tree=Try&rev=17d1a96652a2
> 2.add reviewer

3. rename ENFORCED_AUDIBLE to SYSTEM_ENFORCED for cubeb constant
https://hg.mozilla.org/integration/b2g-inbound/rev/12f6d73e8fb1

b2g-4.4.2_r1: https://github.com/mozilla-b2g/platform_frameworks_wilhelm/commit/98289897bc4b0035d6ec799f03e838ac3c9052cd

In the future, please try to be more conscientious about what platforms and test suites you use on your Try pushes. "All" runs consume over 300hr of machine time and contribute to long backlogs for other developers. Thanks!
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/12f6d73e8fb1
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S3 (6june)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #35)
> https://hg.mozilla.org/integration/b2g-inbound/rev/12f6d73e8fb1
> 
> b2g-4.4.2_r1:
> https://github.com/mozilla-b2g/platform_frameworks_wilhelm/commit/
> 98289897bc4b0035d6ec799f03e838ac3c9052cd
> 
> In the future, please try to be more conscientious about what platforms and
> test suites you use on your Try pushes. "All" runs consume over 300hr of
> machine time and contribute to long backlogs for other developers. Thanks!
> https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Hi, Ryan

Thank you remind me about that. I will considerate of our try server resource.
This bug isn't actually fixed until patches reach SoC vendor trees (CAF and spreadtrum). Upstreaming to AOSP will help with that. Please let me know the progress on this.
Flags: needinfo?(scheng)
Flags: needinfo?(scheng)
You need to log in before you can comment on or make changes to this bug.