audio API: mozSetup/mozWriteAPI should join audio channel

RESOLVED FIXED

Status

defect
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: alive, Assigned: mchen)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(blocking-b2g:leo+, firefox21 wontfix, firefox22 wontfix, firefox23 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

Details

(Whiteboard: [fixed-in-birch], )

Attachments

(2 attachments, 4 obsolete attachments)

See https://bugzilla.mozilla.org/show_bug.cgi?id=846092#c10

Dialer is using mozSetup/mozWriteAudio API to generate DTMF tone.
However in this case it doesn't join any audio channel even when we force assign the mozAudioChannelType to audio element.


+++ This bug was initially created as a clone of Bug #846092 +++

1. Title : In dial pad screen, cannot change keypad volume.
2. Precondition : Settings >> Sound >> Keypad - check >> Home >> Run Dialer >> Dial pad screen
3. Tester's Action : Press side volume up/down key
4. Detailed Symptom (ENG.) : Not change keypad volume.
6. Expected : Can change keypad volume.
7. Reproducibility: Y
1) Frequency Rate : 100%
8. Gaia revision : 6916e18d1f72936e892543cf4a104a7d457f78ad
Tracking gecko codes and it looks like it's already manipulated in
http://hg.mozilla.org/mozilla-central/file/9db46ddfb517/content/html/content/src/HTMLAudioElement.cpp#l126

> aRv = mAudioStream->Init(aChannels, aRate, mAudioChannelType);

When the audio element is doing |mozSetup|. So there may be something wrong before audio-channel-changed event fires.
Taken first after discussing with :mchen.

Ignore comment 1 because audioStream isn't audioChannelAgent. It's the way down to gonk but won't pass by audioChannelAgent nor audioElement.

What we could do is: call audioChannelAgent.startPlaying when |mozWriteAudio| and make an interval to check the audio playing is ended to call audioChannelAgent.stopPlaying because mozWriteAudio doesn't support ended event now.
Assignee: nobody → alive
Blocks a blocker.
blocking-b2g: leo? → leo+
Posted patch Patch v1 (obsolete) — Splinter Review
Hi Matthew,

Could you help to review this patch? Thanks.
This would be needed on b2g18 branch although audio data api is depreciated on m-c.

The purpose of this patch is that HTMLAudioElement doesn't use mDeocder so it's parent class HTMLMediaElement doesn't trigger the AudioChannelAgent to join AudioChannelService.

This patch inherited some virtual functions from parent then re-implement them for audio data API. And according to that there is no pause/stop point for audio data API, I create a timer for monitoring the flow of data and timeout is set to 1 second because it is the max buffer in audio backend.
Attachment #736706 - Flags: review?(kinetik)
Assignee: alive → mchen
Comment on attachment 736706 [details] [diff] [review]
Patch v1

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

::: content/html/content/public/HTMLAudioElement.h
@@ +78,5 @@
> +  // Due to that audio data API doesn't indicate the timing of pause or end,
> +  // the timer is used to defer the timing of pause/stop after writing data.
> +  nsCOMPtr<nsITimer> mDeferStopPlayTimer;
> +  // To indicate mDeferStopPlayTimer is on fire or not.
> +  bool mTimerActivated;

I wonder if we can do away with this bool and just use a non-null check of mDeferStopPlayTimer instead?  That does require freeing the timer when it's not in use, but that seems preferable to keeping an inactive timer around.

::: content/html/content/src/HTMLAudioElement.cpp
@@ +46,5 @@
>  
>  
>  HTMLAudioElement::HTMLAudioElement(already_AddRefed<nsINodeInfo> aNodeInfo)
> +  : mTimerActivated(false),
> +    HTMLMediaElement(aNodeInfo),

Keep the HTMLMediaElement initialization first.

@@ +47,5 @@
>  
>  HTMLAudioElement::HTMLAudioElement(already_AddRefed<nsINodeInfo> aNodeInfo)
> +  : mTimerActivated(false),
> +    HTMLMediaElement(aNodeInfo),
> +    mDeferStopPlayTimer(nullptr)

nsCOMPtrs are initialized to null by default.
Attachment #736706 - Flags: review?(kinetik) → review+
Depends on: 862409
1. Keep the HTMLMediaElement initialization first.
2. nsCOMPtrs are initialized to null by default so remove initialization from constructor.

Add reviewer and approval by leo.
Attachment #736706 - Attachment is obsolete: true
Attachment #741188 - Flags: review+
Version for b2g18 branch.
Attachment #741189 - Flags: review+
Keywords: checkin-needed
Do some additional test then ask for landing.
Keywords: checkin-needed
Posted patch Patch v2 (obsolete) — Splinter Review
Hi Matthew,

I missed that the overriding of UpdateAudioChannelPlayingState()/CanPlayChanged() will cause audio tag to use the version in HTMLAudioElement but HTMLMediaElement, even it is used to play from a file.

So in this patch I use mAudioStream is null or not to check whether UpdateAudioChannelPlayingState()/CanPlayChanged() should be called from HTMLAudioElement or HTMLMediaElement.

Thanks for your review again.
Attachment #741188 - Attachment is obsolete: true
Attachment #741189 - Attachment is obsolete: true
Attachment #741235 - Flags: review?(kinetik)
Comment on attachment 741235 [details] [diff] [review]
Patch v2

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

Thanks!

::: content/html/content/src/HTMLAudioElement.cpp
@@ +248,5 @@
> +  // Only Audio_Data API will initialize the mAudioStream, so we call the parent
> +  // one when this audio tag is not used by Audio_Data API.
> +  if (!mAudioStream) {
> +    HTMLMediaElement::CanPlayChanged(canPlay);
> +    return NS_OK;

Make these two lines:

  return HTMLMediaElement::CanPlayChanged(canPlay);
Attachment #741235 - Flags: review?(kinetik) → review+
1. https://tbpl.mozilla.org/?tree=Try&rev=b68ee23af8f6
2. Add reviewer and approval for Leo.
3. Patch for m-c
Attachment #741235 - Attachment is obsolete: true
Attachment #742946 - Flags: review+
Patch for b2g18.
Attachment #742947 - Flags: review+
Keywords: checkin-needed
Whiteboard: [fixed-in-birch]
https://hg.mozilla.org/mozilla-central/rev/06f4af96580e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Flags: in-moztrap?
Flags: in-moztrap? → in-moztrap+
You need to log in before you can comment on or make changes to this bug.