Closed Bug 1049957 Opened 5 years ago Closed 5 years ago

Use UniquePtr for ownership in GLScreenBuffer, SharedSurface, SurfaceFactory, and SurfaceStream

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: jgilbert, Assigned: jgilbert)

Details

Attachments

(7 files, 2 obsolete files)

We should use UniquePtr to establish and track ownership.
Yes please.
Attachment #8468887 - Flags: review?(jwalden+bmo)
Attachment #8468888 - Flags: review?(dglastonbury)
Attachment #8468889 - Flags: review?(dglastonbury)
Attachment #8468890 - Flags: review?(dglastonbury)
Attachment #8468887 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8468888 [details] [diff] [review]
patch 1: GLScreenBuffer

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

::: gfx/gl/GLScreenBuffer.cpp
@@ +606,5 @@
>  
>      if (surf->mAttachType == AttachmentType::Screen) {
>          // Don't need anything. Our read buffer will be the 'screen'.
>  
> +        return UniquePtr<ReadBuffer>( new ReadBuffer(gl, 0, 0, 0,

Not a fan of spaces after ( but I understand the reason.
Is it not possible to use MakeUnique? I suppose not, since contructor params.
Attachment #8468888 - Flags: review?(dglastonbury) → review+
Attachment #8468889 - Flags: review?(dglastonbury) → review+
Comment on attachment 8468888 [details] [diff] [review]
patch 1: GLScreenBuffer

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

::: gfx/gl/GLScreenBuffer.cpp
@@ +66,5 @@
>  
> +    UniquePtr<GLScreenBuffer> ret( new GLScreenBuffer(gl, caps,
> +                                                      Move(factory),
> +                                                      stream) );
> +    return Move(ret);

Could just

  return MakeUnique<GLScreenBuffer>(gl, caps, Move(factor), stream);

to avoid a local, a move out of it, and a bunch of repetition.

@@ +568,5 @@
>      gl->fGenFramebuffers(1, &fb);
>      gl->AttachBuffersToFB(0, colorMSRB, depthRB, stencilRB, fb);
>  
> +    UniquePtr<DrawBuffer> ret( new DrawBuffer(gl, size, fb, colorMSRB,
> +                                              depthRB, stencilRB) );

auto ret = MakeUnique<DrawBuffer>(gl, size, fb, colorMSRB, depthRB, stencilRB); might be nicer.

@@ +607,5 @@
>      if (surf->mAttachType == AttachmentType::Screen) {
>          // Don't need anything. Our read buffer will be the 'screen'.
>  
> +        return UniquePtr<ReadBuffer>( new ReadBuffer(gl, 0, 0, 0,
> +                                                     surf) );

return MakeUnique<ReadBuffer>(gl, 0, 0, 0, surf);

@@ +642,5 @@
>      gl->AttachBuffersToFB(colorTex, colorRB, depthRB, stencilRB, fb, target);
>      gl->mFBOMapping[fb] = surf;
>  
> +    UniquePtr<ReadBuffer> ret( new ReadBuffer(gl, fb, depthRB,
> +                                              stencilRB, surf) );

auto ret = MakeUnique<ReadBuffer>(gl, fb, depthRB, stencilRB, surf);
Comment on attachment 8468890 [details] [diff] [review]
patch 3: SharedSurface, SurfaceFactory

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

::: gfx/gl/GLScreenBuffer.cpp
@@ +36,5 @@
>      {
>          return nullptr;
>      }
>  
> +    UniquePtr<SurfaceFactory> factory = nullptr;

Explicit initialization isn't necessary, it'll auto-init to nullptr if you're fine with that.

::: gfx/gl/GLScreenBuffer.h
@@ +126,5 @@
>      GLContext* const mGL; // Owns us.
>  public:
>      const SurfaceCaps mCaps;
>  protected:
> +    UniquePtr<SurfaceFactory> mFactory; // Owned by us.

UniquePtr makes explicit what the comment says, seems to me.  Same for the others.  But up to you.

::: gfx/gl/SharedSurface.cpp
@@ +259,1 @@
>          mScraps.pop();

If this queue already contains UniquePtr, popping out of it will move out the UniquePtr, and then that temporary that's created by the pop() call will destroy the contained surface for you.  No need for a local at all here, if the queue contains UniquePtr.

@@ +269,1 @@
>          mScraps.pop();

UniquePtr<SharedSurface> cur = mScraps.pop();

@@ +286,4 @@
>  
>      if (surf->mType == mType) {
> +        mScraps.push(Move(surf));
> +        MOZ_ASSERT(!surf);

You're not passing in a reference any more, so whatever the state of |surf| at this point, nobody can observe it, so who cares?  And to the extent null matters for double-deletion, that's inherent in UniquePtr, not really worth asserting, to me.

::: gfx/gl/SharedSurfaceANGLE.cpp
@@ +200,5 @@
>      }
>  
> +    typedef SharedSurface_ANGLEShareHandle ptrT;
> +    return UniquePtr<ptrT>( new ptrT(gl, egl, size, hasAlpha, context,
> +                                     pbuffer, shareHandle) );

return MakeUnique<ptrT>(gl, egl, size, hasAlpha, context, pbuffer, sharedHandle);

@@ +218,5 @@
> +        return nullptr;
> +    }
> +
> +    typedef SurfaceFactory_ANGLEShareHandle ptrT;
> +    return UniquePtr<ptrT>( new ptrT(gl, egl, caps) );

return MakeUnique<ptrT>(gl, egl, caps);

::: gfx/gl/SharedSurfaceEGL.cpp
@@ +47,5 @@
>      }
>  
> +    typedef SharedSurface_EGLImage ptrT;
> +    return UniquePtr<ptrT>( new ptrT(prodGL, egl, size, hasAlpha,
> +                                     formats, prodTex, image) );

MakeUnique

@@ +202,5 @@
>          return nullptr;
>      }
>  
> +    typedef SurfaceFactory_EGLImage ptrT;
> +    return UniquePtr<ptrT>( new ptrT(prodGL, context, caps) );

MakeUnique

::: gfx/gl/SharedSurfaceGL.cpp
@@ +46,5 @@
>          MOZ_CRASH("Unhandled Tex format.");
>      }
> +
> +    typedef SharedSurface_Basic ptrT;
> +    return UniquePtr<ptrT>( new ptrT(gl, size, hasAlpha, format, tex) );

MakeUnique

@@ +128,5 @@
>      }
>  
> +    typedef SharedSurface_GLTexture ptrT;
> +    return UniquePtr<ptrT>( new ptrT(prodGL, consGL, size, hasAlpha,
> +                                     tex, ownsTex) );

