Move DecodedStreamData from MediaDecoder to MediaDecoderStateMachine

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(6 attachments)

(Assignee)

Updated

3 years ago
Depends on: 1163469
(Assignee)

Updated

3 years ago
Depends on: 1163474
(Assignee)

Updated

3 years ago
Depends on: 1163489
(Assignee)

Updated

3 years ago
Depends on: 1163497
(Assignee)

Updated

3 years ago
Depends on: 1163924
(Assignee)

Comment 2

3 years ago
Created attachment 8610449 [details] [diff] [review]
1163467_part1_refactor_RecreateData-v1.patch

Part 1 - refactor DecodedStream::RecreateData() which is responsible for destroying DecodedStreamData if necessary.
Assignee: nobody → jwwang
Status: NEW → ASSIGNED
Attachment #8610449 - Flags: review?(roc)
(Assignee)

Comment 3

3 years ago
Created attachment 8610450 [details] [diff] [review]
1163467_part2_rename_UpdateDecodedStream-v1.patch

Part 2 - rename MediaDecoder::UpdateDecodedStream to MediaDecoder::UpdateStreamBlockingForPlayState.
Attachment #8610450 - Flags: review?(roc)
(Assignee)

Comment 4

3 years ago
Created attachment 8610453 [details] [diff] [review]
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().
Attachment #8610453 - Flags: review?(roc)
(Assignee)

Comment 5

3 years ago
Created attachment 8610455 [details] [diff] [review]
1163467_part4_move_MediaDecoder_UpdateStreamBlockingForStateMachinePlaying-v1.patch

Part 4 - move MediaDecoder::UpdateStreamBlockingForStateMachinePlaying to MediaDecoderStateMachine.
Attachment #8610455 - Flags: review?(roc)
(Assignee)

Comment 6

3 years ago
Created attachment 8610456 [details] [diff] [review]
1163467_part5_move_MediaDecoder_UpdateStreamBlockingForPlayState-v1.patch

Part 5 - move MediaDecoder::UpdateStreamBlockingForPlayState to MediaDecoderStateMachine.
Attachment #8610456 - Flags: review?(roc)
(Assignee)

Comment 7

3 years ago
Created attachment 8610458 [details] [diff] [review]
1163467_part6_refactor_MediaDecoder_AddOutputStream-v1.patch

Part 6 - refactor MediaDecoder::AddOutputStream and move related code to MediaDecoderStateMachine.
Attachment #8610458 - Flags: review?(roc)
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();
}
Attachment #8610449 - Flags: review?(roc) → review+
Comment on attachment 8610458 [details] [diff] [review]
1163467_part6_refactor_MediaDecoder_AddOutputStream-v1.patch

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

Excellent!
Attachment #8610458 - Flags: review?(roc) → review+
(Assignee)

Comment 10

3 years ago
Thanks for the review!
This added more main-thread monitor-dependent methods to MediaDecoderStateMachine. Please don't do that in the future!
Depends on: 1178691
(Assignee)

Comment 14

3 years ago
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.
See bug 1178691. Specifically, we now access thread-bound state in IsPlaying etc.
You need to log in before you can comment on or make changes to this bug.