Closed Bug 468190 Opened 13 years ago Closed 13 years ago

ended reports true when seeking after playback ended

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.1b3

People

(Reporter: kinetik, Assigned: kinetik)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 3 obsolete files)

It's possible for ended to still be true in the seeked event handler after seeking to 0 in the ended event handler.  In this case, mEnded in nsHTMLMediaElement can only be reset to false in ResourceLoaded, which may not run soon enough.

According to the spec, ended should be false if currentTime is not the end of the media.  The seek algorithm changes currentTime during the synchronous part of the algorithm, so the following should be true (but may not be with the current code):

function ended() {
  is(v.ended == true);
  v.currentTime = 0;
  is(v.ended == false);
}
Flags: blocking1.9.1?
Blocks: 465498
Similar problem with v.seeking, according to my reading of the spec this should be true immediately after the setting of currentTime returns.  The current implementation only returns true once the decoder has transitioned to the seeking state, which may happen some time later.
Attached patch patch v1 (obsolete) — Splinter Review
Drop the mEnded flag in the media element, and instead ask the decoder if playback has ended.

Change IsSeeking() to report if a seek has been requested by also checking the next state of the decoder.

Also silence a warning in the Wave decoder where an event may be null when seeking after playback end.

Unfortunately the Wave test for this bug depends on yet another bugfix which is at the other end of my patch queue.  I'm in the process of untangling that from a big ball of fixes and will file a new bug with patch for that and link up the dependencies appropriately.
Attachment #351842 - Flags: superreview?(roc)
Attachment #351842 - Flags: review?(chris.double)
Attachment #351842 - Flags: superreview?(roc) → superreview+
Flags: blocking1.9.1? → blocking1.9.1+
Attachment #351842 - Flags: review?(chris.double) → review+
Attached patch patch v2 (obsolete) — Splinter Review
Same patch, but removes the test because it depends on another bug being fixed.
 I have a bunch of patches like this, where the testcase tests the patch but
breaks because of some other bug.  I'm going to open a new bug and attached a
patch to that included all of the testcases, and that can land on top of the
stack of bugfixes.

SR carried forward from roc.
Attachment #351842 - Attachment is obsolete: true
Attachment #352674 - Flags: superreview+
Attachment #352674 - Flags: review?(chris.double)
Blocks: 469268
Comment on attachment 352674 [details] [diff] [review]
patch v2

+  // Call on the mai thread only.

'main' typo. r+ with that fixed.
Attachment #352674 - Flags: review?(chris.double) → review+
Attached patch patch v2.1 (obsolete) — Splinter Review
Make patch less kawaii.  R+SR carried forward.
Attachment #352674 - Attachment is obsolete: true
Attachment #352683 - Flags: superreview+
Attachment #352683 - Flags: review+
Keywords: checkin-needed
No longer blocks: 465498
This needs to be applied on top of bug 469255.
Rebased.
Attachment #352683 - Attachment is obsolete: true
Attachment #352876 - Flags: superreview+
Attachment #352876 - Flags: review+
Whiteboard: [needs landing] → [needs landing] [patch depends on bug 469255]
Comment on attachment 352876 [details] [diff] [review]
patch v2.2
[Checkin: Comment 8 & 9]

http://hg.mozilla.org/mozilla-central/rev/3d8e98242012
Attachment #352876 - Attachment description: patch v2.2 → patch v2.2 [Checkin: Comment 8]
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] [patch depends on bug 469255] → [c-n: baking for 1.9.1][needs landing]
Comment on attachment 352876 [details] [diff] [review]
patch v2.2
[Checkin: Comment 8 & 9]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1fcfd425228f
Attachment #352876 - Attachment description: patch v2.2 [Checkin: Comment 8] → patch v2.2 [Checkin: Comment 8 & 9]
Whiteboard: [c-n: baking for 1.9.1][needs landing]
Target Milestone: mozilla1.9.1 → mozilla1.9.1b3
Depends on: 496697
Depends on: 493647
No longer depends on: 496697
You need to log in before you can comment on or make changes to this bug.