1172264 - Mirror duration from the MDSM to the MediaDecoder

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

This will ideally let us eliminate the manual firing of MediaDecoder::DurationChanged that we have now. Once we have bug 1163223, we can do this.

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Require Manual disconnection for all mirrors. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Track the MDSM's duration as a TimeUnit and eliminate the separate concept of 'end time'. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Switch MediaDecoder's mDuration represenation to a double. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 4 - Mirror duration from the MDSM to the MediaDecoder. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 5 - Mirror duration from the MDSM to the MediaDecoderReader and remove MDSM::GetDuration. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 6 - Route mExplicitDuration directly from the mediasource code to MediaDecoder, and stop passing an argument to DurationChanged. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 7 - Watch mStateMachineDuration, and stop manually firing DurationChanged. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 8 - Mark WPT as succeeding. v1

Comment 10

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

Comment on attachment 8623373 [details] [diff] [review]
Part 1 - Require Manual disconnection for all mirrors. v1

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

::: dom/media/StateMirroring.h
@@ +286,5 @@
> +    // As a member of complex objects, a Mirror<T> may be destroyed on a
> +    // different thread than its owner, or late in shutdown during CC. Given
> +    // that, we require manual disconnection so that callers can put things in
> +    // the right place.
> +    MOZ_DIAGNOSTIC_ASSERT(!mImpl->IsConnected());

How do we ensure no data race when Mirror is deleted on a thread other than the owner thread?

Comment 11

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

Comment on attachment 8623374 [details] [diff] [review]
Part 2 - Track the MDSM's duration as a TimeUnit and eliminate the separate concept of 'end time'. v1

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

Nice!

Comment 12

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

Comment on attachment 8623377 [details] [diff] [review]
Part 5 - Mirror duration from the MDSM to the MediaDecoderReader and remove MDSM::GetDuration. v1

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

::: dom/media/MediaDecoderReader.h
@@ +81,5 @@
>    // destroyed.
>    explicit MediaDecoderReader(AbstractMediaDecoder* aDecoder, MediaTaskQueue* aBorrowedTaskQueue = nullptr);
>  
> +  // Does any spinup that needs to happen on this task queue. This may run
> +  // before Init().

How can this be run before Init() when we have tail dispatcher?

Comment 13

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

Comment on attachment 8623378 [details] [diff] [review]
Part 6 - Route mExplicitDuration directly from the mediasource code to MediaDecoder, and stop passing an argument to DurationChanged. v1

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

