Closed Bug 1119757 Opened 5 years ago Closed 5 years ago

MSE: Treat 0 duration in MP4 init segment as infinity

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox36 --- fixed
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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.
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.
Attachment #8546687 - Flags: review?(matt.woodrow)
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
See Also: → 1098030
See Also: → 1096755
See Also: 1098030
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?
(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...
(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?
(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..
Add support of infinity duration in MDSM; by differentiating an endTime set to -1 (unset) and -1 (infinity)
Attachment #8547236 - Flags: review?(cpearce)
Rebase; and handle infinity explicitly now
Attachment #8547237 - Flags: review?(matt.woodrow)
Attachment #8546687 - Attachment is obsolete: true
Attachment #8546687 - Flags: review?(matt.woodrow)
Attachment #8547237 - Flags: review?(matt.woodrow) → review+
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...
Attachment #8547236 - Flags: review?(cpearce) → review-
(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 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.
Attachment #8547236 - Flags: review- → review+
Priority: -- → P2
https://hg.mozilla.org/mozilla-central/rev/2184ef60ef14
https://hg.mozilla.org/mozilla-central/rev/f41487ad574d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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.
Attachment #8547236 - Flags: approval-mozilla-beta?
Attachment #8547236 - Flags: approval-mozilla-aurora?
Attachment #8547236 - Flags: approval-mozilla-beta?
Attachment #8547236 - Flags: approval-mozilla-beta+
Attachment #8547236 - Flags: approval-mozilla-aurora?
Attachment #8547236 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.