Closed Bug 1392217 Opened 7 years ago Closed 7 years ago

Implement VR thread for submitting frames

Categories

(Core :: WebVR, enhancement, P1)

Product:
Core
Component:
WebVR
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox59 --- fixed

People

(Reporter: daoshengmu, Assigned: daoshengmu)

References

Details

Attachments

(2 files)

Bug 1392217 - Part 1: Refactor the VR vibrate thread to be as a VRThread and manage its lifetime;
59 bytes, text/x-review-board-request
kip
: review+
Details
Bug 1392217 - Part 2: Spawn a VR submit thread in VRDisplayHost;
59 bytes, text/x-review-board-request
kip
: review+
Details 
      No description provided.
Blocks: 1392215
Summary: Implement VR thread for MacOSX → Implement VR thread for submitting frames
Severity: normal → enhancement
status-firefox57: --- → wontfix
Priority: -- → P1
Assignee: nobody → dmu
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]
Depends on: 1404534
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]
Attachment #8928459 - Flags: review?(kgilbert)
Attachment #8929378 - Flags: review?(kgilbert)
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!
Attachment #8928459 - Flags: review?(kgilbert) → review+
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.
Attachment #8929378 - Flags: review?(kgilbert) → review+
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)
Pushed by dmu@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/5ff619c6c130
https://hg.mozilla.org/mozilla-central/rev/7f2e53267220
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: