Bug 212249

Summary: [ANGLE - iOS] fast/canvas/webgl/uninitialized-test.html is still failing
Product: WebKit Reporter: Justin Fan <justin_fan>
Component: WebGLAssignee: James Darpinian <jdarpinian>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, ews-watchlist, graouts, jdarpinian, kbr, kondapallykalyan, webkit-bug-importer
Priority: P2 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

Justin Fan
Reported 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.
Attachments
Patch (20.82 KB, patch)
2020-06-04 17:10 PDT, James Darpinian 		no flags
DetailsFormatted DiffDiff
Patch (20.88 KB, patch)
2020-06-04 17:22 PDT, James Darpinian 		no flags
DetailsFormatted DiffDiff
Patch (21.17 KB, patch)
2020-06-04 17:40 PDT, James Darpinian 		no flags
DetailsFormatted DiffDiff
Patch (21.17 KB, patch)
2020-06-05 15:13 PDT, James Darpinian 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Justin Fan
Comment 1 2020-05-21 19:32:07 PDT
<rdar://problem/63416418>
James Darpinian
Comment 2 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.
James Darpinian
Comment 3 2020-06-04 17:10:05 PDT
Created attachment 401098 [details] Patch
James Darpinian
Comment 4 2020-06-04 17:22:38 PDT
Created attachment 401102 [details] Patch
EWS Watchlist
Comment 5 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
James Darpinian
Comment 6 2020-06-04 17:40:07 PDT
Created attachment 401103 [details] Patch
James Darpinian
Comment 7 2020-06-05 15:13:53 PDT
Created attachment 401205 [details] Patch
Dean Jackson
Comment 8 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 :)
James Darpinian
Comment 9 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.
EWS
Comment 10 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].
Note You need to log in before you can comment on or make changes to this bug.