Last Comment Bug 705665 - Don't whine while glDeleting non-existent GL object
: Don't whine while glDeleting non-existent GL object
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
Mentors:
Depends on: 766666
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-28 07:14 PST by Benoit Jacob [:bjacob] (mostly away)
Modified: 2012-06-20 12:10 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
don't delete the same FBO twice (797 bytes, patch)
2011-11-28 07:14 PST, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review-
Details | Diff | Splinter Review
don't delete the same FBO twice, updated (2.84 KB, patch)
2011-12-01 13:09 PST, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
don't delete the same FBO twice, updated (3.33 KB, patch)
2011-12-01 13:10 PST, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review-
Details | Diff | Splinter Review
allow that (814 bytes, patch)
2011-12-02 15:04 PST, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
Details | Diff | Splinter Review

Description Benoit Jacob [:bjacob] (mostly away) 2011-11-28 07:14:33 PST
Created attachment 577254 [details] [diff] [review]
don't delete the same FBO twice

When DrawFBO == ReadFBO, we were deleting it twice. While not a GL error, this caused our GL debug helpers to make noise.
Comment 1 Jeff Gilbert [:jgilbert] 2011-11-28 14:20:12 PST
Comment on attachment 577254 [details] [diff] [review]
don't delete the same FBO twice

Review of attachment 577254 [details] [diff] [review]:
-----------------------------------------------------------------

Good, but we also do this in ResizeOffscreenFBO, so we should fix these all together.
Comment 2 Jeff Gilbert [:jgilbert] 2011-11-28 14:25:03 PST
http://www.khronos.org/opengles/sdk/docs/man/xhtml/glDeleteFramebuffers.xml
http://www.opengl.org/sdk/docs/man3/xhtml/glDeleteFramebuffers.xml

Wait, this behavior is explicitly allowed by the spec. What's going on that we need to change this?
Comment 3 Benoit Jacob [:bjacob] (mostly away) 2011-11-28 15:40:04 PST
Yes it's definitely allowed by the spec, but our own debug helpers still make warnings about that,
http://hg.mozilla.org/mozilla-central/file/bc48009a6bbb/gfx/gl/GLContext.cpp#l2577

and that warning has proved very useful in the course of working on bug 704839 as it helped discover a double-delete issue, so I think that we should actively try to avoid deleting twice the same OpenGL object even if OpenGL allows this.
Comment 4 Benoit Jacob [:bjacob] (mostly away) 2011-12-01 13:09:14 PST
Created attachment 578364 [details] [diff] [review]
don't delete the same FBO twice, updated

How about this? It doesn't seem to actually matter that we set ids to 0 in MarkDestroyed. See the implementation of IsDestroyed. MarkDestroy zeros all symbols so we can't make GL calls anymore anyway.
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2011-12-01 13:10:08 PST
Created attachment 578365 [details] [diff] [review]
don't delete the same FBO twice, updated

even this.
Comment 6 Jeff Gilbert [:jgilbert] 2011-12-02 02:55:24 PST
Comment on attachment 578365 [details] [diff] [review]
don't delete the same FBO twice, updated

Review of attachment 578365 [details] [diff] [review]:
-----------------------------------------------------------------

I don't like that we're adapting to the non-spec behavior of our helpers. Is it not possible to fix them?

Ability to delete unused names is similar to the ability to delete 0 without effect. These effectively-no-op calls shouldn't really cost us anything, and keep things nice and simple.

::: gfx/gl/GLContext.cpp
@@ +1306,5 @@
>      if (!framebuffersComplete) {
>          NS_WARNING("Error resizing offscreen framebuffer -- framebuffer(s) not complete");
>  
>          // Clean up the mess
> +        DeleteOffscreenFBO();

We can't make this replacement since here we need to be deleting the new FBOs we allocated, but failed for some reason. We need to leave the previous working FBOs in place.
Comment 7 Benoit Jacob [:bjacob] (mostly away) 2011-12-02 15:04:38 PST
Created attachment 578736 [details] [diff] [review]
allow that
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2011-12-02 15:05:15 PST
(In reply to Jeff Gilbert [:jgilbert] from comment #6)
> I don't like that we're adapting to the non-spec behavior of our helpers. Is
> it not possible to fix them?

Everything is possible. The world is full of possibilities.
Comment 9 Jeff Gilbert [:jgilbert] 2011-12-02 18:57:00 PST
Comment on attachment 578736 [details] [diff] [review]
allow that

Review of attachment 578736 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome.

If we ever find that these useless calls are giving us too much overhead, we can go through and avoid double-deletion and deleting of 0.
Comment 10 Benoit Jacob [:bjacob] (mostly away) 2011-12-04 11:21:05 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/c7e1e8cb36d4
Comment 11 Matt Brubeck (:mbrubeck) 2011-12-05 10:15:29 PST
https://hg.mozilla.org/mozilla-central/rev/c7e1e8cb36d4

Note You need to log in before you can comment on or make changes to this bug.