refactor: Isolate MJPEG helpers in the driver - #2915
Conversation
…chdog axios's `timeout` option applied to the whole request lifetime, not just the connect phase, so it raced against MJpegStream's own "no frame received yet" watchdog on slow CI runners, occasionally surfacing a bare 'aborted' error instead of the intended "never sent any images" message. Bound the connect phase with its own AbortController instead, cleared as soon as axios resolves. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR removes the xcuitest-driver’s runtime dependency on deprecated MJPEG helpers from @appium/support by porting the MJPEG stream implementation into the driver, along with moving MJPEG session option handling into standalone helper functions.
Changes:
- Added a new
MJpegStreamimplementation (plus a small multipart/JPEG frame parser) underlib/commands/helpers/mjpeg.ts, and refactored MJPEG option handling into exported helper functions. - Updated
lib/driver.tsto use the new helpers instead ofappium/support’s MJPEG export. - Added unit tests for
MJpegStream,allocateMjpegServerPort, andhandleMjpegOptions, and adjustedsharpto be an optional dependency.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/commands/helpers/mjpeg.spec.ts | Adds unit coverage for the new MJPEG helpers and stream behavior. |
| package.json | Moves sharp to optionalDependencies to support runtime lazy-loading in the new MJPEG helper. |
| lib/driver.ts | Switches driver session setup to call the new handleMjpegOptions(this) helper and updates the mjpegStream type. |
| lib/commands/helpers/mjpeg.ts | Introduces the new MJpegStream, a vendored MJPEG frame parser, and exported MJPEG helper functions. |
| lib/commands/helpers/index.ts | Re-exports handleMjpegOptions and the MJpegStream type from the helpers barrel. |
Comments suppressed due to low confidence (4)
lib/commands/helpers/mjpeg.ts:73
- In MjpegFrameParser.appendChunk, the completion check uses
this.bytesWritten === this.expectedLength. IfbytesWrittenever exceeds the declared length (e.g., due to chunk boundaries/markers), the frame will never be emitted. Using>=is safer and avoids getting stuck mid-frame.
if (end > -1 || this.bytesWritten === this.expectedLength) {
lib/commands/helpers/mjpeg.ts:85
- MjpegFrameParser.emitFrame() currently pushes the entire preallocated buffer (which may include trailing zero padding) and never clears
this.buffer/ counters afterwards. This can yield invalid JPEG data and can also cause subsequent chunks to be appended to an already-emitted buffer. Emit only the bytes written and reset parser state after pushing.
this.isReading = false;
if (this.buffer) {
this.push(this.buffer);
}
}
lib/commands/helpers/mjpeg.ts:252
- MJpegStream overrides
write()but never calls the provided callback. If any caller writes with a callback (or if stream internals rely on the standard Writable implementation), this can lead to hangs and bypasses backpressure/error semantics. Implement_write()instead and always invokecallback.
override write(
lib/commands/helpers/mjpeg.ts:192
- In the axios connection error path,
JSON.stringify(error.response)can throw (Axios response objects can include circular references viarequest/config). If that happens, the code will throw aTypeErrorwhile trying to build the error message, masking the real failure. Prefer extracting safe fields (status/statusText) instead of stringifying the whole response.
if (e && typeof e === 'object' && 'response' in e) {
message = JSON.stringify((e as {response: unknown}).response);
} else if (e instanceof Error) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- MjpegFrameParser.appendChunk now anchors the copy at the JPEG SOI marker whenever one is present in the chunk, even if the matching EOI hasn't arrived yet, instead of silently falling back to offset 0 and risking multipart boundary/header bytes leaking into the frame buffer. - Keep sharp in devDependencies (in addition to optionalDependencies) since unit tests import it directly and must not depend on optional-dependency install behavior. - Rewrote the "keep track of newer frames" test to assert on observable behavior (lastChunkBase64 reflecting a second, distinct JPEG payload) instead of reaching into MJpegStream's private updateCount field. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
lib/commands/helpers/mjpeg.ts:72
- MjpegFrameParser.appendChunk increments bytesWritten by (copyEnd - copyStart) rather than the actual number of bytes copied. If the destination buffer has less remaining capacity than the source range, Buffer.copy truncates but bytesWritten is still advanced too far, so subsequent chunk data can be dropped and the emitted JPEG can be incomplete.
const copyStart = start > -1 ? start : 0;
const copyEnd = end > -1 ? end + JPEG_EOI.length : chunk.length;
chunk.copy(this.buffer, this.bytesWritten, copyStart, copyEnd);
this.bytesWritten += copyEnd - copyStart;
lib/commands/helpers/mjpeg.ts:85
- MjpegFrameParser.emitFrame() pushes the current buffer but never resets buffer/expectedLength/bytesWritten. This leaves stale state behind, so a subsequent chunk containing a new SOI (but not a Content-Length header) can be appended into the previous frame buffer and corrupt output.
private emitFrame(): void {
this.isReading = false;
if (this.buffer) {
this.push(this.buffer);
}
}
- MJpegStream now overrides Writable._write() and invokes the completion callback instead of overriding the public write() method, which bypassed Node's writable buffering/backpressure/callback machinery relied on by pipe(). - MjpegFrameParser.startFrame/appendChunk now derive bytesWritten from Buffer.copy()'s own return value instead of the requested source-range size, since Buffer.copy() silently truncates when the destination has less room than requested. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
🎉 This PR is included in version 12.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
@appium/support's mjpeg and sharp-backed image-util helpers are deprecated and slated for removal from Appium core ("Consumers are expected to implement MJpegStream class on their side"). This PR ports the MJpegStream implementation into xcuitest-driver itself so the driver no longer depends on it.