[PATCH 0/1] MR9716: mf: Don't crash if stream is null during sample request.
JR East Train Simulator's sink sometimes requests a sample before the stream is added. This would then lead to a crash. If no stream is present, buffer sample requests until a stream is added (This is necessary to prevent swallowing requests). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9716
From: Charlotte Pabst <cpabst(a)codeweavers.com> --- dlls/mf/session.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/dlls/mf/session.c b/dlls/mf/session.c index 8d2ca079634..ce77dbbcded 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -207,6 +207,7 @@ struct topo_node { IMFMediaSource *source; DWORD stream_id; + unsigned int init_requests; } source; struct { @@ -3010,6 +3011,7 @@ static HRESULT session_add_media_stream(struct media_session *session, IMFMediaS IMFStreamDescriptor *sd; DWORD stream_id = 0; HRESULT hr; + unsigned int i; if (FAILED(hr = IMFMediaStream_GetStreamDescriptor(stream, &sd))) return hr; @@ -3032,6 +3034,13 @@ static HRESULT session_add_media_stream(struct media_session *session, IMFMediaS node->object.source_stream = stream; IMFMediaStream_AddRef(node->object.source_stream); + + for (i = 0; i < node->u.source.init_requests; ++i) + { + if (FAILED(hr = IMFMediaStream_RequestSample(node->object.source_stream, NULL))) + WARN("Sample request failed, hr %#lx.\n", hr); + } + node->u.source.init_requests = 0; break; } } @@ -3968,7 +3977,9 @@ static HRESULT session_request_sample_from_node(struct media_session *session, s switch (topo_node->type) { case MF_TOPOLOGY_SOURCESTREAM_NODE: - if (FAILED(hr = IMFMediaStream_RequestSample(topo_node->object.source_stream, NULL))) + if (!topo_node->object.source_stream) + ++topo_node->u.source.init_requests; + else if (FAILED(hr = IMFMediaStream_RequestSample(topo_node->object.source_stream, NULL))) WARN("Sample request failed, hr %#lx.\n", hr); break; case MF_TOPOLOGY_TRANSFORM_NODE: -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9716
There is an order in which objects are created. When starting the pipeline we should be waiting for sources to start, and that implies creating streams. Sink should only request after clock changes, that comes later. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9716#note_125092
It might very well be API misuse that only functions on windows by chance. We're talking about a custom sink implementation here. Seems like after calling IMFMediaSession::Start, all the application does is wait until the sink's media type handler obtains a media type (SetCurrentMediaType) before queueing the MEStreamSinkRequestSample event (not sure if that wait is even related to this problem). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9716#note_125093
On Sat Dec 6 16:45:01 2025 +0000, Charlotte Pabst wrote:
It might very well be API misuse that only functions on windows by chance. We're talking about a custom sink implementation here. Seems like after calling IMFMediaSession::Start, all the application does is wait until the sink's media type handler obtains a media type (SetCurrentMediaType) before queueing the MEStreamSinkRequestSample event (not sure if that wait is even related to this problem). Which SetCurrentMediaType in particular? I think we normally use it for SetTopology, which is before we start listening to node events. Anyway, this needs some manual test program to see what happens, I don't trust this change as is, my first choice would be to drop requests like that.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9716#note_125094
Which SetCurrentMediaType in particular?
Specifically, it waits until IMFMediaTypeHandler::GetCurrentMediaType (on the handler associated with the sink) returns a non-null type. The code looks roughly like this: ``` PROPVARIANT pv = { 0 }; IMFMediaType *type = NULL; IMFMediaSession_Start(session, NULL, &pv); while (type == NULL) { IMFMediaTypeHandler_GetCurrentMediaType(sink->media_type_handler, &type); WaitForSingleObject(sample_ready, 1); } ResetEvent(sample_ready); IMFStreamSink_QueueEvent(&sink->iface, MEStreamSinkRequestSample, &NULLGUID, S_OK, NULL); if (WAIT_TIMEOUT == WaitForSingleObject(sample_ready, 1000)) fatal_failure(); ``` Yes, I know a lot of this is quite nonsensical. The code quality isn't great.
my first choice would be to drop requests like that.
Mine too, I've tried dropping the requests and it crashes the game as well because it now times out waiting for the requested sample. I'll write a test. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9716#note_125095
On Sat Dec 6 17:08:49 2025 +0000, Charlotte Pabst wrote:
Which SetCurrentMediaType in particular? Specifically, it waits until IMFMediaTypeHandler::GetCurrentMediaType (on the handler associated with the sink) returns a non-null type. The code looks roughly like this:
PROPVARIANT pv = { 0 }; IMFMediaType *type = NULL; IMFMediaSession_Start(session, NULL, &pv); while (type == NULL) { IMFMediaTypeHandler_GetCurrentMediaType(sink->media_type_handler, &type); WaitForSingleObject(sample_ready, 1); } ResetEvent(sample_ready); IMFStreamSink_QueueEvent(&sink->iface, MEStreamSinkRequestSample, &NULLGUID, S_OK, NULL); if (WAIT_TIMEOUT == WaitForSingleObject(sample_ready, 1000)) fatal_failure();Yes, I know a lot of this is quite nonsensical. The code quality isn't great.
my first choice would be to drop requests like that. Mine too, I've tried dropping the requests and it crashes the game as well because it now times out waiting for the requested sample. I'll write a test. Just a standalone test program is fine, no need to integrate that to wine tests.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9716#note_125096
On Sat Dec 6 17:25:44 2025 +0000, Nikolay Sivov wrote:
Just a standalone test program is fine, no need to integrate that to wine tests. Media Session on Windows seems to have only one sample request (per stream) in flight at a time. So it must have a mechanism for keeping track of the number of requests it has received from a particular sink.
In Wine, we currently have [`requests`](https://gitlab.winehq.org/wine/wine/-/blob/5375446b5c6398adaf6d0ee1b8f3564ba...) for each sink to keep track of that. I wonder if we can't just increment that if we know we're not up to the point where we have started the sources, and then make the first sample request once the source is started. We do similar in [`session_flush_sinks`](https://gitlab.winehq.org/wine/wine/-/blob/5375446b5c6398adaf6d0ee1b8f3564ba...). That is we will send a sample request if one is pending. Although I think the decrement there is a bug (as it should only be decremented once the sample is delivered to the sink). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9716#note_125120
On Mon Dec 8 01:20:03 2025 +0000, Brendan McGrath wrote:
Media Session on Windows seems to have only one sample request (per stream) in flight at a time. So it must have a mechanism for keeping track of the number of requests it has received from a particular sink. In Wine, we currently have [`requests`](https://gitlab.winehq.org/wine/wine/-/blob/5375446b5c6398adaf6d0ee1b8f3564ba...) for each sink to keep track of that. I wonder if we can't just increment that if we know we're not up to the point where we have started the sources, and then make the first sample request once the source is started. We do similar in [`session_flush_sinks`](https://gitlab.winehq.org/wine/wine/-/blob/5375446b5c6398adaf6d0ee1b8f3564ba...). That is we will send a sample request if one is pending. Although I think the decrement there is a bug (as it should only be decremented once the sample is delivered to the sink). Actually, I had a test app already where it was easy for me to modify and test this scenario. It looks like Windows doesn't read from the sink event queue until after it has received the `MENewStream` events. Here's the relevant lines from my test apps log file:
1158.448380|05904|sink_mock_QueueEvent: 305 (MEStreamSinkRequestSample) {00000000-0000-0000-0000-000000000000} (GUID_NULL) 0 <EMPTY>
1158.448966|13744|source_mock_BeginGetEvent:
1158.449218|05904|source_mock_QueueEvent: 205 (MENewStream) {00000000-0000-0000-0000-000000000000} (GUID_NULL) 0 0000019EEF766B08
1158.449235|05904|source_mock_QueueEvent: 201 (MESourceStarted) {00000000-0000-0000-0000-000000000000} (GUID_NULL) 0 0 (20)
1158.449312|13744|source_mock_EndGetEvent: hr: 0, 205 (MENewStream) {00000000-0000-0000-0000-000000000000} (GUID_NULL) 0 0000019EEF766B08
1158.449323|13744|source_mock_BeginGetEvent:
1158.449350|13744|source_mock_EndGetEvent: hr: 0, 201 (MESourceStarted) {00000000-0000-0000-0000-000000000000} (GUID_NULL) 0 0 (20)
1158.449383|13744|source_mock_BeginGetEvent:
1158.449597|05904|sink_mock_BeginGetEvent:
1158.449636|05904|sink_mock_EndGetEvent: hr: 0, 305 (MEStreamSinkRequestSample) {00000000-0000-0000-0000-000000000000} (GUID_NULL) 0 <EMPTY>
You can see the `MEStreamSinkRequestSample` is queued first, but `sink_mock_BeginGetEvent` isn't called until after `source_mock_EndGetEvent` with `MESourceStarted`. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9716#note_125125
Actually, I had a test app already where it was easy for me to modify and test this scenario. It looks like Windows doesn't read from the sink event queue until after it has received the `MENewStream` events
I think that makes more sense. Is it clear that those are ordered steps, e.g. if you never send MENewStream does it ever start listening to the sink events?
The other thing I wanted to note (which may no longer be relevant) is that we would need to send the pending sample requests in to the very last transform (assuming there is one) rather than the source itself, as the transform may need multiple samples from the source to provide the sink with a single sample (and the number of samples provided to the sink should match the number requested). An example would be a H.264 decoder which might receive samples out of presentation order (such as when 'B-frames' are present)
Yes, that's another reason why proposed change is wrong I think. One more complicated thing to try is 2-input transform with two sources, feeding into a sink. Should it wait until all streams are created? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9716#note_125133
Is it clear that those are ordered steps, e.g. if you never send MENewStream does it ever start listening to the sink events?
Seems to be. I suppressed the `MENewStream` event and the program just halted after the `source_mock_BeginGetEvent` call. ``` 2064.717368|10700|sink_mock_QueueEvent: 305 (MEStreamSinkRequestSample) {00000000-0000-0000-0000-000000000000} (GUID_NULL) 0 <EMPTY> 2064.727559|10700|source_mock_BeginGetEvent: 2064.732447|01496|source_mock_QueueEvent: 201 (MESourceStarted) {00000000-0000-0000-0000-000000000000} (GUID_NULL) 0 0 (20) 2064.732690|10700|source_mock_EndGetEvent: hr: 0, 201 (MESourceStarted) {00000000-0000-0000-0000-000000000000} (GUID_NULL) 0 0 (20) 2064.732770|10700|source_mock_BeginGetEvent: ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9716#note_125249
On Mon Dec 8 23:30:23 2025 +0000, Brendan McGrath wrote:
Is it clear that those are ordered steps, e.g. if you never send MENewStream does it ever start listening to the sink events? Seems to be. I suppressed the `MENewStream` event and the program just halted after the `source_mock_BeginGetEvent` call.
2064.717368|10700|sink_mock_QueueEvent: 305 (MEStreamSinkRequestSample) {00000000-0000-0000-0000-000000000000} (GUID_NULL) 0 <EMPTY> 2064.727559|10700|source_mock_BeginGetEvent: 2064.732447|01496|source_mock_QueueEvent: 201 (MESourceStarted) {00000000-0000-0000-0000-000000000000} (GUID_NULL) 0 0 (20) 2064.732690|10700|source_mock_EndGetEvent: hr: 0, 201 (MESourceStarted) {00000000-0000-0000-0000-000000000000} (GUID_NULL) 0 0 (20) 2064.732770|10700|source_mock_BeginGetEvent:Actually, it looks like Windows might wait for the `MEStreamStarted` event (as suppressing that is enough to halt the test program).
@CharlottePabst FWIW - I'm hitting this issue too with my scrubbing work. Here's the work-around I have at the moment: https://gitlab.winehq.org/redmcg/wine/-/commit/c96938c642ceabe564487664816b2... Windows might have a lot more nuance. For example: it might wait for the `MEStreamStarted` event and then only subscribe to the sink(s) to which it connects at the end of the attached pipeline(s). Edit: Here's the relevant output from the new test: ``` 1465.043300|03640|sink_mock_QueueEvent: 305 (MEStreamSinkRequestSample) {00000000-0000-0000-0000-000000000000} (GUID_NULL) 0 <EMPTY> 1465.054398|04532|source_mock_BeginGetEvent: 1465.054679|04532|source_mock_QueueEvent: 205 (MENewStream) {00000000-0000-0000-0000-000000000000} (GUID_NULL) 0 00000216581C7A18 1465.054908|04532|source_mock_QueueEvent: 201 (MESourceStarted) {00000000-0000-0000-0000-000000000000} (GUID_NULL) 0 <EMPTY> 1465.055050|03640|source_mock_EndGetEvent: hr: 0, 205 (MENewStream) {00000000-0000-0000-0000-000000000000} (GUID_NULL) 0 00000216581C7A18 1465.055141|03640|stream_mock_BeginGetEvent: 1465.055216|03640|source_mock_BeginGetEvent: 1465.055282|03640|source_mock_EndGetEvent: hr: 0, 201 (MESourceStarted) {00000000-0000-0000-0000-000000000000} (GUID_NULL) 0 <EMPTY> 1465.055365|03640|source_mock_BeginGetEvent: ``` So it subscribes to the new stream immediately after the `MENewStream` event and likely waits for the `MEStreamStarted` event before subscribing to the sink(s). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9716#note_125318
On Tue Dec 9 21:38:37 2025 +0000, Brendan McGrath wrote:
Actually, it looks like Windows might wait for the `MEStreamStarted` event (as suppressing that is enough to halt the test program). @CharlottePabst FWIW - I'm hitting this issue too with my scrubbing work. Here's the work-around I have at the moment: https://gitlab.winehq.org/redmcg/wine/-/commit/c96938c642ceabe564487664816b2... Windows might have a lot more nuance. For example: it might wait for the `MEStreamStarted` event and then only subscribe to the sink(s) to which it connects at the end of the attached pipeline(s). Edit: Here's the relevant output from the new test: ``` 1465.043300|03640|sink_mock_QueueEvent: 305 (MEStreamSinkRequestSample) {00000000-0000-0000-0000-000000000000} (GUID_NULL) 0 <EMPTY> 1465.054398|04532|source_mock_BeginGetEvent: 1465.054679|04532|source_mock_QueueEvent: 205 (MENewStream) {00000000-0000-0000-0000-000000000000} (GUID_NULL) 0 00000216581C7A18 1465.054908|04532|source_mock_QueueEvent: 201 (MESourceStarted) {00000000-0000-0000-0000-000000000000} (GUID_NULL) 0 <EMPTY> 1465.055050|03640|source_mock_EndGetEvent: hr: 0, 205 (MENewStream) {00000000-0000-0000-0000-000000000000} (GUID_NULL) 0 00000216581C7A18 1465.055141|03640|stream_mock_BeginGetEvent: 1465.055216|03640|source_mock_BeginGetEvent: 1465.055282|03640|source_mock_EndGetEvent: hr: 0, 201 (MESourceStarted) {00000000-0000-0000-0000-000000000000} (GUID_NULL) 0 <EMPTY> 1465.055365|03640|source_mock_BeginGetEvent: ``` So it subscribes to the new stream immediately after the `MENewStream` event and likely waits for the `MEStreamStarted` event before subscribing to the sink(s). I'd rather we made it work as close as possible to how it should be. Deferring it until MEStreamStarted is not hard, but we need to carefully test when this happens in relation to other things, like MF_TOPOSTATUS_STARTED_SOURCE event or presentation clock handling.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9716#note_125352
On Tue Dec 9 22:09:53 2025 +0000, Nikolay Sivov wrote:
I'd rather we made it work as close as possible to how it should be. Deferring it until MEStreamStarted is not hard, but we need to carefully test when this happens in relation to other things, like MF_TOPOSTATUS_STARTED_SOURCE event or presentation clock handling. Agree. There's actually a number of things to test. Like calling `IMFRateControl::SetRate` on the media session before calling `IMFMediaSession::Start`. It must forward that rate change to the sink, but how does it then receive the `MEStreamSinkRateChanged` event if it isn't subscribed? In my test program, it looks like it doesn't. And it doesn't care. It just generates a `MESessionRateChanged` event anyway.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9716#note_125358
participants (4)
-
Brendan McGrath (＠redmcg) -
Charlotte Pabst
-
Charlotte Pabst (＠CharlottePabst)
-
Nikolay Sivov (＠nsivov)