858025 - Playback speed changes to 1x after seeking or pausing media

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Mihaela Velimiroviciu (:mihaelav)]

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

Comment 1

[Paul Adenot (:padenot)]

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.

Comment 2

[Mihaela Velimiroviciu (:mihaelav)]

(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

Comment 3

[Paul Adenot (:padenot)]

You are seeing bug 825329 for the play/pause that does not reset the playback rate. I should land it.

Comment 4

[Matthew N. [:MattN]]

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.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Sunny, would you like to work on this?

Comment 6

[Alex Keybl [:akeybl]]

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

Comment 7

[Sunny [:darkowlzz]]

Sure.

Comment 8

[Sunny [:darkowlzz]]

Sure.(In reply to Jared Wein [:jaws] from comment #5)
> Sunny, would you like to work on this?

Sure.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 10

[Sunny [:darkowlzz]]

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?

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

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!

Comment 12

[Sunny [:darkowlzz]]

Attachment: Changed the DefaultPlaybackRate to current playbackRate before calling |Play()| and reverted it back after the call

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.

Comment 13

[Paul Adenot (:padenot)]

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;

Comment 14

[Sankha Narayan Guria [:sankha]]

Assigning to myself

Comment 15

[Sankha Narayan Guria [:sankha]]

Attachment: patch v1

Comment 16

[Sankha Narayan Guria [:sankha]]

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 17

[Paul Adenot (:padenot)]

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/

Comment 18

[Sankha Narayan Guria [:sankha]]

Attachment: patch v2

This working with the patches on bug 886173.

Comment 19

[Sankha Narayan Guria [:sankha]]

Attachment: patch v2

Comment 20

[Sankha Narayan Guria [:sankha]]

Attachment: patch v2

Sorry, for uploading the wrong file twice.

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

This is our last week to get this on Beta for FF23 - please nominate for uplift if low risk.

Comment 22

[Paul Adenot (:padenot)]

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()" ?

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 24

[Sankha Narayan Guria [:sankha]]

Attachment: playbackSpeed.patch

Removed the defaultPlaybackrate lines.

Comment 25

[Sankha Narayan Guria [:sankha]]

(In reply to [e-mail] [: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.

Comment 26

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/03d0dc9e1cb8

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/03d0dc9e1cb8

Comment 28

[Cornel Ionce [:noni] [Hubs QA]]

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.