From: Thong Thai thong.thai@amd.com
Signed-off-by: Thong Thai thong.thai@amd.com --- dlls/winegstreamer/unixlib.c | 2 +- dlls/winegstreamer/wg_transform.c | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/dlls/winegstreamer/unixlib.c b/dlls/winegstreamer/unixlib.c index 513ece95a90..e4702b2abbb 100644 --- a/dlls/winegstreamer/unixlib.c +++ b/dlls/winegstreamer/unixlib.c @@ -84,7 +84,7 @@ GstElement *find_element(GstElementFactoryListType type, GstCaps *src_caps, GstC GList *tmp, *transforms; const gchar *name;
- if (!(transforms = gst_element_factory_list_get_elements(type, GST_RANK_MARGINAL))) + if (!(transforms = gst_element_factory_list_get_elements(type, GST_RANK_NONE))) goto done;
if (src_caps) diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index e2b14527a20..ae758f72d03 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -355,6 +355,24 @@ NTSTATUS wg_transform_create(void *args) switch (input_format.major_type) { case WG_MAJOR_TYPE_VIDEO_H264: + GstElement *hw_element = find_element(GST_ELEMENT_FACTORY_TYPE_DECODER | GST_ELEMENT_FACTORY_TYPE_HARDWARE, src_caps, raw_caps); + if (hw_element) { + if (!(element = create_element("h264parse", "base")) + || !append_element(transform->container, element, &first, &last)) + { + free(hw_element); + gst_caps_unref(raw_caps); + goto out; + } + if (!(element = hw_element) + || !append_element(transform->container, element, &first, &last)) + { + free(hw_element); + gst_caps_unref(raw_caps); + goto out; + } + break; + } case WG_MAJOR_TYPE_AUDIO_MPEG1: case WG_MAJOR_TYPE_AUDIO_MPEG4: case WG_MAJOR_TYPE_AUDIO_WMA:
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 full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=138189
Your paranoid android.
=== debian11 (build log) ===
../wine/dlls/winegstreamer/wg_transform.c:358:13: error: a label can only be part of a statement and a declaration is not a statement Task: The win32 Wine build failed
=== debian11b (build log) ===
../wine/dlls/winegstreamer/wg_transform.c:358:13: error: a label can only be part of a statement and a declaration is not a statement Task: The wow64 Wine build failed
I don't think adding `GST_ELEMENT_FACTORY_TYPE_HARDWARE` is required, we don't disallow hardware decoders. Then we sort the elements by rank, so if the decoder has a rank `None` it will unlikely be chosen over software decoders with a higher rank.
I don't know if there's a good solution here, we could maybe order hardware decoders first but at the same time I'm not sure that hardware decoding is very useful right now. It will always force an additional copy of the frames back to CPU memory, as we have no way to keep the frame on the GPU and pass them downstream as is. I haven't done much measurements though, so if you have numbers showing that even with the cost of the copy it is still worth it, please share :).
In any case, some of the changes look alright. I would approve changing to `GST_RANK_NONE` for instance, and adding a parser upfront seems useful too after all (there's still some shenanigans with stream formats as discussed in https://gitlab.winehq.org/wine/wine/-/merge_requests/3810#note_45157). I would make it optional and generic though, something like that:
```c if ((element = find_element(GST_ELEMENT_FACTORY_TYPE_PARSER, src_caps, src_caps)) && !append_element(transform->container, element, &first, &last)) { gst_caps_unref(raw_caps); goto out; } ```
What problem are you trying to solve here?
My natural inclination is that we should let GStreamer decide whether hardware decoding is best. Do we have any information that would better inform that decision?
In any case, some of the changes look alright. I would approve changing to `GST_RANK_NONE` for instance,
That doesn't seem like a good idea; why should we do that?
That doesn't seem like a good idea; why should we do that?
Because we order the elements by rank, and I don't see any reason to exclude elements that GStreamer lists unless they actually prove to be problematic.
Some libav muxers are ranked `none` for some apparently historical (and IIUC IMO dubious) reasons, although they seem to be functional enough.
On Tue Oct 3 16:45:29 2023 +0000, Rémi Bernon wrote:
I don't think adding `GST_ELEMENT_FACTORY_TYPE_HARDWARE` is required, we don't disallow hardware decoders. Then we sort the elements by rank, so if the decoder has a rank `None` it will unlikely be chosen over software decoders with a higher rank. I don't know if there's a good solution here, we could maybe order hardware decoders first but at the same time I'm not sure that hardware decoding is very useful right now. It will always force an additional copy of the frames back to CPU memory, as we have no way to keep the frame on the GPU and pass them downstream as is. I haven't done much measurements though, so if you have numbers showing that even with the cost of the copy it is still worth it, please share :). In any case, some of the changes look alright. I would approve changing to `GST_RANK_NONE` for instance, and adding a parser upfront seems useful too after all (there's still some shenanigans with stream formats as discussed in https://gitlab.winehq.org/wine/wine/-/merge_requests/3810#note_45157). I would make it optional and generic though, something like that:
if ((element = find_element(GST_ELEMENT_FACTORY_TYPE_PARSER, src_caps, src_caps)) && !append_element(transform->container, element, &first, &last)) { gst_caps_unref(raw_caps); goto out; }
Thanks for the quick feedback!
It seems with gstreamer, the hardware decoder bits aren't under the list of decoder factories. Here's what it looks like on my end before and after the change (with the gstreamer-vaapi, and gst-plugin-va packages): ``` Before: -------------------------------------------------- gst-va: WINE unixlib.c:110:find_element: -- avdec_h264
gstreamer-vaapi: WINE unixlib.c:110:find_element: -- vaapidecodebin WINE unixlib.c:110:find_element: -- avdec_h264
After: -------------------------------------------------- gstreamer-vaapi: WINE unixlib.c:110:find_element: -- vaapidecodebin WINE unixlib.c:110:find_element: -- vaapih264dec
gst-va: WINE unixlib.c:110:find_element: -- vah264dec ```
I'll try to see if I can help in any way to improve the performance/elminate the copying to and from CPU memory.
On Tue Oct 3 16:45:29 2023 +0000, Thong Thai wrote:
Thanks for the quick feedback! It seems with gstreamer, the hardware decoder bits aren't under the list of decoder factories. Here's what it looks like on my end before and after the change (with the gstreamer-vaapi, and gst-plugin-va packages):
Before: -------------------------------------------------- gst-va: WINE unixlib.c:110:find_element: -- avdec_h264 gstreamer-vaapi: WINE unixlib.c:110:find_element: -- vaapidecodebin WINE unixlib.c:110:find_element: -- avdec_h264 After: -------------------------------------------------- gstreamer-vaapi: WINE unixlib.c:110:find_element: -- vaapidecodebin WINE unixlib.c:110:find_element: -- vaapih264dec gst-va: WINE unixlib.c:110:find_element: -- vah264decI'll try to see if I can help in any way to improve the performance/elminate the copying to and from CPU memory.
I think the difference comes from rank and ordering differences, vaapih264dec has an equal rank (256) as avdec_h264 (256), and we use `gst_plugin_feature_rank_compare_func` which sorts by rank, then by name, so unless you prioritize hardware decoders specifically, avdec_h264 will always be selected first.
On Tue Oct 3 17:17:52 2023 +0000, Rémi Bernon wrote:
I think the difference comes from rank and ordering differences, vaapih264dec has an equal rank (256) as avdec_h264 (256), and we use `gst_plugin_feature_rank_compare_func` which sorts by rank, then by name, so unless you prioritize hardware decoders specifically, avdec_h264 will always be selected first.
Doh, you're right. I also overlooked the `element == NULL` part of the loop...
To make prioritizing the hardware decoder an optional thing, should it be through some environment variable? Or what would the preferred method be?
Thanks.
On Mon Oct 2 16:23:08 2023 +0000, Rémi Bernon wrote:
That doesn't seem like a good idea; why should we do that?
Because we order the elements by rank, and I don't see any reason to exclude elements that GStreamer lists unless they actually prove to be problematic. Some libav muxers are ranked `none` for some apparently historical (and IIUC IMO dubious) reasons, although they seem to be functional enough.
GST_RANK_NONE is for elements which should not be autoplugged. gst internally requires at least GST_RANK_MARGINAL when autoplugging. I'm not sure if there are negative consequences to removing that limitation, but I'm also not sure there aren't.
If there are elements that we think should not be ranked NONE, then we should fix that on the GStreamer side.
What is this patch trying to accomplish in general? The subject says "Add hardware H264 decoding support", but unless I'm mistaken, our current logic doesn't actually prevent hardware elements from being used.
Rather, the patch tries to prefer hardware elements, but I don't see why? I don't see why Wine should prefer hardware decoding. GStreamer can decide whether or not to use hardware decoding on its own; I don't think we currently have any information to provide to GStreamer that would aid that decision. Am I missing something?
-
Marvin -
Rémi Bernon (＠rbernon) -
Thong Thai
-
Thong Thai (＠thongthai)
-
Zebediah Figura (＠zfigura)