Closed Bug 465498 Opened 16 years ago Closed 16 years ago

HTML5 <audio>: setting 'currentTime' throws exception sometimes

Categories

(Core :: Audio/Video, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9.1b3

People

(Reporter: walther, Assigned: kinetik)

References

Details

(Keywords: verified1.9.1)

Attachments

(1 file, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US) AppleWebKit/525.19 (KHTML, like Gecko) Chrome/0.3.154.9 Safari/525.19
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081114 Minefield/3.1b2pre

Using this HTML snippet throws JS exception after mousing over link a few times:

<audio id="sound1">
  <source src="hello.wav" type="audio/x-wav"></source>
</audio>
<a onmouseover="document.getElementById('sound1').currentTime=0.297984;document.getElementById('sound1').play();" onmouseout="document.getElementById('sound1').currentTime=0;document.getElementById('sound1').pause();">Mouse over me to start sound</a>


Reproducible: Always

Steps to Reproduce:
1. Run above HTML snippet
2. Move mouse over link a few times
3. Observe exception in error console
Actual Results:  
Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMHTMLAudioElement.currentTime]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///C:/develop/WebBased/SoundPlayback/playSoundMinefield.html :: onmouseover :: line 1"  data: no]

Expected Results:  
No exception, audio playback invariably resumes at 'currentTime'.
Component: General → Video/Audio
Product: Firefox → Core
QA Contact: general → video.audio
Version: unspecified → Trunk
This happens when the Wave file ends playback.  After this happens, the nsWaveStateMachine is destroyed, and is only instantiated again on play(), so an attempt to set currentTime after end results in an exception.
Assignee: nobody → kinetik
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch patch v0 [Backout: Comment 4] (obsolete) — Splinter Review
Start a new load in Seek() if there is no current playback state machine.  Move mTimeOffset sanitization into the code that does the seek, since we might not have finished loading metadata when seek is called.  Includes a testcases that passes all current tests, plus a bunch of new ones I'm in the process of submitting via other bugs.

While I'm here, include a fix (and testcase) for bug 467972, since fixing that depends on fixing this anyway.
Attachment #351494 - Flags: superreview?(roc)
Attachment #351494 - Flags: review?(roc)
Attachment #351494 - Flags: superreview?(roc)
Attachment #351494 - Flags: superreview+
Attachment #351494 - Flags: review?(roc)
Attachment #351494 - Flags: review+
Keywords: checkin-needed
Comment on attachment 351494 [details] [diff] [review]
patch v0
[Backout: Comment 4]

http://hg.mozilla.org/mozilla-central/rev/cb9da8789fce

after fixing context for
{
patching file content/media/video/test/Makefile.in
Hunk #1 FAILED at 45
1 out of 2 hunks FAILED
}
Attachment #351494 - Attachment description: patch v0 → patch v0 [Checkin: Comment 3]
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [c-n: baking for 1.9.1]
Target Milestone: --- → mozilla1.9.2a1
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
Whiteboard: [c-n: baking for 1.9.1]
Comment on attachment 351494 [details] [diff] [review]
patch v0
[Backout: Comment 4]

Backed out:
http://hg.mozilla.org/mozilla-central/rev/e8a055de467c
http://hg.mozilla.org/mozilla-central/rev/7864ddfabf4a
as test_wav_onloadedmetadata.html is failing.

See for example:
{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1228493554.1228496655.17550.gz
Linux mozilla-central moz2-linux-slave08 dep unit test on 2008/12/05 08:12:34

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1228493734.1228498132.21564.gz
MacOSX Darwin 9.2.2 mozilla-central moz2-darwin8-slave01 dep unit test on 2008/12/05 08:15:34

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1228494161.1228497666.20719.gz
WINNT 5.2 mozilla-central moz2-win32-slave07 dep unit test on 2008/12/05 08:22:41
}
Attachment #351494 - Attachment description: patch v0 [Checkin: Comment 3] → patch v0 [Backout: Comment 4]
Attached patch patch v1 (obsolete) — Splinter Review
Fix test failures.  I wrote the test on top of the media decoder selection patch I'm working on, and forgot about the fact that it wouldn't work without that patch applied.  Change the test to use the already supported <source src= type=> manual decoder selection for now.

Trivial change, so no review necessary.  I'm holding off on setting checkin-needed because I saw test_bug465498.html random fail on my machine, so I'm trying to track that down now.
Attachment #351494 - Attachment is obsolete: true
The random failure is a timing issue.  test_bug465498.html checks !v.ended in the seeked event handler after seeking to the start of the media in the ended handler.  nsHTMLMediaElement's mEnded flag can still be true depending on timing, because in this situation it would only be reset when the channel called the ResourceLoaded callback.

The spec doesn't explicitly say that ended should be reset, but the synchronous part of the seek sets the new currentTime, and ended can only be true when the currentTime is at the end of the media.

I'll file a new bug and work on a patch.  This bug will have to depend on the new one.
Depends on: 468190
Attached patch patch v2 (obsolete) — Splinter Review
Trivial change from patch v1, this adds an additional check for !v.ended immediately after seeking in test_bug465498.html to make the test fail fast while bug 468190 is still present.
Attachment #351621 - Attachment is obsolete: true
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.
Attachment #351824 - Attachment is obsolete: true
Blocks: 469268
Comment on attachment 352675 [details] [diff] [review]
patch v3
[Checkin: Comment 13 & 14]

Carrying forward review.
Attachment #352675 - Flags: superreview+
Attachment #352675 - Flags: review+
No longer on depends bug 468190 because the tests have been split off in to bug 469268.
No longer depends on: 468190
Keywords: checkin-needed
Comment on attachment 352675 [details] [diff] [review]
patch v3
[Checkin: Comment 13 & 14]

patching file content/media/video/src/nsWaveDecoder.cpp
Hunk #1 FAILED at 389
Hunk #3 FAILED at 630
Hunk #4 FAILED at 1006
3 out of 4 hunks FAILED
Attachment #352675 - Attachment is obsolete: true
Status: REOPENED → NEW
Flags: in-testsuite+ → in-testsuite-
Keywords: checkin-needed
Whiteboard: [needs landing]
This needs to be applied on top of bug 468190.
Keywords: checkin-needed
Whiteboard: [needs landing]
Attachment #352675 - Attachment is obsolete: false
Status: NEW → ASSIGNED
Whiteboard: [needs landing] → [needs landing] [patch depends on bug 468190]
Attachment #352675 - Attachment description: patch v3 → patch v3 [Checkin: Comment 13]
Status: ASSIGNED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] [patch depends on bug 468190] → [c-n: baking for 1.9.1][needs landing]
Attachment #352675 - Attachment description: patch v3 [Checkin: Comment 13] → patch v3 [Checkin: Comment 13 & 14]
Whiteboard: [c-n: baking for 1.9.1][needs landing]
Target Milestone: mozilla1.9.2a1 → mozilla1.9.1b3
Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US;
rv:1.9.2a1pre) Gecko/20090420 Minefield/3.6a1pre
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre)
Gecko/20090420 Shiretoko/3.5b4pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: