1119757 - MSE: Treat 0 duration in MP4 init segment as infinity

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P2)

Description

[Jean-Yves Avenard [:jya]]

According to spec:
http://w3c.github.io/media-source/#sourcebuffer-init-segment-received

"Update the duration attribute if it currently equals NaN:

1.If the initialization segment contains a duration:
    Run the duration change algorithm with new duration set to the duration in the initialization segment.
Otherwise:
    Run the duration change algorithm with new duration set to positive Infinity.
"

Most init segment contains 0 as duration. We should handle a duration of 0 as if the init segment didn't contain the duration rather than an explicit 0.

As such, the duration would be set to infinity.

Treating as infinity simplifies and avoid some issues where under most circumstances, a non-zero duration is expected.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: MSE: handle duration of 0 in metadata as infinity

Treat an init segment with a duration of 0 as being Infinity. This is consistent with the w3c spec (which dictates that an init segment with no duration should use infinity) and how IE and Chrome handle it. This causes issue in the web-ref where we timeout however. This appears to expose incorrect handling of playback when the duration is infinity. In particular, it seems to only play the first few seconds and then pause.
For example: http://people.mozilla.org/~jyavenard/tests/mse_mp4/infinityduration.html?eos=0&duration=-1 ; will not play properly (stutters and pause early) while http://people.mozilla.org/~jyavenard/tests/mse_mp4/infinityduration.html?eos=0&duration=20 which will set the original duration to 20s plays just fine, even though there's only 10s of data.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8546687 [details] [diff] [review]
MSE: handle duration of 0 in metadata as infinity

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

::: dom/media/mediasource/MediaSourceDecoder.cpp
@@ +229,5 @@
> +  if (aDuration >= 0) {
> +    mDecoderStateMachine->SetDuration(aDuration * USECS_PER_S);
> +    mMediaSourceDuration = aDuration;
> +  } else {
> +    mMediaSourceDuration = PositiveInfinity<double>();

Why don't we need to notify the state machine of the duration in this case?

Comment 3

[Jean-Yves Avenard [:jya]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> Comment on attachment 8546687 [details] [diff] [review]
> MSE: handle duration of 0 in metadata as infinity
> 
> Review of attachment 8546687 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/mediasource/MediaSourceDecoder.cpp
> @@ +229,5 @@
> > +  if (aDuration >= 0) {
> > +    mDecoderStateMachine->SetDuration(aDuration * USECS_PER_S);
> > +    mMediaSourceDuration = aDuration;
> > +  } else {
> > +    mMediaSourceDuration = PositiveInfinity<double>();
> 
> Why don't we need to notify the state machine of the duration in this case?

Because it would be a no-op: MDSM::SetDuration returns immediately if duration = -1...

Comment 4

[Matt Woodrow (:mattwoodrow)]

(In reply to Jean-Yves Avenard [:jya] from comment #3)
> 
> Because it would be a no-op: MDSM::SetDuration returns immediately if
> duration = -1...

Ok, are you sure we don't need to fix that?

In particular, why does the state machine need to know the duration for finite durations, but for infinite ones we can just not set anything?

Comment 5

[Jean-Yves Avenard [:jya]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> (In reply to Jean-Yves Avenard [:jya] from comment #3)
> > 
> > Because it would be a no-op: MDSM::SetDuration returns immediately if
> > duration = -1...
> 
> Ok, are you sure we don't need to fix that?
> 
> In particular, why does the state machine need to know the duration for
> finite durations, but for infinite ones we can just not set anything?

Yes, I've fixed that and added a way for MDSM to differentiate a duration of -1 vs infinite.
(bug 1119757)

I still have errors showing for another bug that have delayed me pushing those..

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Allow seeking on media with infinite duration

Add support of infinity duration in MDSM; by differentiating an endTime set to -1 (unset) and -1 (infinity)

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: MSE: handle duration of 0 in metadata as infinity

Rebase; and handle infinity explicitly now

Comment 8

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

Comment on attachment 8547236 [details] [diff] [review]
Allow seeking on media with infinite duration

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

I think you can make this simpler by using INT64_MAX to represent +infinity, rather than (mEndTime == -1 && mDurationSet) representing infinity.

::: dom/media/MediaDecoderStateMachine.cpp
@@ +1366,5 @@
>  }
>  
> +int64_t MediaDecoderStateMachine::GetEndTime()
> +{
> +  if (mEndTime == -1 && mDurationSet) {

Can you just initialize mEndTime to -1, and then use a value of -1 to mean uninitialized? Then you don't need mDurationSet to be metadata about your data which is implicit in the data.

@@ +1379,5 @@
>                 "Should be on main or decode thread.");
>    AssertCurrentThreadInMonitor();
>  
>    if (aDuration == -1) {
> +    mDurationSet = false;

mEndTime = INFINITE_DURATION;

::: dom/media/MediaDecoderStateMachine.h
@@ +169,5 @@
>  
>    // Called from the main thread to get the duration. The decoder monitor
>    // must be obtained before calling this. It is in units of microseconds.
>    int64_t GetDuration();
>  

I think you should #define INT64_MAX INFINITE_DURATION, and use that instead of INT64 and -1, interchangeably.

@@ +838,5 @@
>  
>    // Time of the last frame in the media, in microseconds. This is the
>    // end time of the last frame in the media. Accessed on state
>    // machine, decode, and main threads. Access controlled by decoder monitor.
> +  // It will be set to -1 if the duration is infinite

Here you use -1 as infinite, but above you have INT64_MAX as infinite...

Comment 9

[Jean-Yves Avenard [:jya]]

(In reply to Chris Pearce (:cpearce) from comment #8)
> Comment on attachment 8547236 [details] [diff] [review]
> Allow seeking on media with infinite duration
> 
> Review of attachment 8547236 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think you can make this simpler by using INT64_MAX to represent +infinity,
> rather than (mEndTime == -1 && mDurationSet) representing infinity.

To summarise what I wrote on IRC.

Using a different value than -1 would actually make the change much bigger.
MediaDecoder treats a duration of -1 as being infinity.
MDSM handles infinity with mEndTime as -1 which also indicates that we can't seek.

Now we want infinity, *and* we want to seek.

That mEndTime is -1 to mean infinity is tested in various places in MDSM, having two values to both mean infinity is a pain.


> 
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +1366,5 @@
> >  }
> >  
> > +int64_t MediaDecoderStateMachine::GetEndTime()
> > +{
> > +  if (mEndTime == -1 && mDurationSet) {
> 
> Can you just initialize mEndTime to -1, and then use a value of -1 to mean
> uninitialized? Then you don't need mDurationSet to be metadata about your
> data which is implicit in the data.

mEndTime is initialised to -1 already; which means uninitialised in some places, and mean: we don't have a end time and as such it's infinity in another (as well as dependent code such as MediaDecoder)

> 
> @@ +1379,5 @@
> >                 "Should be on main or decode thread.");
> >    AssertCurrentThreadInMonitor();
> >  
> >    if (aDuration == -1) {
> > +    mDurationSet = false;
> 
> mEndTime = INFINITE_DURATION;

no, but setting a duration to -1 has to be kept like it was. As it otherwise would make some code now seekable (and we only want this to be active for new code explicitely setting a duration of infinity)

> 
> ::: dom/media/MediaDecoderStateMachine.h
> @@ +169,5 @@
> >  
> >    // Called from the main thread to get the duration. The decoder monitor
> >    // must be obtained before calling this. It is in units of microseconds.
> >    int64_t GetDuration();
> >  
> 
> I think you should #define INT64_MAX INFINITE_DURATION, and use that instead
> of INT64 and -1, interchangeably.

agreed.

> 
> @@ +838,5 @@
> >  
> >    // Time of the last frame in the media, in microseconds. This is the
> >    // end time of the last frame in the media. Accessed on state
> >    // machine, decode, and main threads. Access controlled by decoder monitor.
> > +  // It will be set to -1 if the duration is infinite
> 
> Here you use -1 as infinite, but above you have INT64_MAX as infinite...

yes... and that's why I added this line of documentation.
Issue is that -1 means infinity in some places, and uninitialized (and error) in others.

Comment 10

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

Comment on attachment 8547236 [details] [diff] [review]
Allow seeking on media with infinite duration

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

OK, let's fix this some other time.

Comment 11

[Jean-Yves Avenard [:jya]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/f41487ad574d
http://hg.mozilla.org/integration/mozilla-inbound/rev/2184ef60ef14

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2184ef60ef14
https://hg.mozilla.org/mozilla-central/rev/f41487ad574d

Comment 13

[Ralph Giles (:rillian)]

Comment on attachment 8547236 [details] [diff] [review]
Allow seeking on media with infinite duration

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent testing, sites more likely to serve flash video.
[Describe test coverage new/current, TBPL]: Landed on m-c.
[Risks and why]: This affects normal mp4 playback, but fixes a nit around duration handling. It's not an isolated change, but has been stable on nightly and lets us pass more tests.
[String/UUID change made/needed]: None.

This request applies to all patches on this bug.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/aa827ff4747c
https://hg.mozilla.org/releases/mozilla-aurora/rev/d592da062369

https://hg.mozilla.org/releases/mozilla-beta/rev/b0c42a7f0dc7
https://hg.mozilla.org/releases/mozilla-beta/rev/3e5d8c21f3a2