Mirror duration from the MDSM to the MediaDecoder

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Blocks: 1 bug)

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(8 attachments)

(Assignee)

Description

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

Updated

4 years ago
Blocks: 1148673
(Assignee)

Updated

4 years ago
Depends on: 1173001
(Assignee)

Updated

4 years ago
Depends on: 1173641
(Assignee)

Comment 2

4 years ago
Created attachment 8623373 [details] [diff] [review]
Part 1 - Require Manual disconnection for all mirrors. v1
Attachment #8623373 - Flags: review?(jwwang)
(Assignee)

Comment 3

4 years ago
Created attachment 8623374 [details] [diff] [review]
Part 2 - Track the MDSM's duration as a TimeUnit and eliminate the separate concept of 'end time'. v1
Attachment #8623374 - Flags: review?(jwwang)
(Assignee)

Comment 4

4 years ago
Created attachment 8623375 [details] [diff] [review]
Part 3 - Switch MediaDecoder's mDuration represenation to a double. v1
Attachment #8623375 - Flags: review?(jwwang)
(Assignee)

Comment 5

4 years ago
Created attachment 8623376 [details] [diff] [review]
Part 4 - Mirror duration from the MDSM to the MediaDecoder. v1
Attachment #8623376 - Flags: review?(jwwang)
(Assignee)

Comment 6

4 years ago
Created attachment 8623377 [details] [diff] [review]
Part 5 - Mirror duration from the MDSM to the MediaDecoderReader and remove MDSM::GetDuration. v1
Attachment #8623377 - Flags: review?(jwwang)
(Assignee)

Comment 7

4 years ago
Created attachment 8623378 [details] [diff] [review]
Part 6 - Route mExplicitDuration directly from the mediasource code to MediaDecoder, and stop passing an argument to DurationChanged. v1
Attachment #8623378 - Flags: review?(jwwang)
(Assignee)

Comment 8

4 years ago
Created attachment 8623379 [details] [diff] [review]
Part 7 - Watch mStateMachineDuration, and stop manually firing DurationChanged. v1
Attachment #8623379 - Flags: review?(jwwang)
(Assignee)

Comment 9

4 years ago
Created attachment 8623380 [details] [diff] [review]
Part 8 - Mark WPT as succeeding. v1
Attachment #8623380 - Flags: review?(jwwang)
(Assignee)

Updated

4 years ago
Assignee: nobody → bobbyholley
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?
Attachment #8623373 - Flags: review?(jwwang) → review+
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!
Attachment #8623374 - Flags: review?(jwwang) → review+
Attachment #8623375 - Flags: review?(jwwang) → review+
Attachment #8623376 - Flags: review?(jwwang) → review+
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?
Attachment #8623377 - Flags: review?(jwwang) → review+
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?
Attachment #8623379 - Flags: review?(jwwang) → review+
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.
Attachment #8623380 - Flags: review?(jwwang) → review?(jyavenard)
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.
Attachment #8623380 - Flags: review?(jyavenard) → review+
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.
(Assignee)

Comment 17

4 years ago
(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 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.
Attachment #8623378 - Flags: review?(jwwang) → review+
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().
(Assignee)

Comment 21

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

Updated

4 years ago
Blocks: 1175768

Updated

4 years ago
Depends on: 1179110

Updated

3 years ago
Depends on: 1222866
Depends on: 1315631
You need to log in before you can comment on or make changes to this bug.