1163467 - Move DecodedStreamData from MediaDecoder to MediaDecoderStateMachine

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

Per comment at https://hg.mozilla.org/mozilla-central/file/617dbce26726/dom/media/MediaDecoderStateMachine.cpp#l456.

Comment 1

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

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bb9223e4f47

Comment 2

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

Attachment: 1163467_part1_refactor_RecreateData-v1.patch

Part 1 - refactor DecodedStream::RecreateData() which is responsible for destroying DecodedStreamData if necessary.

Comment 3

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

Attachment: 1163467_part2_rename_UpdateDecodedStream-v1.patch

Part 2 - rename MediaDecoder::UpdateDecodedStream to MediaDecoder::UpdateStreamBlockingForPlayState.

Comment 4

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

Attachment: 1163467_part3_move_AddOutputStream_below_Load-v1.patch

Part 3 - move AddOutputStream code below MediaDecoder::Load().

Thanks to tail dispatcher, MDSM cycles won't start running until current event cycle is finished. We are safe to move AddOutputStream code below MediaDecoder::Load() call and simplify the code in MediaDecoder::AddOutputStream().

Comment 5

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

Attachment: 1163467_part4_move_MediaDecoder_UpdateStreamBlockingForStateMachinePlaying-v1.patch

Part 4 - move MediaDecoder::UpdateStreamBlockingForStateMachinePlaying to MediaDecoderStateMachine.

Comment 6

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

Attachment: 1163467_part5_move_MediaDecoder_UpdateStreamBlockingForPlayState-v1.patch

Part 5 - move MediaDecoder::UpdateStreamBlockingForPlayState to MediaDecoderStateMachine.

Comment 7

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

Attachment: 1163467_part6_refactor_MediaDecoder_AddOutputStream-v1.patch

Part 6 - refactor MediaDecoder::AddOutputStream and move related code to MediaDecoderStateMachine.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8610449 [details] [diff] [review]
1163467_part1_refactor_RecreateData-v1.patch

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

::: dom/media/DecodedStream.cpp
@@ +230,2 @@
>  
> +  aGraph = aGraph ? aGraph : mData->mStream->Graph();

if (!aGraph) {
  aGraph = mData->mStream->Graph();
}

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8610458 [details] [diff] [review]
1163467_part6_refactor_MediaDecoder_AddOutputStream-v1.patch

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

Excellent!

Comment 10

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

Thanks for the review!

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0df38f5ec5b
https://hg.mozilla.org/integration/mozilla-inbound/rev/079326caec09
https://hg.mozilla.org/integration/mozilla-inbound/rev/092ce739460d
https://hg.mozilla.org/integration/mozilla-inbound/rev/8563a422f957
https://hg.mozilla.org/integration/mozilla-inbound/rev/68285bdfa4d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bda9d3fec3b

Comment 12

[Ryan VanderMeulen (PTO, back July 27)]

https://hg.mozilla.org/mozilla-central/rev/f0df38f5ec5b
https://hg.mozilla.org/mozilla-central/rev/079326caec09
https://hg.mozilla.org/mozilla-central/rev/092ce739460d
https://hg.mozilla.org/mozilla-central/rev/8563a422f957
https://hg.mozilla.org/mozilla-central/rev/68285bdfa4d7
https://hg.mozilla.org/mozilla-central/rev/1bda9d3fec3b

Comment 13

[Bobby Holley (:bholley)]

This added more main-thread monitor-dependent methods to MediaDecoderStateMachine. Please don't do that in the future!

Comment 14

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

Can you indicate which piece of the code creating more dependency? This bug is basically move part of the stream-related code to another file.

Comment 15

[Bobby Holley (:bholley)]

See bug 1178691. Specifically, we now access thread-bound state in IsPlaying etc.