[PATCH 0/5] MR61: winegstreamer: Dynamic transform stream format change support.

[Zebediah Figura]

On 5/12/22 14:00, Rémi Bernon (@rbernon) wrote:
> On Thu May 12 18:24:35 2022 +0000, **** wrote:
>> Zebediah Figura replied on the mailing list:
>> ```
>> 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.
>> ```
> VA-API gets in the way of a simple implementation. We can workaround all of its defects, but it will make everything more complicated than it needs to be.
> 
> Yes, we can support their low quality plugins which ignore the buffer requirements that downstream explicitly requests (I'm not even talking about using an allocator we would provide), which means that we would have instead to 1) detect that they didn't, 2) do the plane alignment and copy ourselves, with all the additional code that it involves to do it properly for any of the various video formats that could be used.

I don't think that these things are prohibitive.

We can detect that the upstream element isn't using our pool simply by 
checking the pool. We can copy, without manually implementing alignment 
logic, by using GstVideoFrame APIs.

I recognize that I'm asking for more work to be done than you were 
planning on doing; that can be frustrating; I'm frankly happy to do the 
work myself. I'd just rather it be done this way, since according to my 
understanding of the GStreamer API this is the right thing to do.

> 
> Or, we can instead disable these plugins for now, as we are not interested in them, and as they are only ever going to be useful in the future when we can pass GPU images through to the presenter.
> 
> Tbh I even think we should instead hardcode the plugins we want to use, because this is delegating too much control to GStreamer plugin matching algorithm when the transforms, and the games using them, actually require a very specific behavior from the decoder.
> 
> Same for the implicit videoconvert, we don't want it because it makes the transform behavior different from how native transform behaves. We want to be able to change video format any time, even after some buffers have already been decoded and are queued for output.
> 

I really don't think we should be hardcoding plugins, especially plugins 
that aren't part of core, base, or good. (That we already hardcode some 
plugins for wg_parser is not ideal and should probably be changed.) Nor 
do I think we should depend on implementation details if we can at all 
avoid it.