[PATCH v2] winegstreamer: Do not block forever if EOS races with command queue.
Currently, the winegstreamer media source checks for EOS when RequestSample() is called, but doesn't handle the cases when EOS is detected between the RequestSample() call and the moment when the request is popped from the command queue and serviced. This can result in the media source waiting forever for a sample and get stuck. This commit fixes the bug by adding a check for EOS in wait_for_event(). This commit fixes Medieval Dynasty hanging on developer logos on the Steam Deck. Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- dlls/winegstreamer/media_source.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index 85ec31d2498..ca8f92b07ea 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -534,6 +534,12 @@ static void wait_on_sample(struct media_stream *stream, IUnknown *token) TRACE("%p, %p\n", stream, token); + if (stream->eos) + { + IMFMediaEventQueue_QueueEventParamVar(stream->event_queue, MEError, &GUID_NULL, MF_E_END_OF_STREAM, &empty_var); + return; + } + for (;;) { if (!wg_parser_stream_get_event(stream->wg_stream, &event)) -- 2.34.1
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- Looks fine to me, but it probably deserves review from Nikolay as well.
On 2/9/22 11:40, Giovanni Mascellani wrote:
Currently, the winegstreamer media source checks for EOS when RequestSample() is called, but doesn't handle the cases when EOS is detected between the RequestSample() call and the moment when the request is popped from the command queue and serviced. This can result in the media source waiting forever for a sample and get stuck.
This commit fixes the bug by adding a check for EOS in wait_for_event().
This commit fixes Medieval Dynasty hanging on developer logos on the Steam Deck.
Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- dlls/winegstreamer/media_source.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index 85ec31d2498..ca8f92b07ea 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -534,6 +534,12 @@ static void wait_on_sample(struct media_stream *stream, IUnknown *token)
TRACE("%p, %p\n", stream, token);
+ if (stream->eos) + { + IMFMediaEventQueue_QueueEventParamVar(stream->event_queue, MEError, &GUID_NULL, MF_E_END_OF_STREAM, &empty_var); + return; + } + for (;;) { if (!wg_parser_stream_get_event(stream->wg_stream, &event)) Sorry, I missed this one. What I don't understand about this is why wg_parser_stream_get_event() can't simply return for streams that reached eos. It seems it should be able to track such state differences. There is already eos flag in wg_parser_stream(), but I see it means something else, and is never reset (I think).
Anyway, since there is no way to test this situation on Windows, at least for file-based sources that I tried, I think we should simply return without producing an event.
On 2/22/22 15:09, Nikolay Sivov wrote:
On 2/9/22 11:40, Giovanni Mascellani wrote:
Currently, the winegstreamer media source checks for EOS when RequestSample() is called, but doesn't handle the cases when EOS is detected between the RequestSample() call and the moment when the request is popped from the command queue and serviced. This can result in the media source waiting forever for a sample and get stuck.
This commit fixes the bug by adding a check for EOS in wait_for_event().
This commit fixes Medieval Dynasty hanging on developer logos on the Steam Deck.
Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- dlls/winegstreamer/media_source.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index 85ec31d2498..ca8f92b07ea 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -534,6 +534,12 @@ static void wait_on_sample(struct media_stream *stream, IUnknown *token) TRACE("%p, %p\n", stream, token); + if (stream->eos) + { + IMFMediaEventQueue_QueueEventParamVar(stream->event_queue, MEError, &GUID_NULL, MF_E_END_OF_STREAM, &empty_var); + return; + } + for (;;) { if (!wg_parser_stream_get_event(stream->wg_stream, &event)) Sorry, I missed this one. What I don't understand about this is why wg_parser_stream_get_event() can't simply return for streams that reached eos. It seems it should be able to track such state differences. There is already eos flag in wg_parser_stream(), but I see it means something else, and is never reset (I think).
The current design of winegstreamer has wg_parser_stream_get_event() only returning an EOS event once. The DirectShow frontend actually depends on this design. I'm currently working on reorganizing things so that it doesn't, though, at which point we could allow that function to never block on EOS, and probably make things easier.
On 2/22/22 15:14, Zebediah Figura (she/her) wrote:
On 2/22/22 15:09, Nikolay Sivov wrote:
On 2/9/22 11:40, Giovanni Mascellani wrote:
Currently, the winegstreamer media source checks for EOS when RequestSample() is called, but doesn't handle the cases when EOS is detected between the RequestSample() call and the moment when the request is popped from the command queue and serviced. This can result in the media source waiting forever for a sample and get stuck.
This commit fixes the bug by adding a check for EOS in wait_for_event().
This commit fixes Medieval Dynasty hanging on developer logos on the Steam Deck.
Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- dlls/winegstreamer/media_source.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index 85ec31d2498..ca8f92b07ea 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -534,6 +534,12 @@ static void wait_on_sample(struct media_stream *stream, IUnknown *token) TRACE("%p, %p\n", stream, token); + if (stream->eos) + { + IMFMediaEventQueue_QueueEventParamVar(stream->event_queue, MEError, &GUID_NULL, MF_E_END_OF_STREAM, &empty_var); + return; + } + for (;;) { if (!wg_parser_stream_get_event(stream->wg_stream, &event)) Sorry, I missed this one. What I don't understand about this is why wg_parser_stream_get_event() can't simply return for streams that reached eos. It seems it should be able to track such state differences. There is already eos flag in wg_parser_stream(), but I see it means something else, and is never reset (I think).
The current design of winegstreamer has wg_parser_stream_get_event() only returning an EOS event once. The DirectShow frontend actually depends on this design. I'm currently working on reorganizing things so that it doesn't, though, at which point we could allow that function to never block on EOS, and probably make things easier.
FWIW, this is done now, in 28c9c138d26.
On 2/23/22 00:09, Nikolay Sivov wrote:
On 2/9/22 11:40, Giovanni Mascellani wrote:
Currently, the winegstreamer media source checks for EOS when RequestSample() is called, but doesn't handle the cases when EOS is detected between the RequestSample() call and the moment when the request is popped from the command queue and serviced. This can result in the media source waiting forever for a sample and get stuck.
This commit fixes the bug by adding a check for EOS in wait_for_event().
This commit fixes Medieval Dynasty hanging on developer logos on the Steam Deck.
Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- dlls/winegstreamer/media_source.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index 85ec31d2498..ca8f92b07ea 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -534,6 +534,12 @@ static void wait_on_sample(struct media_stream *stream, IUnknown *token) TRACE("%p, %p\n", stream, token); + if (stream->eos) + { + IMFMediaEventQueue_QueueEventParamVar(stream->event_queue, MEError, &GUID_NULL, MF_E_END_OF_STREAM, &empty_var); + return; + } + for (;;) { if (!wg_parser_stream_get_event(stream->wg_stream, &event)) Sorry, I missed this one. What I don't understand about this is why wg_parser_stream_get_event() can't simply return for streams that reached eos. It seems it should be able to track such state differences. There is already eos flag in wg_parser_stream(), but I see it means something else, and is never reset (I think).
Anyway, since there is no way to test this situation on Windows, at least for file-based sources that I tried, I think we should simply return without producing an event.
Let's have it with an event for now, I'll have to test how pipeline should react to that, we currently ignore async errors.
On 2/23/22 00:15, Nikolay Sivov wrote:
On 2/23/22 00:09, Nikolay Sivov wrote:
On 2/9/22 11:40, Giovanni Mascellani wrote:
Currently, the winegstreamer media source checks for EOS when RequestSample() is called, but doesn't handle the cases when EOS is detected between the RequestSample() call and the moment when the request is popped from the command queue and serviced. This can result in the media source waiting forever for a sample and get stuck.
This commit fixes the bug by adding a check for EOS in wait_for_event().
This commit fixes Medieval Dynasty hanging on developer logos on the Steam Deck.
Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- dlls/winegstreamer/media_source.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index 85ec31d2498..ca8f92b07ea 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -534,6 +534,12 @@ static void wait_on_sample(struct media_stream *stream, IUnknown *token) TRACE("%p, %p\n", stream, token); + if (stream->eos) + { + IMFMediaEventQueue_QueueEventParamVar(stream->event_queue, MEError, &GUID_NULL, MF_E_END_OF_STREAM, &empty_var); + return; + } + for (;;) { if (!wg_parser_stream_get_event(stream->wg_stream, &event)) Sorry, I missed this one. What I don't understand about this is why wg_parser_stream_get_event() can't simply return for streams that reached eos. It seems it should be able to track such state differences. There is already eos flag in wg_parser_stream(), but I see it means something else, and is never reset (I think).
Anyway, since there is no way to test this situation on Windows, at least for file-based sources that I tried, I think we should simply return without producing an event.
Let's have it with an event for now, I'll have to test how pipeline should react to that, we currently ignore async errors.
I tested for Source Reader, and error event is ignored there, it won't break anything, but won't trigger MF_SOURCE_READERF_ERROR or MF_SOURCE_READERF_ENDOFSTREAM. In our case it will rely on MEEndStream being sent before MEError, otherwise it will wait forever. So at least for reader sending MEError here is unnecessary, will check the session next. I also found additional bug that we don't handle failed MEMediaSample response properly (when it has error set and does not have attached sample), this should return READERF_ERROR flag and error code from the event as ReadSample() return code.
Hi, Il 25/02/22 17:22, Nikolay Sivov ha scritto:
I tested for Source Reader, and error event is ignored there, it won't break anything, but won't trigger MF_SOURCE_READERF_ERROR or MF_SOURCE_READERF_ENDOFSTREAM. In our case it will rely on MEEndStream being sent before MEError, otherwise it will wait forever. So at least for reader sending MEError here is unnecessary, will check the session next.
It's embarrassing how Microsoft seems unable to properly use the (terrible) interfaces they invented themselves. I guess that from the point of view of the bug I was trying to solve, the really important point is to avoid being stuck waiting for an EOS that will never come (because it has already been processed), which blocks the whole media source command queue. I think that queueing an error is a good thing because it makes sense and because it follows the specifications, but maybe is not required for that bug. Giovanni.
On 2/25/22 19:47, Giovanni Mascellani wrote:
Hi,
Il 25/02/22 17:22, Nikolay Sivov ha scritto:
I tested for Source Reader, and error event is ignored there, it won't break anything, but won't trigger MF_SOURCE_READERF_ERROR or MF_SOURCE_READERF_ENDOFSTREAM. In our case it will rely on MEEndStream being sent before MEError, otherwise it will wait forever. So at least for reader sending MEError here is unnecessary, will check the session next.
It's embarrassing how Microsoft seems unable to properly use the (terrible) interfaces they invented themselves. I guess that from the point of view of the bug I was trying to solve, the really important point is to avoid being stuck waiting for an EOS that will never come (because it has already been processed), which blocks the whole media source command queue. I think that queueing an error is a good thing because it makes sense and because it follows the specifications, but maybe is not required for that bug.
I don't think there is anything broken with interface itself. The problem is how our source is working - during last successful RequestSample -> MEMediaSample response it's probably meant to figure out that eos is reached. This way it could fail next request properly. I'm not proposing to remove MEError right now, but I'll look at fixing the other thing I mentioned.
Giovanni.
Hi, Il 22/02/22 22:09, Nikolay Sivov ha scritto:
Anyway, since there is no way to test this situation on Windows, at least for file-based sources that I tried, I think we should simply return without producing an event.
The patch is now accepted (thanks for that), but for the sake of the argument: I think that returning success from RequestSample() and never emitting a corresponding event is wrong; this is explicitly said in the docs[1] and it also makes sense: the application needs a way to know whether it still has to wait for an event from the media source or not, so it must be stipulated that an event has to be eventually emitted. In case of error, such event has to be MEError, if the error could not be detected in time for being returned from RequestSample() directly. [1] https://docs.microsoft.com/en-us/windows/win32/api/mfidl/nf-mfidl-imfmediast... Giovanni.
participants (4)
-
Giovanni Mascellani -
Nikolay Sivov -
Zebediah Figura -
Zebediah Figura (she/her)