MakeUnique

::: gfx/gl/SharedSurfaceGralloc.cpp
@@ +112,5 @@
>  
> +
> +    typedef SharedSurface_Gralloc ptrT;
> +    UniquePtr<ptrT> surf( new ptrT(prodGL, size, hasAlpha, egl,
> +                                   allocator, grallocTC, prodTex) );

auto surf = MakeUnique<ptrT>(...);

::: gfx/gl/SharedSurfaceIO.cpp
@@ +23,5 @@
>  
>      gfx::IntSize size(surface->GetWidth(), surface->GetHeight());
> +
> +    typedef SharedSurface_IOSurface ptrT;
> +    return UniquePtr<ptrT>( new ptrT(ioSurf, gl, size, hasAlpha) );

MakeUnique

@@ +148,5 @@
>      gfx::IntSize maxDims(MacIOSurface::GetMaxWidth(),
>                           MacIOSurface::GetMaxHeight());
> +
> +    typedef SurfaceFactory_IOSurface ptrT;
> +    return UniquePtr<ptrT>( new ptrT(gl, caps, maxDims) );

MakeUnique

::: gfx/gl/SurfaceStream.cpp
@@ +87,5 @@
>      if (surf) {
>          // Before next use, wait until SharedSurface's buffer
>          // is no longer being used.
>          surf->WaitForBufferOwnership();
> +        mSurfaces.insert(surf.get());

This...reads very dodgy.  Perhaps all the lifetimes work out all right, but this sort of shared-ownership of pointers thing is not really what UniquePtr was designed for at all.  I would hesitate to do this if it were me.

@@ +103,5 @@
> +    UniquePtr<SharedSurface>& to = *slotTo;
> +
> +    MOZ_ASSERT(!to);
> +    to = Move(from);
> +    from = nullptr;

Assigning a Move(uniqueptr) into a UniquePtr, nulls out uniqueptr.  This second line is unnecessary.

@@ +132,3 @@
>          surf = nullptr;
>      }
>      MOZ_ASSERT(!surf);

This assertion is vacuously correct now.

@@ +146,5 @@
> +
> +    UniquePtr<SharedSurface> ret = Move(surf);
> +    surf = nullptr;
> +
> +    return Move(ret);

These three lines are equivalent to

  return Move(surf);

@@ +164,5 @@
> +
> +    UniquePtr<SharedSurface> ret = Move(surf);
> +    surf = nullptr;
> +
> +    return Move(ret);

And these are

  return Move(surf);

@@ +179,3 @@
>          surf = nullptr;
>      }
>      MOZ_ASSERT(!surf);

!surf is similarly obvious here, if push() always succeeds.

@@ +190,1 @@
>          mScraps.pop();

UniquePtr<SharedSurface> cur = mScraps.pop();

@@ +217,1 @@
>          mScraps.pop();

UniquePtr<SharedSurface> cur = mScraps.pop();

::: gfx/layers/client/ClientCanvasLayer.cpp
@@ +98,5 @@
>              // Well, this *should* work...
>  #ifdef XP_MACOSX
>              factory = SurfaceFactory_IOSurface::Create(mGLContext, caps);
>  #else
> +            factory = MakeUnique<SurfaceFactory_GLTexture>(mGLContext, nullptr, caps);

As long as gcc < 4.6 is a thing, you can't pass plain nullptr here.  0 will work (...I think), or you can just cast to the explicit pointer type.
Of course this was all explained to me just the other day, but the places that don't use MakeUnique, probably don't because the relevant constructor is private/protected/not accessible to MakeUnique.  It might or might not be possible to friend that particular MakeUnique template arity, not sure, but at the least it's tricky.  :-\

And also pop() may not return the element from the queue/vector/whatever with stl, in which case the current code is more the right thing.  Although it's kind of a sad right thing, seems to me, that you can't extract all in one go but rather have to mutate the front/top element, then pop it in a second go.
Comment on attachment 8468890 [details] [diff] [review]
patch 3: SharedSurface, SurfaceFactory

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

If my questions are unfounded then an easy r+ but I'd like to see the reasons recorded.

::: gfx/gl/GLScreenBuffer.cpp
@@ +54,5 @@
>          factory = SurfaceFactory_IOSurface::Create(gl, caps);
>      }
>  #endif
>  
> +    if (!factory) {

Are the extra { }'s necessary?

@@ +371,4 @@
>  {
>      MOZ_ASSERT(mStream);
>  
>      if (newFactory) {

ditto

::: gfx/gl/GLScreenBuffer.h
@@ +174,5 @@
>          return mStream;
>      }
>  
>      SurfaceFactory* Factory() const {
> +        return mFactory.get();

Just an observation and rumination: This leaks the pointer from the unique ptr. So it's possible that some one could keep the raw pointer and pass it around and possibly delete it.

::: gfx/gl/SharedSurfaceANGLE.cpp
@@ +199,5 @@
>          return nullptr;
>      }
>  
> +    typedef SharedSurface_ANGLEShareHandle ptrT;
> +    return UniquePtr<ptrT>( new ptrT(gl, egl, size, hasAlpha, context,

Does that need a Move() like the others?

@@ +218,5 @@
> +        return nullptr;
> +    }
> +
> +    typedef SurfaceFactory_ANGLEShareHandle ptrT;
> +    return UniquePtr<ptrT>( new ptrT(gl, egl, caps) );

Move()?

::: gfx/gl/SurfaceStream.cpp
@@ +87,5 @@
>      if (surf) {
>          // Before next use, wait until SharedSurface's buffer
>          // is no longer being used.
>          surf->WaitForBufferOwnership();
> +        mSurfaces.insert(surf.get());

Is it dangerous to be extracting and storing a raw pointer?

@@ +146,5 @@
> +
> +    UniquePtr<SharedSurface> ret = Move(surf);
> +    surf = nullptr;
> +
> +    return Move(ret);

Can't this just be return Move(surf); instead of moving into ret?

I'm basing this suggestion upon this code fragment from Waldo's blog post:

    int* i = new int(37);
    UniquePtr<int> i1(i);
    UniquePtr<int> i2(Move(i1));
    assert(i1 == nullptr);
    assert(i2.get() == i);

@@ +159,5 @@
> +    UniquePtr<SharedSurface>& surf = *surfSlot;
> +
> +    if (surf) {
> +        mSurfaces.insert(surf.get());
> +    }

remove extra {}s

@@ +164,5 @@
> +
> +    UniquePtr<SharedSurface> ret = Move(surf);
> +    surf = nullptr;
> +
> +    return Move(ret);

Same comment above.

@@ +179,1 @@
>          surf = nullptr;

Isn't surf == nullptr because of Move(surf)?

@@ +402,3 @@
>  
>          if (mProducer && mStaging->mSize == mProducer->mSize)
> +            SharedSurface::ProdCopy(mStaging.get(), mProducer.get(), factory);

Perhaps in this case add extra {}s to show that the else is for the outer if. On first glance, I thought the else was incorrect indented.

@@ +498,1 @@
>          }

Does WaitForCompositor update mStaging?

Why not?
if (mStaging) {
    WaitForCompositor();
    Scrap(&mStaging);
}
Attachment #8468890 - Flags: review?(dglastonbury) → review-
(In reply to Dan Glastonbury :djg :kamidphish from comment #6)
> Comment on attachment 8468888 [details] [diff] [review]
> patch 1: GLScreenBuffer
> 
> Review of attachment 8468888 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLScreenBuffer.cpp
> @@ +606,5 @@
> >  
> >      if (surf->mAttachType == AttachmentType::Screen) {
> >          // Don't need anything. Our read buffer will be the 'screen'.
> >  
> > +        return UniquePtr<ReadBuffer>( new ReadBuffer(gl, 0, 0, 0,
> 
> Not a fan of spaces after ( but I understand the reason.
> Is it not possible to use MakeUnique? I suppose not, since contructor params.

All of the static Create() helpers that have protected/private ctors can't use MakeUnique, because MakeUnique isn't a friend of the class. (accessing protected/private function errors)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> Comment on attachment 8468888 [details] [diff] [review]
> patch 1: GLScreenBuffer
> 
> Review of attachment 8468888 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLScreenBuffer.cpp
> @@ +66,5 @@
> >  
> > +    UniquePtr<GLScreenBuffer> ret( new GLScreenBuffer(gl, caps,
> > +                                                      Move(factory),
> > +                                                      stream) );
> > +    return Move(ret);
> 
> Could just
> 
>   return MakeUnique<GLScreenBuffer>(gl, caps, Move(factor), stream);
> 
> to avoid a local, a move out of it, and a bunch of repetition.
GLScreenBuffer ctor is protected, so MakeUnique fails to access it. ([1])
> 
> @@ +568,5 @@
> >      gl->fGenFramebuffers(1, &fb);
> >      gl->AttachBuffersToFB(0, colorMSRB, depthRB, stencilRB, fb);
> >  
> > +    UniquePtr<DrawBuffer> ret( new DrawBuffer(gl, size, fb, colorMSRB,
> > +                                              depthRB, stencilRB) );
> 
> auto ret = MakeUnique<DrawBuffer>(gl, size, fb, colorMSRB, depthRB,
> stencilRB); might be nicer.
[1]
> 
> @@ +607,5 @@
> >      if (surf->mAttachType == AttachmentType::Screen) {
> >          // Don't need anything. Our read buffer will be the 'screen'.
> >  
> > +        return UniquePtr<ReadBuffer>( new ReadBuffer(gl, 0, 0, 0,
> > +                                                     surf) );
> 
> return MakeUnique<ReadBuffer>(gl, 0, 0, 0, surf);
[1]
> 
> @@ +642,5 @@
> >      gl->AttachBuffersToFB(colorTex, colorRB, depthRB, stencilRB, fb, target);
> >      gl->mFBOMapping[fb] = surf;
> >  
> > +    UniquePtr<ReadBuffer> ret( new ReadBuffer(gl, fb, depthRB,
> > +                                              stencilRB, surf) );
> 
> auto ret = MakeUnique<ReadBuffer>(gl, fb, depthRB, stencilRB, surf);
[1]
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9)
> And also pop() may not return the element from the queue/vector/whatever
> with stl, in which case the current code is more the right thing.  Although
> it's kind of a sad right thing, seems to me, that you can't extract all in
> one go but rather have to mutate the front/top element, then pop it in a
> second go.

STL pop() doesn't return the popped value, which is sucky.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)
> Comment on attachment 8468890 [details] [diff] [review]
> patch 3: SharedSurface, SurfaceFactory
> 
> Review of attachment 8468890 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLScreenBuffer.cpp
> @@ +36,5 @@
> >      {
> >          return nullptr;
> >      }
> >  
> > +    UniquePtr<SurfaceFactory> factory = nullptr;
> 
> Explicit initialization isn't necessary, it'll auto-init to nullptr if
> you're fine with that.
Right, change.
> 
> ::: gfx/gl/GLScreenBuffer.h
> @@ +126,5 @@
> >      GLContext* const mGL; // Owns us.
> >  public:
> >      const SurfaceCaps mCaps;
> >  protected:
> > +    UniquePtr<SurfaceFactory> mFactory; // Owned by us.
> 
> UniquePtr makes explicit what the comment says, seems to me.  Same for the
> others.  But up to you.
Agreed, changed.
> 
> ::: gfx/gl/SharedSurface.cpp
> @@ +259,1 @@
> >          mScraps.pop();
> 
> If this queue already contains UniquePtr, popping out of it will move out
> the UniquePtr, and then that temporary that's created by the pop() call will
> destroy the contained surface for you.  No need for a local at all here, if
> the queue contains UniquePtr.
std::queue's pop() doesn't work this way. It's `void std::queue::pop()`. We need to use `front()` to access the top of the queue.
> 
> @@ +269,1 @@
> >          mScraps.pop();
> 
> UniquePtr<SharedSurface> cur = mScraps.pop();
> 
> @@ +286,4 @@
> >  
> >      if (surf->mType == mType) {
> > +        mScraps.push(Move(surf));
> > +        MOZ_ASSERT(!surf);
> 
> You're not passing in a reference any more, so whatever the state of |surf|
> at this point, nobody can observe it, so who cares?  And to the extent null
> matters for double-deletion, that's inherent in UniquePtr, not really worth
> asserting, to me.
Right, changed.
> 
> ::: gfx/gl/SharedSurfaceANGLE.cpp
> @@ +200,5 @@
> >      }
> >  
> > +    typedef SharedSurface_ANGLEShareHandle ptrT;
> > +    return UniquePtr<ptrT>( new ptrT(gl, egl, size, hasAlpha, context,
> > +                                     pbuffer, shareHandle) );
> 
> return MakeUnique<ptrT>(gl, egl, size, hasAlpha, context, pbuffer,
> sharedHandle);
Class has a protected ctor.[1]
> 
> @@ +218,5 @@
> > +        return nullptr;
> > +    }
> > +
> > +    typedef SurfaceFactory_ANGLEShareHandle ptrT;
> > +    return UniquePtr<ptrT>( new ptrT(gl, egl, caps) );
> 
> return MakeUnique<ptrT>(gl, egl, caps);
[1]
> 
> ::: gfx/gl/SharedSurfaceEGL.cpp
> @@ +47,5 @@
> >      }
> >  
> > +    typedef SharedSurface_EGLImage ptrT;
> > +    return UniquePtr<ptrT>( new ptrT(prodGL, egl, size, hasAlpha,
> > +                                     formats, prodTex, image) );
> 
> MakeUnique
[1]
> 
> @@ +202,5 @@
> >          return nullptr;
> >      }
> >  
> > +    typedef SurfaceFactory_EGLImage ptrT;
> > +    return UniquePtr<ptrT>( new ptrT(prodGL, context, caps) );
> 
> MakeUnique
[1]
> 
> ::: gfx/gl/SharedSurfaceGL.cpp
> @@ +46,5 @@
> >          MOZ_CRASH("Unhandled Tex format.");
> >      }
> > +
> > +    typedef SharedSurface_Basic ptrT;
> > +    return UniquePtr<ptrT>( new ptrT(gl, size, hasAlpha, format, tex) );
> 
> MakeUnique
[1]
> 
> @@ +128,5 @@
> >      }
> >  
> > +    typedef SharedSurface_GLTexture ptrT;
> > +    return UniquePtr<ptrT>( new ptrT(prodGL, consGL, size, hasAlpha,
> > +                                     tex, ownsTex) );
> 
> MakeUnique
[1]
> 
> ::: gfx/gl/SharedSurfaceGralloc.cpp
> @@ +112,5 @@
> >  
> > +
> > +    typedef SharedSurface_Gralloc ptrT;
> > +    UniquePtr<ptrT> surf( new ptrT(prodGL, size, hasAlpha, egl,
> > +                                   allocator, grallocTC, prodTex) );
> 
> auto surf = MakeUnique<ptrT>(...);
[1]
> 
> ::: gfx/gl/SharedSurfaceIO.cpp
> @@ +23,5 @@
> >  
> >      gfx::IntSize size(surface->GetWidth(), surface->GetHeight());
> > +
> > +    typedef SharedSurface_IOSurface ptrT;
> > +    return UniquePtr<ptrT>( new ptrT(ioSurf, gl, size, hasAlpha) );
> 
> MakeUnique
[1]
> 
> @@ +148,5 @@
> >      gfx::IntSize maxDims(MacIOSurface::GetMaxWidth(),
> >                           MacIOSurface::GetMaxHeight());
> > +
> > +    typedef SurfaceFactory_IOSurface ptrT;
> > +    return UniquePtr<ptrT>( new ptrT(gl, caps, maxDims) );
> 
> MakeUnique
[1]
> 
> ::: gfx/gl/SurfaceStream.cpp
> @@ +87,5 @@
> >      if (surf) {
> >          // Before next use, wait until SharedSurface's buffer
> >          // is no longer being used.
> >          surf->WaitForBufferOwnership();
> > +        mSurfaces.insert(surf.get());
> 
> This...reads very dodgy.  Perhaps all the lifetimes work out all right, but
> this sort of shared-ownership of pointers thing is not really what UniquePtr
> was designed for at all.  I would hesitate to do this if it were me.
`mSurfaces` is only for DEBUG, but it's not behind ifdefs for some reason. I put in the ifdefs.
> 
> @@ +103,5 @@
> > +    UniquePtr<SharedSurface>& to = *slotTo;
> > +
> > +    MOZ_ASSERT(!to);
> > +    to = Move(from);
> > +    from = nullptr;
> 
> Assigning a Move(uniqueptr) into a UniquePtr, nulls out uniqueptr.  This
> second line is unnecessary.
This is true, but what's our upgrade strategy for std::unique_ptr and std::move? If we're going to upgrade at some point, it's important that I null this out, instead of leaving this in a 'destructable but undefined' state.
> 
> @@ +132,3 @@
> >          surf = nullptr;
> >      }
> >      MOZ_ASSERT(!surf);
> 
> This assertion is vacuously correct now.
This is about establishing postconditions for the function.
> 
> @@ +146,5 @@
> > +
> > +    UniquePtr<SharedSurface> ret = Move(surf);
> > +    surf = nullptr;
> > +
> > +    return Move(ret);
> 
> These three lines are equivalent to
> 
>   return Move(surf);
I want to make sure *surfSlot is nulled.
> 
> @@ +164,5 @@
> > +
> > +    UniquePtr<SharedSurface> ret = Move(surf);
> > +    surf = nullptr;
> > +
> > +    return Move(ret);
> 
> And these are
> 
>   return Move(surf);
I want to make sure *surfSlot is nulled.
> 
> @@ +179,3 @@
> >          surf = nullptr;
> >      }
> >      MOZ_ASSERT(!surf);
> 
> !surf is similarly obvious here, if push() always succeeds.
This is about establishing postconditions for the function.
> 
> @@ +190,1 @@
> >          mScraps.pop();
> 
> UniquePtr<SharedSurface> cur = mScraps.pop();
std::queue::pop returns void.
> 
> @@ +217,1 @@
> >          mScraps.pop();
> 
> UniquePtr<SharedSurface> cur = mScraps.pop();
std::queue::pop returns void.
> 
> ::: gfx/layers/client/ClientCanvasLayer.cpp
> @@ +98,5 @@
> >              // Well, this *should* work...
> >  #ifdef XP_MACOSX
> >              factory = SurfaceFactory_IOSurface::Create(mGLContext, caps);
> >  #else
> > +            factory = MakeUnique<SurfaceFactory_GLTexture>(mGLContext, nullptr, caps);
> 
> As long as gcc < 4.6 is a thing, you can't pass plain nullptr here.  0 will
> work (...I think), or you can just cast to the explicit pointer type.
Right, my bad. I'll do one of the things we talked about.
(In reply to Dan Glastonbury :djg :kamidphish from comment #10)
> Comment on attachment 8468890 [details] [diff] [review]
> patch 3: SharedSurface, SurfaceFactory
> 
> Review of attachment 8468890 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If my questions are unfounded then an easy r+ but I'd like to see the
> reasons recorded.
> 
> ::: gfx/gl/GLScreenBuffer.cpp
> @@ +54,5 @@
> >          factory = SurfaceFactory_IOSurface::Create(gl, caps);
> >      }
> >  #endif
> >  
> > +    if (!factory) {
> 
> Are the extra { }'s necessary?
Technically no, but I felt like I wanted them, for some reason. It's technically better style, but I don't care strongly.
> 
> @@ +371,4 @@
> >  {
> >      MOZ_ASSERT(mStream);
> >  
> >      if (newFactory) {
> 
> ditto
> 
> ::: gfx/gl/GLScreenBuffer.h
> @@ +174,5 @@
> >          return mStream;
> >      }
> >  
> >      SurfaceFactory* Factory() const {
> > +        return mFactory.get();
> 
> Just an observation and rumination: This leaks the pointer from the unique
> ptr. So it's possible that some one could keep the raw pointer and pass it
> around and possibly delete it.
Yep, we shouldn't do this.
If we make a BorrowedPointer<T> type for this, I'd return that. (ideally the BorrowedPointer<T> in DEBUG would be a WeakPtr that asserts if the WeakPtr is ever set to null while a BorrowedPointer<T> still exists)
> 
> ::: gfx/gl/SharedSurfaceANGLE.cpp
> @@ +199,5 @@
> >          return nullptr;
> >      }
> >  
> > +    typedef SharedSurface_ANGLEShareHandle ptrT;
> > +    return UniquePtr<ptrT>( new ptrT(gl, egl, size, hasAlpha, context,
> 
> Does that need a Move() like the others?
No, because we create the UniquePtr directly 'on' the 'return var'.[1]
> 
> @@ +218,5 @@
> > +        return nullptr;
> > +    }
> > +
> > +    typedef SurfaceFactory_ANGLEShareHandle ptrT;
> > +    return UniquePtr<ptrT>( new ptrT(gl, egl, caps) );
> 
> Move()?
[1]
> 
> ::: gfx/gl/SurfaceStream.cpp
> @@ +87,5 @@
> >      if (surf) {
> >          // Before next use, wait until SharedSurface's buffer
> >          // is no longer being used.
> >          surf->WaitForBufferOwnership();
> > +        mSurfaces.insert(surf.get());
> 
> Is it dangerous to be extracting and storing a raw pointer?
I mentioned this in my response to :waldo, but this code is for DEBUG-only, but wasn't #ifdef'd off. I added the ifdefs in my updated patch.
> 
> @@ +146,5 @@
> > +
> > +    UniquePtr<SharedSurface> ret = Move(surf);
> > +    surf = nullptr;
> > +
> > +    return Move(ret);
> 
> Can't this just be return Move(surf); instead of moving into ret?
> 
> I'm basing this suggestion upon this code fragment from Waldo's blog post:
> 
>     int* i = new int(37);
>     UniquePtr<int> i1(i);
>     UniquePtr<int> i2(Move(i1));
>     assert(i1 == nullptr);
>     assert(i2.get() == i);

I want to make sure that `surf` is nulled out. (specifically that *surfSlot is nulled out)
This is because we access the value of `surf` later, and `move` means that `surf` will be left in a "destructable but undefined state". It would only be safe to use assignment on `surf` after moving it. Requesting its value after `move` is undefined.

I believe if we switch to std::unique_ptr and std::move later, we won't be guaranteed that it is nulled out and deleted. (even if it is now?) Example:

> UniquePtr<Qux> foo = MakeCurrent<Qux>();
> UniquePtr<Qux> bar;
> 
> void MoveTo(UniquePtr<Qux>* from, UniquePtr<Qux>* to) {
>   MOZ_ASSERT(!*to);
>   *to = Move(*from);
> }
> 
> MoveTo(foo, bar);
> MoveTo(foo, bar); // This can assert with loose move guarantees.

Remember that the scope of the UniquePtrs involved here is external to the local function scope. (scope of the UniquePtrs here in approximately the scope of SurfaceStream's lifetime)

>
> 
> @@ +159,5 @@
> > +    UniquePtr<SharedSurface>& surf = *surfSlot;
> > +
> > +    if (surf) {
> > +        mSurfaces.insert(surf.get());
> > +    }
> 
> remove extra {}s
I think I'd like to keep these. I mostly like to omit the {}s when we're doing an early-return case, or both the condition and the expression are both simple. (really vague here, sorry)
> 
> @@ +164,5 @@
> > +
> > +    UniquePtr<SharedSurface> ret = Move(surf);
> > +    surf = nullptr;
> > +
> > +    return Move(ret);
> 
> Same comment above.
> 
> @@ +179,1 @@
> >          surf = nullptr;
> 
> Isn't surf == nullptr because of Move(surf)?
'No', see above.
> 
> @@ +402,3 @@
> >  
> >          if (mProducer && mStaging->mSize == mProducer->mSize)
> > +            SharedSurface::ProdCopy(mStaging.get(), mProducer.get(), factory);
> 
> Perhaps in this case add extra {}s to show that the else is for the outer
> if. On first glance, I thought the else was incorrect indented.
I agree, something this complicated should have {}.
> 
> @@ +498,1 @@
> >          }
> 
> Does WaitForCompositor update mStaging?
Not itself, it just waits.
> 
> Why not?
> if (mStaging) {
>     WaitForCompositor();
>     Scrap(&mStaging);
> }
The idea here is that the consuming thread might move mStaging to mConsumer, so we want to wait for that to happen. I'm not sure why we don't WaitForCompositor unconditionally, though. I do not want to change this code in this patch.
Attachment #8468890 - Flags: review- → review?(dglastonbury)
Updated from previous review.
Attachment #8468890 - Attachment is obsolete: true
Attachment #8468890 - Flags: review?(dglastonbury)
Attachment #8468963 - Flags: review?(dglastonbury)
Sorry for the churn. I forgot to fix the pass-nullptr-to-makeunique issue.
Attachment #8468963 - Attachment is obsolete: true
Attachment #8468963 - Flags: review?(dglastonbury)
Attachment #8468983 - Flags: review?(dglastonbury)
(In reply to Jeff Gilbert [:jgilbert] from comment #14)
> Class has a protected ctor.[1]

Does that win us anything? It's a rub, I know. Remove protected ctor to make MakeUnique accessible vs. the slight chance that someone might construct these objects. (We have code reviews to aid catching that kind of thing)

> This is true, but what's our upgrade strategy for std::unique_ptr and
> std::move? If we're going to upgrade at some point, it's important that I
> null this out, instead of leaving this in a 'destructable but undefined'
> state.

jwalden should answer this, but why would be upgrade? Mozilla has a tradition of having the equivalent of stl (MFBT and ns). If our UniquePtr adds functionality that is useful, such as nulling the old pointer, why would we "upgrade" to something that is inferior?
(In reply to Jeff Gilbert [:jgilbert] from comment #17)
> Created attachment 8468983 [details] [diff] [review]
> patch 3: SharedSurface, SurfaceFactory
> 
> Sorry for the churn. I forgot to fix the pass-nullptr-to-makeunique issue.

I dream of a better code review system where I could just review the diffs to the patch... :-)
Attachment #8468983 - Flags: review?(dglastonbury) → review+
(In reply to Jeff Gilbert [:jgilbert] from comment #14)
> > @@ +103,5 @@
> > > +    UniquePtr<SharedSurface>& to = *slotTo;
> > > +
> > > +    MOZ_ASSERT(!to);
> > > +    to = Move(from);
> > > +    from = nullptr;
> > 
> > Assigning a Move(uniqueptr) into a UniquePtr, nulls out uniqueptr.  This
> > second line is unnecessary.
> This is true, but what's our upgrade strategy for std::unique_ptr and
> std::move? If we're going to upgrade at some point, it's important that I
> null this out, instead of leaving this in a 'destructable but undefined'
> state.

First: it's not "undefined", it's "unspecified".  The former is super-bad stuff like eating all your cheese or overwriting your hard drive with pi to the trillionth digit.  Unspecified isn't that at all.  I should change the comment to not use a wildly misleading term.  :-)

Second: the language for that is C++11 super-general language, but specific instances may give more detail if they want.  For C++11, the move assignment operator is specified to behave as if by reset(other.release()), explicitly nulling out the input.  And for move construction, the move constructor and the unique_ptr<U, E>&& constructor have the effect of "transferring ownership" from other to *this.  That phrase is defined to leave the incoming pointer nulled out, with its pointer instead residing in *this.  (And also some effects on the deleter, but unless the deleter holds state -- an infrequent occurrence -- those don't matter.)

So it is very much defined that assigning Move(uniqueptr), or std::move(u_p), into another will null out that input.

> > @@ +146,5 @@
> > > +
> > > +    UniquePtr<SharedSurface> ret = Move(surf);
> > > +    surf = nullptr;
> > > +
> > > +    return Move(ret);
> > 
> > These three lines are equivalent to
> > 
> >   return Move(surf);
> I want to make sure *surfSlot is nulled.

Given |surf| is defined as |*surfSlot|, and the above, you're still guaranteed it's nulled out.

(In reply to Jeff Gilbert [:jgilbert] from comment #15)
> If we make a BorrowedPointer<T> type for this, I'd return that. (ideally the
> BorrowedPointer<T> in DEBUG would be a WeakPtr that asserts if the WeakPtr
> is ever set to null while a BorrowedPointer<T> still exists)

There's at least one mild proposal for an exempt_ptr<T>, that wouldn't give the assert here, but would at least be a smart pointer to "decorate" the pointer and indicate something's funny about it.

(In reply to Dan Glastonbury :djg :kamidphish from comment #18)
> jwalden should answer this, but why would be upgrade? Mozilla has a
> tradition of having the equivalent of stl (MFBT and ns).

The tradition exists because STL has been inadequate for one reason or another at the time, or we couldn't use it because of compiler requirements.  It's not a great tradition to emulate.  There's nothing special about unique_ptr that would prevent us using it, except for compiler requirements.  If/when those update, we'll get rid of it.

> If our UniquePtr adds functionality that is useful, such as nulling
> the old pointer, why would we "upgrade" to something that is
> inferior?

No such addition has occurred.  To the best of my knowledge, we've added no functionality to UniquePtr or MakeUnique that isn't already present in unique_ptr, or that isn't permitted as "undefined behavior" (e.g. dereferencing a null UniquePtr).  A few omissions (some of laziness, some of necessity), but no additions.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #20)
> For C++11, the move assignment
> operator is specified to behave as if by reset(other.release()), explicitly
> nulling out the input.  And for move construction, the move constructor and
> the unique_ptr<U, E>&& constructor have the effect of "transferring
> ownership" from other to *this.

The operator/constructor specifically of unique_ptr, that is.
(In reply to Jeff Gilbert [:jgilbert] from comment #23)
> https://tbpl.mozilla.org/?tree=Try&rev=4fedbc53a342

Oops, wrong bug.
Attachment #8473433 - Flags: review?(dglastonbury)
Comment on attachment 8473432 [details] [diff] [review]
patch 5: Custom wrapper for std::queue<UniquePtr<T>>

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

::: gfx/gl/SharedSurface.cpp
@@ +268,5 @@
>  
>  SurfaceFactory::~SurfaceFactory()
>  {
> +    while (!mScraps.Empty()) {
> +        UniquePtr<SharedSurface> cur = mScraps.Pop();

The local isn't necessary here, with the return type being a UniquePtr.

::: gfx/gl/SurfaceStream.cpp
@@ +189,1 @@
>          surf = nullptr;

Null-assignment still not necessary as explained before.  :-)
Comment on attachment 8473432 [details] [diff] [review]
patch 5: Custom wrapper for std::queue<UniquePtr<T>>

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

::: gfx/gl/SharedSurface.h
@@ +122,5 @@
> +public:
> +    ~UniquePtrQueue() {
> +        MOZ_ASSERT(Empty());
> +    }
> +    

WS

@@ +142,5 @@
> +        mQueue.pop();
> +
> +        ret.reset(p);
> +        return Move(ret);
> +    }

UniquePtr<T> Pop() {
    UniquePtr<T> ret;
    if (!mQueue.empty()) {
        ret.reset(mQueue.front());
        mQueue.pop();
    }

    return Move(ret);
}

::: gfx/gl/SurfaceStream.cpp
@@ +189,1 @@
>          surf = nullptr;

I'll agree with :Waldo and suggest code not written is better.
Attachment #8473432 - Flags: review?(dglastonbury) → review+
Comment on attachment 8473433 [details] [diff] [review]
patch 4: Fix compilation errors

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

::: gfx/gl/GLScreenBuffer.cpp
@@ -651,5 @@
>  
>      UniquePtr<ReadBuffer> ret( new ReadBuffer(gl, fb, depthRB,
>                                                stencilRB, surf) );
> -    if (!gl->IsFramebufferComplete(fb))
> -        return nullptr;

I prefer this version with out the extra {}s
Attachment #8473433 - Flags: review?(dglastonbury) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #30)
> Comment on attachment 8473432 [details] [diff] [review]
> patch 5: Custom wrapper for std::queue<UniquePtr<T>>
> 
> Review of attachment 8473432 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/SharedSurface.cpp
> @@ +268,5 @@
> >  
> >  SurfaceFactory::~SurfaceFactory()
> >  {
> > +    while (!mScraps.Empty()) {
> > +        UniquePtr<SharedSurface> cur = mScraps.Pop();
> 
> The local isn't necessary here, with the return type being a UniquePtr.
Right, ok.

> 
> ::: gfx/gl/SurfaceStream.cpp
> @@ +189,1 @@
> >          surf = nullptr;
> 
> Null-assignment still not necessary as explained before.  :-)
Right. I'll assume it from now on. For readability, I'm going to add asserts as a form of indicating precondition.
(In reply to Dan Glastonbury :djg :kamidphish from comment #31)
> Comment on attachment 8473432 [details] [diff] [review]
> patch 5: Custom wrapper for std::queue<UniquePtr<T>>
> 
> Review of attachment 8473432 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/SharedSurface.h
> @@ +122,5 @@
> > +public:
> > +    ~UniquePtrQueue() {
> > +        MOZ_ASSERT(Empty());
> > +    }
> > +    
> 
> WS
Fixed.
> 
> @@ +142,5 @@
> > +        mQueue.pop();
> > +
> > +        ret.reset(p);
> > +        return Move(ret);
> > +    }
> 
> UniquePtr<T> Pop() {
>     UniquePtr<T> ret;
>     if (!mQueue.empty()) {
>         ret.reset(mQueue.front());
>         mQueue.pop();
>     }
> 
>     return Move(ret);
> }
Sure.
> 
> ::: gfx/gl/SurfaceStream.cpp
> @@ +189,1 @@
> >          surf = nullptr;
> 
> I'll agree with :Waldo and suggest code not written is better.

Alright, removed, and in some places replaced with asserts.
(In reply to Dan Glastonbury :djg :kamidphish from comment #32)
> Comment on attachment 8473433 [details] [diff] [review]
> patch 4: Fix compilation errors
> 
> Review of attachment 8473433 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLScreenBuffer.cpp
> @@ -651,5 @@
> >  
> >      UniquePtr<ReadBuffer> ret( new ReadBuffer(gl, fb, depthRB,
> >                                                stencilRB, surf) );
> > -    if (!gl->IsFramebufferComplete(fb))
> > -        return nullptr;
> 
> I prefer this version with out the extra {}s

I agree, but B2G on ICS's GCC 4.4 doesn't allow `return nullptr` to return into UniquePre<T>, so we would need to create a temporary UniquePtr<T> and move-return it.
Carrying forward r+ on this, since it's just cleanup for r+'d patches.
Attachment #8473911 - Flags: review+
Let's double check that this still builds:
https://tbpl.mozilla.org/?tree=Try&rev=b394a348c706
You need to log in before you can comment on or make changes to this bug.