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

RESOLVED FIXED in Firefox 44

Status

P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: alwu, Assigned: alwu)

Tracking

unspecified
FxOS-S10 (30Oct)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:2.5+, firefox44 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
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|?
(Assignee)

Updated

3 years ago
See Also: → bug 1198165
(Assignee)

Comment 2

3 years ago
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)
(Assignee)

Comment 4

3 years ago
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?

Updated

3 years ago
blocking-b2g: 2.5? → 2.5+
Priority: -- → P1
(Assignee)

Updated

3 years ago
Assignee: nobody → alwu
(Assignee)

Comment 6

3 years ago
Created 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().
(Assignee)

Comment 7

3 years ago
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+
(Assignee)

Updated

3 years ago
Attachment #8676233 - Flags: feedback?(roc)
Attachment #8676233 - Flags: feedback?(amarchesini)
Attachment #8676233 - Flags: feedback-
(Assignee)

Updated

3 years ago
Attachment #8676233 - Flags: feedback-
(Assignee)

Comment 10

3 years ago
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+
(Assignee)

Updated

3 years ago
Attachment #8676233 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a1059127579d
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S10 (30Oct)
You need to log in before you can comment on or make changes to this bug.