976813 - Separate ShSurf::Texture() for Producer and Consumer

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

Right now, the Texture is always accessed via ShSurf::Texture(). This has caused a bug where we don't actually use IOSurfaces properly.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: mac-io-surf

Comment 2

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

Comment on attachment 8381746 [details] [diff] [review]
mac-io-surf

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

::: gfx/gl/GLContext.h
@@ +2678,5 @@
>      ScopedDeletePtr<GLBlitTextureImageHelper> mBlitTextureImageHelper;
>      ScopedDeletePtr<GLReadTexImageHelper> mReadTexImageHelper;
>  
>  public:
> +    ScopedDeletePtr<GLBlitHelper> mBlitHelper;

Unintentional change?

Comment 3

[Kelsey Gilbert [:jgilbert]]

Attachment: lock-tex

These patches were sort of going with the wrong bugs.

Comment 4

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

Comment on attachment 8387273 [details] [diff] [review]
lock-tex

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

Looks good, though I don't really get moving that stuff from SharedSurface into SharedSurface_GL and using that everywhere

::: gfx/gl/GLContext.cpp
@@ +1560,5 @@
>  
>      return true;
>  }
>  
> +SharedSurface_GL*

So why do we have SharedSurface at all now?

::: gfx/gl/SharedSurfaceEGL.cpp
@@ +107,5 @@
>      }
>  }
>  
> +GLuint
> +SharedSurface_EGLImage::ProdTexture()

can probably stick trivial stuff like this in the header

Comment 5

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

Comment on attachment 8387273 [details] [diff] [review]
lock-tex

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

::: gfx/gl/SharedSurfaceGL.h
@@ +68,5 @@
> +        MOZ_ASSERT(AttachType() == AttachmentType::GLTexture);
> +        MOZ_CRASH("Did you forget to override this function?");
> +    }
> +
> +    virtual GLenum TextureTarget() const {

Maybe this should be ProdTextureTarget(). We do have some places now where this differs between prod/cons...

Comment 6

[Kelsey Gilbert [:jgilbert]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #4)
> Comment on attachment 8387273 [details] [diff] [review]
> lock-tex
> 
> Review of attachment 8387273 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, though I don't really get moving that stuff from SharedSurface
> into SharedSurface_GL and using that everywhere
> 
> ::: gfx/gl/GLContext.cpp
> @@ +1560,5 @@
> >  
> >      return true;
> >  }
> >  
> > +SharedSurface_GL*
> 
> So why do we have SharedSurface at all now?
Originally it was supposed to be a base class that could support CPU-only resources. Since this isn't really needed for WebGL, it was never developed. I suppose we can remove it, since MozSurface is going to be the future, anyways.
> 
> ::: gfx/gl/SharedSurfaceEGL.cpp
> @@ +107,5 @@
> >      }
> >  }
> >  
> > +GLuint
> > +SharedSurface_EGLImage::ProdTexture()
> 
> can probably stick trivial stuff like this in the header

Comment 7

[Kelsey Gilbert [:jgilbert]]

Attachment: patch

Added ProTextureTarget.

Comment 8

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Try&rev=4121f674335e

Comment 9

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6e7ed4cd0a6

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d6e7ed4cd0a6