788873 - Improve TextureImageGLX logic to reduce glXBindTexImageEXT and glXWaitX calls

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Karl Tomlinson (:karlt)]

If Begin or EndUpdate() mark TExtureImageGLX as having been updated, then BindTexture() need only call BindTexImage when it has been updated.

There is a patch in bug 680817 to special case NVIDIA drivers and skip BindTexImage calls after X rendering updates the surface, but the approach proposed in this bug could also benefit other drivers and would also reduce glxWaitX calls (not yet handled in attachment 555645 [details] [diff] [review]).

Comment 1

[Marco Castelluccio [:marco]]

Layers disabled: https://tbpl.mozilla.org/?tree=Try&rev=d56fe8671b68
Without the patch: https://tbpl.mozilla.org/?tree=Try&rev=939fcbaa5186
With the patch: https://tbpl.mozilla.org/?tree=Try&rev=9f59a4e47b66

Comment 2

[Marco Castelluccio [:marco]]

Attachment: draft

Comment 3

[Karl Tomlinson (:karlt)]

Comment on attachment 663801 [details] [diff] [review]
draft

The ReleaseTexImage call in ReleaseTexture is probably the cause of the reftest failures.  With the other changes here, ReleaseTexImage needs to only be called when necessary.

http://www.opengl.org/registry/specs/EXT/texture_from_pixmap.txt says
"Rendering to the drawable while it is bound to a texture will leave the
    contents of the texture in an undefined state."

so BeginUpdate seems the right place for the ReleaseTexImage call.
Only call ReleaseTexImage after BindTexImage has been called.
i.e. when !mNeedsBindTex.
Think about whether negating the meaning of that variable and renaming to mHaveBoundTexImage would be helpful.

Comment 4

[Karl Tomlinson (:karlt)]

(In reply to Marco Castelluccio [:marco] from comment #1)
> Without the patch: https://tbpl.mozilla.org/?tree=Try&rev=939fcbaa5186
> With the patch: https://tbpl.mozilla.org/?tree=Try&rev=9f59a4e47b66

I'm not seeing much if any improvement in the talos benchmarks, but this still seems the right thing to do.

Comment 5

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 6

[Marco Castelluccio [:marco]]

https://tbpl.mozilla.org/?tree=Try&rev=7f454641dff5

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 665159 [details] [diff] [review]
Patch

Looks like the right kind of thing, but need to be consistent in the variable names and what true and false mean.

(Looks like this makes TextureImage::ReleaseTexture a no-op in all implementations now, so that and ScopedBindTexture could be removed.)

Comment 8

[Marco Castelluccio [:marco]]

Attachment: Patch

(In reply to Karl Tomlinson (:karlt) from comment #7)
> Looks like the right kind of thing, but need to be consistent in the
> variable names and what true and false mean.

Oops, sorry... I was really absent-minded yesterday :D

https://tbpl.mozilla.org/?tree=Try&rev=9a391dd326ee

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: Rebased patch

I guess we might want this again, rebased against the current architecture.

I haven't tested this yet since I don't have my linux machine with me.

Comment 10

[Andrew Comminos [:acomminos]]

Attachment: Only rebind a GLXPixmap if the texture has changed.

Slightly tweaked version of Matt's patch using UpdatedInternal. Thanks!

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 8652967 [details] [diff] [review]
Only rebind a GLXPixmap if the texture has changed.

># User Andrew Comminos <[e-mail]>

Include Matt Woodrow <[e-mail]> here too (combine with "and").

Comment 12

[Andrew Comminos [:acomminos]]

Attachment: Only rebind a GLXPixmap if the texture has changed. Carry r=karlt

Updated, thanks.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c84e1f31ceef

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c84e1f31ceef