1054831 - Improve MediaDecoderStateMachine state transition about decoding metadata

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

|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.

Comment 1

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

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.

Comment 2

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

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.

Comment 3

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

Sounds good.

Comment 4

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

Attachment: 1054831_MediaDecoderStateMachine_improve_state_transition_decode_metadata-v1.patch

See comment 0 for the detail.

Try: https://tbpl.mozilla.org/?tree=Try&rev=2783a3ad0c4a
Lets see if it breaks anything.

Comment 5

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

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.

Comment 6

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

See comment 5 for the Try results.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad84c1e95b99

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ad84c1e95b99