Implement 'Baking' WebGL compositing project

RESOLVED WONTFIX

Status

()

defect
RESOLVED WONTFIX
5 years ago
Last year

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(14 attachments, 7 obsolete attachments)

3.13 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
7.69 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
29.33 KB, patch
kamidphish
: review+
mattwoodrow
: review+
Details | Diff | Splinter Review
588 bytes, patch
kamidphish
: review+
Details | Diff | Splinter Review
2.73 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
1.22 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
854 bytes, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
825 bytes, patch
bas.schouten
: review+
jgilbert
: checkin+
Details | Diff | Splinter Review
17.18 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
7.48 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
30.40 KB, patch
nical
: review+
kamidphish
: review+
Details | Diff | Splinter Review
17.68 KB, patch
nical
: review+
Details | Diff | Splinter Review
3.63 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
3.70 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
I am slitting off a separate bug here to handle the implementation of bug 1023558.
Attachment #8471294 - Flags: review?(dglastonbury)
Attachment #8471298 - Flags: review?(matt.woodrow)
Attachment #8471298 - Flags: review?(dglastonbury)
Attachment #8471300 - Flags: feedback?(matt.woodrow)
Attachment #8471300 - Flags: feedback?(dglastonbury)
Attachment #8471302 - Flags: review?(dglastonbury)
These patches are sufficient to implement Project Baking for Windows with ANGLE shared-HANDLE compositing, but no other non-readback backends. (yet)
Your project codename skills are the *worst*! I hope there's a companion Project Toaster Oven or something at least...
(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 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?
Attachment #8471294 - Flags: review?(dglastonbury) → review+
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
Attachment #8471295 - Flags: review?(dglastonbury) → review+
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
Attachment #8471298 - Flags: review?(dglastonbury) → review+
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.
Attachment #8471300 - Flags: feedback?(dglastonbury) → feedback+
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?
Attachment #8471302 - Flags: review?(dglastonbury) → review-
Attachment #8473434 - Flags: review?(dglastonbury)
Attachment #8471300 - Attachment is obsolete: true
Attachment #8471300 - Flags: feedback?(matt.woodrow)
Attachment #8473436 - Flags: feedback?(dglastonbury)
Attachment #8473438 - Flags: review?(dglastonbury)
Attachment #8473436 - Attachment is obsolete: true
Attachment #8473436 - Flags: feedback?(dglastonbury)
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 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.
Attachment #8473434 - Flags: review?(dglastonbury) → review+
Attachment #8473438 - Flags: review?(dglastonbury) → review+
(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.
Depends on: 1060085
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.
Attachment #8471302 - Attachment is obsolete: true
Attachment #8480911 - Flags: review?(dglastonbury)
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.
Attachment #8480928 - Flags: review?(matt.woodrow)
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 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?
Attachment #8480911 - Flags: review?(dglastonbury) → review+
(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.
(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)
Flags: needinfo?(matt.woodrow)
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);
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?
(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.
Flags: needinfo?(matt.woodrow)
I agree about avoiding interdependence. I'll look at adding something for this, but it'll be a little less-trivial design-wise.
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.
Attachment #8480928 - Attachment is obsolete: true
Attachment #8480928 - Flags: review?(matt.woodrow)
Attachment #8481727 - Flags: review?(matt.woodrow)
Attachment #8481727 - Flags: review?(dglastonbury)
r=kamidphish
Unbitrot.
Attachment #8480911 - Attachment is obsolete: true
Attachment #8481736 - Flags: review+
It's glReadPixels, so it needs yflip to become origin-top-left.
Attachment #8481747 - Flags: review?(matt.woodrow)
Attachment #8481747 - Attachment description: patch 3.81 → patch 3.81: Add yflip for readback
Attachment #8481748 - Flags: review?(bas) → review+
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
Attachment #8481748 - Attachment description: patch 3.82: Allow R8G8B8{A,X}8 TextureHosts for D3D11 → patch 0: Allow R8G8B8{A,X}8 TextureHosts for D3D11
Attachment #8481748 - Flags: checkin+
Keywords: leave-open
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.
Attachment #8481747 - Flags: review?(matt.woodrow) → review+
Attachment #8481727 - Flags: review?(dglastonbury) → review+
(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.
Flags: needinfo?(matt.woodrow)
What should I do if LockBits fails? Leave it blank?
(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.
Flags: needinfo?(matt.woodrow)
r=kamidphish already.
Attachment #8481727 - Attachment is obsolete: true
Attachment #8481727 - Flags: review?(matt.woodrow)
Attachment #8483134 - Flags: review?(matt.woodrow)
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.
Attachment #8483134 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8471298 - Flags: review?(matt.woodrow) → review+
Let's see how broken the build is:
  https://tbpl.mozilla.org/?tree=Try&rev=0d3f4b4665d8
Attachment #8473440 - Attachment is obsolete: true
Attachment #8483971 - Flags: review?(matt.woodrow)
Attachment #8483972 - Flags: review?(matt.woodrow)
Attachment #8483972 - Flags: review?(dglastonbury)
Attachment #8483974 - Flags: review?(matt.woodrow)
Attachment #8483976 - Flags: review?(matt.woodrow)
Attachment #8483977 - Flags: review?(matt.woodrow)
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.
Attachment #8483972 - Flags: review?(matt.woodrow) → review+
Attachment #8483972 - Flags: review?(dglastonbury) → review?(matt.woodrow)
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
Attachment #8483971 - Flags: review?(matt.woodrow) → review+
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
Attachment #8483976 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8483977 - Flags: review?(matt.woodrow) → review+
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
Attachment #8483974 - Flags: review?(matt.woodrow) → review?(nical.bugzilla)
Attachment #8483972 - Flags: review?(matt.woodrow) → review?(nical.bugzilla)
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.
Attachment #8483972 - Flags: review?(nical.bugzilla) → review+
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)
Attachment #8483974 - Flags: review?(nical.bugzilla) → review+
We did something different in bug 1066280.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
See Also: → 1066280
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.