refactor(webcodecs): convert decoder to be stream based #827
Conversation
📝 WalkthroughWalkthroughRename ADB PTY types; convert scrcpy decoders and renderers to TransformStream-based pipelines; promote PauseController and PerformanceCounter to public exports; add VideoDecoderStream and increasingNow(); update public types, method signatures, and metric getters across decoders and renderers. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. 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: 5
🤖 Fix all issues with AI Agents
In @libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts:
- Line 211: The dispose() method can throw if called before the microtask that
initializes this.#controller completes; guard the call by checking that
this.#controller is defined before invoking terminate(). Update dispose() to do
something like: if (this.#controller) this.#controller.terminate(); (or
null-check and no-op) so calling dispose() immediately after construction is
safe without relying on initialization timing for the controller.
- Around line 142-152: The code in resume() can access frames[frames.length - 1]
when this.#pendingFrames is empty causing a spread of undefined; add a guard at
the start (e.g., const frames = this.#pendingFrames.slice(); if (frames.length
=== 0) return;) to bail out early, then proceed with the existing loop and final
enqueue; reference this.#pendingFrames, resume(), and this.#controller.enqueue
when making the change.
- Around line 50-55: The constructor defers setting private field #controller
via "await Promise.resolve()", which can leave #controller undefined if
dispose(), resume(), or #resumeInternal() are invoked synchronously after
construction; fix by ensuring #controller is assigned synchronously in the
constructor (remove the microtask await) or, alternatively, introduce a ready
promise (e.g., this.controllerReady) that resolves when controller is set and
await that promise at the start of dispose(), resume(), and #resumeInternal() to
guard access; update references in constructor, dispose(), resume(), and
#resumeInternal() accordingly.
In @libraries/scrcpy-decoder-webcodecs/src/video/time.ts:
- Around line 21-28: The increasingNow function violates strict monotonicity by
calling nextUp(now) when now <= prevValue; change the branch so it calls
nextUp(prevValue) instead, assign that result to prevValue, and return it;
ensure you still call performance.now() to get now, compare with prevValue, and
only call nextUp(prevValue) when now <= prevValue so the returned value is
strictly greater than the prior prevValue.
In @libraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts:
- Around line 71-84: The code calls this.#decoder.configure(this.#config!) after
resetting the decoder but uses a non-null assertion for the optional field
this.#config; add an explicit runtime guard before calling configure (check if
this.#config is falsy and throw a clear Error such as "Decoder not configured")
so decoding paths like the keyframe handler in video-decoder-stream.ts validate
configuration instead of relying on the protocol; keep the throw synchronous so
callers see a clear error and only call this.#decoder.configure when
this.#config is present.
🧹 Nitpick comments (1)
libraries/scrcpy-decoder-tinyh264/src/decoder.ts (1)
114-139: Consider logging or handling pipeline errors instead of silently swallowing them.The
.catch(noop)on line 138 silently discards any errors from the streaming pipeline. While thepipeTopromise is intentionally not awaited (fire-and-forget), silently swallowing errors could make debugging difficult if the pipeline fails unexpectedly.Based on learnings, configuration failures should propagate up rather than being recovered locally. Consider at minimum logging the error for diagnostic purposes.
🔎 Proposed improvement
void this.#pause.readable .pipeTo( new WritableStream({ write: async (packet) => { // ... existing logic }, }), ) - .catch(noop); + .catch((error) => { + // Pipeline errors are expected on dispose, but log unexpected errors + if (error && !(error instanceof TypeError && error.message.includes("terminated"))) { + console.error("TinyH264Decoder pipeline error:", error); + } + });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
libraries/adb/src/commands/subprocess/none/pty.tslibraries/adb/src/commands/subprocess/none/service.tslibraries/adb/src/commands/subprocess/pty.tslibraries/adb/src/commands/subprocess/shell/pty.tslibraries/adb/src/commands/subprocess/shell/service.tslibraries/scrcpy-decoder-tinyh264/src/decoder.tslibraries/scrcpy-decoder-tinyh264/src/types.tslibraries/scrcpy-decoder-tinyh264/src/utils/pause.tslibraries/scrcpy-decoder-tinyh264/src/utils/performance.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/av1.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/h264.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/h265.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/type.tslibraries/scrcpy-decoder-webcodecs/src/video/decoder.tslibraries/scrcpy-decoder-webcodecs/src/video/time.tslibraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.tslibraries/struct/src/utils.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-03-30T19:57:44.305Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 739
File: apps/cli/src/index.ts:163-167
Timestamp: 2025-03-30T19:57:44.305Z
Learning: The none protocol in ADB doesn't pass exit codes (always returns 0), unlike the shell protocol. The API is designed to make this distinction clear by using `exited` instead of `exit` with a code parameter for none protocol.
Applied to files:
libraries/adb/src/commands/subprocess/none/pty.tslibraries/adb/src/commands/subprocess/pty.tslibraries/adb/src/commands/subprocess/none/service.ts
📚 Learning: 2025-08-23T14:04:38.056Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 786
File: libraries/adb/src/commands/sync/sync.ts:154-157
Timestamp: 2025-08-23T14:04:38.056Z
Learning: Both ADB none protocol (`exec:`) and shell protocol (`shell:`) are executed by shell using `sh -c ...` on the Android side and require proper argument escaping. The two protocols only differ in how standard input/output are encoded for transmission, not in their execution model. The `escapeArg()` function should be used for both protocols to prevent command injection when paths contain special characters.
Applied to files:
libraries/adb/src/commands/subprocess/shell/service.tslibraries/adb/src/commands/subprocess/shell/pty.tslibraries/adb/src/commands/subprocess/none/service.ts
📚 Learning: 2025-08-24T03:50:43.091Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 782
File: libraries/scrcpy-decoder-tinyh264/src/decoder.ts:193-196
Timestamp: 2025-08-24T03:50:43.091Z
Learning: In TinyH264Decoder, configuration failures should propagate the actual error up the client pipeline rather than being recovered locally. The decoder is designed to handle mid-stream configuration switches when the video encoder restarts, and subsequent valid config packets will re-configure correctly regardless of current decoder state.
Applied to files:
libraries/scrcpy-decoder-tinyh264/src/utils/pause.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/h264.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/h265.tslibraries/scrcpy-decoder-webcodecs/src/video/decoder.tslibraries/scrcpy-decoder-tinyh264/src/decoder.ts
📚 Learning: 2025-08-14T11:06:21.851Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 780
File: libraries/adb-credential-web/src/storage/index.ts:1-5
Timestamp: 2025-08-14T11:06:21.851Z
Learning: The yume-chan/adb-credential-web package doesn't use "exports" in package.json for better compatibility. Instead, they re-export everything from the main entrypoint rather than using subpath exports.
Applied to files:
libraries/adb/src/commands/subprocess/none/service.ts
🧬 Code graph analysis (13)
libraries/scrcpy-decoder-tinyh264/src/utils/performance.ts (1)
libraries/scrcpy-decoder-tinyh264/src/types.ts (1)
ScrcpyVideoDecoderPerformanceCounter(15-36)
libraries/adb/src/commands/subprocess/none/pty.ts (1)
libraries/adb/src/commands/subprocess/pty.ts (1)
AdbPty(8-15)
libraries/adb/src/commands/subprocess/shell/service.ts (1)
libraries/adb/src/commands/subprocess/shell/pty.ts (1)
AdbShellProtocolPty(20-112)
libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (4)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (2)
Input(13-15)Output(22-22)libraries/scrcpy-decoder-tinyh264/src/types.ts (1)
ScrcpyVideoDecoderPauseController(38-63)libraries/adb-scrcpy/src/client.ts (1)
controller(285-287)
libraries/scrcpy-decoder-webcodecs/src/video/codec/h264.ts (2)
libraries/adb-scrcpy/src/video.ts (3)
data(76-79)data(81-84)data(86-100)libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (2)
CodecDecoder(7-10)Config(17-20)
libraries/adb/src/commands/subprocess/shell/pty.ts (1)
libraries/adb/src/commands/subprocess/pty.ts (1)
AdbPty(8-15)
libraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.ts (3)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (4)
CodecDecoder(7-10)Input(13-15)Output(22-22)Config(17-20)libraries/stream-extra/src/concat.ts (1)
concatUint8Arrays(120-136)
libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (2)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (2)
Input(221-221)Output(222-224)
libraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts (2)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/event/src/event-emitter.ts (1)
EventEmitter(12-61)
libraries/scrcpy-decoder-webcodecs/src/video/codec/h265.ts (1)
libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (2)
CodecDecoder(7-10)Config(17-20)
libraries/scrcpy-decoder-webcodecs/src/video/codec/av1.ts (3)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (3)
CodecDecoder(7-10)Input(13-15)Output(22-22)libraries/media-codec/src/av1.ts (1)
Av1(197-762)
libraries/adb/src/commands/subprocess/none/service.ts (1)
libraries/adb/src/commands/subprocess/none/pty.ts (1)
AdbNoneProtocolPty(12-45)
libraries/scrcpy-decoder-tinyh264/src/decoder.ts (2)
libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (1)
PauseController(12-218)libraries/scrcpy-decoder-tinyh264/src/utils/performance.ts (1)
PerformanceCounter(3-64)
🪛 Biome (2.1.2)
libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts
[error] 296-296: Unexpected constant condition.
(lint/correctness/noConstantCondition)
⏰ 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). (2)
- GitHub Check: diffray code review
- GitHub Check: build (20.x)
🔇 Additional comments (40)
libraries/struct/src/utils.ts (2)
11-14: LGTM! Adding standard TextEncoder method.The
encodeIntomethod is part of the standard TextEncoder API and is more efficient thanencode()for performance-critical paths where you can reuse buffers. The signature correctly matches the specification.
32-32: LGTM! Exposing shared encoder instance.Making
SharedEncoderpublic is a reasonable optimization that allows consuming code to reuse the singleton instance. SinceTextEncoderis stateless, sharing the instance is safe.libraries/adb/src/commands/subprocess/shell/service.ts (3)
5-5: LGTM! Import properly updated.The import correctly references the renamed
AdbShellProtocolPtyclass.
42-42: LGTM! Return type properly updated.The return type correctly reflects the renamed class.
62-62: LGTM! Instantiation properly updated.The instantiation correctly uses the renamed
AdbShellProtocolPtyclass.libraries/adb/src/commands/subprocess/none/pty.ts (2)
10-10: LGTM! Import properly updated.The import correctly references the renamed
AdbPtyinterface.
12-45: LGTM! Class rename with no behavioral changes.The class rename from
AdbNoneProtocolPtyProcesstoAdbNoneProtocolPtyis cleaner and more consistent. The implementation remains unchanged, maintaining all functionality.libraries/adb/src/commands/subprocess/shell/pty.ts (2)
16-16: LGTM! Import properly updated.The import correctly references the renamed
AdbPtyinterface.
20-112: LGTM! Class rename with no behavioral changes.The class rename from
AdbShellProtocolPtyProcesstoAdbShellProtocolPtyis cleaner and more consistent. The implementation remains unchanged, maintaining all functionality including exit code handling, stdout streaming, and terminal control.libraries/adb/src/commands/subprocess/none/service.ts (3)
4-4: LGTM! Import properly updated.The import correctly references the renamed
AdbNoneProtocolPtyclass.
45-45: LGTM! Return type properly updated.The return type correctly reflects the renamed class.
55-55: LGTM! Instantiation properly updated.The instantiation correctly uses the renamed
AdbNoneProtocolPtyclass.libraries/adb/src/commands/subprocess/pty.ts (1)
8-15: LGTM! Clean interface rename.The interface rename from
AdbPtyProcesstoAdbPtyis cleaner and more concise. All references across the codebase have been properly updated, including the implementing classes (AdbNoneProtocolPtyandAdbShellProtocolPty) and all imports. The interface contract remains unchanged with no breaking modifications.libraries/scrcpy-decoder-tinyh264/src/types.ts (2)
15-36: LGTM!The renamed properties (
framesRendered,framesPresented,framesSkippedRendering) provide clearer semantics, and the documentation accurately explains the distinction between rendered and presented frames, including the Chrome bug caveat.
52-52: Good flexibility withMaybePromiseLike.Changing the return type from
Promise<undefined>toMaybePromiseLike<undefined>allows implementations to return synchronously when no async work is needed, which is a good performance-conscious design choice.libraries/scrcpy-decoder-tinyh264/src/utils/performance.ts (1)
3-17: LGTM!The class correctly implements
ScrcpyVideoDecoderPerformanceCounterwith the renamed getters. Keeping the internal field names (#framesDrawn,#framesSkipped) while renaming only the public API is a reasonable choice that minimizes churn.libraries/scrcpy-decoder-tinyh264/src/decoder.ts (3)
44-51: LGTM!Clean delegation to
PauseControllerfor both thepausedstate andwritablestream. This aligns well with the stream-based architecture.
64-90: LGTM!The performance counter getters are properly delegated with clear documentation that matches the interface definitions.
223-225: Verify return type matches interface.The
resume()method now returnsundefinedsynchronously, while the interfaceScrcpyVideoDecoderPauseController.resume()specifiesMaybePromiseLike<undefined>. This is compatible sinceundefinedsatisfiesMaybePromiseLike<undefined>, but ensure this is intentional given the previous async behavior.libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (1)
12-15: Good design: TransformStream-based pause controller with typed namespace.The
PauseControllerextendingTransformStreamis a clean streaming-based design. The namespace exports provide well-typedInputandOutputtypes that make the data flow explicit.Also applies to: 220-225
libraries/scrcpy-decoder-webcodecs/src/video/time.ts (1)
3-17: LGTM!The
nextUpimplementation correctly handles special cases and uses bit manipulation to compute the next representable float64 value.libraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts (3)
48-49: Clarify the async Promise.resolve pattern.The
await Promise.resolve()pattern is unusual. If this is intentional to defer initialization to the next microtask, consider adding a comment explaining why.
64-69: LGTM!Configuration handling is correct - stores the config for potential decoder resets and immediately configures the decoder.
86-89: LGTM!Flush handling correctly waits for pending decodes before closing the decoder.
libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts (5)
195-259: LGTM! Streaming pipeline is well-structured.The pipeline composition correctly chains pause control, timestamp annotation, codec decoding, configuration inspection, frame decoding, and rendering. The use of
increasingNow()for timestamps enables both decode time tracking and frame skipping logic.Note: Line 259 uses
catch(noop)which silently suppresses errors. This appears intentional for cleanup scenarios, but ensure critical errors are logged elsewhere if needed.
262-299: LGTM! The do-while loop is correct.The
while (true)at line 296 is used correctly in a do-while loop with an explicit break condition at line 294. This pattern ensures all queued frames are rendered in sequence, minimizing latency. The static analysis warning is a false positive.The frame queuing strategy (lines 264-271) correctly handles backpressure by dropping intermediate frames when the renderer can't keep up.
347-357: LGTM! Options interface is appropriately simplified.The interface correctly exposes only the relevant decoder configuration options while requiring the essential codec and renderer parameters.
320-326: LGTM!Pause/resume methods correctly delegate to the PauseController.
179-193: LGTM!Codec selection logic is clear and handles unsupported codecs appropriately.
libraries/scrcpy-decoder-webcodecs/src/video/codec/h264.ts (1)
7-15: LGTM! Configuration method aligns with streaming architecture.The
configuremethod correctly returns aCodecDecoder.Configobject instead of performing side effects, which aligns with the new stream-based decoder architecture.libraries/scrcpy-decoder-webcodecs/src/video/codec/h265.ts (1)
7-18: LGTM! Configuration method matches H264 pattern.The
configuremethod correctly returns aCodecDecoder.Configobject. The comment about Microsoft Edge's explicit size requirement (lines 12-14) provides valuable context for the implementation.libraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.ts (3)
5-8: Clean stream-based refactor with proper Annex B handling.The class now properly extends
TransformStreamwith the appropriate input/output types. The design cleanly separates configuration from data processing.
11-42: Transform logic is correct for H.264/H.265 Annex B format.The implementation correctly:
- Stores configuration and emits the parsed config on configuration packets
- Concatenates configuration with the first frame data (per WebCodecs AVC/HEVC Annex B requirements)
- Clears the stored config after use to avoid re-concatenation
- Treats
undefinedkeyframe askey(necessary for proper decoding)The reference to the W3C WebCodecs spec in the comment is helpful for maintainability.
44-44: Abstract method signature aligns with the streaming design.The
configuremethod now returnsCodecDecoder.Configinstead ofvoid, enabling the transformer to emit configuration downstream. This is consistent with theOutputtype definition.libraries/scrcpy-decoder-webcodecs/src/video/codec/av1.ts (3)
6-9: Consistent streaming pattern with H26x decoder.The class properly extends
TransformStreamwith the same input/output types, maintaining consistency with the other codec implementations.
18-30: Config emission depends on sequence header presence in each packet.The implementation correctly parses for sequence headers on every data packet and emits config when found. For AV1, sequence headers can appear in any keyframe, so this on-demand config emission is appropriate.
One consideration: if the downstream consumer requires config before processing chunks, ensure it handles the case where the first few packets might not contain a sequence header. Based on the
Outputtype beingConfig | EncodedVideoChunk, downstream should be prepared to handle either.
32-39: EncodedVideoChunk creation is correct.The keyframe handling (
packet.keyframe === false ? "delta" : "key") is consistent with the H26x implementation and the comment explains the rationale clearly.libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (3)
7-10: Clean type alias design for streaming decoders.The
CodecDecodertype alias cleanly defines the streaming contract. Using a namespace for associated types (Input,Config,Output) is a good TypeScript pattern for grouping related types.
25-27: Simplified constructor signature.The parameterless constructor is cleaner and aligns with the new streaming architecture where decoders are self-contained TransformStreams.
12-22: Well-structured type definitions.The types are well-designed:
Inputcorrectly unions configuration and timestamped data packetsConfigextendsVideoDecoderConfigwith required dimensions (needed for sizing)Outputunion enables emitting both config and encoded chunksThe tsconfig includes
"DOM"lib, which provides the WebCodecs API types (VideoDecoderConfigandEncodedVideoChunk) as globals, so they are properly available.
diffray
bot
added
diffray-review-failed
diffray
bot
added
diffray-review-started
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (1)
120-167: Non-null assertions unsafe due to constructor race condition.Lines 142 and 157 use non-null assertions (
this.#controller!) which are unsafe given the asynchronous controller initialization in the constructor. These assertions will throw if#resumeInternalis called before the microtask completes.The iteration logic using
entries()correctly handles empty arrays and properly assigns theskipRenderingflag.
🤖 Fix all issues with AI Agents
In @libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts:
- Around line 52-63: The constructor's start handler defers assigning the
private field #controller via await Promise.resolve(), causing a race where
dispose(), resume(), or other methods may access an undefined #controller;
remove the unnecessary await Promise.resolve() so the assignment
this.#controller = controller happens synchronously in the start callback, and
also add a defensive guard (e.g., if (!this.#controller) return or no-op) in
dispose() and resume() to avoid non-null assertion failures when they are called
before controller is set.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.vscode/settings.jsonlibraries/scrcpy-decoder-tinyh264/src/utils/pause.tslibraries/scrcpy-decoder-webcodecs/src/video/time.tslibraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts
✅ Files skipped from review due to trivial changes (1)
- .vscode/settings.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-24T03:50:43.091Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 782
File: libraries/scrcpy-decoder-tinyh264/src/decoder.ts:193-196
Timestamp: 2025-08-24T03:50:43.091Z
Learning: In TinyH264Decoder, configuration failures should propagate the actual error up the client pipeline rather than being recovered locally. The decoder is designed to handle mid-stream configuration switches when the video encoder restarts, and subsequent valid config packets will re-configure correctly regardless of current decoder state.
Applied to files:
libraries/scrcpy-decoder-tinyh264/src/utils/pause.tslibraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts
🧬 Code graph analysis (2)
libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (3)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (2)
Input(13-15)Output(22-22)libraries/scrcpy-decoder-tinyh264/src/types.ts (1)
ScrcpyVideoDecoderPauseController(38-63)
libraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts (3)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/event/src/event-emitter.ts (1)
EventEmitter(12-61)libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts (1)
frame(262-299)
⏰ 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). (2)
- GitHub Check: build (20.x)
- GitHub Check: diffray code review
🔇 Additional comments (9)
libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (4)
64-103: Transform logic correctly handles pause/resume flow.The transform callback properly manages paused state by queuing configuration and data packets, and correctly awaits any in-progress resume operation before enqueueing new packets. The use of the
controllerparameter (notthis.#controller) avoids race condition issues.
169-171: Resume signature correctly implements interface.The return type change from
MaybePromiseLike<undefined>toundefinedis valid, asundefinedsatisfies the interface requirement forMaybePromiseLike<undefined>.
213-221: Good defensive programming with optional chaining.Line 220 correctly uses optional chaining (
this.#controller?.terminate()) to handle the case where#controllermight be undefined. This resolves the past crash issue, though the root cause (constructor race condition) should still be addressed.
229-234: Clean public API definition.The namespace exports provide a clear and well-structured public API for the PauseController, with
InputandOutputtypes that accurately reflect the streaming transformation.libraries/scrcpy-decoder-webcodecs/src/video/time.ts (2)
1-17: Correct IEEE-754 implementation for next representable value.The
nextUpfunction correctly implements IEEE-754 bit manipulation to compute the next representable double-precision value. Special cases (NaN, Infinity, zero) are properly handled, and the bit arithmetic correctly increments for positive values and decrements for negative values.
21-28: Monotonicity correctly implemented.The function now correctly calls
nextUp(prevValue)(line 24) instead ofnextUp(now)whennow <= prevValue, ensuring strict monotonicity as required. The past review issue has been resolved.libraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts (3)
46-63: Decoder initialization correctly structured.The constructor properly initializes the VideoDecoder within the TransformStream's
startcallback, ensuring the decoder is ready before any transform operations. The output callback correctly increments the frames counter and enqueues frames, while the error callback propagates errors appropriately.
64-73: Config handling with appropriate guards.Lines 65-68 correctly save the configuration and configure the decoder. The guard at lines 71-73 ensures the decoder is configured before processing chunks, with a clear error message. This addresses the protocol requirement that configuration must precede data chunks.
90-93: Proper flush and cleanup logic.The flush callback correctly awaits pending decode operations before closing the decoder, ensuring clean shutdown of the video pipeline.
diffray
bot
added
diffray-review-failed
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (1)
50-98: Excellent fix for the race condition!The closure pattern elegantly solves the previous race condition by capturing
controllersynchronously duringsuper()construction, then assigning it to#controllerimmediately after. This ensures#controlleris always initialized before any methods can access it.The transform logic correctly handles:
- Buffering configuration and frames when paused
- Clearing pending frames on new keyframes
- Blocking during resume with
await this.#resuming- Enqueuing with appropriate
skipRenderingflagsOptional: Add clarifying comment
Consider adding a brief comment explaining the closure pattern for future maintainers:
constructor() { + // Capture controller via closure to ensure synchronous initialization let controller!: TransformStreamDefaultController<PauseController.Output>;libraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts (1)
91-94: Consider adding cleanup for abnormal stream termination.While the flush callback properly closes the decoder, this only runs on normal stream completion. If the stream is cancelled or errors before flush completes, the decoder may remain open (though it will eventually be garbage collected).
For cleaner resource management, consider adding an abort signal or catch handler to explicitly close the decoder on abnormal termination.
💡 Optional enhancement for robust cleanup
You could capture abort/cancel by listening to the controller's abort signal or by adding error handling in the transform. However, given the current architecture and the fact that the decoder will be GC'd eventually, this is more of a nice-to-have than a must-fix.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libraries/scrcpy-decoder-tinyh264/src/utils/pause.tslibraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-24T03:50:43.091Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 782
File: libraries/scrcpy-decoder-tinyh264/src/decoder.ts:193-196
Timestamp: 2025-08-24T03:50:43.091Z
Learning: In TinyH264Decoder, configuration failures should propagate the actual error up the client pipeline rather than being recovered locally. The decoder is designed to handle mid-stream configuration switches when the video encoder restarts, and subsequent valid config packets will re-configure correctly regardless of current decoder state.
Applied to files:
libraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts
🧬 Code graph analysis (2)
libraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts (3)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/event/src/event-emitter.ts (1)
EventEmitter(12-61)libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts (1)
frame(262-299)
libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (4)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (2)
Input(13-15)Output(22-22)libraries/scrcpy-decoder-tinyh264/src/types.ts (1)
ScrcpyVideoDecoderPauseController(38-63)libraries/adb-scrcpy/src/client.ts (1)
controller(285-287)
⏰ 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: build (20.x)
🔇 Additional comments (10)
libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (3)
145-156: Resume logic correctly handles all cases.The iteration using
.entries()safely handles empty#pendingFramesarrays (the loop simply won't execute), and theskipRenderinglogic correctly marks all frames except the last one. This resolves the previous critical issue about accessing empty arrays.The comment at lines 145-147 accurately explains the synchronization guarantee.
164-165: Disposal and return type are correct.
- The
resume()return typeundefinedcorrectly satisfies theMaybePromiseLike<undefined>interface requirement.dispose()safely callsterminate()because#controlleris always initialized via the constructor's closure pattern, resolving the previous crash concern.Also applies to: 208-221
224-229: Clean type exports using namespace pattern.The namespace pattern cleanly associates
InputandOutputtypes with thePauseControllerclass, making the API surface clear and type-safe.libraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts (7)
1-7: LGTM! Clean stream-based architecture.The class properly extends
TransformStreamwith appropriate input/output types. The union input typeVideoDecoderConfig | EncodedVideoChunkelegantly handles both configuration and data packets in a single stream, aligning well with the PR's goal of converting to stream-based decoders.
8-44: Well-designed public API surface.The exposed metrics (
decodeQueueSize,framesDecoded,framesSkipped) andonDequeueevent provide excellent observability into decoder behavior, which is essential for monitoring performance and latency in streaming video applications.
46-64: Proper decoder initialization and event wiring.The initialization pattern correctly creates the
VideoDecoderin thestartcallback and wires up output/error handlers. The error propagation on line 58 ensures configuration failures and decode errors bubble up the pipeline as expected.
65-70: Config handling looks correct.The type guard on line 66 appropriately distinguishes
VideoDecoderConfigfromEncodedVideoChunk. Storing the config before callingconfigure()is essential for the keyframe reset logic later in the transform.
72-74: Excellent defensive coding.The explicit state check with a clear error message ensures that encoded chunks are only processed after configuration. This validation makes the non-null assertion on line 86 safe, as the decoder can only be configured through line 68, which always sets
#configfirst.
76-87: Smart latency management with justified non-null assertion.The keyframe-based frame dropping strategy effectively caps latency when the decoder can't keep up. The non-null assertion on line 86 is safe because the state check on line 72 guarantees the decoder was previously configured via line 68, which always sets
#configbefore callingconfigure().
89-89: LGTM!The decode call is straightforward and safe at this point, with proper error handling wired through the decoder's error callback.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts:
- Around line 129-141: The snapshot() method lacks an explicit return type;
update its signature to declare the correct async return type (Promise<Blob |
undefined>) so callers and the compiler know it returns either a Blob or
undefined, keeping the existing logic that borrows a VideoFrameCapturerPool,
calls capturer.capture(frame), and returns that result; ensure the method
signature on the snapshot() function is changed accordingly.
- Around line 102-106: The close handler currently only closes
this.#readableController and disposes this.#counter, risking leaked VideoFrame
objects; update the close function where the ReadableStream is created (the
close: () => { ... } block) to also check and close any pending VideoFrame
references by calling close() on this.#captureFrame and this.#nextFrame if they
are non-null and then nulling them out before disposing the counter and closing
the controller so all frames are released at shutdown.
In @libraries/scrcpy-decoder-webcodecs/src/video/render/type.ts:
- Around line 1-5: The interface uses the nonstandard property name `writeable`;
change `VideoFrameRenderer.writeable` to `writable` to match the standard stream
API and the `RendererController` usage in `flow-control.ts`, then update all
implementations and references to `VideoFrameRenderer` (including any
classes/functions that consume or construct that interface) to use `writable` so
names are consistent across the codebase.
In @libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts:
- Around line 14-60: createProgram may receive null from gl.createProgram() if
the WebGL context is lost; add an immediate null check after const program =
gl.createProgram() in createProgram and handle it before calling
gl.attachShader/linkProgram. Specifically, if program is null, delete the
vertexShader and fragmentShader (to avoid leaks) and throw a clear Error (e.g.,
"Failed to create program: WebGL context lost") so subsequent
gl.attachShader/gl.linkProgram calls never run on a null program.
In @libraries/scrcpy-decoder-webcodecs/src/video/snapshot.ts:
- Around line 13-16: The explicit cast "(this.#canvas as HTMLCanvasElement)" is
unnecessary because both OffscreenCanvas and HTMLCanvasElement support
getContext("bitmaprenderer"); remove the cast and call
this.#canvas.getContext("bitmaprenderer", { alpha: false })! to preserve type
safety; if TypeScript still complains about the union type, either add a short
comment explaining why a cast is required or narrow the type first (e.g., check
instanceof HTMLCanvasElement or assert via a single localized cast) while
keeping the comment explaining the reason.
🧹 Nitpick comments (5)
libraries/scrcpy-decoder-webcodecs/src/video/codec/av1.ts (1)
18-30: Consider caching to avoid redundant sequence header parsing.The code instantiates a new
Av1parser and searches for sequence headers on every packet. Since AV1 sequence headers typically appear only once at stream start, this creates unnecessary overhead for the majority of packets that won't contain sequence headers.Consider tracking whether a configuration has already been emitted and skipping the parsing for subsequent packets, unless dynamic resolution changes are expected.
♻️ Potential optimization approach
+ private configSent = false; + constructor() { super({ transform: (packet, controller) => { if (packet.type === "configuration") { // AV1 decoder doesn't need configuration packets return; } - const parser = new Av1(packet.data); - const sequenceHeader = parser.searchSequenceHeaderObu(); - - if (sequenceHeader) { + if (!this.configSent) { + const parser = new Av1(packet.data); + const sequenceHeader = parser.searchSequenceHeaderObu(); + + if (sequenceHeader) { - const width = sequenceHeader.max_frame_width_minus_1 + 1; - const height = sequenceHeader.max_frame_height_minus_1 + 1; - - controller.enqueue({ - codec: Av1.toCodecString(sequenceHeader), - codedWidth: width, - codedHeight: height, - }); + const width = sequenceHeader.max_frame_width_minus_1 + 1; + const height = sequenceHeader.max_frame_height_minus_1 + 1; + + controller.enqueue({ + codec: Av1.toCodecString(sequenceHeader), + codedWidth: width, + codedHeight: height, + }); + this.configSent = true; + } } controller.enqueue(Note: If the stream supports dynamic resolution changes, you may want to check for sequence headers periodically rather than only once.
libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts (1)
13-15: Consider documenting the pool capacity choice.The pool capacity of
4appears to be a magic number. Consider adding a comment explaining the rationale for this size (e.g., expected concurrent snapshot operations, memory constraints, etc.).📝 Suggested documentation improvement
+// Pool size of 4 allows up to 4 concurrent snapshot operations +// while keeping memory usage bounded const VideoFrameCapturerPool = /* #__PURE__ */ new Pool(() => new VideoFrameCapturer(), 4);libraries/scrcpy-decoder-webcodecs/src/video/render/bitmap.ts (1)
11-14: Consider aligning bitmaprenderer context configurations across renderers.The
alpha: trueconfiguration in bitmap.ts follows MDN best practices to avoid the performance cost ofalpha: false. However, snapshot.ts (line 15) still usesalpha: falsefor its ImageBitmapRenderingContext in the VideoFrameCapturer class. While the use cases differ (rendering vs. capturing), inconsistent configurations can create confusion. Either align both to usealpha: truefollowing the MDN guidance, or document why snapshot.ts requiresalpha: false.libraries/scrcpy-decoder-webcodecs/src/video/render/insertable-stream.ts (1)
24-26: Consider renaming getter to match standard spelling.The getter is named
writeablebut exposeswritablefrom the generator. The standard spelling is "writable" (as used in the Web Streams API and line 25). Consider renaming for consistency.📝 Suggested rename
- get writeable() { + get writable() { return this.#generator.writable; }libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (1)
19-35: Frame lifecycle looks correct, but consider getter naming.The stream implementation properly manages frame lifecycle by closing the previous frame before saving the new one. The close/abort handlers correctly call
dispose().However, the getter is named
writeable(non-standard spelling) instead ofwritable(standard Web Streams API spelling). This inconsistency also appears ininsertable-stream.ts.📝 Suggested rename for consistency
- get writeable() { + get writable() { return this.#writable; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
libraries/scrcpy-decoder-tinyh264/src/decoder.tslibraries/scrcpy-decoder-tinyh264/src/utils/performance.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/av1.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/index.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/utils.tslibraries/scrcpy-decoder-webcodecs/src/video/decoder.tslibraries/scrcpy-decoder-webcodecs/src/video/render/bitmap.tslibraries/scrcpy-decoder-webcodecs/src/video/render/canvas.tslibraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.tslibraries/scrcpy-decoder-webcodecs/src/video/render/index.tslibraries/scrcpy-decoder-webcodecs/src/video/render/insertable-stream.tslibraries/scrcpy-decoder-webcodecs/src/video/render/type.tslibraries/scrcpy-decoder-webcodecs/src/video/render/webgl.tslibraries/scrcpy-decoder-webcodecs/src/video/snapshot.tslibraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts
💤 Files with no reviewable changes (2)
- libraries/scrcpy-decoder-webcodecs/src/video/codec/utils.ts
- libraries/scrcpy-decoder-webcodecs/src/video/codec/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libraries/scrcpy-decoder-tinyh264/src/utils/performance.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-24T03:50:43.091Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 782
File: libraries/scrcpy-decoder-tinyh264/src/decoder.ts:193-196
Timestamp: 2025-08-24T03:50:43.091Z
Learning: In TinyH264Decoder, configuration failures should propagate the actual error up the client pipeline rather than being recovered locally. The decoder is designed to handle mid-stream configuration switches when the video encoder restarts, and subsequent valid config packets will re-configure correctly regardless of current decoder state.
Applied to files:
libraries/scrcpy-decoder-tinyh264/src/decoder.tslibraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.tslibraries/scrcpy-decoder-webcodecs/src/video/decoder.ts
🧬 Code graph analysis (8)
libraries/scrcpy-decoder-webcodecs/src/video/render/type.ts (1)
libraries/stream-extra/src/consumable.ts (1)
WritableStream(71-71)
libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts (4)
libraries/scrcpy-decoder-webcodecs/src/video/pool.ts (1)
Pool(1-37)libraries/scrcpy-decoder-tinyh264/src/types.ts (1)
ScrcpyVideoDecoderPerformanceCounter(15-36)libraries/stream-extra/src/push-readable.ts (2)
PushReadableStreamController(6-14)PushReadableStream(41-307)libraries/scrcpy-decoder-tinyh264/src/utils/performance.ts (1)
PerformanceCounter(3-64)
libraries/scrcpy-decoder-webcodecs/src/video/render/bitmap.ts (2)
libraries/scrcpy-decoder-tinyh264/src/decoder.ts (1)
canvas(40-42)libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (1)
canvas(8-10)
libraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts (2)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/event/src/event-emitter.ts (1)
EventEmitter(12-61)
libraries/scrcpy-decoder-webcodecs/src/video/codec/av1.ts (3)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (3)
CodecDecoder(7-10)Input(13-15)Output(22-22)libraries/media-codec/src/av1.ts (1)
Av1(197-762)
libraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.ts (3)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (4)
CodecDecoder(7-10)Input(13-15)Output(22-22)Config(17-20)libraries/stream-extra/src/concat.ts (1)
concatUint8Arrays(120-136)
libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts (1)
libraries/scrcpy-decoder-tinyh264/src/utils/gl.ts (2)
glCreateContext(11-59)glLoseContext(61-67)
libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (2)
libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts (1)
Options(205-212)libraries/scrcpy-decoder-tinyh264/src/utils/gl.ts (1)
createCanvas(1-9)
⏰ 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: build (20.x)
🔇 Additional comments (38)
libraries/scrcpy-decoder-webcodecs/src/video/codec/av1.ts (3)
1-4: LGTM!The imports are clean and correctly reference the necessary dependencies for the stream-based codec implementation.
6-9: LGTM!The class declaration correctly extends
TransformStreamand implementsCodecDecoder, aligning with the PR's objective to convert decoders to a stream-based architecture.
32-39: LGTM!The
EncodedVideoChunkgeneration correctly handles the keyframe fallback—treatingundefinedas"key"is a safe default that ensures decodability. The packet data and timestamp are properly forwarded to the chunk.libraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.ts (4)
1-1: LGTM!The new imports are properly used:
concatUint8Arraysfor merging configuration with frame data (line 26), andTransformStreamfor the stream-based decoder architecture (line 6).
5-8: LGTM!The refactor to extend
TransformStream<CodecDecoder.Input, CodecDecoder.Output>correctly aligns the decoder with the stream-based architecture, replacing the previous direct VideoDecoder approach.
11-42: LGTM!The transform logic correctly implements the stream-based decoding:
- Configuration packets are stored and the
configure()result is enqueued for downstream processing- Data packets properly handle Annex B format by concatenating configuration with the first frame (lines 25-27), then clearing the config
- The keyframe type derivation (line 35) correctly treats
undefinedas"key"to ensure frames decode properlyThe absence of error handling allows configuration failures to propagate up the client pipeline, which aligns with the intended error handling strategy.
Based on learnings, configuration failures should propagate to the client pipeline for proper error handling.
44-44: All subclasses have been updated to implement the new signature. BothH264DecoderandH265Decodercorrectly overrideconfigure()with the return typeCodecDecoder.Config, and the return values are properly enqueued into the stream.libraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts (2)
1-44: LGTM: Well-structured stream wrapper with clean metrics API.The class structure is sound. The definite assignment assertion on line 8 is necessary because the field is assigned after the
super()call. The public getters provide a clean observability surface.
46-103: LGTM: Stream implementation correctly handles decoder lifecycle.The constructor properly implements the TransformStream pattern:
- Start handler initializes the decoder with appropriate callbacks
- Transform handler validates configuration state before decoding
- Flush handler correctly awaits pending frames before closing
- Cancel handler immediately closes on error
The implementation follows best practices for WebCodecs decoder management.
libraries/scrcpy-decoder-tinyh264/src/decoder.ts (4)
26-28: LGTM: Appropriate noop export for error handling.Exporting the noop function supports the streaming pattern used throughout the refactoring, particularly for
.catch(noop)on void promises.
44-90: LGTM: Improved metric naming and documentation.The refactoring to use
PauseControllerandPerformanceCounterimproves the public API surface. The renamed metrics (framesRendered,framesSkippedRendering) better distinguish rendering from decoding stages, and the documentation clearly explains frame presentation semantics including the Chrome bug caveat.
114-138: LGTM: Streaming pipeline correctly handles TinyH264 constraints.The refactored pipeline properly:
- Validates decoder configuration before processing data packets
- Documents the TinyH264 limitation regarding frame metadata and skipping
- Copies packet data to avoid BufferCombiner issues
- Uses the standard void/catch(noop) pattern for fire-and-forget error handling
208-208: No action required. ThePerformanceCounterclass correctly provides theincreaseFramesRendered()method at line 54 inlibraries/scrcpy-decoder-tinyh264/src/utils/performance.ts, which increments the#framesDrawncounter. The method is properly used at line 208 in the decoder.libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts (5)
42-57: LGTM: Clean pause controller with timestamp-based frame skipping.The
PauseControllerintegration and#skipFramesUntilmechanism provide a robust approach to handling frame skipping during pause/resume cycles using monotonic timestamps.
74-115: LGTM: Well-documented decode time tracking rationale.The delegation to
VideoDecoderStreamis clean, and the comment at lines 104-108 clearly explains whytotalDecodeTimeis tracked at this level rather than in the stream—it includes pre-processing time and relies on timestamps set by this class.
117-147: LGTM: Comprehensive rendering metrics with clear documentation.The
RendererControllerintegration provides good observability. The documentation onframesPresented(lines 127-135) effectively explains the latency optimization and documents the Chrome nested Worker limitation.
152-176: LGTM: Clean codec selection and well-typed Options interface.The constructor properly instantiates codec-specific decoders with appropriate defaults. The
Optionsinterface cleanly extends relevantVideoDecoderConfigproperties, and the default values (hardwareAcceleration = "no-preference",optimizeForLatency = true) align well with low-latency streaming requirements.Also applies to: 269-279
177-240: No action required. Line 239 correctly usesrenderer.writeable, which matches theVideoFrameRendererinterface definition intype.ts. While the standard Web Streams API useswritable, this codebase intentionally defines and consistently useswriteableas the property name across allVideoFrameRendererimplementations (CanvasVideoFrameRenderer,InsertableStreamVideoFrameRenderer).Likely an incorrect or invalid review comment.
libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts (4)
78-101: LGTM! Well-designed frame queuing with proper lifecycle management.The write logic correctly handles:
- Cloning frames for snapshot capture (Line 82) to avoid ownership conflicts
- Skipping intermediate frames (Lines 88-91) to minimize latency
- Fire-and-forget drawing (Line 100) to avoid blocking the writable stream
This design aligns with the official Scrcpy client's low-latency approach.
109-127: LGTM! Proper backpressure handling with frame draining.The
#drawmethod correctly:
- Drains all queued frames in a loop (Lines 120-124)
- Respects backpressure via
awaitonenqueue(Line 122)- Updates performance metrics (Line 123)
The comments on Lines 112-117 provide excellent context for the latency-focused design choice.
71-77: LGTM! Proper abort signal wiring.The writable's
starthandler correctly propagates cancellation from the readable stream to the writable stream (Lines 74-75), ensuring proper cleanup when the consumer cancels.
38-64: LGTM! Clean delegation to PerformanceCounter.The performance counter getters correctly delegate to the internal
PerformanceCounterinstance with comprehensive JSDoc documentation that matches the interface contract.libraries/scrcpy-decoder-webcodecs/src/video/render/index.ts (1)
5-5: LGTM! Properly exports the new flow-control module.The export is consistent with the existing pattern and makes the
RendererControllerclass available as part of the public render API.libraries/scrcpy-decoder-webcodecs/src/video/render/bitmap.ts (1)
6-7: Reference the HTML spec clarifying ImageBitmap width/height behavior.The HTML specification explicitly states that
transferFromImageBitmap()makes the context's output bitmap refer to the provided ImageBitmap, with its intrinsic size derived from the ImageBitmap itself—not the canvas dimensions. ImageBitmap width/height can differ from canvas width/height; that's the intended behavior. Update the comment to reflect this, or clarify if there's a different reasonoptionsis not supported.libraries/scrcpy-decoder-webcodecs/src/video/render/insertable-stream.ts (3)
3-3: LGTM!The type import for
WritableStreamis correctly added to support the new stream-based API.
29-31: LGTM!The
streamgetter appropriately exposes the internalMediaStreamfor external use.
49-52: LGTM!The resize handler correctly synchronizes element dimensions with video frame dimensions, maintaining proper aspect ratio.
libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (6)
2-2: LGTM!The
WritableStreamimport is correctly added to support the new stream-based rendering pipeline.
12-17: LGTM!The new private fields appropriately support the device-pixel rendering feature and frame lifecycle management.
37-77: Verify error handling for concurrent draw calls.The
useDevicePixelsfeature is well-implemented with proper runtime validation. However, on line 72,void this.draw(this.#frame)discards the promise, which could lead to unhandled rejections ifdraw()fails. Additionally, rapid resize events might trigger overlapping draw calls.Consider whether the draw operation should be awaited or if there should be debouncing/throttling for resize events.
79-92: LGTM!The
#updateSizemethod correctly clamps dimensions to the frame size and avoids unnecessary updates when dimensions haven't changed.
105-122: LGTM!The
Optionsinterface is well-documented with clear guidance on theuseDevicePixelsfeature, including performance implications and usage constraints.
96-102: No breaking changes from the return type modification.The
dispose()method is not part of theVideoFrameRendererinterface contract, nor does it implement the standardDisposableinterface which would requiredispose(): void. The current callers at lines 30-31 (stream handler callbacks) do not await the method, so the return type change toundefinedintroduces no compatibility issues.The cleanup logic properly disconnects the ResizeObserver and closes the saved frame.
libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts (5)
1-2: LGTM!The cspell directives correctly suppress false positives for GLSL precision qualifiers.
95-96: Verify context type narrowing is intentional.Line 95 narrows the context type to
WebGLRenderingContext, butglCreateContext(line 111) can returnWebGL2RenderingContext. While theinstanceofcheck on line 161 handles WebGL2 features at runtime, the type narrowing loses compile-time type safety for WebGL2-specific APIs.Consider using a union type to preserve type information:
#context: WebGLRenderingContext | WebGL2RenderingContext;
105-152: LGTM!The constructor properly integrates the options parameter, passing it to the parent class and using
enableCaptureto configure WebGL context attributes. The trade-off betweenpreserveDrawingBufferanddesynchronizedis well-balanced for capture vs. latency.The program creation uses the new helper function and is properly initialized with
useProgram.
169-191: LGTM!The
overridekeyword correctly indicates method inheritance. The addition ofgl.flush()ensures commands are submitted for execution, which can improve frame timing in streaming scenarios, especially with the desynchronized context setting.
193-213: LGTM!The disposal sequence correctly cleans up resources in the proper order: program deletion, context invalidation, then parent cleanup. The
Optionsinterface appropriately extends the parent options and adds theenableCaptureflag with clear documentation.
| close: () => { | ||
| this.#readableController.close(); | ||
| this.#counter.dispose(); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Potential resource leak: unclosed VideoFrames at shutdown.
The close handler disposes the performance counter but doesn't close #captureFrame and #nextFrame. If these hold VideoFrame objects when the stream closes, they won't be released, causing a resource leak.
🔧 Proposed fix to close pending frames
close: () => {
+ this.#captureFrame?.close();
+ this.#nextFrame?.close();
this.#readableController.close();
this.#counter.dispose();
},📝 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.
| close: () => { | |
| this.#readableController.close(); | |
| this.#counter.dispose(); | |
| }, | |
| }); | |
| close: () => { | |
| this.#captureFrame?.close(); | |
| this.#nextFrame?.close(); | |
| this.#readableController.close(); | |
| this.#counter.dispose(); | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In @libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts around
lines 102 - 106, The close handler currently only closes
this.#readableController and disposes this.#counter, risking leaked VideoFrame
objects; update the close function where the ReadableStream is created (the
close: () => { ... } block) to also check and close any pending VideoFrame
references by calling close() on this.#captureFrame and this.#nextFrame if they
are non-null and then nulling them out before disposing the counter and closing
the controller so all frames are released at shutdown.
libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts
Outdated
Show resolved
Hide resolved
| import type { WritableStream } from "@yume-chan/stream-extra"; | ||
|
|
||
| export interface VideoFrameRenderer { | ||
| setSize(width: number, height: number): void; | ||
|
|
||
| draw(frame: VideoFrame): MaybePromiseLike<void>; | ||
|
|
||
| dispose(): MaybePromiseLike<undefined>; | ||
| writeable: WritableStream<VideoFrame>; | ||
| } |
There was a problem hiding this comment.
Excellent stream-based simplification, but verify spelling: "writeable" vs "writable".
The conversion to a stream-based interface is a clean architectural improvement. However, there's a potential spelling inconsistency:
- This interface uses
writeable(Line 4) RendererControllerinflow-control.tsuseswritable(Line 29)- Standard property naming in streams uses
writable(e.g.,TransformStream.writable)
Verify whether writeable is intentional or should be writable for consistency with standard stream APIs and other implementations in this PR.
#!/bin/bash
# Description: Check usage of writeable vs writable across the codebase.
echo "=== Searching for 'writeable' usage ==="
rg -n '\bwriteable\b' --type ts
echo -e "\n=== Searching for 'writable' usage in stream contexts ==="
rg -n '\bwritable\b.*Stream' --type ts -A 1🤖 Prompt for AI Agents
In @libraries/scrcpy-decoder-webcodecs/src/video/render/type.ts around lines 1 -
5, The interface uses the nonstandard property name `writeable`; change
`VideoFrameRenderer.writeable` to `writable` to match the standard stream API
and the `RendererController` usage in `flow-control.ts`, then update all
implementations and references to `VideoFrameRenderer` (including any
classes/functions that consume or construct that interface) to use `writable` so
names are consistent across the codebase.
| function createShader(gl: WebGLRenderingContext, type: number, source: string) { | ||
| const shader = gl.createShader(type)!; | ||
| gl.shaderSource(shader, source); | ||
| gl.compileShader(shader); | ||
| return shader; | ||
| } | ||
|
|
||
| function createProgram( | ||
| gl: WebGLRenderingContext, | ||
| vertexShaderSource: string, | ||
| fragmentShaderSource: string, | ||
| ) { | ||
| const vertexShader = createShader(gl, gl.VERTEX_SHADER, vertexShaderSource); | ||
| const fragmentShader = createShader( | ||
| gl, | ||
| gl.FRAGMENT_SHADER, | ||
| fragmentShaderSource, | ||
| ); | ||
|
|
||
| const program = gl.createProgram(); | ||
| gl.attachShader(program, vertexShader); | ||
| gl.attachShader(program, fragmentShader); | ||
| gl.linkProgram(program); | ||
|
|
||
| try { | ||
| if (gl.getProgramParameter(program, gl.LINK_STATUS)) { | ||
| return program; | ||
| } | ||
|
|
||
| // Don't check shader compile status unless linking fails | ||
| // https://developer.mozilla.org/en-US/docs/Web/API/WebGL_API/WebGL_best_practices#dont_check_shader_compile_status_unless_linking_fails | ||
| if (!gl.getShaderParameter(vertexShader, gl.COMPILE_STATUS)) { | ||
| throw new Error(gl.getShaderInfoLog(vertexShader)!); | ||
| } | ||
|
|
||
| if (!gl.getShaderParameter(fragmentShader, gl.COMPILE_STATUS)) { | ||
| throw new Error(gl.getShaderInfoLog(fragmentShader)!); | ||
| } | ||
|
|
||
| throw new Error(gl.getProgramInfoLog(program)!); | ||
| } finally { | ||
| // Delete objects eagerly | ||
| // https://developer.mozilla.org/en-US/docs/Web/API/WebGL_API/WebGL_best_practices#delete_objects_eagerly | ||
| gl.deleteShader(vertexShader); | ||
| gl.deleteShader(fragmentShader); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add null check for program creation.
Line 33 calls gl.createProgram() which can return null if the WebGL context is lost. The subsequent operations (lines 34-36, 39) assume a non-null program, which could throw if the context was lost between initialization and program creation.
The shader compilation error handling follows best practices by checking compile status only when linking fails. The eager deletion of shaders in the finally block is appropriate.
🛡️ Proposed null safety fix
const program = gl.createProgram();
+ if (!program) {
+ throw new Error("Failed to create WebGL program");
+ }
gl.attachShader(program, vertexShader);
gl.attachShader(program, fragmentShader);
gl.linkProgram(program);📝 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.
| function createShader(gl: WebGLRenderingContext, type: number, source: string) { | |
| const shader = gl.createShader(type)!; | |
| gl.shaderSource(shader, source); | |
| gl.compileShader(shader); | |
| return shader; | |
| } | |
| function createProgram( | |
| gl: WebGLRenderingContext, | |
| vertexShaderSource: string, | |
| fragmentShaderSource: string, | |
| ) { | |
| const vertexShader = createShader(gl, gl.VERTEX_SHADER, vertexShaderSource); | |
| const fragmentShader = createShader( | |
| gl, | |
| gl.FRAGMENT_SHADER, | |
| fragmentShaderSource, | |
| ); | |
| const program = gl.createProgram(); | |
| gl.attachShader(program, vertexShader); | |
| gl.attachShader(program, fragmentShader); | |
| gl.linkProgram(program); | |
| try { | |
| if (gl.getProgramParameter(program, gl.LINK_STATUS)) { | |
| return program; | |
| } | |
| // Don't check shader compile status unless linking fails | |
| // https://developer.mozilla.org/en-US/docs/Web/API/WebGL_API/WebGL_best_practices#dont_check_shader_compile_status_unless_linking_fails | |
| if (!gl.getShaderParameter(vertexShader, gl.COMPILE_STATUS)) { | |
| throw new Error(gl.getShaderInfoLog(vertexShader)!); | |
| } | |
| if (!gl.getShaderParameter(fragmentShader, gl.COMPILE_STATUS)) { | |
| throw new Error(gl.getShaderInfoLog(fragmentShader)!); | |
| } | |
| throw new Error(gl.getProgramInfoLog(program)!); | |
| } finally { | |
| // Delete objects eagerly | |
| // https://developer.mozilla.org/en-US/docs/Web/API/WebGL_API/WebGL_best_practices#delete_objects_eagerly | |
| gl.deleteShader(vertexShader); | |
| gl.deleteShader(fragmentShader); | |
| } | |
| } | |
| function createShader(gl: WebGLRenderingContext, type: number, source: string) { | |
| const shader = gl.createShader(type)!; | |
| gl.shaderSource(shader, source); | |
| gl.compileShader(shader); | |
| return shader; | |
| } | |
| function createProgram( | |
| gl: WebGLRenderingContext, | |
| vertexShaderSource: string, | |
| fragmentShaderSource: string, | |
| ) { | |
| const vertexShader = createShader(gl, gl.VERTEX_SHADER, vertexShaderSource); | |
| const fragmentShader = createShader( | |
| gl, | |
| gl.FRAGMENT_SHADER, | |
| fragmentShaderSource, | |
| ); | |
| const program = gl.createProgram(); | |
| if (!program) { | |
| throw new Error("Failed to create WebGL program"); | |
| } | |
| gl.attachShader(program, vertexShader); | |
| gl.attachShader(program, fragmentShader); | |
| gl.linkProgram(program); | |
| try { | |
| if (gl.getProgramParameter(program, gl.LINK_STATUS)) { | |
| return program; | |
| } | |
| // Don't check shader compile status unless linking fails | |
| // https://developer.mozilla.org/en-US/docs/Web/API/WebGL_API/WebGL_best_practices#dont_check_shader_compile_status_unless_linking_fails | |
| if (!gl.getShaderParameter(vertexShader, gl.COMPILE_STATUS)) { | |
| throw new Error(gl.getShaderInfoLog(vertexShader)!); | |
| } | |
| if (!gl.getShaderParameter(fragmentShader, gl.COMPILE_STATUS)) { | |
| throw new Error(gl.getShaderInfoLog(fragmentShader)!); | |
| } | |
| throw new Error(gl.getProgramInfoLog(program)!); | |
| } finally { | |
| // Delete objects eagerly | |
| // https://developer.mozilla.org/en-US/docs/Web/API/WebGL_API/WebGL_best_practices#delete_objects_eagerly | |
| gl.deleteShader(vertexShader); | |
| gl.deleteShader(fragmentShader); | |
| } | |
| } |
🤖 Prompt for AI Agents
In @libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts around lines 14
- 60, createProgram may receive null from gl.createProgram() if the WebGL
context is lost; add an immediate null check after const program =
gl.createProgram() in createProgram and handle it before calling
gl.attachShader/linkProgram. Specifically, if program is null, delete the
vertexShader and fragmentShader (to avoid leaks) and throw a clear Error (e.g.,
"Failed to create program: WebGL context lost") so subsequent
gl.attachShader/gl.linkProgram calls never run on a null program.
| this.#context = (this.#canvas as HTMLCanvasElement).getContext( | ||
| "bitmaprenderer", | ||
| { alpha: false }, | ||
| )!; |
There was a problem hiding this comment.
Verify the necessity of the HTMLCanvasElement type cast.
The type cast (this.#canvas as HTMLCanvasElement) appears suspicious because:
this.#canvascan be eitherOffscreenCanvas(Line 7) orHTMLCanvasElement(Line 9).- Both canvas types support
getContext("bitmaprenderer").
If the cast is needed due to TypeScript type definitions, please add a comment explaining why. Otherwise, consider removing the cast to preserve type safety.
🔍 Proposed fix to remove unnecessary cast
- this.#context = (this.#canvas as HTMLCanvasElement).getContext(
- "bitmaprenderer",
- { alpha: false },
- )!;
+ this.#context = this.#canvas.getContext(
+ "bitmaprenderer",
+ { alpha: false },
+ )!;📝 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.
| this.#context = (this.#canvas as HTMLCanvasElement).getContext( | |
| "bitmaprenderer", | |
| { alpha: false }, | |
| )!; | |
| this.#context = this.#canvas.getContext( | |
| "bitmaprenderer", | |
| { alpha: false }, | |
| )!; |
🤖 Prompt for AI Agents
In @libraries/scrcpy-decoder-webcodecs/src/video/snapshot.ts around lines 13 -
16, The explicit cast "(this.#canvas as HTMLCanvasElement)" is unnecessary
because both OffscreenCanvas and HTMLCanvasElement support
getContext("bitmaprenderer"); remove the cast and call
this.#canvas.getContext("bitmaprenderer", { alpha: false })! to preserve type
safety; if TypeScript still complains about the union type, either add a short
comment explaining why a cast is required or narrow the type first (e.g., check
instanceof HTMLCanvasElement or assert via a single localized cast) while
keeping the comment explaining the reason.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libraries/scrcpy-decoder-webcodecs/src/video/render/insertable-stream.ts (1)
13-62: Missing cleanup logic for MediaStreamTrackGenerator.The class no longer has a
dispose()method after removing the lifecycle methods. TheMediaStreamTrackGeneratortrack should be stopped when no longer needed to release resources. Consider adding cleanup:🔧 Suggested addition
+ dispose(): undefined { + this.#generator.stop(); + this.#stream.getTracks().forEach(track => track.stop()); + }libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (1)
212-223: Missingcontroller.terminate()in dispose.The
dispose()method clears internal state but doesn't terminate theTransformStream. Downstream consumers may hang waiting for more data. Consider terminating the controller:🔧 Suggested fix
dispose() { if (this.#disposed) { return; } this.#disposed = true; this.#pendingConfiguration = undefined; this.#pendingFrames.length = 0; this.#disposeVisibilityTracker?.(); + this.#controller.terminate(); }
🤖 Fix all issues with AI agents
In @libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts:
- Around line 220-234: The TransformStream transform callback filters out frames
when frame.timestamp <= this.#skipFramesUntil but never closes them, leaking GPU
resources; update the transform function inside the TransformStream in
decoder.ts to call frame.close() for skipped VideoFrame instances (i.e., when
frame.timestamp <= this.#skipFramesUntil) before returning, and keep
controller.enqueue(frame) only for frames that are forwarded so that all skipped
frames are explicitly closed.
In @libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts:
- Around line 108-113: The abort() handler currently leaves #nextFrame open
which can keep a pending VideoFrame alive; after calling
this.#readableController.error(reason) and this.#counter.dispose() also
explicitly close/cancel #nextFrame to release any held VideoFrame and avoid
leaks (but keep the existing behavior of not closing #captureFrame so snapshots
still work); make the close/cancel call defensive (check for existence and
whether it's already closed) so it’s safe to call from abort and won’t interfere
with normal #draw() completion.
🧹 Nitpick comments (1)
libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (1)
56-76: Consider error handling for resize redraw.The
void this.draw()on line 72 swallows any errors from the redraw operation. While this prevents unhandled promise rejections, errors during resize-triggered redraws will be silently lost.💡 Optional: Add error logging
if (this.#updateSize(width, height) && this.#frame) { - void this.draw(this.#frame); + void this.draw(this.#frame).catch((e) => { + console.error("Failed to redraw on resize:", e); + }); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
libraries/scrcpy-decoder-tinyh264/src/utils/pause.tslibraries/scrcpy-decoder-webcodecs/src/video/decoder.tslibraries/scrcpy-decoder-webcodecs/src/video/render/canvas.tslibraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.tslibraries/scrcpy-decoder-webcodecs/src/video/render/insertable-stream.tslibraries/scrcpy-decoder-webcodecs/src/video/render/type.tslibraries/scrcpy-decoder-webcodecs/src/video/snapshot.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-24T03:50:43.091Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 782
File: libraries/scrcpy-decoder-tinyh264/src/decoder.ts:193-196
Timestamp: 2025-08-24T03:50:43.091Z
Learning: In TinyH264Decoder, configuration failures should propagate the actual error up the client pipeline rather than being recovered locally. The decoder is designed to handle mid-stream configuration switches when the video encoder restarts, and subsequent valid config packets will re-configure correctly regardless of current decoder state.
Applied to files:
libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts
🧬 Code graph analysis (3)
libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts (8)
libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (3)
PauseController(12-224)Output(228-230)Input(227-227)libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts (2)
RendererController(17-159)frame(117-136)libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (3)
CodecDecoder(7-10)Output(22-22)Input(13-15)libraries/scrcpy-decoder-webcodecs/src/video/codec/h264.ts (1)
H264Decoder(6-16)libraries/scrcpy-decoder-webcodecs/src/video/codec/h265.ts (1)
H265Decoder(6-19)libraries/scrcpy-decoder-webcodecs/src/video/codec/av1.ts (1)
Av1Codec(6-43)libraries/scrcpy-decoder-webcodecs/src/video/time.ts (1)
increasingNow(21-28)libraries/stream-extra/src/inspect.ts (1)
InspectStream(5-23)
libraries/scrcpy-decoder-webcodecs/src/video/render/type.ts (1)
libraries/stream-extra/src/consumable.ts (1)
WritableStream(71-71)
libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts (7)
libraries/scrcpy-decoder-webcodecs/src/video/pool.ts (1)
Pool(1-37)libraries/scrcpy-decoder-webcodecs/src/video/snapshot.ts (1)
VideoFrameCapturer(1-42)libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/scrcpy-decoder-tinyh264/src/types.ts (1)
ScrcpyVideoDecoderPerformanceCounter(15-36)libraries/stream-extra/src/push-readable.ts (2)
PushReadableStreamController(6-14)PushReadableStream(41-307)libraries/scrcpy-decoder-tinyh264/src/utils/performance.ts (1)
PerformanceCounter(3-64)libraries/adb-scrcpy/src/client.ts (1)
controller(285-287)
⏰ 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: build (20.x)
🔇 Additional comments (11)
libraries/scrcpy-decoder-webcodecs/src/video/render/type.ts (1)
1-5: Clean stream-based interface design.The simplified interface exposing only
writable: WritableStream<VideoFrame>aligns well with the stream-based architecture and matches the standardTransformStream.writablenaming convention.libraries/scrcpy-decoder-webcodecs/src/video/render/insertable-stream.ts (1)
49-52: Good use of resize event for dimension synchronization.The resize event listener correctly syncs the video element's
width/heightattributes with its intrinsicvideoWidth/videoHeight, ensuring proper rendering dimensions.libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts (2)
152-158: Proper cleanup in dispose.The
dispose()method correctly closes both#captureFrameand#nextFrame, preventing resource leaks when manually cleaning up.
78-94: Frame lifecycle is correct; no use-after-close issue.The write handler intentionally stores the original frame in
#nextFrame(line 92) without cloning because frame ownership transfers to the stream viaenqueue()in the#drawmethod. The#nextFramereference is cleared toundefined(line 129) before the frame is enqueued, so when subsequent writes arrive during drawing, they do not close an in-flight frame—they only replace the queued frame reference. The renderer receives ownership of the enqueued frame and is responsible for closing it. The pattern is sound:#captureFrameis cloned for snapshot safety, while#nextFramemaintains the original reference that transfers ownership downstream.libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (2)
50-102: Clean synchronous controller initialization.The controller capture pattern correctly assigns the controller synchronously in the
startcallback, avoiding the race condition from the previous implementation that usedawait Promise.resolve().
149-162: Empty array case handled correctly.The
for...ofiteration overentries()safely handles an empty#pendingFramesarray by simply not executing the loop body, avoiding the previous concern about accessingframes[-1].libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (2)
19-32: Good stream-based frame handling.The
WritableStreamproperly manages the frame lifecycle: closes the previous frame before storing the new one, conditionally updates size based on options, and delegates to the abstractdraw()method.
79-92: Well-designed size update logic.The
#updateSizemethod correctly clamps dimensions to avoid upscaling beyond source resolution and returns a boolean to enable conditional redraws, avoiding unnecessary work.libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts (3)
177-240: Well-structured streaming pipeline.The pipeline composition is clean and readable with clear stage responsibilities:
- Timestamp injection
- Codec-specific packet transformation
- Configuration inspection/size tracking
- Raw video decoding
- Decode time tracking and skip filtering
- Render flow control
- Final rendering
The
void ... .catch(noop)pattern prevents unhandled rejections.
270-281: Clean options interface using type reuse.Using
Pick<VideoDecoderConfig, ...>ensures type consistency with the native WebCodecs API and avoids duplicating type definitions.
259-267: dispose() effectively halts pipeline processing through the#disposedflag.The dispose() method doesn't need to explicitly abort the writable stream. When
#pause.dispose()is called, it sets#disposed = true, which causes PauseController's transform function to return early without enqueueing packets (lines 58-59 and 83-84). This prevents any further data from flowing through the pipeline, even if the source stream continues sending data.The design is intentional: dispose() handles resource cleanup while stream closure is managed separately via the comment's note about cleanup happening "when
writableends."Likely an incorrect or invalid review comment.
| .pipeThrough( | ||
| new TransformStream<VideoFrame, VideoFrame>({ | ||
| transform: (frame, controller) => { | ||
| // `frame.timestamp` is the same `EncodedVideoChunk.timestamp` set above | ||
| this.#totalDecodeTime += | ||
| performance.now() - frame.timestamp; | ||
|
|
||
| // Don't count these frames as skipped rendering | ||
| if (frame.timestamp <= this.#skipFramesUntil) { | ||
| return; | ||
| } | ||
|
|
||
| controller.enqueue(frame); | ||
| }, | ||
| }), |
There was a problem hiding this comment.
Resource leak: skipped frames not closed.
When frame.timestamp <= this.#skipFramesUntil, the frame is filtered out but not closed. VideoFrame objects hold GPU resources and must be explicitly closed to avoid memory leaks.
🔧 Proposed fix
transform: (frame, controller) => {
// `frame.timestamp` is the same `EncodedVideoChunk.timestamp` set above
this.#totalDecodeTime +=
performance.now() - frame.timestamp;
// Don't count these frames as skipped rendering
if (frame.timestamp <= this.#skipFramesUntil) {
+ frame.close();
return;
}
controller.enqueue(frame);
},📝 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.
| .pipeThrough( | |
| new TransformStream<VideoFrame, VideoFrame>({ | |
| transform: (frame, controller) => { | |
| // `frame.timestamp` is the same `EncodedVideoChunk.timestamp` set above | |
| this.#totalDecodeTime += | |
| performance.now() - frame.timestamp; | |
| // Don't count these frames as skipped rendering | |
| if (frame.timestamp <= this.#skipFramesUntil) { | |
| return; | |
| } | |
| controller.enqueue(frame); | |
| }, | |
| }), | |
| .pipeThrough( | |
| new TransformStream<VideoFrame, VideoFrame>({ | |
| transform: (frame, controller) => { | |
| // `frame.timestamp` is the same `EncodedVideoChunk.timestamp` set above | |
| this.#totalDecodeTime += | |
| performance.now() - frame.timestamp; | |
| // Don't count these frames as skipped rendering | |
| if (frame.timestamp <= this.#skipFramesUntil) { | |
| frame.close(); | |
| return; | |
| } | |
| controller.enqueue(frame); | |
| }, | |
| }), |
🤖 Prompt for AI Agents
In @libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts around lines 220 -
234, The TransformStream transform callback filters out frames when
frame.timestamp <= this.#skipFramesUntil but never closes them, leaking GPU
resources; update the transform function inside the TransformStream in
decoder.ts to call frame.close() for skipped VideoFrame instances (i.e., when
frame.timestamp <= this.#skipFramesUntil) before returning, and keep
controller.enqueue(frame) only for frames that are forwarded so that all skipped
frames are explicitly closed.
| abort: (reason) => { | ||
| this.#readableController.error(reason); | ||
| this.#counter.dispose(); | ||
| // Don't close `#captureFrame` to allow using `snapshot` on the last frame | ||
| // Don't close `#nextFrame` to make sure all frames are rendered | ||
| }, |
There was a problem hiding this comment.
Consider closing #nextFrame on abort.
On abort(), the stream terminates abnormally and #draw() won't complete. The comment says "Don't close #nextFrame to make sure all frames are rendered," but after abort, no more frames will be rendered. This could leak the VideoFrame.
🔧 Suggested fix
abort: (reason) => {
this.#readableController.error(reason);
this.#counter.dispose();
// Don't close `#captureFrame` to allow using `snapshot` on the last frame
- // Don't close `#nextFrame` to make sure all frames are rendered
+ this.#nextFrame?.close();
+ this.#nextFrame = undefined;
},🤖 Prompt for AI Agents
In @libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts around
lines 108 - 113, The abort() handler currently leaves #nextFrame open which can
keep a pending VideoFrame alive; after calling
this.#readableController.error(reason) and this.#counter.dispose() also
explicitly close/cancel #nextFrame to release any held VideoFrame and avoid
leaks (but keep the existing behavior of not closing #captureFrame so snapshots
still work); make the close/cancel call defensive (check for existence and
whether it's already closed) so it’s safe to call from abort and won’t interfere
with normal #draw() completion.
Summary by CodeRabbit
New Features
Refactor
Breaking changes
✏️ Tip: You can customize this high-level summary in your review settings.