Closed Bug 1419293 Opened 2 years ago Closed 2 years ago

Create SwapChain with DXGI_SWAP_EFFECT_FLIP_SEQUENTIAL if possible in ANGLE

Categories

(Core :: Graphics: WebRender, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

(Whiteboard: [wr-reserve])

Attachments

(2 files, 16 obsolete files)

1.25 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
15.69 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
SwapChain with DXGI_SWAP_EFFECT_FLIP_SEQUENTIAL seems to provide better performance.

https://msdn.microsoft.com/en-us/library/windows/desktop/hh706346(v=vs.85).aspx
Assignee: nobody → sotaro.ikeda.g
Attachment #8930376 - Flags: review?(jgilbert)
Attachment #8930376 - Flags: review?(jgilbert)
Attachment #8930376 - Flags: feedback?(jgilbert)
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [wr-mvp]
Attachment #8930376 - Flags: feedback?(jgilbert) → feedback+
Add checks for using DXGI_SWAP_EFFECT_FLIP_SEQUENTIAL by referring https://msdn.microsoft.com/en-us/library/windows/desktop/hh706346(v=vs.85).aspx
Attachment #8930376 - Attachment is obsolete: true
Whiteboard: [wr-mvp] → [wr-reserve]
See Also: → 1191971
Depends on: 1429997
Attached patch wip (obsolete) — Splinter Review
Attachment #8934050 - Attachment is obsolete: true
Attachment #8934050 - Attachment is obsolete: false
Attached patch wip (obsolete) — Splinter Review
Attachment #8942576 - Attachment is obsolete: true
Attached patch wip (obsolete) — Splinter Review
Attachment #8942800 - Attachment is obsolete: true
Attached patch wip (obsolete) — Splinter Review
Attachment #8942801 - Attachment is obsolete: true
Attached patch wip (obsolete) — Splinter Review
Attachment #8942802 - Attachment is obsolete: true
Attached patch wip (obsolete) — Splinter Review
Attachment #8943107 - Attachment is obsolete: true
Attachment #8934050 - Attachment is obsolete: true
Attachment #8934050 - Attachment is obsolete: false
Attachment #8934050 - Attachment is obsolete: true
Add constants that is necessary for creating Pbuffer from d3d texture.
Attachment #8945403 - Attachment is obsolete: true
Attachment #8945724 - Attachment description: patch - Add constants to GLDefs.h → patch part 1 - Add constants to GLDefs.h
Attachment #8945726 - Attachment description: patch - patch - Create SwapChain and manage it directly in RenderCompositorANGLE → patch part 2 - patch - Create SwapChain and manage it directly in RenderCompositorANGLE
attachment 8945728 [details] [diff] [review] creates GLContext with dummy EGLSurface, the dummy EGLSurface is not used. Instread we override it with EGLSurface of SwapChain's back buffer. When size of the SwapChain is changed, RenderCompositorANGLE creates EGLSurface that wrap SwapChain's back buffer and overrides GLContext's EGLSurface with GLContextEGL::SetEGLSurfaceOverride().

This idea was borrowed from chromium. DirectCompositionSurfaceWin and DirectCompositionChildSurfaceWin do almost same thing for how to handle EGLSurface.
 > https://cs.chromium.org/chromium/src/gpu/ipc/service/direct_composition_child_surface_win.cc?l=311
 > https://cs.chromium.org/chromium/src/gpu/ipc/service/direct_composition_child_surface_win.cc?l=75
 > https://cs.chromium.org/chromium/src/gpu/ipc/service/direct_composition_surface_win.cc?l=1200

SwapChain management code was borrowed from MLGDeviceD3D11.
Attachment #8945724 - Flags: review?(jgilbert)
Attachment #8945733 - Flags: review?(jgilbert)
Attachment #8945724 - Flags: review?(jgilbert) → review+
Comment on attachment 8945733 [details] [diff] [review]
patch part 2 - patch - Create SwapChain and manage it directly in RenderCompositorANGLE

Review of attachment 8945733 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/webrender_bindings/RenderCompositorANGLE.cpp
@@ +8,5 @@
>  
>  #include "GLContext.h"
>  #include "GLContextEGL.h"
>  #include "GLContextProvider.h"
> +#include "mozilla/WindowsVersion.h"

"mozilla/Win" should come after "mozilla/wid"

@@ +64,5 @@
> +
> +  RefPtr<IDXGIFactory> dxgiFactory;
> +  {
> +    RefPtr<IDXGIAdapter> adapter;
> +    dxgiDevice->GetAdapter(getter_AddRefs(adapter));

Why are we using both StartAssignment and getter_addRefs?

@@ +77,5 @@
> +  {
> +    RefPtr<IDXGISwapChain1> swapChain1;
> +
> +    DXGI_SWAP_CHAIN_DESC1 desc;
> +    ::ZeroMemory(&desc, sizeof(desc));

`DXGI_SWAP_CHAIN_DESC1 desc{};` or `= {}` zeros it for us. (aggregate initialization)

@@ +90,5 @@
> +    desc.Scaling = DXGI_SCALING_NONE;
> +    desc.Flags = 0;
> +
> +    HRESULT hr = dxgiFactory2->CreateSwapChainForHwnd(
> +      mDevice,

I recommend against dropping these values way down here, rather than aligned-wrapping them at the column limit.
HRESULT hr = dxgiFactory2->CreateSwapChainForHwnd(mDevice, hwnd, &desc,
                                                  nullptr, nullptr,
                                                  getter_AddRefs(swapchain1))

@@ +107,5 @@
> +  if (!mSwapChain) {
> +    DXGI_SWAP_CHAIN_DESC swapDesc;
> +    ::ZeroMemory(&swapDesc, sizeof(swapDesc));
> +    swapDesc.BufferDesc.Width = 0;
> +    swapDesc.BufferDesc.Height = 0;

Elsewhere you say "dxgi doesn't like 0-size swapchains", but it looks like you create one here?

@@ +114,5 @@
> +    swapDesc.BufferDesc.RefreshRate.Denominator = 1;
> +    swapDesc.SampleDesc.Count = 1;
> +    swapDesc.SampleDesc.Quality = 0;
> +    swapDesc.BufferUsage = DXGI_USAGE_RENDER_TARGET_OUTPUT;
> +    swapDesc.BufferCount = 1;

Why is this 1, when above it's 2?

@@ +137,5 @@
>      // Then, there will be no texture synchronization.
>      return false;
>    }
>  
> +  gl::CreateContextFlags flags = gl::CreateContextFlags::PREFER_ES3;

You don't need to repeat the type:
const auto flags = gl::CreateContextFlags::PREFER_ES3;

@@ +138,5 @@
>      return false;
>    }
>  
> +  gl::CreateContextFlags flags = gl::CreateContextFlags::PREFER_ES3;
> +  mozilla::gfx::IntSize dummySize = mozilla::gfx::IntSize(1, 1);

Construct the type directly:
const mozilla::gfx::IntSize dummySize(1, 1);

@@ +141,5 @@
> +  gl::CreateContextFlags flags = gl::CreateContextFlags::PREFER_ES3;
> +  mozilla::gfx::IntSize dummySize = mozilla::gfx::IntSize(1, 1);
> +  gl::SurfaceCaps dummyCaps = gl::SurfaceCaps::Any();
> +  dummyCaps.depth = true;
> +  dummyCaps.stencil = true;

Don't use Any with depth/stencil.
You use FLEXIBLE_SURFACE_COMPATIBILITY_SUPPORTED below anyway, so you should be able to use Any without using depth/stencil.

@@ +146,5 @@
> +  nsCString str;
> +
> +  // Create GLContext with dummy EGLSurface, the EGLSurface is not used.
> +  // Instread we override it with EGLSurface of SwapChain's back buffer.
> +  mGL = gl::GLContextEGL::CreateEGLPBufferOffscreenContext(flags, dummySize, dummyCaps, &str);

This should probably be CreateHeadless, since both size and caps are dummies.

@@ +205,5 @@
>    InsertPresentWaitQuery();
>  
> +  MOZ_ASSERT(!mEGLSurface);
> +
> +  mGL->fFlush();

Why do we need to flush here? Is there ANGLE documentation you can reference for why this is needed?

@@ +226,5 @@
>    WaitForPreviousPresentQuery();
>  }
>  
> +bool
> +RenderCompositorANGLE::ResizeBuffers(const LayoutDeviceIntSize& aSize)

SetBufferSize.

@@ +243,5 @@
> +    gl::GLContextEGL::Cast(mGL)->SetEGLSurfaceOverride(EGL_NO_SURFACE);
> +    egl->fDestroySurface(egl->Display(), mEGLSurface);
> +    mEGLSurface = nullptr;
> +  }
> +  hr = mSwapChain->ResizeBuffers(0, aSize.width, aSize.height, DXGI_FORMAT_B8G8R8A8_UNORM, 0);

Does this handle 0x0 properly?

@@ +260,5 @@
> +  }
> +
> +  const EGLint pbuffer_attribs[]{
> +    LOCAL_EGL_WIDTH,
> +    aSize.width,

Please try to group these:
  LOCAL_EGL_WIDTH, aSize.width,
  LOCAL_EGL_HEIGHT, aSize.height,
  LOCAL_EGL_FLEXIBLE_SURFACE_COMPATIBILITY_SUPPORTED_ANGLE,
    LOCAL_EGL_TRUE,
  LOCAL_EGL_NONE
};

