fix(recording): write fragmented MP4 so a killed helper keeps its file - #338
Merged
Conversation
A frozen recording on Windows costs the user the whole file, and the freeze is not why. A plain MP4 has no index until IMFSinkWriter::Finalize() writes moov at the very end, so when the shutdown watchdog force-exits a wedged helper the .mp4 on disk holds every frame and no way to read them. Minutes of capture thrown away for about eight kilobytes of table. That is issues #252 / #292 / #327. A fragmented MP4 writes its index up front and its samples in self-describing moof+mdat pairs, so the same kill leaves a file that plays up to the last complete fragment. This does not fix the freeze -- the recording still stops on its own -- it stops the freeze from destroying the recording, and it does so without anyone knowing which call is stuck. MFCreateFMPEG4MediaSink wants both output media types at construction, which inverts the order the encoder was written in: it created the sink writer and described the streams afterwards with AddStream. So the AAC output type moves out of configureAudioStream into buildAacOutputType, the format guard moves with it, and createSinkWriter grows a `fragmented` switch instead of a second copy of the software-MFT registration, the hardware-transform flag and the D3D manager. The stream positions are read back off the sink by major type rather than assumed: a wrong index would aim audio writes at the video track. Two things this had to not break. The first is the fallback chain that produced the #336 regression -- all four paths stay fragmented, and a fifth attempt with the plain container runs only after every fragmented one has failed, so a machine the fragmented sink does not fit records exactly as it did before this commit rather than not recording. `container` in the encoder-selection event says which one was used, because a bug report that cannot tell them apart cannot say whether a truncated file was supposed to survive its kill. The second is ownership: MFCreateSinkWriterFromMediaSink does not take it, so releasing the writer left the sink live and the output file open, and the next attempt's MFCreateFile(DELETE_IF_EXIST) would have raced the previous attempt's own handle. releaseSinkWriter() shuts the sink down and closes the byte stream, between attempts and at finalize(). macOS gets the same property from one AVAssetWriter.movieFragmentInterval; finishWriting() still writes a normal moov on a clean stop. Linux stays plain: empty_moov makes the output permanently non-seekable and the native Linux path has no re-index step, so the editor's scrub cost has to be measured first. The helper test truncates its own output to 60% and reprobes it. That is a proxy for the kill, not a replacement -- it proves the container survives losing its tail, not that the helper flushed anything before dying.
Contributor
📝 WalkthroughWalkthroughRecording now uses one-second fragmented MP4 output on macOS and Windows. Windows retries with plain MP4 when fragmented setup fails, reports the selected container, and tests truncated output readability. ChangesFragmented MP4 recording
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CaptureHelper
participant MFEncoder
participant MP4Sink
participant EncoderEvent
CaptureHelper->>MFEncoder: Initialize recording
MFEncoder->>MP4Sink: Create one-second fragmented MP4 sink
MP4Sink-->>MFEncoder: Return sink or setup failure
MFEncoder->>MP4Sink: Retry plain MP4 when fragmented setup fails
MFEncoder->>EncoderEvent: Report selected container
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
electron/native/wgc-capture/src/mf_encoder.cpp (1)
795-850: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftThe plain-MP4 fallback forces the software encoder, so a container failure downgrades the video encoder.
configureUnfragmentedFallbackis called withforceSoftwareEncoder = trueon both exit paths (Line 817 and Line 850). Consider a machine whereMFCreateFMPEG4MediaSinkor the fragmented stream layout is unavailable, but the default hardware encoder works. Every fragmented attempt fails for a container reason, and the only plain attempt then runs withforceSoftwareEncoder = true. The recording lands on the Microsoft software H.264 encoder, reportssoftware-fallback, and shows the HUD software-encoder notice, even though the default encoder was never tried with the plain container.Try the plain container with the default encoder before falling back to the software encoder.
♻️ Proposed change to keep the encoder ladder independent of the container
- if (configureSinkWriterAttempt(true, kVideoEncoderSelectionSoftwareFallback, false, true)) { - return true; - } - return configureUnfragmentedFallback(true, kVideoEncoderSelectionSoftwareFallback); + if (configureSinkWriterAttempt(true, kVideoEncoderSelectionSoftwareFallback, false, true)) { + return true; + } + // Plain container with the default encoder first: a fragmented-sink failure + // is a container problem, and it must not decide which encoder is used. + if (configureUnfragmentedFallback(false, kVideoEncoderSelectionDefault)) { + return true; + } + return configureSinkWriterAttempt(true, kVideoEncoderSelectionSoftwareFallback, true, false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/native/wgc-capture/src/mf_encoder.cpp` around lines 795 - 850, Update the fallback flow around configureUnfragmentedFallback so a fragmented-container failure first retries the plain MP4 container with the default encoder selection and forceSoftwareEncoder=false, including after the DXGI retry path. Only invoke the software-encoder plain-container fallback after the default-encoder plain-container attempt fails, preserving the existing software fallback behavior and selection symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@electron/native/wgc-capture/src/mf_encoder.cpp`:
- Around line 795-850: Update the fallback flow around
configureUnfragmentedFallback so a fragmented-container failure first retries
the plain MP4 container with the default encoder selection and
forceSoftwareEncoder=false, including after the DXGI retry path. Only invoke the
software-encoder plain-container fallback after the default-encoder
plain-container attempt fails, preserving the existing software fallback
behavior and selection symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 052ab637-8bd8-4c44-8cfa-b1817555ebe3
📒 Files selected for processing (7)
electron/native/README.mdelectron/native/screencapturekit/Sources/OpenScreenScreenCaptureKitHelper/ScreenCaptureRecorder.swiftelectron/native/wgc-capture/src/main.cppelectron/native/wgc-capture/src/mf_encoder.cppelectron/native/wgc-capture/src/mf_encoder.hscripts/test-windows-wgc-helper.mjstechnical-documentation/architecture/recording.md
3 tasks
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
✅ The kill test passed, with the ablation. Killed 20 s into a recording, the pre-change binary leaves an unreadable file and this one leaves a file that plays to the end. Numbers in Testing.
On #252 / #292 / #327 a Windows recording freezes and the user loses the whole file. The freeze is not what causes that.
A plain MP4 has no index until
IMFSinkWriter::Finalize()writesmoovat the very end (mf_encoder.cpp). When the shutdown watchdog force-exits a wedged helper withTerminateProcess, the.mp4on disk holds every captured frame and no way to read them. Minutes of capture thrown away for ~8 KB of table.A fragmented MP4 writes its index up front and its samples in self-describing
moof+mdatpairs. The same kill leaves a file that plays up to the last complete fragment (1 s here).The reason to take this angle: it works without knowing which call is stuck. Every other lead in the investigation moves probabilities; this one moves the outcome.
Related issue
Refs #252
Refs #292
Refs #327
Type of change
Release impact
Desktop impact
Linux is deliberately out of scope — see below.
What actually changed
Windows —
MFCreateFMPEG4MediaSinkrequires both output media types at construction, which inverts the order the encoder was written in (create the sink writer, thenAddStream). So:configureAudioStreamintobuildAacOutputType, and thesampleRate/channels/blockAlignguard moves with it;createSinkWriterFromUrlbecomescreateSinkWriterwith afragmentedswitch, rather than a second copy of the software-MFT registration, the hardware-transform flag and the D3D manager;SetInputMediaType— an H.264 input type against an AAC stream sink has no encoder that can bridge it — so the failure mode here is a refused init, never silent corruption.macOS — one line:
writer.movieFragmentInterval = CMTime(seconds: 1, ...).finishWriting()still writes a normalmoovon a clean stop; the interval only matters if the process dies first.Linux — deferred, not forgotten.
frag_keyframe+empty_moovwould work, butempty_moovmakes the output permanently non-seekable and the native Linux path never passes throughreindexRecordingOnDisk(that normalization is WebM/MediaRecorder only). Nothing would put an index back. The editor's scrub cost on a long fMP4 has to be measured before this ships.Two things beyond the plan, both about not regressing
This area is the fallback chain that produced #336 last week, so:
A fifth attempt with the plain container, tried only after every fragmented attempt has failed. A machine where anything about
MFCreateFMPEG4MediaSinkdoes not work — an output type it refuses, a stream layout that does not resolve — records exactly as it does today instead of not recording. The container is the point of this PR and it is still not worth a recording.containerin theencoder-selectionevent reportsfragmented-mp4ormp4, because a bug report that cannot tell them apart cannot say whether a truncated file was supposed to survive its kill.releaseSinkWriter().MFCreateSinkWriterFromMediaSinkdoes not take ownership: releasing the writer left the fragmented sink live and the output file open, so the next attempt'sMFCreateFile(MF_OPENMODE_DELETE_IF_EXIST)would have raced the previous attempt's own handle. That alone would have broken the fallback chain this PR is required to preserve. It now shuts the sink down and closes the byte stream, both between attempts and atfinalize().The four existing fallback paths (
preferSoftwareEncoder, default, DXGI→CPU, software encoder) all stay fragmented.injectDefaultSinkWriterFailureOncestill fires exactly once, and now fires before the byte stream is created so an injected failure cannot leave the output file open.Testing
Results
/W4)mf_encoder.cpptest:wgc-helper:win, exit 0--system-audio, exit 0--software-fallback, exit 0, stillfragmented-mp4The kill test — the merge criterion
Record 20 s,
taskkill /Fthe helper by PID (the watchdog'sTerminateProcess, reproduced), then ask ffprobe and a fullffmpeg -f null -decode what survived. Same sequence against the CI build ofmain@4f9d74a4, this branch's base — that ablation is the half that makes it mean anything.Invalid data found when processing inputfragmented-mp4fragmented-mp4Both "before" rows are the bug exactly as reported: every captured frame is on disk, and none of it is reachable.
The audio row also settles the one thing this PR could not check by reading:
MFCreateFMPEG4MediaSinkaccepts the hand-built AAC output type, so noMF_MT_AUDIO_BLOCK_ALIGNMENT/MF_MT_AAC_AUDIO_PROFILE_LEVEL_INDICATIONis needed and the container does not silently fall back to plain MP4 when audio is on. Fragments also close on schedule — noCODECAPI_AVEncMPVGOPSizework needed.Nominal and fallback paths
Against the same binary, via
OPENSCREEN_WGC_CAPTURE_EXEso no MSVC is required:npm run test:wgc-helper:win,--system-audioand--software-fallbackall exit 0, including the two assertions added here:container === "fragmented-mp4", and a 60 % prefix of the output still decodes.Honest about coverage:
--software-fallbackexercises attempt 1 (fragmented, default, injected failure) → attempt 2 (fragmented, software encoder). It does not reach the DXGI→CPU retry (the GPU path is off by default) or the unfragmented last resort, which by construction only runs when every fragmented attempt has failed. Neither has been exercised on real hardware.Reproducing
Put the helper somewhere with a short path. A helper whose own
.exepath approachesMAX_PATHdies at startup withSTATUS_STACK_BUFFER_OVERRUN(0xC0000409) and no output at all — unrelated to anything here, but it will waste an afternoon. 259 characters crashed; ~250 was fine.Still open after this
The freeze itself is neither reproduced nor fixed. Also unrelated-but-adjacent and untouched here: the
--selftestprobe only checks that the file exists, the frozen-helper leak on macOS, and pinning the D3D device to the captured monitor's adapter.