Speed up shader compilation and avoid WebGL crashes #8950
Conversation
… compilation and avoid WebGL crashes when many layers are present
📝 WalkthroughWalkthroughReplaced unrolled bilinear/trilinear sampling with loop-collected arrays and stricter negative-alpha checks; removed Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2024-11-22T17:18:43.411ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/javascripts/viewer/shaders/filtering.glsl.ts (4)
17-33: Looped sampling looks good; avoid hardcoding the bucket-address flag and add int→float casts.
- Consider not dropping the precomputed-bucket-address optimization in filtered paths. Either thread the flag through, or at least make it a compile-time constant.
- Cast loop indices to float in the offset to avoid strict GLSL ES 3.0 drivers complaining.
Apply this diff to add explicit casts (safe across GLSL ES versions):
- vec3 offset = vec3(x, y, 0); + vec3 offset = vec3(float(x), float(y), 0.0);Outside this range, prefer making the flag a constant:
// before loops const bool supportsPrecomputedBucketAddress = false;Alternatively (recommended), thread the flag through the signatures and call sites:
// function signatures vec4 getBilinearColorFor(float layerIndex, float d_texture_width, float packingDegree, vec3 coordsUVW, bool supportsPrecomputedBucketAddress); vec4 getTrilinearColorFor(float layerIndex, float d_texture_width, float packingDegree, vec3 coordsUVW, bool supportsPrecomputedBucketAddress); // call sites in getMaybeFilteredColor(...) color = getBilinearColorFor(layerIndex, d_texture_width, packingDegree, worldPositionUVW, supportsPrecomputedBucketAddress); color = getTrilinearColorFor(layerIndex, d_texture_width, packingDegree, worldPositionUVW, supportsPrecomputedBucketAddress);Please verify that forcing
supportsPrecomputedBucketAddress = falseinside bilinear/trilinear paths does not regress performance on datasets where precomputed addressing is available. Also sanity-check on Safari/WebGL1 and Android GPUs for dynamic array writes in loops.
21-33: Optional: compute “anyNegative” during sampling to avoid long OR chain and enable short-circuiting.Keeps code simple while reducing a large conditional and lets compilers short-circuit.
Apply this diff:
- int idx = 0; + int idx = 0; + bool anyNegative = false; for (int y = 0; y <= 1; y++) { for (int x = 0; x <= 1; x++) { vec3 offset = vec3(float(x), float(y), 0.0); samples[idx] = getColorForCoords( layerIndex, d_texture_width, packingDegree, coordsUVW + offset, supportsPrecomputedBucketAddress ); + anyNegative = anyNegative || (samples[idx].a < 0.0); idx++; } } - if (samples[0].a < 0.0 || samples[1].a < 0.0 || samples[2].a < 0.0 || samples[3].a < 0.0) { + if (anyNegative) { // We need to check all four colors for a negative parts, because there will be black // lines at the borders otherwise (black gets mixed with data) return vec4(0.0, 0.0, 0.0, -1.0); }Also applies to: 34-34
61-77: Trilinear looped sampling: same nits—don’t hardcode the flag; add int→float casts.
- Prefer not dropping the precomputed-bucket optimization on filtered paths; thread the flag through (see prior comment).
- Add explicit casts for portability.
Apply this diff to add explicit casts:
- vec3 offset = vec3(x, y, z); + vec3 offset = vec3(float(x), float(y), float(z));Please verify WebGL1 (GLSL ES 1.00) and WebGL2 (GLSL ES 3.00) drivers, especially Safari/ANGLE variants, for dynamic indexing into local arrays in nested loops.
79-81: Optional: replace 8-way OR with a loop-aggregated flag.Reduces condition size and can short-circuit.
Apply this diff:
- if (samples[0].a < 0.0 || samples[1].a < 0.0 || samples[2].a < 0.0 || samples[3].a < 0.0 || - samples[4].a < 0.0 || samples[5].a < 0.0 || samples[6].a < 0.0 || samples[7].a < 0.0) { + bool anyNegative = false; + for (int i = 0; i < 8; ++i) { + anyNegative = anyNegative || (samples[i].a < 0.0); + } + if (anyNegative) { // We need to check all eight colors for a negative parts, because there will be black
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/viewer/shaders/filtering.glsl.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
🔇 Additional comments (2)
frontend/javascripts/viewer/shaders/filtering.glsl.ts (2)
40-41: LGTM: correct bilinear interpolation order.Indexing order matches ab (y=0), cd (y=1) and mixing along x then y.
86-87: LGTM: correct trilinear interpolation sequence.ab/cd at z=0, ab2/cd2 at z=1, then mix along x→y→z in the right order.
Also applies to: 90-91
|
FYI: Safari on Mac and iOS work :-) |
Use loops for bi- and trilinear filtering in shader to speed up shader compilation and avoid WebGL crashes for datasets with many layers.
Turns out, WebGL drivers usually can optimize loops much better than unrolled ones. In this case, the
getColorForCoordsfunction is very complex which exacerbates the issue.There's still a lot of room for improvement, but this is a low-hanging fruit we should definitely pick.
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)