Closed Bug 1159974 Opened 4 years ago Closed 4 years ago

Use mirroring for StateMachineParams

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(6 files)

The stuff in MediaDecoderStateMachine::SetStateMachineParameters accounts for a lot of remaining cross-thread access, and is ripe for mirroring.
Depends on: 1159987
Blocks: 1160695
There are a handful of methods where we can't yet assert this. This bug will
tackle several of them.
Attachment #8600625 - Flags: review?(jwwang)
Attachment #8600626 - Flags: review?(jwwang)
Attachment #8600627 - Flags: review?(jwwang)
Attachment #8600628 - Flags: review?(jwwang)
While we're at it, I figured it was time to experiment with lambdas. :-)
Attachment #8600629 - Flags: review?(jwwang)
Attachment #8600625 - Flags: review?(jwwang) → review+
Attachment #8600626 - Flags: review?(jwwang) → review+
Attachment #8600627 - Flags: review?(jwwang) → review+
Attachment #8600628 - Flags: review?(jwwang) → review+
Comment on attachment 8600630 [details] [diff] [review]
Part 6 - Dispatch SetMinimizePrerollUntilPlabackStarts. v1

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

::: dom/media/MediaDecoderStateMachine.h
@@ +409,5 @@
> +      self->mMinimizePreroll = true;
> +
> +      // Make sure that this arrives before playback starts, otherwise this won't
> +      // have the intended effect.
> +      MOZ_DIAGNOSTIC_ASSERT(self->mPlayState != MediaDecoder::PLAY_STATE_PLAYING);

Shouldn't we assert |mPlayState == MediaDecoder::PLAY_STATE_LOADING| which is the initial state? However, we must be wary of the ordering issue where this lambdas must be run before self->mPlayState is updated from the canonical value. Can we also mirror mMinimizePreroll to free from the worries?
Attachment #8600630 - Flags: review?(jwwang) → review+
Attachment #8600629 - Flags: review?(jwwang) → review+
(In reply to JW Wang [:jwwang] from comment #9)
> Shouldn't we assert |mPlayState == MediaDecoder::PLAY_STATE_LOADING| which
> is the initial state?

Sure.

> However, we must be wary of the ordering issue where
> this lambdas must be run before self->mPlayState is updated from the
> canonical value. Can we also mirror mMinimizePreroll to free from the
> worries?

I considered doing that, but mirroring seems like overkill given the current code. Moreover, I don't think it would actually help, because we don't have logic to handle preroll-minimization-after-play. The assert should tell us if it's ever a problem.
You need to log in before you can comment on or make changes to this bug.