All users were logged out of Bugzilla on October 13th, 2018

Improve TextureImageGLX logic to reduce glXBindTexImageEXT and glXWaitX calls

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: karlt, Assigned: mattwoodrow)

Tracking

18 Branch
mozilla43
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

6 years ago
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]).
Created attachment 663801 [details] [diff] [review]
draft
Attachment #663801 - Flags: feedback?(karlt)
(Reporter)

Comment 3

6 years ago
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.
Attachment #663801 - Flags: feedback?(karlt) → feedback+
(Reporter)

Comment 4

6 years ago
(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.
Created attachment 665159 [details] [diff] [review]
Patch
Attachment #663801 - Attachment is obsolete: true
Attachment #665159 - Flags: review?(karlt)
(Reporter)

Comment 7

6 years ago
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.)
Attachment #665159 - Flags: review?(karlt) → review-
Created attachment 665292 [details] [diff] [review]
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
Attachment #665159 - Attachment is obsolete: true
Attachment #665292 - Flags: review?(karlt)
Attachment #665292 - Flags: review?(karlt)
(Assignee)

Comment 9

5 years ago
Created attachment 8379404 [details] [diff] [review]
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.
(Assignee)

Updated

5 years ago
Assignee: nobody → matt.woodrow
(Assignee)

Updated

5 years ago
Attachment #665292 - Attachment is obsolete: true
Created attachment 8652967 [details] [diff] [review]
Only rebind a GLXPixmap if the texture has changed.

Slightly tweaked version of Matt's patch using UpdatedInternal. Thanks!
Attachment #8379404 - Attachment is obsolete: true
Attachment #8652967 - Flags: review?(karlt)
(Reporter)

Comment 11

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

># User Andrew Comminos <acomminos@mozilla.com>

Include Matt Woodrow <mwoodrow@mozilla.com> here too (combine with "and").
Attachment #8652967 - Flags: review?(karlt) → review+
Created attachment 8653255 [details] [diff] [review]
Only rebind a GLXPixmap if the texture has changed. Carry r=karlt

Updated, thanks.
Attachment #8652967 - Attachment is obsolete: true
Attachment #8653255 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/c84e1f31ceef
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.