Closed Bug 467972 Opened 16 years ago Closed 16 years ago

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

Categories

(Core :: Audio/Video, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.1b3

People

(Reporter: cpearce, Assigned: cpearce)

References

Details

(Keywords: fixed1.9.1)

Attachments

(3 files, 2 obsolete files)

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.
Attached file content/media/video/test/320x240.ogg (obsolete) —
Uploading ogg file so that testcase works over web.
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.
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)
Attachment #351460 - Flags: review?(chris.double) → review+
Attachment #351460 - Flags: superreview?(roc) → superreview+
Depends on: 465498
Whiteboard: [c-n: waiting for bug 465498 first, correct !?]
No longer depends on: 465498
Whiteboard: [c-n: waiting for bug 465498 first, correct !?]
Whiteboard: needs-landing
This patch needs to be updated to trunk, there was a merge conflict.
Whiteboard: needs-landing → [needs 191 landing]
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]
As patch v1, but updated for trunk.
Attachment #351460 - Attachment is obsolete: true
Attachment #352185 - Flags: superreview+
Attachment #352185 - Flags: review+
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
Closed: 16 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]
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
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+
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]
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.

Attachment

General

Created:
Updated:
Size: