Last Comment Bug 609219 - [OpenGL] CairoImageOGL::SetData fails with gfxXlibSurfaces
: [OpenGL] CairoImageOGL::SetData fails with gfxXlibSurfaces
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Matt Woodrow (:mattwoodrow)
: Milan Sreckovic [:milan]
Depends on:
  Show dependency treegraph
Reported: 2010-11-03 00:18 PDT by Matt Woodrow (:mattwoodrow)
Modified: 2011-11-17 20:47 PST (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Part 1 - Switch CairoImageOGL to TextureImage (8.34 KB, patch)
2010-11-03 02:12 PDT, Matt Woodrow (:mattwoodrow)
romaxa: feedback+
Details | Diff | Splinter Review
New Plan Part 1 - Remove broken code in SetData (1.13 KB, patch)
2010-11-03 16:46 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
Part 2 - Remove unused CreateForNativePixmapSurface (2.82 KB, patch)
2010-11-03 16:47 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review

Description Matt Woodrow (:mattwoodrow) 2010-11-03 00:18:39 PDT
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) 2010-11-03 02:12:57 PDT
Created attachment 487864 [details] [diff] [review]
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) 2010-11-03 06:13:50 PDT
Comment on attachment 487864 [details] [diff] [review]
Part 1 - Switch CairoImageOGL to TextureImage

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


> 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] 2010-11-03 09:41:57 PDT
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) 2010-11-03 13:59:54 PDT
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) 2010-11-03 16:46:59 PDT
Created attachment 488072 [details] [diff] [review]
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) 2010-11-03 16:47:29 PDT
Created attachment 488073 [details] [diff] [review]
Part 2 - Remove unused CreateForNativePixmapSurface
Comment 7 Matt Woodrow (:mattwoodrow) 2011-04-13 22:42:35 PDT
Fixed by bug 640082

Note You need to log in before you can comment on or make changes to this bug.