Bobby Holley (:bholley) Assignee
	Description • 9 years ago

I'm splitting this out of bug 1159974 because the stuff related to duration, start time, and end time is all incredibly interdependent and complex. This is going to be quite tricky to get right, but I have some ideas.

All the same, I may wait on this one until I get more stuff converted to the new threading invariants, and possibly even do it last. :-)

	Bobby Holley (:bholley) Assignee
	Comment 1 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6db5b09f4df3

	Bobby Holley (:bholley) Assignee
	Comment 2 • 9 years ago

We need operator != to make state mirroring work properly.

I contemplated using the mIsValid bit on CheckedInt for nullability, but
eventually decided that it's too much of an overload. typedef-ing a Maybe<>
solves things nicely.

The current clash between the constants makes it impossible to do using
|using namespace mozilla::media;| in most cpp files. It would be nice to
put the constants in mozilla::media, but that requires updating a bunch of
files, so I'm not doing it now.

	Bobby Holley (:bholley) Assignee
	Comment 11 • 9 years ago

It would be better for SetDuration to do this, and base it off of the current
MDSM state rather than the cause of the duration change. But this clarifies the
current situation a bit without adding more risk to this patch stack.

	Bobby Holley (:bholley) Assignee
	Comment 12 • 9 years ago

Without this, we get failures in test_TruncatedDuration.html etc. I decided that
integrating this into the precisely correct position in the patch stack was too
much work.

	Bobby Holley (:bholley) Assignee
	Comment 13 • 9 years ago

From the bug it corresponds to, this test was designed to make sure that we don't
read ogg duration by seeking (which causes network activity) when the server
offers the X-Content-Duration header. However, preserving this behavior requires
the ogg reader to be aware of the presence of network duration when it's reading
metadata, which isn't easily accomplished in our new model here. I think both ogg
and X-Content-Duration are sufficiently unimportant at this point that this is an
acceptable regression.

	Jean-Yves Avenard [:jya]
	Comment 14 • 9 years ago

Comment on attachment 8615729 [details] [diff] [review]
Part 1 - Miscellaneous changes to TimeUnit. v1

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

I already have this pending in bug 1165819.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 15 • 9 years ago

Comment on attachment 8615741 [details] [diff] [review]
Part 12 - Fix up test to require the true duration, rather than the fake duration. v1

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

I agree Ogg is not likely to achieve widespread adoption, and so unimportant.

If we removed support for X-Content-Duration, would we need to keep the concept of "network duration"? Our code would be simpler then right? I'd be happy removing X-Content-Duration.

	JW Wang [:jwwang] [:jw_wang]
	Comment 16 • 9 years ago

Comment on attachment 8615731 [details] [diff] [review]
Part 3 - Track "metadata duration" separately and mirror it to MediaDecoderReader. v1

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

