619752 - Duration should be +Inf on infinite length video streams

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

We're in violation of the spec, we should report the duration as +Inf
when a video stream is known to be unbounded (e.g. a live stream):

"The duration attribute must return the time of the end of the media resource,
in seconds, on the media timeline. If no media data is available, then the
attributes must return the Not-a-Number (NaN) value. If the media resource is
known to be unbounded (e.g. a streaming radio), then the attribute must return
the positive Infinity value."

http://www.whatwg.org/specs/web-apps/current-work/multipage/video.html#dom-media-duration

We're currently increasing the duration as we play. This is causing jitter in the progress bar, i.e. bug 485696.

Comment 1

[cajbir (:cajbir)]

This was never implemented because we couldn't come up with a way to identify a stream as being unbounded.

Comment 2

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

Why can't we assume that streams which don't have a Content-Length header are infinite?

Comment 3

[cajbir (:cajbir)]

Because we don't know that they are. At the time the oggz-chop CGI generated streams without a Content-Length and I'm sure there are others.

Comment 4

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

That would be a bug in oggz-chop. It looks like Chrome and Opera are assuming streams without Content-Length are infinite. We should do the same for interoperability.

Comment 5

[cajbir (:cajbir)]

(In reply to comment #4)
> That would be a bug in oggz-chop.

Not really, it's perfectly fine to not send a content-length if it can't be computed in advance. A lot of CGI scripts do this.

Comment 6

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

This is a tough one. It seems to me that the only way we can be sure a stream is unbounded is if the author gives explicit metadata saying so.

It's not clear from the spec what to return if the duration is unknown. I'll ask in the WHATWG list.

Comment 7

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

I proposed changing the spec text "If the media resource is known to be unbounded" to "If the media resource is not known to be bounded" and Philip agrees:
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2010-December/029488.html

So basically for us that means if we have media data, but the stream duration is not known and we have no Content-Type or other metadata that tells us the length of the resource, we should return +Inf for the duration.

Comment 8

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

(In reply to comment #7)
> is not known and we have no Content-Type or other metadata that tells us the

I meant Content-Length of course

Comment 9

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

(In reply to comment #7)
> I proposed changing the spec text "If the media resource is known to be
> unbounded" to "If the media resource is not known to be bounded" and Philip
> agrees:
> http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2010-December/029488.html
> 
> So basically for us that means if we have media data, but the stream
> duration is not known and we have no Content-Type or other metadata that
> tells us the length of the resource, we should return +Inf for the duration.

Ian has made Roc's proposed change:

http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-June/031916.html

On 3/06/2011 11:28 a.m., Ian Hickson wrote:
> On Sat, 18 Dec 2010, Robert O'Callahan wrote:
>> >
>> > http://www.whatwg.org/specs/web-apps/current-work/multipage/video.html#dom-media-duration says:
>> > [...]
>> > 
>> > What if the duration is not currently known?
> The user agent must determine the duration of the media resource before 
> playing any part of the media data and before setting readyState to a 
> value equal to or greater than HAVE_METADATA, even if doing so requires 
> fetching multiple parts of the resource.
>
>
>> > I think in general it will be very difficult for a user-agent to know 
>> > that a stream is unbounded. In Ogg or WebM a stream might not contain an 
>> > explicit duration but still eventually end. Maybe it would make more 
>> > sense for the last sentence to read "If the media resource is not known 
>> > to be bounded, ..."
> Done.

Comment 10

[Paul Adenot (:padenot)]

Attachment: Patch v0 - Implemented duration=Infinity

Here is a proposal for this bug, it seems to work quite well.

As discussed above, the duration is set to be "Infinity" if :
- The stream is indeed infinite (webradio, tv stream, etc.).
- We couldn't deduce a duration from the Content-Length header
- Our media doesn't provide metadata that provide a way to compute the duration (ogg audio).
- The server doesn't support range-requests, and therefore we are not able to fetch the end of an ogg stream to compute the duration.

If the stream was infinite and ends, it means that we can deduce the duration of the media from the currentTime member when the stream ends.

Comment 11

[Paul Adenot (:padenot)]

Attachment: Patch v0 - Fix a test that was testing the old behavior

All the media mochitetests are passing, except content/media/test_contentDuration3.html, which was testing the old behavior. This patch fixes the test.

Comment 12

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

Comment on attachment 541367 [details] [diff] [review]
Patch v0 - Fix a test that was testing the old behavior

You really want the lifetime of mInfiniteStream to be tied to the lifetime of the decoder, not the media element, then it's easier to manage; you don't have to worry about resetting it when the element's src changes. So put mInfiniteStream in nsBuiltinDecoder instead. nsBuiltinDecoder::GetDuration() can then directly return Infinity if the stream is infinte.

Use "if (condition)", not "if(condition)".

The controls behave very differently on live streams (at least for me on the server I'm testing on). The duration label reads "Infinity : NaN : NaN", and the timelabel stays to the far left of the duration bar. You'll need to update the controls (their code is in toolkit/content/widgets/videocontrols.xml). Make another patch on top of this one and ask review from dolske.


>@@ -2014,16 +2021,22 @@ void nsHTMLMediaElement::MetadataLoaded(
>+void nsHTMLMediaElement::SetInfinite(PRBool aInfinite)
>+{
>+  LOG(PR_LOG_DEBUG, ("%p, Duration set to Infinity: %d", this, aInfinite));
>+  mInfiniteStream = aInfinite;
>+}
>+

You're writing to mInfiniteStream on another thread (see my comment on nsBuiltinDecoderStateMachine::SetDuration below), and also reading and writing it from the main thread elsewhere. Without synchronisation this could be troublesome. It's easier to just restrict its use to the main thread. So make SetInfinite() assert that it runs on the main thread to enforce this. You'll need to move SetInfinite() into nsBuiltinDecoder of course.


>@@ -2106,16 +2119,25 @@ void nsHTMLMediaElement::PlaybackEnded()
>+  // If the stream was infinite, and is now ended, we now know the duration of
>+  // the audio stream
>+  if (mInfiniteStream) {
>+    mDecoder->SetDuration(end);

We call UpdatePlaybackPosition() in the COMPLETED case of nsBuiltinDecoderStateMachine::Run(), which causes the duration stored in the decoder to keep up with the playback position. So you shouldn't need to set the duration in nsHTMLMediaElement::PlaybackEnded(), though you may need to fire a durationchange event. Move the rest into nsBuiltinDecoder::PlaybackEnded().

Us calling UpdatePlaybackPosition() in nsBuiltinDecoderStateMachine::Run() causes us to continually fire durationchange events on live streams, even though we'll be reporting an unchanging duration nsHTMLMediaElement::GetDuration(). So you'll neeed to prevent firing durationchange events in nsBuiltinDecoder::DurationChanged() if mInfiniteStream==PR_TRUE.


>+    mInfiniteStream = false;

Use SetInfinite(PR_FALSE) instead.

>diff --git a/content/media/nsBuiltinDecoderStateMachine.cpp b/content/media/nsBuiltinDecoderStateMachine.cpp
>--- a/content/media/nsBuiltinDecoderStateMachine.cpp
>+++ b/content/media/nsBuiltinDecoderStateMachine.cpp
>@@ -812,18 +812,22 @@ PRInt64 nsBuiltinDecoderStateMachine::Ge
> }
> 
> void nsBuiltinDecoderStateMachine::SetDuration(PRInt64 aDuration)
> {
>   NS_ASSERTION(NS_IsMainThread() || mDecoder->OnStateMachineThread(),
>     "Should be on main or state machine thread.");
>   mDecoder->GetReentrantMonitor().AssertCurrentThreadIn();
> 
>+  LOG(PR_LOG_DEBUG, ("Duration set to %d", aDuration));
>+
>   if (aDuration == -1) {
>     return;
>+  } else {
>+    mDecoder->GetMediaElement()->SetInfinite(PR_FALSE);
>   }

This method can run on a non-main thread if we have a duration from the media's index, so if you did this you'd need to worry about thread safety and synchronisation. Lets just only use mInfiniteStream on the main thread (all the other uses are main-thread only). So don't set non-infinite here. Instead check the duration in nsBuiltinDecoder::MetadataLoaded(), and set infiniteness there. By then we will know the duration if we can determine it, and we will know from the HTTP headers/response if the stream is infinite/seekable.

 
>diff --git a/content/media/nsMediaStream.cpp b/content/media/nsMediaStream.cpp
>--- a/content/media/nsMediaStream.cpp
>+++ b/content/media/nsMediaStream.cpp
>@@ -195,16 +195,17 @@ nsMediaChannelStream::OnStartRequest(nsI
>       return NS_OK;
>     }
> 
>     nsCAutoString ranges;
>     hc->GetResponseHeader(NS_LITERAL_CSTRING("Accept-Ranges"),
>                           ranges);
>     PRBool acceptsRanges = ranges.EqualsLiteral("bytes");
> 
>+

Don't add extra line breaks please.

>     if (mOffset == 0) {
>       // Look for duration headers from known Ogg content systems.
>       // In the case of multiple options for obtaining the duration
>       // the order of precedence is:
>       // 1) The Media resource metadata if possible (done by the decoder itself).
>       // 2) Content-Duration message header.
>       // 3) X-AMZ-Meta-Content-Duration.
>       // 4) X-Content-Duration.
>@@ -218,17 +219,20 @@ nsMediaChannelStream::OnStartRequest(nsI
>       if (NS_FAILED(rv)) {
>         rv = hc->GetResponseHeader(NS_LITERAL_CSTRING("X-Content-Duration"), durationText);
>       }
> 
>       if (NS_SUCCEEDED(rv)) {
>         double duration = durationText.ToDouble(&ec);
>         if (ec == NS_OK && duration >= 0) {
>           mDecoder->SetDuration(duration);
>+          element->SetInfinite(PR_FALSE);

This sets mInfinteStream to its default value, you shouldn't need to do this if the lifetime of mInfiniteStream is tied to the decoder.

Comment 13

[Paul Adenot (:padenot)]

Attachment: Patch v1 - moved feature to nsBuiltinDecoder

Comment 14

[Paul Adenot (:padenot)]

Attachment: Patch v0 - spotted other test failure.

Comment 15

[Paul Adenot (:padenot)]

Attachment: Patch v0 - Fixed frontend for this feature

Comment 16

[Paul Adenot (:padenot)]

Comment on attachment 541690 [details] [diff] [review]
Patch v0 - Fixed frontend for this feature

Justin, when the media is not known to be bounded, "Ukn." is displayed in place of the duration. I guess we need some kind of localization here, don't we ?

Apart from that, I think it's probably better to use Math.round instead of Math.floor when rounding values, since the duration reported by Firefox was off by one second, compared to other popular media player, on my computer.

Comment 17

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

(In reply to comment #16)
> Comment on attachment 541690 [details] [diff] [review] [review]
> Patch v0 - Fixed frontend for this feature
> 
> Justin, when the media is not known to be bounded, "Ukn." is displayed in
> place of the duration. 

My vote would be for something like -:--:-- or simply - if the media is unbounded. I'm sure you'll get as many opinions as developers though, so wait for what dolske says.

Comment 18

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

Comment on attachment 541688 [details] [diff] [review]
Patch v1 - moved feature to nsBuiltinDecoder

Looks good! r+ with the following changes:

>@@ -2106,16 +2106,21 @@ void nsHTMLMediaElement::PlaybackEnded()
[...]
> 
>+  if (mDecoder->IsInfinite()) {
>+    LOG(PR_LOG_DEBUG, ("%p, got duration by reaching the end of the stream", this));
>+    DispatchAsyncEvent(NS_LITERAL_STRING("durationchange"));
>+  }
>+

Make that:
if (mDecoder && mDecoder->IsInfinite()) {


>@@ -478,16 +498,20 @@ void nsBuiltinDecoder::PlaybackEnded()
> 
>   PlaybackPositionChanged();
>   ChangeState(PLAY_STATE_ENDED);
> 
>   if (mElement)  {
>     UpdateReadyStateForData();
>     mElement->PlaybackEnded();
>   }
>+
>+  if (IsInfinite()) {
>+    SetInfinite(PR_FALSE);
>+  }

Add a comment saying that we must do the SetInfinite(PR_FALSE) after we call mElement->PlaybackEnded(), else PlaybackEnded() won't fire the required durationchange event.

> 
> void nsBuiltinDecoderStateMachine::SetDuration(PRInt64 aDuration)
> {
>   NS_ASSERTION(NS_IsMainThread() || mDecoder->OnStateMachineThread(),
>     "Should be on main or state machine thread.");
>   mDecoder->GetReentrantMonitor().AssertCurrentThreadIn();
> 
>+  LOG(PR_LOG_DEBUG, ("Duration set to %d", aDuration));

LOG(PR_LOG_DEBUG, ("%p ...", mDecoder, ...))

This way we can tell the log messages from different media apart when we've got multiple decoders active at the same time.


>+  virtual void SetInfinite(PRBool aInfinite) = 0;
>+  virtual PRBool IsInfinite() = 0;

Add a comment for these methods, and make sure you describe what being "infinite" means.

Comment 19

[Paul Adenot (:padenot)]

Attachment: Patch v2 - added comments + fixes

Comment 20

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

Comment on attachment 542140 [details] [diff] [review]
Patch v2 - added comments + fixes

>+  // A media stream is infinite if :
>+  // - it is indeed infinite (e.g. a webradio),
>+  // - we can't determine the duration for now.
>+  //
>+  // When the media stream ends, we can know the duration, thus the stream is
>+  // no longer considered to be infinite.
>+  virtual void SetInfinite(PRBool aInfinite) = 0;

A stream is infinite when a stream is indeed infinite? Is recursion recursion when it is indeed recursion? ;) Please define what infinite means in concrete terms. For example whether the stream has a Content-Length header, supports byte range requests, etc.

Other than that, looks fine.

Comment 21

[Paul Adenot (:padenot)]

Attachment: patch v3 -- Fixed recursive comment.

Here is a more informative comment :)

Comment 22

[Justin Dolske [:Dolske]]

Comment on attachment 541690 [details] [diff] [review]
Patch v0 - Fixed frontend for this feature

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

::: toolkit/content/widgets/videocontrols.xml
@@ +57,5 @@
>            <![CDATA[
>                var timeString;
> +              var hours = Math.round(time / 3600000);
> +              var mins  = Math.round(time % 3600000 / 60000);
> +              var secs  = Math.round(time % 60000 / 1000);

This isn't correct -- consider a time of 7140000ms (which is 119 minutes, or 1 minute shy of 2 hours)... Math.floor(7140000 / 3600000) = 1, Math.round(7140000 / 3600000) = 2. Similarly, a time 1ms shy of 2 minutes shouldn't show up as "1:60". :)

The right way to fix this is to round |time| to the nearest full-second value. (Or Math.round() for |secs| only, and fixup for the special case of secs==60, but that sounds more complex).

In any case, let's deal with this in another bug!

@@ +541,4 @@
>                  formatTime : function(aTime) {
> +                    if (aTime == Infinity) {
> +                      return "Ukn.";
> +                    }

Checked with UX, and an empty-string would be preferred (and also sidesteps localization ;). But see below.

@@ +545,4 @@
>                      // Format the duration as "h:mm:ss" or "m:ss"
> +                    let hours = Math.round(aTime / 3600000);
> +                    let mins  = Math.round(aTime % 3600000 / 60000);
> +                    let secs  = Math.round(aTime % 60000 / 1000);

Also incorrect.

@@ +561,5 @@
>  
>                  showDuration : function (duration) {
>                      if (isNaN(duration))
>                          duration = this.maxCurrentTimeSeen;
> +                    if (duration == Infinity) {

Hmm. So if I understand things correctly...

Prior to this bug, we would return NaN for .duration both (1) before any metadata has loaded and (2) when we have data but couldn't determine an actual duration for one reason or another. The spec previously said Infinity means the media is definitely unbounded, but was vague about what value should be used when a length couldn't be determined. Our implementation didn't return Infinity in any situation.

With this bugfix, we (1) still return NaN when no metadata has been loaded, but (2) now return Infinity when we have data but can't determine the length of the media (or if the server explicitly told us so? can that happen now?). The spec has been clarified, but there's still no way for page content to know the difference between "couldn't figure out duration" and "definitely an infinite stream".

[Please correct me if I'm mistaken above.]

So, the _minimal_ change needed control-wise should be to replace all(?) instances of "isNaN(duration)" with "isNaN(duration) || duration == Infinity". That should make the controls functionally equivalent to the way they currently work. I think I'd prefer to just do that.

The additional issue you're raised here is what we should show when the stream really is infinite. I think we should handle that in another bug too. :) Mainly because from the controls POV, we still don't know the difference between a really-infinite stream and a broken server. But since we're got more real-world experience with HTML5 media under our belts, maybe that's a case we should stop worrying about? I don't know.

I should also note that the current controls are not at all idea for watching a really-infinite stream. If we can ever reliably detect that, there are things we can do... UX previously did some design sketches here: https://wiki.mozilla.org/Firefox/Projects/Video_Controls

Comment 23

[Jeff Walden [:Waldo]]

Note that |a % b / c| is interpreted as |a % (b / c)|, not |(a % b) / c|.  I think for your cases where |b| is a multiple of |c| it happens to turn out identically, but at the very least it's a mental speed bump to the reader.  I recommend parenthesizing to make order of operations explicit, if indeed this modulus code actually goes anywhere (at a skim of the comment it wasn't clear if those hunks were or not).

Comment 24

[Paul Adenot (:padenot)]

Attachment: Patch v1 - Fix label, removed round()

Justin, I've addressed your remarks in this new patch.

You are right about the way infinite stream are supposed to work.
However, if we stick to replacing all the

> isNaN(duration)

instances by 

> isNaN(duration) || duration == Infinity

the widget breaks, since the max value for the scrubber is not set, and somehow, the |valueChanged| method is not called, causing the widget not to update the current time in the thumb. I fixed that by setting a max value for the scrubber equal to the currentTime (so the thumb is always at the extreme right of the seek bar, which seems correct according to the sketches on the UX page you provided).

I think we cannot find a reliable way to distinguish real infinite stream from broken server though, but I'm in the process of trying to determine the duration sooner in that case (559468, comment 24).

I will create the additional bugs needed and cc you on those.

Comment 25

[Justin Dolske [:Dolske]]

Comment on attachment 544173 [details] [diff] [review]
Patch v1 - Fix label, removed round()

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

::: toolkit/content/widgets/videocontrols.xml
@@ +570,1 @@
>                          return;

This function would be simpler if we don't try to preserve |duration = Infinity|.

I think the only needed change would be:

  let wasInfinite = (duration == Infinity);
  if (isNaN(duration) || wasInfinite)
      duration = this.maxCurrentTimeSeen;
  ...
  let timeString = wasInfinite ? "" : this.formatTime(duration);

@@ +621,1 @@
>                        duration = this.maxCurrentTimeSeen;

The extra condition can't be hit... If duration is NaN, then the extra term will be true. If duration is a real value or Infinity, then isNaN() will be false and short circuit.

Comment 26

[Paul Adenot (:padenot)]

Attachment: Patch v1 - Adressed dolke concerns

Dolke, I've simplified the code quite a bit.

However, I had to add a check to know if the infinity state changed, since there was problems when the duration was finally known at the end of the stream (i.e. if the timeupdate event was fired two times with the same currentTime, and when the duration was Infinity, and then equal to currentTime). Without this extra check, the duration wasn't updated in the controls in about a fifth of the times.

Comment 27

[Justin Dolske [:Dolske]]

(In reply to comment #26)

> However, I had to add a check to know if the infinity state changed, since
> there was problems when the duration was finally known at the end of the
> stream (i.e. if the timeupdate event was fired two times with the same
> currentTime, and when the duration was Infinity, and then equal to
> currentTime). Without this extra check, the duration wasn't updated in the
> controls in about a fifth of the times.

Oh, blah. I guess that's because the the |duration == this.lastDurationSeen| is bailing out thinking there's nothing to do, but that last duration _shown_ is actually different (was blank, now a number).

[Hmm, If you seek backwards after the end, does .duration continue to report the determined length of the video, or does it revert back to Infinity?]

How about we just remove .lastDurationSeen and that check, instead of adding .lastInfiniteState? I don't this optimization helps any more... It's not being called very frequently, and showPosition() already does similar work unconditionally.

Also, since you're here, could you remove the existing "if NaN, duration = max" lines from showPosition(), right before showDuration() is called? It's not causing a problem, but it's clearly superfluous given that showDuration() already does exactly that...

Comment 28

[Paul Adenot (:padenot)]

Attachment: Patch v2 - Code cleaning + dolske remarks.

> [Hmm, If you seek backwards after the end, does .duration continue to report 
> the determined length of the video, or does it revert back to Infinity?]

This is handled by the backend, which gives us the determined length of the video, as expected.

I've also addressed your comments, and removed the NaN check in |showPosition|.

Comment 29

[Justin Dolske [:Dolske]]

Comment on attachment 549068 [details] [diff] [review]
Patch v2 - Code cleaning + dolske remarks.

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

By Jove I think you've got it! Woot!

::: toolkit/content/widgets/videocontrols.xml
@@ +562,5 @@
>                  },
>  
>                  showDuration : function (duration) {
> +                    let isInfinite = (duration == Infinity);
> +                    this.log("Duration is " + duration + "ms.\n");

final nit: no newline needed, since log() adds one because dump() is so dumb. :)

Comment 30

[Paul Adenot (:padenot)]

Attachment: Patch v4 -- Updated to patch against last trunk

Comment 31

[Paul Adenot (:padenot)]

Attachment: Patch v0 - fix mochitests

Comment 32

[Boris Zbarsky [:bzbarsky]]

So which patches need to be checked in here, and in which order to make sure there are no broken changesets when bisecting later?

Comment 33

[Paul Adenot (:padenot)]

https://bug619752.bugzilla.mozilla.org/attachment.cgi?id=549764 (implements the feature), then https://bug619752.bugzilla.mozilla.org/attachment.cgi?id=549068 (adds the tests), then https://bug619752.bugzilla.mozilla.org/attachment.cgi?id=549766 (fixes other test bustage).

Comment 34

[Boris Zbarsky [:bzbarsky]]

Shouldn't that third one then land in the same changeset as the first?

Comment 35

[Paul Adenot (:padenot)]

Oh my bad, I took this bug for another.

https://bug619752.bugzilla.mozilla.org/attachment.cgi?id=549068 is the front-end part of this bug, and https://bug619752.bugzilla.mozilla.org/attachment.cgi?id=549766 is the test update, so I guess they should be in separate patches, but I can merge them as you want.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Well.  If someone is bisecting and builds with just attachment 549068 [details] [diff] [review] applied but not attachment 549766 [details] [diff] [review] applied, will the resulting build pass tests?  If not, then they should probably be merged, yes.

Comment 37

[Boris Zbarsky [:bzbarsky]]

And either way, please put the right attachment order for landing in the status whiteboard, unless the attachment descriptions already make that clear (e.g. part 1, part 2, etc).

Comment 38

[Paul Adenot (:padenot)]

Attachment: Part 1 -- Implementation + test fix

Comment 39

[Paul Adenot (:padenot)]

Attachment: Part 2 -- Front end update

Comment 40

[Boris Zbarsky [:bzbarsky]]

Thanks.

http://hg.mozilla.org/integration/mozilla-inbound/rev/993507024dc6
http://hg.mozilla.org/integration/mozilla-inbound/rev/37e5ad24bc64

Comment 41

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/993507024dc6
http://hg.mozilla.org/mozilla-central/rev/37e5ad24bc64