1048171 - [System::StatusBar] The music play indicator in status bar is blinking when we seek song from music application

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Sharaf]

STR
 * Open music application
 * Start playing a song
 * fast seek the song by long pressing the seek buttons

Its observed that the music play icon in status bar is flashing when we do seeking

Comment 1

[Sharaf]

After analyzing this issue happens because we are getting audio channel change events during media seek.
We are getting channel change events from "none" to "content" continuously. System app will hide the play icon in status bar whenever channel is not "content"

ni? Andrea Marchesini for comments

Comment 2

[Andrea Marchesini [:baku]]

The reason why this happens is because seeking actually stops the stream, and this generates an event.
In order to prevent this we can add a short timeout in HTMLMediaElement OR in the system app.
To me is the same but probably the system app is a better place for this.

roc, what do you suggest about this issue?

Comment 3

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

Maybe for audiochannel purposes we should treat a seek that started while we were playing (see HTMLMediaElement::mPlayingBeforeSeek) as still playing?

Comment 4

[Andrea Marchesini [:baku]]

Attachment: seek.patch

Comment 5

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

Comment on attachment 8468472 [details] [diff] [review]
seek.patch

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

Excellent :-)

Comment 6

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/02c486347756

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/02c486347756

Comment 8

[mail2tapas]

Checking with patch, but the problem is not resolved completely.

HTMLMediaElement::mPlayingBeforeSeek becomes 0 after few calls to HTMLMediaElement::Seek.
The value returned from IsPotentiallyPlaying() becomes 0 because the value of mReadyState = 2[HAVE_CURRENT_DATA]

Then the value of playingThroughTheAudioChannel starts toggling[0/1] , which triggers StartPlaying/StopPlaying continuously. which makes the music icon  blinking again.

Log shows as below
-------------------------
HTMLMediaElement::Seek time(18.222000) Starting seek 
### mPlayingBeforeSeek = IsPotentiallyPlaying() = [1] 
UpdateAudioChannelPlayingState :: mPlayingBeforeSeek[1]
HTMLMediaElement::IsPotentiallyPlaying ## mReadyState[4]
HTMLMediaElement::SeekCompleted
UpdateAudioChannelPlayingState :: mPlayingBeforeSeek[0]

HTMLMediaElement::IsPotentiallyPlaying ## mReadyState[4]
HTMLMediaElement::Seek time(20.233437) Starting seek 
### mPlayingBeforeSeek = IsPotentiallyPlaying() = [1] 
UpdateAudioChannelPlayingState :: mPlayingBeforeSeek[1]
HTMLMediaElement::IsPotentiallyPlaying ## mReadyState[4]
HTMLMediaElement::SeekCompleted
UpdateAudioChannelPlayingState :: mPlayingBeforeSeek[0]

HTMLMediaElement::IsPotentiallyPlaying ## mReadyState[4]
HTMLMediaElement::Seek time(22.244875) Starting seek 
### mPlayingBeforeSeek = IsPotentiallyPlaying() = [1] 

HTMLMediaElement::IsPotentiallyPlaying ## mReadyState[1]
HTMLMediaElement::Seek time(24.256312) Starting seek 
### mPlayingBeforeSeek = IsPotentiallyPlaying() = [0] 
UpdateAudioChannelPlayingState :: mPlayingBeforeSeek[0]

HTMLMediaElement::IsPotentiallyPlaying ## mReadyState[2]
HTMLMediaElement::Seek time(26.267750) Starting seek 
### mPlayingBeforeSeek = IsPotentiallyPlaying() = [0] 
UpdateAudioChannelPlayingState :: mPlayingBeforeSeek[0]

HTMLMediaElement::IsPotentiallyPlaying ## mReadyState[2]
HTMLMediaElement::Seek time(28.279125) Starting seek

Comment 9

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

Alright. I think we need to change this patch so that as well as mPlayingBeforeSeek we have a separate mPlayingThroughTheAudioChannelBeforeSeek.

Comment 10

[mail2tapas]

Need suggestion if the below approach is correct logic to resolve the issue with mPlayingThroughTheAudioChannelBeforeSeek

Problem : SeekStart/Seekcompleted should be triggered continuously in intervals for long press on seek button.
But once the ready state changes the Seekcompleted not getting triggered.

Below changes solves the Issue:
-------------------------------

step 1
Initialize the mPlayingThroughTheAudioChannelBeforeSeek false.

step 2
//set the value to true on SeekStarted
void HTMLMediaElement::SeekStarted(){
......
 if(mPlayingThroughTheAudioChannel)
      mPlayingThroughTheAudioChannelBeforeSeek = true;
}

step 3
//un-set the value to true on SeekStarted
void HTMLMediaElement::SeekCompleted(){
......
mPlayingThroughTheAudioChannelBeforeSeek = false;
}

step 4
//add check condition to change the audio channel
void HTMLMediaElement::UpdateAudioChannelPlayingState(){
......
bool playingThroughTheAudioChannel =
     (!mPaused &&
      (HasAttr(kNameSpaceID_None, nsGkAtoms::loop) ||
       (mReadyState >= nsIDOMHTMLMediaElement::HAVE_CURRENT_DATA && !IsPlaybackEnded()) ||
        mPlayingThroughTheAudioChannelBeforeSeek));
......
}

Comment 11

[Andrea Marchesini [:baku]]

> Below changes solves the Issue:

Would be nice if you submit a patch for this. Your code seems correct.
If you need help, ping me on IRC.

Comment 12

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

Yes, that looks good.

Comment 13

[Andrea Marchesini [:baku]]

Any luck with the patch? Let me know if you want me to write it.

Comment 14

[mail2tapas]

Today I will update the patch.

Comment 15

[mail2tapas]

Attachment: music-icon-blink-on-seek.patch

- Music Icon blinks on Continuous seek
- Added variable to check the seek condition while playing and prevent the audiochannel toggle.

Comment 16

[Andrea Marchesini [:baku]]

Comment on attachment 8478955 [details] [diff] [review]
music-icon-blink-on-seek.patch

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

Thanks! It's good, just a couple of comments about the style. Can you fix them and upload it again?

::: content/html/content/public/HTMLMediaElement.h
@@ +1144,5 @@
>    // in progress seeking. If FALSE then the media element is either not seeking
>    // or was not actively playing before the current seek. Used to decide whether
>    // to raise the 'waiting' event as per 4.7.1.8 in HTML 5 specification.
>    bool mPlayingBeforeSeek;
> +  

remove extra space.

::: content/html/content/src/HTMLMediaElement.cpp
@@ +2033,5 @@
>      mAllowCasting(false),
>      mIsCasting(false),
>      mAudioCaptured(false),
>      mPlayingBeforeSeek(false),
> +	mPlayingThroughTheAudioChannelBeforeSeek(false),

indentation

@@ +3056,5 @@
>  
>  void HTMLMediaElement::SeekStarted()
>  {
>    DispatchAsyncEvent(NS_LITERAL_STRING("seeking"));
> +//Set the Variable if the Seekstarted while active playing

indentation, plus add a space between // and 'Set'

@@ +3057,5 @@
>  void HTMLMediaElement::SeekStarted()
>  {
>    DispatchAsyncEvent(NS_LITERAL_STRING("seeking"));
> +//Set the Variable if the Seekstarted while active playing
> +  if(mPlayingThroughTheAudioChannel)

{}

@@ +3077,5 @@
>    if (mCurrentPlayRangeStart == -1.0) {
>      mCurrentPlayRangeStart = CurrentTime();
>    }
> +  //Unset the variable on seekend
> +  mPlayingThroughTheAudioChannelBeforeSeek = false;

Space between // and Unset

@@ +3918,5 @@
>       (!mPaused &&
>        (HasAttr(kNameSpaceID_None, nsGkAtoms::loop) ||
>         (mReadyState >= nsIDOMHTMLMediaElement::HAVE_CURRENT_DATA &&
> +	   !IsPlaybackEnded()) ||
> +        mPlayingThroughTheAudioChannelBeforeSeek)); 

extra space

Comment 17

[mail2tapas]

Attachment: music-icon-blink-on-seek-updated.patch

Updated patch as per review comments.

Comment 18

[mail2tapas]

Attachment: music-icon-blink-on-seek.patch

Updated the patch as per review comments

Comment 19

[Andrea Marchesini [:baku]]

Comment on attachment 8479009 [details] [diff] [review]
music-icon-blink-on-seek.patch

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

I want to have a feedback from roc, but it looks good to me.

::: content/html/content/src/HTMLMediaElement.cpp
@@ +3059,5 @@
>    DispatchAsyncEvent(NS_LITERAL_STRING("seeking"));
> +  // Set the Variable if the Seekstarted while active playing
> +  if(mPlayingThroughTheAudioChannel) {
> +    mPlayingThroughTheAudioChannelBeforeSeek = true;
> +  }	  

extra space here.

@@ +3918,5 @@
>       (!mPaused &&
>        (HasAttr(kNameSpaceID_None, nsGkAtoms::loop) ||
>         (mReadyState >= nsIDOMHTMLMediaElement::HAVE_CURRENT_DATA &&
> +	!IsPlaybackEnded()) ||
> +       mPlayingThroughTheAudioChannelBeforeSeek)); 

extra space here :)

Comment 20

[Andrea Marchesini [:baku]]

Good. If you can upload a new patch without these 2 extra space, we can land it. But before we must be sure that it doesn't break any existing test.

I pushed your patch to try to see if it breaks some existing mochitests:

https://tbpl.mozilla.org/?tree=Try&rev=bbf68122e903

Would be nice if you can provide a mercurial version of this patch:

https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 21

[mail2tapas]

Attachment: updated-music-icon-blink-on-seek.patch

Updated the patch, Removed the extra spaces.

Comment 22

[mail2tapas]

I will go through the procedure the create mercurial version of the patch. Since first time i am trying for this kind patch, it may take some time.

Comment 23

[Andrea Marchesini [:baku]]

Comment on attachment 8479575 [details] [diff] [review]
updated-music-icon-blink-on-seek.patch

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

Perfect! And it's green on tbpl. Let me know if you need help for the mercurial patch.

Comment 24

[mail2tapas]

Attachment: Mercurial Patch format for Music icon Blink Issue

- Mercurial Patch format for the issue.

Comment 25

[Andrea Marchesini [:baku]]

Comment on attachment 8480479 [details] [diff] [review]
Mercurial Patch format for Music icon Blink Issue

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

It applies on m-c. Remove this space and we can land it.

::: content/html/content/public/HTMLMediaElement.h
@@ +1149,5 @@
>    // in progress seeking. If FALSE then the media element is either not seeking
>    // or was not actively playing before the current seek. Used to decide whether
>    // to raise the 'waiting' event as per 4.7.1.8 in HTML 5 specification.
>    bool mPlayingBeforeSeek;
> +  

Extra space :)

Comment 26

[mail2tapas]

Attachment: updated-hg-music-icon-blink-on-seek.patch

- Updated the Patch 
- Please let us know procedure and  which tool/editor to be used to cross check these extra spaces .

Comment 27

[Andrea Marchesini [:baku]]

Perfect! Now your patch is ready to land.
Add 'checkin-needed' in the Keywords and a sheriff will land this patch to mozilla-inbound.

About the extra spaces: before uploading a patch, I check and re-read it using 'hg qdiff'.
If you use the 'color' mercurial extension, the extra spaces are clearly visible.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Please mark old patches obsolete when attaching new ones in the future.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d0b459bfc36

Thanks for the patches! One request, please try to be conscientious about the platforms/tests you run on your Try pushes. "All" runs are very expensive from a machine time perspective and contribute to long backlogs experienced by all developers. The link below has some recommendations to consider. Thanks!
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0d0b459bfc36