Closed Bug 1215447 Opened 4 years ago Closed 4 years ago

Do not unregister audio channel agent of the media element during seeking

Categories

(Firefox OS Graveyard :: AudioChannel, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:2.5+, firefox44 fixed)

RESOLVED FIXED
FxOS-S10 (30Oct)
blocking-b2g 2.5+
Tracking Status
firefox44 --- fixed

People

(Reporter: alwu, Assigned: alwu)

References

Details

Attachments

(1 file)

During the seeking, in HTMLMediaElement::UpdateAudioChannelPlayingState, the |playingThroughTheAudioChannel| would be set to false, and it's incorrect.

We shouldn't unregister the audio channel agent during seeking.
In our expectation, during the seeking, the flag |mPlayingThroughTheAudioChannelBeforeSeek| should be true, and this flag can prevent us to stop the audio channel agent.

In this case, 
|mReadyState| would become "HAVE_METADATA" and the |mPlayingThroughTheAudioChannelBeforeSeek| was set too late. This flag was set after we check the audio channel state. 
It caused that the |playingThroughTheAudioChannel| was set to false.

Two solution,
(1) Set this flag in Seek(), instead of SeekStarted().
(2) Rechecking the checking-condition, whether we still need to check the |mReadyState|?
See Also: → 1198165
Hi, Baku,
We would unregister the audio channel agent during seeking, because the |mReadyState| would become "HAVE_METADATA", and |mPlayingThroughTheAudioChannelBeforeSeek| doesn't be updated at that time [1]. 

From the edit history, you added the |mReadyState| checking.
Therefore, I want to ask, can we discard the checking-condition of the |mReadyState|? 
Maybe we can just use other conditions to check the audio playing?(ex. mPause, mute, loop attribute...)

Very appreciate :)

[1] 
|mPlayingThroughTheAudioChannelBeforeSeek| would be update in SeekStarted().
However, the order of the function call is like that,
> Seek() -> .... -> UpdateAudioChannelPlayingState() -> ... -> SeekStarted().
Flags: needinfo?(amarchesini)
If you unregister the AudioChannelAgent, something else would be playing, the window will receive the notification that nothing active. I don't think this is what you want. Btw, we already use mMuted, mPaused, and the loop attribute.

The question is: why mPlayingThoughTheAudioChannelBeforeSeek is set too late?
Flags: needinfo?(amarchesini)
Hi, Baku,
As comment2 said, it's because the |mPlayingThoughTheAudioChannelBeforeSeek| is set in SeekStarted().

The SeekStarted() is always called after Seek(), and the UpdateAudioChannelPlayingState() is called from the Seek(). Therefore, the |mPlayingThoughTheAudioChannelBeforeSeek| always be set after the UpdateAudioChannelPlayingState().

In present codebase, the |mPlayingThoughTheAudioChannelBeforeSeek| is totally meaningless.

I have two solutions,
(1) Move the checking of |mPlayingThoughTheAudioChannelBeforeSeek| from SeekStarted() to Seek().
(2) Maybe we can think about whether we really need the |mReadyState| to decide the audio channel playing state.
[Blocking Requested - why for this release]: this bug blocks bug 1198165, which is a 2.5+ blocker, so I'm nominating this one, too.
Blocks: 1198165
blocking-b2g: --- → 2.5?
blocking-b2g: 2.5? → 2.5+
Priority: -- → P1
Assignee: nobody → alwu
Bug 1215447 - move flag setting from SeekStarted() to Seek().
Comment on attachment 8676233 [details]
MozReview Request: Bug 1215447 - move flag setting from SeekStarted() to Seek(). r=roc.

Hi, Baku/Robert,
Could you give me some suggestions about this patch?

During the seeking, we don't want to unregister the AudioChannelAgent, and we used the flag |mPlayingThroughTheAudioChannelBeforeSeek| to prevent this situation. 

If this flag is true, then we would not unregister the AudioChannelAgent in UpdateAudioChannelPlayingState().

However, in present codebase, this flag is set in the SeekStarted(), it's too late. The code flow is like that,
> Seek() -> .... -> UpdateAudioChannelPlayingState() -> ... -> SeekStarted()

Therefore, this flag is totally meaningless now. We need to move it before the 
UpdateAudioChannelPlayingState().


Very appreciate!!
Attachment #8676233 - Flags: feedback?(roc)
Attachment #8676233 - Flags: feedback?(amarchesini)
Comment on attachment 8676233 [details]
MozReview Request: Bug 1215447 - move flag setting from SeekStarted() to Seek(). r=roc.

https://reviewboard.mozilla.org/r/22631/#review20201

Looks good to me

::: dom/html/HTMLMediaElement.cpp:1594
(Diff revision 1)
> +  if(mPlayingThroughTheAudioChannel) {

space before (
Attachment #8676233 - Flags: review+
Attachment #8676233 - Flags: feedback?(roc)
Attachment #8676233 - Flags: feedback?(amarchesini)
Attachment #8676233 - Flags: feedback-
Attachment #8676233 - Flags: feedback-
Comment on attachment 8676233 [details]
MozReview Request: Bug 1215447 - move flag setting from SeekStarted() to Seek(). r=roc.

Bug 1215447 - move flag setting from SeekStarted() to Seek(). r=roc.
Attachment #8676233 - Attachment description: MozReview Request: Bug 1215447 - move flag setting from SeekStarted() to Seek(). → MozReview Request: Bug 1215447 - move flag setting from SeekStarted() to Seek(). r=roc.
Attachment #8676233 - Flags: review+
Attachment #8676233 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a1059127579d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S10 (30Oct)
You need to log in before you can comment on or make changes to this bug.