1321450 - InvalidateFramebuffer is not implemented safely

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect, P1)

Description

[Kelsey Gilbert [:jgilbert]]

We can't just call InvalidateFramebuffer and not track which resources are invalidated.

We should probably be implementing this with glClear, which generally behaves very similarly to InvalidateFramebuffer.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1321450 - Fix Invalidate[Sub]Framebuffer. -

Review commit: https://reviewboard.mozilla.org/r/96738/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/96738/

Comment 2

[Ethan Lin[:ethlin]]

Comment on attachment 8815976 [details]
Bug 1321450 - Fix Invalidate[Sub]Framebuffer. -

https://reviewboard.mozilla.org/r/96738/#review97000

::: dom/canvas/WebGL2ContextFramebuffers.cpp:247
(Diff revision 1)
>  
> -    if (width < 0 || height < 0) {
> -        ErrorInvalidValue("%s: width and height must be >= 0.", funcName);
> +    // Some drivers (like OSX 10.9 GL) just don't support invalidate_framebuffer.
> +    const bool useFBInvalidation = (mAllowFBInvalidation &&
> +                                    gl->IsSupported(gl::GLFeature::invalidate_framebuffer));
> +    if (useFBInvalidation) {
> +        gl->fInvalidateFramebuffer(target, glNumAttachments, glAttachments);

We should call MakeContextCurrent() before fInvalidateFramebuffer.

::: dom/canvas/WebGL2ContextFramebuffers.cpp:277
(Diff revision 1)
>  
> -    if (!fb && !isDefaultFB) {
> -        dom::Sequence<GLenum> tmpAttachments;
> -        if (!TranslateDefaultAttachments(attachments, &tmpAttachments)) {
> -            rv.Throw(NS_ERROR_OUT_OF_MEMORY);
> +    // Some drivers (like OSX 10.9 GL) just don't support invalidate_framebuffer.
> +    const bool useFBInvalidation = (mAllowFBInvalidation &&
> +                                    gl->IsSupported(gl::GLFeature::invalidate_framebuffer));
> +    if (useFBInvalidation) {
> +        gl->fInvalidateSubFramebuffer(target, glNumAttachments, glAttachments, x, y,

MakeContextCurrent() too.

Comment 3

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1321450 - Check our GLsizei args. -

Review commit: https://reviewboard.mozilla.org/r/97470/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97470/

Comment 4

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1321450 - Update tests.

Review commit: https://reviewboard.mozilla.org/r/97472/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97472/

Comment 5

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8815976 [details]
Bug 1321450 - Fix Invalidate[Sub]Framebuffer. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96738/diff/1-2/

Comment 6

[Ethan Lin[:ethlin]]

Comment on attachment 8817029 [details]
Bug 1321450 - Check our GLsizei args. -

https://reviewboard.mozilla.org/r/97470/#review97814

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/80afe5f38d07
Fix Invalidate[Sub]Framebuffer. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d7106de1332
Check our GLsizei args. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ee72d9b8e3e
Update tests.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/80afe5f38d07
https://hg.mozilla.org/mozilla-central/rev/0d7106de1332
https://hg.mozilla.org/mozilla-central/rev/5ee72d9b8e3e

Comment 9

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8815976 [details]
Bug 1321450 - Fix Invalidate[Sub]Framebuffer. -

Approval Request Comment (for all patches)
[Feature/Bug causing the regression]: webgl2
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:

Comment 10

[Gerry Chang [:gchang]]

Comment on attachment 8815976 [details]
Bug 1321450 - Fix Invalidate[Sub]Framebuffer. -

We need WebGL 2 in 51. Beta51+ & Aurora52+. Should be in 51 beta 8.

Comment 11

[Carsten Book [:Tomcat]]

needs rebasing for aurora

Comment 12

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3d32a5d8dc3e

Comment 13

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/releases/mozilla-beta/rev/15ecd2d4cce5