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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Proposed "TextureImage" API

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?

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 3

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

Ignore Android for this bug -- I need to get more info there; we'll make it work once we have that info.

Comment 4

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

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.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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)?

Comment 6

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

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!

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #7)
> Oleg is going to land bug 571832 "soon"

Er, like a minute ago :).

Comment 9

[Oleg Romashin (:romaxa)]

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...

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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().

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Add a TextureImage interface to manage synchronizing thebes-drawn content with a GL texture

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Add a BasicTextureImage implementation that synchronizes new pixels to its texture with a scratch gfxASurface and glTexSubImage2D()

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: Implement TextureImage for EGL/X11

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 4: Implement TextureImage for GLX

Sorry vlad, I don't know of any other GLX reviewers.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 5: Implement TextureImage for CGL and WGL

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 6: RefactorThebesLayerOGL to use TextureImage

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 0: Move the DEBUG_GL_ERROR_CHECK() macro into GLContext.h

This macro is extremely useful outside of gfx/layers/opengl; I wanted to use it from gfx/thebes/src.

Comment 19

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

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 20

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

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.

Comment 21

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

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.

Comment 22

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

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.

Comment 23

[Oleg Romashin (:romaxa)]

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 24

[Oleg Romashin (:romaxa)]

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..

Comment 25

[Oleg Romashin (:romaxa)]

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*'

Comment 26

[Oleg Romashin (:romaxa)]

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 27

[Oleg Romashin (:romaxa)]

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 28

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Add a BasicTextureImage implementation that synchronizes new pixels to its texture with a scratch gfxASurface and glTexSubImage2D(), v2

Updated per vlad's comments.

Comment 29

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

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.

Comment 30

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: Implement TextureImage for EGL/X11, v2

Fixed compile errors; builds locally for me.  Thanks Oleg.

Comment 31

[Oleg Romashin (:romaxa)]

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.

Comment 32

[Oleg Romashin (:romaxa)]

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 33

[Oleg Romashin (:romaxa)]

Comment on attachment 454771 [details] [diff] [review]
part 6: RefactorThebesLayerOGL to use TextureImage

Ok problem seems in EGL patch

Comment 34

[Oleg Romashin (:romaxa)]

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

Comment 35

[Oleg Romashin (:romaxa)]

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)

Comment 36

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(Punting the GLXPixmap impl means that bug 572873 doesn't block this first step anymore.)

Comment 37

[Bas Schouten (:bas.schouten)]

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.

Comment 38

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/83c3913a77d2
http://hg.mozilla.org/mozilla-central/rev/a2afad95833c
http://hg.mozilla.org/mozilla-central/rev/21ea4d14a5a6
http://hg.mozilla.org/mozilla-central/rev/da3ce7a706c5
http://hg.mozilla.org/mozilla-central/rev/c23cf0f3b601
http://hg.mozilla.org/mozilla-central/rev/f9d19c26f101
http://hg.mozilla.org/mozilla-central/rev/6683cd803cc4