Bug 126448 – [WebGL2] Framebuffer and renderbuffer updates

• Component: WebGL
• Status: RESOLVED FIXED

[Dean Jackson]

There are a bunch of new framebuffer methods:

void blitFramebuffer(GLint srcX0, GLint srcY0, GLint srcX1, GLint srcY1, GLint dstX0, GLint dstY0, GLint dstX1, GLint dstY1, GLbitfield mask, GLenum filter);
  void framebufferTextureLayer(GLenum target, GLenum attachment, GLuint texture, GLint level, GLint layer);
  any getInternalformatParameter(GLenum target, GLenum internalformat, GLenum pname);
  void invalidateFramebuffer(GLenum target, sequence<GLenum> attachments);
  void invalidateSubFramebuffer (GLenum target, sequence<GLenum> attachments, GLint x, GLint y, GLsizei width, GLsizei height);
  void readBuffer(GLenum mode);

[Dean Jackson]

<rdar://problem/15002334>

[Kenneth Russell]

Taking this bug. James implemented at least some of this functionality in Bug 209098.

[Kenneth Russell]

*** Bug 126447 has been marked as a duplicate of this bug. ***

[Kenneth Russell]

Expanding this bug to cover adding the following entrypoints:

getInternalformatParameter
getRenderbufferParameter
renderbufferStorage
renderbufferStorageMultisample

jdarpinian@ hooked most of these up in Bug 209098, but it's necessary to scan through and make sure all of the necessary validation is in place.

[Kenneth Russell]

Created attachment 405103 [details]
Patch

[Kenneth Russell]

The attached patch is large, but the code was ported over from Chromium's WebGL 2.0 implementation, and has been well tested. The new code makes several more WebGL 2.0 conformance tests pass 100%.

[Kenneth Russell]

Created attachment 405104 [details]
Patch

[Kenneth Russell]

Removed a parameter that was unused in release builds.

[Dean Jackson]

Comment on attachment 405104 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405104&action=review

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:760
> +        if (!validateTexFuncLayer("framebufferTextureLayer", texTarget, layer))
> +            return;
> +        if (!validateTexFuncLevel("framebufferTextureLayer", texTarget, level))
> +            return;

Took me a few reads to work out what was different :)

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:887
> +    Vector<GCGLenum> translatedAttachments = attachments;
> +    if (!checkAndTranslateAttachments("invalidateFramebuffer", target, translatedAttachments))

Since you're taking a copy anyway, why not have checkAndTranslateAttachments return a translated vector?

[Kenneth Russell]

Comment on attachment 405104 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405104&action=review

Thanks for your super fast review Dean! Some more unused parameters to be fixed up; I'll upload revised patches.

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:887
>> +    if (!checkAndTranslateAttachments("invalidateFramebuffer", target, translatedAttachments))
> 
> Since you're taking a copy anyway, why not have checkAndTranslateAttachments return a translated vector?

It's not clear to me how to express errors in this scenario. Do you have a suggestion?

[Kenneth Russell]

Created attachment 405108 [details]
Patch

[Kenneth Russell]

Created attachment 405110 [details]
Patch

[Kenneth Russell]

Marked Dean as the reviewer in the latest patch. I'll CQ if the EWS bots are all green.

[Kenneth Russell]

Created attachment 405112 [details]
Patch

[Kenneth Russell]

Created attachment 405113 [details]
Patch

[Kenneth Russell]

Created attachment 405116 [details]
Patch

[Kenneth Russell]

Created attachment 405155 [details]
Patch

[Kenneth Russell]

Finally all green! CQ'ing.

[EWS]

Committed r264845: <https://trac.webkit.org/changeset/264845>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405155 [details].

[Radar WebKit Bug Importer]

<rdar://problem/66065236>

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/264845/webkit

has broken webgl/2.0.0/conformance2/textures/misc/tex-unpack-params.html

for all Mac platforms. History:
https://results.webkit.org/?suite=layout-tests&test=webgl%2F2.0.0%2Fconformance2%2Ftextures%2Fmisc%2Ftex-unpack-params.html

Diff: https://build.webkit.org/results/Apple-Catalina-Debug-WK2-Tests/r264919%20(5729)/webgl/2.0.0/conformance2/textures/misc/tex-unpack-params-diff.txt

It looks like a lot of the individual subtests are getting reported in the diff. 
It looks like this was not caught by EWS because of some preexisting expectation causing EWS to skip running this test. 

Can this get looked at today?

[Kenneth Russell]

For the record, Bug 214763 was filed about the new failure of webgl/2.0.0/conformance2/textures/misc/tex-unpack-params.html after this change.