feat: enhance AtemSocket to handle caching#191
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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: drop / emit error
else payload valid
Socket->>Parser: _parseCommands(buffer, packetId)
Parser->>Cache: check isFirstCommand & remainder
alt first is TimeCommand and remainder == cached
Cache-->>Parser: match -> process 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
|
@Julusian CodeRabbit was initially unhappy with my cache handling in some edge cases. Had some more iterations but think it is very robust now. |
|
could you send me an xml export of your atem? Im not sure what I need to change to reproduce this, the old trigger doesnt appear to cause this anymore. I'm currently wondering whether the risk (and minor complexity) this introduces is worth the performance gain, vs potentially finding another solution. I'm a little worried about the potential for causing something to be lost if it gets dumped at the end of the sequence. Maybe a bit cautious, I can't think of what could be there that could happen in 2 frames in a row. I think this will be invalidated unnecessarily at times (eg, when theres a cut, that gets put after the TimeCommand in the same packet), but thats fine, as itll only be 2 packets of that. I'm also wondering how much actual performance gain this gives, as parsing those commands cant be all that much work surely. its a bit of extra parsing of the buffers and object construction, but we are swallowing the change when it is doing nothing, so it shouldnt be leaving the bounds of the library code. |
Funny, I just realized from the Source Code you referenced below that this issue on our device series has been known (and documented plus partially mitigated by you) since 2023. 😆 I created a XML export and can send it via any channel of your choice, but I'm 99 % sure that it won't provide you any helpful insight.
Right, it will be invalidated unnecessarily often times because I implemented it as defensive as possible to get the risk of error down to the absolute minimum. As stated, the cache is flushed iff (equivalence relation!) the sequence does not start with a The one and only risk I cannot safely rule out is "external state mutation" between frames with identical data, i.e., whether there is some code in the lib that could cause a state change which is not reflected by a command from the ATEM right after and might remain stale due to skipping of cached commands.
I totally agree with you in every part but for the "as parsing those commands cant be all that much work surely" assumption. TBH, I thought so too, until I did the profiling and noticed that even this parsing and checking of 32 unspectacular I have to speculate about precise reasons here, but I assume that something of the comparison logic might work against caches or the prediction units of the CPU. Further, I have observed heavy fluctuation of the heap utilization before my caching patch which almost entirely vanished. That actually makes perfect sense since about 1.6k |
|
I haven't entirely forgotten about this, but I'm still not 100% sure on if it introduces risk of error or not. With the range of users, that makes me nervous. Maybe this could be gated behind a boolean on Or, I know we don't have the structure in place, but maybe this should be gated on models that we know do or could have this issue? |
Both approaches sound reasonable to me. If you want me to implement either one of that approaches, please tell me what I shall do. |
2 participants
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 aTimeCommandthe remainder is compared against the cached remainder. If the remainder equals the cache, only theTimeCommandis processed and parsing stops. If different, the remainder is cached viaBuffer.from(remainder). If the first command is not aTimeCommand, the cache is cleared.length < 8 || length > buffer.length)._lastTimeCommandRemainderon connect and disconnect.Public API / Type Changes
ThreadedPayloadexported from the AtemSocket module.ThreadedPayload; tests and mocks updated accordingly.Tests (src/lib/tests/atemSocket.spec.ts)
ThreadedPayloadand the normalization path.Uint8Array,{ type: 'Buffer'; data: number[] }) and invalid payload handling.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