1160695 - Clean up duration tracking and use mirroring for cross-thread access

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

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. :-)

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Miscellaneous changes to TimeUnit. v1

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.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Track "network duration" separately and mirror it to the MDSM. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Track "metadata duration" separately and mirror it to MediaDecoderReader. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 4 - Remove unused codepath for off-main-thread MediaSource duration change. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 5 - Make MediaDecoder::DurationChanged take a duration argument rather than reading it back synchronously from the MDSM. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 6 - Track explicit (mediasource) duration separately. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 7 - Track demuxer-estimated duration separately. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 8 - Track observed duration separately. v1

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 9 - Clean up SetDuration and remove negative duration case. v1

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 10 - Clarify the semantics of when we fire DurationChanged. v1

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.

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 11 - Hoist post-duration-change seeking into MediaDecoder where it belongs. v1

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.

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 12 - Fix up test to require the true duration, rather than the fake duration. v1

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.

Comment 14

[Jean-Yves Avenard [:jya]]

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.

Comment 15

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

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.

Comment 16

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 17

[JW Wang [:jwwang] [:jw_wang]]

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?

Comment 18

[Jean-Yves Avenard [:jya]]

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.

Comment 19

[Jean-Yves Avenard [:jya]]

(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.

Comment 20

[Jean-Yves Avenard [:jya]]

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.

Comment 21

[JW Wang [:jwwang] [:jw_wang]]

(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.

Comment 22

[Bobby Holley (:bholley)]

Attachment: Part 6 - Track explicit (mediasource) duration separately. v2

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.

Comment 23

[Bobby Holley (:bholley)]

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

Comment 24

[Bobby Holley (:bholley)]

Attachment: Part 13 - Drop Support for Content-Duration. v1

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

Comment 25

[Bobby Holley (:bholley)]

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

\o/

Comment 26

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

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.

Comment 27

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

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.

Comment 28

[Brion Vibber]

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.

Comment 29

[Pulsebot]

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

Comment 30

[Bobby Holley (:bholley)]

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.

Comment 31

[Bobby Holley (:bholley)]

(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).

Comment 33

[Ryan VanderMeulen [:RyanVM]]

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

Comment 34

[Jean-Yves Avenard [:jya]]

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?

Comment 35

[Bobby Holley (:bholley)]

(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.

Comment 36

[Chris Mills [:cmills]]

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!

Comment 37

[Bobby Holley (:bholley)]

> 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.

Comment 38

[Chris Mills [:cmills]]

(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!

Comment 39

[Kohei Yoshino]

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