feat: enhance AtemSocket to handle caching#191
feat: enhance AtemSocket to handle caching#191milux wants to merge 2 commits intoSofie-Automation:mainfrom
Conversation
WalkthroughExports a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Socket as AtemSocket
participant Normalizer as Normalizer
participant Parser as Parser
participant Cache as TimeCache
Client->>Socket: send payload (ThreadedPayload, packetId)
Socket->>Normalizer: _normalizePayload(payload)
Normalizer-->>Socket: Buffer or undefined
alt payload invalid
Socket-->>Client: emit error / drop
else payload valid
Socket->>Parser: _parseCommands(buffer, packetId)
Parser->>Cache: check isFirstCommand & remainder
alt first is TimeCommand and remainder == cached
Cache-->>Parser: match -> consume first only, stop parsing
else first is TimeCommand and remainder != cached
Cache-->>Parser: cache remainder -> stop parsing
else not TimeCommand
Cache-->>Parser: clear cached remainder
end
Parser-->>Socket: parsed commands
Socket-->>Client: deliver commands / callbacks
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/atemSocket.ts`:
- Line 44: The cached buffer _lastTimeCommandRemainder must be cleared on
connection lifecycle changes to avoid stale-remainder matches; update the
AtemSocket.connect and AtemSocket.disconnect methods to reset
_lastTimeCommandRemainder = undefined (and likewise reset any other "remainder"
buffer fields you have, e.g., packet/command remainder variables) so no leftover
bytes from a previous session can be matched against a new device/session.
- Around line 215-233: The bug is that isFirstCommand stays true if command
deserialization fails or is unknown, causing a later command to be treated as
the "first" and wrongly apply the TimeCommand remainder logic; to fix, ensure
isFirstCommand is set to false for every command slot immediately after
attempting to read/deserialise a command (regardless of success), so move the
isFirstCommand = false assignment out of the TimeCommand-success branch and into
the per-command processing loop right after command identification/attempted
deserialization; update logic around TimeCommand, _lastTimeCommandRemainder,
parsedCommands and any early-break paths so the remainder cache is only applied
when isFirstCommand was true for that slot and then cleared/updated
consistently.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/lib/atemSocket.ts (2)
56-77:connect()still does not reset_lastTimeCommandRemainder— stale cache via therestartedpath.The
restartedevent (line 176) callsthis.connect()directly without going throughdisconnect(), so a stale remainder from the previous session can survive into the new connection and silently suppress real state-update commands on the first matching batch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/atemSocket.ts` around lines 56 - 77, The connect() method must clear the stale command remainder so a leftover _lastTimeCommandRemainder from a previous session doesn't suppress new commands; update connect() (the method invoked by the restarted event) to reset/empty the _lastTimeCommandRemainder before establishing a new socket (e.g., at the start of connect() or right after setting this._address/this._port) so the restarted path behaves the same as disconnect()+connect().
239-243:isFirstCommandstill not reset for the unknown-command path.
isFirstCommand = false(line 240) is inside theif (cmdConstructor && …)branch, so if the first command is unrecognised (falls into theelseat line 241),isFirstCommandremainstrueand the second command inherits first-command status, opening the TimeCommand-cache logic to misapplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/atemSocket.ts` around lines 239 - 243, The flag isFirstCommand is only cleared inside the recognized-command branch, so if the very first incoming command is unrecognized the flag remains true and mislabels the next command; update the handler around isFirstCommand (the code that checks cmdConstructor and emits 'debug' for unknown commands) to always set isFirstCommand = false after processing any command path (either move the assignment after the if/else or add the assignment inside the else branch) so the first-command state is cleared regardless of whether the command was recognized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/atemSocket.ts`:
- Around line 151-158: Change the callback parameter type from Buffer to the
union type used at runtime (ThreadedPayload) where the IPC message handler is
declared so TypeScript matches reality; update the async handler signature (the
function that calls this._normalizePayload and this._parseCommands) to accept
payload: ThreadedPayload, and ensure ThreadedPayload is imported or referenced
in the file so the call to _normalizePayload(payload) and subsequent
this._parseCommands(normalizedPayload) type-check correctly.
---
Duplicate comments:
In `@src/lib/atemSocket.ts`:
- Around line 56-77: The connect() method must clear the stale command remainder
so a leftover _lastTimeCommandRemainder from a previous session doesn't suppress
new commands; update connect() (the method invoked by the restarted event) to
reset/empty the _lastTimeCommandRemainder before establishing a new socket
(e.g., at the start of connect() or right after setting
this._address/this._port) so the restarted path behaves the same as
disconnect()+connect().
- Around line 239-243: The flag isFirstCommand is only cleared inside the
recognized-command branch, so if the very first incoming command is unrecognized
the flag remains true and mislabels the next command; update the handler around
isFirstCommand (the code that checks cmdConstructor and emits 'debug' for
unknown commands) to always set isFirstCommand = false after processing any
command path (either move the assignment after the if/else or add the assignment
inside the else branch) so the first-command state is cleared regardless of
whether the command was recognized.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/atemSocket.ts`:
- Around line 240-244: The first-command state isn't cleared when a command is
unrecognized: ensure isFirstCommand and the cached _lastTimeCommandRemainder are
reset regardless of whether a cmdConstructor exists or deserialization succeeds;
specifically, move or duplicate the assignment isFirstCommand = false (and set
this._lastTimeCommandRemainder = undefined) out of the successful-deserialize
branch so that the else branch that emits `Unknown command ${name} (${length}b)`
also clears both flags/caches, preventing stale TimeCommand remainder from
affecting subsequent commands/batches.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/atemSocket.ts (1)
217-234:⚠️ Potential issue | 🟡 MinorTimeCommand remainder-caching logic is sound, but
_lastTimeCommandRemainderis not invalidated when the first command is unknown or fails to deserialize.When the first command in a batch is unrecognized (line 240–241) or its deserialization throws (line 237–238), the
isFirstCommandblock (lines 217–234) is never entered, so_lastTimeCommandRemaindersilently persists from a previous batch. A later batch whose first command is a TimeCommand could then match against that stale remainder and incorrectly skip processing.In practice this is a narrow edge case (unknown first commands are unusual, and the byte comparison provides a safety net), but for correctness it would be cleaner to clear the cache whenever the first-command slot is consumed without entering the TimeCommand path.
Proposed fix — clear cache for unknown / failed first commands
} else { this.emit('debug', `Unknown command ${name} (${length}b)`) } // Always clear the first command flag after processing the first command. + if (isFirstCommand) { + // First command was unknown or deserialization failed — invalidate the cache. + this._lastTimeCommandRemainder = undefined + } isFirstCommand = falseThis works because whenever the code does reach the
isFirstCommandblock inside the try (line 217), it either caches a new remainder or clears it explicitly, and then falls through here withisFirstCommandalready consumed. Adding this guard catches only the paths that bypassed the inner block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/atemSocket.ts` around lines 217 - 234, The _lastTimeCommandRemainder cache can persist when the first-command slot is consumed by an unknown command or when deserialization throws, so update the branches that consume the first command but bypass the isFirstCommand TimeCommand logic to explicitly clear this cache: inside the unknown-command branch (where the code currently handles unrecognized commands) and inside the catch block that handles deserialization errors, set this._lastTimeCommandRemainder = undefined so stale remainders cannot later cause a TimeCommand batch to be skipped; refer to the isFirstCommand flag, TimeCommand class check, and the _lastTimeCommandRemainder field to locate the exact places to clear the cache.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/atemSocket.ts`:
- Around line 217-234: The _lastTimeCommandRemainder cache can persist when the
first-command slot is consumed by an unknown command or when deserialization
throws, so update the branches that consume the first command but bypass the
isFirstCommand TimeCommand logic to explicitly clear this cache: inside the
unknown-command branch (where the code currently handles unrecognized commands)
and inside the catch block that handles deserialization errors, set
this._lastTimeCommandRemainder = undefined so stale remainders cannot later
cause a TimeCommand batch to be skipped; refer to the isFirstCommand flag,
TimeCommand class check, and the _lastTimeCommandRemainder field to locate the
exact places to clear the cache.
1 participant
About the Contributor
This PR is a personal contribution to the
sofie-atem-connectionlibrary in order to improve overall efficiency.Fixes #190.
Type of Contribution
This is a:
Bug fix / Feature
Current Behavior
As discussed in #190, highly frequent command batches of
TimeCommands followed by other redundant commands with identical contents cause significant CPU load, even when ATEM devices are not actively "doing" anything in particular.New Behavior
The CPU load was (again) cut down approximately by half for affected ATEM devices.
This is achieved by two means primarily:
TimeCommandare (partially, after theTimeCommanditself) cached. Iff the next batch also starts with a time command and otherwise equals the previous batch (byte-wise) entirely. Only theTimeCommandis emitted and the remaining contents are dropped without (redundant) deserialization and further processing.Testing Instructions
I tried to provide full (jest) test coverage for all added and modified code snippets.
I took precautions to make sure my caching strategy is sound by always flushing/updating the cached copy if a command batch does either not start with a
TimeCommandor differs by content or length after theTimeCommand.My optimization should be valid iff the following assumptions hold:
_parseCommands, i.e., there is no "side channel" that may cause state changes that would normally be overwritten by the commands received.If I am mistaken about any of those assumptions, my PR may introduce subtile semantic errors and should not be merged as is.
Other Information
Since this is my first contribution to this lib, I want to kindly ask you carefully reviewing my changes and see if they fit your expectations.
Specifically, please see if my assumptions above hold. If they do, I am optimistic that this PR can be safely merged.
Status
Overview
Implements an optimization to reduce CPU load from frequent, redundant ATEM command batches (notably TimeCommand-heavy traffic). Adds payload normalization to avoid buffer copies and a caching mechanism to skip redundant deserialization of command batch remainders when batches start with a TimeCommand.
Key Changes
Core Implementation (src/lib/atemSocket.ts)
ThreadedPayload = Buffer | Uint8Array | { type: 'Buffer'; data: number[] }._normalizePayload(payload: ThreadedPayload)to convert supported payload variants to a nativeBufferview without making full copies; returnsundefinedfor invalid shapes._lastTimeCommandRemainder: Buffer | undefinedto cache the bytes following a leadingTimeCommand._parseCommands()tracks the first command; if it is aTimeCommandand the remainder matches the cached remainder, it processes only theTimeCommandand skips the remainder (no deserialization/processing). If different, updates the cache. If the first command is not aTimeCommand, clears the cache.length < 8 || length > buffer.length)._lastTimeCommandRemainderon connect and disconnect.Public API / Type Changes
ThreadedPayloadis exported from the AtemSocket module.onCommandsReceivednow acceptsThreadedPayloadinstead of rawBuffer.Tests (src/lib/tests/atemSocket.spec.ts)
ThreadedPayloadand the normalization path.Uint8Arrayand{ type: 'Buffer'; data: number[] }.TimeCommandandThreadedPayload.Implications & Assumptions
_parseCommands()(author-documented assumptions). Reviewers should validate these assumptions before merging.TimeCommand; flushed whenever a batch starts with a different command or the remainder differs in content or length.Performance Impact