Repository navigation
Fix flaky real MP4 encoder tests (MF_E_SINK_NO_SAMPLES_PROCESSED) - #966
Conversation
The real-encoder tests wrote exactly one frame before Complete(). The H.264 encoder MFT holds a multi-frame lookahead (12 frames measured with the Microsoft software encoder), so a single frame reaches IMFSinkWriter.Finalize() with nothing delivered to the MP4 sink and relies entirely on the end-of-stream drain. On some hosts that yields no sample and Finalize() throws MF_E_SINK_NO_SAMPLES_PROCESSED. Write 30 frames, well past the lookahead, so the encoder emits samples during normal input processing. Applied to the three tests that encode a single frame and then complete. Fixes #834 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Nikola Metulev (nmetulev)
added
the
agent-preparing
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The targeted build succeeded and all seven MP4 encoder tests passed with no unresolved findings.
Review effort: Balanced
Findings: None
What changed in this PR
Improves Media Foundation encoder test reliability without changing product behavior.
Changes:
- Writes 30 frames before finalization to exceed encoder lookahead.
- Reuses the helper in three real-encoder tests.
- Updates outdated one-frame coverage comments.
| File | Description |
|---|---|
Mp4SinkWriterEncoder.cs |
Updates coverage comments. |
UiCommandTests.Record.Encoder.cs |
Uses robust multi-frame encoding setup. |
Mp4SinkWriterEncoderTests.cs |
Adds the shared frame-writing helper. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Build Metrics ReportValidation passed. All required build and validation jobs succeeded. Binary Sizes
.NET Test Results (TRX reports)Other suites are reflected in the overall validation status above. ✅ 8046 passed, 37 skipped out of 8083 tests in 1131.0s (-225.0s vs. baseline) Test Coverage✅ 86.4% line coverage, 81% branch coverage · ✅ no change vs. baseline CLI Startup Time52ms median (x64, Try This BuildInstalls the MSIX for your architecture, replacing any previously installed build. Needs the GitHub CLI — the command offers to install it and sign you in if it is missing. & ([scriptblock]::Create((irm https://raw.githubusercontent.com/microsoft/winappCli/main/scripts/winapp-pr.ps1))) 966Switching between builds often?Put the tool on your PATH once: & ([scriptblock]::Create((irm https://raw.githubusercontent.com/microsoft/winappCli/main/scripts/winapp-pr.ps1))) -AddToPathThen this build is just: winapp-pr 966Run Updated 2026-10-06 03:28:48 UTC · commit |
Nikola Metulev (nmetulev)
added
ready-for-review
Zach Teutsch (zateutsch)
left a comment
Zach Teutsch (zateutsch)
left a comment
There was a problem hiding this comment.
🤖 AI-generated review (winappcli pr-review skill) — verify before acting.
The core fix is right: writing past the encoder lookahead so Finalize() doesn't depend on the end-of-stream drain is the correct fix for #834, and the GetStatistics measurements justify 30 frames. Build is clean.
One change requested: the longer write loop widens a pre-existing cross-test race on the process-wide s_testPublishAtomic seam in MoveFails_TempFileCleanedUp (inline note). It's a one-line move, and it keeps this de-flaking PR from introducing a new flake vector. Everything else looks good to merge once that's addressed.
s_testPublishAtomic is process-wide, and real-encoder tests in other classes can call Complete() in parallel. Setting it after the 30-frame write loop keeps the throwing seam active only for the Complete() call that needs it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Nikola Metulev (nmetulev)
added
agent-preparing
Nikola Metulev (nmetulev)
added
ready-for-review
Nikola Metulev (nmetulev)
added
agent-preparing
Nikola Metulev (nmetulev)
added
ready-for-review
Nikola Metulev (nmetulev)
added
ready-for-review
Description
Mp4SinkWriterEncoder_RealEncoderCoversValidationAndSuccessfulCompleteintermittently failed in CI withCOMException 0xC00D4A44(MF_E_SINK_NO_SAMPLES_PROCESSED) fromIMFSinkWriter.Finalize().The test wrote exactly one frame to a real Media Foundation H.264 encoder and then called
Complete(). The encoder holds a lookahead of input frames before it emits anything, so a single frame reachesFinalize()with nothing delivered to the MP4 sink, and the test depended entirely on the encoder's end-of-stream drain producing a sample. On some hosts it doesn't.Measured locally with
IMFSinkWriter.GetStatistics(Microsoft software H.264 encoder, 64x64 @ 1 fps, stats read just beforeFinalize()):Finalize()The tests now write 30 frames (well past the 12-frame lookahead) through a small shared helper, so the encoder emits samples during normal input processing and
Finalize()no longer depends on the drain. Everything the test covered is preserved: the short-bufferArgumentException, idempotent secondComplete(), and the published-file assertions.The same one-frame-then-
Complete()pattern was in twoUiCommandTestsencoder tests (MoveFails_TempFileCleanedUp,NoClobberRacePreservesLateDestinationAndCleansTemp). They expect anIOExceptionthrown afterFinalize()succeeds, so they had the same latent flake and use the same helper.No product behavior changes.
UiRecordingServicealready treats a failingencoder.Complete()as a recoverable video-output failure. The only production-file edits are two doc-comment wording updates that referred to "one-frame" coverage.I chose this over treating
0xC00D4A44as inconclusive, because that would hollow out the assertion that a completed MP4 is published. I also didn't add a product-side statistics accessor to poll the sink, since it would add surface only for tests.Related Issue
Fixes #834
Type of Change
Checklist
Additional Notes
Limitation: I couldn't reproduce the original failure locally. 1,000 single-frame encodes at 32-way parallelism with every core saturated all passed on an ARM64 host, while CI is x64. The fix targets the mechanism the measurements show (no samples reach the sink before
Finalize()for one frame) rather than a reproduced failure.Validation: 20 consecutive runs of
dotnet run --project src/winapp-CLI/WinApp.Cli.Tests/WinApp.Cli.Tests.csproj -c Debug --no-build -- --filter "FullyQualifiedName~Mp4SinkWriterEncoder"passed 7/7 every run, with 0 skipped or inconclusive.