webgl: Fix the incorrect result with shapes uniforms #5502
Conversation
|
@mattsoulanille @pyu10055 @lina128 Please take a look, thanks. @mattsoulanille please help to trigger the |
| isLogicalShapTexShapeEqual}_${rank1}_${rank2}_${rank34}_${ | ||
| isTexShapeGreaterThanOne}_${hasOffset}`; | ||
| keyInputs += `${xRank}_${isInOutTexShapeEqual}_${ | ||
| useSqueezeShape ? keptDims : ''}_${uniformShape.length}_${isScalar}_${ |
There was a problem hiding this comment.
Some failures are caused by missing keptDims in shader key.
Previously, we couldn't distinguish
float getA(int row, int col, int depth)
{
return getA(col, depth);
}
vs
float getA(int row, int col, int depth)
{
return getA(row, depth);
}
| const isScalar = util.sizeFromShape(x.shape) === 1; | ||
| const broadcastDims = | ||
| backend_util.getBroadcastDims(x.shape, output.shape); | ||
| const isInOutTexShapeEqual = !program.packedInputs && | ||
| xRank === output.shape.length && | ||
| util.arraysEqual(xTexShape, output.texData.texShape); | ||
| const isTexShapeGreaterThanOne = program.packedInputs || xRank > 2 ? | ||
| const isTexShapeGreaterThanOne = | ||
| program.packedInputs || uniformShape.length > 2 ? |
There was a problem hiding this comment.
Some failures are caused by using xRank instead of uniformShape.length.
Previously, if we have a 3d shape, which maybe squeeze to a 2d shape, we should check whether the 2d shape's texShape is greater than one to be as the part of shaderKey. However, if we use xRank > 2 as the condition, isTexShapeGreaterThanOne will be an empty string which is not we expected.
|
Thank you! Here's the nightly run (still running at the time of writing). |
It timed out. Could you please try it again? |
Started retry. |
|
This is timing out due to another bug with Firefox on MacOS likely introduced by a PR between last Friday and Tuesday (but haven't narrowed it down yet). @pyu10055 is looking into it. I've manually tested this PR on Chrome Mac (browserstack), so I think the uniforms bug is fixed. However, It's probably best if we fix the nightly timeout and then get a successful nightly run on this branch before merging it. Thanks again for the fix! I'll post another nightly run here once the timeout bug is fixed / reverted. |
|
It seems that Ping has fixed the nightly test. Please take another look, thanks. |
There was a problem hiding this comment.
New nightly run, which is passing! I also took a look at the changes, and they look good to me. Thank you! I would have taken a look at this earlier, but I was OOO Thursday and Friday.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status:complete! 1 of 1 approvals obtained (waiting on @lina128 and @pyu10055)
|
Fixes #5479 |
BUG #5479
To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
This change is