Closed Bug 462960 Opened 16 years ago Closed 13 years ago

Implement nsIDOMHTMLMediaElement::GetSeekable()

Categories

(Core :: Audio/Video, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: cpearce, Assigned: padenot)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 11 obsolete files)

20.70 KB, patch
Details | Diff | Splinter Review
      No description provided.
You'll need a nsIDOMHTMLTimeRanges interface to implement this. Stubs for that are removed in patch for bug 462953.
I need the initialTime member to implement this, but I guess I can write a "good enough" implementation using initialTime == 0.0. I'll may create a separate bug for a proper initialTime implementation.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla6
Bug 648595 adds initialTime as part of the support for media fragments.
Depends on: 619752
Attached patch Patch v0 -- Add seekable member. (obsolete) — Splinter Review
Note : One need to apply bug 619752 patch for this implementation to work, since this patch relies on the duration member value.
Attachment #542191 - Flags: review?(chris)
Here is a test proposal, using .sjs files to test different server-side configuration (e.g. no range requests or no content-length).
Attachment #542193 - Flags: review?(chris)
Comment on attachment 542191 [details] [diff] [review]
Patch v0 -- Add seekable member.

>+#include "nsIResumableChannel.h"

Do you need this?

>+/* readonly attribute nsIDOMHTMLTimeRanges seekable; */
>+NS_IMETHODIMP nsHTMLMediaElement::GetSeekable(nsIDOMTimeRanges * *aSeekable)
>+{

Return an empty range if mReadyState == HAVE_NOTHING.

>+  nsTimeRanges* ranges = new nsTimeRanges();
>+  NS_ADDREF(*aSeekable = ranges);
>+  double duration = 0.0;
>+  GetDuration(&duration);
>+  //TODO : change 0.0 to GetInitialTime() when available
>+  double initialTime = 0.0;
>+
>+  if (mDecoder && mDecoder->GetSeekable()) {
>+    if (mDecoder->IsInfinite()) {
>+      ranges->Add(initialTime, NS_IEEEPositiveInfinity());
>+      return NS_OK;
>+    }
>+    ranges->Add(initialTime, initialTime + duration);
>+    return NS_OK;
>+  }
>+  if (mDecoder && mDecoder->IsInfinite()) {
>+    return NS_OK;

We can still safely seek inside the buffered ranges in this case, so no need to return here.

In the case of an unseekable resource, the media cache tries it's hardest to not throw away data after the current playback position, so in most cases we should be able to seek around and then seek back into the "live" range without the "live" range's download stopping.

>+  }
>+
>+  GetBuffered(aSeekable);

GetBuffered allocates another nsTimeRanges and assigns it to *aSeekable, so you'll leak |ranges| here. You can call mDecoder->GetBuffered(ranges) directly here to avoid the leak.

If you run firefox (or mochitests) with the environment variable XPCOM_MEM_LEAK_LOG=2 set, you should get a log of leaked objects on exit.

Can you also implement step 7 in the W3 seeking specification: http://www.w3.org/TR/html5/video.html#seeking ?

This should be as easy as calling your GetSeekable() implementation in nsBuiltinDecoderStateMachine::Seek(double), and using that to sanitize the value of aTime so that it's the nearest value in the seekable ranges. If seekable is empty, just return.

I'd prefer to put the guts of GetSeekable() into nsBuiltinDecoder. It seems wrong for nsBuiltinDecoderStateMachine::Seek to have to ask the element for decoder-centric data like this.
Depends on: 667929
Here is the updated version of the patch, implementing point 7 of the seeking spec, and moving the code to nsBuiltinDecoder.

Anyway, this feature relies on the buffered member, thus it doesn't work quite well on ogg for the moment (bug 667929).
Attachment #542191 - Attachment is obsolete: true
Attachment #542522 - Flags: review?(chris)
Attachment #542191 - Flags: review?(chris)
I've added todos when the tests are failing due to the broken buffered member.
Attachment #542193 - Attachment is obsolete: true
Attachment #542524 - Flags: review?(chris)
Attachment #542193 - Flags: review?(chris)
Comment on attachment 542522 [details] [diff] [review]
Patch v1 -- Implements point 7 of seeking in spec + moved core code to the decoder

Can you rename |PRBool nsMediaDecoder GetSeekable()| and friends to IsSeekable() so that we can distinguish it from GetSeekable(nsTimeRanges*) easier? Thanks.

>+/* readonly attribute nsIDOMHTMLTimeRanges seekable; */
>+NS_IMETHODIMP nsHTMLMediaElement::GetSeekable(nsIDOMTimeRanges * *aSeekable)

NS_IMETHODIMP nsHTMLMediaElement::GetSeekable(nsIDOMTimeRanges** aSeekable)


>+{
>+  nsTimeRanges* ranges = new nsTimeRanges();
>+  NS_ADDREF(*aSeekable = ranges);
>+
>+  if(mReadyState > nsIDOMHTMLMediaElement::HAVE_NOTHING) {

if (...) 

Always put a space between "if" and "(".

Also return here if !mDecoder, else you'll crash if mDecoder is null.

>+PRBool
>+nsTimeRanges::IsInRanges(double aValue, PRUint32* aIntervalIndex) {
>+  for (PRUint32 i = 0; i < mRanges.Length(); i++) {
>+    if (mRanges[i].mStart <= aValue && aValue <= mRanges[i].mEnd) {
>+      if (aIntervalIndex != nsnull) {
>+        *aIntervalIndex = i;
>+      }
>+      return PR_TRUE;
>+    }
>+  }
>+  return PR_FALSE;
>+}

Because this is only used in one place, and it only uses data which is publically exposed anyway, there's no need to add this method to nsTimeRanges. Just add it as a static function in nsBuiltinDecoder.cpp.


>diff --git a/content/media/nsBuiltinDecoder.cpp b/content/media/nsBuiltinDecoder.cpp
>--- a/content/media/nsBuiltinDecoder.cpp
>+++ b/content/media/nsBuiltinDecoder.cpp
>@@ -292,16 +292,54 @@ nsresult nsBuiltinDecoder::Play()
> nsresult nsBuiltinDecoder::Seek(double aTime)
> {
>   NS_ASSERTION(NS_IsMainThread(), "Should be on main thread.");
>   ReentrantMonitorAutoEnter mon(mReentrantMonitor);
> 
>   if (aTime < 0.0)
>     return NS_ERROR_FAILURE;

While you're here, can you change this to NS_ABORT_IF_FALSE(aTime >= 0.0, "Some useful error message") instead? nsHTMLMediaElement::SetCurrentTime() ensures that aTime is >= 0.

> 
>+  nsTimeRanges seekable;
>+  PRUint32 length = 0;
>+  GetSeekable(&seekable);

Check return value of GetSeekable(nsTimeRanges), and return NS_OK if GetSeekable() failed (our return value will return all the way up to JS, and we don't want JS throwing an exception in this case).

>+  seekable.GetLength(&length);
>+  if (!length) {
>+    return NS_OK;
>+  }
>+
>+  PRUint32 range = 0;
>+  if (!seekable.IsInRanges(aTime, &range)) {

|range| is not set when IsInRanges() returns PR_FALSE, so this code would end up assuming the range before the seek target is range 0.

I'd change IsInRanges() to loop over the contents of seekable until either you find the range containing aTime, or returning once you reach the first range which starts after aTime. You should return the last range which starts before aTime (or store it in *aIntervalIndex).

It sort of looks like you were intending to do that anyway?

>+    nsresult res;
>+    double leftBound, rightBound;
>+    res = seekable.Start(range, &leftBound);

Assuming |range| is the index of the range prior to aTime, shouldn't this be calling End(), not Start() ?

>+    NS_ENSURE_SUCCESS(res, 0);

Return NS_OK, not 0.

>+    double distanceLeft = NS_ABS(leftBound - aTime);
>+
>+    double distanceRight = -1;
>+    if (range < length) {

How can this be false? Did you mean (range + 1 < length) ?

>+      res = seekable.End(range+1, &rightBound);

Assuming |range| is the index of the range prior to aTime, shouldn't this be calling Start(), not End() ?



>+    if (distanceLeft != distanceRight || distanceRight == -1) {
>+      if (distanceLeft > distanceRight) {
>+        aTime = leftBound;
>+      } else {
>+        aTime = rightBound;
>+      }
>+    } else {
>+      if (NS_ABS(distanceLeft - mCurrentTime) < NS_ABS(distanceRight - mCurrentTime)) {
>+        aTime = leftBound;
>+      } else {
>+        aTime = rightBound;
>+      }
>+    }

To save lines and branches you could write this block as:

      if (distanceLeft == distanceRight) {
        distanceLeft = NS_ABS(leftBound - mCurrentTime);
        distanceRight = NS_ABS(rightBound - mCurrentTime);
      } 
      aTime = (distanceLeft < distanceRight) ? leftBound : rightBound;

      


>+nsresult nsBuiltinDecoder::GetSeekable(nsTimeRanges* aSeekable)
>+{
>+  //TODO : change 0.0 to GetInitialTime() when available
>+  double initialTime = 0.0;
>+
>+  if (GetSeekable()) {
>+    if (IsInfinite()) {
>+      aSeekable->Add(initialTime, std::numeric_limits<double>::infinity());
>+      return NS_OK;
>+    }
>+
>+    aSeekable->Add(initialTime, initialTime + GetDuration());
>+    return NS_OK;

To make the code less "branchy", I'd structure that as:

    double end = IsInfinite() ? std::numeric_limits<double>::infinity()
                              : initialTime + GetDuration();
    aSeekable->Add(initialTime, end);
    return NS_OK;                          


>+  }
>+
>+  GetBuffered(aSeekable);
>+  return NS_OK;

GetBuffered() can fail, so instead of ignoring the return value, |return GetBuffered(seekable)|.

>diff --git a/content/media/nsMediaDecoder.h b/content/media/nsMediaDecoder.h
>[...]
>+  // Return the time range that can be seeked to.
>+  virtual nsresult GetSeekable(nsTimeRanges* aSeekable) = 0;

Return the time ranges that can be seeked into.
(In reply to comment #8)
> Created attachment 542524 [details] [diff] [review] [review]
> Patch v1 - Updated test to reflect the changes in the implementation of the
> feature
> 
> I've added todos when the tests are failing due to the broken buffered
> member.

They could also fail if the entire media loads before loadedmetadata fire. You're probably safer comparing |seekable| with |buffered| in these cases?
Here is the corrected code. I wonder why it seemed to work, considering all the things that were wrong...
Attachment #542522 - Attachment is obsolete: true
Attachment #542822 - Flags: review?(chris)
Attachment #542522 - Flags: review?(chris)
Attached patch Patch v1 -- removed todos (obsolete) — Splinter Review
I wonder if it would be acceptable to move range_equals() into some kind of header file (manifest.js ?), to avoid duplication ? I'm going to try to merge the tests, anyway, that should take care of the problem.
Attachment #542524 - Attachment is obsolete: true
Attachment #542825 - Flags: review?(chris)
Attachment #542524 - Flags: review?(chris)
Comment on attachment 542822 [details] [diff] [review]
Patch v2 - addressed chris' remarks.

>diff --git a/content/media/nsBuiltinDecoder.cpp b/content/media/nsBuiltinDecoder.cpp
>+static PRBool IsInRanges(nsTimeRanges& ranges, double aValue, PRUint32* aIntervalIndex) {

Nit: function/method arguments should be prefixed with "a", so |ranges| should be named |aRanges| unfortunately.

>+  nsresult rv;
>+  PRUint32 length;
>+  rv = ranges.GetLength(&length);

This actually can't fail, so there's no point storing (or checking) the return value.

>+  for (PRUint32 i = 0; i < length; i++) {
>+    double start, end;
>+    rv = ranges.Start(i, &start);
>+    NS_ENSURE_SUCCESS(rv, 0);

You should be returning NS_* values here rather than ints, because it means readers of the code don't need to remember what 0 means in this context. You've done this a few times.

Note if this fails, the caller would still assume that *aIntervalIndex contains a valid value, which would be bad. However Start() (and End()) only fail if i >= length, but that can't happen because of the loop condition. So provided you don't change the loop condition, you can just remove the return value check for Start() and End(). (We're needlessly doing this check elsewhere, but oh well).


>+    if (start > aValue) {
>+      if (aIntervalIndex != nsnull) {
>+        *aIntervalIndex = i - 1;

What if i == 0? This will overflow then.

You'll need to figure out how to handle this case.

>+      }
>+      return PR_FALSE;
>+    }
>+    rv = ranges.End(i, &end);
>+    NS_ENSURE_SUCCESS(rv, 0);
>+    if (start <= aValue && aValue <= end) {

If start > aValue, we will have returned earlier in the loop, so no need to check if start > aValue.


>+      if (aIntervalIndex != nsnull) {
>+        *aIntervalIndex = i;
>+      }
>+      return PR_TRUE;
>+    }
>+  }
>+  return PR_FALSE;

If we get here, *aIntervalIndex won't have a valid value either. This can happen if |ranges| contains only ranges which end before aValue.

You'll need to figure out how to handle this case.

>+}
>+
>+

Nit: Only 1 newline between functions please.

> nsresult nsBuiltinDecoder::Seek(double aTime)
> {
>   NS_ASSERTION(NS_IsMainThread(), "Should be on main thread.");
>   ReentrantMonitorAutoEnter mon(mReentrantMonitor);
> 
>-  if (aTime < 0.0)
>-    return NS_ERROR_FAILURE;
>+  NS_ABORT_IF_FALSE(aTime >= 0.0, "Cannot seek to a negative value.");
>+
>+  nsTimeRanges seekable;
>+  nsresult res;
>+  PRUint32 length = 0;
>+  res = GetSeekable(&seekable);
>+  if (res) {
>+    return NS_OK;
>+  }

Failure is unexpected here, so use NS_ENSURE_SUCCESS(res, NS_OK).

Use NS_ENSURE_* in cases where failure is unexpected (it prints a warning). Use |if (NS_FAILED(res)) {...}| in cases where failure is expected.

Don't use |if (res) {...}|, it's not as obvious as NS_FAILED(res) and friends.

>+  seekable.GetLength(&length);
>+  if (!length) {
>+    return NS_OK;
>+  }

This is ok, because length==0 is possible (though rare!) and not catastrophic in this case.
Comment on attachment 542825 [details] [diff] [review]
Patch v1 -- removed todos

(In reply to comment #12)
> Created attachment 542825 [details] [diff] [review] [review]
> Patch v1 -- removed todos
> 
> I wonder if it would be acceptable to move range_equals() into some kind of
> header file (manifest.js ?), to avoid duplication ?  I'm going to try to
> merge the tests, anyway, that should take care of the problem.

Avoiding duplication is an excellent idea. Go ahead and move range_equals() into manifest.js if you decide not to merge the tests.

If you don't merge the tests, in test_seekable1.html there's no point in passing |tests| to |createTestArray()|, since it contains only 1 element.

Uses spaces instead of tabs, tab width 2 spaces please.

>diff --git a/content/media/test/test_seekable2.html b/content/media/test/test_seekable2.html
[...]
>+function on_metadataloaded() {
>+  var v = document.getElementById('v');
>+	ok(v.seekable.length ==  0 || range_equals(v.seekable, v.buffered), "seekable.length should be initialy empty, or equal to buffered");

Can seekable.length be 0 when buffered.length is non 0? Shouldn't seekable == buffered in the non-seekable case? Same question for test_seekable3.html as well.
Attachment #542822 - Attachment is obsolete: true
Attachment #543171 - Flags: review?(chris)
Attachment #542822 - Flags: review?(chris)
Chris, indeed such a situation cannot happen in those tests, since this is the behavior we want to test (i.e. if data is buffered, we can play it, so we should be able to seek to it). I blindly kept this test while adding the range equality test.
Attachment #542825 - Attachment is obsolete: true
Attachment #543174 - Flags: review?(chris)
Attachment #542825 - Flags: review?(chris)
(In reply to comment #15)
> Created attachment 543171 [details] [diff] [review] [review]
> Patch v3 -- Adressed chris' remarks

Wrong patch? ;)
Indeed :)
Attachment #543171 - Attachment is obsolete: true
Attachment #543383 - Flags: review?(chris)
Attachment #543171 - Flags: review?(chris)
Comment on attachment 543383 [details] [diff] [review]
Patch v3 -- Adressed chris' remarks (good file).

>+/**
>+ * Returns PR_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, PR_FALSE is returned, and aIntervalIndex, if
>+ * not null, is set to the index of the range which end immediatly before aValue

s/end immediatly/ends immediately/ 

>+ * (and can be -1 if aValue is before aRanges.Start(0)).
>+ */
>+static PRBool IsInRanges(nsTimeRanges& aRanges, double aValue, PRInt32* aIntervalIndex) {

Pass aIntervalIndex by reference, then the compiler enforces that it can't be null for you.


> nsresult nsBuiltinDecoder::Seek(double aTime)
> {
>   NS_ASSERTION(NS_IsMainThread(), "Should be on main thread.");
>   ReentrantMonitorAutoEnter mon(mReentrantMonitor);
> 
>-  if (aTime < 0.0)
>-    return NS_ERROR_FAILURE;
>+  NS_ABORT_IF_FALSE(aTime >= 0.0, "Cannot seek to a negative value.");
>+
>+  nsTimeRanges seekable;
>+  nsresult res;
>+  PRUint32 length = 0;
>+  res = GetSeekable(&seekable);
>+  NS_ENSURE_SUCCESS(res, NS_OK);
>+
>+  seekable.GetLength(&length);
>+  if (!length) {
>+    return NS_OK;
>+  }
>+

Add a comment here explaining what we're doing here.

>+  PRInt32 range = 0;
>+  if (!IsInRanges(seekable, aTime, &range)) {
>+    if (range != -1) {
>+      double leftBound, rightBound;
>+      res = seekable.End(range, &leftBound);
>+      NS_ENSURE_SUCCESS(res, 0);

NS_ENSURE_SUCCESS(res, NS_OK);

>+      double distanceLeft = NS_ABS(leftBound - aTime);
>+
>+      double distanceRight = -1;
>+      if (range + 1 < length) {
>+        res = seekable.Start(range+1, &rightBound);
>+        NS_ENSURE_SUCCESS(res, NS_OK);
>+        distanceRight = NS_ABS(rightBound - aTime);
>+      }
>+
>+      if (distanceLeft == distanceRight) {
>+        distanceLeft = NS_ABS(leftBound - mCurrentTime);
>+        distanceRight = NS_ABS(rightBound - mCurrentTime);
>+      } 
>+      aTime = (distanceLeft < distanceRight) ? leftBound : rightBound;
>+    } else {
>+      seekable.Start(0, &aTime);

Shouldn't this be calling End() not Start()?

Since it's not obvious what case we're handling here, please add a comment explaining why we're doing this.


>+  // Return the time range that can be seeked into.

s/range/ranges/
> Shouldn't this be calling End() not Start()?

If range == -1, aTime is before the first seekable range, so we want the Start() of the first range. I've added a comment to clarify this part.
Attachment #543383 - Attachment is obsolete: true
Attachment #543745 - Flags: review?(chris)
Attachment #543383 - Flags: review?(chris)
Comment on attachment 543745 [details] [diff] [review]
Patch v3 - typos + comments + bugfix

Nit; please change:

+  // If the position we want to seek to is not in a seekable range, we seek
+  // to the closest position in the seekable ranges instead . If two positions
+  // matches, we seek to the closest position from the currentTime.

to:

+  // If the position we want to seek to is not in a seekable range, we seek
+  // to the closest position in the seekable ranges instead. If two positions
+  // are equally close, we seek to the position closest to the currentTime.

r+ with that change. Looks good.
Attachment #543745 - Flags: review?(chris) → review+
Attached patch Patch v4 -- Last nit. (obsolete) — Splinter Review
Attachment #543745 - Attachment is obsolete: true
Attachment #543902 - Flags: review?(chris)
Comment on attachment 543902 [details] [diff] [review]
Patch v4 -- Last nit.

Review of attachment 543902 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #543902 - Flags: review?(chris) → review+
Comment on attachment 543174 [details] [diff] [review]
Patch v2 -- Moved range_equals to the manifest.js file

Looks good. Sorry for the delay, I must have lost this review request somehow. If in future I (or someone else for that matter) doesn't respond to a review request in a week or so, you should email them or comment to ensure they've not forgotten.

Can this land now, or must bug 619752 land first?
Attachment #543174 - Flags: review?(chris) → review+
bug 619752 must indeed land first.

I've pushed several patches to try (http://tbpl.mozilla.org/?tree=Try&rev=4215ba20177f), and refactored the patches for them to be appliable without the patches for bug 462959.
No longer blocks: CVE-2011-2992
Keywords: checkin-needed
I see two patches, v2 and v4. I assume both need to land? v2 apparently adds tests for nsIDOMHTMLMediaElement.seekable. v4, according to its commit message (which contains random asterisks but lacks the reviewer), implements nsIDOMHTMLMediaElement.played, but as far as I can tell, that's a lie. If it implements nsIDOMHTMLMediaElement.seekable as this bug's summary indicates, there's no reason for it to be separate from the tests -- a unified patch with a cleaned up commit message would be appreciated.
Target Milestone: mozilla6 → ---
Here is a cleaned and unified patch, which applies and tests fine against last tip. I've fixed the commit message.
Attachment #543174 - Attachment is obsolete: true
Attachment #543902 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/e2fbc8276dbd
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Documentation updated:

https://developer.mozilla.org/en/DOM/HTMLMediaElement

Added info here:

https://developer.mozilla.org/en/Using_HTML5_audio_and_video#Seeking_through_media

An example needs to be added to the previous article, but otherwise this is done.

Also listed on Firefox 8 for developers.
Depends on: 751539
You need to log in before you can comment on or make changes to this bug.