Load() invoked to do seek in onended results in sending onloadedmetadata

RESOLVED FIXED in mozilla1.9.1b3

Status

()

Core
Audio/Video
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.1b3
x86
All
fixed1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

9 years ago
If a video plays back to the end, it will seek to the start of the media, but we implement this by calling load(), which results in us sending onloadedmetadata (and maybe other events).

The spec: http://www.whatwg.org/specs/web-apps/current-work/multipage/video.html#dom-media-play says that we're (only) allowed to call load() if NETWORK_STATE is EMPTY, but the NETWORK_STATE is LOADED at this time. The seek spec doesn't mention load() at all.

So to comply with the spec, we need to suppress the loading events when doing a load() in order to do a seek.

---

A side effect of the above, is that if you have a video v which:

* Has an onloadedmetadata handler which calls v.play().
* Has an onended handler which seeks.

Then in onload it will start to play, and when it reaches end of playback it will seek to 0, but the onseeking and onseeked events for the seek are not dispatched to the media element. The problem is that the seek is calling load() to seek the resource. Load() is sending onloadedmetadata, and the JS handler is calling nsOggDecoder::Play() which ignores the fact that nsOggDecoder::mNextState==SEEKING, and changes state to PLAYING, and so the seek algorithm isn't invoked, and its events don't fire. If we decide not to suppress the load() events in seek in ended, then in nsOggDecoder::Play() we need to check for (mPlayState == PLAY_STATE_LOADING && mNextState == PLAY_STATE_SEEKING) in nsOggDecoder::Play() and set mNextState = PLAY_STATE_PLAYING and call ChangeState(PLAY_STATE_SEEKING) instead of running the default path.
(Assignee)

Comment 1

9 years ago
Created attachment 351443 [details]
content/media/video/test/320x240.ogg

Uploading ogg file so that testcase works over web.
(Assignee)

Comment 2

9 years ago
Created attachment 351445 [details]
Testcase - seek events not fired because of multiple onmetadataloaded events

This testcase shows that multiple onmetadataloaded events with a handler which calls play() can cause seek events to be lost when seeking in onended handler.
(Assignee)

Comment 3

9 years ago
Created attachment 351460 [details] [diff] [review]
Patch v1 - prevent multiple onmetadataloaded events

Changes nsOggDecoder to check that we're not servicing a seek before passing FirstFrameLoaded() and MetadataLoaded() up to the media element. Prevents multiple metadataloaded and loadedfirstframe events.

All existing video mochitests still pass on Vista with this patch.
Assignee: nobody → chris
Attachment #351460 - Flags: superreview?(roc)
Attachment #351460 - Flags: review?(chris.double)

Updated

9 years ago
Attachment #351460 - Flags: review?(chris.double) → review+
Attachment #351460 - Flags: superreview?(roc) → superreview+
Flags: blocking1.9.1+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Depends on: 465498
Whiteboard: [c-n: waiting for bug 465498 first, correct !?]
(Assignee)

Updated

9 years ago
No longer depends on: 465498
Whiteboard: [c-n: waiting for bug 465498 first, correct !?]
(Assignee)

Updated

9 years ago
Whiteboard: needs-landing
This patch needs to be updated to trunk, there was a merge conflict.
Whiteboard: needs-landing → [needs 191 landing]
(Assignee)

Comment 5

9 years ago
Comment on attachment 351443 [details]
content/media/video/test/320x240.ogg

Oops, bugzilla's attachment.cgi doesn't do byte range requests, so don't make testcases point to videos in bugzilla attachments, they won't seek correctly!
Attachment #351443 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [needs 191 landing] → [needs landing]
(Assignee)

Comment 6

9 years ago
Created attachment 352185 [details] [diff] [review]
Patch v1.1 - updated for trunk
[Checkin: Comment 7]

As patch v1, but updated for trunk.
Attachment #351460 - Attachment is obsolete: true
Attachment #352185 - Flags: superreview+
Attachment #352185 - Flags: review+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Comment on attachment 352185 [details] [diff] [review]
Patch v1.1 - updated for trunk
[Checkin: Comment 7]

http://hg.mozilla.org/mozilla-central/rev/6de3075c6522
Attachment #352185 - Attachment description: Patch v1.1 - updated for trunk → Patch v1.1 - updated for trunk [Checkin: Comment 7]
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing] → [c-n: baking for 1.9.1] [needs landing]
Target Milestone: --- → mozilla1.9.2a1
Comment on attachment 352185 [details] [diff] [review]
Patch v1.1 - updated for trunk
[Checkin: Comment 7]

Does not apply to 1.9.1:
{
patching file content/media/video/src/nsOggDecoder.cpp
Hunk #1 FAILED at 1380
1 out of 1 hunks FAILED
}
Keywords: checkin-needed
Whiteboard: [c-n: baking for 1.9.1] [needs landing]
(Assignee)

Comment 9

9 years ago
This needs bug 465832 to land on 1.9.1 first.
Keywords: checkin-needed
Whiteboard: [c-n: baking for 1.9.1] [needs landing]
We should not add checkin-needed until it's actually time to land the patch.
Keywords: checkin-needed
(Assignee)

Comment 11

9 years ago
Created attachment 354885 [details] [diff] [review]
Patch v1.1 unbitrotten for 1.9.1
[Checkin: Comment 12]

Same as Patch v1.1, but unbitrotten for 1.9.1 landing. Note this must land on 1.9.1 before 462570 can land on 1.9.1.
Attachment #354885 - Flags: superreview+
Attachment #354885 - Flags: review+
(Assignee)

Updated

9 years ago
Blocks: 462570
Keywords: checkin-needed
Comment on attachment 354885 [details] [diff] [review]
Patch v1.1 unbitrotten for 1.9.1
[Checkin: Comment 12]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/07c1761bf1ce
Attachment #354885 - Attachment description: Patch v1.1 unbitrotten for 1.9.1 → Patch v1.1 unbitrotten for 1.9.1 [Checkin: Comment 12]
Keywords: checkin-needed → fixed1.9.1
Whiteboard: [c-n: baking for 1.9.1] [needs landing]
Target Milestone: mozilla1.9.2a1 → mozilla1.9.1b3
You need to log in before you can comment on or make changes to this bug.