Closed
Bug 788873
Opened 12 years ago
Closed 9 years ago
Improve TextureImageGLX logic to reduce glXBindTexImageEXT and glXWaitX calls
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: karlt, Assigned: mattwoodrow)
References
Details
Attachments
(1 file, 5 obsolete files)
6.08 KB,
patch
|
acomminos
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
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•12 years ago
|
||
Attachment #663801 -
Flags: feedback?(karlt)
Reporter | ||
Comment 3•12 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•12 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.
Comment 5•12 years ago
|
||
Attachment #663801 -
Attachment is obsolete: true
Attachment #665159 -
Flags: review?(karlt)
Comment 6•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=7f454641dff5
Reporter | ||
Comment 7•12 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-
Comment 8•12 years ago
|
||
(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)
Updated•12 years ago
|
Attachment #665292 -
Flags: review?(karlt)
Assignee | ||
Comment 9•10 years ago
|
||
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•10 years ago
|
Assignee: nobody → matt.woodrow
Assignee | ||
Updated•10 years ago
|
Attachment #665292 -
Attachment is obsolete: true
Comment 10•9 years ago
|
||
Slightly tweaked version of Matt's patch using UpdatedInternal. Thanks!
Attachment #8379404 -
Attachment is obsolete: true
Attachment #8652967 -
Flags: review?(karlt)
Reporter | ||
Comment 11•9 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+
Comment 12•9 years ago
|
||
Updated, thanks.
Attachment #8652967 -
Attachment is obsolete: true
Attachment #8653255 -
Flags: review+
Comment 14•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c84e1f31ceef
Status: NEW → RESOLVED
Closed: 9 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.
Description
•