Last Comment Bug 778077 - Implement fastSeek API on media elements (and switch the built-in controls over to it)
: Implement fastSeek API on media elements (and switch the built-in controls ov...
Status: RESOLVED FIXED
[ucid:multimedia13,2.0,ft:multimeida-...
: dev-doc-complete
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla31
Assigned To: Chris Pearce (:cpearce)
:
:
Mentors:
https://www.w3.org/Bugs/Public/show_b...
: 847375 (view as bug list)
Depends on: 1026330 882718 1023771 1046003
Blocks: 2.0-multimedia 982695 984576
  Show dependency treegraph
 
Reported: 2012-07-27 02:29 PDT by Matthew Gregan [:kinetik]
Modified: 2015-07-17 03:01 PDT (History)
24 users (show)
anthony.s.hughes: in‑qa‑testsuite?
jsmith: in‑moztrap-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
2.0
fixed


Attachments
WIP patch (65.12 KB, patch)
2012-09-18 22:32 PDT, Reuben Morais [:reuben]
no flags Details | Diff | Splinter Review
Patch v1: Implement HTMLMediaElement.fastSeek() (629.54 KB, patch)
2014-03-25 20:44 PDT, Chris Pearce (:cpearce)
cajbir.bugzilla: review+
Details | Diff | Splinter Review
Patch 2: MediaOMXReader seek fix (1.81 KB, patch)
2014-03-31 17:31 PDT, Chris Pearce (:cpearce)
cajbir.bugzilla: review+
Details | Diff | Splinter Review

Description Matthew Gregan [:kinetik] 2012-07-27 02:29:02 PDT
I can't find a bug for this, despite being fairly sure we had one.

Implement the fastSeek API for media elements being proposed here: https://www.w3.org/Bugs/Public/show_bug.cgi?id=14851, specifically Hixie's proposal in comment 9.
Comment 1 Reuben Morais [:reuben] 2012-09-06 16:47:35 PDT
We currently use fast seek for WebM and accurate seek for all other formats.
Should elem.mozFastSeek() fail as not implemented or simply perform an accurate seek in a media format where faster seek can't be done (like raw)?
Comment 2 Matthew Gregan [:kinetik] 2012-09-06 17:08:41 PDT
(In reply to Reuben Morais [:reuben] from comment #1)
> We currently use fast seek for WebM and accurate seek for all other formats.

That's not correct, WebM seeking is frame and sample accurate.  It does use an index if one is available in the file, but so does Ogg if an index is present.  Most of the actual seeking logic is shared between all decoder implementations.

Sadly, doing "fast" seeking in WebM without an index is fairly painful, since it doesn't provide the same resync guarantees that Ogg does.

> Should elem.mozFastSeek() fail as not implemented or simply perform an
> accurate seek in a media format where faster seek can't be done (like raw)?

Accurate seeking is usually implemented by seeking to the nearest preceding resync point and then decoding up to the exact position requested.  It's hard to imagine a format where accurate seeking is possible but approximate seeking is not.  Why don't you think it's possible for raw?
Comment 3 Reuben Morais [:reuben] 2012-09-06 18:03:51 PDT
(In reply to Matthew Gregan [:kinetik] from comment #2)
> (In reply to Reuben Morais [:reuben] from comment #1)
> > We currently use fast seek for WebM and accurate seek for all other formats.
> 
> That's not correct, WebM seeking is frame and sample accurate.  It does use
> an index if one is available in the file, but so does Ogg if an index is
> present.  Most of the actual seeking logic is shared between all decoder
> implementations.

Hm, the comment at https://mxr.mozilla.org/mozilla-central/source/media/libnestegg/include/nestegg.h#183 made me think the algorithm would always seek to the nearest key frame.

> > Should elem.mozFastSeek() fail as not implemented or simply perform an
> > accurate seek in a media format where faster seek can't be done (like raw)?
> 
> Accurate seeking is usually implemented by seeking to the nearest preceding
> resync point and then decoding up to the exact position requested.  It's
> hard to imagine a format where accurate seeking is possible but approximate
> seeking is not.  Why don't you think it's possible for raw?

I had the impression raw videos had no interframe compression so every frame was a keyframe, making approximate seeking just as costly as accurate seeking.
Comment 4 Matthew Gregan [:kinetik] 2012-09-06 18:31:20 PDT
(In reply to Reuben Morais [:reuben] from comment #3)
> Hm, the comment at
> https://mxr.mozilla.org/mozilla-central/source/media/libnestegg/include/
> nestegg.h#183 made me think the algorithm would always seek to the nearest
> key frame.

Look at nsWebMReader::Seek and nsBuiltinDecoderReader::DecodeToTarget.

> I had the impression raw videos had no interframe compression so every frame
> was a keyframe, making approximate seeking just as costly as accurate
> seeking.

More resync points make seeking easier, because there's less to decode before arriving at the desired timecode.
Comment 5 Reuben Morais [:reuben] 2012-09-06 18:58:06 PDT
(In reply to Matthew Gregan [:kinetik] from comment #4)
> nsBuiltinDecoderReader::DecodeToTarget.

A-ha! There it is, I knew I was missing something in this complex class hierarchy. So, assuming all readers have the ability to seek to the nearest key frame, essentially all you'd need to do is skip the DecodeToTarget call and adjust mCurrentTime appropriately on the media element?
Comment 6 Reuben Morais [:reuben] 2012-09-18 22:32:29 PDT
Created attachment 662418 [details] [diff] [review]
WIP patch

I've been working on this bug. Attached is a work in progress combined patch.
Todo, mostly as a note to myself:
- Enforce the invariants mentioned in https://www.w3.org/Bugs/Public/show_bug.cgi?id=14851
- Ogg backend seeks to offset in bytes, needs extra logic to make sure we're in a keyframe.
- Make sure currentTime, seeking, etc are all updated accordingly on the media element during a fast seek
- Test other backends extensively
Comment 7 Reuben Morais [:reuben] 2012-10-06 19:14:30 PDT
Sorry guys, college is consuming 25 hours of my days so I can't work on this right now, feel free to take it.
Comment 8 Paul Adenot (:padenot) 2013-03-11 06:01:34 PDT
*** Bug 847375 has been marked as a duplicate of this bug. ***
Comment 9 Chris Pearce (:cpearce) 2014-03-18 15:30:03 PDT
I'll do this. We want this to speed up thumbnailing on B2G, specifically on low end devices like the Tarako.
Comment 10 Ivan Tsay (:ITsay) 2014-03-24 23:01:00 PDT
Block Firefox OS multimedia platform 1.5 feature meta bug
Comment 11 Chris Pearce (:cpearce) 2014-03-25 20:44:12 PDT
Created attachment 8396889 [details] [diff] [review]
Patch v1: Implement HTMLMediaElement.fastSeek()

* Implement HTMLMediaElement.fastSeek(), basically by changing all the MediaDecoderReader::Seek() overrides to not call MediaDecoderReader::DecodeToTarget(), and have MediaDecoderReader::DecodeSeek() call DecodeToTarget() if we're doing an accurate (non-fast) seek.
* Update gizmo.mp4 to have a keyframe every second, instead of only 1 keyframe at the start of stream. This makes the unit test I added more useful for mp4...
* I pushed most of the seek target clamping logic in MediaDecoder up into HTMLMediaElement, so that we're clamping in fewer places. Note MediaDecoderStateMachine::Seek() still sanity checks the seek target.
* We have to update the currentTime/MediaDecoder playback position after a seek completes now, rather than assuming the seek always got it exactly right.
* Removed those pesky assertions about seek target lying in the first frame after seek, since actually sometimes the media doesn't have samples for all streams after a seek (either due to the media being encoded like that, or because of a bug in the platform's decoder, not entirely sure).
* Green: https://tbpl.mozilla.org/?tree=Try&rev=b028258565e2
Comment 12 cajbir (:cajbir) 2014-03-27 18:50:15 PDT
Comment on attachment 8396889 [details] [diff] [review]
Patch v1: Implement HTMLMediaElement.fastSeek()

Review of attachment 8396889 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with changes below.

::: content/html/content/public/HTMLMediaElement.h
@@ +35,5 @@
>  class ErrorResult;
>  class MediaResource;
>  class MediaDecoder;
>  class VideoFrameContainer;
> +class SeekTarget;

Why is this needed? I don't see SeekTarget used - although perhaps it should be to avoid the boolean parameter added to Seek.

@@ +875,5 @@
>    void PopulatePendingTextTrackList();
>  
> +  // Seeks to aTime seconds. If aDoFastSeek is true, this instead seeks to
> +  // the preceeding sync point, or thereabouts.
> +  void Seek(double aTime, bool aDoFastSeek, ErrorResult& aRv);

I'm not a fan of introducing boolean parameters. I'd prefer a SeekType enum with values like Exact and EarliestKeyframe (assuming that's what fastseek is). Then have this function take that as an argument.

::: content/html/content/src/HTMLMediaElement.cpp
@@ +1336,5 @@
> +}
> +
> +/**
> + * Returns true if aValue is inside a range of aRanges, and put the range
> + * index in aIntervalIndex if it is not null.

If what is not null? aIntervalIndex is a reference and can't be null.

@@ +1338,5 @@
> +/**
> + * Returns true if aValue is inside a range of aRanges, and put the range
> + * index in aIntervalIndex if it is not null.
> + * If aValue is not inside a range, false is returned, and aIntervalIndex, if
> + * not null, is set to the index of the range which ends immediately before aValue

Same here re 'not null'

@@ +1345,5 @@
> +static bool
> +IsInRanges(dom::TimeRanges& aRanges, double aValue, int32_t& aIntervalIndex)
> +{
> +  uint32_t length;
> +  aRanges.GetLength(&length);

aRanges.GetLength is defined to return an nsresult. I know that the implementation always returns NS_OK but if that's ever changed then this will silently pass and 'length' will be used uninitialized further on. I suggest either (a) check the return value and do something, or (b) initialize 'length' to zero so failure at least won't have undefined results. I'd go for (b).

@@ +1348,5 @@
> +  uint32_t length;
> +  aRanges.GetLength(&length);
> +  for (uint32_t i = 0; i < length; i++) {
> +    double start, end;
> +    aRanges.Start(i, &start);

Same here regarding initialization and error result. In this case however Start() can actually fail so it is possibly an issue.

@@ +1353,5 @@
> +    if (start > aValue) {
> +      aIntervalIndex = i - 1;
> +      return false;
> +    }
> +    aRanges.End(i, &end);

Check error result or intialize 'end'.

::: content/media/MediaDecoder.h
@@ +344,5 @@
>  
>    // Seek to the time position in (seconds) from the start of the video.
> +  // If aDoFastSeek is true, we'll seek to the sync point/keyframe preceeding
> +  // the seek target.
> +  virtual nsresult Seek(double aTime, bool aDoFastSeek);

Don't use boolean parameter.

::: content/media/test/test_fastSeek.html
@@ +24,5 @@
> +      v.fastSeek(v.target);
> +      ok(Math.abs(v.currentTime - v.target) < 0.01,
> +         v.name + " seekTo=" + v.target + " currentTime (" + v.currentTime + ") should be close to seek target initially");
> +    }
> +    

trailing white space here and in other places in the file.
Comment 13 Chris Pearce (:cpearce) 2014-03-27 19:23:51 PDT
Comment on attachment 8396889 [details] [diff] [review]
Patch v1: Implement HTMLMediaElement.fastSeek()

Review of attachment 8396889 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the review!

::: content/html/content/public/HTMLMediaElement.h
@@ +875,5 @@
>    void PopulatePendingTextTrackList();
>  
> +  // Seeks to aTime seconds. If aDoFastSeek is true, this instead seeks to
> +  // the preceeding sync point, or thereabouts.
> +  void Seek(double aTime, bool aDoFastSeek, ErrorResult& aRv);

OK, I'll #include "MediaDecoder.h" in HTMLMediaElement.h and use SeekTarget::Type.
Comment 14 Chris Pearce (:cpearce) 2014-03-27 19:55:14 PDT
Landed for Gecko 31:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25b633e7dcd5
Comment 15 Phil Ringnalda (:philor) 2014-03-27 21:54:16 PDT
Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/9b4d9313e348 since I didn't know whether it was this, bug 6331058, or both, causing Mac debug mochitest failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=36849618&tree=Mozilla-Inbound, Windows mochitest failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=36850811&tree=Mozilla-Inbound, Android like https://tbpl.mozilla.org/php/getParsedLog.php?id=36850825&tree=Mozilla-Inbound, and Windows reftest failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=36850065&tree=Mozilla-Inbound. So far.
Comment 16 Chris Pearce (:cpearce) 2014-03-28 00:30:53 PDT
bug 6331058 is to blame, here's a try push without bug 6331058, and it's green:
https://tbpl.mozilla.org/?tree=Try&rev=46f7f3566c8d
Comment 17 Chris Pearce (:cpearce) 2014-03-28 02:40:06 PDT
Once more, landed for Gecko 31, without bug 631058.
https://hg.mozilla.org/integration/mozilla-inbound/rev/00fa39c23b44
Comment 19 Carsten Book [:Tomcat] 2014-03-28 06:25:26 PDT
(In reply to Ed Morley [:edmorley UTC+0] from comment #18)
> Backed out for reftest failures:
> https://tbpl.mozilla.org/?tree=Mozilla-
> Inbound&rev=00fa39c23b44&jobname=reftest
> 
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/525d4e2eab84

seems https://tbpl.mozilla.org/php/getParsedLog.php?id=36868539&tree=Mozilla-Inbound is also a related failure
Comment 20 Chris Pearce (:cpearce) 2014-03-31 17:29:52 PDT
(In reply to Ed Morley [:edmorley UTC+0] from comment #18)
> Backed out for reftest failures:
> https://tbpl.mozilla.org/?tree=Mozilla-
> Inbound&rev=00fa39c23b44&jobname=reftest
> 
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/525d4e2eab84

This is due to the logic changes I made to DecodeToTarget() while I was adding logging to it. I will revert the logic changes I made, but keep the additional logging.


(In reply to Carsten Book [:Tomcat] from comment #19)
> (In reply to Ed Morley [:edmorley UTC+0] from comment #18)
> > Backed out for reftest failures:
> > https://tbpl.mozilla.org/?tree=Mozilla-
> > Inbound&rev=00fa39c23b44&jobname=reftest
> > 
> > remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/525d4e2eab84
> 
> seems
> https://tbpl.mozilla.org/php/getParsedLog.php?id=36868539&tree=Mozilla-
> Inbound is also a related failure

This is caused by MediaOMXReader demuxing and seeking audio and video streams separately. When doing fastSeek the MediaOMXReader seeks both audio and video to the "key frame" preceeding the seek target. But because typically audio has many more sync points than video, the audio stream can be seeked to much closer to the seek target, and because we use the audio to drive the clock when we have both audio and video streams, we'll set the current time to the timestamp of the audio stream after the seek, which is not near the video stream's keyframe, so the test is failing.

We can fix this by changing the MediaOMXReader to seek the video stream first, getting the timestamp of the first video sample after the seek, and then seeking the audio stream to that timestamp.
Comment 21 Chris Pearce (:cpearce) 2014-03-31 17:31:11 PDT
Created attachment 8399725 [details] [diff] [review]
Patch 2: MediaOMXReader seek fix

This applies on top of the previous. Fixes failures are per previous comment.

https://tbpl.mozilla.org/?tree=Try&rev=63b5ecb927eb
Comment 22 Chris Pearce (:cpearce) 2014-03-31 20:41:06 PDT
Landed rollup of two patches above for Gecko 31:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c0afbe41ce8
Comment 23 Ryan VanderMeulen [:RyanVM] 2014-04-01 13:57:05 PDT
https://hg.mozilla.org/mozilla-central/rev/9c0afbe41ce8
Comment 24 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2014-04-07 16:25:06 PDT
Does this need any QA testing before this code is released, perhaps some video control regression testing?
Comment 25 Chris Pearce (:cpearce) 2014-04-08 13:07:43 PDT
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #24)
> Does this need any QA testing before this code is released, perhaps some
> video control regression testing?

Yes please! Currently this is enabled in Fennec Nightly builds in the built-in video controls. You can test by just seeking with the built in controls.

I still need to create a Gaia bug to get this used for seeking in the Video App in Firefox OS, but seeking in the Firefox OS Browser App should be using this, and should be faster.
Comment 26 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2014-04-08 13:43:31 PDT
Does this only apply to mobile builds? Is desktop impacted?
Comment 27 Chris Pearce (:cpearce) 2014-04-08 17:18:29 PDT
The feature ships in desktop builds, but the video controls don't use it on desktop. The videocontrols only use fastSeek when operating in "touch" mode, i.e. on mobile.
Comment 28 Paul Yang [:pyang] (away 9/22-9/26) 2014-04-15 01:29:53 PDT
For b2g, do we have STR or pass criteria for this bug?
Comment 29 Chris Pearce (:cpearce) 2014-04-15 14:45:18 PDT
(In reply to Paul Yang [: pyang] from comment #28)
> For b2g, do we have STR or pass criteria for this bug?

Yes. To verify that this works correctly, you test that seeking in an HTML5 <video> works using the built in controls in the B2G Browser App.

i.e.
1. Open http://pearce.org.nz/h in B2G Browser App.
2. Seek the video on that page using the controls displayed on the video

If that works, it's verified.

Note that the Gaia Video App doesn't yet use fastSeek, that's being worked on in bug 996398.
Comment 30 Paul Yang [:pyang] (away 9/22-9/26) 2014-04-15 21:18:10 PDT
I just tried and find some difficulties.
Can't scroll controll since events of moving screen, long tapping are triggered at the same time.  Who should I ask for?
Comment 31 cajbir (:cajbir) 2014-04-15 21:25:28 PDT
(In reply to Chris Pearce (:cpearce) from comment #29)
> Yes. To verify that this works correctly, you test that seeking in an HTML5
> <video> works using the built in controls in the B2G Browser App.
> 
> i.e.
> 1. Open http://pearce.org.nz/h in B2G Browser App.
> 2. Seek the video on that page using the controls displayed on the video
> 
> If that works, it's verified.

That doesn't actually verify fastSeek works does it? It just verifies seeking works. The verification steps above work with or without the fastSeek patch. What's probably needed is an example that uses keyframes wide enough apart to tell the difference between fastSeek and normal seek.
Comment 32 Paul Yang [:pyang] (away 9/22-9/26) 2014-05-12 23:19:34 PDT
Sounds reasonable.  Chris - Do we have an appropriate sample for this, or we can reuse the one you provided, but just show the trick to enable fastseek?  Thanks in advance.

(In reply to cajbir (:cajbir) from comment #31)
> (In reply to Chris Pearce (:cpearce) from comment #29)
> > Yes. To verify that this works correctly, you test that seeking in an HTML5
> > <video> works using the built in controls in the B2G Browser App.
> > 
> > i.e.
> > 1. Open http://pearce.org.nz/h in B2G Browser App.
> > 2. Seek the video on that page using the controls displayed on the video
> > 
> > If that works, it's verified.
> 
> That doesn't actually verify fastSeek works does it? It just verifies
> seeking works. The verification steps above work with or without the
> fastSeek patch. What's probably needed is an example that uses keyframes
> wide enough apart to tell the difference between fastSeek and normal seek.
Comment 33 Chris Pearce (:cpearce) 2014-05-13 17:06:14 PDT
You can use this test case:
http://people.mozilla.org/~cpearce/fastSeek/
Use the big sliders beneath the video to test slow and fast seek, an average time taken is displayed.

This video has a 600-frame keyframe distance. Which is ridiculous, but it demonstrates that fastSeek is better on this video.

Note You need to log in before you can comment on or make changes to this bug.