875232 - Crash in mozilla::gl::SharedSurface_GLTexture::Fence @ libGPUSupport on OS X w/ OMTC

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

I see this on http://gooengine.com/demofiles/pearl-boy/index.html

Comment 1

[Jeff Muizelaar [:jrmuizel]]

https://crash-stats.mozilla.com/report/index/bp-c980401b-47b3-477c-9f6b-a5a5d2130523

Comment 2

[Matt Woodrow (:mattwoodrow)]

Fixed by bug 874823

Comment 3

[Scoobidiver (away)]

Jeff, are you able to reproduce it because I see these crashes from 24.0a1/20130622 along with bug 886670?

Comment 4

[Jeff Muizelaar [:jrmuizel]]

I am able to reproduce this still.

Comment 5

[Matt Woodrow (:mattwoodrow)]

I can't reproduce this.

Both this and bug 886670 have crash reports with the same device and vendor ID's (0x8086 and 0x166).

This is different to what I have (0x126), so it might be a device specific issue.

Comment 6

[Scoobidiver (away)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> Both this and bug 886670 have crash reports with the same device and vendor
> ID's (0x8086 and 0x166).
No. This one happens on NVIDIA GPUs.

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

I can reproduce this pretty consistently on http://www.clicktorelease.com/code/cross-hatching/

Comment 8

[Matt Woodrow (:mattwoodrow)]

Jeff, what do you think we should do here?

Add a way to blacklist certain drivers from using fences?

I don't think we want to stop these cards from using GL layers entirely.

Comment 9

[Kelsey Gilbert [:jgilbert]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> Jeff, what do you think we should do here?
> 
> Add a way to blacklist certain drivers from using fences?
> 
> I don't think we want to stop these cards from using GL layers entirely.

Yeah, we'll need to add this to the blacklistable feature set.

Also, have we tried these cards with ClientWaitSync instead of WaitSync? That's the minimal step back from where we are now. If it's fences themselves which are broken, we'll need to head back to glFinish land, for these cards.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: WIP to try using IOSurfaces instead

Has errors on the first frame, since it's not actually an IOSurface.

Also crashes pretty soon after that trying to bind an existing SharedSurface to the framebuffer (framebuffer incomplete assertion).

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Use IOSurfaces instead

Does this fix the crash?

Comment 12

[Jeff Muizelaar [:jrmuizel]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> Created attachment 770877 [details] [diff] [review]
> Use IOSurfaces instead
> 
> Does this fix the crash?

Seems to.

Here's some documentation about synchronizing with io surfaces:
http://lists.apple.com/archives/mac-opengl/2009/Sep/msg00111.html

Comment 13

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 770877 [details] [diff] [review]
Use IOSurfaces instead

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

Awesome, but let's reduce this and see if it still works. Apologies for any premature feedback.

::: gfx/gl/GLScreenBuffer.cpp
@@ +574,5 @@
>                                          nullptr, pDepthRB, pStencilRB);
>  
>      GLuint colorTex = 0;
>      GLuint colorRB = 0;
> +    GLenum target = LOCAL_GL_TEXTURE_2D;

[1] Init this to 0, so that if it sneaks through, we should get INVALID_ENUM from Bind* calls, etc.

@@ +625,5 @@
>      // Nothing else is needed for AttachType Screen.
>      if (surf->AttachType() != AttachmentType::Screen) {
>          GLuint colorTex = 0;
>          GLuint colorRB = 0;
> +        GLenum target = LOCAL_GL_TEXTURE_2D;

See [1].

::: gfx/gl/SharedSurfaceIO.h
@@ +23,5 @@
> +   
> +    virtual void LockProdImpl()
> +    {
> +      mGL->MakeCurrent();
> +      mGL->fActiveTexture(LOCAL_GL_TEXTURE0);

Why do we switch the current tex unit here?

@@ +29,5 @@
> +      void *nativeCtx = mGL->GetNativeData(GLContext::NativeGLContext);
> + 
> +      mSurface->CGLTexImageIOSurface2D(nativeCtx,
> +                                       LOCAL_GL_RGBA, LOCAL_GL_BGRA,
> +                                       LOCAL_GL_UNSIGNED_INT_8_8_8_8_REV, 0);

Calling `CGLTexImageIOSurface2D` again doesn't seem necessary. Doesn't the binding between the IOSurface and the texture persist until texture respecification?

@@ +31,5 @@
> +      mSurface->CGLTexImageIOSurface2D(nativeCtx,
> +                                       LOCAL_GL_RGBA, LOCAL_GL_BGRA,
> +                                       LOCAL_GL_UNSIGNED_INT_8_8_8_8_REV, 0);
> + 
> +      mGL->fBindTexture(LOCAL_GL_TEXTURE_RECTANGLE_ARB, 0);

We need to save and restore this binding, not zero it.

@@ +38,5 @@
> +
> +    virtual void Fence()
> +    { 
> +      mGL->MakeCurrent();
> +      mGL->fFinish();

The link posted by jrmuizel says that we should be able to get away with a glFlush here, so let's try that. If we have to glFinish, we should just stick with the existing shared-texture ShSurf and disable Fences for affected hardware/drivers.

@@ +67,5 @@
> +    {
> +      mGL->MakeCurrent();
> +      mGL->fGenTextures(1, &mTexture);
> +
> +      mGL->fActiveTexture(LOCAL_GL_TEXTURE0);

Don't change the current tex unit if it's not necessary.

@@ +89,5 @@
> +      aSurface->CGLTexImageIOSurface2D(nativeCtx,
> +                                       LOCAL_GL_RGBA, LOCAL_GL_BGRA,
> +                                       LOCAL_GL_UNSIGNED_INT_8_8_8_8_REV, 0);
> + 
> +      mGL->fBindTexture(LOCAL_GL_TEXTURE_RECTANGLE_ARB, 0);

We're in content, so we need to save+restore all state we change.

::: gfx/layers/client/CanvasClient.cpp
@@ +143,5 @@
> +    SharedTextureHandle handle =
> +      GLContextProviderCGL::CreateSharedHandle(GLContext::SameProcess,
> +                                               ioSurf->GetIOSurface(),
> +                                               GLContext::IOSurface);
> +    mTextureClient->SetDescriptor(SharedTextureDescriptor(GLContext::SameProcess,

Why do we switch to ShTexDesc here?

Comment 14

[Matt Woodrow (:mattwoodrow)]

(In reply to Jeff Gilbert [:jgilbert] from comment #13)

> Awesome, but let's reduce this and see if it still works. Apologies for any
> premature feedback.

I don't think you ever need to apologise for this.

> ::: gfx/layers/client/CanvasClient.cpp
> @@ +143,5 @@
> > +    SharedTextureHandle handle =
> > +      GLContextProviderCGL::CreateSharedHandle(GLContext::SameProcess,
> > +                                               ioSurf->GetIOSurface(),
> > +                                               GLContext::IOSurface);
> > +    mTextureClient->SetDescriptor(SharedTextureDescriptor(GLContext::SameProcess,
> 
> Why do we switch to ShTexDesc here?

Because we already have CompositorOGL support for rendering IOSurfaces passed via a SharedTextureDescriptor and I didn't see any point in duplicating this. We're also going to need this once we attempt to do this cross-process.

Comment 15

[Matt Woodrow (:mattwoodrow)]

Attachment: Made alpha channel optional for MacIOSurface

Comment 16

[Matt Woodrow (:mattwoodrow)]

Attachment: Make BlitFramebufferToFramebuffer and friends support targets other than GL_TEXTURE_2D

Comment 17

[Matt Woodrow (:mattwoodrow)]

Attachment: Add SharedSurface_IOSurface for sharing textures on OSX

Comment 18

[Matt Woodrow (:mattwoodrow)]

Attachment: Fix ownership model with SharedTextureHostOGL

Hopefully this doesn't conflict with your texture changes *too* much.

Currently we assume that we're always getting the textures from plugins, which are single buffered, and we release the old handle when we get a new one.

WebGL is going to be using this same host with double-buffering, so we need to make sure we release things at the right time.

Comment 19

[Matt Woodrow (:mattwoodrow)]

We tested this on Jeff's laptop (with s/fFinish/fFlush), and it appeared to perform even better than Fence/WaitSync.

Comment 20

[Benoit Girard (:BenWa)]

Comment on attachment 772441 [details] [diff] [review]
Made alpha channel optional for MacIOSurface

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

::: gfx/2d/MacIOSurface.h
@@ +30,4 @@
>    static void ReleaseIOSurface(MacIOSurface *aIOSurface);
>    static mozilla::TemporaryRef<MacIOSurface> LookupSurface(IOSurfaceID aSurfaceID,
> +                                                           double aContentsScaleFactor = 1.0,
> +                                                           bool aHasAlpha = true);

Optional: prefer enum over bool

Comment 21

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 772443 [details] [diff] [review]
Make BlitFramebufferToFramebuffer and friends support targets other than GL_TEXTURE_2D

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

::: gfx/gl/GLContext.cpp
@@ +1071,5 @@
>  GLContext::CreateTexture(GLenum internalFormat, GLenum format, GLenum type, const gfxIntSize& size)
>  {
>      GLuint tex = 0;
>      fGenTextures(1, &tex);
> +    ScopedBindTexture autoTex(this, tex, LOCAL_GL_TEXTURE_2D);

[1] This shouldn't need LOCAL_GL_TEXTURE_2D.

::: gfx/gl/GLContext.h
@@ +3134,3 @@
>  
>  protected:
> +    GLenum mOldTexture;

s/Texture/TexUnit/

@@ +3137,3 @@
>  
>  public:
> +    ScopedBindTextureUnit(GLContext* gl, GLenum texture)

The spec uses `texture` here, but let's use `texUnit` instead.
I would actually rather use 0...N instead of GL_TEXTURE0...GL_TEXTUREN.
Which ever way we go, we must assert such that the *wrong* way will fail:
For staying with TEXTURE0+, this means asserting that `texUnit >= GL_TEXTURE0`.
Otherwise, `texUnit < GL_TEXTURE0`.

@@ +3173,5 @@
> +        mGL->GetUIntegerv(bindingTarget, &mOldTex);
> +    }
> +
> +public:
> +    explicit ScopedBindTexture(GLContext* gl, GLenum target = LOCAL_GL_TEXTURE_2D)

I don't think we can use the `explicit` keyword here, since this is no longer a copy-constructor. I could be wrong here.

@@ +3179,5 @@
> +    {
> +        Init(target);
> +    }
> +
> +    ScopedBindTexture(GLContext* gl, GLuint newTex, GLenum target = LOCAL_GL_TEXTURE_2D)

I'm not sure if this overload with optional 3rd param will work properly in tandem with the previous ctor. How do we pick which ctor for `ctor(gl, LOCAL_GL_TEXTURE_2D)`? It could be either `ctor(gl, target=GL_TEXTURE_2D` or `ctor(gl, newTex=GL_TEXTURE_2D[, target=GL_TEXTURE_2D])`. It's possible to get a texture ID that collides with GL_TEXTURE_2D, so we can't even infer the path from the values.

::: gfx/gl/GLContextUtils.cpp
@@ +77,5 @@
> +      fragShader = &mTex2DBlit_FragShader;
> +      fragShaderSource = kTex2DBlit_FragShaderSource;
> +    } else {
> +      program = &mTex2DRectBlit_Program;
> +      fragShader = &mTex2DRectBlit_FragShader;

After grabbing the pointers in these branches, we should switch from pointers to references in the code below.

@@ +115,3 @@
>  
> +            fEnableVertexAttribArray(0);
> +            fVertexAttribPointer(0,

I'm pretty sure we don't need these up here. These look like dupes of where we do this same thing down below.

@@ +210,5 @@
>          fUniform1i(texUnitLoc, 0);
>  
> +        if (target == LOCAL_GL_TEXTURE_RECTANGLE_ARB) {
> +          GLuint texCoordMultLoc = fGetUniformLocation(*program, "uTexCoordMult");
> +          fUniform2f(texCoordMultLoc, srcSize.width, srcSize.height);

Can't do this yet. We should really pull all this init code out into another function which has no knowledge of sizes, etc.

We have to set this uniform for the program for each blit. (Of a different size)

@@ +336,5 @@
>      }
>  
>  
>      ScopedBindFramebuffer boundFB(this, destFB);
> +    ScopedBindTextureUnit boundTU(this, LOCAL_GL_TEXTURE0);

Create a new scope for each ScopeBindTextureUnit, since we have to rebind the old texture before rebinding to the old tex unit.

::: gfx/gl/GLTextureImage.cpp
@@ +598,5 @@
>  
>      GLuint texture = 0;
>      aGL->fGenTextures(1, &texture);
>  
> +    ScopedBindTexture bind(aGL, texture, LOCAL_GL_TEXTURE_2D);

See [1].

::: gfx/gl/SharedSurfaceEGL.cpp
@@ +229,5 @@
>      }
>  
>      if (!mConsTex) {
>          consGL->fGenTextures(1, &mConsTex);
> +        ScopedBindTexture autoTex(consGL, mConsTex, LOCAL_GL_TEXTURE_2D);

See [1].

Comment 22

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 772444 [details] [diff] [review]
Add SharedSurface_IOSurface for sharing textures on OSX

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

::: gfx/gl/SharedSurfaceIO.cpp
@@ +76,5 @@
> +  mGL->fTexParameteri(LOCAL_GL_TEXTURE_RECTANGLE_ARB,
> +                      LOCAL_GL_TEXTURE_WRAP_T,
> +                      LOCAL_GL_CLAMP_TO_EDGE);
> +
> +  void *nativeCtx = mGL->GetNativeData(GLContext::NativeGLContext);

Assert non-null?

@@ +78,5 @@
> +                      LOCAL_GL_CLAMP_TO_EDGE);
> +
> +  void *nativeCtx = mGL->GetNativeData(GLContext::NativeGLContext);
> +
> +  aSurface->CGLTexImageIOSurface2D(nativeCtx);

Does `CGLTexImageIOSurface2D` not have a failure case? Even a relatively unlikely one?

::: gfx/gl/SharedSurfaceIO.h
@@ +6,5 @@
> +#ifndef SHARED_SURFACEIO_H_
> +#define SHARED_SURFACEIO_H_
> +
> +#include "SharedSurfaceGL.h"
> +#include "mozilla/gfx/MacIOSurface.h"

Move this include to the CPP file, since we should be able to forward declare this now.

::: gfx/layers/client/CanvasClient.cpp
@@ +129,5 @@
> +#ifdef XP_MACOSX
> +    // swap staging -> consumer so we can send it to the compositor
> +    SharedSurface* surf = stream->SwapConsumer();
> +    if (!surf) {
> +      printf_stderr("surf is null post-SwapConsumer!\n");

This should be expected behavior for new triple-buffered streams that haven't `SwapProducer`d yeah, so I don't think we want this spew.

@@ +134,5 @@
> +      return;
> +    }
> +
> +    if (surf->Type() != SharedSurfaceType::IOSurface) {
> +      printf_stderr("Unexpected non-IOSurface SharedSurface!");

This is not true, since we should still have fallback options. Our order for fallback surface types should be:
IOSurfaces
Shared GLTexture w/Fences
Shared GLTexture w/Finish
Basic readback surface

We at least need to preserve support for readback surfaces, since this is our nuclear fallback option in case we have to blocklist drivers. (We also test this with `webgl.force-readback`) If IOSurfs are busted, I probably don't trust shared GLTextures even with Finish, so we probably don't have to maintain that on Mac.

::: gfx/layers/opengl/CanvasLayerOGL.cpp
@@ +222,5 @@
>          }
> +#ifdef XP_MACOSX
> +        case SharedSurfaceType::IOSurface: {
> +          ioSurf = SharedSurface_IOSurface::Cast(surf);
> +          updatedSurface = ioSurf->LockAsSurface();

Why would we ever want to lock this as a CPU surface instead of attaching it to a GLTexture and texturing from it directly?

Comment 23

[Matt Woodrow (:mattwoodrow)]

(In reply to Jeff Gilbert [:jgilbert] from comment #22)
> 
> @@ +78,5 @@
> > +                      LOCAL_GL_CLAMP_TO_EDGE);
> > +
> > +  void *nativeCtx = mGL->GetNativeData(GLContext::NativeGLContext);
> > +
> > +  aSurface->CGLTexImageIOSurface2D(nativeCtx);
> 
> Does `CGLTexImageIOSurface2D` not have a failure case? Even a relatively
> unlikely one?

It does not appear to. If it does, MacIOSurface doesn't appear to catch it and return anything.

> 
> @@ +134,5 @@
> > +      return;
> > +    }
> > +
> > +    if (surf->Type() != SharedSurfaceType::IOSurface) {
> > +      printf_stderr("Unexpected non-IOSurface SharedSurface!");
> 
> This is not true, since we should still have fallback options. Our order for
> fallback surface types should be:
> IOSurfaces
> Shared GLTexture w/Fences
> Shared GLTexture w/Finish
> Basic readback surface
> 
> We at least need to preserve support for readback surfaces, since this is
> our nuclear fallback option in case we have to blocklist drivers. (We also
> test this with `webgl.force-readback`) If IOSurfs are busted, I probably
> don't trust shared GLTextures even with Finish, so we probably don't have to
> maintain that on Mac.

Alright, I'll fix this.

> 
> ::: gfx/layers/opengl/CanvasLayerOGL.cpp
> @@ +222,5 @@
> >          }
> > +#ifdef XP_MACOSX
> > +        case SharedSurfaceType::IOSurface: {
> > +          ioSurf = SharedSurface_IOSurface::Cast(surf);
> > +          updatedSurface = ioSurf->LockAsSurface();
> 
> Why would we ever want to lock this as a CPU surface instead of attaching it
> to a GLTexture and texturing from it directly?

With in-process OpenGL layers, we're going to Morph to a normal GLTexture factory after the first frame.

Given that we're in the process of phasing in-process GL layers out, I didn't see the point in making this file support TEXTURE_RECTANGLE_ARB properly. On second look it appears to be quite easy, so I guess I'll just do that.

Comment 24

[Matt Woodrow (:mattwoodrow)]

(In reply to Jeff Gilbert [:jgilbert] from comment #21)

> @@ +3179,5 @@
> > +    {
> > +        Init(target);
> > +    }
> > +
> > +    ScopedBindTexture(GLContext* gl, GLuint newTex, GLenum target = LOCAL_GL_TEXTURE_2D)
> 
> I'm not sure if this overload with optional 3rd param will work properly in
> tandem with the previous ctor. How do we pick which ctor for `ctor(gl,
> LOCAL_GL_TEXTURE_2D)`? It could be either `ctor(gl, target=GL_TEXTURE_2D` or
> `ctor(gl, newTex=GL_TEXTURE_2D[, target=GL_TEXTURE_2D])`. It's possible to
> get a texture ID that collides with GL_TEXTURE_2D, so we can't even infer
> the path from the values.

Yep, you are correct. That's why I had to add all the explicit LOCAL_GL_TEXTURE_2D arguments. I doesn't look like this constructor is actually used, so I'll get rid of it along with the other cruft.

> 
> @@ +336,5 @@
> >      }
> >  
> >  
> >      ScopedBindFramebuffer boundFB(this, destFB);
> > +    ScopedBindTextureUnit boundTU(this, LOCAL_GL_TEXTURE0);
> 
> Create a new scope for each ScopeBindTextureUnit, since we have to rebind
> the old texture before rebinding to the old tex unit.

This should not be necessary. The c++ spec defines that these objects will be destructed in the reverse order than which they were created. So we'll always revert the texture binding before reverting the texture unit.

Comment 25

[Matt Woodrow (:mattwoodrow)]

(In reply to Jeff Gilbert [:jgilbert] from comment #22)

> @@ +134,5 @@
> > +      return;
> > +    }
> > +
> > +    if (surf->Type() != SharedSurfaceType::IOSurface) {
> > +      printf_stderr("Unexpected non-IOSurface SharedSurface!");
> 
> This is not true, since we should still have fallback options. Our order for
> fallback surface types should be:
> IOSurfaces
> Shared GLTexture w/Fences
> Shared GLTexture w/Finish
> Basic readback surface
> 
> We at least need to preserve support for readback surfaces, since this is
> our nuclear fallback option in case we have to blocklist drivers. (We also
> test this with `webgl.force-readback`) If IOSurfs are busted, I probably
> don't trust shared GLTextures even with Finish, so we probably don't have to
> maintain that on Mac.

Actually, how should this work?

We've already called SwapConsumer to determine the surface type. I don't think we can un-swap the consumer and just pass the SurfaceStreamHandle instead.

Comment 26

[Kelsey Gilbert [:jgilbert]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #25)
> (In reply to Jeff Gilbert [:jgilbert] from comment #22)
> 
> > @@ +134,5 @@
> > > +      return;
> > > +    }
> > > +
> > > +    if (surf->Type() != SharedSurfaceType::IOSurface) {
> > > +      printf_stderr("Unexpected non-IOSurface SharedSurface!");
> > 
> > This is not true, since we should still have fallback options. Our order for
> > fallback surface types should be:
> > IOSurfaces
> > Shared GLTexture w/Fences
> > Shared GLTexture w/Finish
> > Basic readback surface
> > 
> > We at least need to preserve support for readback surfaces, since this is
> > our nuclear fallback option in case we have to blocklist drivers. (We also
> > test this with `webgl.force-readback`) If IOSurfs are busted, I probably
> > don't trust shared GLTextures even with Finish, so we probably don't have to
> > maintain that on Mac.
> 
> Actually, how should this work?
> 
> We've already called SwapConsumer to determine the surface type. I don't
> think we can un-swap the consumer and just pass the SurfaceStreamHandle
> instead.

Oh, I just realized where this call is. We don't want to call SwapConsumer on the content thread. (where `Update` is called from, correct?) Just implement the attachment of the IOSurface in the compositor, similar to the EGLImage path.

Comment 27

[Matt Woodrow (:mattwoodrow)]

Attachment: Make BlitFramebufferToFramebuffer and friends support targets other than GL_TEXTURE_2D v2

Comment 28

[Matt Woodrow (:mattwoodrow)]

Attachment: Add SharedSurface_IOSurface for sharing textures on OSX v2

Comment 29

[Matt Woodrow (:mattwoodrow)]

Attachment: Workaround broken glReadPixels with IOSurfaces

IOSurfaces have a bug where attempting to use glReadPixels on framebuffer that is backed by one will return corrupt results. It also corrupts future calls to glReadPixels on the same context, even for sources that aren't IOSurfaces.

This implements the same workaround that WebKit uses, where we allocate a temporary texture, copy to that, and then readback from there.

For this to work, we need to be able to determine if the current read framebuffer is backed by an IOSurface.

I've added a map for looking this up, and whenever we create a ReadBuffer we add the mapping between fb id and the SharedSurface_GL that backs it.

Comment 30

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 775908 [details] [diff] [review]
Make BlitFramebufferToFramebuffer and friends support targets other than GL_TEXTURE_2D v2

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

Leaving r? pending answer to the question about texture unit switching. I think we should absolutely have the helper RAII class for this, but I don't think it's needed in this case, and I don't want to risk cargo-culting this.

::: gfx/gl/GLContext.cpp
@@ +1181,5 @@
>      if (colorTex) {
>          MOZ_ASSERT(fIsTexture(colorTex));
>          fFramebufferTexture2D(LOCAL_GL_FRAMEBUFFER,
>                                LOCAL_GL_COLOR_ATTACHMENT0,
> +                              target,

It's probably best to assert that `target` is something we should expect.

::: gfx/gl/GLContextUtils.cpp
@@ +55,5 @@
> +varying vec2 vTexCoord;                             \n\
> +                                                    \n\
> +void main(void) {                                   \n\
> +    gl_FragColor =                                  \n\
> +      texture2DRect(uTexUnit,                       \n\

At least push this indenting out to 4-space. It's probably best to move the '\n\' part further out so we can at least get:
foo = bar(baz,
          bat)

@@ +70,3 @@
>      bool success = false;
>  
> +    GLuint *programPtr;

I actually prefer prefer clarifying this with hungarian warts, but this works too.

@@ +234,5 @@
> +        return false;
> +    }
> +
> +    if (target == LOCAL_GL_TEXTURE_RECTANGLE_ARB) {
> +        GLuint texCoordMultLoc = fGetUniformLocation(mTex2DRectBlit_Program, "uTexCoordMult");

Assert that this didn't give us -1. ('uniform not found')

@@ +341,5 @@
>      }
>  
>  
>      ScopedBindFramebuffer boundFB(this, destFB);
> +    ScopedBindTextureUnit boundTU(this, LOCAL_GL_TEXTURE0);

Why do we bother to switch texture units here? Don't switch unless we need to, in which case we should have a comment as to why we need to. We don't want to cargo-cult these things.

::: gfx/gl/SharedSurface.h
@@ +108,5 @@
>          MOZ_CRASH("Did you forget to override this function?");
>      }
>  
> +    virtual GLenum TextureTarget() const {
> +        return LOCAL_GL_TEXTURE_2D;

Let's return `Texture() ? TEXTURE_2D : 0` here for extra break-early safety here.

Comment 31

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 775916 [details] [diff] [review]
Workaround broken glReadPixels with IOSurfaces

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

This is correct, but I think it would be better done in GLContextCGL. However, I'd be grudgingly willing to take this now, and do the better fix in a week or so.

::: gfx/gl/GLContext.h
@@ +721,5 @@
>          BeforeGLReadCall();
> +
> +        if (!mScreen ||
> +            !mScreen->ReadPixels(x, y, width, height, format, type, pixels)) {
> +            raw_fReadPixels(x, y, width, height, format, type, pixels);

These three lines in particular look pretty bad. Maybe pull them apart?
bool didReadPixels = false
if mScreen:
  didReadPixels = mScreen->ReadPixels()

if !didReadPixels:
  raw_fReadPixels()

::: gfx/gl/GLScreenBuffer.cpp
@@ +297,5 @@
> +    SharedSurface_GL* surf;
> +    if (GetReadFB() == 0) {
> +        surf = SharedSurf();
> +    } else {
> +        surf = mGL->mFBOMapping[GetReadFB()];

If we're doing this anyways, why don't we just track it in GLContextCGL? It should take about the same amount of work, but be less intrusive, and shouldn't have any problematic overhead.

Also, in what case is this hit? Snapshotting is what I suspect, but leave a comment.

::: gfx/gl/GLScreenBuffer.h
@@ +243,5 @@
>      void AssureBlitted();
>      void AfterDrawCall();
>      void BeforeReadCall();
>  
> +    bool ReadPixels(GLint x, GLint y, GLsizei width, GLsizei height,

Note that this returns `false` not on failure, but rather when nothing special is required, and the caller should just call glReadPixels.

Comment 32

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 775909 [details] [diff] [review]
Add SharedSurface_IOSurface for sharing textures on OSX v2

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

It would be better to #ifdef in ShSurfIO.cpp, and not ifdef in calling code, if we can get away with it, IMO. (We can do this here, but it might fall to me to do it if I'm the only one who wants it)

Leaving as r? pending answer to MakeCurrent in dtor question.

::: gfx/gl/SharedSurfaceIO.cpp
@@ +13,5 @@
> +
> +using namespace gfx;
> +
> +/* static */ SharedSurface_IOSurface*
> +SharedSurface_IOSurface::Create(MacIOSurface* aSurface, GLContext *aGL, bool aHasAlpha)

Can we move away from using an 'a' prefix for function params?

@@ +15,5 @@
> +
> +/* static */ SharedSurface_IOSurface*
> +SharedSurface_IOSurface::Create(MacIOSurface* aSurface, GLContext *aGL, bool aHasAlpha)
> +{
> +  gfxIntSize size(aSurface->GetWidth(), aSurface->GetHeight());

ShSurfIO.h is 4-space, as is most (all?) of gfx/gl, and it looks like the header on this file claims it's 4-space as well.

@@ +60,5 @@
> +
> +SharedSurface_IOSurface::~SharedSurface_IOSurface()
> +{
> +  if (mTexture) {
> +    mGL->MakeCurrent();

We should assert this is already true.
If it's not necessarily true, things get trickier.

Comment 33

[Matt Woodrow (:mattwoodrow)]

Attachment: Make BlitFramebufferToFramebuffer and friends support targets other than GL_TEXTURE_2D v3

Comment 34

[Matt Woodrow (:mattwoodrow)]

Attachment: Workaround broken glReadPixels with IOSurfaces v2

Carrying forward r=jgilbert

Comment 35

[Matt Woodrow (:mattwoodrow)]

Attachment: Add SharedSurface_IOSurface for sharing textures on OSX v3

Comment 36

[Matt Woodrow (:mattwoodrow)]

Attachment: Add BGRXRect Shaders

We now have IOSurfaces that are GL_RGB, so we need a BGRX shader to render them correctly.

Comment 37

[Matt Woodrow (:mattwoodrow)]

 > @@ +234,5 @@
> > +        return false;
> > +    }
> > +
> > +    if (target == LOCAL_GL_TEXTURE_RECTANGLE_ARB) {
> > +        GLuint texCoordMultLoc = fGetUniformLocation(mTex2DRectBlit_Program, "uTexCoordMult");
> 
> Assert that this didn't give us -1. ('uniform not found')

Oops, both this and the earlier one are incorrectly using GLuint. Fixed them both to GLint and added the assertion.

> 
> @@ +341,5 @@
> >      }
> >  
> >  
> >      ScopedBindFramebuffer boundFB(this, destFB);
> > +    ScopedBindTextureUnit boundTU(this, LOCAL_GL_TEXTURE0);
> 
> Why do we bother to switch texture units here? Don't switch unless we need
> to, in which case we should have a comment as to why we need to. We don't
> want to cargo-cult these things.
> 

The shader blit program we're using reads from GL_TEXTURE0. I guess we could read the current texture unit, and change the shader uniform to match, but I don't think it's worth it.

> ::: gfx/gl/GLScreenBuffer.cpp
> @@ +297,5 @@
> > +    SharedSurface_GL* surf;
> > +    if (GetReadFB() == 0) {
> > +        surf = SharedSurf();
> > +    } else {
> > +        surf = mGL->mFBOMapping[GetReadFB()];
> 
> If we're doing this anyways, why don't we just track it in GLContextCGL? It
> should take about the same amount of work, but be less intrusive, and
> shouldn't have any problematic overhead.
> 
> Also, in what case is this hit? Snapshotting is what I suspect, but leave a
> comment.

It's not obvious how to do this without exposing a weird-ish API on GLContext similar to virtual MaybeRecordFBOMapping(GLint fb, SharedShared* surf) { //no nothing by default }

I'll leave this for now and you can change it to what you prefer.

And yes, snapshotting. Always snapshotting. Comment added.

> 
> ::: gfx/gl/SharedSurfaceIO.cpp
> @@ +13,5 @@
> > +
> > +using namespace gfx;
> > +
> > +/* static */ SharedSurface_IOSurface*
> > +SharedSurface_IOSurface::Create(MacIOSurface* aSurface, GLContext *aGL, bool aHasAlpha)
> 
> Can we move away from using an 'a' prefix for function params?

Not my personal preference, but happy to match local style. Fixed.

> 
> @@ +60,5 @@
> > +
> > +SharedSurface_IOSurface::~SharedSurface_IOSurface()
> > +{
> > +  if (mTexture) {
> > +    mGL->MakeCurrent();
> 
> We should assert this is already true.
> If it's not necessarily true, things get trickier.

I feel like I added this because it wasn't always true. That may have been dependent on earlier bugs, so I've removed it for now.

Why are things trickier?

I'll test that it isn't needed on try, but if it is, what would be the correct fix?


I think I've fixed all other requests that I haven't mentioned above.

Comment 38

[Kelsey Gilbert [:jgilbert]]

Actually, the MakeCurrent is fine, but we need to assert that it succeeded.

Comment 39

[Matt Woodrow (:mattwoodrow)]

Attachment: Add SharedSurface_IOSurface for sharing textures on OSX v4

Added back MakeCurrent(), with an assertion that it succeeded.

Comment 40

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 776818 [details] [diff] [review]
Add SharedSurface_IOSurface for sharing textures on OSX v4

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

::: gfx/gl/SharedSurfaceIO.h
@@ +28,5 @@
> +    virtual bool WaitSync() { return true; }
> +
> +    virtual GLuint Texture() const
> +    {
> +      return mTexture;

4-space, also elsewhere in this file.

::: gfx/layers/opengl/CanvasLayerOGL.cpp
@@ +324,5 @@
>  
>    program->Activate();
>    if (mLayerProgram == RGBARectLayerProgramType) {
>      // This is used by IOSurface that use 0,0...w,h coordinate rather then 0,0..1,1.
> +    program->SetTexCoordMultiplier(mBounds.width, mBounds.height);

I remember doing this in the past and having it not always work. Sometimes mBounds was (0,0) for some reason. Someone else should review this bit. The best way to do this is to just use the Size of the surf we got, for ShSurfs, at least.

It's also possible the issue I ran into before is no longer present. Can we assert that width,height are non-zero, maybe?

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +401,5 @@
>        SharedSurface_EGLImage* eglImageSurf =
>            SharedSurface_EGLImage::Cast(sharedSurf);
>  
>        mTextureHandle = eglImageSurf->AcquireConsumerTexture(mGL);
> +      mTextureTarget = LOCAL_GL_TEXTURE_2D;

Why not pull from ->TextureTarget() here as well?

Comment 41

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 776418 [details] [diff] [review]
Make BlitFramebufferToFramebuffer and friends support targets other than GL_TEXTURE_2D v3

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

It looks like `fragShaderPtr` went away.

::: gfx/gl/GLContextUtils.cpp
@@ +70,5 @@
>  
> +    GLuint *programPtr;
> +    const char* fragShaderSource;
> +    if (target == LOCAL_GL_TEXTURE_2D) {
> +      programPtr = &mTex2DBlit_Program;

4-space

@@ +78,5 @@
> +      fragShaderSource = kTex2DRectBlit_FragShaderSource;
> +    }
> +
> +    GLuint& program = *programPtr;
> +    GLuint& fragShader = *programPtr;

This is surely not right.

@@ +341,5 @@
>      ScopedBindFramebuffer boundFB(this, destFB);
> +    // UseTexQuadProgram initializes a shader that reads
> +    // from texture unit 0.
> +    ScopedBindTextureUnit boundTU(this, LOCAL_GL_TEXTURE0);
> +    ScopedBindTexture boundTex(this, srcTex, srcTarget);

Man, this really does look so much better.

Comment 42

[Matt Woodrow (:mattwoodrow)]

Attachment: Make BlitFramebufferToFramebuffer and friends support targets other than GL_TEXTURE_2D v4

Awesome, wonder how I managed that. Rebase disaster :)

Good catch!

Comment 43

[Nick Cameron [:nrc]]

Comment on attachment 776818 [details] [diff] [review]
Add SharedSurface_IOSurface for sharing textures on OSX v4

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

Not sure I can add much to the review - if jgilbert added me to offer a second opinion on the mBounds thing, then I'm afraid I don't really know, sorry. I had a quick look through the layers stuff (I have no real idea about the GL stuff) and it all looks OK, but I'm familiar with the issues here really, so its not much of a review.

::: gfx/layers/client/CanvasClient.cpp
@@ +16,5 @@
>  #include "SharedSurfaceGralloc.h"
>  #endif
> +#ifdef XP_MACOSX
> +#include "SharedSurfaceIO.h"
> +#endif

why do you need this include if there are no other changes in the file?

Comment 44

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 776870 [details] [diff] [review]
Make BlitFramebufferToFramebuffer and friends support targets other than GL_TEXTURE_2D v4

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

::: gfx/gl/GLContextUtils.cpp
@@ +196,5 @@
>              break;
>          }
>  
> +        MOZ_ASSERT(fGetAttribLocation(program, "aPosition") == 0);
> +        GLint texUnitLoc = fGetUniformLocation(program, "uTexUnit");

Unlikely as it is, let's assert non-`-1` here too.

Comment 45

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aeb5c295ead1
https://hg.mozilla.org/integration/mozilla-inbound/rev/0db7008c7f9f
https://hg.mozilla.org/integration/mozilla-inbound/rev/925fe6245160
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b8139c0dc74
https://hg.mozilla.org/integration/mozilla-inbound/rev/62d61e36d610

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aeb5c295ead1
https://hg.mozilla.org/mozilla-central/rev/0db7008c7f9f
https://hg.mozilla.org/mozilla-central/rev/925fe6245160
https://hg.mozilla.org/mozilla-central/rev/6b8139c0dc74
https://hg.mozilla.org/mozilla-central/rev/62d61e36d610

Comment 47

[Simona Badau, Desktop QA]

I can't reproduce the initial issue on my mini Mac 10.7.5 with a Nightly from May the 20th. 

Jeff, since you were able to reproduce the crash, could you please confirm the issue is fixed on Firefox 25 beta 1?

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/25.0b1-candidates/build1/

Comment 48

[Ioana (away)]

When searching for crashes with the signatures associated to this bug, I get the same results:
Results within 4 weeks of 2013-10-22 14:00:00 where the crash signature contains 'libGPUSupport.dylib@3...', and the product is one of Firefox, and the crashing process was of any type (including unofficial release channels).
No results were found.

This means that there were no crashes with these signatures registered during the last 4 weeks. Considering this and the lack of response to comment 47, I will mark the bug as verified.