[PATCH 0/1] MR6447: winegstreamer/wm_reader: Use codec format only for compressed types.
The intention of wg_parser_stream_get_codec_format is to return the codec for compressed stream. We shouldn't use it for uncompressed result. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6447
From: Ziqing Hui <zhui(a)codeweavers.com> The intention of wg_parser_stream_get_codec_format is to return the codec for compressed stream. We shouldn't use it for uncompressed result. --- dlls/winegstreamer/wm_reader.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/dlls/winegstreamer/wm_reader.c b/dlls/winegstreamer/wm_reader.c index e3016b0918b..05a296871f7 100644 --- a/dlls/winegstreamer/wm_reader.c +++ b/dlls/winegstreamer/wm_reader.c @@ -551,7 +551,9 @@ static HRESULT WINAPI stream_props_GetMediaType(IWMMediaProps *iface, WM_MEDIA_T TRACE("iface %p, mt %p, size %p.\n", iface, mt, size); wg_parser_stream_get_codec_format(config->stream->wg_stream, &codec_format); - format = (codec_format.major_type != WG_MAJOR_TYPE_UNKNOWN) ? &codec_format : &config->stream->format; + format = (codec_format.major_type != WG_MAJOR_TYPE_UNKNOWN + && codec_format.major_type != WG_MAJOR_TYPE_AUDIO + && codec_format.major_type != WG_MAJOR_TYPE_VIDEO) ? &codec_format : &config->stream->format; if (!amt_from_wg_format(&stream_mt, format, true)) return E_OUTOFMEMORY; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6447
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=148239 Your paranoid android. === debian11b (64 bit WoW report) === kernel32: comm.c:1586: Test failed: Unexpected time 1002, expected around 500
This seems very suspicious. For an uncompressed stream the codec format should be the same as the preferred format, so it shouldn't matter which we return. Why does this make a difference? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6447#note_81232
On Fri Sep 6 02:26:20 2024 +0000, Elizabeth Figura wrote:
This seems very suspicious. For an uncompressed stream the codec format should be the same as the preferred format, so it shouldn't matter which we return. Why does this make a difference? I've found that the preferred_format(which is current_caps now) is different from the actual output format in Proton for Steam game "Marlow Briggs and the Mask of Death" (249680).
We are setting RGB24 using wg_parser_stream_enable() in stream_props_GetMediaType(), and this game do finally output RGB24. However, the preferred_format is RGB32. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6447#note_81313
On Fri Sep 6 02:26:20 2024 +0000, Ziqing Hui wrote:
I've found that the preferred_format(which is current_caps now) is different from the actual output format in Proton for Steam game "Marlow Briggs and the Mask of Death" (249680). We are setting RGB24 using wg_parser_stream_enable() in stream_props_GetMediaType(), and this game do finally output RGB24. However, the preferred_format is RGB32. I know in theory, the preferred_format/current_caps which is obtained from GST_EVENT_CAPS was expected to be the actual output format. However, in practice, sometimes the actual output format is different from it.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6447#note_81314
On Fri Sep 6 02:29:00 2024 +0000, Ziqing Hui wrote:
I know in theory, the preferred_format/current_caps which is obtained from GST_EVENT_CAPS was expected to be the actual output format. However, in practice, sometimes the actual output format is different from it. Before the codec format patch, we are always returning config->stream->format in stream_props_GetMediaType(). So at least it's safe to return it for uncompressed format like things before.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6447#note_81315
On Fri Sep 6 02:31:15 2024 +0000, Ziqing Hui wrote:
Before the codec format patch, we are always returning config->stream->format in stream_props_GetMediaType(). So at least it's safe to return it for uncompressed format like things before. OK， I think I find out the reason why preferred_format is different from what the actual output format.
We are calling wg_parser_stream_enable() in init_stream() to set our desired output format. And wg_parser_stream_enable() will send a reconfigure event to upstream, and we expect upstream element send a GST_EVENT_CAPS event downstream to my_sink pad, and we only set preferred_format to the actual output format when we are handling GST_EVENT_CAPS in sink_event_cb(). The problem is, the GST_EVENT_CAPS is not synced with wg_parser_stream_enable(). If we try to use preferred_format after wg_parser_stream_enable() and before GST_EVENT_CAPS, it will be wrong format. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6447#note_81317
On Fri Sep 6 04:47:54 2024 +0000, Ziqing Hui wrote:
OK， I think I find out the reason why preferred_format is different from what the actual output format. We are calling wg_parser_stream_enable() in init_stream() to set our desired output format. And wg_parser_stream_enable() will send a reconfigure event to upstream, and we expect upstream element send a GST_EVENT_CAPS event downstream to my_sink pad, and we only set preferred_format to the actual output format when we are handling GST_EVENT_CAPS in sink_event_cb(). The problem is, the GST_EVENT_CAPS is not synced with wg_parser_stream_enable(). If we try to use preferred_format after wg_parser_stream_enable() and before GST_EVENT_CAPS, it will be wrong format. The issue of the game I mentioned above is because it calls stream_props_GetMediaType() after parser created, but before CAPS event. So using preferred_format here will result in a wrong result. And this is a common usage, there is high possibility that we want to know the output format in front side after we creating the reader. Ah, right, this is what I was alluding to with 5667. The preferred_format *should* be only set when we first get it; it's a bug that it's not.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6447#note_81427
On Fri Sep 6 18:12:56 2024 +0000, Elizabeth Figura wrote:
Ah, right, this is what I was alluding to with 5667. The preferred_format *should* be only set when we first get it; it's a bug that it's not. I found that you said this in !5667: The point of the "preferred" format is that it should be the one that does not require videoconvert, and therefore offers the best latency.
If that's true, we should not use preferred_format in stream_props_GetMediaType() for uncompressed situation, because "the one that does not require videoconvert" may differ from the actual output format. In front side, games will use the result of stream_props_GetMediaType() to create d3d textures, if we get a bpp which is different from the actual output, there will be buffer overflow. So I think this patch is doing the right thing. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6447#note_81595
On Mon Sep 9 02:52:32 2024 +0000, Ziqing Hui wrote:
I found that you said this in !5667:
The point of the "preferred" format is that it should be the one that does not require videoconvert, and therefore offers the best latency. If that's true, we should not use preferred_format in stream_props_GetMediaType() for uncompressed situation, because "the one that does not require videoconvert" may differ from the actual output format. In front side, games will use the result of stream_props_GetMediaType() to create d3d textures, if we get a bpp which is different from the actual output, there will be buffer overflow. So I think this patch is doing the right thing. Oh, right, because of videoflip... although I'm not sure if we're doing the right thing for uncompressed video in that case.
I was going to suggest anyway that using codec_format instead of preferred_format for uncompressed video would make sense; even if it comes out to the same thing it's documenting that assumption. That'd be kind of the opposite of what this patch does, though. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6447#note_81646
I was going to suggest anyway that using codec_format instead of preferred_format for uncompressed video would make sense;
Do you mean that, in wg_parser_stream_get_codec_format(), for uncompressed situation, we should also return codec_format instead of returning preferred_format? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6447#note_81849
I was going to suggest anyway that using codec_format instead of preferred_format for uncompressed video would make sense;
Do you mean that, in wg_parser_stream_get_codec_format(), for uncompressed situation, we should also return codec_format instead of returning preferred_format?
Yes, if the codec format is uncompressed, we should return it first, since that's what applications expect. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6447#note_82484
On Wed Sep 18 03:03:16 2024 +0000, Elizabeth Figura wrote:
I was going to suggest anyway that using codec_format instead of preferred_format for uncompressed video would make sense;
Do you mean that, in wg_parser_stream_get_codec_format(), for uncompressed situation, we should also return codec_format instead of returning preferred_format? Yes, if the codec format is uncompressed, we should return it first, since that's what applications expect. But even if we return codec_format in wg_parser_stream_get_codec_format() for uncompressed, this is still not what the application expect.
The application expect the result of wm_reader:stream_props_GetMediaType() to be the actual output format, so that it can create d3d texture later based on it. However, our codec_format is the output format of the first decodebin (see: wg_parser.c:pad_added_cb()), it is the format before handled by videoconverter. Application expect it to be the final output format. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6447#note_82493
On Wed Sep 18 06:49:54 2024 +0000, Ziqing Hui wrote:
But even if we return codec_format in wg_parser_stream_get_codec_format() for uncompressed, this is still not what the application expect. The application expect the result of wm_reader:stream_props_GetMediaType() to be the actual output format, so that it can create d3d texture later based on it. However, our codec_format is the output format of the first decodebin (see: wg_parser.c:pad_added_cb()), it is the format before handled by videoconverter. It is not the final output format. I don't understand. We should be calling wg_parser_stream_enable() with that format, right?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6447#note_82550
On Wed Sep 18 16:50:37 2024 +0000, Elizabeth Figura wrote:
I don't understand. We should be calling wg_parser_stream_enable() with that format, right? For uncompressed situation, we are calling wg_parser_stream_enable() with RGB24 in wm_reader:init_stream(), so wg_parser will finally output RGB24.
But the codec_format is not RGB24, it is the format of the stream. It is converted to RGB24 by videoconvert. In uncompressed situation, application expect the result of stream_props_GetMediaType() to be the final output format, which is RGB24, so we should not use codec_format in stream_props_GetMediaType() for uncompressed. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6447#note_82596
We should be calling wg_parser_stream_enable() with that format, right?
We are not calling wg_parser_stream_enable() with codec_format, we are calling wg_parser_stream_enable() with RGB24, see: wm_reader():init_stream() -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6447#note_82597
On Thu Sep 19 04:41:48 2024 +0000, Ziqing Hui wrote:
We should be calling wg_parser_stream_enable() with that format, right? We are not calling wg_parser_stream_enable() with codec_format, we are calling wg_parser_stream_enable() always with RGB24, see: wm_reader.c:init_stream() If the application never sets a format and expects that a certain format should be output by default (e.g. the first format enumerated, or the codec format) then we should be selecting that by default instead of RGB24.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6447#note_82673
participants (4)
-
Elizabeth Figura (＠zfigura) -
Marvin -
Ziqing Hui
-
Ziqing Hui (＠zhui)