::: dom/media/mediasource/TrackBuffer.cpp
@@ +813,1 @@
>    if (!duration) {

I am confused. MediaSourceDecoder::SetInitialDuration() treats |duration==-1| as infinity. However here we treat |duration==0| as infinity. It seems to me we can completely remove this if block.

::: dom/media/ogg/OggReader.cpp
@@ +473,5 @@
>    if (HasAudio() || HasVideo()) {
>      ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
>  
>      MediaResource* resource = mDecoder->GetResource();
> +    if (!mInfo.mMetadataDuration.isSome() && !mDecoder->IsShutdown() &&

replace !isSome() with isNothing() since we prefer positive expressions.

	JW Wang [:jwwang] [:jw_wang]
	Comment 17 • 9 years ago

Comment on attachment 8615734 [details] [diff] [review]
Part 6 - Track explicit (mediasource) duration separately. v1

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +1472,5 @@
> +    // The explicit MediaSource duration is NaN by default, but we need an
> +    // actual value (0) for the MDSM.
> +    double d = mExplicitDuration.Ref().ref();
> +    TimeUnit duration = IsNaN(d) ? TimeUnit() : TimeUnit::FromSeconds(d);
> +    SetDuration(duration.ToMicroseconds());

I will have jya to take a look. Does MediaDecoderStateMachine::SetDuration() treat |aDuration==0| a valid duration and |aDuration==INT64_MAX| as infinity?

	Jean-Yves Avenard [:jya]
	Comment 18 • 9 years ago

Comment on attachment 8615731 [details] [diff] [review]
Part 3 - Track "metadata duration" separately and mirror it to MediaDecoderReader. v1

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

::: dom/media/mediasource/TrackBuffer.cpp
@@ +813,1 @@
>    if (!duration) {

It is confusing...

The metadata read in the trackbuffer is the one reported by the metadata/demuxer. By MSE spec, when no duration is specified it means infinity.

The "initial duration" in MediaSourceDecoder of -1 means NaN (which is what duration should be set at when the medisource object is created).

Every component treats their duration with either a different time or a different meaning for special value.

A call to MediaSourceDecoder::SetInitialDuration with -1 actually means infinity. And will set the MDSM duration value to INT64_MAX.

	Jean-Yves Avenard [:jya]
	Comment 19 • 9 years ago

(In reply to JW Wang [:jwwang] from comment #17)

> I will have jya to take a look. Does MediaDecoderStateMachine::SetDuration()
> treat |aDuration==0| a valid duration and |aDuration==INT64_MAX| as infinity?

that's right.

I couldn't use 0 as meaning infinity, as it would break other MediaDecoderReader that treats the value 0 differently to media source.

	Jean-Yves Avenard [:jya]
	Comment 20 • 9 years ago

Comment on attachment 8615734 [details] [diff] [review]
Part 6 - Track explicit (mediasource) duration separately. v1

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +1472,5 @@
> +    // The explicit MediaSource duration is NaN by default, but we need an
> +    // actual value (0) for the MDSM.
> +    double d = mExplicitDuration.Ref().ref();
> +    TimeUnit duration = IsNaN(d) ? TimeUnit() : TimeUnit::FromSeconds(d);
> +    SetDuration(duration.ToMicroseconds());

Duration will be NaN in the mediasource at creation only, prior any data being loaded. It should never be a NaN, as it means the mediasource object isn't ready for use

this doesn't feel right.

I'm not fully aware of the entire context. But if the value is NaN, it means the mediasource is not ready to read the metadata nor recompute duration.

	JW Wang [:jwwang] [:jw_wang]
	Comment 21 • 9 years ago

(In reply to Jean-Yves Avenard [:jya] from comment #19)
> that's right.
> 
> I couldn't use 0 as meaning infinity, as it would break other
> MediaDecoderReader that treats the value 0 differently to media source.

This is confusing. I wish we have a global definition applied to all of our media code where 0 has a ubiquitous meaning.

	Bobby Holley (:bholley) Assignee
	Comment 22 • 9 years ago

The annoying thing here is that we want to disambiguate _having_ an explicit
duration (a trait of the reader) versus the explicit duration being ready.
This should do the trick for now so that we can get this landed.

	Bobby Holley (:bholley) Assignee
	Comment 23 • 9 years ago

So, my recent try push [1] has a smattering of failures around the contentDuration tests. These failures are racey, and debugging them changes them.

The basic issue is that we have tests that explicitly send an ogg with neither Content-Duration _nor_ Content-Length, and then expect that we'll be able to get the duration somehow. Whether we do or not boils down to whether the entire media stream has been read in by the end of OggReader::ReadMetadata (so that resource->GetLength() stops returning -1 at [2]).

As far as I can tell, nothing in the code actually enforces this, and by making OggReader::ReadMetadata run a bit faster (and acquire fewer locks), I'm surfacing this race condition.

What do you think Chris? Are you ok with just disabling these tests or sending Content-Length for now? Presumably they'll all go away when we get rid of Content-Duration anyhow.


[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=6db5b09f4df3
[2] https://hg.mozilla.org/mozilla-central/annotate/b0315d00af9e/dom/media/ogg/OggReader.cpp#l479

	Bobby Holley (:bholley) Assignee
	Comment 24 • 9 years ago

Screw it, let's just do this here and be done with it.

	Bobby Holley (:bholley) Assignee
	Comment 25 • 9 years ago

Full green try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99086bbcb9c6

\o/

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 26 • 9 years ago

Comment on attachment 8616292 [details] [diff] [review]
Part 13 - Drop Support for Content-Duration. v1

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

I wish more patches could be like this.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 27 • 9 years ago

Brion: FYI, we're removing support for the X-Content-Duration HTTP header from Firefox. AFAICT, it's not used much, and it's a hassle for us to maintain. Sites that care about getting a duration without seeking to the end of stream can index their Oggs.

	Brion Vibber
	Comment 28 • 9 years ago

Note this'll mostly affect audio playback for Wikipedia, since our videos are usually served as webm (and usually have an index if Ogg). I can rig up indexing for audio if we find we need it.

	Pulsebot
	Comment 29 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/89dd8f457bec
https://hg.mozilla.org/integration/mozilla-inbound/rev/149efc9c88cc
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd83b74ed1e6
https://hg.mozilla.org/integration/mozilla-inbound/rev/74cda113cc88
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5fbb5fa03c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/095925624f9a
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7ec50d8a96e
https://hg.mozilla.org/integration/mozilla-inbound/rev/c06038fccdc1
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dab6ccaaab6
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac8ea767163c
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cb6e12a8b89
https://hg.mozilla.org/integration/mozilla-inbound/rev/f80895a195ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/636412252449

	Bobby Holley (:bholley) Assignee
	Comment 30 • 9 years ago

Part 6 has f=jww, and I addressed jya's review comment. Given the large patch stack, I went ahead and landed this now. I'll address any other comments in a prompt followup.

	Bobby Holley (:bholley) Assignee
	Comment 31 • 9 years ago

(Note that the rc4 failure just happened to another try push as well, so it seems unlikely to be related to this change - see bug 930431 comment 32).

	Ryan VanderMeulen [:RyanVM]
	Comment 33 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/89dd8f457bec
https://hg.mozilla.org/mozilla-central/rev/149efc9c88cc
https://hg.mozilla.org/mozilla-central/rev/cd83b74ed1e6
https://hg.mozilla.org/mozilla-central/rev/74cda113cc88
https://hg.mozilla.org/mozilla-central/rev/e5fbb5fa03c9
https://hg.mozilla.org/mozilla-central/rev/095925624f9a
https://hg.mozilla.org/mozilla-central/rev/f7ec50d8a96e
https://hg.mozilla.org/mozilla-central/rev/c06038fccdc1
https://hg.mozilla.org/mozilla-central/rev/8dab6ccaaab6
https://hg.mozilla.org/mozilla-central/rev/ac8ea767163c
https://hg.mozilla.org/mozilla-central/rev/0cb6e12a8b89
https://hg.mozilla.org/mozilla-central/rev/f80895a195ef
https://hg.mozilla.org/mozilla-central/rev/636412252449

	Jean-Yves Avenard [:jya]
	Comment 34 • 9 years ago

Comment on attachment 8616267 [details] [diff] [review]
Part 6 - Track explicit (mediasource) duration separately. v2

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

forgot to submit that one.. and it's after the fact.

::: dom/media/MediaDecoderStateMachine.cpp
@@ +1475,5 @@
> +      // any other duration sources), but the duration isn't ready yet.
> +      return;
> +    }
> +    TimeUnit duration = TimeUnit::FromSeconds(d);
> +    SetDuration(duration.ToMicroseconds());

Couldn't we have used TimeUnit all the way?

	Bobby Holley (:bholley) Assignee
	Comment 35 • 9 years ago

(In reply to Jean-Yves Avenard [:jya] from comment #34)
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +1475,5 @@
> > +      // any other duration sources), but the duration isn't ready yet.
> > +      return;
> > +    }
> > +    TimeUnit duration = TimeUnit::FromSeconds(d);
> > +    SetDuration(duration.ToMicroseconds());
> 
> Couldn't we have used TimeUnit all the way?

It becomes a TimeUnit in part 9.

	Chris Mills [:cmills]
	Comment 36 • 9 years ago

I've mentioned what I think is the net effect to web developers (X-Content-Duration header no longer sent) in the Firefox release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/41#HTTP 

And also included a mention here:

https://developer.mozilla.org/en-US/docs/Web/HTTP/Configuring_servers_for_Ogg_media#Serve_X-Content-Duration_headers

Let me know if you think that is sufficient. Thanks!

	Bobby Holley (:bholley) Assignee
	Comment 37 • 9 years ago

> The X-Content-Duration header is no longer sent to Firefox

I replaced this with "no longer supported", since the header can still be sent by the server, but will just be ignored.

	Chris Mills [:cmills]
	Comment 38 • 9 years ago

(In reply to Bobby Holley (:bholley) from comment #37)
> > The X-Content-Duration header is no longer sent to Firefox
> 
> I replaced this with "no longer supported", since the header can still be
> sent by the server, but will just be ignored.

Ok, great - thanks!

	Kohei Yoshino [:kohei]
	Comment 39 • 9 years ago

Added the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/x-content-duration-header-for-ogg-media-is-no-longer-supported/