1383907 - Enable WebVR tests on macOS

Status: RESOLVED FIXED

(Core :: WebVR, enhancement)

Description

[:kip (Kearwood Gilbert)]

Bug 1310656 adds support for WebVR on macOS, which was previously only supported on Windows.

Before enabling by default in Bug 1374399, we should turn on the reftests and mochitests for WebVR and ensure that they are all passing on macOS.

Comment 1

[Daosheng Mu[:daoshengmu]]

I am going to work on it.

Comment 2

[Daosheng Mu[:daoshengmu]]

Before enabling reftest for WebVR on MacOS, we have to support VRDisplayPuppet::SubmitFrame() for catching the frame results on MacOS.

Comment 3

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1383907 - Enable WebVR reftests on macOS;

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

Comment 4

[Daosheng Mu[:daoshengmu]]

(In reply to Daosheng Mu[:daoshengmu] from comment #3)
> Created attachment 8901010 [details]
> Bug 1383907 - Enable WebVR reftests on macOS;
> 
> Review commit: https://reviewboard.mozilla.org/r/172478/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/172478/

It run well in local, waiting for the try result.

Comment 5

[Daosheng Mu[:daoshengmu]]

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

We need to add a fuzzy-if because there is platform specific color-bleeding difference with the reference texture on change_size.html.

Comment 6

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8901010 [details]
Bug 1383907 - Enable WebVR reftests on macOS;

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

Comment 7

[:kip (Kearwood Gilbert)]

Comment on attachment 8901010 [details]
Bug 1383907 - Enable WebVR reftests on macOS;

https://reviewboard.mozilla.org/r/172478/#review178792

Excellent, thanks for bringing this up!

Comment 8

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8901010 [details]
Bug 1383907 - Enable WebVR reftests on macOS;

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

Comment 9

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8901010 [details]
Bug 1383907 - Enable WebVR reftests on macOS;

Before landing it, I would like to have your feedback. Because it seems to be related with Bug 1382104 that you are working on. Should I wait for your patches landed?

Comment 10

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8901010 [details]
Bug 1383907 - Enable WebVR reftests on macOS;

https://reviewboard.mozilla.org/r/172478/#review179826

::: dom/vr/test/reftest/reftest.list:9
(Diff revision 3)
> -# VR SubmitFrame is only implemented for D3D11 now.
> +# VR SubmitFrame is only implemented for D3D11 and MacOSX now.
>  # We need to continue to investigate why these reftests can be run well in local,
> -# but will be suspended until terminating on reftest debug build.
> -skip-if(!winWidget||!layersGPUAccelerated||isDebugBuild) == draw_rect.html wrapper.html?draw_rect.png
> -skip-if(!winWidget||!layersGPUAccelerated||isDebugBuild) == change_size.html wrapper.html?change_size.png
> +# but will be suspended until terminating on reftest D3D11 debug build.
> +skip-if(Android||gtkWidget||(winWidget&&isDebugBuild)||!layersGPUAccelerated) == draw_rect.html wrapper.html?draw_rect.png
> +# On MacOSX platform, because of getting different color interpolation result,
> +# we need to adjust it to fuzzy-if(cocoaWidget, 32,1800) in local.

This sounds like a bug.

::: gfx/vr/gfxVRPuppet.cpp:366
(Diff revision 3)
>        // Ideally, we should convert the srcData to a PNG image and decode it
>        // to a Base64 string here, but the GPU process does not have the privilege to
>        // access the image library. So, we have to convert the RAW image data
>        // to a base64 string and forward it to let the content process to
>        // do the image conversion.
> -      char* srcData = static_cast<char*>(mapInfo.pData);
> +      char *srcData = static_cast<char*>(mapInfo.pData);

Why did you move the star against the name? It should remain against the type, to the left.

::: gfx/vr/gfxVRPuppet.cpp:434
(Diff revision 3)
>        mContext->RSSetViewports(1, &viewport);
>        mContext->RSSetScissorRects(1, &scissor);
>        mContext->IASetPrimitiveTopology(D3D11_PRIMITIVE_TOPOLOGY_TRIANGLESTRIP);
>        mContext->VSSetShader(mQuadVS, nullptr, 0);
>        mContext->PSSetShader(mQuadPS, nullptr, 0);
> -      ID3D11ShaderResourceView* srView = aSource->GetShaderResourceView();
> +      ID3D11ShaderResourceView *srView = aSource->GetShaderResourceView();

star to left

::: gfx/vr/gfxVRPuppet.cpp:475
(Diff revision 3)
>  {
> -  if (!mIsPresenting) {
> +  if (!mIsPresenting || !aMacIOSurface) {
>      return false;
>    }
>  
> -  // TODO: Bug 1343730, Need to block until the next simulated
> +  VRManager *vm = VRManager::Get();

star to left

::: gfx/vr/gfxVRPuppet.cpp:495
(Diff revision 3)
> +        // Ideally, we should convert the srcData to a PNG image and decode it
> +        // to a Base64 string here, but the GPU process does not have the privilege to
> +        // access the image library. So, we have to convert the RAW image data
> +        // to a base64 string and forward it to let the content process to
> +        // do the image conversion.
> +        const uint8_t *srcData = dataSurf->GetData();

star to left

::: gfx/vr/gfxVRPuppet.cpp:505
(Diff revision 3)
> +        nsCString rawString(Substring((char*)srcData,
> +                            dataSurf->Stride() * surfSize.height));

This is wrong if the data is not tightly strided. Handle or assert this.

Comment 11

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8901010 [details]
Bug 1383907 - Enable WebVR reftests on macOS;

https://reviewboard.mozilla.org/r/172478/#review179830

Comment 12

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8901010 [details]
Bug 1383907 - Enable WebVR reftests on macOS;

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

Comment 13

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8901010 [details]
Bug 1383907 - Enable WebVR reftests on macOS;

https://reviewboard.mozilla.org/r/172478/#review179826

> This is wrong if the data is not tightly strided. Handle or assert this.

indeed. I have handled it by rows when the width is not equal to (stride / formatSize).

Comment 14

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8901010 [details]
Bug 1383907 - Enable WebVR reftests on macOS;

https://reviewboard.mozilla.org/r/172478/#review179826

> This sounds like a bug.

After fixing, the fuzzy-if(1,600) is on try servers and (1, 1200) in local. It looks like the Retina display difference.

Comment 15

[Daosheng Mu[:daoshengmu]]

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

Comment 16

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8901010 [details]
Bug 1383907 - Enable WebVR reftests on macOS;

https://reviewboard.mozilla.org/r/172478/#review182112

::: dom/vr/test/reftest/reftest.list:11
(Diff revision 4)
> -# but will be suspended until terminating on reftest debug build.
> -skip-if(!winWidget||!layersGPUAccelerated||isDebugBuild) == draw_rect.html wrapper.html?draw_rect.png
> -skip-if(!winWidget||!layersGPUAccelerated||isDebugBuild) == change_size.html wrapper.html?change_size.png
> +# but will be suspended until terminating on reftest D3D11 debug build.
> +skip-if(Android||gtkWidget||(winWidget&&isDebugBuild)||!layersGPUAccelerated) == draw_rect.html wrapper.html?draw_rect.png
> +# On MacOSX platform, because of getting different color interpolation result,
> +# we need to adjust it to fuzzy-if(cocoaWidget,1,1200) in local.
> +# However, it is fine to let fuzzy-if(cocoaWidget,1,600) on try servers.
> +fuzzy-if(cocoaWidget,1,600) skip-if(Android||gtkWidget||(winWidget&&isDebugBuild)||!layersGPUAccelerated) == change_size.html wrapper.html?change_size.png

This is really suspicious. I really suspect a bug here. Check the output of the reftests.

::: gfx/vr/gfxVRPuppet.cpp:495
(Diff revision 4)
> +        // Ideally, we should convert the srcData to a PNG image and decode it
> +        // to a Base64 string here, but the GPU process does not have the privilege to
> +        // access the image library. So, we have to convert the RAW image data
> +        // to a base64 string and forward it to let the content process to
> +        // do the image conversion.
> +        const uint8_t* srcData = dataSurf->GetData();

GetData and GetStride are deprecated. Use Map.

::: gfx/vr/gfxVRPuppet.cpp:505
(Diff revision 4)
> +        if ((surfSize.width * 4) == dataSurf->Stride()) {
> +          rawString = Substring((char*)srcData, dataSurf->Stride() * surfSize.height);

Don't bother with a fastpath. It's more important that this be correct than that it be fast.

::: gfx/vr/gfxVRPuppet.cpp:509
(Diff revision 4)
> +        nsCString rawString;
> +        if ((surfSize.width * 4) == dataSurf->Stride()) {
> +          rawString = Substring((char*)srcData, dataSurf->Stride() * surfSize.height);
> +        } else {
> +          for (int32_t i = 0; i < surfSize.height; i++) {
> +            rawString += Substring((char*)(srcData) + i * dataSurf->Stride(),

const char*, if you need to cast at all.

::: gfx/vr/gfxVRPuppet.cpp:525
(Diff revision 4)
> +      }
> +      break;
> +    }
> +    case 2:
> +    {
> +      MOZ_ASSERT(false, "Not support show VR frames on MacOSX yet.");

s/Not support show/No support for showing/

Comment 17

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8901010 [details]
Bug 1383907 - Enable WebVR reftests on macOS;

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

Comment 18

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8901010 [details]
Bug 1383907 - Enable WebVR reftests on macOS;

https://reviewboard.mozilla.org/r/172478/#review182112

> This is really suspicious. I really suspect a bug here. Check the output of the reftests.

I have confirmed it with other 15" MBP. I can get pass for fuzzy-if(cocoaWidget,1,600) no matter integrated or independent GPUs. I think it is because I was using 13" MBP that has lower resolution.

Comment 19

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8901010 [details]
Bug 1383907 - Enable WebVR reftests on macOS;

https://reviewboard.mozilla.org/r/172478/#review184738

::: gfx/vr/gfxVRPuppet.cpp:501
(Diff revision 5)
> +        if (!dataSurf->Map(gfx::DataSourceSurface::MapType::READ, &map)) {
> +          MOZ_ASSERT(false, "Read DataSourceSurface fail.");
> -    return false;
> +          return false;
> -  }
> +        }
> +        const uint8_t* srcData = map.mData;
> +        IntSize surfSize(dataSurf->GetSize());

Prefer:
const auto& surfSize = dataSurf->GetSize();

Comment 20

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8901010 [details]
Bug 1383907 - Enable WebVR reftests on macOS;

https://reviewboard.mozilla.org/r/172478/#review179826

> indeed. I have handled it by rows when the width is not equal to (stride / formatSize).

Like this:
https://reviewboard.mozilla.org/r/170344/diff/1#index_header

Comment 21

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8901010 [details]
Bug 1383907 - Enable WebVR reftests on macOS;

https://reviewboard.mozilla.org/r/172478/#review184740

Comment 22

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8901010 [details]
Bug 1383907 - Enable WebVR reftests on macOS;

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

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd638dbfa55d
Enable WebVR reftests on macOS; r=jgilbert,kip

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fd638dbfa55d