Closed Bug 573889 Opened 10 years ago Closed 10 years ago

Add an API and implementations for efficiently synchronizing Thebes-drawn content to GL textures

Categories

(Core :: Graphics, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: cjones, Assigned: cjones)

References

Details

Attachments

(8 files, 2 obsolete files)

6.04 KB, patch
vlad
: review+
Details | Diff | Splinter Review
4.78 KB, patch
vlad
: review+
romaxa
: review-
Details | Diff | Splinter Review
4.16 KB, patch
vlad
: review+
Details | Diff | Splinter Review
5.87 KB, patch
vlad
: review+
Details | Diff | Splinter Review
10.79 KB, patch
bas.schouten
: review+
romaxa
: review+
Details | Diff | Splinter Review
2.01 KB, patch
vlad
: review+
Details | Diff | Splinter Review
7.82 KB, patch
vlad
: review+
Details | Diff | Splinter Review
5.09 KB, patch
romaxa
: review+
Details | Diff | Splinter Review
This means on X11 platforms.  Vlad tells me that Android code similar to that in bug 571832 (except using something other than Pixmap) can be used to create an EGLImage that can be bound to a texture.  Not sure if that's in scope for this bug.

We find common ground for the code from bug 571832 and what's needed to make this work for GLX.
Attached patch Proposed "TextureImage" API (obsolete) — Splinter Review
The basic overview should be given by the comments in the patch, if it's unclear please ask.

I want to use this in (at least) ThebesLayerOGL to manage the complexity of dealing with all the different image/texture backends.  The usage I have in mind is something like

  LayerOGL::Init(size):
    mTexImage = gl()->CreateTextureImage(size);
    // ...
 
  LayerOGL::Update():
    context = mTexImage->BeginUpdate(mDirtyRect - mScreenOffset);
    // draw to context...
    mTexImage->EndUpdate();

  LayerOGL::Render():
    mTexImage->ActivateTexture();
    // GL stuff
    mTexImage->DeactiveTexture();

For Pixmap-backed TextureImages, TextureImage::BeginUpdate(rect) would return a gfxContext wrapped around the Pixmap's gfxXlibSurface and clipped to rect.  EndUpdate() would XSync() and/or glBindTexImage() and/or whatever as needed.

Elsewhere, TextureImage would own texture memory and a scratch buffer of |TextureImage::mSize|.  Roughly speaking, BeginUpdate(rect) would first create a gfxImageSurface wrapping the first, contiguous rect.width*rect.height*sizeof(pixel) bytes of the scratch buffer, then it would return a gfxContext for that image surface.  EndUpdate() would upload the temporary gfxImageSurface with glTexSubImage2D().  (We might want to create the scratch buffer on demand, or expire it on a timer, or whatever, as experience dictates.)

The end result of both is an "identical" texture, wrt GL API, that can be rendered using the generic ThebesLayerOGL code from bug 573829.

I believe that this, combined with the ThebesLayerBuffer management logic, is about the best we can do with this scheme.  If it's not performant, we might want to look into different approaches for OGL (I have some ideas).

Vlad, this bug doesn't necessarily cover an Android backend, but would Android's image/texture capabilities fit the proposed TextureImage API?
Assignee: nobody → jones.chris.g
Attachment #453344 - Flags: feedback?(vladimir)
Attachment #453344 - Flags: feedback?(romaxa)
I should that TextureImage is intended to be a separate entity from the render-to-texture objects we use for WebGL, if that wasn't clear.
Ignore Android for this bug -- I need to get more info there; we'll make it work once we have that info.
Comment on attachment 453344 [details] [diff] [review]
Proposed "TextureImage" API

This looks fine, though I have a couple of concerns/comments:


>+/**
>+ * A TextureImage encapsulates a surface that can be drawn to by a GL
>+ * client and (hopefully efficiently!) synchronized to a texture in

First, that comment is wrong; by a Thebes gfxContext, not a GL client :-)

>+ * the server.  TextureImages are associated with one and only one
>+ * GLContext.

Second, the texture kind that you create here, specifically the wrap modes, are an issue -- are these optimized for the rotated coordinate system that ThebesLayers want to use?  If so, then the wrap modes will have to be REPEAT, and that'll make this a much less generic thing (and makes me think it should just live inside ThebesLayer).  Video (and, say, Canvas 2D) would want CLAMP_TO_EDGE.  So I think that this comment needs to be very explicit on what the texture state is of the resulting texture -- that is, LINEAR min/mag modes, and whatever the repeat mode is, or to have the option of passing in a repeat mode when it's allocated at least.

>+    /**
>+     * state: IDLE
>+     *
>+     * Activate this's texture in preparation for GL rendering.
>+     *
>+     * next state: TEXTURE_ACTIVE
>+     */
>+    virtual PRBool ActivateTexture() = 0;
>+    /**
>+     * state: TEXTURE_ACTIVE
>+     *
>+     * Deactivate this's texture.  (In practice, a no-op call made for
>+     * the state-transition side effect.)
>+     *
>+     * next state: IDLE
>+     */
>+    virtual PRBool DeactivateTexture() = 0;

I'm not sure I like this, though I understand that you want to keep the state machine.. though I wonder if we shouldn't just get rid of the state machine and have it be a "You better know what you're doing" and have a "virtual GLint TextureName()" call that returns the GL texture id for you to bind/unbind.  That would be a little simpler and clearer to use -- having to call ActivateTexture/DeactivateTexture when they're just going to call glBindTexture with the name and 0 seems overkill, and potentially confusing when you're reading a stream of GL code.
(In reply to comment #4)
> >+ * the server.  TextureImages are associated with one and only one
> >+ * GLContext.
> 
> Second, the texture kind that you create here, specifically the wrap modes, are
> an issue -- are these optimized for the rotated coordinate system that
> ThebesLayers want to use?  If so, then the wrap modes will have to be REPEAT,
> and that'll make this a much less generic thing (and makes me think it should
> just live inside ThebesLayer).  Video (and, say, Canvas 2D) would want
> CLAMP_TO_EDGE.  So I think that this comment needs to be very explicit on what
> the texture state is of the resulting texture -- that is, LINEAR min/mag modes,
> and whatever the repeat mode is, or to have the option of passing in a repeat
> mode when it's allocated at least.
> 

I figured we'd want a configurable wrap mode, but I left that out of this patch.  I didn't have initially plan to parameterize on filter types but that's easy enough to support.  I'll add both to v2.

> >+    /**
> >+     * state: IDLE
> >+     *
> >+     * Activate this's texture in preparation for GL rendering.
> >+     *
> >+     * next state: TEXTURE_ACTIVE
> >+     */
> >+    virtual PRBool ActivateTexture() = 0;
> >+    /**
> >+     * state: TEXTURE_ACTIVE
> >+     *
> >+     * Deactivate this's texture.  (In practice, a no-op call made for
> >+     * the state-transition side effect.)
> >+     *
> >+     * next state: IDLE
> >+     */
> >+    virtual PRBool DeactivateTexture() = 0;
> 
> I'm not sure I like this, though I understand that you want to keep the state
> machine.. though I wonder if we shouldn't just get rid of the state machine and
> have it be a "You better know what you're doing" and have a "virtual GLint
> TextureName()" call that returns the GL texture id for you to bind/unbind. 
> That would be a little simpler and clearer to use -- having to call
> ActivateTexture/DeactivateTexture when they're just going to call glBindTexture
> with the name and 0 seems overkill, and potentially confusing when you're
> reading a stream of GL code.

OK.  With that change, really only BeginUpdate()/EndUpdate() are stateful, and the state is binary: mUpdateContext or !mUpdateContext.  So I'll ditch the state machine stuff.

Do you want to do a feedback review for a v2 interface or should I start implementing (with your comments above addressed)?
Ddon't parametrize on filter type -- just leave it LINEAR (or maybe NEAREST, so maybe just have a boolean).  The reason is that non-power-of-two textures are often only supported with non-mipmapped filter types, so we don't want someone creating an accidental broken texture.  Everything else looks good though, so go ahead and implement!
Oleg is going to land bug 571832 "soon", so I think we should implement TextureImage and refactor ThebesLayerOGL to use that before we add ThebesLayerBuffer-optimized repainting.  The latter two patches in bug 573829 shouldn't need to change much to use TextureImage.  Also, TextureImage doesn't depend on retained layers.
Blocks: 573829
No longer depends on: 573829
Summary: Make ThebesLayerOGL fast where we have Pixmaps that can be bound to textures → Add an API and implementations for efficiently synchronizing Thebes-drawn content to GL textures
(In reply to comment #7)
> Oleg is going to land bug 571832 "soon"

Er, like a minute ago :).
Comment on attachment 453344 [details] [diff] [review]
Proposed "TextureImage" API

Sounds reasonable for me...
Not really sure about Allocate method... isn't it enough to alloc it in ctor which is called in CreateTextureImage? and destroy when TextureImage destroyed?
Otherwise we need also deallocate method...
Attachment #453344 - Flags: feedback?(romaxa) → feedback+
(In reply to comment #9)
> (From update of attachment 453344 [details] [diff] [review])
> Sounds reasonable for me...
> Not really sure about Allocate method... isn't it enough to alloc it in ctor
> which is called in CreateTextureImage? and destroy when TextureImage destroyed?
> Otherwise we need also deallocate method...

Yes, I agree.  My local v2 dropped Allocate().
Attachment #453344 - Attachment is obsolete: true
Attachment #454766 - Flags: superreview?(vladimir)
Attachment #454766 - Flags: review?(jmuizelaar)
Attachment #453344 - Flags: feedback?(vladimir)
Sorry vlad, I don't know of any other GLX reviewers.
Attachment #454769 - Flags: review?(vladimir)
(In reply to comment #13)
> Created an attachment (id=454768) [details]
> part 3: Implement TextureImage for EGL/X11

Two notes for this patch: first, I ignored android per comment 3.  I think we'll need an additional BasicTextureImage EGL implementation for android.  Second, Oleg --- I believe that these patches should be faster than the current implementation on the N900.  Do you mind giving them a spin?  In addition to bug 572873, you also need to apply https://bugzilla.mozilla.org/attachment.cgi?id=453177 before parts 1, 3, and 6.
This macro is extremely useful outside of gfx/layers/opengl; I wanted to use it from gfx/thebes/src.
Attachment #454772 - Flags: review?(vladimir)
Attachment #454766 - Flags: superreview?(vladimir)
Attachment #454766 - Flags: superreview+
Attachment #454766 - Flags: review?(jmuizelaar)
Attachment #454766 - Flags: review+
Comment on attachment 454767 [details] [diff] [review]
part 2: Add a BasicTextureImage implementation that synchronizes new pixels to its texture with a scratch gfxASurface and glTexSubImage2D()


>Bug 573889, part 2: Add a BasicTextureImage implementation that synchronizes new pixels to its texture with a scratch gfxASurface and glTexSubImage2D(). r=vlad

Looks fine, except as I mentioned on IRC, the first time a basic image is rendered to, you have to ask the caller to render the entire rectangle, regardless of what was passed in.  TexImage2D with NULL only allocates memory, but it makes no guarantees as to what's there -- it doesn't clear it or anything.

suggestion from irc:

23:39 < vlad> I think the easiest thing is to just have a boolean for the first beginupdate
23:39 < vlad> and then allocate a gfx*Surface for the entire rect, and use TexImage2D
23:39 < vlad> (instead of SubImage)
23:40 < vlad> because the returned gfx*Surface will be cleared
23:40 < vlad> that way you can actually get rid of the TexImage2D call at creation time, too
Comment on attachment 454770 [details] [diff] [review]
part 5: Implement TextureImage for CGL and WGL

>Bug 573889, part 5: Implement TextureImage for CGL and WGL. r=Bas

(CGL)
>+        // FIXME: make me fast!

It's been a while, but you can create a gfxImageSurface and wrap it in a gfxQuartzImageSurface.  I can't remember how fast that is to draw into, though.  Another option is to grab the quartz surface's CGContext, and try CGBitmapContextGetData etc. on it in case it's a bitmap context.

Can be fixed in a followup, though.
Attachment #454770 - Flags: review?(bas.schouten) → review+
Comment on attachment 454768 [details] [diff] [review]
part 3: Implement TextureImage for EGL/X11

This looks fine, though as we talked about on irc, TextureImageEGL is really only useful for MOZ_X11, as written.  But we can fix things in a followup, as well as make things use EGLImage etc. as appropriate.
Attachment #454768 - Flags: review?(romaxa) → review+
Comment on attachment 454772 [details] [diff] [review]
part 0: Move the DEBUG_GL_ERROR_CHECK() macro into GLContext.h

Call this GLDebugPrintError and leave the actual inline function definition outside of #ifdef DEBUG?  That way it can be quickly dropped into place anywhere in the code even when doing a debug build, without having to rebuild the world.
Attachment #454772 - Flags: review?(vladimir) → review+
Comment on attachment 454768 [details] [diff] [review]
part 3: Implement TextureImage for EGL/X11


>+    virtual gfxContext* BeginUpdate(nsIntRegion& aRegion)
>+    {
>+        NS_ASSERTION(!mUpdateContext, "BeginUpdate() without EndUpdate()?");
>+
>+        mUpdateContext = new gfxContext(impl->mASurface);
should it be mImpl?
Comment on attachment 454768 [details] [diff] [review]
part 3: Implement TextureImage for EGL/X11


>+  nsRefPtr<gfxASurface> pixmap =
>+    gfxPlatform::GetPlatform()->

gfxPlatform include is missing, and fail to compile.

>+  GLuint texture;
>+  gl()->fGenTextures(1, &texture);

this is fail to compile for me:

GLContextProviderEGL.cpp:459: error: expected primary-expression before '(' token
I gues gl() is missing here..
Attachment #454768 - Flags: review-
Comment on attachment 454768 [details] [diff] [review]
part 3: Implement TextureImage for EGL/X11


>+  nsRefPtr<TextureImageEGL> teximage =
>+      new TextureImageEGL(texture, aSize, aContentType, this, impl);

GLContextProviderEGL.cpp:473: error: invalid conversion from 'mozilla::gl::GLContext*' to 'mozilla::gl::GLContextEGL*'
GLContextProviderEGL.cpp:473: error:   initializing argument 5 of 'mozilla::gl::TextureImageEGL::TextureImageEGL(GLuint, const nsIntSize&, gfxASurface::gfxContentType, mozilla::gl::GLContext*, mozilla::gl::GLContextEGL*)'
make[2]: *** [GLContextProviderEGL.o] Error 1
Comment on attachment 454768 [details] [diff] [review]
part 3: Implement TextureImage for EGL/X11


>+    virtual gfxContext* BeginUpdate(nsIntRegion& aRegion)
>+    {
>+        NS_ASSERTION(!mUpdateContext, "BeginUpdate() without EndUpdate()?");
>+
>+        mUpdateContext = new gfxContext(impl->mASurface);
>+        // TextureImageEGL can handle updates to disparate regions
>+        // aRegion = aRegion;
>+        return mUpdateContext.forget();

here is should be 
return mUpdateContext.forget().get();



>+already_AddRefed<TextureImage>
>+GLContextEGL::CreateTextureImage(const nsIntSize& aSize,
>+                                 TextureImage::ContentType aContentType,
>+                                 GLint aWrapMode,
>+                                 PRBool aUseNearestFilter=PR_FALSE)

Default parameter in definition?
Comment on attachment 454944 [details] [diff] [review]
part 2: Add a BasicTextureImage implementation that synchronizes new pixels to its texture with a scratch gfxASurface and glTexSubImage2D(), v2

Looks good, one nit:

>+    } else
>+    {

don't hang that { on its own line, stick it with the else.
Fixed compile errors; builds locally for me.  Thanks Oleg.
Attachment #454981 - Flags: review?(romaxa)
Attachment #454981 - Flags: review?(romaxa) → review+
Comment on attachment 454771 [details] [diff] [review]
part 6: RefactorThebesLayerOGL to use TextureImage

Something wrong with this patch... when I'm applying this patch using mcmq queue. it start working almost 1.5x slower (scrolling)

Investigating where is problem...
without this patch it works fine.
Attachment #454771 - Flags: review-
Comment on attachment 454771 [details] [diff] [review]
part 6: RefactorThebesLayerOGL to use TextureImage

I'm playing test page with HTML5 video content, and video content playing fine, but page is black... als I found that we are repainting thebesLayer for each video frame
Comment on attachment 454771 [details] [diff] [review]
part 6: RefactorThebesLayerOGL to use TextureImage

Ok problem seems in EGL patch
Attachment #454771 - Flags: review- → review+
Comment on attachment 454981 [details] [diff] [review]
part 3: Implement TextureImage for EGL/X11, v2

This line was removed from ThebesLayerOGL::RenderLayer

mGLContext->fBindTexture(LOCAL_GL_TEXTURE_2D, mTexture);

And need to be added to TextureImageEGL EndUpdate
Attachment #454981 - Flags: review+ → review-
Comment on attachment 454981 [details] [diff] [review]
part 3: Implement TextureImage for EGL/X11, v2


>+        // X has already uploaded the new pixels to our Pixmap, so
>+        // there's nothing else we need to do here
>+        mUpdateContext = NULL;
>+        return PR_FALSE;        // texture not bound

oh sorry for spam, we are returning false here, and that activate texture bound in ThebesLayerOGL code... (which was missing during merge)
Attachment #454981 - Flags: review- → review+
(Punting the GLXPixmap impl means that bug 572873 doesn't block this first step anymore.)
No longer depends on: 572873
Comment on attachment 454771 [details] [diff] [review]
part 6: RefactorThebesLayerOGL to use TextureImage

As a nit: I believe the instances of NULL should be replaced by nsnull for consistency.

>+  nsIntSize visibleSize = mVisibleRegion.GetBounds().Size();
>+  TextureImage::ContentType contentType =
>+    CanUseOpaqueSurface() ? gfxASurface::CONTENT_COLOR :
>+                            gfxASurface::CONTENT_COLOR_ALPHA;
>+  if (!mTexImage ||
>+      mTexImage->GetSize() != visibleSize ||
>+      mTexImage->GetContentType() != contentType) {
>+    mValidRegion.SetEmpty();
>+    mInvalidatedRegion.Or(mInvalidatedRegion, mVisibleRegion);

I believe the assignment operator will do here, outside of the visible region validation does not need to be tracked.

>+  nsIntRegion rgnToPaint = mInvalidatedRegion;

We could dump mInvalidatedRegion and use visible region substracted by the valid region.

>+  nsIntRegion mInvalidatedRegion;
>+  nsRefPtr<TextureImage> mTexImage;
>+  // XXX this only exists because of the GetInvalidatedRect() API

That API can be removed as far as I'm concerned.
Attachment #454771 - Flags: review?(bas.schouten) → review+
You need to log in before you can comment on or make changes to this bug.