1004373 - Can dispatch multiple DecodeMetadata tasks to state machine

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 1

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

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.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 3

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

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.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch

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.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: 1004373-multiple-read-metadata-calls.patch

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.

Comment 7

[Matthew Gregan [: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.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/59df45b49f02

We should uplift this.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/59df45b49f02