Bug 126448 - [WebGL2] Framebuffer and renderbuffer updates
Summary: [WebGL2] Framebuffer and renderbuffer updates
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kenneth Russell
URL:
Keywords: InRadar
: 126447 (view as bug list)
Depends on: 209098
Blocks: 126404 214660 214763
  Show dependency treegraph
 
Reported: 2014-01-03 12:05 PST by Dean Jackson
Modified: 2020-07-31 17:46 PDT (History)
12 users (show)

See Also:


Attachments
Patch (177.45 KB, patch)
2020-07-23 17:34 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (177.35 KB, patch)
2020-07-23 17:43 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (177.17 KB, patch)
2020-07-23 18:03 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (177.12 KB, patch)
2020-07-23 18:11 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (177.18 KB, patch)
2020-07-23 18:21 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (178.53 KB, patch)
2020-07-23 18:34 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (178.65 KB, patch)
2020-07-23 18:43 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (178.66 KB, patch)
2020-07-24 09:27 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2014-01-03 12:05:32 PST
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);
Comment 1 Dean Jackson 2014-01-03 12:05:45 PST
<rdar://problem/15002334>
Comment 2 Kenneth Russell 2020-07-21 17:09:06 PDT
Taking this bug. James implemented at least some of this functionality in Bug 209098.
Comment 3 Kenneth Russell 2020-07-23 08:58:18 PDT
*** Bug 126447 has been marked as a duplicate of this bug. ***
Comment 4 Kenneth Russell 2020-07-23 09:00:14 PDT
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.
Comment 5 Kenneth Russell 2020-07-23 17:34:58 PDT
Created attachment 405103 [details]
Patch
Comment 6 Kenneth Russell 2020-07-23 17:37:11 PDT
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%.
Comment 7 Kenneth Russell 2020-07-23 17:43:29 PDT
Created attachment 405104 [details]
Patch
Comment 8 Kenneth Russell 2020-07-23 17:43:58 PDT
Removed a parameter that was unused in release builds.
Comment 9 Dean Jackson 2020-07-23 17:52:59 PDT
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?
Comment 10 Kenneth Russell 2020-07-23 17:56:04 PDT
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?
Comment 11 Kenneth Russell 2020-07-23 18:03:01 PDT
Created attachment 405108 [details]
Patch
Comment 12 Kenneth Russell 2020-07-23 18:11:12 PDT
Created attachment 405110 [details]
Patch
Comment 13 Kenneth Russell 2020-07-23 18:11:52 PDT
Marked Dean as the reviewer in the latest patch. I'll CQ if the EWS bots are all green.
Comment 14 Kenneth Russell 2020-07-23 18:21:30 PDT
Created attachment 405112 [details]
Patch
Comment 15 Kenneth Russell 2020-07-23 18:34:21 PDT
Created attachment 405113 [details]
Patch
Comment 16 Kenneth Russell 2020-07-23 18:43:33 PDT
Created attachment 405116 [details]
Patch
Comment 17 Kenneth Russell 2020-07-24 09:27:34 PDT
Created attachment 405155 [details]
Patch
Comment 18 Kenneth Russell 2020-07-24 11:27:15 PDT
Finally all green! CQ'ing.
Comment 19 EWS 2020-07-24 11:33:20 PDT
Committed r264845: <https://trac.webkit.org/changeset/264845>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405155 [details].
Comment 20 Radar WebKit Bug Importer 2020-07-24 11:34:23 PDT
<rdar://problem/66065236>
Comment 21 Truitt Savell 2020-07-27 09:50:02 PDT
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?
Comment 22 Kenneth Russell 2020-07-31 17:46:10 PDT
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.