609219 - [OpenGL] CairoImageOGL::SetData fails with gfxXlibSurfaces

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Matt Woodrow (:mattwoodrow)]

There appears to be multiple problems here. What I'm seeing is inside GLContextProviderGLX::CreateForNativePixmapSurface we call glXMakeCurrent on the newly created context, and the contexts of the pixmap turn to garbage.

We also don't check the result of mASurfaceAsGLContext->BindTexImage(); and this function isn't implemented on our GLX backend.

I also don't see how binding the pixmap as a texture to this new context will let us render it to the main context. ImageLayerOGL::Render() only uses mTexture.

This is causing a few reftest failures with OpenGL layers

REFTEST TEST-START | file:///home/cltbld/talos-slave/tryserver_fedora_test-reftest/build/reftest/tests/modules/plugin/test/reftest/div-sanity.html
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/tryserver_fedora_test-reftest/build/reftest/tests/modules/plugin/test/reftest/plugin-sanity.html |

Suggestion solution is (thanks romaxa!):

1) Convert CairoImageOGL to use TextureImage instead of its own texture handling
2) Implement fast pixmap to texture copying for TextureImageGLX
3) ???

I'll start working on a patch for part 1 as this should be the last thing standing between us and a green RGL run.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1 - Switch CairoImageOGL to TextureImage

This fixes our reftest failures but is very definitely a slow path.

I think we could also add a new option to TextureImage to create one using an existing surface and eliminate that copy.

I'll have a go at this and adding a fast path to TextureImageGLX at some point

Comment 2

[Oleg Romashin (:romaxa)]

Comment on attachment 487864 [details] [diff] [review]
Part 1 - Switch CairoImageOGL to TextureImage


>-    ApplyFilter(mFilter);
>+    ColorTextureLayerProgram* program = mOGLManager->GetBGRALayerProgram();
> 

Use    
mOGLManager->GetBasicLayerProgram(CanUseOpaqueSurface(),
                                  cairoImage->mTexture->IsRGB());



> void
> CairoImageOGL::SetData(const CairoImage::Data &aData)
> {
>-  if (!mTexture.IsAllocated())
>-    return;
>+  nsIntSize size = nsIntSize(aData.mSize.width, aData.mSize.height);
>+  mTexture = mManager->gl()->CreateTextureImage(size,
>+                                                aData.mSurface->GetContentType(),
>+                                                LOCAL_GL_CLAMP_TO_EDGE);

Can we avoid somehow permanent texture create/destroy ? like cache and check if mTexture already available and size and type are the same...
The rest seems to works fine for me

Comment 3

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

Why are we making CairoImageOGL use TextureImage?  Images are write-once static things; texture images are not.  By using TextureImage here we're ensuring at least one needless copy.

I don't understand how comment #0 relates to CairoImageOGL at all, except perhaps

> I also don't see how binding the pixmap as a texture to this new context will
> let us render it to the main context. ImageLayerOGL::Render() only uses
> mTexture.

.. though now that I look at the code, I see CairoImageOGL is trying to use CreateForNativePixmapSurface.  Why not just do a texture upload directly?  Again, these are read-only things, so there isn't much to be gained from having them be shared with an xlib surface.

Comment 4

[Matt Woodrow (:mattwoodrow)]

A fast texture upload is going to be backend specific. GLX will want to be using glXBindTexImage instead of drawing to an image surface and then using glTexSubImage2D.

Using TextureImage seemed like an easy way to make use of the backend specific optimizations without duplicating much code. I think we can extend the TextureImage API to avoid the extra copy.

I guess we could add a second class next to TextureImage that's more suited for these once off operations.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: New Plan Part 1 - Remove broken code in SetData

So it looks like CreateForNativePixmapSurface will never work in this situation because we can't guarantee the lifetime of the incoming surface.

Part 1 is just removing this path so we always take the slow (but working!) fallback path.

Part 2 is deleting GLContextProviderGLX::CreateForNativePixmapSurface since nothing uses it anymore and we're trying to phase this code out.

I think from here we could add a BindTexThebesSurface() API to GLContext which attempts to do what CreateForNativePixmapSurface did, but without creating a new context.

For it to work here we'd need to detect compatibility with the current surface and make a copy, deferring the Bind call until render time. Not sure if that would give us noticeable performance improvements.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2 - Remove unused CreateForNativePixmapSurface

Comment 7

[Matt Woodrow (:mattwoodrow)]

Fixed by bug 640082