Closed Bug 1300497 Opened 3 years ago Closed 3 years ago

Fix some code about the shutdown state of MDSM

Categories

(Core :: Audio/Video: Playback, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(6 files)

No description provided.
Assignee: nobody → jwwang
Blocks: 1295892
Priority: -- → P3
Attachment #8788727 - Flags: review?(kaku)
Attachment #8788728 - Flags: review?(kaku)
Attachment #8788729 - Flags: review?(kaku)
Attachment #8788730 - Flags: review?(kaku)
Attachment #8788731 - Flags: review?(kaku)
Attachment #8788732 - Flags: review?(kaku)
Comment on attachment 8788727 [details]
Bug 1300497. Part 1 - assert we never escape the SHUTDOWN state.

https://reviewboard.mozilla.org/r/77114/#review75472
Attachment #8788727 - Flags: review?(kaku) → review+
Comment on attachment 8788728 [details]
Bug 1300497. Part 2 - constify and fix the comment of IsShutdown().

https://reviewboard.mozilla.org/r/77116/#review75474
Attachment #8788728 - Flags: review?(kaku) → review+
Comment on attachment 8788729 [details]
Bug 1300497. Part 3 - move some code from FinishShutdown() to Shutdown().

https://reviewboard.mozilla.org/r/77118/#review75490
Attachment #8788729 - Flags: review?(kaku) → review+
Comment on attachment 8788730 [details]
Bug 1300497. Part 4 - remove the call to ScheduleStateMachine() before |SetState(DECODER_STATE_SHUTDOWN)| and cancel mDelayedScheduler in Shutdown().

https://reviewboard.mozilla.org/r/77120/#review75492
Attachment #8788730 - Flags: review?(kaku) → review+
Comment on attachment 8788731 [details]
Bug 1300497. Part 5 - run MDSM cycles immediately in the callback of mDelayedScheduler.

https://reviewboard.mozilla.org/r/77122/#review75488

Not sure what doen "scheduling an addition cycle" mean in the comment, what I see is reducing function calls..., and it seems that this patch is not related to "shoutdown"...

Whatever, this is a good clean-up.
Attachment #8788731 - Flags: review?(kaku) → review+
Comment on attachment 8788732 [details]
Bug 1300497. Part 6 - remove the call to |SetState(DECODER_STATE_SHUTDOWN)| from DecodeError().

https://reviewboard.mozilla.org/r/77124/#review75486

::: dom/media/MediaDecoderStateMachine.cpp:2002
(Diff revision 1)
>    if (IsShutdown()) {
>      // Already shutdown.
>      return;
>    }

I think we can also remove this check here beacuse patch part 3 makes it impossible to reach here one the shutdown has begun.
Attachment #8788732 - Flags: review?(kaku) → review+
Comment on attachment 8788731 [details]
Bug 1300497. Part 5 - run MDSM cycles immediately in the callback of mDelayedScheduler.

https://reviewboard.mozilla.org/r/77122/#review75488

It means a call to ScheduleStateMachine() to schedule another cycle to run RunStateMachine(). I was planning to assert RunStateMachine() should happen before shutdown. However, it is not possible with current code changes. I will come back at this issue after doing more refactoring.
Comment on attachment 8788732 [details]
Bug 1300497. Part 6 - remove the call to |SetState(DECODER_STATE_SHUTDOWN)| from DecodeError().

https://reviewboard.mozilla.org/r/77124/#review75718

::: dom/media/MediaDecoderStateMachine.cpp:2002
(Diff revision 1)
>    if (IsShutdown()) {
>      // Already shutdown.
>      return;
>    }

Good catch! Will assert that instead.
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8fcadb9544bb
Part 1 - assert we never escape the SHUTDOWN state. r=kaku
https://hg.mozilla.org/integration/autoland/rev/85c7215cdead
Part 2 - constify and fix the comment of IsShutdown(). r=kaku
https://hg.mozilla.org/integration/autoland/rev/6fcdb8ab059d
Part 3 - move some code from FinishShutdown() to Shutdown(). r=kaku
https://hg.mozilla.org/integration/autoland/rev/78525890f801
Part 4 - remove the call to ScheduleStateMachine() before |SetState(DECODER_STATE_SHUTDOWN)| and cancel mDelayedScheduler in Shutdown(). r=kaku
https://hg.mozilla.org/integration/autoland/rev/da34e47f89b5
Part 5 - run MDSM cycles immediately in the callback of mDelayedScheduler. r=kaku
https://hg.mozilla.org/integration/autoland/rev/afb8c6617510
Part 6 - remove the call to |SetState(DECODER_STATE_SHUTDOWN)| from DecodeError(). r=kaku
You need to log in before you can comment on or make changes to this bug.