Closed Bug 1144519 Opened 5 years ago Closed 5 years ago

Rename and streamline media threading assertions

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(5 files)

I have patches for this. I was going to do it as part of bug 1142336 but it involves some slight behavior changes, so I'm going to land the pieces separately.

Note to self - local branch is streamline_media_assertions. Need to fix assertions that end up firing in test_BufferedSeek.html.
Blocks: MediaMonitor
Blocks: 1148234
Comment on attachment 8584202 [details] [diff] [review]
Part 1 - Rename OnStateMachineThread-like functions to reflect the fact that it's a task queue. v1

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

::: dom/media/MediaDecoder.cpp
@@ +1092,5 @@
>  
>  double MediaDecoder::ComputePlaybackRate(bool* aReliable)
>  {
>    GetReentrantMonitor().AssertCurrentThreadIn();
> +  MOZ_ASSERT(NS_IsMainThread() || OnStateMachineTaskQueue() || OnDecodeThread());

Would be a good time to change all NS_IsMainThread() into IsMainTaskQueue or something. Seeing the main thread is really just a task queue-like.

::: dom/media/MediaDecoderStateMachine.h
@@ +202,5 @@
>  
>    // Functions used by assertions to ensure we're calling things
>    // on the appropriate threads.
>    bool OnDecodeThread() const;
> +  bool OnTaskQueue() const;

I would have liked OnStateMachineTaskQueue better. it's more object that it's not the decode task queue. so many task queues.
As seeing that's what MediaDecoder calls it
Attachment #8584202 - Flags: review?(matt.woodrow) → review+
Attachment #8584203 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8584204 [details] [diff] [review]
Part 3 - Rename MediaDecoderReader::OnDecodeThread to MediaDecoderReader::OnTaskQueue. v1

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

::: dom/media/MediaDecoderReader.h
@@ +111,5 @@
>    virtual nsRefPtr<ShutdownPromise> Shutdown();
>  
>    MediaTaskQueue* EnsureTaskQueue();
>  
> +  virtual bool OnTaskQueue()

In the same vein as my suggestion (and it's just that) of the first patch OnDecodeTaskQueue would seem more appropriate ; it makes it obvious looking at the code in a glance.

So we have OnDecode* and OnStateMachine* irrespective on where we are. Giving consistency across all files.
Attachment #8584204 - Flags: review?(matt.woodrow) → review+
Attachment #8584205 - Flags: review?(matt.woodrow) → review+
Attachment #8584206 - Flags: review?(matt.woodrow) → review+
(In reply to Jean-Yves Avenard [:jya] from comment #8)
> ::: dom/media/MediaDecoder.cpp
> @@ +1092,5 @@
> >  
> >  double MediaDecoder::ComputePlaybackRate(bool* aReliable)
> >  {
> >    GetReentrantMonitor().AssertCurrentThreadIn();
> > +  MOZ_ASSERT(NS_IsMainThread() || OnStateMachineTaskQueue() || OnDecodeThread());
> 
> Would be a good time to change all NS_IsMainThread() into IsMainTaskQueue or
> something. Seeing the main thread is really just a task queue-like.

I think that would be super confusing, especially for people coming from other parts of Gecko - the main thread totally isn't a task queue, and there's lots of ways that nsIThread behaves differently from nsITaskQueue. 

> I would have liked OnStateMachineTaskQueue better. it's more object that
> it's not the decode task queue. so many task queues.
> As seeing that's what MediaDecoder calls it

The idea here is that, long-term, this whole business about asserting that we're on thread X for method Foo::Bar and thread Y for method Foo::Baz is going to mostly go away. A given class will primarily run on a single thread (MediaDecoder on main thread, MDSM on state machine thread, Reader on decode thread, etc). OnTaskQueue/GetTaskQueue will return the task queue that the object is supposed to be operating on, and so it actually makes things _more_ consistent across files, because every method just begins with MOZ_ASSERT(OnTaskQueue()) regardless of whether it's a reader or a state machine.
Blocks: 1145686
You need to log in before you can comment on or make changes to this bug.