@@ +267,5 @@
> +    LOCAL_EGL_FLEXIBLE_SURFACE_COMPATIBILITY_SUPPORTED_ANGLE,
> +    LOCAL_EGL_TRUE,
> +    LOCAL_EGL_NONE};
> +
> +  EGLSurface surface = 0;

No reason to forward-declare this.

@@ +268,5 @@
> +    LOCAL_EGL_TRUE,
> +    LOCAL_EGL_NONE};
> +
> +  EGLSurface surface = 0;
> +  EGLClientBuffer buffer = reinterpret_cast<EGLClientBuffer>(backBuf.get());

You don't need to repeat the type, particularly for casts:
const auto buffer = reinterpret_cast<EGLClientBuffer>(backBuf.get());

@@ +277,5 @@
> +    pbuffer_attribs);
> +
> +  EGLint err = egl->fGetError();
> +  if (err != LOCAL_EGL_SUCCESS) {
> +    gfxCriticalError() << "Failed to create Pbuffer of back buffer";

Include the error value in this output.
Attachment #8945733 - Flags: review?(jgilbert) → review-
(In reply to Jeff Gilbert [:jgilbert] from comment #17)
> Comment on attachment 8945733 [details] [diff] [review]
> patch part 2 - patch - Create SwapChain and manage it directly in
> RenderCompositorANGLE
> 
> Review of attachment 8945733 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/webrender_bindings/RenderCompositorANGLE.cpp
> @@ +8,5 @@
> >  
> >  #include "GLContext.h"
> >  #include "GLContextEGL.h"
> >  #include "GLContextProvider.h"
> > +#include "mozilla/WindowsVersion.h"
> 
> "mozilla/Win" should come after "mozilla/wid"

I will update it in a next patch.

> 
> @@ +64,5 @@
> > +
> > +  RefPtr<IDXGIFactory> dxgiFactory;
> > +  {
> > +    RefPtr<IDXGIAdapter> adapter;
> > +    dxgiDevice->GetAdapter(getter_AddRefs(adapter));
> 
> Why are we using both StartAssignment and getter_addRefs?

The code was borrowed from MLGSwapChainD3D11::Initialize(). StartAssignment and getter_addRefs does actually same thing.
From the code, it seems to use StartAssignment just to simplify code a bit. Majority of layer source code uses getter_addRefs, then I will change the code as to use getter_addRefs.


> 
> @@ +77,5 @@
> > +  {
> > +    RefPtr<IDXGISwapChain1> swapChain1;
> > +
> > +    DXGI_SWAP_CHAIN_DESC1 desc;
> > +    ::ZeroMemory(&desc, sizeof(desc));
> 
> `DXGI_SWAP_CHAIN_DESC1 desc{};` or `= {}` zeros it for us. (aggregate
> initialization)

I will update it in a next patch.

> 
> @@ +90,5 @@
> > +    desc.Scaling = DXGI_SCALING_NONE;
> > +    desc.Flags = 0;
> > +
> > +    HRESULT hr = dxgiFactory2->CreateSwapChainForHwnd(
> > +      mDevice,
> 
> I recommend against dropping these values way down here, rather than
> aligned-wrapping them at the column limit.
> HRESULT hr = dxgiFactory2->CreateSwapChainForHwnd(mDevice, hwnd, &desc,
>                                                   nullptr, nullptr,
>                                                   getter_AddRefs(swapchain1))

I'll update it in a next patch.

> 
> @@ +107,5 @@
> > +  if (!mSwapChain) {
> > +    DXGI_SWAP_CHAIN_DESC swapDesc;
> > +    ::ZeroMemory(&swapDesc, sizeof(swapDesc));
> > +    swapDesc.BufferDesc.Width = 0;
> > +    swapDesc.BufferDesc.Height = 0;
> 
> Elsewhere you say "dxgi doesn't like 0-size swapchains", but it looks like
> you create one here?

When I read ANGLE source there was a comment like it in SwapChain11::resize()
https://dxr.mozilla.org/mozilla-central/source/gfx/angle/src/libANGLE/renderer/d3d/d3d11/SwapChain11.cpp#460
But when I tested locally, just set to 0-size seems not cause the problem. Then I am going to update the related part.

> 
> @@ +114,5 @@
> > +    swapDesc.BufferDesc.RefreshRate.Denominator = 1;
> > +    swapDesc.SampleDesc.Count = 1;
> > +    swapDesc.SampleDesc.Quality = 0;
> > +    swapDesc.BufferUsage = DXGI_USAGE_RENDER_TARGET_OUTPUT;
> > +    swapDesc.BufferCount = 1;
> 
> Why is this 1, when above it's 2?

It is set to 1 since SwapEffect is DXGI_SWAP_EFFECT_SEQUENTIAL and it is bitblt model. When DXGI_SWAP_EFFECT_FLIP_SEQUENTIAL is used, swap chain works as flip model.

 In the bitblt model, contents of the back buffer are copied into the redirection surface on each call to IDXGISwapChain1::Present1. Then we need to allocate only 1 buffer. In the flip model, all back buffers are shared with the Desktop Window Manager (DWM), then we need to allocate multiple buffers.

https://msdn.microsoft.com/ja-jp/library/windows/desktop/hh706346

Is setting is same to CompositorD3D11::Initialize(), MLGSwapChainD3D11::Initialize() and NativeWindow11Win32::createSwapChain().

https://dxr.mozilla.org/mozilla-central/source/gfx/layers/d3d11/CompositorD3D11.cpp#127
https://dxr.mozilla.org/mozilla-central/source/gfx/layers/d3d11/MLGDeviceD3D11.cpp#246
https://dxr.mozilla.org/mozilla-central/source/gfx/angle/src/libANGLE/renderer/d3d/d3d11/win32/NativeWindow11Win32.cpp#54

> 
> @@ +137,5 @@
> >      // Then, there will be no texture synchronization.
> >      return false;
> >    }
> >  
> > +  gl::CreateContextFlags flags = gl::CreateContextFlags::PREFER_ES3;
> 
> You don't need to repeat the type:
> const auto flags = gl::CreateContextFlags::PREFER_ES3;

I will update in a next patch.

> 
> @@ +138,5 @@
> >      return false;
> >    }
> >  
> > +  gl::CreateContextFlags flags = gl::CreateContextFlags::PREFER_ES3;
> > +  mozilla::gfx::IntSize dummySize = mozilla::gfx::IntSize(1, 1);
> 
> Construct the type directly:
> const mozilla::gfx::IntSize dummySize(1, 1);

I will update it in a next patch.

> 
> @@ +141,5 @@
> > +  gl::CreateContextFlags flags = gl::CreateContextFlags::PREFER_ES3;
> > +  mozilla::gfx::IntSize dummySize = mozilla::gfx::IntSize(1, 1);
> > +  gl::SurfaceCaps dummyCaps = gl::SurfaceCaps::Any();
> > +  dummyCaps.depth = true;
> > +  dummyCaps.stencil = true;
> 
> Don't use Any with depth/stencil.
> You use FLEXIBLE_SURFACE_COMPATIBILITY_SUPPORTED below anyway, so you should
> be able to use Any without using depth/stencil.

depth/stencil was added to make gl::GLContextEGL::Cast(mGL)->mConfig has depth/stencil, since webrender needs depth/stencil. Is there another better way to create EGLConfig as to have depth/stencil?

> 
> @@ +146,5 @@
> > +  nsCString str;
> > +
> > +  // Create GLContext with dummy EGLSurface, the EGLSurface is not used.
> > +  // Instread we override it with EGLSurface of SwapChain's back buffer.
> > +  mGL = gl::GLContextEGL::CreateEGLPBufferOffscreenContext(flags, dummySize, dummyCaps, &str);
> 
> This should probably be CreateHeadless, since both size and caps are dummies.

It was used to create create EGLConfig as to have depth/stencil as explained above. Is there another better way to create EGLConfig as to have depth/stencil?

> 
> @@ +205,5 @@
> >    InsertPresentWaitQuery();
> >  
> > +  MOZ_ASSERT(!mEGLSurface);
> > +
> > +  mGL->fFlush();
> 
> Why do we need to flush here? Is there ANGLE documentation you can reference
> for why this is needed?

It ended up just ID3D11DeviceContext:Flush(), then it seems not necessary to call "mGL->fFlush()". I am going to remove it in a next patch.

> 
> @@ +226,5 @@
> >    WaitForPreviousPresentQuery();
> >  }
> >  
> > +bool
> > +RenderCompositorANGLE::ResizeBuffers(const LayoutDeviceIntSize& aSize)
> 
> SetBufferSize.

I will change it in a next patch.

> 
> @@ +243,5 @@
> > +    gl::GLContextEGL::Cast(mGL)->SetEGLSurfaceOverride(EGL_NO_SURFACE);
> > +    egl->fDestroySurface(egl->Display(), mEGLSurface);
> > +    mEGLSurface = nullptr;
> > +  }
> > +  hr = mSwapChain->ResizeBuffers(0, aSize.width, aSize.height, DXGI_FORMAT_B8G8R8A8_UNORM, 0);
> 
> Does this handle 0x0 properly?

Does it 0x0 mean first argument? The code was from MLGSwapChainD3D11::ResizeBuffers() and it seems work. But it is better to set buffer count correctly like SwapChain11::resize()

https://dxr.mozilla.org/mozilla-central/source/gfx/angle/src/libANGLE/renderer/d3d/d3d11/SwapChain11.cpp#480

> 
> @@ +260,5 @@
> > +  }
> > +
> > +  const EGLint pbuffer_attribs[]{
> > +    LOCAL_EGL_WIDTH,
> > +    aSize.width,
> 
> Please try to group these:
>   LOCAL_EGL_WIDTH, aSize.width,
>   LOCAL_EGL_HEIGHT, aSize.height,
>   LOCAL_EGL_FLEXIBLE_SURFACE_COMPATIBILITY_SUPPORTED_ANGLE,
>     LOCAL_EGL_TRUE,
>   LOCAL_EGL_NONE
> };

I will update it in a next patch.

> 
> @@ +267,5 @@
> > +    LOCAL_EGL_FLEXIBLE_SURFACE_COMPATIBILITY_SUPPORTED_ANGLE,
> > +    LOCAL_EGL_TRUE,
> > +    LOCAL_EGL_NONE};
> > +
> > +  EGLSurface surface = 0;
> 
> No reason to forward-declare this.

