Closed Bug 858025 Opened 9 years ago Closed 9 years ago

Playback speed changes to 1x after seeking or pausing media

Categories

(Toolkit :: Video/Audio Controls, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox22 - wontfix
firefox23 - wontfix
firefox24 --- affected
firefox25 --- verified

People

(Reporter: mihaelav, Assigned: sankha)

References

()

Details

(Keywords: regression, verifyme)

Attachments

(1 file, 5 obsolete files)

Reproducible on*:
* the latest Nightly (BuildID: 20130403090950): Mozilla/5.0 (X11; Linux i686; rv:22.0) Gecko/20130308 Firefox/22.0
* the latest Aurora (BuildID: 20130402004013): Mozilla/5.0 (X11; Linux i686; rv:22.0) Gecko/20130402 Firefox/22.0
OS: All

With latest Nightly (23.0a1) and Aurora (22.0a2), the play speed jumps to normal when navigating to other location in the video.

Steps to reproduce:
1. Start playing a HTML5 video (e.g. https://videos-cdn.mozilla.net/serv/Marketplace-videos/FirefoxMarketplace-airbnb-BR-RC-SD1%20640.webm)
2. Right click on the video and change the play speed 
=> Video is playing at the selected speed
3. Click on some other location in the video progress bar

Expected result: Video starts playing at the new location with the selected speed from step 2

Actual result: Video continues playing at the new location with normal (1x) speed.

Note: 
Issue is reproducible ever since March the 8th Nightly build, when the play speed option was introduced:
Mozilla/5.0 (X11; Linux i686; rv:22.0) Gecko/20130308 Firefox/22.0, BuildID: 20130308031005
Blocks: 840745
Summary: Playback speed changes to 1x when jumping to other time in the video → Playback speed changes to 1x when jumping to other time in the video/audio
This is a problem with our built-in controls, I think.

When we seek, the controls can call |pause()| on the element, and then call |play()| when the playback can resume. Calling |play()| on an element sets the playbackRate to the defaultPlaybackRate (which is 1.0 by default, but can be changed), hence the observed behavior.

This should be fixed in the controls, and would fix the bug where there is a "playing" event fired when seeking.
(In reply to Paul Adenot (:padenot) from comment #1)
> This is a problem with our built-in controls, I think.
> 
> When we seek, the controls can call |pause()| on the element, and then call
> |play()| when the playback can resume. Calling |play()| on an element sets
> the playbackRate to the defaultPlaybackRate (which is 1.0 by default, but
> can be changed), hence the observed behavior.
> 
> This should be fixed in the controls, and would fix the bug where there is a
> "playing" event fired when seeking.

If that's the case, shouldn't that cause the changing of the play speed to normal when pressing pause and and then play, as well? It doesn't - if you change the speed, press pause and then play, the video/audio will play with the selected speed, not with normal one.
I tested with latest Nightly: Mozilla/5.0 (X11; Linux i686; rv:23.0) Gecko/20130403 Firefox/23.0, Build ID: 20130403090950
You are seeing bug 825329 for the play/pause that does not reset the playback rate. I should land it.
It's quite annoying that pausing the video also resets the playback speed (now that bug 825329 is fixed). CC'ing darkowlzz & jaws from bug 840745.
Summary: Playback speed changes to 1x when jumping to other time in the video/audio → Playback speed changes to 1x after seeking or pausing media
Sunny, would you like to work on this?
Flags: needinfo?(indiasuny000)
We're on our final beta - I don't think this should be a tracked bug for 22. I think we can fix in 23 without issue (or relnote).
Sure.
Status: NEW → ASSIGNED
Flags: needinfo?(indiasuny000)
Sure.(In reply to Jared Wein [:jaws] from comment #5)
> Sunny, would you like to work on this?

Sure.
Assignee: nobody → indiasuny000
We really want this fix but we won't be able to take anything in the last couple of weeks of Beta cycle so there's only a couple of weeks left to take a speculative, low-risk patch here.
Flags: needinfo?(indiasuny000)
Hi,
The scrubber code, here (http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#176), seems to be the place to make changes but I don't see how I could pass the current playbackRate to |play()|. 

|Play()| here (http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLMediaElement.cpp#2058) sets the |PlaybackRate| to default rate. Should I be getting the current playbackRate in HTMLMediaElement.cpp and pass it to |SetPlaybackRate()| or, as mentioned in comment #1, videocontrols.xml should be changed in order to achieve the same?
Flags: needinfo?(indiasuny000)
I think all you'll need to do is the following,

 if (this.type == "scrubber") {
     this.Utils.log("--- dragStateChanged: " + isDragging + " ---");
     this.isDragging = isDragging;
     if (isDragging) {
         this.wasPausedBeforeDrag = this.Utils.video.paused;
+        this.previousDefaultPlaybackRate = this.Utils.video.defaultPlaybackRate;
+        this.previousPlaybackRate = this.Utils.video.playbackRate;
         this.Utils.video.pause();
     } else if (!this.wasPausedBeforeDrag) {
         // After the drag ends, resume playing.
+        this.Utils.video.defaultPlaybackRate = this.previousPlaybackRate;
         this.Utils.video.play();
+        this.Utils.video.defaultPlaybackRate = this.previousDefaultPlaybackRate;
     }
 }

Hope that helps!
Made the changes as suggested by jaws.
Due to some reason, I am unable to see the effect of changes on my computer, even after building it many times. Please see it this works on your machine.
Attachment #771127 - Flags: feedback?(paul)
Attachment #771127 - Flags: feedback?(jaws)
Comment on attachment 771127 [details] [diff] [review]
Changed the DefaultPlaybackRate to current playbackRate before calling |Play()| and reverted it back after the call

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

::: toolkit/content/widgets/videocontrols.xml
@@ +178,4 @@
>                      this.Utils.video.pause();
>                  } else if (!this.wasPausedBeforeDrag) {
>                      // After the drag ends, resume playing.
> +                    this.Utils.video.defaultPlaybackRate = this.previousPlaybackRate;

This is wrong, it should be:

> this.Utils.video.playbackRate = this.previousPlaybackRate;
Attachment #771127 - Flags: feedback?(paul)
Assigning to myself
Assignee: indiasuny000 → sankha93
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #771127 - Attachment is obsolete: true
Attachment #775735 - Flags: review?(paul)
Paul, this is working. I have taken this up from darkowlzz because he will not be able to work on it for few days.
Comment on attachment 775735 [details] [diff] [review]
patch v1

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

Please try this patch with the patches from bug 886173.

::: toolkit/content/widgets/videocontrols.xml
@@ +178,4 @@
>                      this.Utils.video.pause();
>                  } else if (!this.wasPausedBeforeDrag) {
>                      // After the drag ends, resume playing.
> +                    this.Utils.video.defaultPlaybackRate = this.previousPlaybackRate;

s/defaultPlaybackRate/playbackRate/
Attached patch patch v2 (obsolete) — Splinter Review
This working with the patches on bug 886173.
Attachment #775735 - Attachment is obsolete: true
Attachment #775735 - Flags: review?(paul)
Attachment #776410 - Flags: review?(paul)
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #776410 - Attachment is obsolete: true
Attachment #776410 - Flags: review?(paul)
Attachment #776413 - Flags: review?(paul)
Attached patch patch v2 (obsolete) — Splinter Review
Sorry, for uploading the wrong file twice.
Attachment #776413 - Attachment is obsolete: true
Attachment #776413 - Flags: review?(paul)
Attachment #776415 - Flags: review?(paul)
Depends on: 886173
This is our last week to get this on Beta for FF23 - please nominate for uplift if low risk.
Flags: needinfo?(sankha93)
Keywords: regression
Comment on attachment 776415 [details] [diff] [review]
patch v2

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

::: toolkit/content/widgets/videocontrols.xml
@@ +182,2 @@
>                      this.Utils.video.play();
> +                    this.Utils.video.defaultPlaybackRate = this.previousDefaultPlaybackRate;

Is there a reason this line is after "this.Utils.video.play()" ?
(In reply to Paul Adenot (:padenot) from comment #22)
> Comment on attachment 776415 [details] [diff] [review]
> patch v2
> 
> Review of attachment 776415 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/content/widgets/videocontrols.xml
> @@ +182,2 @@
> >                      this.Utils.video.play();
> > +                    this.Utils.video.defaultPlaybackRate = this.previousDefaultPlaybackRate;
> 
> Is there a reason this line is after "this.Utils.video.play()" ?

I recommended that at the same time that I recommended changing defaultPlaybackRate before calling play() so that the defaultPlaybackRate would be reset to it's actual value (not the hacked value to get the playback rate to match before pausing).

Now that defaultPlaybackRate is not modified, both lines touch it can be removed.
Removed the defaultPlaybackrate lines.
Attachment #776415 - Attachment is obsolete: true
Attachment #776415 - Flags: review?(paul)
Attachment #778420 - Flags: review?(paul)
(In reply to lsblakk@mozilla.com [:lsblakk] from comment #21)
> This is our last week to get this on Beta for FF23 - please nominate for
> uplift if low risk.

This will be landed along with patches in bug 886173.
Flags: needinfo?(sankha93)
Attachment #778420 - Flags: review?(paul) → review+
https://hg.mozilla.org/mozilla-central/rev/03d0dc9e1cb8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Keywords: verifyme
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (X11; Linux i686; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20100101 Firefox/25.0

Verified on Aurora (build ID: 20130910004002) using the steps from comment 0.
Video starts playing at the new location with the selected rate.
You need to log in before you can comment on or make changes to this bug.