1392217 - Implement VR thread for submitting frames

Status: RESOLVED FIXED

(Core :: WebVR, enhancement, P1)

Comment 1

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1392217 - Part 1: Refactor the VR vibrate thread to be as a VRThread and manage its lifetime;

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

Comment 2

[Code Review Bot [:reviewbot]]

Comment on attachment 8928459 [details]
Bug 1392217 - Part 1: Refactor the VR vibrate thread to be as a VRThread and manage its lifetime;

https://reviewboard.mozilla.org/r/199710/#review204794


C/C++ static analysis found 3 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: gfx/vr/VRThread.h:54
(Diff revision 1)
> +class VRThread final
> +{
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(VRListenerThreadHolder)
> +
> +public:
> +  VRThread(const nsCString& aName);

Error: Bad implicit conversion constructor for 'vrthread' [clang-tidy: mozilla-implicit-constructor]

::: gfx/vr/VRThread.h:54
(Diff revision 1)
> +class VRThread final
> +{
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(VRListenerThreadHolder)
> +
> +public:
> +  VRThread(const nsCString& aName);

Error: Bad implicit conversion constructor for 'vrthread' [clang-tidy: mozilla-implicit-constructor]

::: gfx/vr/VRThread.cpp:119
(Diff revision 1)
>    return VRListenerThread() &&
>  		 VRListenerThread()->thread_id() == PlatformThread::CurrentId();
>  }
>  
> -} // namespace gfx
> -} // namespace mozilla
> +// VRThread lifetime is 30 secs by default
> +VRThread::VRThread(const nsCString& aName)

Error: Bad implicit conversion constructor for 'vrthread' [clang-tidy: mozilla-implicit-constructor]

Comment 3

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1392217 - Part 2: Spawn a VR submit thread in VRDisplayHost;

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

Comment 4

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8928459 [details]
Bug 1392217 - Part 1: Refactor the VR vibrate thread to be as a VRThread and manage its lifetime;

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

Comment 5

[Daosheng Mu[:daoshengmu]]

Try looks good, https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5a8be6f84842e1854648e1158469cab49b6fa20.

This bug needs to wait for Bug 1404534 lands.

Comment 6

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8928459 [details]
Bug 1392217 - Part 1: Refactor the VR vibrate thread to be as a VRThread and manage its lifetime;

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

Comment 7

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8929378 [details]
Bug 1392217 - Part 2: Spawn a VR submit thread in VRDisplayHost;

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

Comment 8

[Code Review Bot [:reviewbot]]

Comment on attachment 8928459 [details]
Bug 1392217 - Part 1: Refactor the VR vibrate thread to be as a VRThread and manage its lifetime;

https://reviewboard.mozilla.org/r/199710/#review210540


C/C++ static analysis found 2 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: gfx/vr/VRThread.cpp:148
(Diff revision 3)
> +    RefPtr<Runnable> runnable =
> +      NewRunnableMethod<TimeStamp>(
> +        "gfx::VRThread::CheckLife", this, &VRThread::CheckLife, TimeStamp::Now());
> +    // Post it to the main thread for tracking the lifetime.
> +    nsCOMPtr<nsIThread> mainThread;
> +    rv = NS_GetMainThread(getter_AddRefs(mainThread));

Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

::: gfx/vr/VRThread.cpp:211
(Diff revision 3)
> +    RefPtr<Runnable> runnable =
> +      NewRunnableMethod<TimeStamp>(
> +        "gfx::VRThread::CheckLife", this, &VRThread::CheckLife, TimeStamp::Now());
> +    // Post it to the main thread for tracking the lifetime.
> +    nsCOMPtr<nsIThread> mainThread;
> +    nsresult rv = NS_GetMainThread(getter_AddRefs(mainThread));

Warning: Value stored to 'rv' during its initialization is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

Comment 9

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8928459 [details]
Bug 1392217 - Part 1: Refactor the VR vibrate thread to be as a VRThread and manage its lifetime;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199710/diff/3-4/

Comment 10

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8929378 [details]
Bug 1392217 - Part 2: Spawn a VR submit thread in VRDisplayHost;

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

Comment 11

[Daosheng Mu[:daoshengmu]]

Try looks good, https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b26545926d5763065b14c32a6de44ce46c219fb

Comment 12

[:kip (Kearwood Gilbert)]

Comment on attachment 8928459 [details]
Bug 1392217 - Part 1: Refactor the VR vibrate thread to be as a VRThread and manage its lifetime;

https://reviewboard.mozilla.org/r/199710/#review210834

I like this, LGTM, thanks!

Comment 13

[:kip (Kearwood Gilbert)]

Comment on attachment 8929378 [details]
Bug 1392217 - Part 2: Spawn a VR submit thread in VRDisplayHost;

https://reviewboard.mozilla.org/r/200704/#review210838

Thanks Daosheng, this is looking good.

I can expect that we would later be doing "vr compositing" of multiple layers and objects in this thread before passing the texture to the VR APIs, but can later refactor if the "submit thread" name is too specific.

Comment 14

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 538892d80ad3 -d b2db8b6d26d0: rebasing 437344:538892d80ad3 "Bug 1392217 - Part 1: Refactor the VR vibrate thread to be as a VRThread and manage its lifetime; r=kip"
merging gfx/vr/gfxVROculus.cpp
merging gfx/vr/gfxVROculus.h
merging gfx/vr/gfxVROpenVR.cpp
merging gfx/vr/gfxVROpenVR.h
warning: conflicts while merging gfx/vr/gfxVROculus.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging gfx/vr/gfxVROpenVR.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 15

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8928459 [details]
Bug 1392217 - Part 1: Refactor the VR vibrate thread to be as a VRThread and manage its lifetime;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199710/diff/4-5/

Comment 16

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8929378 [details]
Bug 1392217 - Part 2: Spawn a VR submit thread in VRDisplayHost;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200704/diff/3-4/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ff619c6c130
Part 1: Refactor the VR vibrate thread to be as a VRThread and manage its lifetime; r=kip
https://hg.mozilla.org/integration/autoland/rev/7f2e53267220
Part 2: Spawn a VR submit thread in VRDisplayHost; r=kip

Comment 18

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/5ff619c6c130
https://hg.mozilla.org/mozilla-central/rev/7f2e53267220