Fix WebP decoding capability detection #2700
Conversation
WalkthroughThe EXT_texture_webp extension's WebP support detection is refactored to use an asynchronous approach by loading a minimal WebP image and resolving a promise. The Changes
Sequence Diagram(s)sequenceDiagram
participant EXT_texture_webp
participant Browser
participant GLTFParserContext
EXT_texture_webp->>Browser: checkWebpSupport() (load minimal WebP image)
Browser-->>EXT_texture_webp: Promise resolves with support status
EXT_texture_webp->>EXT_texture_webp: Await support check in createAndParse
EXT_texture_webp->>GLTFParserContext: Parse texture using WebP or fallback source
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code Graph Analysis (1)packages/core/src/SystemInfo.ts (1)
🪛 Biome (1.9.4)packages/core/src/SystemInfo.ts[error] 91-91: Using this in a static context can be confusing. this refers to the class. (lint/complexity/noThisInStatic) [error] 92-92: Using this in a static context can be confusing. this refers to the class. (lint/complexity/noThisInStatic) [error] 93-93: Using this in a static context can be confusing. this refers to the class. (lint/complexity/noThisInStatic) [error] 109-109: Using this in a static context can be confusing. this refers to the class. (lint/complexity/noThisInStatic) 🔇 Additional comments (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/loader/src/gltf/extensions/EXT_texture_webp.ts (2)
27-27: Fix spacing issue in type declaration.Apply this diff to fix the formatting:
- private _supportWebP = false as boolean| Promise<boolean>; + private _supportWebP = false as boolean | Promise<boolean>;🧰 Tools
🪛 ESLint
[error] 27-27: Insert
·(prettier/prettier)
34-34: Add missing semicolon.Apply this diff to fix the formatting:
- this._supportWebP = checkWebpSupport() + this._supportWebP = checkWebpSupport();🧰 Tools
🪛 ESLint
[error] 34-34: Insert
;(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/loader/src/gltf/extensions/EXT_texture_webp.ts(2 hunks)
🧰 Additional context used
🪛 ESLint
packages/loader/src/gltf/extensions/EXT_texture_webp.ts
[error] 12-12: Delete ··
(prettier/prettier)
[error] 13-13: Delete ····
(prettier/prettier)
[error] 14-14: Replace ········ with ····
(prettier/prettier)
[error] 15-15: Replace ··const·result·=·(img.width·>·0)·&&·(img.height·>·0) with const·result·=·img.width·>·0·&&·img.height·>·0
(prettier/prettier)
[error] 16-16: Replace ········ with ······
(prettier/prettier)
[error] 17-17: Replace ········ with ····
(prettier/prettier)
[error] 18-18: Delete ····
(prettier/prettier)
[error] 19-19: Delete ··
(prettier/prettier)
[error] 20-20: Delete ····
(prettier/prettier)
[error] 21-21: Replace ········img.src·=·'data:image/webp;base64,UklGRhACAABXRUJQVlA4WAoAAAAwAAAAAAAAAAAASUNDUMgBAAAAAAHIAAAAAAQwAABtbnRyUkdCIFhZWiAH4AABAAEAAAAAAABhY3NwAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQAA9tYAAQAAAADTLQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAlkZXNjAAAA8AAAACRyWFlaAAABFAAAABRnWFlaAAABKAAAABRiWFlaAAABPAAAABR3dHB0AAABUAAAABRyVFJDAAABZAAAAChnVFJDAAABZAAAAChiVFJDAAABZAAAAChjcHJ0AAABjAAAADxtbHVjAAAAAAAAAAEAAAAMZW5VUwAAAAgAAAAcAHMAUgBHAEJYWVogAAAAAAAAb6IAADj1AAADkFhZWiAAAAAAAABimQAAt4UAABjaWFlaIAAAAAAAACSgAAAPhAAAts9YWVogAAAAAAAA9tYAAQAAAADTLXBhcmEAAAAAAAQAAAACZmYAAPKnAAANWQAAE9AAAApbAAAAAAAAAABtbHVjAAAAAAAAAAEAAAAMZW5VUwAAACAAAAAcAEcAbwBvAGcAbABlACAASQBuAGMALgAgADIAMAAxADZBTFBIAgAAAAAAVlA4IBgAAAAwAQCdASoBAAEAAUAmJaQAA3AA/v02aAA=' with ····img.src·=⏎······"data:image/webp;base64,UklGRhACAABXRUJQVlA4WAoAAAAwAAAAAAAAAAAASUNDUMgBAAAAAAHIAAAAAAQwAABtbnRyUkdCIFhZWiAH4AABAAEAAAAAAABhY3NwAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQAA9tYAAQAAAADTLQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAlkZXNjAAAA8AAAACRyWFlaAAABFAAAABRnWFlaAAABKAAAABRiWFlaAAABPAAAABR3dHB0AAABUAAAABRyVFJDAAABZAAAAChnVFJDAAABZAAAAChiVFJDAAABZAAAAChjcHJ0AAABjAAAADxtbHVjAAAAAAAAAAEAAAAMZW5VUwAAAAgAAAAcAHMAUgBHAEJYWVogAAAAAAAAb6IAADj1AAADkFhZWiAAAAAAAABimQAAt4UAABjaWFlaIAAAAAAAACSgAAAPhAAAts9YWVogAAAAAAAA9tYAAQAAAADTLXBhcmEAAAAAAAQAAAACZmYAAPKnAAANWQAAE9AAAApbAAAAAAAAAABtbHVjAAAAAAAAAAEAAAAMZW5VUwAAACAAAAAcAEcAbwBvAGcAbABlACAASQBuAGMALgAgADIAMAAxADZBTFBIAgAAAAAAVlA4IBgAAAAwAQCdASoBAAEAAUAmJaQAA3AA/v02aAA="
(prettier/prettier)
[error] 22-22: Replace ··}) with });
(prettier/prettier)
[error] 27-27: Insert ·
(prettier/prettier)
[error] 34-34: Insert ;
(prettier/prettier)
[error] 50-50: Insert ·
(prettier/prettier)
🔇 Additional comments (2)
packages/loader/src/gltf/extensions/EXT_texture_webp.ts (2)
40-46: Excellent improvement to WebP detection reliability.Converting the method to async to properly handle the asynchronous WebP support detection is the correct approach. This addresses the Safari WebP detection issues mentioned in the PR objectives.
55-55: LGTM! Texture selection logic is correctly updated.The texture selection now properly uses the awaited
supportWebPboolean value instead of the potentially Promise-typed_supportWebPproperty.
| let supportWebP = false; | ||
| if(this._supportWebP) { | ||
| supportWebP = await this._supportWebP; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix spacing and improve type safety.
The logic correctly awaits the WebP support promise, but there's a formatting issue and the type handling could be more explicit.
Apply this diff to fix formatting and improve type safety:
- let supportWebP = false;
- if(this._supportWebP) {
- supportWebP = await this._supportWebP;
- }
+ let supportWebP = false;
+ if (this._supportWebP instanceof Promise) {
+ supportWebP = await this._supportWebP;
+ } else {
+ supportWebP = this._supportWebP;
+ }This makes the type checking more explicit and handles both Promise and boolean cases properly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let supportWebP = false; | |
| if(this._supportWebP) { | |
| supportWebP = await this._supportWebP; | |
| } | |
| let supportWebP = false; | |
| if (this._supportWebP instanceof Promise) { | |
| supportWebP = await this._supportWebP; | |
| } else { | |
| supportWebP = this._supportWebP; | |
| } |
🧰 Tools
🪛 ESLint
[error] 50-50: Insert ·
(prettier/prettier)
🤖 Prompt for AI Agents
In packages/loader/src/gltf/extensions/EXT_texture_webp.ts around lines 49 to
52, fix the spacing issue by adding proper indentation and line breaks. Improve
type safety by explicitly checking if this._supportWebP is a Promise or a
boolean, then await or assign accordingly to supportWebP. This ensures correct
handling of both Promise and boolean types and clearer, more maintainable code.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/loader/src/gltf/extensions/EXT_texture_webp.ts (1)
44-53: Method correctly handles asynchronous WebP support detection.The
createAndParsemethod properly uses.then()to wait for WebP support detection before selecting the appropriate texture source. The promise chaining maintains the same return type while enabling asynchronous behavior.Consider caching WebP support detection globally.
The current implementation performs WebP detection for each extension instance. Consider caching the result globally to avoid redundant checks:
+let _globalWebPSupport: AssetPromise<boolean> | null = null; function checkWebpSupport(): AssetPromise<boolean> { + if (_globalWebPSupport) { + return _globalWebPSupport; + } + _globalWebPSupport = new AssetPromise((resolve) => { - return new AssetPromise((resolve) => { // existing implementation }); + return _globalWebPSupport; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/loader/src/gltf/extensions/EXT_texture_webp.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/loader/src/gltf/extensions/EXT_texture_webp.ts (3)
packages/core/src/asset/AssetPromise.ts (2)
AssetPromise(4-234)resolve(25-35)packages/core/src/SystemInfo.ts (1)
SystemInfo(9-126)packages/loader/src/gltf/parser/GLTFParser.ts (1)
registerGLTFExtension(137-143)
🔇 Additional comments (2)
packages/loader/src/gltf/extensions/EXT_texture_webp.ts (2)
11-29: Excellent implementation of reliable WebP detection!The new
checkWebpSupport()function correctly addresses the Safari WebP detection issue by loading a minimal WebP image instead of relying on canvas methods. The implementation properly:
- Uses
AssetPromiseas required by the engine architecture- Handles both browser and non-browser environments
- Uses a minimal 1-pixel WebP image for efficient testing
- Properly resolves
falseon error or in non-browser environmentsThis approach is much more reliable than canvas-based detection for Safari compatibility.
33-33: Property type change correctly supports asynchronous WebP detection.The
_supportWebPproperty now correctly usesAssetPromise<boolean>instead of a synchronous boolean, enabling proper asynchronous WebP support detection. This aligns well with the engine's promise-based architecture.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/loader/src/gltf/extensions/EXT_texture_webp.ts (1)
23-24: Extract the base64 WebP string as a constant for better readability.The inline base64 string is very long and reduces code readability. Consider extracting it as a named constant.
Add a constant at the module level:
+// 1x1 transparent WebP image for feature detection +const WEBP_TEST_IMAGE = "data:image/webp;base64,UklGRhACAABXRUJQVlA4WAoAAAAwAAAAAAAAAAAASUNDUMgBAAAAAAHIAAAAAAQwAABtbnRyUkdCIFhZWiAH4AABAAEAAAAAAABhY3NwAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQAA9tYAAQAAAADTLQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAlkZXNjAAAA8AAAACRyWFlaAAABFAAAABRnWFlaAAABKAAAABRiWFlaAAABPAAAABR3dHB0AAABUAAAABRyVFJDAAABZAAAAChnVFJDAAABZAAAAChiVFJDAAABZAAAAChjcHJ0AAABjAAAADxtbHVjAAAAAAAAAAEAAAAMZW5VUwAAAAgAAAAcAHMAUgBHAEJYWVogAAAAAAAAb6IAADj1AAADkFhZWiAAAAAAAABimQAAt4UAABjaWFlaIAAAAAAAACSgAAAPhAAAts9YWVogAAAAAAAA9tYAAQAAAADTLXBhcmEAAAAAAAQAAAACZmYAAPKnAAANWQAAE9AAAApbAAAAAAAAAABtbHVjAAAAAAAAAAEAAAAMZW5VUwAAACAAAAAcAEcAbwBvAGcAbABlACAASQBuAGMALgAgADIAMAAxADZBTFBIAgAAAAAAVlA4IBgAAAAwAQCdASoBAAEAAUAmJaQAA3AA/v02aAA="; + function checkWebpSupport(): AssetPromise<boolean> {Then update the usage:
- img.src = - "data:image/webp;base64,UklGRhACAABXRUJQVlA4WAoAAAAwAAAAAAAAAAAASUNDUMgBAAAAAAHIAAAAAAQwAABtbnRyUkdCIFhZWiAH4AABAAEAAAAAAABhY3NwAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQAA9tYAAQAAAADTLQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAlkZXNjAAAA8AAAACRyWFlaAAABFAAAABRnWFlaAAABKAAAABRiWFlaAAABPAAAABR3dHB0AAABUAAAABRyVFJDAAABZAAAAChnVFJDAAABZAAAAChiVFJDAAABZAAAAChjcHJ0AAABjAAAADxtbHVjAAAAAAAAAAEAAAAMZW5VUwAAAAgAAAAcAHMAUgBHAEJYWVogAAAAAAAAb6IAADj1AAADkFhZWiAAAAAAAABimQAAt4UAABjaWFlaIAAAAAAAACSgAAAPhAAAts9YWVogAAAAAAAA9tYAAQAAAADTLXBhcmEAAAAAAAQAAAACZmYAAPKnAAANWQAAE9AAAApbAAAAAAAAAABtbHVjAAAAAAAAAAEAAAAMZW5VUwAAACAAAAAcAEcAbwBvAGcAbABlACAASQBuAGMALgAgADIAMAAxADZBTFBIAgAAAAAAVlA4IBgAAAAwAQCdASoBAAEAAUAmJaQAA3AA/v02aAA="; + img.src = WEBP_TEST_IMAGE;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/loader/src/gltf/extensions/EXT_texture_webp.ts(2 hunks)
🔇 Additional comments (1)
packages/loader/src/gltf/extensions/EXT_texture_webp.ts (1)
33-52: LGTM! The asynchronous WebP detection is correctly implemented.The
createAndParsemethod properly returns a promise chain and correctly handles the asynchronous WebP support check usingAssetPromiseas required by the engine guidelines.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/SystemInfo.ts(3 hunks)packages/loader/src/gltf/extensions/EXT_texture_webp.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loader/src/gltf/extensions/EXT_texture_webp.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/core/src/SystemInfo.ts (1)
packages/core/src/asset/AssetPromise.ts (1)
resolve(25-35)
🪛 Biome (1.9.4)
packages/core/src/SystemInfo.ts
[error] 91-91: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 92-92: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 110-110: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (2)
packages/core/src/SystemInfo.ts (2)
5-5: LGTM: Import added for AssetPromise.The import is correctly added to support the new asynchronous WebP detection functionality.
22-22: LGTM: Static property for caching WebP support result.The property correctly caches the AssetPromise to avoid redundant WebP support checks.
Safari 的 WebP 支持和 canvas.toDataURL('image/webp') 并非同时支持，故使用canvas.toDataURL('image/webp')判断webp支持不合适，改成加载一个1px的webp base64图片进行判断
Summary by CodeRabbit
New Features
Bug Fixes