Closed Bug 739775 Opened 8 years ago Closed 8 years ago

Cleanup GLContext ResizeOffscreenFBO

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Attached patch Cleanup ResizeOffscreenFBO (obsolete) — Splinter Review
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.
Attachment #609872 - Flags: feedback?(bjacob)
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 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?
Attachment #609872 - Flags: feedback?(bjacob) → feedback+
(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.
Assignee: nobody → jgilbert
Status: NEW → ASSIGNED
Attached patch Refactor ResizeOffscreenFBO (obsolete) — Splinter Review
Let's actually review this for landing.
Attachment #609872 - Attachment is obsolete: true
Attachment #613475 - Flags: review?(bjacob)
Attached patch Refactor ResizeOffscreenFBO (obsolete) — Splinter Review
This one actually compiles and runs. ><
Attachment #613475 - Attachment is obsolete: true
Attachment #613475 - Flags: review?(bjacob)
Attachment #613808 - Flags: review?(bjacob)
Blocks: 729726
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.
Attachment #613808 - Flags: review?(bjacob) → review+
(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.
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.
Attachment #615544 - Flags: review?(bjacob)
Attachment #615544 - Flags: review?(bjacob) → review+
Attachment #615544 - Flags: checkin?(jgilbert)
Attachment #613808 - Attachment is obsolete: true
Attachment #615544 - Flags: checkin?(jgilbert) → checkin+
https://hg.mozilla.org/mozilla-central/rev/832debbbdedb
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.