960599 - Changing playbackRate on a video element causes the image to freeze

Status: RESOLVED WORKSFORME

(Core :: Audio/Video, defect)

Description

[Stefan]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140116030250

Steps to reproduce:

On Fedora 19 running KDE, in both the installed version 26.0 and nightlies up to 2014-01-16:

I loaded an mp4 file in a new tab, and then on the console:
> var video = document.getElementsByTagName("video")[0];
> video.playbackRate = 1.5;

The issue is also noticeable on websites like Youtube that use HTML5 video.


Actual results:

Playback sped up as expected, but the image froze after playing a few frames. Seeking causes a few more frames to play at the new position, but once again the image freezes. Throughout, the audio plays as expected.

Comment 1

[[:Aleksej]]

(WebM)

Image froze with 2014-01-20-03-02-02-mozilla-central-firefox-29.0a1.en-US.linux-x86_64 (haven't checked audio).

WFM with 27.0 Beta ≈7 with my profile (many extensions, including NoScript).

Comment 2

[Paul Silaghi, QA [:pauly]]

NEW based on comment 1

Comment 3

[Paul Adenot (:padenot)]

(WFM on 2014-01-20 - linux 64)

Comment 4

[[:Aleksej]]

It might happen on the second try, with a different rate.

Reproduced with 2013-05-20-03-12-30-mozilla-central-firefox-24.0a1.en-US.linux-x86_64.

Comment 5

[Paul Silaghi, QA [:pauly]]

Confirmed in 29.0a1(2014-01-21), ubuntu 13.04 x64 on youtube/html5 with
var video = document.getElementsByTagName("video")[0];
video.playbackRate = 2.5;

The image froze, the sound didn't.

Comment 6

[Paul Adenot (:padenot)]

I can repro, but only on youtube videos.

Comment 7

[Alice0775 White]

Regression winodw(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/a4e9c9c9dbf9
Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130912030201
Bad:
http://hg.mozilla.org/mozilla-central/rev/b9029b1de410
Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130913030201
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a4e9c9c9dbf9&tochange=b9029b1de410

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/e9ee43da0c16
Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130911162107
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/0192ffffc633
Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130911162531
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e9ee43da0c16&tochange=0192ffffc633

Regressed by:
0192ffffc633	Edwin Flores — Bug 886181 - Pref on gstreamer backend r=cpearce

Comment 8

[Benjamin Kerensa [:bkerensa]]

Edwin, can you have a look at this?

Comment 9

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 960599.patch

The problem here was that GStreamer was trying to sync the video stream with a clock running in realtime. When playbackRate > 1, we get ahead of the GStreamer clock and it throttles video decode to keep us from getting too far ahead of the clock.

This patch simply disables GStreamer's clock sync.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8363931 [details] [diff] [review]
960599.patch

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

This makes sense to me, but I want Alessandro to f+ before you land, in case this will have unexpected consequences.

Comment 11

[Alessandro Decina]

Comment on attachment 8363931 [details] [diff] [review]
960599.patch

Apparently we're doing


  mVideoSink = gst_parse_bin_from_description("capsfilter name=filter ! "
      "appsink name=videosink sync=true max-buffers=1 "
      "caps=video/x-raw-yuv,format=(fourcc)I420"
      , TRUE, nullptr);

notice sync=true.

In the 1.0 patch I already changed it to sync=false. Edwin, have you tried to flip sync to false? It should be enough, and shouldn't require tinkering with the pipeline clock directly.

Comment 12

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(In reply to Alessandro Decina from comment #11)
> In the 1.0 patch I already changed it to sync=false. Edwin, have you tried
> to flip sync to false? It should be enough, and shouldn't require tinkering
> with the pipeline clock directly.

Yes, I tried that, but gstreamer ends up trying to sync the audio and video streams with each other, which can conflict with gecko's queueing/syncing. This way audio and video are kept separate.

Comment 13

[Alessandro Decina]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #12)
> (In reply to Alessandro Decina from comment #11)
> > In the 1.0 patch I already changed it to sync=false. Edwin, have you tried
> > to flip sync to false? It should be enough, and shouldn't require tinkering
> > with the pipeline clock directly.
> 
> Yes, I tried that, but gstreamer ends up trying to sync the audio and video
> streams with each other, which can conflict with gecko's queueing/syncing.
> This way audio and video are kept separate.

Weird, do you know where it's trying to do that? Because synchronization in gst should happen only in sinks, and only in sinks having sync=true

Comment 14

[Alessandro Decina]

Fwiw, I can't reproduce this with the 1.0 patch from https://bugzilla.mozilla.org/show_bug.cgi?id=806917 applied (which sets sync=false)

Comment 15

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(In reply to Alessandro Decina from comment #14)
> Fwiw, I can't reproduce this with the 1.0 patch from
> https://bugzilla.mozilla.org/show_bug.cgi?id=806917 applied (which sets
> sync=false)

This bug happens with vsync true and async false. Bug 884651 happens when vsync = async.

Comment 16

[Alessandro Decina]

I think the main issue with https://bugzilla.mozilla.org/show_bug.cgi?id=884651 was that we were not setting max-buffers=1 (as we do now), so gst never stopped decoding, even if MediaDecoderStateMachine was paused (because playback was paused). Now with the 1.0 patch, sync=false and max-buffers=1 I can't repro that bug either.

Comment 17

[Alessandro Decina]

(In reply to Alessandro Decina from comment #16)
> I think the main issue with
> https://bugzilla.mozilla.org/show_bug.cgi?id=884651 was that we were not
> setting max-buffers=1 (as we do now), so gst never stopped decoding, even if
> MediaDecoderStateMachine was paused (because playback was paused). Now with
> the 1.0 patch, sync=false and max-buffers=1 I can't repro that bug either.

Sorry that's wrong. The problem there was that we were setting sync=true in both sinks, and because firefox was not consuming fast enough (eg on paused playback), gst QoS was kicking in, causing all the frames to be decoded and discarded. Anyway, I am confident that setting sync=false in both sinks won't cause that issue again.

Comment 18

[Ioana (away)]

Removing qawanted since this request was already answered.

Comment 19

[Alessandro Decina]

I am pretty sure this was fixed as part of landing https://bugzilla.mozilla.org/show_bug.cgi?id=806917. I can't repro, can someone double check?

Comment 20

[Alice0775 White]

I can confirm this is fixed.

Progression window
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/af35bd7bd9a8
Mozilla/5.0 (X11; Linux i686; rv:30.0) Gecko/20100101 Firefox/30.0 ID:20140211061742
Fixed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a99ebc2f515
Mozilla/5.0 (X11; Linux i686; rv:30.0) Gecko/20100101 Firefox/30.0 ID:20140211062342
Fixed pushlog
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=af35bd7bd9a8&tochange=4a99ebc2f515

Yes, Bug 806917 seemed to fix this problem.

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

Marking affected on 30 so this can be updated when landed to central - also please nominate for branch uplift with a risk evaluation.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Bug 806917 won't be uplifted (a 82kb patch) cf comment 119. So, this bug won't be fixed for 28 or 29.
Tagging this bug as wontfix then.

Comment 23

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

No longer reproducible.