Bug 62318 – Avoid always binding FBO 0 implicitly when deleting FBO in DrawingBuffer code because it invalidates Ganesh's cache of the current FBO.

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Brian Salomon]

Don't bind aframebuffer before deleting it in DrawingBuffer::clear()

[Mike Reed]

Created attachment 96487 [details]
Patch

[James Robinson]

Comment on attachment 96487 [details]
Patch

Could you expand on how this leads to a crash?  Binding before deleting is weird, but it shouldn't be harmful unless there is a bug somewhere else.

[Brian Salomon]

Deleting the bound FBO implicitly binds FBO 0. This confuses Ganesh which thinks it knows the currently bound FBO. 

This isn't a fix-all for coordinating GL state changes between DrawingBuffer and Ganesh. But it is a simple low risk change that fixes a M13 blocker.

[James Robinson]

Comment on attachment 96487 [details]
Patch

I think you need to do the same thing on line 87.

Are you sure that this codepath is never hit with the drawingbuffer's m_fbo already bound?

[James Robinson]

(In reply to comment #3)
> Deleting the bound FBO implicitly binds FBO 0. This confuses Ganesh which thinks it knows the currently bound FBO. 
> 
> This isn't a fix-all for coordinating GL state changes between DrawingBuffer and Ganesh. But it is a simple low risk change that fixes a M13 blocker.

OK, so it sounds like the real bug here is that DrawingBuffer::clear() changes the currently bound framebuffer, and it shouldn't?  Could you update the bug title + ChangeLog text to indicate that's what is going on?

It seems pretty fragile to depend on this function not changing the current framebuffer binding...

[Brian Salomon]

Created attachment 96491 [details]
Patch

[Brian Salomon]

(In reply to comment #4)
>(From update of attachment 96487 [details])
>I think you need to do the same thing on line 87.

Done in the latest patch.

>Are you sure that this codepath is never hit with the drawingbuffer's m_fbo >already bound?

In that case we're actually OK. The Gr object that wraps the DrawingBuffer's FBO is destroyed which causes Gr to invalidate its cache of the currently bound FBO.


(In reply to comment #5)
> OK, so it sounds like the real bug here is that DrawingBuffer::clear() changes the currently bound framebuffer, and it shouldn't?  Could you update the bug title + ChangeLog text to indicate that's what is going on?
> 

Yup, done in the latest patch.

> It seems pretty fragile to depend on this function not changing the current framebuffer binding...

I agree it is fragile. There will be a more robust fix for M14+.

[James Robinson]

Comment on attachment 96491 [details]
Patch

Seems OK

[Brian Salomon]

(In reply to comment #8)
> (From update of attachment 96491 [details])
> Seems OK

Thanks! I forgot to set commit:? after I uploaded.

[WebKit Review Bot]

Comment on attachment 96491 [details]
Patch

Clearing flags on attachment: 96491

Committed r88403: <http://trac.webkit.org/changeset/88403>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.