Bug 212249 - [ANGLE - iOS] fast/canvas/webgl/uninitialized-test.html is still failing
Summary: [ANGLE - iOS] fast/canvas/webgl/uninitialized-test.html is still failing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Darpinian
URL:
Keywords: InRadar
Depends on:
Blocks: 207858 209139
  Show dependency treegraph
 
Reported: 2020-05-21 19:31 PDT by Justin Fan
Modified: 2020-07-07 12:11 PDT (History)
7 users (show)

See Also:


Attachments
Patch (20.82 KB, patch)
2020-06-04 17:10 PDT, James Darpinian
no flags Details | Formatted Diff | Diff
Patch (20.88 KB, patch)
2020-06-04 17:22 PDT, James Darpinian
no flags Details | Formatted Diff | Diff
Patch (21.17 KB, patch)
2020-06-04 17:40 PDT, James Darpinian
no flags Details | Formatted Diff | Diff
Patch (21.17 KB, patch)
2020-06-05 15:13 PDT, James Darpinian
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].