Closed Bug 813426 Opened 7 years ago Closed 7 years ago

Fail to assign mozAudioChannelType twice.

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed

People

(Reporter: alive, Assigned: mchen)

References

Details

Attachments

(1 file, 1 obsolete file)

STR:

1.Open music app
2.Play first song
3.Play second song

Expected:
* First song stops and second song plays.

Actual:
* First song continues to play.

Note:
It's because music app reuses the audio element, but only changes the src of music.
Gecko will throw exception if the same element is set mozAudioChannelType already
Blocks: 795237
Component: General → DOM: Device Interfaces
Product: Boot2Gecko → Core
C.C. Music/Video app owner.

Although we can have a workaround in our music/video apps(Create a new audio/video element EVERYTIME the src changes), but we cannot prevent other developer to reuse the same DOM element to play different audio/video files in their app/webpage. This should be fixed.
Per bug 795237 comment 44, SetMozAudioChannelType (see http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLMediaElement.cpp#3748) returns an error if the decoder is already set up, since we don't support changing the mozAudioChannelType after the decoder is set up.

If a media element is reused (by setting @src to a new URI), there's no way to set a new mozAudioChannelType for the new URI because there's no observable time during the @src change when mDecoder would be null which would provide a valid time for script to set the new value.

We decided in bug 795237 not to support dynamic mozAudioChannelType changes because it's complicated.  We'd need to tear down and recreate the underlying AudioStream with a new type.  

If we still don't want to handle this, I think we can improve the current situation by having SetMozAudioChannelType only throw when an AudioStream is presently initialized for a decoder.  In effect, that'd mean it's possible to change mozAudioChannelType on an element any time the element is not actively playing (including "playing but paused", since we keep the audio stream alive during pause so we can resume playback seamlessly).
Request for blocking
blocking-basecamp: --- → ?
(In reply to Matthew Gregan [:kinetik] from comment #2)
> Per bug 795237 comment 44, SetMozAudioChannelType (see
> http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/
> nsHTMLMediaElement.cpp#3748) returns an error if the decoder is already set
> up, since we don't support changing the mozAudioChannelType after the
> decoder is set up.
> 

I suggested to return NS_OK when
  -> mDecoder is not null. 
    -> The type from SetMozAudioChannelType is the same with current one.

If we allow to re-assign the audioChanneltype during paused of decoder's state,
then we need to re-create AudioTrack once new type is different then old one.

Patch will be attached if we all agree this.
Assignee: nobody → mchen
Flags: needinfo?(kinetik)
That's a simple solution.  Does it cover the all the use cases we care about right now?  If it does, I'm fine with that.

Otherwise, how about changing MediaDecoder::SetAudioChannelType to return a boolean, calling a new function MediaDecoderStateMachine::SetAudioChannelType, and have that return an error if mAudioStream is already set up?

Then nsHTMLMediaElement::SetMozAudioChannelType can optimistically change the type by calling into the decoder, and it will return an error only if it's not possible (because the AudioTrack is already initialized, for example).
Flags: needinfo?(kinetik)
Attached patch WIPv1 (obsolete) — Splinter Review
(In reply to Matthew Gregan [:kinetik] from comment #5)
> Then nsHTMLMediaElement::SetMozAudioChannelType can optimistically change
> the type by calling into the decoder, and it will return an error only if
> it's not possible (because the AudioTrack is already initialized, for
> example).
The call sequence from music.js for every song using the same audio tag is
  this.audio.mozAudioChannelType = 'content';
  this.audio.src = url;

So the way you provided seems to not solve the issue here. Refer to your solution, SetMozAudioChannelType() will still return an error because MediaDecoderStateMachine found there is a mAudioStream set up already in case of playing second or later songs.

The onlly benifit from this way is listed as below.
When audio tag didn't be set 'autoplay', mozAudioChannelType can be assigned before the timing of audio.play(). (but this benefit didn't solve this issue.)

So I suggest to apply my patch. Which can solve this bug but still keep the constraint of mozAudioChannelType can't be set while AudioTrack already be created.
Attachment #683568 - Flags: review?(kinetik)
blocking-basecamp: ? → +
Attachment #683568 - Flags: review?(kinetik) → review+
Duplicate of this bug: 813407
Attached patch Check-in-PatchSplinter Review
Test it on m-c and aurora.
Attachment #683568 - Attachment is obsolete: true
Attachment #684410 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2867ca636a70
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.