Refactor MediaDecoderStateMachine::ScheduleStateMachine

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

Trunk
mozilla31
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Assignee

Description

5 years ago
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).
Assignee

Updated

5 years ago
Assignee: nobody → jwwang
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Assignee

Comment 1

5 years ago
Posted patch part1_InitTimer.patch (obsolete) — Splinter Review
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.
Attachment #8399825 - Flags: review?(cpearce)
Assignee

Comment 2

5 years ago
Posted patch part2_useTimerOnly.patch (obsolete) — Splinter Review
Remove |mDispatchedRunEvent|, |mRunAgain|, and |mIsRunning|. We need only |mTimeout| to know whether and when there is a pending state machine task waiting to run.
Attachment #8399826 - Flags: review?(cpearce)
Assignee

Comment 3

5 years ago
Remove nsRunnable base class for timeout==0 will cover the case of GetStateMachineThread()->Dispatch(this, NS_DISPATCH_NORMAL).
Attachment #8399828 - Flags: review?(cpearce)
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.
Attachment #8399825 - Flags: review?(cpearce) → review+
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.
Attachment #8399826 - Flags: review?(cpearce) → review+
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.
Attachment #8399828 - Flags: review?(cpearce) → review+
Assignee

Comment 8

5 years ago
Minor fixes.
Attachment #8399825 - Attachment is obsolete: true
Attachment #8400343 - Flags: review+
Assignee

Comment 9

5 years ago
rebase on part1.
Attachment #8399826 - Attachment is obsolete: true
Attachment #8400345 - Flags: review+
Assignee

Updated

5 years ago
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.