Last Comment Bug 462957 - Implement nsIDOMHTMLMediaElement::GetBuffered()
: Implement nsIDOMHTMLMediaElement::GetBuffered()
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
:
Mentors:
Depends on: 589561 600791
Blocks: video 475312 476984 569993 570904 576539 584615
  Show dependency treegraph
 
Reported: 2008-11-03 17:04 PST by PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
Modified: 2011-08-03 04:51 PDT (History)
23 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
betaN+


Attachments
Patch v1 (71.98 KB, patch)
2009-05-11 20:33 PDT, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch v2 (15.97 KB, patch)
2009-05-13 16:08 PDT, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 1: DOM implementation of HTMLMediaElement.buffered (21.65 KB, patch)
2010-05-31 16:53 PDT, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 2: WAV support for HTMLMediaElement.buffered (2.52 KB, patch)
2010-05-31 16:54 PDT, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 3: Ogg support for HTMLMediaElement.buffered. (20.62 KB, patch)
2010-05-31 16:55 PDT, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
cajbir.bugzilla: review-
Details | Diff | Splinter Review
Patch v3: Testcase (2.66 KB, patch)
2010-05-31 16:55 PDT, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 1 v2: DOM implementation of HTMLMediaElement.buffered (26.71 KB, patch)
2010-06-02 21:06 PDT, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 1 v 3: DOM implementation of HTMLMediaElement.buffered (21.97 KB, patch)
2010-06-10 16:38 PDT, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
roc: review+
Details | Diff | Splinter Review
Patch 2 v 2: WAV support for HTMLMediaElement.buffered (2.33 KB, patch)
2010-06-10 16:39 PDT, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
kinetik: review+
Details | Diff | Splinter Review
Patch 3 v2: Ogg support for HTMLMediaElement.buffered (25.74 KB, patch)
2010-06-10 16:44 PDT, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 4 v2: Testcase (2.64 KB, patch)
2010-06-10 16:45 PDT, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
roc: review+
Details | Diff | Splinter Review
Patch 5: Pin media stream while seeking (6.53 KB, patch)
2010-06-10 16:48 PDT, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
roc: review+
Details | Diff | Splinter Review
Patch 1 v4: DOM implementation of HTMLMediaElement.buffered (21.91 KB, patch)
2010-06-13 16:55 PDT, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
cpearce: review+
Details | Diff | Splinter Review
Patch 5 v2: Pin media stream while seeking (6.72 KB, patch)
2010-06-13 16:56 PDT, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
cpearce: review+
Details | Diff | Splinter Review
Patch 5 v3: Pin media stream while seeking (6.77 KB, patch)
2010-06-13 17:18 PDT, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
cpearce: review+
Details | Diff | Splinter Review
Patch 3 v3: Ogg support for HTMLMediaElement.buffered (25.74 KB, patch)
2010-06-13 17:19 PDT, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
cajbir.bugzilla: review-
Details | Diff | Splinter Review
Patch 3 v4: Ogg support for HTMLMediaElement.buffered (30.21 KB, patch)
2010-06-14 20:37 PDT, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
cajbir.bugzilla: review+
Details | Diff | Splinter Review
Patch 6: Stubs for WebM and Raw decoder backends (3.30 KB, patch)
2010-07-29 21:58 PDT, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
roc: review+
Details | Diff | Splinter Review
Patch 7: Fix out of bounds on nsHTMLTimeRanges (2.57 KB, patch)
2010-07-29 21:59 PDT, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
roc: review+
Details | Diff | Splinter Review

Description PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2008-11-03 17:04:54 PST

    
Comment 1 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2008-11-03 18:09:42 PST
You'll need a nsIDOMHTMLTimeRanges interface to implement this. Stubs for that are removed in patch for bug 462953.
Comment 2 michael dale 2009-03-17 15:20:08 PDT
As I understand to support this we need to improve the exposing the time information from liboggplay. ie
Comment 3 michael dale 2009-03-17 15:21:31 PDT
this bug has changed a bit in the spec to just be a read only attribute "buffered" TimeRanges object
http://www.whatwg.org/specs/web-apps/current-work/#dom-media-buffered

