Don't whine while glDeleting non-existent GL object

RESOLVED FIXED in mozilla11

Status

()

defect
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

unspecified
mozilla11
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

When DrawFBO == ReadFBO, we were deleting it twice. While not a GL error, this caused our GL debug helpers to make noise.
Attachment #577254 - Flags: review?(jgilbert)
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.
Attachment #577254 - Flags: review?(jgilbert) → review-
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?
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.
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.
Attachment #577254 - Attachment is obsolete: true
Attachment #578364 - Flags: review?(jgilbert)
even this.
Attachment #578364 - Attachment is obsolete: true
Attachment #578364 - Flags: review?(jgilbert)
Attachment #578365 - Flags: review?(jgilbert)
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.
Attachment #578365 - Flags: review?(jgilbert) → review-
Posted patch allow thatSplinter Review
Attachment #578365 - Attachment is obsolete: true
Attachment #578736 - Flags: review?(jgilbert)
(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.
Summary: Don't delete the same FBO twice → Don't whine while glDeleting non-existent GL object
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.
Attachment #578736 - Flags: review?(jgilbert) → review+
https://hg.mozilla.org/mozilla-central/rev/c7e1e8cb36d4
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Depends on: 766666
You need to log in before you can comment on or make changes to this bug.