Resizing totally broken on OpenGL

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: joe, Assigned: joe)

Tracking

(Blocks 1 bug)

Trunk
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Right now, resizing a window that's accelerated using OpenGL draws garbage. The reason this happens is because, in the BasicTextureImage destructor, we preferentially set the shared context as current. Unfortunately, all of our previous state was set on the normal mGLContext, meaning important things like the current viewport get unset, causing us not to draw.

This patch just reverses the order we choose our contexts in, but I'd be willing to use a different approach to not make the wrong context current if it'd be cleaner. (One thing that has come to my mind is a resize call on TexImage.)
Attachment #473904 - Flags: review?(vladimir)
Assignee: nobody → joe
blocking2.0: --- → beta6+
Posted patch fixed-er (obsolete) — Splinter Review
Previous patch is obviously wrong if mGLContext is null. Just use mGLContext unconditionally.
Attachment #473904 - Attachment is obsolete: true
Attachment #473905 - Flags: review?(vladimir)
Attachment #473904 - Flags: review?(vladimir)
Posted patch how about this instead? (obsolete) — Splinter Review
Nice!

How about this instead, though?

Fixes the problem on win32 too btw, now just leaves some flashing to black in between rendering normal frames, but that goes away when I turn on double buffering.
Attachment #473933 - Flags: review?(joe)
Also Cc'ing Benoit, since this would have been caught by the debug GLContext that he's working on whose utility I was dubious about all day :-)  (it'll catch among other things calling a function from one GLContext while another is current).
this also fixes the new tab corruption.
I think that your patch is a little wrong if both contexts have been destroyed. This patch is built on current birch (I pushed my previous patch to birch) and fixes the problem I saw.

I also don't really know what the shared context is for, clearly, so I think you're a better person to review it. (And explain it!)
Attachment #473905 - Attachment is obsolete: true
Attachment #473933 - Attachment is obsolete: true
Attachment #474214 - Flags: review?(vladimir)
Attachment #473933 - Flags: review?(joe)
Attachment #473905 - Flags: review?(vladimir)
well, I thought about that afterwards, but I'm pretty sure it'll be correct in either case.  A shared context is a "parent" context that we can use to cleanup any resources; it's associated with a random offscreen framebuffer and is effectively global.  On some platforms we might not be able to share though, but if we can, it can speed up some things (like it can let us do texture loads on a thread etc).

So we look if the real context was destroyed (e.g. the widget went away), and if so, then we need to use the shared context to delete the texture.  If we have no shared context then we're done, because the only context that had access to the texture is gone (and thus the resources were released).  I don't think we'll ever be in a situation where we have a destroyed shared context, though maybe; replacing the if condition with if (ctx && !ctx->IsDestroyed()) is probably the right thing to do.

I'll push the followup to brich.
Sounds good.
Duplicate of this bug: ogl-osx-beta
Duplicate of this bug: 586796
Attachment #474214 - Flags: review?(vladimir) → review+
You need to log in before you can comment on or make changes to this bug.