liboggz seeking bisection is inaccurate

RESOLVED FIXED in mozilla1.9.3a1

Status

()

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

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

({testcase, verified1.9.2})

Trunk
mozilla1.9.3a1
testcase, verified1.9.2
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.2 +
in-testsuite -

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

8 years ago
The liboggz seeking is inaccurate, and causes us to do many more HTTP requests than is really necessary. This makes seeking slow.
(Assignee)

Comment 1

8 years ago
Created attachment 385670 [details]
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.
(Assignee)

Comment 2

8 years ago
Created attachment 385678 [details] [diff] [review]
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? :)
Attachment #385678 - Flags: superreview?(chris.double)
Attachment #385678 - Flags: review?(gmaxwell)
(Assignee)

Comment 3

8 years ago
Oh yeah, mochitests pass on Windows with patch v1.
(Assignee)

Comment 4

8 years ago
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.
(Assignee)

Updated

8 years ago
Duplicate of this bug: 495640
(Assignee)

Updated

8 years ago
Attachment #385678 - Flags: superreview?(chris.double)
Attachment #385678 - Flags: review?(gmaxwell)
Attachment #385678 - Flags: review?(chris.double)
(Assignee)

Comment 6

8 years ago
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.

Updated

8 years ago
Attachment #385678 - Flags: review?(chris.double) → review+
(Assignee)

Comment 7

8 years ago
Created attachment 399121 [details] [diff] [review]
Patch - unbitrotten

Patch unbitrotten, r=doublec.
Attachment #385678 - Attachment is obsolete: true
Attachment #399121 - Flags: review+
(Assignee)

Comment 8

8 years ago
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...
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Flags: wanted1.9.2?
Resolution: --- → FIXED
You'll need to request patch approval
Flags: wanted1.9.2? → wanted1.9.2+
(Assignee)

Comment 10

8 years ago
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+
(Assignee)

Comment 11

8 years ago
Pushed to 1.9.2:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7d7977dbd247
status1.9.2: --- → beta1-fixed
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
Status: RESOLVED → VERIFIED
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.
Status: VERIFIED → RESOLVED
Last Resolved: 8 years ago8 years ago
Flags: in-testsuite?
Keywords: testcase
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.3a1
Depends on: 518125
Depends on: 518169
(Assignee)

Comment 15

8 years ago
(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.