I will update it in a next patch.

> 
> @@ +268,5 @@
> > +    LOCAL_EGL_TRUE,
> > +    LOCAL_EGL_NONE};
> > +
> > +  EGLSurface surface = 0;
> > +  EGLClientBuffer buffer = reinterpret_cast<EGLClientBuffer>(backBuf.get());
> 
> You don't need to repeat the type, particularly for casts:
> const auto buffer = reinterpret_cast<EGLClientBuffer>(backBuf.get());

Thanks, I will update it in a next patch.

> 
> @@ +277,5 @@
> > +    pbuffer_attribs);
> > +
> > +  EGLint err = egl->fGetError();
> > +  if (err != LOCAL_EGL_SUCCESS) {
> > +    gfxCriticalError() << "Failed to create Pbuffer of back buffer";
> 
> Include the error value in this output.

I will update it in a next patch.
> @@ +141,5 @@
> > +  gl::CreateContextFlags flags = gl::CreateContextFlags::PREFER_ES3;
> > +  mozilla::gfx::IntSize dummySize = mozilla::gfx::IntSize(1, 1);
> > +  gl::SurfaceCaps dummyCaps = gl::SurfaceCaps::Any();
> > +  dummyCaps.depth = true;
> > +  dummyCaps.stencil = true;
> 
> Don't use Any with depth/stencil.
> You use FLEXIBLE_SURFACE_COMPATIBILITY_SUPPORTED below anyway, so you should
> be able to use Any without using depth/stencil.

depth/stencil was added to make gl::GLContextEGL::Cast(mGL)->mConfig has depth/stencil, since webrender needs depth/stencil. The mConfig is used to call egl->fCreatePbufferFromClientBuffer(). Is there another better way to create EGLConfig as to have depth/stencil?

> 
> @@ +146,5 @@
> > +  nsCString str;
> > +
> > +  // Create GLContext with dummy EGLSurface, the EGLSurface is not used.
> > +  // Instread we override it with EGLSurface of SwapChain's back buffer.
> > +  mGL = gl::GLContextEGL::CreateEGLPBufferOffscreenContext(flags, dummySize, dummyCaps, &str);
> 
> This should probably be CreateHeadless, since both size and caps are dummies.

It was used to create create EGLConfig as to have depth/stencil as explained above. Is there another better way to create EGLConfig as to have depth/stencil?
Attachment #8945733 - Attachment is obsolete: true
Comment on attachment 8946191 [details] [diff] [review]
patch part 2 - patch - Create SwapChain and manage it directly in RenderCompositorANGLE

Can you review again the patch and answer the questions?
Attachment #8946191 - Flags: review?(jgilbert)
Blocks: 1364504
Comment on attachment 8946191 [details] [diff] [review]
patch part 2 - patch - Create SwapChain and manage it directly in RenderCompositorANGLE

Review of attachment 8946191 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/webrender_bindings/RenderCompositorANGLE.cpp
@@ +59,5 @@
>  
> +  HWND hwnd = mWidget->AsWindows()->GetHwnd();
> +
> +  RefPtr<IDXGIDevice> dxgiDevice;
> +  mDevice->QueryInterface((IDXGIDevice**)getter_AddRefs(dxgiDevice));

Do you really have to do this extra cast? It should coerce automatically.

@@ +131,5 @@
>      // Then, there will be no texture synchronization.
>      return false;
>    }
>  
> +  const auto flags = gl::CreateContextFlags::PREFER_ES3;

