Closed Bug 1123189 Opened 5 years ago Closed 5 years ago

Queue "durationchange" instead of dispatching synchronously

Categories

(Core :: Audio/Video, defect, P2)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox36 --- fixed
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: karlt, Assigned: jya)

References

Details

Attachments

(2 files, 4 obsolete files)

Queuing durationchange from MediaDecoder::DurationChanged() is particularly
important for MediaSourceDecoder, which calls the function synchronously from
content-exposed methods, such as during the MediaSource duration setter for
example, but is also required for all media elements:

http://www.w3.org/TR/2014/REC-html5-20141028/embedded-content-0.html#offsets-into-the-media-resource

"When the length of the media resource changes to a known value (e.g. from being unknown to known, or from a previously established length to a new length) the user agent must queue a task to fire a simple event named durationchange at the media element."

This would seem consistent with efforts to queue timeupdate, abort, and
emptied events.

https://hg.mozilla.org/mozilla-central/rev/a6d06ecdfde1
https://hg.mozilla.org/mozilla-central/rev/e5ae3b72e03c
Queue durationchange event instead of firing it simultaneously
Attachment #8551080 - Flags: review?(karlt)
Assignee: karlt → jyavenard
Comment on attachment 8551080 [details] [diff] [review]
Queue "durationchange" instead of dispatching synchronously

Yes, this is a subset of my patch in comment 1 [1].
As this affects non-MSE media elements, I'd still like to ask cpearce to look at it, in case he is aware of any unexpected side-effects.
I think we can also keep the removal of the now unused MediaDecoderOwner::DispatchEvent().

[1] https://hg.mozilla.org/try/rev/cf705732766f
Attachment #8551080 - Flags: review?(karlt) → review+
Attached patch Fix mochitest (obsolete) — Splinter Review
Fix mochitest test_DurationChanged.html
Attachment #8551509 - Flags: review?(karlt)
Attachment #8551502 - Flags: review?(cpearce) → review+
Attachment #8551080 - Attachment is obsolete: true
Attached patch Fix mochitest (obsolete) — Splinter Review
Fix mochitest test_DurationChanged.html. Patch wasn't properly refreshed.
Attachment #8551525 - Flags: review?(karlt)
Attachment #8551509 - Attachment is obsolete: true
Attachment #8551509 - Flags: review?(karlt)
Attached patch Fix mochitest (obsolete) — Splinter Review
sorry for that, queue being messed up
Attachment #8551548 - Flags: review?(karlt)
Attachment #8551525 - Attachment is obsolete: true
Attachment #8551525 - Flags: review?(karlt)
Comment on attachment 8551548 [details] [diff] [review]
Fix mochitest

Yes, looking at http://www.w3.org/TR/2014/CR-media-source-20140717/#h4_sourcebuffer-coded-frame-processing duration should be queued after loadeddata.
Attachment #8551548 - Flags: review?(karlt) → review+
Attached patch Fix mochitestSplinter Review
As per IRC discussion, using sourceended event trigger, as we don't have to wait for the video to play
Attachment #8551548 - Attachment is obsolete: true
Priority: -- → P2
https://hg.mozilla.org/mozilla-central/rev/bddb69ed8cae
https://hg.mozilla.org/mozilla-central/rev/077140ba65d0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment on attachment 8551502 [details] [diff] [review]
Queue "durationchange" instead of dispatching synchronously

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent testing. Sites more likely to serve flash video.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: Change is straightforward but affects all media playback. This improves our spec compliance, but is a minor behaviour change from before.
[String/UUID change made/needed]: None.
Attachment #8551502 - Flags: approval-mozilla-beta?
Attachment #8551502 - Flags: approval-mozilla-aurora?
Approval request is for both patches. We need the test update as well.
Attachment #8551609 - Flags: approval-mozilla-beta+
Attachment #8551609 - Flags: approval-mozilla-aurora+
Attachment #8551502 - Flags: approval-mozilla-beta?
Attachment #8551502 - Flags: approval-mozilla-beta+
Attachment #8551502 - Flags: approval-mozilla-aurora?
Attachment #8551502 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.