Bug 71057 – [chromium] webkit_gpu_tests are flaky with --accelerated-drawing

• Component: Layout and Rendering
• Status: RESOLVED WONTFIX

[Alok Priyadarshi]

Accelerated drawing path does not reset the framebuffer state. It just deletes the temporary framebuffer. According to OpenGL spec, deleting a bound framebuffer should set the default frame buffer as current, but at least Mesa does not seem to do so. I think it is better to explicitly reset the framebuffer state.

[Alok Priyadarshi]

Created attachment 112767 [details]
proposed patch

[Alok Priyadarshi]

ping!

This needs to be committed before https://bugs.webkit.org/show_bug.cgi?id=70822.

[James Robinson]

Comment on attachment 112767 [details]
proposed patch

I don't think this is right. If there is a bug with our implementation of GraphicsContext3D we should fix that, no?

[James Robinson]

I would suspect that this sort of things would be handled by the command buffer code, not osmesa.

[Alok Priyadarshi]

(In reply to comment #4)
> I would suspect that this sort of things would be handled by the command buffer code, not osmesa.

I should have been clearer. According to the OpenGL spec, if a currently-bound FBO is deleted, the default framebuffer (id = 0) is bound. So it is the job of the underlying GL implementation (osmesa) in this case to implement the spec. I do not this GraphicsContext3D or command-buffer should do anything extra.

[James Robinson]

I'm not sure what point you are trying to make. I see code to handle deleting the currently bound framebuffer in GLES2DecoderImpl::DeleteFramebuffersHelper() and am pretty sure it's relevant here.

Whatever the issue, we shouldn't be working around it in WebKit we should fix it wherever the bug actually lies.

[Alok Priyadarshi]

(In reply to comment #6)
> I'm not sure what point you are trying to make. I see code to handle deleting the currently bound framebuffer in GLES2DecoderImpl::DeleteFramebuffersHelper() and am pretty sure it's relevant here.
> 
> Whatever the issue, we shouldn't be working around it in WebKit we should fix it wherever the bug actually lies.

If you read the OpenGL spec for glDeleteFrameBuffers (http://www.opengl.org/sdk/docs/man3/xhtml/glDeleteFramebuffers.xml), it says:

"If a framebuffer that is currently bound to one or more of the targets GL_DRAW_FRAMEBUFFER or GL_READ_FRAMEBUFFER is deleted, it is as though glBindFramebuffer had been executed with the corresponding target and framebuffer zero."

Anyways my point is that the accelerated drawing path and Ganesh modify the currently bound framebuffer and do not restore it. We either restore it here or like other GL states do it in the LayerRendererChromium at the beginning of each frame. Doing it in LayerRendererChromium would be consistent with other GL states but unnecessary for the software path.

[James Robinson]

I still don't quite get it. Is the scenario you are concerned with that ganesh will think FB #3 is bound, this code will delete FB #4, which binds FB #0 and confuses ganesh? If so then your patch doesn't make much sense since it binds FB #0 explicitly. Is there some other failure scenario? An example would help.

[Alok Priyadarshi]

(In reply to comment #6)
> I'm not sure what point you are trying to make. I see code to handle deleting the currently bound framebuffer in GLES2DecoderImpl::DeleteFramebuffersHelper() and am pretty sure it's relevant here.
> 
> Whatever the issue, we shouldn't be working around it in WebKit we should fix it wherever the bug actually lies.

I think you are right. Command buffer is internally using FBO=1 as the default render-surface for off-screen contexts, which it should bind when currently bound FBO is deleted. I have filed a bug here - crbug.com/102391

The reason this bug was only visible in layout tests and not chrome because layout-tests use an off-screen render surface. It was also not visible when render-to-texture was enabled because that code path explicitly binds FBO=0 when doing the final paint to the window.

[James Robinson]

OK, then when that's fixed we should be good to go without this patch. Thanks for tracing this down to the root cause.