1130237 - loadeddata event should only be fired if we have content to play at playback position

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P3)

Description

[Jean-Yves Avenard [:jya]]

From W3C spec:
http://w3c.github.io/media-source/#buffer-monitoring

If HTMLMediaElement.buffered does not contain a TimeRange for the current playback position:

        Set the HTMLMediaElement.readyState attribute to HAVE_METADATA.
        Abort these steps.

If HTMLMediaElement.buffered contains a TimeRange that includes the current playback position and enough data to ensure uninterrupted playback:

        Set the HTMLMediaElement.readyState attribute to HAVE_ENOUGH_DATA.
        Queue a task to fire a simple event named canplaythrough at the media element.
        Playback may resume at this point if it was previously suspended by a transition to HAVE_CURRENT_DATA.
        Abort these steps.

If HTMLMediaElement.buffered contains a TimeRange that includes the current playback position and some time beyond the current playback position, then run the following steps:

        Set the HTMLMediaElement.readyState attribute to HAVE_FUTURE_DATA.
        If the previous value of HTMLMediaElement.readyState was less than HAVE_FUTURE_DATA, then queue a task to fire a simple event named canplay at the media element.
        Playback may resume at this point if it was previously suspended by a transition to HAVE_CURRENT_DATA.
        Abort these steps.

If HTMLMediaElement.buffered contains a TimeRange that ends at the current playback position and does not have a range covering the time immediately after the current position:

        Set the HTMLMediaElement.readyState attribute to HAVE_CURRENT_DATA.
        If this is the first transition to HAVE_CURRENT_DATA, then queue a task to fire a simple event named loadeddata at the media element.
        Playback is suspended at this point since the media element doesn't have enough data to advance the media timeline.
        Abort these steps.

With MSE, start up position is to be 0 (http://w3c.github.io/media-source/#presentation-start-time : The presentation start time is the earliest time point in the presentation and specifies the initial playback position and earliest possible position. All presentations created using this specification have a presentation start time of 0.)

However, MediaSourceReader currently will decode the first frame available in any of the sourcebuffer. If for example we had a video with a buffered range [2,4] ; it would decode as first frame at time 2s and return it to the state machine ; which will issue loadeddata.

As we don't have data at the current playback position (which is 0), loadeddata shouldn't be fired.

Comment 1

[Jean-Yves Avenard [:jya]]

Make it depends on bug 1129732.
As often with some mp4 content, the first timestamp isn't exactly 0. Which if this bug was to be fixed means we would never start playback.

We also can't seek if we haven't decoded the first frame (and fired loadeddata), for which I will spawn another bug.

Comment 2

[Jean-Yves Avenard [:jya]]

I wrote this test:
http://people.mozilla.org/~jyavenard/tests/mse_mp4/paper-offset.html

it loads data from 2-12s.

Actual Behaviour: first frame at 2s is shown and decoded. Loadeddata is fired. Can't seek until you start playback.
Can start playback which does nothing for 2s and then continue.

Expected Behaviour:
No frame is shown, video element is black.
Pressing play does nothing
Can seek to 2s, which would cause loadeddata to be fired, can start playback there.

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: P1. Only ever return a frame if we have data.

After a Reset() the definition is that the resource was seeked to 0. As such we should only return a frame if there's data at time==0.

The solution isn't complete as we can't start a seek until we have decoded the first frame (this is the same behaviour as Chrome), however I believe that this behaviour is incorrect.

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: P2. Reset decoder state even if no decoder has been created yet.

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] P3. Add mochitest testing behavior.

We load data from [2ish, 4] and ensure that playback didn't start before data [0, 2ish] is loaded.

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8695840 [details] [diff] [review]
[MSE] P3. Add mochitest testing behavior.

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

r+ with nits:

::: dom/media/mediasource/test/test_LoadedDataFired_mp4.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>MSE: Check that playback only starts one we have data at time = 0</title>

'one' -> 'once'

@@ +45,5 @@
> +      ok(el.buffered.end(el.buffered.length-1) <= 4, "no data can be found beyond appendWindowEnd");
> +      el.play();
> +      var promises = [];
> +      promises.push(once(el, "play"));
> +      return Promise.all(promises);

Why the 1-promise array, and not just |return once.bind(null, el, "play");|?

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b2990b89377
https://hg.mozilla.org/integration/mozilla-inbound/rev/c77615eb1faa
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8a9e34748f1

Comment 8

[Jean-Yves Avenard [:jya]]

[Tracking Requested - why for this release]: This is a broken, non compliant behaviour, ideally should have been to 43, but it's too late

Comment 9

[Carsten Book [:Tomcat]]

sorry had to back this test failures out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=18347755&repo=mozilla-inbound

Comment 10

[Pulsebot]

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

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6b2990b89377
https://hg.mozilla.org/mozilla-central/rev/c77615eb1faa

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7c5b0bf84d0

Comment 13

[Jean-Yves Avenard [:jya]]

re-landed removing the line causing the error. Which really shouldn't have been there as we perform 3 appendBuffer and currentTime may move in between

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c7c5b0bf84d0

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Gerald, should we consider uplifting this fix to Beta44?

Comment 16

[Jean-Yves Avenard [:jya]]

It would be nice, as it fixes the regression talked about in bug 1230141 and make us more compliant with the spec. There's a few bugs this depends on however

Comment 17

[Jean-Yves Avenard [:jya]]

Comment on attachment 8695713 [details] [diff] [review]
P1. Only ever return a frame if we have data.

Approval is for patch #1 and #2 only (we leave the mochitest out as it relies on other changes not currently enabled in 44)

Approval Request Comment
[Feature/regressing bug #]: 1130237
[User impact if declined]: Regression on 43 following on changes on AudioSink.
[Describe test coverage new/current, TreeHerder]: In central and aurora for several weeks
[Risks and why]: Low, we're just implementing the proper spec-compliant behaviour. We used to always display the first frame of the video, no matter its position, now we don't (this is the same behaviour as Chrome however)
[String/UUID change made/needed]: None

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Jean-Yves, at this point in Beta44 cycle I am only taking fixes for critical regression and those improve browser stability. Could you please help me understand how often and in what situations will end-users encounter this bug? Do "changes to AudioSink" mean changing the playback device e.g. from speaker to headset? I did read comment 2, but that alone is not helping me decide whether this is a release blocker or not.

Comment 19

[Jean-Yves Avenard [:jya]]

The bug is described in 1230141

Similar case is likely to happen with live streams DASH video.

I can't assess if the issue is purely cosmetic, or if having the time starts ticking when it didn't use to would cause serious regression in playback.

Based on my experience, anything touching and regressing official media element spec (like bug 1230141 does) typically gets really bad. but that may not be the case this time.

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

When looking at bug 1230141 , I noticed that the issue happened due to https://bugzilla.mozilla.org/show_bug.cgi?id=1230141#c5. I am also leaning towards Jean-Yves's suggestion of backing out the patch. Can we do that instead of uplifting another patch in FF44?

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8695713 [details] [diff] [review]
P1. Only ever return a frame if we have data.

Instead of uplifting this, let's backout the offending patch. Please see my previous comment.

Comment 22

[Jean-Yves Avenard [:jya]]

(In reply to Ritu Kothari (:ritu) from comment #20)
> When looking at bug 1230141 , I noticed that the issue happened due to
> https://bugzilla.mozilla.org/show_bug.cgi?id=1230141#c5. I am also leaning
> towards Jean-Yves's suggestion of backing out the patch. Can we do that
> instead of uplifting another patch in FF44?

I agree. Problem with this patch is that in 45 there was a lot of work to make buffer/resource monitoring be spec compliant, and those patches are just a tiny bit of it.

Backing out bug 1194918 only make us have the same (still broken, but less so) behaviour than we've had since 38.
It may have negative consequences on b2g however, I don't know how much they rely on that change

Comment 23

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Ritu Kothari (:ritu) from comment #20)
> When looking at bug 1230141 , I noticed that the issue happened due to
> https://bugzilla.mozilla.org/show_bug.cgi?id=1230141#c5. I am also leaning
> towards Jean-Yves's suggestion of backing out the patch. Can we do that
> instead of uplifting another patch in FF44?

I still can't see how bug 1230141 is resulted from bug 1194918 which only extracts the code of audio/video rendering. It looks more like to me that the code changes of bug 1194918 unveil some other hidden problems.

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(Trying to get more people involved as I am unable to decide which way to go)

Maire, Ralph: I am trying to figure out whether uplifting this patch is a must for 44 or not. Please see comment 20. Would you (someone?) be able to help me determine which is the least risky option? 

As you already know, Fx44 is already in a RC-type mode, the bar for uplifting bugs is very high. We are only taking fixes to critical regressions, stability and sec fixes. Do you think this meets the bar? or should we back out the offending patch as pointed to by the regression range in bug 1230141. Any help would be awesome.

Comment 25

[Maire Reavy [:mreavy]]

Hi Ritu - you want to ping Anthony (engineering manager for playback) on this bug; moving my NeedInfo to him.

Anthony - see comment 24

Comment 26

[Ralph Giles (:rillian)]

Back-out sounds the safer of the two, but still not risk-free. Anthony has more of the context than I do at this point.

Comment 27

[Blake Wu [:bwu][:blakewu]]

Two main reasons that we should not backout Bug 1194918. 

1. As I know, FxOS 2.5 settles on 44 which TV uses it. Bug 1194918 is important to FxOS since TV partner uses it.  
2. Per comment 23 and bug 1230141 comment 12, Bug 1194918 seems not the murderer, may just unveil some problems. And some patch in 45 can fix that. We should find that patch to uplift.

Comment 28

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

This bug is a regression in that it makes something more broken than it was before. However it isn't worth the risk of backing out the change to get what amounts to a better failure mode. It does not affect any known public website and it can be worked around.

Comment 29

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

I recommend we leave it as is and don't try to back anything out.

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #29)
> I recommend we leave it as is and don't try to back anything out.

Thanks Anthony! We will leave bug 1230141 as a wontfix in that case. Appreciate your help here, I am glad we decided not to backout or land a new patch for this issue. I think it's the right call.