Don't whine while glDeleting non-existent GL object

RESOLVED FIXED in mozilla11

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

unspecified
mozilla11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
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?
(Assignee)

Comment 3

6 years ago
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.
(Assignee)

Comment 4

6 years ago
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.
Attachment #577254 - Attachment is obsolete: true
Attachment #578364 - Flags: review?(jgilbert)
(Assignee)

Comment 5

6 years ago
Created attachment 578365 [details] [diff] [review]
don't delete the same FBO twice, updated

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-
(Assignee)

Comment 7

6 years ago
Created attachment 578736 [details] [diff] [review]
allow that
Attachment #578365 - Attachment is obsolete: true
Attachment #578736 - Flags: review?(jgilbert)
(Assignee)

Comment 8

6 years ago
(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.
(Assignee)

Updated

6 years ago
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+
http://hg.mozilla.org/integration/mozilla-inbound/rev/c7e1e8cb36d4
Assignee: nobody → bjacob
https://hg.mozilla.org/mozilla-central/rev/c7e1e8cb36d4
Status: NEW → RESOLVED
Last Resolved: 6 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.