Can dispatch multiple DecodeMetadata tasks to state machine

RESOLVED FIXED in mozilla32

Status

()

defect
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

29 Branch
mozilla32
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

I discovered, to my horror, that we can end up calling MediaDecoderStateMachine::EnqueueDecodeMetadataTask() multiple times during a normal load, and thus we can end up dispatching multiple tasks to call MediaDecoderReader::ReadMetadata().

This happens in at least content/media/mediasource/test/ mochitests.

This is happening because if MediaDecoderStateMachine::RunStateMachine() runs multiple times, there's nothing in the DECODING_METADATA case to stop us dispatching another task to decode metadata.

I think we should not dispatch tasks inside MediaDecoderStateMachine::RunStateMachine(), we should instead add a MediaDecoderStateMachine::ChangeState(State) function, which dispatches tasks to the appropriate task queue upon a change in MediaDecoderStateMachine::mState.
I was also thinking about refactoring state machine code into a reusable framework (HTMLMediaElement, MediaDecoder, and MediaDecoderStateMachine all involve state changes) upon which we can build tools for profiling and debugging.
I'd be happy for you to do that. :) I'd like to see your design/plan, once you've had time to put one together.
I thinking there are many existing code for the state pattern. I will try to find one that is usable in our gecko before trying to crafting one on our own.
Assignee

Updated

5 years ago
Blocks: 925870
Posted patch Patch (obsolete) — Splinter Review
I had a much more complicated patch to centralize changes to MediaDecoderStateMachine::mState, but it caused more random orange, so rather than spend time debugging that, and then having to rebase my bug 979104 patches again, here's a simpler fix. I simply add a sentinel to prevent multiple dispatch. Note we're already pretty much do this for the SEEKING case too.

I'll upload my other more complicated patch to a new bug, we can follow up on the clean up there.
Attachment #8417169 - Flags: review?(kinetik)
Comment on attachment 8417169 [details] [diff] [review]
Patch

Whoops, this was based ontop of my bug 979104 patches, I'll need to rebase it. Or unbase it, or whatever.
Attachment #8417169 - Flags: review?(kinetik)
Rebased on m-c. I had to add a flag for the SEEKING case too, as it wasn't there in trunk builds, I'd added it in my bug 979104 patches.
Attachment #8417169 - Attachment is obsolete: true
Attachment #8417175 - Flags: review?(kinetik)
Comment on attachment 8417175 [details] [diff] [review]
1004373-multiple-read-metadata-calls.patch

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

::: content/media/MediaDecoderStateMachine.cpp
@@ +1611,5 @@
>      NS_NewRunnableMethod(this, &MediaDecoderStateMachine::DecodeSeek));
> +  if (NS_SUCCEEDED(rv)) {
> +    mDispatchedDecodeSeekTask = true;
> +  } else {
> +    NS_WARNING("ReadMetadata() failed.");

Dispatch failed here.
Attachment #8417175 - Flags: review?(kinetik) → review+
https://hg.mozilla.org/mozilla-central/rev/59df45b49f02
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.