Skip to content

fix: emit JPEG frames for default video streams - #94

Closed
SunsetWan wants to merge 1 commit into
lycorp-jp:mainfrom
SunsetWan:codex/fix-mjpeg-jpeg-payload
Closed

SunsetWan wants to merge 1 commit into
lycorp-jp:mainfrom
SunsetWan:codex/fix-mjpeg-jpeg-payload

Conversation

@SunsetWan

Copy link
Copy Markdown
Contributor

Summary

  • ensure screenshot-backed streams encode PNG captures as JPEG at the default scale=1.0 / quality=80 settings
  • preserve byte-for-byte passthrough when the input is already JPEG
  • add shared regression coverage plus iOS and Android MJPEG E2E assertions for MIME, Content-Length, and JPEG magic bytes
  • document the fix under Unreleased

Root cause

Both the iOS screenshot API and Android screencap -p produce PNG data. The shared frame processor treated the default scale and quality as an unconditional passthrough fast path, while both MJPEG writers labeled every frame as image/jpeg. This produced PNG payloads with a JPEG MIME type.

The fast path now checks the source container with ImageIO and only passes through actual JPEG input. PNG captures are re-encoded as JPEG. The shared raw and ffmpeg screenshot-backed formats now also consistently emit the JPEG frames their command descriptions promise.

User impact

Strict MJPEG clients no longer receive PNG frames mislabeled as JPEG at default settings. Non-default quality and scale behavior is unchanged.

Validation

  • swift test --filter VideoFrameProcessingTests: 9 passed
  • make build: passed
  • Standards/spec review: passed
  • make test: all 306 tests passed, then the Swift test process aborted during teardown with freed pointer was not the last allocation; the same post-test abort reproduces on unchanged origin/main
  • Device E2E not run locally: no booted iOS simulator and no adb on PATH

Signed-off-by: wanchenxi <wanchenxi@hikvision.com.cn>
@SunsetWan
SunsetWan marked this pull request as ready for review August 10, 2026 12:32
@onevcat

onevcat commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Thanks for digging into this — the bug is real, and the MJPEG frame parser in Tests/MJPEGTestSupport.swift is a genuine improvement over the old "does stdout contain the string image/jpeg" assertion. That weak assertion is exactly why this slipped through.

Before merging, though, I'd like to take a different route to the fix, because the transcode this adds is avoidable.

The capture is PNG only because we ask for PNG. VideoFrameUtilities.captureScreenshotData hardcodes format: .png, but FBSimulatorControl supports JPEG natively:

// FBSimulatorControl/Commands/FBSimulatorScreenshotCommands.swift
if format == .jpeg { return try image.jpegImageData() }
else if format == .png { return try image.pngImageData() }

Both paths encode the same framebuffer CGImage. So the codec cost per frame is:

approach passes
today (buggy) PNG encode
this PR PNG encode → PNG decode → JPEG encode
ask the framebuffer for what we want one encode

Going from one codec pass to three on the default streaming path is a steep price for a MIME label. record-video pays it twice over, since it immediately decodes the PNG back into a CGImage anyway.

On the premise that JPEG is the right target at all: I measured 12 real UI screenshots (PNG original vs sips JPEG q80). Median ratio was ~1.07x, and in 5 of 12 cases the PNG was smaller. Flat UI — solid fills, system type, sharp edges — is where PNG's row filtering wins and JPEG's DCT struggles. The "JPEG is 10x smaller" intuition comes from photographs, not app screenshots. JPEG's remaining justifications here are protocol conformance (ffmpeg's mjpeg demuxer splits frames on FFD8/FFD9; browsers and VLC expect it) and the fact that --quality is only meaningful for a lossy codec. Neither applies to --format raw, which is self-delimiting via its 4-byte length prefix and can carry PNG untouched.

So the direction we'll take:

  1. Capture the framebuffer CGImage directly and apply --scale and --quality in a single encode. Note that jpegImageData() passes nil properties, so it never sets kCGImageDestinationLossyCompressionQuality — reusing takeScreenshot(format: .jpeg) as-is would silently drop --quality.
  2. Let --format raw pass PNG through with no transcode.
  3. Keep mjpeg / ffmpeg on JPEG for protocol conformance, sourced as JPEG rather than transcoded into it.
  4. Android's screencap has no JPEG option, so one host-side encode is unavoidable there for the JPEG formats; a device-side JPEG endpoint in the bridge is separate follow-up work.

I'll open a PR for this and carry over your MJPEGTestSupport.swift parser and the magic-byte assertions, with credit to you in the changelog. Closing this one in favour of that — the diagnosis here was the hard part, and it was correct. Thank you!

@onevcat

onevcat commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

@SunsetWan a follow-up, since I owe you an update on what I said earlier.

I opened #130 along the lines described above, then closed it. Measuring the alternative changed the plan: native H.264 streaming through idb's createStream runs at ~24 fps versus this path's ~4.1, at 1/8th the bytes per second, on the same device. So stream-video is moving to H.264 as its single capture path and the screenshot-backed formats — mjpeg, raw, ffmpeg — are being deprecated. Hardening them now would be work on a path that is going away. Plan is in #132.

I want to be clear that this does not diminish your contribution. Your diagnosis was correct, and chasing it is what surfaced the real problem: nobody had asked why the capture was PNG in the first place, and the answer turned out to be that we had hardcoded format: .png while never touching the three native stream formats idb has had all along. That is what redirected this. The weak assertion your PR replaced — a substring check for image/jpeg that never fed the stream to a real consumer — is also exactly why the bug shipped, and Tests/MJPEGTestSupport.swift is being kept for the H.264 work.

Thank you for the report and for the patience while the scope moved.

@onevcat

onevcat commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

A better, and full support for correct streaming feature was applied in #138

I am closing this now.

@SunsetWan Thank you for placing this out and opening the PR. It raised the issue and we can get a final improvement! Nice work!

@onevcat onevcat closed this Sep 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants