Closed Bug 1048171 Opened 5 years ago Closed 5 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S3 (29aug)

People

(Reporter: sharaf.tir, Assigned: baku)

Details

(Whiteboard: [LibGLA, TD-83098, WW, B])

Attachments

(1 file, 6 obsolete files)

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
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
Flags: needinfo?(amarchesini)
Whiteboard: [LibGLA, TD-83098, WW, B]
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?
Flags: needinfo?(amarchesini) → needinfo?(roc)
Maybe for audiochannel purposes we should treat a seek that started while we were playing (see HTMLMediaElement::mPlayingBeforeSeek) as still playing?
Flags: needinfo?(roc)
Attached patch seek.patch (obsolete) — Splinter Review
Attachment #8468472 - Flags: review?(roc)
Comment on attachment 8468472 [details] [diff] [review]
seek.patch

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

Excellent :-)
Attachment #8468472 - Flags: review?(roc) → review+
Assignee: nobody → amarchesini
Status: UNCONFIRMED → NEW
Ever confirmed: true
https://hg.mozilla.org/mozilla-central/rev/02c486347756
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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
Flags: needinfo?(roc)
Flags: needinfo?(amarchesini)
Alright. I think we need to change this patch so that as well as mPlayingBeforeSeek we have a separate mPlayingThroughTheAudioChannelBeforeSeek.
Flags: needinfo?(roc)
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));
......
}
Flags: needinfo?(roc)
> 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.
Flags: needinfo?(amarchesini)
Yes, that looks good.
Flags: needinfo?(roc)
Any luck with the patch? Let me know if you want me to write it.
Flags: needinfo?(mail2tapas)
Today I will update the patch.
Flags: needinfo?(mail2tapas)
Attached patch music-icon-blink-on-seek.patch (obsolete) — Splinter Review
- Music Icon blinks on Continuous seek
- Added variable to check the seek condition while playing and prevent the audiochannel toggle.
Attachment #8478955 - Flags: review?(amarchesini)
Flags: needinfo?(amarchesini)
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
Attachment #8478955 - Flags: review?(amarchesini)
Updated patch as per review comments.
Attachment #8478955 - Attachment is obsolete: true
Attached patch music-icon-blink-on-seek.patch (obsolete) — Splinter Review
Updated the patch as per review comments
Attachment #8479006 - Attachment is obsolete: true
Attachment #8479009 - Flags: review?(amarchesini)
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 :)
Attachment #8479009 - Flags: review?(amarchesini)
Attachment #8479009 - Flags: review+
Attachment #8479009 - Flags: feedback?(roc)
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
Flags: needinfo?(amarchesini) → needinfo?(mail2tapas)
Updated the patch, Removed the extra spaces.
Attachment #8479575 - Flags: review?(amarchesini)
Flags: needinfo?(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 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.
Attachment #8479575 - Flags: review?(amarchesini) → review+
- Mercurial Patch format for the issue.
Attachment #8480479 - Flags: review?(amarchesini)
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 :)
Attachment #8480479 - Flags: review?(amarchesini) → review+
- Updated the Patch 
- Please let us know procedure and  which tool/editor to be used to cross check these extra spaces .
Attachment #8480502 - Flags: review?(amarchesini)
Attachment #8480502 - Flags: review?(amarchesini) → review+
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.
Keywords: checkin-needed
Please mark old patches obsolete when attaching new ones in the future.
Attachment #8468472 - Attachment is obsolete: true
Attachment #8479009 - Attachment is obsolete: true
Attachment #8479575 - Attachment is obsolete: true
Attachment #8480479 - Attachment is obsolete: true
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0d0b459bfc36
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S3 (29aug)
You need to log in before you can comment on or make changes to this bug.