Do you actually need ES3?

@@ +140,5 @@
> +  nsCString str;
> +
> +  // Create GLContext with dummy EGLSurface, the EGLSurface is not used.
> +  // Instread we override it with EGLSurface of SwapChain's back buffer.
> +  mGL = gl::GLContextEGL::CreateEGLPBufferOffscreenContext(flags, dummySize, dummyCaps, &str);

Use CreateHeadless.

@@ +195,5 @@
>  RenderCompositorANGLE::EndFrame()
>  {
>    InsertPresentWaitQuery();
>  
> +  MOZ_ASSERT(!mEGLSurface);

Why should this be null? BeginFrame ensures that it is /not/ null. When does it get nulled?

@@ +216,5 @@
>    WaitForPreviousPresentQuery();
>  }
>  
> +bool
> +RenderCompositorANGLE::SetBufferSize()

If it doesn't take arguments, it should be UpdateBufferSize or UpdateSize.

@@ +227,5 @@
> +  size.width = std::max(size.width, 0);
> +  size.height = std::max(size.height, 0);
> +
> +  if (size == mSize) {
> +    return true;

Are any of the below `return false` paths recoverable? I think the failed resize path should probably be recoverable, so we should ensure that we don't get this:

UpdateBufferSize([800,600]) -> true
UpdateBufferSize([80000,600]) -> false
UpdateBufferSize([800,600]) -> true // Claims to succeed, but is in an invalid state!

This branch should at least ASSERT(mEGLSurface)

@@ +237,5 @@
> +  RefPtr<ID3D11Texture2D> backBuf;
> +
> +  // Release EGLSurface of back buffer before calling ResizeBuffers().
> +  if (mEGLSurface) {
> +    gl::GLContextEGL::Cast(mGL)->SetEGLSurfaceOverride(EGL_NO_SURFACE);

This code is also in RenderCompositorANGLE::Destroy(). Can you centralize it into a helper function?

@@ +246,5 @@
> +  // Resize swap chain
> +  DXGI_SWAP_CHAIN_DESC desc;
> +  hr = mSwapChain->GetDesc(&desc);
> +  if (FAILED(hr)) {
> +    gfxCriticalNote << "Failed to read swap chain description: " << gfx::hexa(hr) << " Size:" << size;

You're missing a space after the ':' here and below.

@@ +251,5 @@
> +    return false;
> +  }
> +  hr = mSwapChain->ResizeBuffers(desc.BufferCount, size.width, size.height, DXGI_FORMAT_B8G8R8A8_UNORM, 0);
> +  if (FAILED(hr)) {
> +    gfxCriticalNote << "Failed to resize swap chain buffers: " << gfx::hexa(hr) << " Size:" << size;

Isn't this a condition we should expect, if the requested size is too large for some reason?

@@ +271,5 @@
> +    LOCAL_EGL_FLEXIBLE_SURFACE_COMPATIBILITY_SUPPORTED_ANGLE, LOCAL_EGL_TRUE,
> +    LOCAL_EGL_NONE};
> +
> +  const auto buffer = reinterpret_cast<EGLClientBuffer>(backBuf.get());
> +  const auto config = gl::GLContextEGL::Cast(mGL)->mConfig;

If you're using EGL_FLEXIBLE_SURFACE_COMPATIBILITY_SUPPORTED_ANGLE, you probably shouldn't be passing mConfig. (That is, if mConfig matches the format of the EGLSurface, then you don't need EGL_FLEXIBLE_SURFACE_COMPATIBILITY_SUPPORTED_ANGLE) Does it work without EGL_FLEXIBLE_SURFACE_COMPATIBILITY_SUPPORTED_ANGLE?

It should only work without SURFACE_COMPAT if you happened to have created the original context with the correct config. Instead here you should rely on SURFACE_COMPAT, and choose (and cache!) the config that reflects the formats of these surfaces you want to attach to an otherwise-headless context.

Also don't call GLContextEGL::Cast multiple times when you can reuse the result.

@@ +279,5 @@
> +    pbuffer_attribs);
> +
> +  EGLint err = egl->fGetError();
> +  if (err != LOCAL_EGL_SUCCESS) {
> +    gfxCriticalError() << "Failed to create Pbuffer of back buffer error: " << gfx::hexa(hr) << " Size:" << size;

s/hr/err/

@@ +305,5 @@
>  
>  LayoutDeviceIntSize
>  RenderCompositorANGLE::GetClientSize()
>  {
> +  return mSize;

Are you sure you want to disconnect this query from the true value? I don't like that the name of this function sounds like it should be querying mWidget->GetClientSize as before, but now is instead returning the 'last successfully updated size'.
Attachment #8946191 - Flags: review?(jgilbert) → review-
(In reply to Jeff Gilbert [:jgilbert] from comment #21)
> Comment on attachment 8946191 [details] [diff] [review]
> patch part 2 - patch - Create SwapChain and manage it directly in
> RenderCompositorANGLE
> 
> Review of attachment 8946191 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/webrender_bindings/RenderCompositorANGLE.cpp
> @@ +59,5 @@
> >  
> > +  HWND hwnd = mWidget->AsWindows()->GetHwnd();
> > +
> > +  RefPtr<IDXGIDevice> dxgiDevice;
> > +  mDevice->QueryInterface((IDXGIDevice**)getter_AddRefs(dxgiDevice));
> 
> Do you really have to do this extra cast? It should coerce automatically.

Without the cast, the build was failed. All another IUnknown::QueryInterface() + getter_AddRefs do same thing in gecko.

It seemed to be caused by RefPtrGetterAddRefs implementation and QueryInterface() implementation. RefPtrGetterAddRefs supports to cast to void**. QueryInterface() is impelemented like the following. The argument was cast to void**, but the function also use the argument as __uuidof(Q).

>            QueryInterface(_COM_Outptr_ Q** pp)
>            {
>                return QueryInterface(__uuidof(Q), (void **)pp);
>            }


> 
> @@ +131,5 @@
> >      // Then, there will be no texture synchronization.
> >      return false;
> >    }
> >  
> > +  const auto flags = gl::CreateContextFlags::PREFER_ES3;
> 
> Do you actually need ES3?

Yes, without it, shader compile was failed.

> 
> @@ +140,5 @@
> > +  nsCString str;
> > +
> > +  // Create GLContext with dummy EGLSurface, the EGLSurface is not used.
> > +  // Instread we override it with EGLSurface of SwapChain's back buffer.
> > +  mGL = gl::GLContextEGL::CreateEGLPBufferOffscreenContext(flags, dummySize, dummyCaps, &str);
> 
> Use CreateHeadless.

I will update it in a next patch.

> 
> @@ +195,5 @@
> >  RenderCompositorANGLE::EndFrame()
> >  {
> >    InsertPresentWaitQuery();
> >  
> > +  MOZ_ASSERT(!mEGLSurface);
> 
> Why should this be null? BeginFrame ensures that it is /not/ null. When does
> it get nulled?

Sorry, I forgot to remove it. It was used in the past.

> 
> @@ +216,5 @@
> >    WaitForPreviousPresentQuery();
> >  }
> >  
> > +bool
> > +RenderCompositorANGLE::SetBufferSize()
> 
> If it doesn't take arguments, it should be UpdateBufferSize or UpdateSize.

I will change it to UpdateBufferSize().

> 
> @@ +227,5 @@
> > +  size.width = std::max(size.width, 0);
> > +  size.height = std::max(size.height, 0);
> > +
> > +  if (size == mSize) {
> > +    return true;
> 
> Are any of the below `return false` paths recoverable? I think the failed
> resize path should probably be recoverable, so we should ensure that we
> don't get this:
> 
> UpdateBufferSize([800,600]) -> true
> UpdateBufferSize([80000,600]) -> false
> UpdateBufferSize([800,600]) -> true // Claims to succeed, but is in an
> invalid state!

I will update the patch as recoverable.

> 
> This branch should at least ASSERT(mEGLSurface)

I will update it in a next patch.

> 
> @@ +237,5 @@
> > +  RefPtr<ID3D11Texture2D> backBuf;
> > +
> > +  // Release EGLSurface of back buffer before calling ResizeBuffers().
> > +  if (mEGLSurface) {
> > +    gl::GLContextEGL::Cast(mGL)->SetEGLSurfaceOverride(EGL_NO_SURFACE);
> 
> This code is also in RenderCompositorANGLE::Destroy(). Can you centralize it
> into a helper function?

Yes, I will update it in a next patch.

> 
> @@ +246,5 @@
> > +  // Resize swap chain
> > +  DXGI_SWAP_CHAIN_DESC desc;
> > +  hr = mSwapChain->GetDesc(&desc);
> > +  if (FAILED(hr)) {
> > +    gfxCriticalNote << "Failed to read swap chain description: " << gfx::hexa(hr) << " Size:" << size;
> 
> You're missing a space after the ':' here and below.

I will update it in a next patch.

> 
> @@ +251,5 @@
> > +    return false;
> > +  }
> > +  hr = mSwapChain->ResizeBuffers(desc.BufferCount, size.width, size.height, DXGI_FORMAT_B8G8R8A8_UNORM, 0);
> > +  if (FAILED(hr)) {
> > +    gfxCriticalNote << "Failed to resize swap chain buffers: " << gfx::hexa(hr) << " Size:" << size;
> 
> Isn't this a condition we should expect, if the requested size is too large
> for some reason?

In normal situation, we do not expect it to happen, since the size is come from GetClientRect()
 https://msdn.microsoft.com/en-us/library/windows/desktop/ms633503(v=vs.85).aspx

CompositorD3D11 and MLGSwapChainD3D11 and ANGLE also output error log to log.

> 
> @@ +271,5 @@
> > +    LOCAL_EGL_FLEXIBLE_SURFACE_COMPATIBILITY_SUPPORTED_ANGLE, LOCAL_EGL_TRUE,
> > +    LOCAL_EGL_NONE};
> > +
> > +  const auto buffer = reinterpret_cast<EGLClientBuffer>(backBuf.get());
> > +  const auto config = gl::GLContextEGL::Cast(mGL)->mConfig;
> 
> If you're using EGL_FLEXIBLE_SURFACE_COMPATIBILITY_SUPPORTED_ANGLE, you
> probably shouldn't be passing mConfig. (That is, if mConfig matches the
> format of the EGLSurface, then you don't need
> EGL_FLEXIBLE_SURFACE_COMPATIBILITY_SUPPORTED_ANGLE) Does it work without
> EGL_FLEXIBLE_SURFACE_COMPATIBILITY_SUPPORTED_ANGLE?

Yes.

> It should only work without SURFACE_COMPAT if you happened to have created
> the original context with the correct config. Instead here you should rely
> on SURFACE_COMPAT, and choose (and cache!) the config that reflects the
> formats of these surfaces you want to attach to an otherwise-headless
> context.

I will update a patch as to choose and cache EGLConfig.

> 
> Also don't call GLContextEGL::Cast multiple times when you can reuse the
> result.

I will update it in a next patch.

> 
> @@ +279,5 @@
> > +    pbuffer_attribs);
> > +
> > +  EGLint err = egl->fGetError();
> > +  if (err != LOCAL_EGL_SUCCESS) {
> > +    gfxCriticalError() << "Failed to create Pbuffer of back buffer error: " << gfx::hexa(hr) << " Size:" << size;
> 
> s/hr/err/

Thanks, I will update in a next patch.

> 
> @@ +305,5 @@
> >  
> >  LayoutDeviceIntSize
> >  RenderCompositorANGLE::GetClientSize()
> >  {
> > +  return mSize;
> 
> Are you sure you want to disconnect this query from the true value?

Yes. The return value is passed to WebRender as frame buffer size.

  https://github.com/servo/webrender/blob/master/webrender/src/renderer.rs#L2772

> I don't like that the name of this function sounds like it should be querying
> mWidget->GetClientSize as before, but now is instead returning the 'last
> successfully updated size'.

I will change the name to make clear the intent.
Attachment #8947046 - Flags: review?(jgilbert)
Comment on attachment 8947046 [details] [diff] [review]
patch part 2 - patch - Create SwapChain and manage it directly in RenderCompositorANGLE

Review of attachment 8947046 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLContextProviderEGL.cpp
@@ +955,5 @@
> +/* static */ EGLConfig
> +GLContextEGL::CreateEGLConfig(int32_t bpp, bool enableDepthBuffer)
> +{
> +    EGLConfig config;
> +    if (!CreateConfig(&config, bpp, enableDepthBuffer)) {

Just make the function called here non-static and expose it in the header, and use it directly. No need for this indirection. (Though this called function is /super/ weird. Out params should always go at the end of the arg list, not the beginning)

::: gfx/webrender_bindings/RenderCompositorANGLE.cpp
@@ +35,5 @@
>  RenderCompositorANGLE::RenderCompositorANGLE(RefPtr<widget::CompositorWidget>&& aWidget)
>    : RenderCompositor(Move(aWidget))
> +  , mEGLConfig(nullptr)
> +  , mEGLSurface(nullptr)
> +  , mBufferSize(-1, -1)

0,0 is safer than -1,-1. Prefer using a different variable (mEGLSurface?) as an indication of validity, or use Maybe<IntSize>.

@@ +211,5 @@
>    WaitForPreviousPresentQuery();
>  }
>  
> +bool
> +RenderCompositorANGLE::ResizeBuffers()

The thing is that this doesn't necessarily resize the buffers, such as when the size hasn't changed. (which is the most common case!)

ResizeBufferIfNeeded() or EnsureBuffer().
Attachment #8947046 - Flags: review?(jgilbert) → review+
Blocks: 1415072
(In reply to Jeff Gilbert [:jgilbert] from comment #24)
> Comment on attachment 8947046 [details] [diff] [review]
> patch part 2 - patch - Create SwapChain and manage it directly in
> RenderCompositorANGLE
> 
> Review of attachment 8947046 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLContextProviderEGL.cpp
> @@ +955,5 @@
> > +/* static */ EGLConfig
> > +GLContextEGL::CreateEGLConfig(int32_t bpp, bool enableDepthBuffer)
> > +{
> > +    EGLConfig config;
> > +    if (!CreateConfig(&config, bpp, enableDepthBuffer)) {
> 
> Just make the function called here non-static and expose it in the header,
> and use it directly. No need for this indirection. (Though this called
> function is /super/ weird. Out params should always go at the end of the arg
> list, not the beginning)

I will update it in a next patch.

> 
> ::: gfx/webrender_bindings/RenderCompositorANGLE.cpp
> @@ +35,5 @@
> >  RenderCompositorANGLE::RenderCompositorANGLE(RefPtr<widget::CompositorWidget>&& aWidget)
> >    : RenderCompositor(Move(aWidget))
> > +  , mEGLConfig(nullptr)
> > +  , mEGLSurface(nullptr)
> > +  , mBufferSize(-1, -1)
> 
> 0,0 is safer than -1,-1. Prefer using a different variable (mEGLSurface?) as
> an indication of validity, or use Maybe<IntSize>.

0,0 could not be used here. Then I am going to use Maybe<>.

> 
> @@ +211,5 @@
> >    WaitForPreviousPresentQuery();
> >  }
> >  
> > +bool
> > +RenderCompositorANGLE::ResizeBuffers()
> 
> The thing is that this doesn't necessarily resize the buffers, such as when
> the size hasn't changed. (which is the most common case!)
> 
> ResizeBufferIfNeeded() or EnsureBuffer().

I am going to use ResizeBufferIfNeeded() here.
Attachment #8947361 - Flags: review+
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03d8ada6d1aa
Create SwapChain with DXGI_SWAP_EFFECT_FLIP_SEQUENTIAL if possible in ANGLE r=jgilbert
https://hg.mozilla.org/mozilla-central/rev/03d8ada6d1aa
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Depends on: 1435200
No longer depends on: 1435200
Depends on: 1435995
You need to log in before you can comment on or make changes to this bug.