501031 - liboggz seeking bisection is inaccurate

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

The liboggz seeking is inaccurate, and causes us to do many more HTTP requests than is really necessary. This makes seeking slow.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Testcase - Measure seek time

Testcase - measures seek times and averages them over a large number of runs. By default it seeks on a Tinyvid.tv 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.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1 - bisect with fuzz factor

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? :)

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Oh yeah, mochitests pass on Windows with patch v1.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch - unbitrotten

Patch unbitrotten, r=doublec.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

http://hg.mozilla.org/mozilla-central/rev/06cf7d35dbcd

Do we want to take this on 1.9.2? It speeds up <video> seeking over HTTP significantly...

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You'll need to request patch approval

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

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?

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Pushed to 1.9.2:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7d7977dbd247

Comment 12

[Tony Chung [:tchung]]

awesome work Chris.

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.4pre) 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

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This patch has been caused bug 518125.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Tony, have you used a Minefield build to verify it on trunk too?

We need a test here.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.