Windows: report why an export failed instead of "channel closed" - #184
Draft
ruccho wants to merge 8 commits into
Draft
Windows: report why an export failed instead of "channel closed"#184ruccho wants to merge 8 commits into
ruccho wants to merge 8 commits into
Conversation
The Windows backend runs its stream sinks and async MFTs on spawned loops whose Result was discarded. When one of those loops failed, the only trace left was a closed channel, so an export reported "Failed to complete muxer: channel closed" no matter which HRESULT actually stopped it, and an encoder that died mid-stream looked like a short recording. - The sink stream loop now sends its error down the finish channel it was going to drop, so the muxer task fails with the HRESULT rather than with a RecvError. - Where the failing channel points the wrong way or has more than one observer, the loop leaves its error in a shared `ErrorSlot` that whoever notices the closure reads. That covers `MuxerInput::push`, `Transform::push`, and `EncoderOutput::pull`. - The muxer task hands its error to anyone still waiting in `LazyStream::get` instead of dropping the sender. - `WindowsError::OneshotRecv` is no longer transparent: `RecvError` renders as a bare "channel closed", which was the string being reported. - Handle MF_E_TRANSFORM_STREAM_CHANGE by accepting the MFT's new output type and asking for output again. Intel's Quick Sync H.264 encoder reports it on its very first output, which killed the encoder loop outright on machines where that MFT is selected. - The BeginGetEvent callback no longer panics when the awaiting task is already gone; Media Foundation invokes it on a thread we do not own, and the abort-on-panic profile turned that into a process exit. - ProcessOutput failures that are not MF_E_TRANSFORM_NEED_MORE_INPUT now end the call instead of spinning on the same failing call forever. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both sides rewrote the Windows backend's event loops: native-logging swapped their `println!` calls for the `log` facade, while this branch restructured them so a failing loop reports its HRESULT instead of dropping a channel. Resolved by keeping this branch's structure and routing every one of its diagnostics — including the two new "loop failed" reports — through `log`. Those reports are the reason the merge is worth having: until now the Windows backend printed to stdout, which the Unity editor does not capture, so none of this reached the log a user can actually send us. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ruccho
marked this pull request as ready for review
September 1, 2026 05:51
Placing an end-of-segment marker is how a stream sink is told a segment ended, and the sink answers with MEStreamSinkMarker once it has consumed everything placed before it. That answer was being discarded as an unhandled event, and the stream was instead reported as drained the moment the marker was placed — so the marker was being paid for without being used, and finalization started without ever learning that the sink had caught up. Worse, the sink asks for samples ahead of consuming them, so sample requests are still queued once the segment has ended. Every one of them drove the loop into the drained branch again and placed another marker. Instrumenting the calls showed two markers per stream, and the second one on the audio stream landing after BeginFinalize had been called: a marker placed on a sink that is already finalizing. Nothing synchronises the stream loops against finalization, so which side of it that second marker falls on is timing. The loop now places the marker once, ignores the sample requests that follow it, and reports the stream drained when MEStreamSinkMarker arrives. Failure to place the marker stays non-fatal. No marker event follows a failed PlaceMarker, so that path reports the stream drained on the strength of ProcessSample having returned for every sample, which is what the previous behaviour rested on for every export. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ruccho
marked this pull request as draft
September 1, 2026 06:45
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
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.
Stacked on #174. Follows up #156.
What
The Windows backend drives its media sink streams and its async MFTs from spawned loops whose
Resultwas thrown away. A Media Foundation failure inside one of those loops left no traceexcept a closed channel, and a closed channel carries no HRESULT. So every failure — whichever
call actually returned an error — reached the caller as the same sentence:
Reports of an export that produced an unplayable file, or never completed, could not be taken
any further than that: the message names the channel that noticed, never the call that failed.
That is what this PR is mostly about. It also fixes two failures found while working on it, one
of which stops the encoder outright on some machines.
Reporting the error that actually happened
Where a channel points the right way and has one waiter, the error travels down it. The sink
stream loop's finish signal is now
oneshot::Sender<Result<()>>rather thanoneshot::Sender<()>: a loop that fails sends its error instead of dropping the sender, and themuxer task fails with that HRESULT rather than with a
RecvError. This is the main path — it iswhat
CompleteAsyncreports.The remaining cases have no channel to use. The loop that dies is the receiver of the sample
channel, so it cannot answer its own producer; and one loop's death is observed by two different
objects, driven by two different tasks (
Transform::pushseessample_txclose,EncoderOutput::pullseesoutput_rxclose). Both observers already know the loop is gone —they need to look up why, not wait for it. So the loop leaves its error in a small shared
ErrorSlotthat whoever notices the closure reads, and the write is ordered before the channelsdrop so an observer can never see a closed channel with an empty slot.
The muxer task also hands its error to anyone still waiting in
LazyStream::getinstead ofdropping the sender, and
WindowsError::OneshotRecvis no longer#[error(transparent)]—RecvErrorrenders as a bare "channel closed", which is where that string was coming from.MF_E_TRANSFORM_STREAM_CHANGE
Running the e2e harness on a machine whose selected H.264 MFT is Intel's Quick Sync encoder
reproduced the class of failure above, and with the error reporting in place it named itself:
process_outputdid not handle it. That MFT reports it on its very first output, before anysample, and produces nothing at all until the new output type is accepted — so the encoder loop
died on the first frame and the export failed with "channel closed".
process_outputnowaccepts the type the MFT switched to, refreshes the cached
MFT_OUTPUT_STREAM_INFO(the newtype can change the buffer we are expected to supply) and asks for output again, in the async
loop, the sync
push, and the drain inDrop.The sink is still described by the type the encoder announced up front — the MPEG-4 sink cannot
be told a different one after it is created — so this restores the encoder rather than changing
what the container says. In practice the harness's verification of the output is unchanged.
The end-of-segment marker was placed but never used
Placing an end-of-segment marker is how a stream sink is told a segment ended, and the sink
answers with
MEStreamSinkMarkeronce it has consumed everything placed before it. That answerwas being discarded as an unhandled event, and the stream was reported as drained the moment the
marker was placed — so the marker was paid for without being used, and finalization started
without ever learning the sink had caught up.
The sink also asks for samples ahead of consuming them, so sample requests are still queued once
the segment has ended, and every one of them drove the loop into the drained branch again and
placed another marker. Instrumenting the calls showed two markers per stream, and the second one
on the audio stream landing after
BeginFinalizehad been called:Nothing synchronises the stream loops against finalization — they run until the sink is shut
down — so which side of it that second marker falls on is timing. On the build tested it returns
Okand finalization completes anyway; a sink is under no obligation to be that forgiving, and"finalization never returns" is the shape of failure a disturbed finalization state machine would
produce.
The loop now places the marker once, ignores the sample requests that follow it, and reports the
stream drained when
MEStreamSinkMarkerarrives. Both streams are therefore known to be drainedbefore
BeginFinalizeis called, which is what makes finalization safe to start at all.Failure to place the marker stays non-fatal — the case #156 was about. No marker event follows a
failed
PlaceMarker, so that path reports the stream drained on the strength ofProcessSamplehaving returned for every sample, which is what the previous behaviour rested on for every
export. It also means a build that rejects the marker still reaches finalization, as it does
today.
Two smaller fixes
A panic on a thread we do not own. The
BeginGetEventcallback didtx.send(..).unwrap().Media Foundation invokes it on one of its own work queue threads and may do so after the
awaiting task is gone, and with
panic = "abort"that ends the host process. The closedreceiver simply means nobody is listening, so it is no longer an error.
A spin on a failing call.
ProcessOutputerrors that were notMF_E_TRANSFORM_NEED_MORE_INPUTfell through thematchand looped, calling the same failingProcessOutputforever. They now end the call.Logging
With the
logfacade from the branch below, the backend can finally say where an export got to.At info, one line per milestone:
Those two finalization lines are the ones worth having. Finalization is where the
moovbox iswritten, and it is the only stage that seeks back over a file the process has just finished
appending to — telling "stopped before it" from "stopped inside it" previously needed a
debugger.
At debug: the media type each track is described with — subtypes rendered as their fourcc or
WAVE format tag rather than a raw GUID, plus whether a sequence header is present — and sink
events by name instead of by number (
MEStreamSinkMarker (306)was being reported as"unhandled" when it is the normal answer to
PlaceMarker). Loop failures and failed MFTactivations log at error and warn, the latter now naming the MFT that failed.
A media sink that is not finalizable now warns. Behaviour is unchanged, but the export would
otherwise report success while producing a file with no
moovbox.Testing
cargo fmt --check,cargo clippy(no new warnings), andcargo test -p unienc_testkit -p unienc_common -p unienc_windows_mfpass;cargo build -p unienc_c --features unity --releasebuilds the shipped configuration.The e2e harness now passes on the hardware encoder path that previously failed on the first
frame, and verifies the output it produced.
Error propagation was checked by injecting a failure at three points and confirming what the
caller is told, where each case previously read "channel closed":
ProcessSampleCompleteAsyncThe marker handshake was checked by logging every
PlaceMarkercall with its return value andwhether the stream had already been reported drained — which is where the second call and its
position relative to
BeginFinalizecame from. After the change the log shows one marker perstream, the leftover sample requests ignored, and both streams drained before finalization
starts.
On the log level
#174 sets the release default to
Warn, so the milestone trail above does not appear until thelevel is raised to info. That is deliberate: these lines report an export going well, and warn is
the wrong level to say so on every successful export. Asking for the level to be raised is a step
in reproducing a problem, and #174 makes it one — the level is settable from the editor and baked
into player builds.
What a user sees without touching the level is unchanged in kind and better in content: failures
still report at warn and error, and they now name the Media Foundation call that failed.