1519035 - moz_external_vr.h: Add another browserState for Servo

Status: RESOLVED FIXED

(Core :: WebVR, enhancement)

Description

[Paul Rouget [:paul]]

Attachment: v1.patch

In Firefox Reality, we want to be able to render VR content with Servo as well. We then need 2 browser states.

Comment 1

[Paul Rouget [:paul]]

Comment on attachment 9035555 [details] [diff] [review]
v1.patch

I kept it very simple. You mentioned maybe using an array, but I ended up using 2 fields. Let me know what you think.

Comment 2

[:kip (Kearwood Gilbert)]

(In reply to Paul Rouget [:paul] from comment #1)

Comment on attachment 9035555 [details] [diff] [review]
v1.patch

I kept it very simple. You mentioned maybe using an array, but I ended up using 2 fields. Let me know what you think.

Hi Paul,

This is looking good; however, we should also duplicate the fields used for synchronization of these structures.

For Intel/x86 platforms (non-android currently), we use:
int64_t generationA;
int64_t generationB;

For ARM platforms we use:
pthread_mutex_t browserMutex;
pthread_cond_t browserCond;

Could you add the corresponding fields for Servo?

You can go ahead and rename the existing variables to avoid confusion:

/s/browserMutex/geckoMutex/
/s/browserCond/geckoCond/
/s/browserGenerationA/geckoGenerationA/
/s/browserGenerationB/geckoGenerationB/

Thanks again!

Comment 3

[Paul Rouget [:paul]]

Right, totally missed the mutex variables.

For non-android code, I'm not exactly sure what should be done.

I renamed browserGenerationA/B to geckoGenerationA/B for consistency.

I'm not sure how generationA/B and browserGenerationA/B are used. Should I also duplicate generationA?

Comment 4

[:kip (Kearwood Gilbert)]

(In reply to Paul Rouget [:paul] from comment #3)

Right, totally missed the mutex variables.

For non-android code, I'm not exactly sure what should be done.

I renamed browserGenerationA/B to geckoGenerationA/B for consistency.

I'm not sure how generationA/B and browserGenerationA/B are used. Should I also duplicate generationA?

I'd suggest having a geckoGenerationA/B as well as a servoGenerationA/B.
The generationA/B would only need to be extended if there were multiple VRSystemState's.

The generationA/B pattern enables the implementation of a lock-free struct on x86...
When the "writer" side updates the struct:

browserGenerationA is incremented
VRBrowserState struct is memcpy'ed from the working copy
browserGenerationB is incremented

The "reader" can memcpy all of this to its own working copy. It then compares generationA and generationB to detect if there was a change and to validate that it was a clean copy.

As other platforms (eg, ARM) do not guarantee memory access order, the mutexes are used instead.

The resulting VRExternalShmem struct should look like:

struct VRExternalShmem
{
int32_t version;
int32_t size;
#if defined(ANDROID)
pthread_mutex_t systemMutex;
pthread_mutex_t geckoMutex;
pthread_mutex_t servoMutex;
pthread_cond_t systemCond;
pthread_cond_t geckoCond;
pthread_cond_t servoCond;
#else
int64_t generationA;
#endif // defined(ANDROID)
VRSystemState state;
#if !defined(ANDROID)
int64_t generationB;
int64_t geckoGenerationA;
#endif // !defined(ANDROID)
VRBrowserState geckoState;
#if !defined(ANDROID)
int64_t geckoGenerationB;
int64_t servoGenerationA;
#endif // !defined(ANDROID)
VRBrowserState servoState;
#if !defined(ANDROID)
int64_t servoGenerationB;
#endif // !defined(ANDROID)
};

Comment 5

[Paul Rouget [:paul]]

Attachment: v2.patch

Comment 6

[Paul Rouget [:paul]]

What about systemCond? We use it in PushSystemState(), can it be safely shared between both or also needs a split?

(from https://github.com/MozillaReality/FirefoxReality/pull/921)

Comment 7

[:kip (Kearwood Gilbert)]

(In reply to Paul Rouget [:paul] from comment #6)

What about systemCond? We use it in PushSystemState(), can it be safely shared between both or also needs a split?

(from https://github.com/MozillaReality/FirefoxReality/pull/921)

As there is only one VRSystemState, you will only need one systemCond. No need to split it.

Comment 8

[Paul Rouget [:paul]]

I've updated the patch.

Comment 9

[:kip (Kearwood Gilbert)]

Comment on attachment 9036842 [details] [diff] [review]
v2.patch

LGTM, please also increment the version field.  R=me with that, thanks!

Comment 10

[Paul Rouget [:paul]]

Attachment: v3.patch, r=kip

Comment 11

[Cosmin Sabou [:CosminS]]

Patch failed to apply on mozilla-inbound:
applying https://bug1519035.bmoattachments.org/attachment.cgi?id=9039717
patching file gfx/vr/gfxVRExternal.cpp
Hunk #1 FAILED at 828
1 out of 1 hunks FAILED -- saving rejects to file gfx/vr/gfxVRExternal.cpp.rej
patching file gfx/vr/service/VRService.cpp
Hunk #1 FAILED at 439
1 out of 1 hunks FAILED -- saving rejects to file gfx/vr/service/VRService.cpp.rej

Comment 12

[Cosmin Sabou [:CosminS]]

--- VRService.cpp
+++ VRService.cpp
@@ -440,32 +440,32 @@ void VRService::PullState(mozilla::gfx::VRBrowserState& aState) {
if (!mAPIShmem) {
return;
}
// Copying the browser state from the shmem is non-blocking
// on x86/x64 architectures. Arm requires a mutex that is
// locked for the duration of the memcpy to and from shmem on
// both sides.
// On x86/x64 It is fallable -- If a dirty copy is detected by

• // a mismatch of browserGenerationA and browserGenerationB,

• // a mismatch of geckoGenerationA and geckoGenerationB,
  // the copy is discarded and will not replace the last known
  // browser state.

#if defined(MOZ_WIDGET_ANDROID)

• if (pthread_mutex_lock((pthread_mutex_t*)&(mExternalShmem->browserMutex)) ==

• if (pthread_mutex_lock((pthread_mutex_t*)&(mExternalShmem->geckoMutex)) ==
  0) {

• memcpy(&aState, &tmp.browserState, sizeof(VRBrowserState));
• pthread_mutex_unlock((pthread_mutex_t*)&(mExternalShmem->browserMutex));

• memcpy(&aState, &tmp.geckoState, sizeof(VRBrowserState));
• pthread_mutex_unlock((pthread_mutex_t*)&(mExternalShmem->geckoMutex));
  }
  #else
  VRExternalShmem tmp;

• if (mAPIShmem->browserGenerationA != mBrowserGeneration) {

• if (mAPIShmem->geckoGenerationA != mBrowserGeneration) {
  memcpy(&tmp, mAPIShmem, sizeof(VRExternalShmem));

• if (tmp.browserGenerationA == tmp.browserGenerationB &&

•    tmp.browserGenerationA != 0 && tmp.browserGenerationA != -1) {
  

•  memcpy(&aState, &tmp.browserState, sizeof(VRBrowserState));
  

•  mBrowserGeneration = tmp.browserGenerationA;
  

• if (tmp.geckoGenerationA == tmp.geckoGenerationB &&

•    tmp.geckoGenerationA != 0 && tmp.geckoGenerationA != -1) {
  

•  memcpy(&aState, &tmp.geckoState, sizeof(VRBrowserState));
  

•  mBrowserGeneration = tmp.geckoGenerationA;
  

  }
  }
  #endif
  }

VRExternalShmem* VRService::GetAPIShmem() { return mAPIShmem; }

Comment 13

[Paul Rouget [:paul]]

Attachment: v4.patch

I rebased the patch.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8231701da677
Add dedicated browser state for Servo. r=kip

Comment 15

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/8231701da677