[PATCH 1/5] winegstreamer: Disable VA-API plugins in wg_transform.

[Zebediah Figura]

On 5/12/22 03:15, Rémi Bernon wrote:
> From: Rémi Bernon <rbernon at codeweavers.com>
> 
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=45988
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=47084
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=49715
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52183
> Signed-off-by: Rémi Bernon <rbernon at codeweavers.com>
> ---
>   dlls/winegstreamer/wg_transform.c | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c
> index 21392a82509..dcc334caf7a 100644
> --- a/dlls/winegstreamer/wg_transform.c
> +++ b/dlls/winegstreamer/wg_transform.c
> @@ -114,7 +114,18 @@ static GstElement *transform_find_element(GstElementFactoryListType type, GstCap
>       for (tmp = transforms; tmp != NULL && element == NULL; tmp = tmp->next)
>       {
>           name = gst_plugin_feature_get_name(GST_PLUGIN_FEATURE(tmp->data));
> -        if (!(element = gst_element_factory_create(GST_ELEMENT_FACTORY(tmp->data), NULL)))
> +
> +        /* VA-API plugins are not supported at the moment, they do not support
> +         * buffer pool negociation and will consistently ignore our pool, as
> +         * it is not VA-API compatible, but also our plane alignment requirements
> +         * from video meta. They also are asynchronous by design, which isn't
> +         * going to let us implement zero-copy in the transforms reliably, and
> +         * vaapidecodebin includes a videoconvert element which does more than
> +         * we would like.
> +         */

I'll admit, none of these sound like good reasons to ignore vaapi.

Regarding allocation: the source is not actually required to use an 
allocator proposed by the sink in an ALLOCATION query. The documentation 
isn't really clear about this, but I asked for a clarification from the 
GStreamer developers. vaapi's behaviour here is not a bug.

Regarding asynchronicity: It's not obvious from the comment why 
asynchronicity implies that zero-copy can't be done. Regardless, though, 
there's no API guarantee that filters are synchronous, including 
decoders. We shouldn't hardcode vaapi on those grounds.

Regarding videoconvert: why is this a problem?

To summarize, it's not clear from the comment why any of these things 
are a problem, and, even assuming they are, none of them inhere to vaapi 
specifically.

While zero-copy is desirable, I don't think it should be an absolute 
requirement if we can't easily guarantee it, and the details regarding 
ALLOCATION queries suggests that we indeed can't.

It may be that hardware plugins provide consistently worse performance 
than anything else, and should be disabled on those grounds. (Perhaps 
even for the above reasons—although I'd find it surprising that a CPU 
copy is the bottleneck and not the download itself—and in any case the 
comment doesn't make that clear). It's not immediately clear that 
hardware plugins are *always* going to provide worse performance, 
regardless of CPU setup, but it also wouldn't surprise me if so.

Ideally this should be a decision made on a lower level than Wine, but I 
also recognize that it's going to be hard to do so, since it depends on 
a lot of context, and I think it's fine to put code in Wine for those 
reasons. If we do, though:

  * I think we should match elements based on 
GST_ELEMENT_FACTORY_KLASS_HARDWARE than string-matching the element name;

  * ideally we should disprefer hardware decoders rather than disabling 
them completely.

> +        if (!strncmp(name, "vaapi", 5))
> +            GST_FIXME("Ignoring unsupported %s plugin", name);
> +        else if (!(element = gst_element_factory_create(GST_ELEMENT_FACTORY(tmp->data), NULL)))
>               GST_WARNING("Failed to create %s element.", name);
>       }
>       gst_plugin_feature_list_free(transforms);