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

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

({dev-doc-complete, site-compat})

unspecified
mozilla41
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(13 attachments, 1 obsolete attachment)

3.97 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
16.46 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
44.01 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
5.72 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
5.50 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
14.64 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
8.62 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
3.04 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
2.71 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
3.35 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
1.97 KB, patch
jwwang
: review+
cpearce
: superreview+
Details | Diff | Splinter Review
12.74 KB, patch
jya
: review+
Details | Diff | Splinter Review
41.85 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
Assignee

Description

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

Updated

4 years ago
Depends on: 1163223
Assignee

Updated

4 years ago
Depends on: 1169544
Assignee

Updated

4 years ago
No longer depends on: 1163223, 1169544
Assignee

Updated

4 years ago
Blocks: 1163223
Assignee

Comment 2

4 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.
Attachment #8615729 - Flags: review?(jwwang)
Assignee

Comment 11

4 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.
Attachment #8615738 - Flags: review?(jwwang)
Assignee

Comment 12

4 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.
Attachment #8615739 - Flags: review?(jwwang)
Assignee

Comment 13

4 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.
Attachment #8615741 - Flags: superreview?(cpearce)
Attachment #8615741 - Flags: review?(jwwang)
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 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.
Attachment #8615741 - Flags: superreview?(cpearce) → superreview+
Attachment #8615729 - Flags: review?(jwwang) → review+
Attachment #8615730 - Flags: review?(jwwang) → review+
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.
Attachment #8615731 - Flags: review?(jwwang) → review+
Attachment #8615732 - Flags: review?(jwwang) → review+
Attachment #8615733 - Flags: review?(jwwang) → review+
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?
Attachment #8615734 - Flags: review?(jyavenard)
Attachment #8615734 - Flags: review?(jwwang)
Attachment #8615734 - Flags: feedback+
Attachment #8615735 - Flags: review?(jwwang) → review+
Attachment #8615736 - Flags: review?(jwwang) → review+
Attachment #8615737 - Flags: review?(jwwang) → review+
Attachment #8615738 - Flags: review?(jwwang) → review+
Attachment #8615739 - Flags: review?(jwwang) → review+
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.
(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.
Attachment #8615741 - Flags: review?(jwwang) → review+
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.
Attachment #8615734 - Flags: review?(jyavenard)
(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.
Assignee

Comment 22

4 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.
Attachment #8615734 - Attachment is obsolete: true
Attachment #8616267 - Flags: review?(jyavenard)
Assignee

Updated

4 years ago
Blocks: 1172169
Assignee

Comment 23

4 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
Assignee

Updated

4 years ago
Flags: needinfo?(cpearce)
Assignee

Comment 24

4 years ago
Screw it, let's just do this here and be done with it.
Attachment #8616292 - Flags: review?(cpearce)
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.
Attachment #8616292 - Flags: review?(cpearce) → review+
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.
Flags: needinfo?(cpearce)

Comment 28

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

Comment 30

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

Comment 31

4 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).
Assignee

Updated

4 years ago
No longer blocks: 1172169
Assignee

Updated

4 years ago
Duplicate of this bug: 1172169
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?
Attachment #8616267 - Flags: review?(jyavenard) → review+
Assignee

Comment 35

4 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.
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!
Assignee

Comment 37

4 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.
(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!
You need to log in before you can comment on or make changes to this bug.