::: dom/media/MediaDecoder.cpp
@@ +1102,5 @@
>  
> +  // See https://www.w3.org/Bugs/Public/show_bug.cgi?id=28822 for a discussion
> +  // of whether we should fire durationchange on explicit infinity.
> +  if (mOwner && mFiredMetadataLoaded &&
> +      (!mozilla::IsInfinite<double>(mDuration) || mExplicitDuration.Ref().isSome())) {

Why do we fire "durationchange" if we have mExplicitDuration in spite of infinite mDuration?

Comment 14

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

Comment on attachment 8623380 [details] [diff] [review]
Part 8 - Mark WPT as succeeding. v1

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

Jya is more familiar with MSE tests.

Comment 15

[Jean-Yves Avenard [:jya]]

Comment on attachment 8623380 [details] [diff] [review]
Part 8 - Mark WPT as succeeding. v1

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

Hopefully they aren't going to intermittently timeout anymore.

Comment 16

[Jean-Yves Avenard [:jya]]

Comment on attachment 8623380 [details] [diff] [review]
Part 8 - Mark WPT as succeeding. v1

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

::: testing/web-platform/meta/media-source/mediasource-duration.html.ini
@@ +12,5 @@
>  
>    [Test setting same duration multiple times does not fire duplicate durationchange]
> +    expected:
> +      if os == "linux": FAIL # Duration estimates wrong for webm - bug 1065207
> +      if (os == "win") and (version == "5.1.2600"): FAIL # Same as above

You need the trailing CR.

Comment 17

[Bobby Holley (:bholley)]

(In reply to JW Wang [:jwwang] from comment #10)
> How do we ensure no data race when Mirror is deleted on a thread other than
> the owner thread?

I don't understand this question - the point here is that the caller is always required to tear down the mirror on the owner thread (before the owner thread goes away), so that it doesn't matter which thread the object is destroyed on.

(In reply to JW Wang [:jwwang] from comment #12)
> How can this be run before Init() when we have tail dispatcher?

Fair point - I'll update the comment.

(In reply to JW Wang [:jwwang] from comment #13)
> > +  // See https://www.w3.org/Bugs/Public/show_bug.cgi?id=28822 for a discussion
> > +  // of whether we should fire durationchange on explicit infinity.
> > +  if (mOwner && mFiredMetadataLoaded &&
> > +      (!mozilla::IsInfinite<double>(mDuration) || mExplicitDuration.Ref().isSome())) {
> 
> Why do we fire "durationchange" if we have mExplicitDuration in spite of
> infinite mDuration?

It's explained in the comment that you quote. ;-)

Assuming that the patch was unobjectionable aside from that, I'm going to push this rpending to avoid losing a day. I'll of course address any further feedback immediately upon arrival.

I apologize for my continued bending of the rules here, but I have exactly 3 days left to get all this threading model stuff sorted out, and we're very, very close.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4f44192009b
https://hg.mozilla.org/integration/mozilla-inbound/rev/394c85b4a06b
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ead3466f84a
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc8f5c62dbef
https://hg.mozilla.org/integration/mozilla-inbound/rev/645564f3f35f
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a1fd02713e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/549275b488a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5eb0b1fcf39

Comment 19

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

Comment on attachment 8623378 [details] [diff] [review]
Part 6 - Route mExplicitDuration directly from the mediasource code to MediaDecoder, and stop passing an argument to DurationChanged. v1

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

::: dom/media/MediaDecoder.cpp
@@ +1102,5 @@
>  
> +  // See https://www.w3.org/Bugs/Public/show_bug.cgi?id=28822 for a discussion
> +  // of whether we should fire durationchange on explicit infinity.
> +  if (mOwner && mFiredMetadataLoaded &&
> +      (!mozilla::IsInfinite<double>(mDuration) || mExplicitDuration.Ref().isSome())) {

I see. It makes sense to me.

Comment 20

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

Comment on attachment 8623373 [details] [diff] [review]
Part 1 - Require Manual disconnection for all mirrors. v1

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

::: dom/media/StateMirroring.h
@@ +286,5 @@
> +    // As a member of complex objects, a Mirror<T> may be destroyed on a
> +    // different thread than its owner, or late in shutdown during CC. Given
> +    // that, we require manual disconnection so that callers can put things in
> +    // the right place.
> +    MOZ_DIAGNOSTIC_ASSERT(!mImpl->IsConnected());

If mImpl->IsConnected() reads mCanonical on a non-owner thread, there will be data race since mCanonical is written in the owner thread in DisconnectIfConnected().

However, I believe our media code always ensures DisconnectIfConnected() "happen before" ~Mirror().

Comment 21

[Bobby Holley (:bholley)]

(In reply to JW Wang [:jwwang] from comment #20)
> If mImpl->IsConnected() reads mCanonical on a non-owner thread, there will
> be data race since mCanonical is written in the owner thread in
> DisconnectIfConnected().
> 
> However, I believe our media code always ensures DisconnectIfConnected()
> "happen before" ~Mirror().

Right. The assertion is racey only in the case where we get it wrong. :-)

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f4f44192009b
https://hg.mozilla.org/mozilla-central/rev/394c85b4a06b
https://hg.mozilla.org/mozilla-central/rev/3ead3466f84a
https://hg.mozilla.org/mozilla-central/rev/bc8f5c62dbef
https://hg.mozilla.org/mozilla-central/rev/645564f3f35f
https://hg.mozilla.org/mozilla-central/rev/5a1fd02713e2
https://hg.mozilla.org/mozilla-central/rev/549275b488a7
https://hg.mozilla.org/mozilla-central/rev/a5eb0b1fcf39