Closed Bug 1321450 Opened 3 years ago Closed 3 years ago

InvalidateFramebuffer is not implemented safely

Categories

(Core :: Canvas: WebGL, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox50 --- disabled
firefox51 --- fixed
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

Attachments

(3 files)

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.
Priority: -- → P1
Blocks: webgl2-blockers
No longer blocks: webgl2
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.
Attachment #8815976 - Flags: review?(ethlin) → review+
Comment on attachment 8817029 [details]
Bug 1321450 - Check our GLsizei args. -

https://reviewboard.mozilla.org/r/97470/#review97814
Attachment #8817029 - Flags: review?(ethlin) → review+
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]:
Attachment #8815976 - Flags: approval-mozilla-beta?
Attachment #8815976 - Flags: approval-mozilla-aurora?
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.
Attachment #8815976 - Flags: approval-mozilla-beta?
Attachment #8815976 - Flags: approval-mozilla-beta+
Attachment #8815976 - Flags: approval-mozilla-aurora?
Attachment #8815976 - Flags: approval-mozilla-aurora+
needs rebasing for aurora
Flags: needinfo?(jgilbert)
Blocks: 1249361
You need to log in before you can comment on or make changes to this bug.