Closed Bug 1054831 Opened 5 years ago Closed 5 years ago

Improve MediaDecoderStateMachine state transition about decoding metadata

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: jwwang, Assigned: jwwang)

Details

Attachments

(1 file)

|mDispatchedDecodeMetadataTask| is used to prevent decoding metadata task from running twice. However, it is tricky to know when to reset |mDispatchedDecodeMetadataTask|. Before async decoder is introduced, we reset it at exit of MediaDecoderStateMachine::CallDecodeMetadata(). However, things are different after async decoder is involved.

MediaDecoderStateMachine::DecodeMetadata will call MediaDecoderReader::Request{Audio|Video}Data to preroll audio/video samples and MediaDecoderReader::Request{Audio|Video}Data are async functions. Decoding metadata won't be completed until audio/video samples are returned from MediaDecoderStateMachine::On{Audio|Video}Decoded. It is until then we are safe to reset |mDispatchedDecodeMetadataTask|. If we reset |mDispatchedDecodeMetadataTask| at exit of MediaDecoderStateMachine::CallDecodeMetadata() like before, we are risking decoding metadata task might be run again before audio/video samples are returned. And that is exactly bug 1046837 tried to fix.

Therefore, I think it is a good idea to remove |mDispatchedDecodeMetadataTask| and encode the policy that decoding metadata should only run once into the state transition code to make the code clearer and more robust.

The idea is to introduce a new state=DECODER_STATE_DECODING_NONE into MediaDecoderStateMachine. When RunStateMachine() sees this state, it will transition into DECODER_STATE_DECODING_METADATA immediately and call EnqueueDecodeMetadataTask() for decoding metadata. By doing so, most of the code will remain the same and EnqueueDecodeMetadataTask() will never be called again before decoding metadata is finished.

And this design should also properly handle the case of both sync and asyc decoders.
Sounds good. I had a patch to centralize all the state transitions into a single ChangeState(newState) function that enforced all this, but never got around to cleaning it up.
Sounds good. Having a ChangeState() functions will allow us to track state changes.

I was also thinking about adding entry/exit actions to state transition so that we can call EnqueueDecodeMetadataTask() in the entry action of DECODER_STATE_DECODING_METADATA. By doing so, we can remove DECODER_STATE_DECODING_NONE above whose purpose is identical to entry actions.

To err on the caution side, I will proceed step by step on the way of refactoring MediaDecoderStateMachine to make sure each change doesn't regress.
Sounds good.
See comment 0 for the detail.

Try: https://tbpl.mozilla.org/?tree=Try&rev=2783a3ad0c4a
Lets see if it breaks anything.
Assignee: nobody → jwwang
Status: NEW → ASSIGNED
Comment on attachment 8476483 [details] [diff] [review]
1054831_MediaDecoderStateMachine_improve_state_transition_decode_metadata-v1.patch

https://tbpl.mozilla.org/?tree=Try&rev=2783a3ad0c4a
Looks green.
Attachment #8476483 - Flags: review?(cpearce)
Attachment #8476483 - Flags: review?(cpearce) → review+
See comment 5 for the Try results.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ad84c1e95b99
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.