Bug 238788

Created attachment 456667 [details] Patch

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

Created attachment 456671 [details] Patch

Comment on attachment 456671 [details] Patch webkit-patch uploaded the wrong patch.

Created attachment 456672 [details] Patch

Comment on attachment 456672 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456672&action=review nice find! some things to consider, not an ANGLE reviewer > Source/ThirdParty/ANGLE/ChangeLog:7 > + 8-bits, so mMaxColorMask can't use used with the bit difference Nit: "A is extended to 8-bits" initially throws the non-expert reader like me off, since in c++ nothing can be extended to 8-bits as everything is at least 8-bits. Also "extension" would refer to type promotion. Reading the ANGLE code I see you're talking about using BlendStateExt::StorageType::kBits value being 8 instead of 4 on 32-bit system. Also the commit message is a bit begging the question for non-expert reader like me (solve A because we solve A). > Source/ThirdParty/ANGLE/src/libANGLE/angletypes.cpp:444 > + return compareColorMask(ColorMaskStorage::GetReplicatedValue( I'm not a ANGLE reviewer, but the way I understand BlendStateExt is that it 1) contains various mask values based on various lengths 2) encapsulates the complexities of the various buffer mask calculations based on the mask values and lengths it stores. from this perspective it would seem that the bug is in BlendStateExt::compareColorMask() where the function sometimes returns values outside the domain it should. This would mean that if you change the calculation strategy to fix this particular bug, the next user of compareColorMask() would potentially not know that and run into the same or similar issue elsewhere. I see most of the BlendStateExt setters and getters clamp the set and get value with t he mask limit values, in this case mMaxColorMask. Would it make sense to clamp the values returned by compareColorMask() E.g. if we consider ColorMaskStorage::GetDiffMask() as "calculate the bit difference" BendStateExt::compareColorMask() as "calculate the bit difference as it makes sense to this particular blend state" In practice I believe the code would be above return ColorMaskStorage::GetDiffMask(mColorMask, other) & mMaxColorMask; Alternatively we could maybe consider that the bug is that mColorMask stores a value outside its domain? So then tracking the setter that doesn't clamp would fix all bugs related to assuming that mColorMask is never outside the domain.

Thanks for reviewing, :kimmo. I'm not an ANGLE coder, let alone ANGLE reviewer. :-) My assessment of the issue is: 4 bits per draw buffer are used to represent if a channel is masked. (0 - not drawn to, 1 - drawn). So unmasked, draw to all channels is 0xF. Considering 4 draw buffers, in the 32-bit case this is stored as 0x0000_FFFF and in the 64-bit case as 0x00000000_0F0F0F0F (because the elements are widened to 8-bits each). mMaxColorMask appears to be a mask uses to discard parts of the storage that aren't valid elements, ie. the high 16-bits and high 32-bits in the 32-bit and 64-bit cases, respectively. I believe that the mMaxColorMask of 0xFFFF was being used in the 32-bit case because it also happened to match what the correct unmasked write bit pattern. In moving to 64-bit, the mask becomes 0xFFFFFFFF, which doesn't match 0x0F0F0F0F and leads to the error. > In practice I believe the code would be above > return ColorMaskStorage::GetDiffMask(mColorMask, other) & mMaxColorMask; This will still be wrong. `other` needs to be masked first by either 0xF or 0x0F depending on kBits. eg: ``` return ColorMaskStorage::GetDiffMask(mColorMask, 0x0F0F0F0FULL); ``` I'm relying on `ColorMaskStorage::GetReplicatedValue(BlendStateExt::PackColorMask(true, true, true, true))` being constexpr to generate the correct value of 0xFFFF or 0x0F0F0F0F.

(In reply to Dan Glastonbury from comment #7) > ``` > return ColorMaskStorage::GetDiffMask(mColorMask, 0x0F0F0F0FULL); > ``` Sorry, I mean: ``` return ColorMaskStorage::GetDiffMask(mColorMask, other & 0x0F0F0F0FULL); ```

Created attachment 456784 [details] Patch

<rdar://problem/91606700>

CC'ing some other ANGLE/Chromium folks. It'd be ideal if this patch were uploaded to ANGLE's code review tool for more in-depth review by the ANGLE experts and then rolled into WebKit, rather than landing this here and requiring later upstreaming.

Thanks for the patch. I'm worried that the new test might not pass on all of the platforms we test. Can you upload this CL through Gerrit so we can land safely upstream and then cherry-pick it into WebKit? https://chromium-review.googlesource.com/q/project:angle

Also please talk with Dean or Kimmo about getting added to the Apple-side list for permission to upload CLs to ANGLE's code review tool. Thanks.

Looks like I wrote that code some time ago. The color mask comparison does indeed produce incorrect results sometimes. I think the simplest fix would be to adjust mMaxColorMask initialization to keep the unused bits unset on 64-bit systems. Also this would better align the value with the variable name.

Specifically, could you please try updating the BlendStateExt ctor with these expressions? ``` mMaxColorMask(ColorMaskStorage::GetReplicatedValue(PackColorMask(true, true, true, true), ColorMaskStorage::GetMask(drawBuffers))), mColorMask(mMaxColorMask), ```

Really I am very impressed from this post. the creator of this post, it’s a great human being. Thanks for sharing this with us. I will visit your site!

Really I am very impressed from this post. the creator of this post, it’s a great human being. Thanks for sharing this with us. I will visit your site! https://tmmenards.top/