990356 - Refactor MediaDecoderStateMachine::ScheduleStateMachine

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/32.0.1700.102 Chrome/32.0.1700.102 Safari/537.36

Steps to reproduce:

Motivation:
In MediaDecoderStateMachine::Init(), stateMachinePool is created with thread limit == 1 which guarantee all tasks will run in sequence. With that guarantee, we are able to remove some variables to simplify the code. Also in Bug 907162, a redundant timer task is schedule again despite |mRunAgain| is true which means implicitly a pending task will run soon. Therefore, I would like to simplify the logic of state machine scheduling.

Todo:
1. Remove lazy init of mTimer in MediaDecoderStateMachine::ScheduleStateMachine, since it is almost impossible for the state machine not to dispatch a task with timeout != 0.
2. Remove |mDispatchedRunEvent|, |mRunAgain|, and |mIsRunning|. We need only |mTimeout| to know whether and when there is a pending state machine task waiting to run.
3. Remove nsRunnable base class for timeout==0 will cover the case of GetStateMachineThread()->Dispatch(this, NS_DISPATCH_NORMAL).

Comment 1

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

Attachment: part1_InitTimer.patch

Remove lazy init of mTimer in MediaDecoderStateMachine::ScheduleStateMachine, since it is almost impossible for the state machine not to dispatch a task with timeout != 0.

Comment 2

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

Attachment: part2_useTimerOnly.patch

Remove |mDispatchedRunEvent|, |mRunAgain|, and |mIsRunning|. We need only |mTimeout| to know whether and when there is a pending state machine task waiting to run.

Comment 3

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

Attachment: part3_remove_nsIRunnable.patch

Remove nsRunnable base class for timeout==0 will cover the case of GetStateMachineThread()->Dispatch(this, NS_DISPATCH_NORMAL).

Comment 4

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

try: https://tbpl.mozilla.org/?tree=Try&rev=2fd921cfd39d
No new failures.

Comment 5

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

Comment on attachment 8399825 [details] [diff] [review]
part1_InitTimer.patch

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

::: content/media/MediaDecoderStateMachine.cpp
@@ +1043,5 @@
>  
> +  nsresult rv;
> +  mTimer = do_CreateInstance("@mozilla.org/timer;1", &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  mTimer->SetTarget(GetStateMachineThread());

Please also check the return value of SetTarget(). We didn't before, but we really should.

@@ +2793,3 @@
>                                       this,
>                                       ms,
>                                       nsITimer::TYPE_ONE_SHOT);

I would prefer:

  rv = mTimer->InitWithFuncCallback(...);
  NS_ENSURE_SUCCESS(rv, rv);

  return NS_OK;

As then if the InitWithFuncCallback call fails, you'll get a warning printed to stdout.

Comment 6

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

Comment on attachment 8399826 [details] [diff] [review]
part2_useTimerOnly.patch

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

::: content/media/MediaDecoderStateMachine.cpp
@@ +2667,5 @@
>    resource->Unpin();
>    return res;
>  }
>  
>  nsresult MediaDecoderStateMachine::Run()

The best way to ensure code doesn't run is to remove the code. ;)

Since we no longer need to have a MediaDecoderStateMachine::Run() method, please remove it, and make MediaDecoderStateMachine inherit from nsISupports instead of nsRunnable.

You can use the NS_DECL_THREADSAFE_ISUPPORTS macro to declare the AddRef/Release/QueryInterface functions, and NS_IMPL_ISUPPORTS0(MediaDecoderStateMachine) to implement them.

Comment 7

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

Comment on attachment 8399828 [details] [diff] [review]
part3_remove_nsIRunnable.patch

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

OK, you can disregard my comment about making MediaDecoderStateMachine inherit from nsISupports, this is fine too.

Comment 8

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

Attachment: part1_InitTimer_v2.patch

Minor fixes.

Comment 9

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

Attachment: part2_useTimerOnly_v2.patch

rebase on part1.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/6c27d78bc562
https://hg.mozilla.org/integration/b2g-inbound/rev/166f661850a6
https://hg.mozilla.org/integration/b2g-inbound/rev/8716d29886e7

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6c27d78bc562
https://hg.mozilla.org/mozilla-central/rev/166f661850a6
https://hg.mozilla.org/mozilla-central/rev/8716d29886e7