Closed Bug 1130237 Opened 5 years ago Closed 4 years ago

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

Categories

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

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox42 --- wontfix
firefox43 --- wontfix
firefox44 - wontfix
firefox45 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

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.
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.
Depends on: 1129732
Priority: -- → P2
Depends on: 1135170
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.
Priority: P2 → P3
Component: Audio/Video → Audio/Video: Playback
Blocks: 1230141
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.
Attachment #8695713 - Flags: review?(gsquelart)
Assignee: nobody → jyavenard
Attachment #8695713 - Flags: review?(gsquelart) → review+
Attachment #8695730 - Flags: review?(gsquelart) → review+
We load data from [2ish, 4] and ensure that playback didn't start before data [0, 2ish] is loaded.
Attachment #8695840 - Flags: review?(gsquelart)
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");|?
Attachment #8695840 - Flags: review?(gsquelart) → review+
[Tracking Requested - why for this release]: This is a broken, non compliant behaviour, ideally should have been to 43, but it's too late
sorry had to back this test failures out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=18347755&repo=mozilla-inbound
Flags: needinfo?(jyavenard)
https://hg.mozilla.org/mozilla-central/rev/6b2990b89377
https://hg.mozilla.org/mozilla-central/rev/c77615eb1faa
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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
Flags: needinfo?(jyavenard)
Gerald, should we consider uplifting this fix to Beta44?
Flags: needinfo?(gsquelart)
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
Flags: needinfo?(gsquelart)
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
Attachment #8695713 - Flags: approval-mozilla-beta?
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.
Flags: needinfo?(jyavenard)
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.
Flags: needinfo?(jyavenard)
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?
Flags: needinfo?(jyavenard)
Flags: needinfo?(jwwang)
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.
Attachment #8695713 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
(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
Flags: needinfo?(jyavenard)
(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.
Flags: needinfo?(jwwang)
(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.
Flags: needinfo?(mreavy)
Flags: needinfo?(giles)
Hi Ritu - you want to ping Anthony (engineering manager for playback) on this bug; moving my NeedInfo to him.

Anthony - see comment 24
Flags: needinfo?(mreavy) → needinfo?(ajones)
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.
Flags: needinfo?(giles)
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.
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.
Flags: needinfo?(ajones)
I recommend we leave it as is and don't try to back anything out.
(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.
You need to log in before you can comment on or make changes to this bug.