Closed Bug 1383907 Opened 7 years ago Closed 7 years ago

Enable WebVR tests on macOS

Categories

(Core :: WebVR, enhancement)

Product:
Core
Component:
WebVR
Platform:
All
macOS
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: kip, Assigned: daoshengmu)

References

Details

Attachments

(1 file)

Bug 1383907 - Enable WebVR reftests on macOS;
59 bytes, text/x-review-board-request
kip
: review+
jgilbert
: review+
Details 
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.
Depends on: 1310665
I am going to work on it.
Assignee: nobody → dmu
Before enabling reftest for WebVR on MacOS, we have to support VRDisplayPuppet::SubmitFrame() for catching the frame results on MacOS.
(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.
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 on attachment 8901010 [details]
Bug 1383907 - Enable WebVR reftests on macOS;

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

Excellent, thanks for bringing this up!
Attachment #8901010 - Flags: review?(kgilbert) → review+
See Also: → 1382104
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?
Attachment #8901010 - Flags: feedback?(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.
Attachment #8901010 - Flags: review-
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 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 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/
Attachment #8901010 - Flags: review?(jgilbert) → review-
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.
Target Milestone: --- → mozilla57
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();
Attachment #8901010 - Flags: review?(jgilbert) → review+
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
Pushed by dmu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fd638dbfa55d
Enable WebVR reftests on macOS; r=jgilbert,kip
https://hg.mozilla.org/mozilla-central/rev/fd638dbfa55d
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Depends on: 1400091
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: