Conversation
WalkthroughAdds optional HEVC selection to scene loading via a new Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Player as Player
participant Loader as SceneLoader
participant Multimedia as MultimediaUtils
participant Browser as Browser/CodecAPI
participant Network as Network
App->>Player: create(playerOptions)
Player->>Loader: loadScene(sceneData, { useHevcVideo: true })
Loader->>Multimedia: processMultimedia(mediaItems, options)
Multimedia->>Browser: canPlayHevcCodec(codec)
Browser-->>Multimedia: supported? (true/false)
alt HEVC supported & enabled
Multimedia->>Multimedia: getVideoUrl() → select HEVC source
else fallback
Multimedia->>Multimedia: getVideoUrl() → select base/source URL
end
Multimedia->>Network: fetch(videoUrl)
Network-->>Multimedia: video data
Multimedia-->>Loader: resolved media (URLs/buffers)
Loader-->>Player: scene assets ready
Player-->>App: playback starts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ 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). (1)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/effects-core/src/asset-manager.ts (1)
204-450: Fix early-return inprocessVideoURLto process all videos, not just the first valid one.The integration point and overall flow look good (running codec selection before bins/images and timing it via
hookTimeInfo), but there is a critical logic bug:Early exit prevents processing remaining videos: The function contains a
returnstatement inside the per-video loop:for (const video of jsonScene.videos) { const hevc = video.hevc as { url?: string, codec?: string } | undefined; // @ts-expect-error if (!hevc?.url || !hevc?.codec || !spec.HevcVideoCodec) {return;} const codec = this.stringToHevcVideoCodec(hevc.codec); if (codec && this.canPlayHevcCodec(codec)) { video.url = hevc.url; } }As soon as any video is missing
hevc.url/hevc.codec, the function exits entirely and no subsequent videos are processed. This silently skips remaining videos without error.Change
returntocontinue:for (const video of jsonScene.videos) { const hevc = video.hevc as { url?: string, codec?: string } | undefined; // @ts-expect-error - if (!hevc?.url || !hevc?.codec || !spec.HevcVideoCodec) {return;} + if (!hevc?.url || !hevc?.codec || !spec.HevcVideoCodec) {continue;} const codec = this.stringToHevcVideoCodec(hevc.codec); if (codec && this.canPlayHevcCodec(codec)) { video.url = hevc.url; } }This preserves the "skip when
spec.HevcVideoCodecis unavailable" behavior while still processing all valid videos.Secondary notes: The
jsonScene: anyparameter could be narrowed tospec.JSONSceneonceHevcVideoCodecis officially available in the spec, allowing you to remove the@ts-expect-errorcomments. Additionally,canPlayHevcCodeccreates a new video element per call; if processing many videos, consider memoizing results by codec.
🧹 Nitpick comments (1)
plugin-packages/multimedia/demo/src/hevc-video.ts (1)
223-246: Harden demo against real-page issues (HTTP video URL and container null).
- The HEVC and fallback URLs are plain
http://. If the demo is ever served over HTTPS, this will be blocked as mixed content and the video won’t load. If an HTTPS endpoint exists, it’s safer to switch to it here.const container = document.getElementById('J-container');may benullif the element is missing or the script runs before DOMReady. You might want to guard and log a clear error, or assert non-null with a runtime check to avoid a less-obvious failure insidePlayer.These are demo-only, so feel free to treat them as polish rather than blockers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/effects-core/src/asset-manager.ts(2 hunks)plugin-packages/multimedia/demo/src/hevc-video.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: yiiqii
Repo: galacean/effects-runtime PR: 214
File: packages/effects/src/player.ts:416-429
Timestamp: 2024-10-17T07:15:16.523Z
Learning: Data compatibility or processing logic related to replacing text variables in scene compositions should be handled in the AssetManager class through the updateOptions method, not in the Player class.
⏰ 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). (1)
- GitHub Check: Analyze (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/effects-core/src/asset-manager.ts (1)
409-409: Fix parameter type to avoidany.Based on past review feedback and coding guidelines, the parameter should be typed as
spec.JSONSceneinstead ofanyfor better type safety.Apply this diff:
- private async processVideoURL (jsonScene: any): Promise<void> { + private async processVideoURL (jsonScene: spec.JSONScene): Promise<void> {
🧹 Nitpick comments (2)
packages/effects-core/src/asset-manager.ts (2)
204-206: Consider parallelizing video processing with other asset loading.Based on past review feedback, the video codec selection could run concurrently with bins/images/fonts loading in the
Promise.allbelow, reducing overall load time. As per coding guidelines, processVideoURL doesn't appear to have dependencies on the other loading tasks.Apply this diff to parallelize the loading:
- if (this.options.useHevcVideo) { - await hookTimeInfo('selectVideoCodec', () => this.processVideoURL(jsonScene)); - } const [loadedBins, loadedImages] = await Promise.all([ hookTimeInfo('processBins', () => this.processBins(bins)), hookTimeInfo('processImages', () => this.processImages(images, isKTX2Supported)), hookTimeInfo('processFontURL', () => this.processFontURL(fonts as spec.FontDefine[])), + this.options.useHevcVideo + ? hookTimeInfo('selectVideoCodec', () => this.processVideoURL(jsonScene)) + : Promise.resolve(), ]);
425-444: Review the necessity of thestringToHevcVideoCodecmethod and its type handling.This private method has 9
@ts-expect-errorsuppressions across 20 lines of code, indicating significant type incompatibility. Additionally, the method appears to be unused—no calls to it are found in the codebase. Consider whether this method is needed at all. If it is required for future use, the type assertions should be properly addressed by:
- Verifying that
spec.HevcVideoCodecis correctly imported and typed in the specification package- Replacing type suppressions with proper type guards or factory functions
- If the enum definition doesn't support these operations, consider restructuring the validation logic
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/effects-core/src/asset-manager.ts(2 hunks)packages/effects-core/src/scene.ts(1 hunks)web-packages/demo/src/single.ts(1 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). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
packages/effects-core/src/scene.ts (1)
89-93: LGTM! Clean API addition.The new
useHevcVideofield follows the established pattern for optional feature flags and is properly documented.web-packages/demo/src/single.ts (1)
4-245: LGTM! Effective demo setup.The demo properly showcases the HEVC video feature with an embedded scene JSON containing HEVC configuration and the
useHevcVideo: trueoption.packages/effects-core/src/asset-manager.ts (1)
445-452: Implementation is correct, but type suppressions indicate upstream issue.The
canPlayHevcCodecmethod correctly uses thecanPlayTypeAPI. However, the@ts-expect-errorsuppressions suggest the same type definition issue as noted instringToHevcVideoCodec. This should be resolved by addressing thespec.HevcVideoCodectype availability.This will be resolved when the type issues flagged in the previous comment are addressed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/effects-core/src/asset-manager.ts (1)
407-422: Critical: loop usesreturn(exits early); should becontinueLine 414 exits the whole function on the first non-HEVC video, skipping subsequent entries.
for (const video of jsonScene.videos) { // @ts-expect-error const hevc = video.hevc as { url?: string, codec?: string } | undefined; - if (!hevc?.url || !hevc?.codec) {return;} + if (!hevc?.url || !hevc?.codec) { continue; } const codec = this.stringToHevcVideoCodec(hevc.codec); if (codec && this.canPlayHevcCodec(codec)) { video.url = hevc.url; } }
🧹 Nitpick comments (2)
packages/effects-core/src/asset-manager.ts (2)
423-442: Avoid@ts-expect-errorhere; strongly type the “hevc extension” onceRather than suppressing type checks repeatedly, introduce a narrow local type (e.g.
type VideoWithHevc = spec.VideoSource & { hevc?: { url?: string; codec?: string } }) and use it inprocessVideoURL/stringToHevcVideoCodec. This will also make it clearer what shape you expect fromjsonScene.videos.
443-450: Add memoization for repeated codec checksCalling
canPlayType()repeatedly for the same HEVC codec is unnecessary when processing multiple videos that may share codecs. Add a simple cache:+ private readonly hevcSupportCache = new Map<string, boolean>(); + private canPlayHevcCodec (codecString: spec.HevcVideoCodec): boolean { + const cached = this.hevcSupportCache.get(String(codecString)); + if (cached !== undefined) { return cached; } + const v = document.createElement('video'); const contentType = `video/mp4; codecs="${codecString}"`; const result = v.canPlayType(contentType); - - return result === 'probably' || result === 'maybe'; + const ok = result === 'probably' || result === 'maybe'; + this.hevcSupportCache.set(String(codecString), ok); + return ok; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/effects-core/src/asset-manager.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-10T07:59:18.725Z
Learnt from: ChengYi996
Repo: galacean/effects-runtime PR: 1263
File: packages/effects-core/src/texture/ktx2/transcoder/binomial-workercode.ts:99-110
Timestamp: 2025-11-10T07:59:18.725Z
Learning: In packages/effects-core/src/texture/ktx2/transcoder/binomial-workercode.ts, the KTX2TargetFormat.ETC case in getTranscodeFormatFromTarget already handles ETC1 through the hasAlpha flag (returns BasisFormat.ETC1 when hasAlpha is false, BasisFormat.ETC2 when true), so a separate ETC1 case is not needed.
Applied to files:
packages/effects-core/src/asset-manager.ts
⏰ 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). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
packages/effects-core/src/asset-manager.ts (1)
202-210:processVideoURLruns in parallel with bins/images; confirm this ordering is intendedYou’re mutating
jsonScene.videoswhileprocessBins/processImagesrun in the samePromise.all. If any downstream logic readsjsonScene.videosduring those steps (now or in future), you can get surprising ordering dependencies. Consider runningprocessVideoURLbefore thePromise.all(or at least add a brief comment that it’s safe to run concurrently).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/effects-core/src/fallback/particle.ts (1)
146-156: Consider properly typing textureSheetAnimation instead of suppressing errors.The
@ts-expect-errordirective suppresses type checking for thetextureSheetAnimationassignment. While this allows the code to compile, it bypasses TypeScript's type safety. Consider either updating the type definitions in@galacean/effects-specificationto match the actual structure, or creating a proper type assertion.Based on learnings, ensure this isn't temporary debugging code that should be properly implemented.
packages/effects-core/src/fallback/sprite.ts (1)
96-103: Consider properly typing textureSheetAnimation instead of suppressing errors.Similar to the change in
particle.ts, this@ts-expect-errordirective bypasses type checking. Consider updating type definitions or using proper type assertions to maintain type safety.packages/effects-core/src/utils/hevc-video.ts (1)
8-14: Consider caching the video element or results.The function creates a new video element on each call. If this function is called multiple times with the same codec, consider caching either the video element or the results to improve performance.
+const codecCache = new Map<spec.HevcVideoCodec, boolean>(); + export function canPlayHevcCodec (codec: spec.HevcVideoCodec): boolean { + if (codecCache.has(codec)) { + return codecCache.get(codec)!; + } + const video = document.createElement('video'); const contentType = `video/mp4; codecs="${codec}"`; const result = video.canPlayType(contentType); - - return result === 'probably' || result === 'maybe'; + + const canPlay = result === 'probably' || result === 'maybe'; + codecCache.set(codec, canPlay); + return canPlay; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
packages/effects-core/package.json(1 hunks)packages/effects-core/src/asset-manager.ts(3 hunks)packages/effects-core/src/fallback/particle.ts(1 hunks)packages/effects-core/src/fallback/sprite.ts(1 hunks)packages/effects-core/src/plugins/sprite/sprite-item.ts(2 hunks)packages/effects-core/src/utils/hevc-video.ts(1 hunks)packages/effects-core/src/utils/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-05T07:51:18.728Z
Learnt from: ChengYi996
Repo: galacean/effects-runtime PR: 1118
File: packages/effects-core/src/components/ffd-component.ts:162-164
Timestamp: 2025-08-05T07:51:18.728Z
Learning: In packages/effects-core/src/components/ffd-component.ts, the //ts-expect-error usage for accessing siblingComponent.splits (around lines 162-164) is temporary debugging code that will be properly implemented later, as confirmed by ChengYi996.
Applied to files:
packages/effects-core/src/fallback/sprite.tspackages/effects-core/src/plugins/sprite/sprite-item.tspackages/effects-core/src/fallback/particle.ts
📚 Learning: 2025-12-09T08:44:15.153Z
Learnt from: Fryt1
Repo: galacean/effects-runtime PR: 1305
File: packages/effects-core/src/fallback/migration.ts:325-330
Timestamp: 2025-12-09T08:44:15.153Z
Learning: In `packages/effects-core/src/fallback/migration.ts`, the `textColor` 0-255 to 0-1 conversion in `version35Migration` is intentionally applied only to `TextComponent` and not to `RichTextComponent`, as confirmed by Fryt1.
Applied to files:
packages/effects-core/src/fallback/sprite.tspackages/effects-core/src/fallback/particle.ts
📚 Learning: 2025-08-05T07:50:26.317Z
Learnt from: ChengYi996
Repo: galacean/effects-runtime PR: 1118
File: packages/effects-core/src/components/ffd-component.ts:135-219
Timestamp: 2025-08-05T07:50:26.317Z
Learning: In packages/effects-core/src/components/ffd-component.ts, the mesh subdivision logic (lines 147-219) within the collectSpriteComponents method is temporary debug code that will be properly implemented later, as confirmed by ChengYi996.
Applied to files:
packages/effects-core/src/plugins/sprite/sprite-item.ts
📚 Learning: 2024-10-18T09:15:08.038Z
Learnt from: wumaolinmaoan
Repo: galacean/effects-runtime PR: 691
File: packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts:39-42
Timestamp: 2024-10-18T09:15:08.038Z
Learning: In `packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts`, when accumulating color components, avoid using `clone()` methods because it can cause additional garbage collection overhead.
Applied to files:
packages/effects-core/src/plugins/sprite/sprite-item.tspackages/effects-core/src/fallback/particle.ts
📚 Learning: 2025-11-10T07:59:18.725Z
Learnt from: ChengYi996
Repo: galacean/effects-runtime PR: 1263
File: packages/effects-core/src/texture/ktx2/transcoder/binomial-workercode.ts:99-110
Timestamp: 2025-11-10T07:59:18.725Z
Learning: In packages/effects-core/src/texture/ktx2/transcoder/binomial-workercode.ts, the KTX2TargetFormat.ETC case in getTranscodeFormatFromTarget already handles ETC1 through the hasAlpha flag (returns BasisFormat.ETC1 when hasAlpha is false, BasisFormat.ETC2 when true), so a separate ETC1 case is not needed.
Applied to files:
packages/effects-core/src/asset-manager.ts
🧬 Code graph analysis (1)
packages/effects-core/src/asset-manager.ts (1)
packages/effects-core/src/utils/hevc-video.ts (2)
parseCodec(21-33)canPlayHevcCodec(8-14)
⏰ 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). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
packages/effects-core/src/plugins/sprite/sprite-item.ts (1)
224-225: LGTM!The formatting changes improve code clarity without altering functionality.
Also applies to: 242-243
packages/effects-core/src/utils/index.ts (1)
10-10: LGTM!The export follows the established pattern and properly exposes the new HEVC utilities.
packages/effects-core/src/asset-manager.ts (3)
11-11: LGTM!The imports are correctly added to support HEVC video processing.
208-208: LGTM!The HEVC processing step is correctly integrated into the parallel loading pipeline with proper conditional execution when
useHevcVideois enabled. This addresses previous feedback about running in parallel with other loading steps.
407-425: LGTM!The
processVideoURLmethod correctly implements HEVC video URL processing. Previous review concerns have been addressed:
- Type annotation uses
spec.JSONSceneinstead ofany- Uses
forEachwithreturn(correct for iteration skip)- Removes meaningless enum existence check
- Properly validates
hevc.urlandhevc.codecbefore processingpackages/effects-core/src/utils/hevc-video.ts (2)
21-33: LGTM!The
parseCodecfunction correctly handles both enum value and enum key inputs, providing flexible parsing with appropriate fallback toundefinedfor invalid inputs.
35-45: LGTM!The helper functions correctly validate codec strings against the enum definition using appropriate TypeScript type guards.
packages/effects-core/package.json (1)
54-54: Version2.7.0of@galacean/effects-specificationis published on npm and no security advisories were found.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin-packages/multimedia/src/utils.ts (1)
32-32: Error message references wrong variable.The error message uses
media(the entire array) instead ofmedium(the specific item that failed therenderLevelcheck). This makes debugging harder.- throw new Error(`Invalid ${type} source: ${JSON.stringify(media)}.`); + throw new Error(`Invalid ${type} source: ${JSON.stringify(medium)}.`);
🧹 Nitpick comments (2)
plugin-packages/multimedia/src/utils.ts (2)
82-86: Dead code branch due to type signature.The
urlparameter is typed asstring, making theelsebranch unreachable. Either remove the dead code or update the type signature to acceptMediaProviderif that's a valid use case (similar toloadVideoindownloader.ts).- // 设置音频源 - if (typeof url === 'string') { - audio.src = url; - } else { - audio.srcObject = url; - } + audio.src = url;
166-172: Consider usingObject.values()for cleaner code.The current implementation iterates over keys to check values. Using
Object.values()would be more direct and readable.function isCodecValue (codec: string): codec is spec.HevcVideoCodec { - return Object - .keys(spec.HevcVideoCodec) - .some(key => - spec.HevcVideoCodec[key as keyof typeof spec.HevcVideoCodec] as string === codec - ); + return Object.values(spec.HevcVideoCodec).includes(codec as spec.HevcVideoCodec); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugin-packages/multimedia/src/utils.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugin-packages/multimedia/src/utils.ts (2)
packages/effects-core/src/downloader.ts (1)
loadVideo(271-303)packages/effects-core/src/scene.ts (1)
SceneLoadOptions(62-146)
⏰ 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). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
plugin-packages/multimedia/src/utils.ts (4)
108-127: LGTM!The custom error class follows best practices: sets
nameproperly and usesError.captureStackTracewhen available with appropriate feature detection.
134-140: LGTM!Correctly uses
canPlayTypeAPI to detect HEVC codec support.
147-159: LGTM!The function correctly handles both enum value strings and enum key names, providing flexibility for consumers.
189-203: LGTM!The function follows a clear fallback strategy: only use HEVC URL when explicitly enabled, HEVC config is present, and the browser supports the codec. Otherwise, gracefully falls back to the standard URL.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
plugin-packages/multimedia/src/utils.ts (2)
134-140: Consider caching or memoizing HEVC codec detection.The function creates a new video element on each call. If
canPlayHevcCodecis invoked multiple times with the same codec during scene loading, consider caching the results or reusing a single video element to improve performance.Example caching approach:
+const codecCache = new Map<spec.HevcVideoCodec, boolean>(); + export function canPlayHevcCodec (codec: spec.HevcVideoCodec): boolean { + if (codecCache.has(codec)) { + return codecCache.get(codec)!; + } const video = document.createElement('video'); const contentType = `video/mp4; codecs="${codec}"`; const result = video.canPlayType(contentType); - - return result === 'probably' || result === 'maybe'; + const canPlay = result === 'probably' || result === 'maybe'; + codecCache.set(codec, canPlay); + return canPlay; }
166-172: Simplify with Object.values() for better readability.The current implementation iterates through enum keys to check values. Using
Object.values()is more direct and readable.Apply this diff:
function isCodecValue (codec: string): codec is spec.HevcVideoCodec { - return Object - .keys(spec.HevcVideoCodec) - .some(key => - spec.HevcVideoCodec[key as keyof typeof spec.HevcVideoCodec] as string === codec - ); + return Object.values(spec.HevcVideoCodec).includes(codec as spec.HevcVideoCodec); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
packages/effects-core/package.json(1 hunks)plugin-packages/multimedia/src/utils.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/effects-core/package.json
🧰 Additional context used
🧬 Code graph analysis (1)
plugin-packages/multimedia/src/utils.ts (2)
packages/effects-core/src/downloader.ts (1)
loadVideo(271-303)packages/effects-core/src/scene.ts (1)
SceneLoadOptions(62-146)
⏰ 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). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
plugin-packages/multimedia/src/utils.ts (5)
12-36: LGTM! URL resolution logic is sound.The addition of HEVC-aware video URL selection via
getVideoUrland the conversion to absolute URLs usingnew URL()properly handles both relative and absolute URL inputs.
147-159: LGTM! Codec parsing logic handles both enum values and keys correctly.The function appropriately checks for enum values first, then enum keys, with a clear undefined fallback for invalid inputs.
179-181: LGTM! Clean and efficient enum key check.The use of the
inoperator is the appropriate approach for checking enum key membership.
189-203: LGTM! HEVC fallback logic is well-structured.The function correctly validates all preconditions (feature flag, HEVC data presence, codec validity, browser support) before selecting the HEVC URL, with appropriate fallback to the base URL.
134-203: Verify HEVC types are defined in @galacean/effects-specification@2.7.1.The new HEVC functionality depends on
spec.HevcVideoCodecenum andspec.VideoInfo.hevcproperty. Ensure these types are properly exported from the updated@galacean/effects-specification@2.7.1package to avoid runtime errors.Run the following script to verify the types are available:
#!/bin/bash # Verify that HevcVideoCodec and VideoInfo.hevc are exported from the specification package # Check if HevcVideoCodec is exported rg -n "HevcVideoCodec" packages/effects-core/node_modules/@galacean/effects-specification/ --type-add 'types:*.d.ts' --type types -C3 # Check if VideoInfo has hevc property rg -n "interface VideoInfo" packages/effects-core/node_modules/@galacean/effects-specification/ --type-add 'types:*.d.ts' --type types -A10
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.