648595 - Implement temporal dimension portion of Media Fragments URI specification for video/audio

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[cajbir (:cajbir)]

The Media Fragments URI specification has a section for temporal fragments for media resources. This allows 'clipping' video and audio so that only a certain range is played. See:

http://www.w3.org/TR/media-frags/#naming-time

Support for this has been requested by a number of people including the popcorn/butter group doing interesting media mashups.

For this bug I propose an initial implementation that provides support for the media fragment URI temporal dimension's on audio and video elements only (ie. not top level browser navigation to a video). I also propose implementing only 'Normal Play Time' at this stage.

Relevant bugs on the W3C issue tracker related to this specification:

This subset of the media fragments URI spec was what was discussed at FOMS 2010 with parties involved in the specification and the Opera and Firefox browsers as being a useful initial implementation (IIRC, correct me if I'm wrong!)

http://www.w3.org/Bugs/Public/show_bug.cgi?id=12426
http://www.w3.org/Bugs/Public/show_bug.cgi?id=12425

Comment 1

[Silvia Pfeiffer]

There is a Firefox plugin implementation available from the working group.

You can see the demos at
http://www.w3.org/2008/WebVideo/Fragments/code/plugin/demos/
and the code at
http://www.w3.org/2008/WebVideo/Fragments/code/plugin/

I've also got a video recording of it working at
http://www.youtube.com/watch?v=LfRRYp6mnu0

Comment 2

[Philip Jägenstedt [:foolip]]

While I of course expect that you'll critically review the spec and make decisions that make sense for Mozilla, I thought I'd share some conclusions I've reached through working with the MF working group and of course as media implementor at Opera. Implementors effectively vote against features of a spec by not implementing them, so this is my thinking:

#t: I will not implement any of the smpte syntaxes, since these do not make sense unless the format has embedded SMPTE time stamps. Ogg and WebM do not, and lacking that the only recourse is to convert it to a floating point number, which we already have NPT for.

#xywh: I'm in no hurry to implement the default pixel syntax, but think it is basically sane. The percent syntax only allows integer precision and seems to lack any compelling use cases, so I will not support it. If you do, make sure to be careful about the rounding, which is not specified in the spec yet.

#track: All good, but it's not clear how to map it onto formats that don't have unique strings for all tracks.

#id: Do not intend to support at all: unclear use cases and unclear mapping to WebM.

Comment 3

[cajbir (:cajbir)]

Thanks for the feedback Philip. I agree with your points.

For #t I plan to implement NPT only. I don't have plans to implement #xywh in this bug but once this one is done I'll look at what's involved. It probably won't be a priority though as I haven't had any requests from users about it (possibly because it's not well known yet). I've been following the mailing list thread on the rounding issues and will keep up to date with that. 

#track I do want to implement and I'll do it in a follow up bug. #id, no plans to support.

Comment 4

[cajbir (:cajbir)]

Attachment: Part 3: Tests

Comment 5

[cajbir (:cajbir)]

Attachment: Part 2: User interface

The user interface changes just add markers to the begin and end of the fragment on the bar. It's pretty unpolished and I'm open to better ways of displaying the media fragments.

Comment 6

[cajbir (:cajbir)]

Attachment: Part 1: Temporal media fragment support

The code changes for temporal media fragments support. 

Spec: http://www.w3.org/TR/media-frags/#naming-time
Test builds and demo: http://www.bluishcoder.co.nz/mediafrag/

Comment 7

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

Comment on attachment 551009 [details] [diff] [review]
Part 1: Temporal media fragment support

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

nsMediaFragmentURIParser should probably go in its own file.

Chris P is probably more than capable of reviewing this.

Comment 8

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

Comment on attachment 551009 [details] [diff] [review]
Part 1: Temporal media fragment support

>From: Chris Double <[e-mail]>
>Bug 648595 - Part 1/3: Implement temporal dimension portion of Media Fragments URI specification for video/audio - r=???

I tested on a live stream and playback always starts at 0, regardless of the fragment start, but the end time ends up being treated as a duration, not an end time.


>+++ b/content/html/content/src/nsHTMLMediaElement.cpp
>+class nsMediaFragmentURIParser
[...]
>+  // Return the start time in seconds obtained from the URI
>+  // fragment. If no start time or no valid temporal fragment
>+  // exists then 'aDefault' is returned.
>+  double GetStartTime(double aDefault);

Would you ever need to use any value other than 0 for aDefault? You only call this in one place, so may as well just return 0.

>+
>+  // Return the end time in seconds obtained from the URI
>+  // fragment. If no end time or no valid temporal fragment
>+  // exists then 'aDefault' is returned.
>+  double GetEndTime(double aDefault);

Would you ever need to use any value other than -1 for aDefault? You only call this in one place, so may as well just return -1.



>+PRBool nsMediaFragmentURIParser::ParseNPTMMSS(nsDependentSubstring& aString, double& aTime)
>+{
>+  nsDependentSubstring original(aString);
>+  PRUint32 mm = 0;
>+  PRUint32 ss = 0;
>+  double fraction = 0.0;
>+  if (!ParseNPTMM(aString, mm)) {
>+    aString.Rebind(original, 0);
>+    return PR_FALSE;
>+  }
>+
>+  if (aString.Length() < 2 || aString[0] != ':') {
>+    aString.Rebind(original, 0);
>+    return PR_FALSE;
>+  }
>+

If you call ParseNPTSec here and then you don't need to repeat the fractional second parsing code below?

>+  aString.Rebind(aString, 1);
>+  if (!ParseNPTSS(aString, ss)) {
>+    aString.Rebind(original, 0);
>+    return PR_FALSE;
>+  }
>+
>+  if (aString.Length() > 0 && aString[0] == '.') {
>+    PRUint32 index = 1;
>+    while (index < aString.Length() && IsDigit(aString[index])) {
>+      ++index;
>+    }
>+
>+    nsDependentSubstring number(aString, 0, index);
>+    PRInt32 ec;
>+    fraction = PromiseFlatString(number).ToDouble(&ec);
>+    if (NS_FAILED(ec)) {
>+      aString.Rebind(original, 0);
>+      return PR_FALSE;
>+    }
>+    aString.Rebind(aString, index);
>+  }
>+
>+  aTime = mm * 60 + ss + fraction;
>+  return PR_TRUE;
>+}


>+PRBool nsMediaFragmentURIParser::ParseNPTHH(nsDependentSubstring& aString, PRUint32& aUnit)

aUint is not a very descriptive name, how about aHours?

And appropriate names for the other aUnit parameters in other methods.

>+{
>+  if (aString.Length() == 0) {
>+    return PR_FALSE;
>+  }
>+
>+  PRUint32 index = 0;
>+  while (index < aString.Length() &&
>+         IsDigit(aString[index])) {
>+    ++index;
>+  }

You have this while loop repeated a few times, could you factor it out?

>+PRBool nsMediaFragmentURIParser::ParseNPTSS(nsDependentSubstring& aString, PRUint32& aUnit)

If you use ParseNPTSec in the ParseNPTMMSS case above, you don't need to implement the seconds parsing code again in ParseNPTSS. ParseNPTMM can just use it for minutes parsing instead.

>+PRBool nsMediaFragmentURIParser::IsDigit(nsDependentSubstring::char_type aChar)
>+{
>+  return
>+    (aChar == '0' ||
>+     aChar == '1' ||
>+     aChar == '2' ||
>+     aChar == '3' ||
>+     aChar == '4' ||
>+     aChar == '5' ||
>+     aChar == '6' ||
>+     aChar == '7' ||
>+     aChar == '8' ||
>+     aChar == '9');

How about:
return aChar >= '0' && aChar <= '9';


>+double nsMediaFragmentURIParser::GetStartTime(double aDefault)
>+{
>+  double result = aDefault;
>+  for (PRUint32 i = 0; i < mFragments.Length(); ++i) {
>+    if (mFragments[i].mName.EqualsLiteral("t")) {
>+      double start = -1;
>+      double end = -1;
>+      nsDependentSubstring s(mFragments[i].mValue, 0);
>+      if (ParseNPT(s, start, end)) {
>+        result = start;

Why not just return start here? Are you looking for the last start time? Then why not start from the other end and work backwards?

>+double nsMediaFragmentURIParser::GetEndTime(double aDefault)
>+{
>+  double result = aDefault;
>+  for (PRUint32 i = 0; i < mFragments.Length(); ++i) {
>+    if (mFragments[i].mName.EqualsLiteral("t")) {
>+      double start = -1;
>+      double end = -1;
>+      nsDependentSubstring s(mFragments[i].mValue, 0);
>+      if (ParseNPT(s, start, end)) {
>+        result = end;

Why not just return end here?


>+/* attribute double mozFragmentStart; */
>+NS_IMETHODIMP nsHTMLMediaElement::GetInitialTime(double *aTime)

Typo? mozFragmentStart?



>diff --git a/content/media/nsBuiltinDecoder.cpp b/content/media/nsBuiltinDecoder.cpp
>@@ -704,18 +705,18 @@ void nsBuiltinDecoder::SeekingStoppedAtEnd()
> 
>     // An additional seek was requested while the current seek was
>     // in operation.
>     if (mRequestedSeekTime >= 0.0) {
>       ChangeState(PLAY_STATE_SEEKING);
>       seekWasAborted = PR_TRUE;
>     } else {
>       UnpinForSeek();
>-      fireEnded = mNextState != PLAY_STATE_PLAYING;
>-      ChangeState(fireEnded ? PLAY_STATE_ENDED : mNextState);
>+      fireEnded = PR_TRUE;
>+      ChangeState(PLAY_STATE_ENDED);

Why do you need this change?


>diff --git a/content/media/nsBuiltinDecoder.h b/content/media/nsBuiltinDecoder.h
>+  // Set the media fragment end time
>+  virtual void SetFragmentEndTime(PRInt64 aEndTime) = 0;

Specify time units in comment please.


>+  // The end time of the media resource. Accessed on the main thread.
>+  // A negative value indicates the end time is the actual end of the
>+  // resource.
>+  double mEndTime;

Isn't this write only? Why do you need to store this?

>diff --git a/content/media/nsBuiltinDecoderStateMachine.cpp b/content/media/nsBuiltinDecoderStateMachine.cpp
>@@ -918,16 +924,23 @@ void nsBuiltinDecoderStateMachine::SetDuration(PRInt64 aDuration)
>+void nsBuiltinDecoderStateMachine::SetFragmentEndTime(PRInt64 aEndTime)
>+{
>+  mDecoder->GetReentrantMonitor().AssertCurrentThreadIn();
>+
>+  mFragmentEndTime = aEndTime;

With the exception of mCurrentFrameTime, all timestamps in the state machine are stored in the media's timeline, i.e. in the range [mStartTime,mEndTime]. So add mStartTime to mFragmentEndTime when setting it please (provided aEndTime >= 0), and update the use of mFragmentEndTime accordingly.

When you do that, use GetMediaTime() instead of mCurrentFrameTime in your change to UpdatePlaybackPosition().

>diff --git a/content/media/nsBuiltinDecoderStateMachine.h b/content/media/nsBuiltinDecoderStateMachine.h
>@@ -273,16 +273,19 @@ public:
>+  // Set Media Fragment end time
>+  void SetFragmentEndTime(PRInt64 aEndTime);

Specify time units in comment please.

Comment 9

[cajbir (:cajbir)]

Attachment: Part 1: Temporal media fragment support

Address review comments

>If you call ParseNPTSec here and then you don't need to repeat the 
> fractional second parsing code below?

ParseNPTSec has slightly different rules. SS only takes two digits whereas NPTSec can take any number. I've refactored the code to share more though.

> Why do you need this change?

This is a fix for an existing bug that media fragments trigger. I've spun it out into bug 679262. I'll do a proper fix there. Until that's fixed the media fragment tests involving seeking to the end will fail.

Comment 10

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

Comment on attachment 553369 [details] [diff] [review]
Part 1: Temporal media fragment support

Looks good!

(In reply to Chris Double (:doublec) from comment #9)
> > Why do you need this change?
> 
> This is a fix for an existing bug that media fragments trigger. [...]

I asked because I wondered if this was related to bug 661076 ("test_seek_out_of_range.html times out intermittently"), a rare orange which would run that code.

Comment 11

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

Comment on attachment 551006 [details] [diff] [review]
Part 3: Tests

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

::: content/media/test/fragment_noplay.js
@@ +2,5 @@
> +
> +var completed = false;
> +
> +function onLoadedMetadata() {
> +  if (completed)

We shouldn't be getting multiple loadedmetadata events right? Are we?

::: content/media/test/fragment_play.js
@@ +12,5 @@
> +  var s = start == null ? 0 : start;
> +  var e = end == null ? v.duration : end;
> +  var a = s - 0.15;
> +  var b = s + 0.15;
> +  ok(v.currentTime >= a && v.currentTime <= b, "loadedmetadata currentTime is " + a + " < " + v.currentTime + " < " + b);

Since bug 670055 landed our seeking should be accurate to the nearest audio sample (effectively half a microsecond accuracy to account for rounding to nearest usec). Do you really need that much fuzz here? Same thing applies to the same case in fragment_noplay.js, and to the onSeeked handler below.

@@ +49,5 @@
> +
> +  var e = end == null ? v.duration : end;
> +  var a = e - 0.15;
> +  var b = e + 0.15;
> +  ok(v.currentTime >= a && v.currentTime <= b, "paused currentTime is " + a + " < " + v.currentTime + " < " + b + " ? " + v._lastTimeUpdate);

This could fail randomly if we're under load, but I can't think of a better way to test this. Better keep an eye out for random failures here.

::: content/media/test/test_fragment_noplay.html
@@ +13,5 @@
> +<script class="testbody" type="text/javascript">
> +
> +var manager = new MediaTestManager;
> +
> +const NUM_FRAGMENT_TESTS = 1;

What purpose does this serve? Doesn't seem like it does anything.

Comment 12

[cajbir (:cajbir)]

Attachment: Part 3: Tests

Address review comments.

Comment 13

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

Comment on attachment 555318 [details] [diff] [review]
Part 3: Tests

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

::: content/media/test/fragment_noplay.js
@@ +6,5 @@
> +  var s = start == null ? 0 : start;
> +  var e = end == null ? v.duration : end;
> +  var a = s - 0.15;
> +  var b = s + 0.15;
> +  ok(v.currentTime >= a && v.currentTime <= b, "loadedmetadata currentTime is " + a + " < " + v.currentTime + " < " + b);

We don't need the 0.15s fuzz here right? r=cpearce with the fuzz removed, or an explanation.

Comment 14

[cajbir (:cajbir)]

Attachment: Part 2: Tests

Fuzz removed as request, carrying forward r+. Made this 'Part 2' with the intention of landing parts 1 and 2 and the user interface as part 3 later as suggested by roc.

Comment 15

[cajbir (:cajbir)]

Part 1: http://hg.mozilla.org/integration/mozilla-inbound/rev/dc1d14484d6e
Part 2: http://hg.mozilla.org/integration/mozilla-inbound/rev/1a1dbcc15b7e

Comment 16

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/dc1d14484d6e
http://hg.mozilla.org/mozilla-central/rev/1a1dbcc15b7e

Comment 17

[Eric Shepherd [:sheppy]]

Documented:

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

Also mentioned on Firefox 9 for developers.