705665 - Don't whine while glDeleting non-existent GL object

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: 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

[Kelsey Gilbert [: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.

Comment 2

[Kelsey Gilbert [:jgilbert]]

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)]

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)]

Attachment: 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)]

Attachment: don't delete the same FBO twice, updated

even this.

Comment 6

[Kelsey Gilbert [: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.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: allow that

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

(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

[Kelsey Gilbert [:jgilbert]]

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)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c7e1e8cb36d4

Comment 11

[Matt Brubeck (:mbrubeck)]

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