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

Status: RESOLVED FIXED

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

Description

[Alastor Wu [:alwu]]

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.

Comment 1

[Alastor Wu [:alwu]]

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|?

Comment 2

[Alastor Wu [:alwu]]

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().

Comment 3

[Andrea Marchesini [:baku]]

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?

Comment 4

[Alastor Wu [:alwu]]

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.

Comment 5

[David Flanagan [:djf]]

[Blocking Requested - why for this release]: this bug blocks bug 1198165, which is a 2.5+ blocker, so I'm nominating this one, too.

Comment 6

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1215447 - move flag setting from SeekStarted() to Seek(). r=roc.

Bug 1215447 - move flag setting from SeekStarted() to Seek().

Comment 7

[Alastor Wu [:alwu]]

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!!

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 (

Comment 9

[Alastor Wu [:alwu]]

Try-server result.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=903c1e790b3c

Comment 10

[Alastor Wu [:alwu]]

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.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/a1059127579d

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a1059127579d