Seeking while seek in progress not handled correctly by Wave decoder

RESOLVED FIXED in mozilla1.9.1b3

Status

()

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

People

(Reporter: kinetik, Assigned: kinetik)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.1b3
fixed1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 years ago
A second seek while seeking can clobber the first seek.  Patch and tests coming up.
Flags: blocking1.9.1?
(Assignee)

Comment 1

9 years ago
Created attachment 352667 [details] [diff] [review]
patch v0

Handle overlapping seeks by splitting state machine's time management into mTimeOffset and mSeekTime, rather than trying to share this variable for two purposes.

While here, also fix a bug handling truncated media files by changing the way duration is calculated.

Includes a pile of tests that cover these cases (and some other cases that already work but were untested).

Also includes two minor fixes (typo fixes) to the Ogg tests.
Attachment #352667 - Flags: superreview?(roc)
Attachment #352667 - Flags: review?(roc)
Attachment #352667 - Flags: superreview?(roc)
Attachment #352667 - Flags: superreview+
Attachment #352667 - Flags: review?(roc)
Attachment #352667 - Flags: review+
Keywords: checkin-needed
Whiteboard: [needs landing]
Flags: blocking1.9.1? → blocking1.9.1+
Comment on attachment 352667 [details] [diff] [review]
patch v0

patching file content/media/video/test/Makefile.in
Hunk #1 FAILED at 71
1 out of 1 hunks FAILED
Attachment #352667 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [needs landing]
(Assignee)

Comment 3

9 years ago
This applies cleanly to current trunk for me:

~/work/mozilla-central/mozilla% hg pull -u
pulling from https://hg.mozilla.org/mozilla-central
searching for changes
no changes found
~/work/mozilla-central/mozilla% hg tip
changeset:   22776:3633177885d6
tag:         tip
user:        Chris Pearce <chris@pearce.org.nz>
date:        Sun Dec 14 04:15:18 2008 +0100
summary:     Bug 469016 - Seeks after playback ended but before playback ended event are lost; r=chris.double sr=roc
~/work/mozilla-central/mozilla% hg qpush
applying bug469255
Now at: bug469255
~/work/mozilla-central/mozilla%

I'm confused.
Whiteboard: [needs landing]
(Assignee)

Updated

9 years ago
Attachment #352667 - Attachment is obsolete: false

Comment 4

9 years ago
Didn't apply cleanly here:

$ hg clone http://hg.mozilla.org/mozilla-central
$ cd mozilla-central
$ curl -k https://bugzilla.mozilla.org/attachment.cgi?id=352667 | patch -p1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 23442  100 23442    0     0  11254      0  0:00:02  0:00:02 --:--:-- 63874
patching file content/media/video/public/nsWaveDecoder.h
patching file content/media/video/src/nsWaveDecoder.cpp
patching file content/media/video/test/Makefile.in
Hunk #1 FAILED at 72.
1 out of 1 hunk FAILED -- saving rejects to file content/media/video/test/Makefile.in.rej
patching file content/media/video/test/test_seek1.html
patching file content/media/video/test/test_seek8.html
patching file content/media/video/test/test_wav_seek3.html
patching file content/media/video/test/test_wav_seek4.html
patching file content/media/video/test/test_wav_seek5.html
patching file content/media/video/test/test_wav_seek6.html
patching file content/media/video/test/test_wav_seek7.html
patching file content/media/video/test/test_wav_seek8.html
patching file content/media/video/test/test_wav_trailing.html
patching file content/media/video/test/test_wav_trunc.html
patching file content/media/video/test/test_wav_trunc_seek.htm

Comment 5

9 years ago
BTW, the approach using 'patch' I used is bad, since it'll skip the binary files in that patch, oops.
(Assignee)

Comment 6

9 years ago
Created attachment 352875 [details] [diff] [review]
patch v1
[Checkin: Comment 8 & 9]

Rebased.
Attachment #352667 - Attachment is obsolete: true
Attachment #352875 - Flags: superreview+
Attachment #352875 - Flags: review+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
(Assignee)

Comment 7

9 years ago
(In reply to comment #3)
> I'm confused.

Actually just stupid!  Sigh.
Comment on attachment 352875 [details] [diff] [review]
patch v1
[Checkin: Comment 8 & 9]

http://hg.mozilla.org/mozilla-central/rev/a993a51a8693
Attachment #352875 - Attachment description: patch v1 → patch v1 [Checkin: Comment 8]
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing] → [c-n: baking for 1.9.1][needs landing]
Comment on attachment 352875 [details] [diff] [review]
patch v1
[Checkin: Comment 8 & 9]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a6be5c92cb07
Attachment #352875 - Attachment description: patch v1 [Checkin: Comment 8] → patch v1 [Checkin: Comment 8 & 9]
Keywords: checkin-needed → fixed1.9.1
Whiteboard: [c-n: baking for 1.9.1][needs landing]
Target Milestone: mozilla1.9.1 → mozilla1.9.1b3
Fwiw, random:
{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1229291257.1229294575.17642.gz
WINNT 5.2 mozilla-central moz2-win32-slave08 dep unit test on 2008/12/14 13:47:37

*** 28186 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/video/test/test_wav_trunc.html | Checking currentTime at end: 0.25
}
(Assignee)

Comment 11

9 years ago
Yuck.  Because currentTime relies on wall-clock timing, it can be a little off based on system load, but the other tests all pass okay with the current thresholds.  This test uses a shorter file, so it might end up being more sensitive to timer slop.  I can change the test to use a longer file, or we can wait for the patch in bug 469266 to land, which avoids using the wall-clock and thus will be deterministic.

cpearce has found problems with some other tests that cause them to be unreliable (I'm surprised they haven't caused an orange yet), which is filed in bug 469595.  I'll post a patch over there to improve the tests.
(In reply to comment #11)
> I can change the test to use a longer file, or we can
> wait for the patch in bug 469266 to land, which avoids using the wall-clock and
> thus will be deterministic.

You confirmed the issue and bug 469644 was filed, that's all I was looking for.
Waiting (if not too long) looks fine to me.
Depends on: 469266, 469595, 469644
No longer depends on: 469266
Depends on: 474754
Depends on: 494769
You need to log in before you can comment on or make changes to this bug.