Note: There are a few cases of duplicates in user autocompletion which are being worked on.

[OpenGL] CairoImageOGL::SetData fails with gfxXlibSurfaces

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

7 years ago
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.
(Assignee)

Comment 1

7 years ago
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
Assignee: nobody → matt.woodrow+bugzilla
Attachment #487864 - Flags: feedback?(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
Attachment #487864 - Flags: feedback?(romaxa) → feedback+
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.
(Assignee)

Comment 4

7 years ago
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.
(Assignee)

Comment 5

7 years ago
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.
Attachment #487864 - Attachment is obsolete: true
Attachment #488072 - Flags: review?(vladimir)
(Assignee)

Comment 6

7 years ago
Created attachment 488073 [details] [diff] [review]
Part 2 - Remove unused CreateForNativePixmapSurface
Attachment #488073 - Flags: review?(vladimir)
(Assignee)

Comment 7

6 years ago
Fixed by bug 640082
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Attachment #488072 - Flags: review?(vladimir)
(Assignee)

Updated

6 years ago
Attachment #488073 - Flags: review?(vladimir)
You need to log in before you can comment on or make changes to this bug.