739775 - Cleanup GLContext ResizeOffscreenFBO

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

Cleaning up this mountain of a function is useful for a number of reasons. We should definitely split out choosing our formats from actually using them.

I am looking at a method of multi-buffering based on switching out the color attachment on our backing FBO, so an easy way to skip straight to buffer attachment/assembly would also be a plus.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: Cleanup ResizeOffscreenFBO

This patch splits everything out into sections. We also have a few other changes:
We no longer specify the RGBA4/RGBA8 for our color renderbuffer (just use RGBA)
We no longer ask for RGB565 anywhere.
GLES no longer tries to use 32-bit depth buffers when available.

Comment 2

[Kelsey Gilbert [:jgilbert]]

Derp, it also looks like I didn't propagate the naming change from DeleteOffscreenFBO to DeleteOffscreenFBOs. I'm only looking for feedback on the structure of the change, really, though.

Comment 3

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

Comment on attachment 609872 [details] [diff] [review]
Cleanup ResizeOffscreenFBO

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

I like where this is going. Some nits about naming. Please add some comments to explain what each function is doing, it will definitely help future readers.

::: gfx/gl/GLContext.cpp
@@ +1222,5 @@
> +        }
> +    }
> +
> +    return formats;
> +}

I like that this seems to be splitting the existing kludge into functions that do one thing each.

@@ +1251,5 @@
> +    fBindTexture(LOCAL_GL_TEXTURE_2D, boundTexture);
> +}
> +
> +static inline void
> +RenderbufferStorageHelper(const GLContext* gl, GLsizei samples, GLenum internalFormat, const gfxIntSize& size)

Hm, please find a better name. This is like "hamburger helper" pasta.

::: gfx/gl/GLContext.h
@@ +1637,5 @@
>          NS_WARNING("ResizeOffscreenFBO failed to resize AA context even without AA!");
>          return false;
>      }
>  
> +    struct GLFormats {

Why the 's' here?

Comment 4

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #3)
> Comment on attachment 609872 [details] [diff] [review]
> Cleanup ResizeOffscreenFBO
> 
> Review of attachment 609872 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like where this is going. Some nits about naming. Please add some comments
> to explain what each function is doing, it will definitely help future
> readers.
> 
> ::: gfx/gl/GLContext.cpp
> @@ +1222,5 @@
> > +        }
> > +    }
> > +
> > +    return formats;
> > +}
> 
> I like that this seems to be splitting the existing kludge into functions
> that do one thing each.
> 
> @@ +1251,5 @@
> > +    fBindTexture(LOCAL_GL_TEXTURE_2D, boundTexture);
> > +}
> > +
> > +static inline void
> > +RenderbufferStorageHelper(const GLContext* gl, GLsizei samples, GLenum internalFormat, const gfxIntSize& size)
> 
> Hm, please find a better name. This is like "hamburger helper" pasta.

'RenderbufferStorageBySamples'?

> 
> ::: gfx/gl/GLContext.h
> @@ +1637,5 @@
> >          NS_WARNING("ResizeOffscreenFBO failed to resize AA context even without AA!");
> >          return false;
> >      }
> >  
> > +    struct GLFormats {
> 
> Why the 's' here?

The struct contains the the formats for color, stencil, and depth buffers, so I thought plural would be appropriate. The idea is that we're picking all of our GL formatS before-hand.

Comment 5

[Kelsey Gilbert [:jgilbert]]

Attachment: Refactor ResizeOffscreenFBO

Let's actually review this for landing.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: Refactor ResizeOffscreenFBO

This one actually compiles and runs. ><

Comment 7

[Kelsey Gilbert [:jgilbert]]

Try is clean:
https://tbpl.mozilla.org/?tree=Try&rev=da9be9ca3f7f

Comment 8

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

Comment on attachment 613808 [details] [diff] [review]
Refactor ResizeOffscreenFBO

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

::: gfx/gl/GLContext.cpp
@@ +1516,2 @@
>          printf_stderr("Framebuffer status: %X\n", status);
> +  #endif

Why can't this printf_stderr be a plain NS_WARNING without the #ifdef?

@@ +1572,3 @@
>  
>      // Replace with the new hotness
> +    std::swap(mOffscreenDrawFBO, drawFBO);

Hah, so that's what you use <algorithm> for.

Comment 9

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #8)
> Comment on attachment 613808 [details] [diff] [review]
> Refactor ResizeOffscreenFBO
> 
> Review of attachment 613808 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLContext.cpp
> @@ +1516,2 @@
> >          printf_stderr("Framebuffer status: %X\n", status);
> > +  #endif
> 
> Why can't this printf_stderr be a plain NS_WARNING without the #ifdef?

NS_WARNING doesn't support printf-like formatting. If we use enough code to append together a string, it'll be big enough to warrant putting in a #if block for clarity's sake. We would also be relying on the unused path being optimized out.

Comment 10

[Kelsey Gilbert [:jgilbert]]

Attachment: Better Refactor ResizeOffscreenFBO

Actually, it looks like I'm the worst, and that patch was slightly old.
New try:
https://tbpl.mozilla.org/?tree=Try&rev=0fa641b899c1

Changes:
Early out in GLContext.cpp 3-arg GLContext::ResizeOffscreenFBOs.
Simplified AA-failure fallback logic in GLContext.h 2-arg GLContext::ResizeOffscreenFBOs.

Comment 11

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/832debbbdedb

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/832debbbdedb