1052234 - Implement 'Baking' WebGL compositing project

Status: RESOLVED WONTFIX

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

I am slitting off a separate bug here to handle the implementation of bug 1023558.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 1: Implement ShSurfHandle

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 2: Implement GLScreenBuffer changes for Project Baking

Comment 3

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3: Implement CanvasClient backend for Project Baking

Comment 4

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 4: WIP Implement TexClient backend

Comment 5

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 5: Use NV_fence to get PollSync on ANGLE

Comment 6

[Kelsey Gilbert [:jgilbert]]

These patches are sufficient to implement Project Baking for Windows with ANGLE shared-HANDLE compositing, but no other non-readback backends. (yet)

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Your project codename skills are the *worst*! I hope there's a companion Project Toaster Oven or something at least...

Comment 8

[Kelsey Gilbert [:jgilbert]]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #7)
> Your project codename skills are the *worst*! I hope there's a companion
> Project Toaster Oven or something at least...

Careful what you ask for... :)

Comment 9

[u480271]

Comment on attachment 8471294 [details] [diff] [review]
patch 1: Implement ShSurfHandle

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

::: gfx/gl/SharedSurface.h
@@ +30,5 @@
>  namespace gl {
>  
>  class GLContext;
>  class SurfaceFactory;
> +class ShSurfHandle;

Sh prefix is used by ANGLE. Is that an issue?

Comment 10

[u480271]

Comment on attachment 8471295 [details] [diff] [review]
patch 2: Implement GLScreenBuffer changes for Project Baking

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

::: gfx/gl/GLScreenBuffer.cpp
@@ +421,5 @@
> +    mFront = mBack;
> +    mBack = newBack;
> +
> +    // Fence before copying.
> +    if (mFront) {

Remove extra { }'s

Comment 11

[u480271]

Comment on attachment 8471298 [details] [diff] [review]
patch 3: Implement CanvasClient backend for Project Baking

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

::: gfx/gl/GLContext.cpp
@@ +1699,5 @@
>  
>  SharedSurface*
>  GLContext::RequestFrame()
>  {
>      MOZ_ASSERT(mScreen);

Superfluous.

::: gfx/gl/GLContext.h
@@ +1452,5 @@
>          BEFORE_GL_CALL;
>          mSymbols.fReadBuffer(mode);
>          AFTER_GL_CALL;
>      }
> +	

Whitespace

::: gfx/gl/ScopedGLHelpers.cpp
@@ +499,5 @@
> +
> +    if (scopedVal != mOldVal) {
> +        gl->fPixelStorei(LOCAL_GL_PACK_ALIGNMENT, scopedVal);
> +    } else {
> +      // Don't try to re-set it during unwrap.

Indent.

::: gfx/layers/client/CanvasClient.cpp
@@ +240,5 @@
>  }
>  
> +////////////////////////////////////////////////////////////////////////
> +
> +CanvasClientBaking::CanvasClientBaking(CompositableForwarder* aLayerForwarder,

I'll defer this to Matt Woodrow.

::: gfx/layers/client/CanvasClient.h
@@ +143,5 @@
> +// Used for GL canvases where we don't need to do any readback, i.e., with a
> +// GL backend.
> +class CanvasClientBaking : public CanvasClient
> +{
> +private:

Superfluous

Comment 12

[u480271]

Comment on attachment 8471300 [details] [diff] [review]
patch 4: WIP Implement TexClient backend

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

::: gfx/layers/client/CanvasClient.cpp
@@ +452,5 @@
> +{
> +    TextureFlags flags = baseFlags | TextureFlags::DEALLOCATE_CLIENT;
> +
> +    switch (abstractSurf->mType) {
> +      case SharedSurfaceType::EGLSurfaceANGLE: {

Indent. 2 or 4, not both.

Comment 13

[u480271]

Comment on attachment 8471302 [details] [diff] [review]
patch 5: Use NV_fence to get PollSync on ANGLE

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

::: gfx/gl/SharedSurfaceANGLE.cpp
@@ +44,5 @@
>  {
> +    if (mFence) {
> +        MOZ_ASSERT(mGL->IsExtensionSupported(GLContext::NV_fence));
> +        mGL->fSetFence(mFence, LOCAL_GL_ALL_COMPLETED_NV);
> +        mGL->fFlush();

Is the flush necessary?

@@ +57,5 @@
> +{
> +    if (mFence) {
> +        mGL->MakeCurrent();
> +        mGL->fFinishFence(mFence);
> +        return true;

Superfluous

::: gfx/layers/client/CanvasClient.cpp
@@ +495,5 @@
>      if (mBaking->Surf()->PollSync()) {
>        // Done baking.
>        mNextFront = mBaking;
>        mBaking = nullptr;
> +    } else {

Do not land?

Comment 14

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3.5: Fixes to CanvasClient

Comment 15

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 4: Implement ANGLE TexClient backend WIP

Comment 16

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 10: Add delay slot

Comment 17

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 4: Implement ANGLE TexClient backend WIP

Comment 18

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8471294 [details] [diff] [review]
patch 1: Implement ShSurfHandle

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

::: gfx/gl/SharedSurface.h
@@ +158,5 @@
>      // Auto-deletes surfs of the wrong type.
>      void Recycle(UniquePtr<SharedSurface> surf);
>  };
>  
> +class ShSurfHandle : public RefCounted<ShSurfHandle>

Can't we just make SharedSurface refcounted and put the mFactory weakptr in it?

This wrapper doesn't seem to add much.

Also, I'm not a fan of abbreviations like 'Sh', since it's non obvious what it's short for (especially once it's used in layers code). The extra typing to name is SharedSurfaceHandle seems worthwhile to me.

Comment 19

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8471298 [details] [diff] [review]
patch 3: Implement CanvasClient backend for Project Baking

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

::: gfx/layers/client/CanvasClient.cpp
@@ +261,5 @@
> +
> +static TemporaryRef<TextureClient>
> +TexClientFromReadback(SharedSurface* src, ISurfaceAllocator* allocator,
> +                      TextureFlags baseFlags,
> +                      LayersBackend layersBackend)

This seems like an enormous amount of code for an operation that we already do. CopyableCanvasLayer::Update has code for reading back into a TextureClient, why can't we use that?

I'm sure we'd need to refactor a few things to get at what we need, but that seems well worth it, we have to much GL readback code already.

@@ +479,5 @@
> +
> +  // Alright, now sort out the IPC goop.
> +  SharedSurface* surf = mNextFront->Surf();
> +  auto forwarder = GetForwarder();
> +  auto flags = GetTextureFlags();

You only use this once, and within the if, not much point having the temp.

@@ +485,5 @@
> +  // Get a TexClient from our surf.
> +  RefPtr<TextureClient> newTex = TexClientFromShSurf(surf);
> +  if (!newTex) {
> +    auto manager = aLayer->ClientManager();
> +    auto shadowForwarder = manager->AsShadowForwarder();

forwarder and shadowForwarder are the same object here, just cast differently. GetCompositorBackendType exists on CompositableForwarder (forwarder), so you don't any need of these temp vars.

@@ +489,5 @@
> +    auto shadowForwarder = manager->AsShadowForwarder();
> +    auto layersBackend = shadowForwarder->GetCompositorBackendType();
> +
> +    newTex = TexClientFromReadback(surf, forwarder, flags,
> +                                   layersBackend);

If we've created a new TextureClient and readback into it, then we're no longer referencing the SharedSurface aren't we?

We could shortcut the pipeline here and recycle it immediately.

Comment 20

[Kelsey Gilbert [:jgilbert]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #19)
> Comment on attachment 8471298 [details] [diff] [review]
> patch 3: Implement CanvasClient backend for Project Baking
> 
> Review of attachment 8471298 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/client/CanvasClient.cpp
> @@ +261,5 @@
> > +
> > +static TemporaryRef<TextureClient>
> > +TexClientFromReadback(SharedSurface* src, ISurfaceAllocator* allocator,
> > +                      TextureFlags baseFlags,
> > +                      LayersBackend layersBackend)
> 
> This seems like an enormous amount of code for an operation that we already
> do. CopyableCanvasLayer::Update has code for reading back into a
> TextureClient, why can't we use that?
> 
> I'm sure we'd need to refactor a few things to get at what we need, but that
> seems well worth it, we have to much GL readback code already.

I'll take a look. I took care to make this use as few copies as possible, but I'll check what the existing version does.
> 
> @@ +479,5 @@
> > +
> > +  // Alright, now sort out the IPC goop.
> > +  SharedSurface* surf = mNextFront->Surf();
> > +  auto forwarder = GetForwarder();
> > +  auto flags = GetTextureFlags();
> 
> You only use this once, and within the if, not much point having the temp.
> 
> @@ +485,5 @@
> > +  // Get a TexClient from our surf.
> > +  RefPtr<TextureClient> newTex = TexClientFromShSurf(surf);
> > +  if (!newTex) {
> > +    auto manager = aLayer->ClientManager();
> > +    auto shadowForwarder = manager->AsShadowForwarder();
> 
> forwarder and shadowForwarder are the same object here, just cast
> differently. GetCompositorBackendType exists on CompositableForwarder
> (forwarder), so you don't any need of these temp vars.
I would rather not rely on that, and just do this naively, unless there's a performance or correctness reason not to do so.
> 
> @@ +489,5 @@
> > +    auto shadowForwarder = manager->AsShadowForwarder();
> > +    auto layersBackend = shadowForwarder->GetCompositorBackendType();
> > +
> > +    newTex = TexClientFromReadback(surf, forwarder, flags,
> > +                                   layersBackend);
> 
> If we've created a new TextureClient and readback into it, then we're no
> longer referencing the SharedSurface aren't we?
> 
> We could shortcut the pipeline here and recycle it immediately.
We should consider this for a follow-up.

Comment 21

[Kelsey Gilbert [:jgilbert]]

   https://tbpl.mozilla.org/?tree=Try&rev=50219f766238

Comment 22

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8471302 [details] [diff] [review]
patch 5: Use NV_fence to get PollSync on ANGLE

This patch is in a dependent bug now.

Comment 23

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3.7: Use {Fence,Poll,Wait}_ThreadLocal

Comment 24

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3.8: Decompose readback function a bit.

This should help with the complexity of this function. (now two functions)

The larger function pretty much needs to be all stuck together though, because of tricky dependencies.

Comment 25

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8480928 [details] [diff] [review]
patch 3.8: Decompose readback function a bit.

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

I still don't understand why we need all this code.

The vast majority of this function appears to be boilerplate which already exists in GLUploadHelpers.

Why do we need new versions of this code? CopyableCanvasLayer already solves the same problem (readback into an existing buffer without using a temporary if possible), so I don't see why we need new code.

Comment 26

[u480271]

Comment on attachment 8480911 [details] [diff] [review]
patch 3.7: Use {Fence,Poll,Wait}_ThreadLocal

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

::: gfx/gl/GLScreenBuffer.cpp
@@ +431,5 @@
>      mBack = newBack;
>  
>      // Fence before copying.
>      if (mFront) {
> +        mFront->Surf()->Fence_ThreadLocal();

I don't get which thread is local?

Comment 27

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #26)
> Comment on attachment 8480911 [details] [diff] [review]
> patch 3.7: Use {Fence,Poll,Wait}_ThreadLocal
> 
> Review of attachment 8480911 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLScreenBuffer.cpp
> @@ +431,5 @@
> >      mBack = newBack;
> >  
> >      // Fence before copying.
> >      if (mFront) {
> > +        mFront->Surf()->Fence_ThreadLocal();
> 
> I don't get which thread is local?

I don't have a good name for it. It means "I promise I'll only call Poll/Wait from the same thread I called Fence on", which would be the content thread.

Comment 28

[Kelsey Gilbert [:jgilbert]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #25)
> Comment on attachment 8480928 [details] [diff] [review]
> patch 3.8: Decompose readback function a bit.
> 
> Review of attachment 8480928 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I still don't understand why we need all this code.
> 
> The vast majority of this function appears to be boilerplate which already
> exists in GLUploadHelpers.
> 
> Why do we need new versions of this code? CopyableCanvasLayer already solves
> the same problem (readback into an existing buffer without using a temporary
> if possible), so I don't see why we need new code.

Maybe I'm going too far trying to do things correctly. I'm trying to create a texClient of the right RGBA/BGRA format, but I can't know which format that is until we have bound a framebuffer to read back from.

This still doesn't work right for D3D11, since it flat-out refuses to alloc a RGBA texClient.

Here's the question. Since RGBA texClients don't have full support, should I just mime CopyableCanvasLayer and unconditionally use BGRA, even if it's wrong?
This doesn't seem like a good way to use our own APIs, hence the complexity in the code here.

Of particular note is that with ANGLE, we should be getting readback as BGRA, and not RGBA like we do currently in CopyableCanvasLayer. (which is slower)

Comment 29

[Kelsey Gilbert [:jgilbert]]

In short the CopyableCanvasLayer code should be replaced with this code.

Perhaps we should consider having a readback function that looks like:
void ReadPixels_FastRGBA(uint8_t* dest, GLenum* out_readFormat, GLenum* out_readType);

Or:
TemporaryRef<TextureClient> ReadPixels(ISurfaceAllocator* allocator, gfx::BackendType backendType, GLenum requestedFormat, GLenum requestedType, GLenum* out_actualFormat, GLenum* out_actualType);

Comment 30

[Matt Woodrow (:mattwoodrow)]

Ok, thanks, that makes a lot more sense.

We probably should be requesting a 'BufferTextureClient' for readback so that we don't ever get a d3d11 TextureClient.

Can we split the function into two pieces (binding the framebuffer, reading back from it) and move them into GLUploadHelpers?

Comment 31

[Matt Woodrow (:mattwoodrow)]

(In reply to Jeff Gilbert [:jgilbert] from comment #29)
> In short the CopyableCanvasLayer code should be replaced with this code.
> 
> Perhaps we should consider having a readback function that looks like:
> void ReadPixels_FastRGBA(uint8_t* dest, GLenum* out_readFormat, GLenum*
> out_readType);
> 
> Or:
> TemporaryRef<TextureClient> ReadPixels(ISurfaceAllocator* allocator,
> gfx::BackendType backendType, GLenum requestedFormat, GLenum requestedType,
> GLenum* out_actualFormat, GLenum* out_actualType);

In general I'd prefer to have the layers code avoid knowing too much about GL internals, and vice-versa. So having a 'bind framebuffer for readback' function that returns a state objects seems easiest. We can use that to determine which TextureClient to create, and then call the actual readback function. RAII can make sure we clean everything up.

Comment 32

[Kelsey Gilbert [:jgilbert]]

I agree about avoiding interdependence. I'll look at adding something for this, but it'll be a little less-trivial design-wise.

Comment 33

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3.8: Refactor the readback func.

In going through this, this is way nicer than the mess we have currently for ShSurf->ReadPixels and the mechanisms we have in GLScreenBuffer to handle mapping FBs to ShSurfs.

Comment 34

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3.7: Use {Fence,Poll,Wait}_ThreadLocal

r=kamidphish
Unbitrot.

Comment 35

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3.81: Add yflip for readback

It's glReadPixels, so it needs yflip to become origin-top-left.

Comment 36

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 0: Allow R8G8B8{A,X}8 TextureHosts for D3D11

Comment 37

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8481748 [details] [diff] [review]
patch 0: Allow R8G8B8{A,X}8 TextureHosts for D3D11

This patch is an independent prereq, so let's just land it:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/746111f6f9af

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/746111f6f9af

Comment 39

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8481727 [details] [diff] [review]
patch 3.8: Refactor the readback func.

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

Looks good, but it'll fail on mac.

::: gfx/layers/client/CanvasClient.cpp
@@ +372,5 @@
>      uint8_t* lockedBits;
>      gfx::IntSize lockedSize;
>      int32_t lockedStride;
>      gfx::SurfaceFormat lockedFormat;
> +    MOZ_ALWAYS_TRUE( drawTarget->LockBits(&lockedBits, &lockedSize, &lockedStride,

This isn't guaranteed to succeed. LockBits is only implemented for DrawTargetCairo, and we can easily get other DT types (like CG).

BufferTextureClient exposes GetBuffer(), GetSize(), GetFormat() and you can probably add something for the stride or just recompute it.

Comment 40

[Kelsey Gilbert [:jgilbert]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #39)
> Comment on attachment 8481727 [details] [diff] [review]
> patch 3.8: Refactor the readback func.
> 
> Review of attachment 8481727 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, but it'll fail on mac.
> 
> ::: gfx/layers/client/CanvasClient.cpp
> @@ +372,5 @@
> >      uint8_t* lockedBits;
> >      gfx::IntSize lockedSize;
> >      int32_t lockedStride;
> >      gfx::SurfaceFormat lockedFormat;
> > +    MOZ_ALWAYS_TRUE( drawTarget->LockBits(&lockedBits, &lockedSize, &lockedStride,
> 
> This isn't guaranteed to succeed. LockBits is only implemented for
> DrawTargetCairo, and we can easily get other DT types (like CG).
> 
> BufferTextureClient exposes GetBuffer(), GetSize(), GetFormat() and you can
> probably add something for the stride or just recompute it.

IIRC, this does *not* work. I tried this originally, but got some weird errors. Debugging showed that GetBuffer() returns a pointer to the beginning of the memory buffer, but the buffer started with a header which recorded the size and format of the (shmem?) data. By writing into GetBuffer(), I would corrupt this header data.

Comment 41

[Kelsey Gilbert [:jgilbert]]

What should I do if LockBits fails? Leave it blank?

Comment 42

[Matt Woodrow (:mattwoodrow)]

(In reply to Jeff Gilbert [:jgilbert] from comment #40)
> IIRC, this does *not* work. I tried this originally, but got some weird
> errors. Debugging showed that GetBuffer() returns a pointer to the beginning
> of the memory buffer, but the buffer started with a header which recorded
> the size and format of the (shmem?) data. By writing into GetBuffer(), I
> would corrupt this header data.

Good point! Ok, pass BackendType::CAIRO into the texture client creation function to ensure that we always get DrawTargetCairo created for the data. Then we can happily assert that LockBits succeeds.

Comment 43

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3.8: Refactor the readback func.

r=kamidphish already.

Comment 44

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8483134 [details] [diff] [review]
patch 3.8: Refactor the readback func.

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

Looks good, just a few more bits of GL-ness that I'd like to keep out of layers/ but it's not critical.

::: gfx/layers/client/CanvasClient.cpp
@@ +329,5 @@
>      // We have a source FB, now we need a format.
>      GLenum destFormat = LOCAL_GL_BGRA;
>      GLenum destType = LOCAL_GL_UNSIGNED_INT_8_8_8_8_REV;
>      GLenum readFormat;
>      GLenum readType;

I'd still prefer if this code that retrieves the GL formats and computes the necessary moz2d format could go into the GLUploadHelpers file instead of here.

@@ +385,5 @@
>  
>      {
>        ScopedPackAlignment autoAlign(gl, 4);
>  
> +      gl->raw_fReadPixels(0, 0, width, height, readFormat, readType, lockedBits);

Same here, I'd prefer if ScopedReadbackFB exposed a function for this instead of directly calling GL code.

Comment 45

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8471298 [details] [diff] [review]
patch 3: Implement CanvasClient backend for Project Baking

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

R+ since the readback comments have been addressed in a separate patch.

Comment 46

[Kelsey Gilbert [:jgilbert]]

Let's see how broken the build is:
  https://tbpl.mozilla.org/?tree=Try&rev=0d3f4b4665d8

Comment 47

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 4.1: ANGLE backend

Comment 48

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 4.2: Shared GLTexture backend

Comment 49

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 4.3: EGLImage backend

Comment 50

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 4.4: IOSurface backend

Comment 51

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 4.5: Gralloc backend

Comment 52

[Kelsey Gilbert [:jgilbert]]

With just readback:
 https://tbpl.mozilla.org/?tree=Try&rev=d0eca3b2edbb

Comment 53

[u480271]

Comment on attachment 8483972 [details] [diff] [review]
patch 4.2: Shared GLTexture backend

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

I didn't review Layers, TextureClient/TextureHost, SurfaceDescriptor too closely since these aren't areas of my expertise. Other the rest looks like basic refactorings of gl context usage and LTGM.

::: gfx/gl/SharedSurface.cpp
@@ -31,5 @@
>      {
>          // Here, we actually need to blit through a temp surface, so let's make one.
>          UniquePtr<SharedSurface_GLTexture> tempSurf;
>          tempSurf = SharedSurface_GLTexture::Create(gl,
> -                                                   gl,

O_O

::: gfx/gl/SharedSurfaceGL.cpp
@@ +196,5 @@
>  
>        ownsTex = true;
>      }
>  
> +    ret.reset( new SharedSurface_GLTexture(gl, size, hasAlpha, tex, ownsTex) );

WS after ( is ugly and an abomination unto Nuggan.

Comment 54

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8483971 [details] [diff] [review]
patch 4.1: ANGLE backend

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

::: gfx/layers/client/CanvasClient.cpp
@@ +259,2 @@
>  static TemporaryRef<TextureClient>
> +TexClientFromShSurf(SharedSurface* surf, TextureFlags baseFlags)

Same here, much prefer using the full names.

::: gfx/layers/d3d11/TextureD3D11.h
@@ +67,5 @@
>                  TextureAllocationFlags aAllocFlags = ALLOC_DEFAULT) const MOZ_OVERRIDE;
>  
> +  bool InitWithSurface(gfx::IntSize aSize, HANDLE aShareHandle);
> +
> +  static TemporaryRef<TextureClient> FromShSurf(gl::SharedSurface* surf,

FromSharedSurface

Comment 55

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8483976 [details] [diff] [review]
patch 4.4: IOSurface backend

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

::: gfx/layers/opengl/MacIOSurfaceTextureClientOGL.h
@@ +41,5 @@
>    // an external producer.
>    virtual TemporaryRef<TextureClient>
>    CreateSimilar(TextureFlags, TextureAllocationFlags) const MOZ_OVERRIDE { return nullptr; }
>  
> +  static TemporaryRef<TextureClient> FromShSurf(gl::SharedSurface* surf,

FromSharedSurface

Comment 56

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8483977 [details] [diff] [review]
patch 4.5: Gralloc backend

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

::: gfx/layers/opengl/GrallocTextureClient.h
@@ +117,5 @@
>    virtual TemporaryRef<TextureClient>
>    CreateSimilar(TextureFlags aFlags = TextureFlags::DEFAULT,
>                  TextureAllocationFlags aAllocFlags = ALLOC_DEFAULT) const MOZ_OVERRIDE;
>  
> +  static TemporaryRef<TextureClient> FromShSurf(gl::SharedSurface* surf,

Same here.

Comment 57

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8483974 [details] [diff] [review]
patch 4.3: EGLImage backend

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

Looks good, but nical should review Texture* additions.

::: gfx/layers/opengl/TextureClientOGL.h
@@ +199,5 @@
> +  {
> +    return false;
> +  }
> +
> +  static TemporaryRef<TextureClient> FromShSurf(gl::SharedSurface* surf,

FromSharedSurface

Comment 58

[Nicolas Silva [:nical]]

Comment on attachment 8483972 [details] [diff] [review]
patch 4.2: Shared GLTexture backend

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

::: gfx/layers/opengl/TextureHostOGL.h
@@ +455,5 @@
> + * A TextureHost for shared GL Textures
> + *
> + * Most of the logic actually happens in SharedTextureSourceOGL.
> + */
> +class SharedGLTextureHost : public TextureHost

A bit confusing since there is also SharedTextureHostOGL. Do we really need both? If so please add a comment explaining the difference.
Also, you left "SharedTextureSourceOGL" in the above comment where I assume you meant GLTextureSource.
I would strongly prefer that we stick with the naming conventions (ex, backend at then end of the name). Maybe rename SharedTextureHostOGL into SharedHandleTextureHostOGL and have this one called GLTextureHostOGL? It makes GL appears twice but I can't seem to come up with a better name (for GLTexure vs SharedHandle).
Same thing for the TextureSource of course.

Comment 59

[Nicolas Silva [:nical]]

Comment on attachment 8483974 [details] [diff] [review]
patch 4.3: EGLImage backend

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

::: gfx/layers/opengl/TextureClientOGL.h
@@ +146,5 @@
>  
> +/**
> + * A TextureClient implementation for EGLImage.
> + */
> +class EGLImageTextureClient : public TextureClient

Same comment as with the previous patch about naming conventions.
EGLImageTextureClientOGL would be better (until we come up with a good naming scheme and fix everything up I'd like that we at least keep things coherent, even if the current naming scheme is not great)

Comment 60

[Kelsey Gilbert [:jgilbert]]

We did something different in bug 1066280.

Comment 61

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.