For this to be supported we need to do some improvements to the liboggplay library. It needs to better expose time information for arbitrary chunks video data. As I understand the support for reading this time info from ogg streams exists in the underling liboggz library.  It just needs to be exposed in a way firefox can get at it without running through the full decode. Maybe liboggplay / liboggz folks could comment on the order of complexity in exposing this info.
Comment 4 Justin Dolske [:Dolske] 2009-03-23 16:25:15 PDT
I think this would be really helpful for the video controls, in two ways:

1) The progress bar is currently based on file size, while the thumb is based on time. This seems to work well enough in practice, but I'd suspect that the two measurement systems can be slightly off in some cases (eg, the byte in the middle of the file might not be in the middle timewise).

2) It provides a solution to bug 473235. Currently, if you pause a live stream there's no way to get back to real-time (or indicate where you're at), even though we've buffered up the data.
Comment 5 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2009-05-04 01:10:07 PDT
I've been thinking a lot about how to implement this, roc/doublec, here's my solution, I'd appreciate your comments.

Proposed solution:

Write a simple Ogg parser, to extract the streams' header info, and extract the granulepos of ogg pages near the extremities of the data cached in the media cache. We can use the streams' header info to convert granulepos into a timestamp. (Incidently we could use the Theora stream's header info to tell us the video dimensions earlier too.) We can memoize this for faster offset->time mapping.

We can also use this data to narrow down the seek bisection search space, as in the patch for bug 469408.

It's not that hard to do this, I've written a test program that parses Ogg streams, the Vorbis and Theora page headers, and (without error checking, most notably without the CRC check to verify the ogg page capture) my test program is only 460 lines of C++.

Possible implementation:

In HTMLMediaElement.buffered (and in the custom seek implementation), we run through the nsMediaCache's cached ranges (similar to my patch in bug 469408), and get the offset->time mappings for extremities of each range. We use an array as a cache/hash table to map bucketed offsets to timestamp mappings. i.e. TimeOf(offset) -> mOffsetTime[offset % bucketSize]. If the time for an offset is unknown, we scan that offset's cached block for the first Ogg page header, and extract the granulepos for that page.

We should make bucketSize=nsMediaCacheStream::BLOCK_SIZE. We probably don't need to worry about rounding the offsets down; anecdotally it appears that you're unlikely to store more than about 100ms of video in 4K anyway. 

One problem is that while scanning for an ogg page header, in order to verify that we've found the capture pattern for an actual page header, we should test the page's CRC value, and to do that we need to read the entire page. Pages are usually a little over (and almost always over) 4K in size - bigger than one block in the media cache. We don't really want to set our bucket size larger than the media cache block size, else we'll be bucketing buffered and unbuffered ranges together, and that may cause us to unwittingly do a blocking read (on the main thread!) when we're getting the timestamp for an offset. So we'd need to actually find the first page header that starts in the current block, but it may finish in a subsequent (not necessarily adjacent) block. If a page for which we're trying to find a timestamp finishes in a non-cached adjacent block, we'd be best to just report in HTMLMediaElement.buffered that the range finishes at the time corresponding to the previous cached block.

Since pages are usually larger than the cache-block-size, maybe we should report the buffered range as being [TimeOf(startOffset), max(startOffset,TimeOf(endOffset-BLOCK_SIZE))]. If the range is too small to determine a time, we can not report in HTMLMediaElement.buffered that it's buffered; liboggz won't find a complete page without a blocking read either, so it's effectively not buffered.

We could create a real hash table which maps the cached-ranges to buffered-ranges if the above rolling over into subsequent blocks is too slow.
Comment 6 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2009-05-11 20:33:22 PDT
Created attachment 376840 [details] [diff] [review]
Patch v1

Implements HTMLMediaElement.buffered by parsing Ogg data in the media cache. I was going to rework my patch for bug 469408 so that it works on top of this one, since they share a bunch of code, but Roc has asked me to do the opposite... So I'm going to rework bug 469408 to include much of the code here instead... Just posting the patch here so that anyone who wants to look at it can...
Comment 7 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2009-05-13 16:08:51 PDT
Created attachment 377280 [details] [diff] [review]
Patch v2

* Based on top of patch from bug 469408.
* Implements HTMLMediaElement.buffered for Ogg media, does not implement it for WAV audio.
Comment 8 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2009-06-28 18:27:04 PDT
We can implement this like so:
* Create an CachedStreamReader as per previous patch, which will read within cached ranges of the media cache.
* Use libogg to find the pages from the extremities of each cached range, and extract the time for each of those pages' granulepos.

We can just pass libogg a memory buffer, and when it runs out it simply reports that it needs more - it's pretty independent of the underlying IO layer, and won't cause us to block while reading, so we can do it safely on the main thread.
Comment 9 Torrance 2010-02-10 13:24:38 PST
I just wanted to add my support for implementing the HTMLmedia.buffered methods. 

I've been designing a custom UI and have run into some limitations using the Firefox's alternative: the e.loaded and e.total properties that are fired as part of a progress event.

They're not ideal since they're measuring buffered data as opposed to buffered seconds, and the resulting percentage only roughly correlates with what Firefox can actually play.

Perhaps more importantly, having read about Firefox's media caching policy, it appears firefox may at any time start dropping cached data from earlier on in
the video, and this information can't be conveyed in a simple 'e.loaded' property. Nor, for example, can it convey the buffered information when a video has been seeked, and has started downloading from the middle of the video via a byte-range request.

Have their been any updates to this?
Comment 10 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2010-02-10 13:29:54 PST
(In reply to comment #9)
> Have their been any updates to this?

No, I haven't had a chance to work on this unfortunately. I know how to implement this, I just need to find the time...
Comment 11 Paul Rouget [:paul] 2010-04-12 09:37:22 PDT
Chris, any updates?

http://jilion.com/sublime/video needs it (even if they can simulate it).
Comment 12 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2010-04-12 14:22:17 PDT
(In reply to comment #11)
> Chris, any updates?

Sorry, no chance to work on this yet. :(
Comment 13 Matthew Gregan [:kinetik] 2010-05-27 17:06:53 PDT
If the container has an index (e.g. Ogg with OggIndex, or most WebM files) we can use the index to implement this fairly easily--the index already provides a mapping of time ranges to byte ranges.

There are a few downsides to this approach, but maybe we can live with them:

- Only works if the container provides an index.

- May require a seek to load the index.  It's common (for now) for WebM files to have an index at the end.  In the future WebM file are more likely to have an index at the start, so a seek wouldn't be required with those files.

- The index will provide an arbitrarily lower granularity than we could calculate via tedious inspection.

Even using the approach of inspecting the pinned data in the media cache, it's easier to implement this for WebM as the frame/block timecodes are trivially available in the container.  It's probably necessary to use top-level Clusters as a sync point in the data, which means that the buffered ranges calculated using this method would only have Cluster-level granularity for the start of the buffered range.  Once we have sync we can calculate block-level granularity for the end of the range.
Comment 14 Matthew Gregan [:kinetik] 2010-05-27 17:22:24 PDT
(In reply to comment #13)
> - May require a seek to load the index.  It's common (for now) for WebM files
> to have an index at the end.  In the future WebM file are more likely to have
> an index at the start, so a seek wouldn't be required with those files.

We can't actually do this because buffered is synchronous, so we can't kick off a new HTTP request to load the index during a call to calculate the buffered ranges.  We could do tricks like preloading the index asynchronously when we start loading the file, but the complexity introduced by dealing with that offsets the simplicity offered by using the index to calculate buffered ranges.
Comment 15 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2010-05-31 16:53:19 PDT
Created attachment 448436 [details] [diff] [review]
Patch 1: DOM implementation of HTMLMediaElement.buffered

Add buffered attribute to HTMLMediaElement.
Comment 16 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2010-05-31 16:54:28 PDT
Created attachment 448437 [details] [diff] [review]
Patch 2: WAV support for HTMLMediaElement.buffered

Wav support.
Comment 17 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2010-05-31 16:55:08 PDT
Created attachment 448438 [details] [diff] [review]
Patch 3: Ogg support for HTMLMediaElement.buffered.

Ogg support for HTMLMediaElement.buffered.
Comment 18 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2010-05-31 16:55:45 PDT
Created attachment 448439 [details] [diff] [review]
Patch v3: Testcase

Backend-independent testcase.
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-05-31 17:08:16 PDT
+  nsHTMLTimeRanges* ranges = new nsHTMLTimeRanges();
+  NS_IF_ADDREF(*aBuffered = ranges);

NS_ADDREF, since 'new' is infallible now.

+  nsAutoTArray<float,4> mStartTimes;
+  nsAutoTArray<float,4> mEndTimes;

I think it would be slightly better to define a struct that contains start+end and have an nsAutoTArray of those --- one less memory allocation, and we don't have to consider whether the array lengths are in sync.

+    s->Pin();
+    nsresult res = mReader->GetBuffered(aBuffered, mStartTime);
+    s->Unpin();

Pin and Unpin not needed.

Where's nsIDOMHTMLTimeRanges.idl?
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-05-31 17:08:48 PDT
Ah, it's already in the tree...
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-05-31 17:15:36 PDT
+  PRInt64 startOffset = mWavePCMOffset;
+  while (PR_TRUE) {
+    PRInt64 endOffset = mStream->GetCachedDataEnd(startOffset);
+    if (endOffset == startOffset) {
+      // Uncached at startOffset.
+      endOffset = mStream->GetNextCachedData(startOffset);
+      if (endOffset == -1) {
+        // Uncached at startOffset until endOffset of stream, or we're at
+        // the end of stream.
+        break;
+      }
+    } else {
+      // Bytes [startOffset..endOffset] are cached.
+      aBuffered->Add(BytesToTime(startOffset), BytesToTime(endOffset));
+    }
+    startOffset = endOffset;
+  }

I think this could just be

  PRInt64 startOffset = mStream->GetNextCachedData(mWavePCMOffset);
  while (startOffset >= 0) {
    PRInt64 endOffset = mStream->GetCachedDataEnd(startOffset);
    // Bytes [startOffset..endOffset] are cached.
    aBuffered->Add(BytesToTime(startOffset), BytesToTime(endOffset));
    startOffset = mStream->GetNextCachedData(endOffset);
  }
Comment 22 Matthew Gregan [:kinetik] 2010-05-31 17:23:35 PDT
Comment on attachment 448437 [details] [diff] [review]
Patch 2: WAV support for HTMLMediaElement.buffered

From the first patch:
+  virtual nsresult GetBuffered(nsHTMLTimeRanges* aBuffered, PRInt64 aStartTime) = 0;

Wrong signature?

Also, don't we need to pin data in the media cache if the user seeks into a buffered range?

From this patch:
+  PRInt64 startOffset = mWavePCMOffset;
[...]
+      aBuffered->Add(BytesToTime(startOffset), BytesToTime(endOffset));

This means that the start time of the first buffered range is going to be non-zero.  mWavePCMOffset is where the PCM data starts, so it defines the "zero time" point in the stream.  You'll need to adjust the data offsets by that before you call BytesToTime.

+nsWaveDecoder::GetBuffered(nsHTMLTimeRanges* aBuffered) {

Braces riding up.
Comment 23 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-05-31 17:27:45 PDT
I think we really want to return 0,duration as the buffered range for a fully buffered video. Also, the tests should test some larger files in case there are interactions with the media cache.
Comment 24 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-05-31 17:28:40 PDT
(In reply to comment #22)
> Also, don't we need to pin data in the media cache if the user seeks into a
> buffered range?

Yes. I think when we start a seek in SetCurrentTime, we should pin the stream and only unpin it when the seek completes.
Comment 25 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2010-05-31 17:41:34 PDT
(In reply to comment #22)
> (From update of attachment 448437 [details] [diff] [review])
> From the first patch:
> +  virtual nsresult GetBuffered(nsHTMLTimeRanges* aBuffered, PRInt64
> aStartTime) = 0;
> 
> Wrong signature?

The aStartTime parameter is only required by the nsOggReader implementation  (and it may be required for WebM too perhaps?), so the call chain which requires that goes though the nsDecoderStateMachine which nsWavDecoder hasn't yet been refactored to use.

> This means that the start time of the first buffered range is going to be
> non-zero.  mWavePCMOffset is where the PCM data starts, so it defines the "zero
> time" point in the stream.  You'll need to adjust the data offsets by that
> before you call BytesToTime.

Thanks! You're right, without this our buffered ranges don't start at 0.
Comment 26 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2010-06-02 21:06:52 PDT
Created attachment 448942 [details] [diff] [review]
Patch 1 v2: DOM implementation of HTMLMediaElement.buffered

Patch 1 with review comments addressed. Also pins the decoder's media stream while we seek, unpinning when we complete the seek, and also checking when we destroy the media element if we need to unpin (this can happen if we destroy the media element while it's seeking).
Comment 27 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-06-02 21:11:31 PDT
+  struct nsTimeRange {

Just call it TimeRange.

 void nsHTMLMediaElement::SeekCompleted()
 {
   mPlayingBeforeSeek = PR_FALSE;
   SetPlayedOrSeeked(PR_TRUE);
   DispatchAsyncSimpleEvent(NS_LITERAL_STRING("seeked"));
   // We changed whether we're seeking so we need to AddRemoveSelfReference
   AddRemoveSelfReference();
+  // Unpin the stream to re-enable media cache evictions.
+  Unpin();

Unpin before dispatching 'seeked'? Or maybe mPinnedStream should be a count? Basically I'm thinking of the case where someone sets currentTime while we're seeking, or in 'seeked'. We don't to suddenly unpin after 'seeked'.
Comment 28 cajbir (:cajbir) 2010-06-02 22:48:01 PDT
Comment on attachment 448438 [details] [diff] [review]
Patch 3: Ogg support for HTMLMediaElement.buffered.

+++ b/content/media/nsMediaStream.cpp
+nsresult nsMediaChannelStream::ReadFromCache(char* aBuffer, PRInt64 aOffset, PRUint32 aCount) {
+  return mCacheStream.ReadFromCache(aBuffer, aOffset, aCount);
+}

Opening brace should start on new line.

+nsresult nsMediaFileStream::ReadFromCache(char* aBuffer, PRInt64 aOffset, PRUint32 aCount)
+{
...
+  Seek(nsISeekableStream::NS_SEEK_SET, aOffset);

Check result of seek and handle failure.

+  PRUint32 discard;
+  nsresult res = mInput->Read(aBuffer, aCount, &discard);

What happens if discard does not equal aCount? Do you need to loop
until you've actually read aCount bytes?

+  Seek(nsISeekableStream::NS_SEEK_SET, aOffset);

Check result of seek and handle failure. If 'res' is a failed code
from the Read call should we still call Seek? If Seek then returns an
error does it hide the original error from the Read call?

+++ b/content/media/nsMediaStream.h
@@ -168,16 +168,20 @@ public:
+  // Reads only data which is cached in the media cache. If you try to read
+  // any data which overlaps uncached data, this function will return failure.
+  // This is the only read operation which is safe to call on the main thread.
+  virtual nsresult ReadFromCache(char* aBuffer, PRInt64 aOffset, PRUint32 aCount) = 0;

The comment says this is the only read operation which is safe to call
on the main thread but the comment at the beginning of nsMediaStream
says:

>   Most methods must be called on the main thread only. Read, Seek and
>   Tell may be called on another thread which may be a non main
>   thread. 

This seems to say that the other Read call can be done on the main
thread as well which contradicts your comment.
 

+++ b/content/media/ogg/nsOggReader.cpp
 PRInt64 nsOggReader::FindEndTime(PRInt64 aEndOffset)
...
+  mDecoder->GetCurrentStream()->Seek(nsISeekableStream::NS_SEEK_SET,
+                                     mDataOffset);

Check return value and handle errors.

In FindEndTime:

+      if (aMode == BLOCKING) {
+        NS_ASSERTION(readHead < aEndOffset,
+                     "Stream pos must be before range end");
+        stream->Seek(nsISeekableStream::NS_SEEK_SET, readHead);

Check return value.

In PageSync:
 
+      nsresult rv = NS_OK;
+      if (aMode == BLOCKING) {
+        aStream->Seek(nsISeekableStream::NS_SEEK_SET, readHead);

Check return value.

+nsresult nsOggReader::GetBuffered(nsHTMLTimeRanges* aBuffered, PRInt64 aStartTime)

Is this main thread only? If so, assert.

+  PRInt64 startOffset = mDataOffset;
+  while (PR_TRUE) {

Infinite loops make me nervous. How do you ensure that this is always terminated?
Can you write a comment explaining how you expect it to terminate so we can match
with how you think it'll behave with what the code actually does?

+      aBuffered->Add((float)startTime/1000.0f, (float)endTime/1000.0f);

C++ style casts.

+++ b/content/media/ogg/nsOggReader.h
+// Read mode. Used to denote whether we want blocking reads, or non-blocking
+// reads from cached data only.
+enum eReadMode {
+  // Allow reads from cached and uncached data. If data isn't cached, we'll
+  // block until its available.
+  BLOCKING = 1,
+  // Only read from cached data. If data is unavailable, fail.
+  CACHED = 2
+};

If this is only used by nsOggReader code, make this local to the
nsOggReader class otherwise BLOCKING and CACHED are in the global
namespace and could conflict with other code.

+  // Get the start time of aOffset. This is the presentation time of the first
+  // frame/sample we'd be able to play if we started playback at aOffset.
   virtual VideoData* FindStartTime(PRInt64 aOffset,
                                    PRInt64& aOutStartTime);

The comment for this is different to that in nsBuiltinDecoderReader. Is there a reason for that? Maybe remove this comment and modify nsBuiltinDecoderReader's comment?

   // This is called on the main thread, and it must not block.
-  virtual nsresult GetBuffered(nsHTMLTimeRanges* aBuffered);
+  virtual nsresult GetBuffered(nsHTMLTimeRanges* aBuffered, PRInt64 aStartTime);

Describe in comment what aStartTime is for. Comment says 'This is
called on the main thread'. Is it required that it *must* only be
called on the main thread? It's a bit unclear the way it is worded.
Comment 29 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2010-06-03 20:46:34 PDT
Roc:

(In reply to comment #27)
> Unpin before dispatching 'seeked'? Or maybe mPinnedStream should be a count?
> Basically I'm thinking of the case where someone sets currentTime while we're
> seeking, or in 'seeked'. We don't to suddenly unpin after 'seeked'.

So to recap: the aim is to pin when script sets currentTime, and unpin when the seek completes. If script sets currentTime before the original seek completes, we should remain pinned until all seeks complete.

For all current backends, ns*Decoder::Seek() is called for every set currentTime. This records the seek time, ensures the state machine is in seeking state, and returns. When the state machine thread loops again, it will enter its seeking logic, cause a "seeking" event to be dispatched, and perform the seek. There can therefore be a delay between calls to ns*Decoder::Seek() and the state machine thread actually performing the seek. Also multiple calls to ns*Decoder::Seek() could have been made by the time the state machine thread gets around to performing the seek, and we'll just seek to the latest seek target. When the state machine thread completes the seek, it sets an event to call ns*Decoder::SeekingStopped[AtEnd](), and that calls nsHTMLMediaElement::SeekCompleted(), which dispatches the "seeked" event. SeekingStopped[AtEnd]() also checks if there's been a seek request while the state machine thread has been seeking, and if so will change the state machine back to seeking state, causing another seek to be performed.

I think we should pin the stream when ns*Decoder::Seek() is called, and unpin the stream in ns*Decoder::SeekingStopped[AtEnd]() if there's not another pending seek.

It doesn't make sense to have a pin count, since there's not a 1:1 relationship between calls to ns*Decoder::Seek() and actual seeks performed. We should just have a flag to record if we're pinned or not. Besides, nsMediaStream already maintains a pin count.

Agreed?
Comment 30 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-06-05 14:43:12 PDT
Yes.
Comment 31 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2010-06-10 16:38:40 PDT
Created attachment 450498 [details] [diff] [review]
Patch 1 v 3: DOM implementation of HTMLMediaElement.buffered

With review comments addressed. I've implemented the media-stream-pinning-while -seeking in another patch.
Comment 32 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2010-06-10 16:39:26 PDT
Created attachment 450499 [details] [diff] [review]
Patch 2 v 2: WAV support for HTMLMediaElement.buffered
Comment 33 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2010-06-10 16:44:07 PDT
Created attachment 450503 [details] [diff] [review]
Patch 3 v2: Ogg support for HTMLMediaElement.buffered

With review comments addressed:
* More liberal comments, and made consistent.
* Removed eReadMode, replaced by a boolean parameter, since it only had two values anyway.
* Changed loop in nsOggReader::GetBuffered to be similar to that in nsWavStateMachine::GetBuffered as Roc suggestes; it's no longer an infinite loop, and is a bit cleaner.
* More paranoid error checking.
* Changed nsMediaFileStream::ReadFromCache() to use the base nsISeekableStream Tell(),Seek() and Read() functions, the nsMediaFileStream wrappers asserted we were not on the main thread, and we were getting buffered ranges on local files.
Comment 34 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2010-06-10 16:45:25 PDT
Created attachment 450504 [details] [diff] [review]
Patch 4 v2: Testcase

Testcase, now tests that we've buffered exactly [0...duration].
Comment 35 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2010-06-10 16:48:30 PDT
Created attachment 450506 [details] [diff] [review]
Patch 5: Pin media stream while seeking

Ensure the media stream is pinned from the time when HTMLMediaElement.currentTime is set, to the last pending seek completes.
Comment 36 Matthew Gregan [:kinetik] 2010-06-10 17:25:32 PDT
Comment on attachment 450499 [details] [diff] [review]
Patch 2 v 2: WAV support for HTMLMediaElement.buffered

+    aBuffered->Add(BytesToTime(startOffset - mWavePCMOffset),

startOffset can be less than mWavePCMOffset, so this needs to be something like:

if startOffset < mWavePCMOffset:
  startTime = 0
else
  startTime = BytesToTime(startOffset - mWavePCMOffset)
Comment 37 Matthew Gregan [:kinetik] 2010-06-10 17:43:57 PDT
cpearce pointed out that:

+  PRInt64 startOffset = mStream->GetNextCachedData(mWavePCMOffset);

Ensures startOffset is at least mWavePCMOffset, so this code is already fine.  BytesToTime has an abort-if-false assert for negative values, so it would've caught this if it were incorrect anyway.
Comment 38 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-06-13 16:30:40 PDT
Comment on attachment 450498 [details] [diff] [review]
Patch 1 v 3: DOM implementation of HTMLMediaElement.buffered

+  virtual nsresult GetBuffered(nsHTMLTimeRanges* aBuffered) = 0;
+

Stray newline
Comment 39 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-06-13 16:35:15 PDT
Comment on attachment 450506 [details] [diff] [review]
Patch 5: Pin media stream while seeking

+    }
+    else {

} else {

I think the Pin/Unpin methods and the flag should be named ...ForSeeking. We don't want anyone using these methods for anything else; if we come up with other reasons to pin, we should have separate methods and a separate flag.
Comment 40 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2010-06-13 16:55:57 PDT
Created attachment 450963 [details] [diff] [review]
Patch 1 v4: DOM implementation of HTMLMediaElement.buffered

Review comments addressed, and unbitrotten.
Comment 41 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2010-06-13 16:56:52 PDT
Created attachment 450964 [details] [diff] [review]
Patch 5 v2: Pin media stream while seeking

Review comments addressed.
Comment 42 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2010-06-13 17:18:15 PDT
Created attachment 450968 [details] [diff] [review]
Patch 5 v3: Pin media stream while seeking

Oops, forgot to rename nsMediaDecoder::mPinned, I've included that change in this updated patch.
Comment 43 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2010-06-13 17:19:29 PDT
Created attachment 450969 [details] [diff] [review]
Patch 3 v3: Ogg support for HTMLMediaElement.buffered

Same as previous, but unbitrotten/rebased.
Comment 44 cajbir (:cajbir) 2010-06-14 18:02:14 PDT
Comment on attachment 450969 [details] [diff] [review]
Patch 3 v3: Ogg support for HTMLMediaElement.buffered

+nsresult nsMediaChannelStream::ReadFromCache(char* aBuffer, PRInt64 aOffset, PRUint32 aCount)

Nit. Line too long (80 characters or less according to style guide). Same
with the function prototype for this in nsMediaStream.h.

 PRInt64 nsOggReader::FindEndTime(PRInt64 aEndOffset)
 {
  ...
+  if (!stream) {
+    return -1;
+  }
+  stream->Seek(nsISeekableStream::NS_SEEK_SET, mDataOffset);
+  return endTime;

Check return value of Seek call.

+      PageSyncResult res = PageSync(stream,
+                                    &state,
+                                    PR_TRUE,
+                                    startOffset,
+                                    endOffset,
+                                    &page,
+                                    discard);
+      if (res == PAGE_SYNC_ERROR) {
+        return NS_ERROR_FAILURE;
+      } else if (res == PAGE_SYNC_END_OF_RANGE) {
+        // Hit the end of range without reading a page, give up trying to
+        // find a start time for this buffered range, skip onto the next one.
+        break;
+      }

In the res == PAGE_SYNC_ERROR branch of the off we return early. I
think we need to do an ogg_sync_clear on the 'state' object before
returning. Or is that handled elsewhere? If we do need to call it,
check this for any other early returns too, including 'break',
'continue' and exiting the loop.
Comment 45 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2010-06-14 20:37:05 PDT
Created attachment 451209 [details] [diff] [review]
Patch 3 v4: Ogg support for HTMLMediaElement.buffered

* Fix nits.
* _All_ media stream seek return values are checked.
* Ensure we ogg_sync_clear() in the "res == PAGE_SYNC_ERROR" return branch and on the success path in nsOggReader::GetBuffered(). The 'break'/'continue' cases are handled on the success path.
Comment 46 Matthew Gregan [:kinetik] 2010-06-24 17:37:51 PDT
When this bug is fixed, the following test should pass:
(Note that the tests current sniff the user agent for Firefox--hopefully that'll be fixed soon.)

http://test.w3.org/html/tests/submission/Microsoft/video/video_002.htm
Comment 47 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-07-29 18:56:27 PDT
We need to finish this, players really need it.
Comment 48 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2010-07-29 21:58:20 PDT
Created attachment 461464 [details] [diff] [review]
Patch 6: Stubs for WebM and Raw decoder backends

Stubs for WebM and Raw decoder backends, so we can land support on trunk. Also changes tests so that they don't try to test buffered on media types whose decoders don't support it.
Comment 49 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2010-07-29 21:59:53 PDT
Created attachment 461465 [details] [diff] [review]
Patch 7: Fix out of bounds on nsHTMLTimeRanges

I realized that HTMLTimeRanges.start() and end() have their bounds check off by 1. This patches fixes that, and adds a test for it.
Comment 52 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2010-08-11 15:08:09 PDT
Thanks sheppy!

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