Closed Bug 501031 Opened 15 years ago Closed 15 years ago

liboggz seeking bisection is inaccurate


(Core :: Audio/Video, defect)

Not set



Tracking Status
status1.9.2 --- beta1-fixed


(Reporter: cpearce, Assigned: cpearce)



(Keywords: testcase, verified1.9.2)


(2 files, 1 obsolete file)

The liboggz seeking is inaccurate, and causes us to do many more HTTP requests than is really necessary. This makes seeking slow.
Testcase - measures seek times and averages them over a large number of runs. By default it seeks on a video, but you can configure it to run off a local server as well. You should reduce your media cache size when running this test, else your results will be inaccurate when seeking in cached ranges.
Speeds up liboggz seeking to (before) keyframe:
* Adds margin_fuzz to seeking, so the seek bisection now stops when the seek cursor is within margin_fuzz ms of the target. This means we don't bounce around close to the target, causing new HTTP requests to fire off.
* When we seek to target, we actually seek to max(0, target - max_frames_to_prev_keyframe * frame_duration - fuzz), and then decode forward to the last frame before the target. This means we're always going to seek encounter the keyframe when decoding forward to the target. No more visual artifacts!
* I fixed the seek_guess (bisection) so that it doesn't integer-overflow, and so that it actually makes sensible guesses.
* We were incorrectly calculating the time of an offset-range for media which didn't start at time 0s. We need to skip header pages when finding the start time of media.

Greg: Would you be able to look over my changes to liboggz and make sure it's all sensible? :)
Attachment #385678 - Flags: superreview?(chris.double)
Attachment #385678 - Flags: review?(gmaxwell)
Oh yeah, mochitests pass on Windows with patch v1.
I should also say that Patch v1 roughly cuts seeking time over HTTP in half. We're looking at about 9s on average for a seek on (when viewing from New Zealand anyway). Roughly half of that time is actually spent doing the bisection search, the other half is usually spent decoding forward from the seeked-to position until the target time. This decode-forward time is limited by the speed at which the data after the seeked-to position up to the target can be downloaded.

To speed this up further, we'd need to more accurately determine where the previous Theora keyframe is, and not set the seeked-to position before that.
Attachment #385678 - Flags: superreview?(chris.double)
Attachment #385678 - Flags: review?(gmaxwell)
Attachment #385678 - Flags: review?(chris.double)
Comment on attachment 385678 [details] [diff] [review]
Patch v1 - bisect with fuzz factor

Chris Double: can you review this patch please? I've been running this patch in my own debug builds for weeks now, and it's solid, roughly cuts seeking over HTTP time in half.
Attachment #385678 - Flags: review?(chris.double) → review+
Patch unbitrotten, r=doublec.
Attachment #385678 - Attachment is obsolete: true
Attachment #399121 - Flags: review+

Do we want to take this on 1.9.2? It speeds up <video> seeking over HTTP significantly...
Closed: 15 years ago
Flags: wanted1.9.2?
Resolution: --- → FIXED
You'll need to request patch approval
Flags: wanted1.9.2? → wanted1.9.2+
Comment on attachment 399121 [details] [diff] [review]
Patch - unbitrotten

Can I have approval for this on 1.9.2? It cuts video seeking time in half?
Attachment #399121 - Flags: approval1.9.2?
Attachment #399121 - Flags: approval1.9.2? → approval1.9.2+
awesome work Chris.

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv: Gecko/20090917 Shiretoko/3.5.4pre
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a2pre) Gecko/20090917 Namoroka/3.6a2pre
Keywords: verified1.9.2
This patch has been caused bug 518125.
Tony, have you used a Minefield build to verify it on trunk too?

We need a test here.
Closed: 15 years ago15 years ago
Flags: in-testsuite?
Keywords: testcase
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.3a1
(In reply to comment #14)
> We need a test here.

I don't think we can test this change independently; I changed the fundamental seek implementation. If it's broken, seek won't work. If it's not, the existing seek tests will pass. If a regression is found (like bug 518169/518125) we should definitely add a test case for that specific case, but I'd say we're covered here by the existing seek tests.
Flags: in-testsuite? → in-testsuite-
Chris, that means that all existing seek tests also use currentTime for seeking and I just hit a special case which hasn't been covered yet by those tests? I just wondered that the native video controls don't show the problem. Using the scrubber directly doesn't show the seeking problems for me. Thanks for the clarification.
You need to log in before you can comment on or make changes to this bug.