469255 - Seeking while seek in progress not handled correctly by Wave decoder

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

A second seek while seeking can clobber the first seek.  Patch and tests coming up.

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: 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.

Comment 2

[Serge Gautherie (:sgautherie)]

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

Comment 3

[Matthew Gregan [:kinetik]]

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 <[e-mail]>
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.

Comment 4

[cajbir (:cajbir)]

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

[cajbir (:cajbir)]

BTW, the approach using 'patch' I used is bad, since it'll skip the binary files in that patch, oops.

Comment 6

[Matthew Gregan [:kinetik]]

Attachment: patch v1 [Checkin: Comment 8 & 9]

Rebased.

Comment 7

[Matthew Gregan [:kinetik]]

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

Actually just stupid!  Sigh.

Comment 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 352875 [details] [diff] [review]
patch v1
[Checkin: Comment 8 & 9]

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

Comment 9

[Serge Gautherie (:sgautherie)]

Comment on attachment 352875 [details] [diff] [review]
patch v1
[Checkin: Comment 8 & 9]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a6be5c92cb07

Comment 10

[Serge Gautherie (:sgautherie)]

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
}

Comment 11

[Matthew Gregan [:kinetik]]

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.

Comment 12

[Serge Gautherie (:sgautherie)]

(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.