1343458 - Label runnables in dom/media/VideoFrameContainer.cpp

Status: RESOLVED FIXED

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

Comment 1

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

Attachment: Bug 1343458 - Label runnables in dom/media/VideoFrameContainer.cpp.

Review commit: https://reviewboard.mozilla.org/r/116478/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116478/

Comment 2

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

Comment on attachment 8842709 [details]
Bug 1343458 - Label runnables in dom/media/VideoFrameContainer.cpp.

https://reviewboard.mozilla.org/r/116478/#review118584

As far as I can tell, what you did here is assign these runnables to a tabgroup, you didn't actually label the runnables, unless I missed something? So either r+ with the changes I suggest to ensure the runnables are labled, or an explanation and/or the commit message adjusted to ensure it reflects what your patch does.

::: dom/media/VideoFrameContainer.cpp:177
(Diff revision 1)
>                                            newImages.LastElement().mFrameID);
>    }
>  
>    SetCurrentFramesLocked(mLastPlayedVideoFrame.GetIntrinsicSize(), images);
>    nsCOMPtr<nsIRunnable> event =
>      new VideoFrameContainerInvalidateRunnable(this);

VideoFrameContainerInvalidateRunnable inherits from mozilla::Runnable, which implements nsINamed, so you should be able to call SetNamed() in its constructor to set a distinctive name.

::: dom/media/VideoFrameContainer.cpp:249
(Diff revision 1)
>      RefPtr<VideoFrameContainer> self = this;
>      PrincipalHandle principalHandle = mPendingPrincipalHandle;
>      mLastPrincipalHandle = mPendingPrincipalHandle;
>      mPendingPrincipalHandle = PRINCIPAL_HANDLE_NONE;
>      mFrameIDForPendingPrincipalHandle = 0;
> -    NS_DispatchToMainThread(NS_NewRunnableFunction([self, principalHandle]() {
> +    mMainThread->Dispatch(NS_NewRunnableFunction([self, principalHandle]() {

It seems we should be using the variant of NS_NewRunnableFunction that takes a name argument going forward?

https://dxr.mozilla.org/mozilla-central/rev/d29f84406483c721a13cf9a52936ecced0c5c98a/xpcom/threads/nsThreadUtils.h#437

Comment 3

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

Comment on attachment 8842709 [details]
Bug 1343458 - Label runnables in dom/media/VideoFrameContainer.cpp.

https://reviewboard.mozilla.org/r/116478/#review118584

By labeling a runnable, it means to associate a special attribue with the runnable so its execution can be prioritized and delayed. This is done by dispatching a runnable through some utility class. See https://wiki.mozilla.org/Quantum/DOM#Q1:_What_is_the_impact_of_leaving_a_runnable_unlabeled.3F for more details. Giving a runnable a name however is another kind of labeling which doesn't change the execution order of runnables and is for debugging purpose only.

Comment 4

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

Comment on attachment 8842709 [details]
Bug 1343458 - Label runnables in dom/media/VideoFrameContainer.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116478/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6874dcb3e095
Label runnables in dom/media/VideoFrameContainer.cpp. r=cpearce

Comment 6

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

Thanks for explaining.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6874dcb3e095