Bug 212249

Summary:

[ANGLE - iOS] fast/canvas/webgl/uninitialized-test.html is still failing

Product:

WebKit

Reporter:

Justin Fan <justin_fan>

Component:

WebGL

Assignee:

James Darpinian <jdarpinian>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dino, ews-watchlist, graouts, jdarpinian, kbr, kondapallykalyan, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

207858, 209139

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none

Description Justin Fan 2020-05-21 19:31:21 PDT

Our test suite contains an extended version of the unitialized-test.html test found in the WebGL conformance suite that contains additional tests for copyTexSubImage2D.

On iOS/Simulator, pixels that lie outside of the bounds of the FBO being copied by copyTexSubImage2D are incorrectly modified. They should remain untouched from the clearColor set previously.

This test passes on macOS WebKit.

Comment 1 Justin Fan 2020-05-21 19:32:07 PDT

<rdar://problem/63416418>

Comment 2 James Darpinian 2020-05-29 16:01:47 PDT

It seems like copyTexSubImage2D is broken in some way in the driver on iOS when copying from renderbuffers. I'm implementing a workaround in ANGLE which uses shaders to implement the copy instead.

Comment 3 James Darpinian 2020-06-04 17:10:05 PDT

Created attachment 401098 [details]
Patch

Comment 4 James Darpinian 2020-06-04 17:22:38 PDT

Created attachment 401102 [details]
Patch

Comment 5 EWS Watchlist 2020-06-04 17:23:59 PDT

Note that there are important steps to take when updating ANGLE. See http://trac.webkit.org/wiki/UpdatingANGLE

Comment 6 James Darpinian 2020-06-04 17:40:07 PDT

Created attachment 401103 [details]
Patch

Comment 7 James Darpinian 2020-06-05 15:13:53 PDT

Created attachment 401205 [details]
Patch

Comment 8 Dean Jackson 2020-06-05 19:34:18 PDT

Comment on attachment 401205 [details]
Patch

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

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/renderergl_utils.cpp:1732
> +        IsApple() && functions->standard == STANDARD_GL_ES && !(isAMD && IsWindows()));

How can both isApple() and GL_ES be true while that last condition is false? Is it necessary?

If you were on Windows AMD, you'd already have failed isApple and GL_ES.

> Source/ThirdParty/ANGLE/util/EGLPlatformParameters.h:78
> -    EGLint renderer                         = EGL_PLATFORM_ANGLE_TYPE_DEFAULT_ANGLE;
> -    EGLint majorVersion                     = EGL_DONT_CARE;
> -    EGLint minorVersion                     = EGL_DONT_CARE;
> -    EGLint deviceType                       = EGL_PLATFORM_ANGLE_DEVICE_TYPE_HARDWARE_ANGLE;
> -    EGLint presentPath                      = EGL_DONT_CARE;
> -    EGLint debugLayersEnabled               = EGL_DONT_CARE;
> -    EGLint contextVirtualization            = EGL_DONT_CARE;
> -    EGLint robustness                       = EGL_DONT_CARE;
> -    EGLint transformFeedbackFeature         = EGL_DONT_CARE;
> -    EGLint allocateNonZeroMemoryFeature     = EGL_DONT_CARE;
> -    angle::PlatformMethods *platformMethods = nullptr;
> +    EGLint renderer                               = EGL_PLATFORM_ANGLE_TYPE_DEFAULT_ANGLE;
> +    EGLint majorVersion                           = EGL_DONT_CARE;
> +    EGLint minorVersion                           = EGL_DONT_CARE;
> +    EGLint deviceType                             = EGL_PLATFORM_ANGLE_DEVICE_TYPE_HARDWARE_ANGLE;
> +    EGLint presentPath                            = EGL_DONT_CARE;
> +    EGLint debugLayersEnabled                     = EGL_DONT_CARE;
> +    EGLint contextVirtualization                  = EGL_DONT_CARE;
> +    EGLint robustness                             = EGL_DONT_CARE;
> +    EGLint transformFeedbackFeature               = EGL_DONT_CARE;
> +    EGLint allocateNonZeroMemoryFeature           = EGL_DONT_CARE;
> +    EGLint emulateCopyTexImage2DFromRenderbuffers = EGL_DONT_CARE;
> +    angle::PlatformMethods *platformMethods       = nullptr;

This diff is my favourite argument against requiring alignment of variable assignments :)

Comment 9 James Darpinian 2020-06-05 22:29:06 PDT

Comment on attachment 401205 [details]
Patch

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

>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/renderergl_utils.cpp:1732
>> +        IsApple() && functions->standard == STANDARD_GL_ES && !(isAMD && IsWindows()));
> 
> How can both isApple() and GL_ES be true while that last condition is false? Is it necessary?
> 
> If you were on Windows AMD, you'd already have failed isApple and GL_ES.

This is explained in the comment above. I had to add a test suppression for Windows AMD, and I added the redundant condition so that this doesn't accidentally get enabled there without someone noticing that they need to unsuppress and fix the test as well.

>> Source/ThirdParty/ANGLE/util/EGLPlatformParameters.h:78
>> +    angle::PlatformMethods *platformMethods       = nullptr;
> 
> This diff is my favourite argument against requiring alignment of variable assignments :)

Yeah, unfortunately this is required by the clang-format options ANGLE uses.

Comment 10 EWS 2020-06-07 14:08:03 PDT

Committed r262701: <https://trac.webkit.org/changeset/262701>

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