[PATCH 1/3] winegstreamer: Support dynamic wg_transform output format change.

[Zebediah Figura]

On 4/5/22 17:39, Rémi Bernon wrote:
> On 4/6/22 00:11, Zebediah Figura wrote:
>> On 4/5/22 07:26, Rémi Bernon wrote:
>>> 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>
>>> ---
>>>
>>> (This need to come first, before 231660-231665 as the spurious todo_wine
>>> is caused by the H264 format being synchronously rejected when libav
>>> plugins are used)
>>>
>>> In this patch I assumed that sink callbacks are always called from the
>>> same thread, and then do not require any locking around sink_caps
>>> accesses.
>>
>> That's not guaranteed by the API. I'd rather just be explicitly 
>> thread-safe.
>>
>>> @@ -264,6 +298,11 @@ NTSTATUS wg_transform_create(void *args)
>>>               break;
>>>           case WG_MAJOR_TYPE_VIDEO:
>>> +            if (!(element = create_element("videoconvert", "base"))
>>> +                    || !transform_append_element(transform, element, 
>>> &first, &last))
>>> +                goto out;
>>
>> This is one of those things that probably should have been split 
>> anyway, but as before, I'm left wondering why we need this, and in 
>> particular why it's related to the rest of this patch. Why would H.264 
>> dynamic format changes imply that the video format changes, and why 
>> would we need to convert it?
>>
> 
> 
> Because what actually may change is the frame size, not the image format 
> that is output, though maybe a transform user could change it, games 
> don't expect it to change and are confused if it does.

But videoconvert doesn't affect frame size, only colour space. Is 
videoconvert relevant to this patch?

> 
> But then yes, probably it could and should be added beforehand, as 
> otherwise caps negotiation is failing.
> 
> 
>>> +            /* Let GStreamer choose a default number of threads. */
>>> +            gst_util_set_object_arg(G_OBJECT(element), "n-threads", 
>>> "0");
>>
>> I'd be curious to learn whether this has a measurable positive impact, 
>> especially with different video sizes.
>>
> 
> 
> I believe I saw that it did especially on large videos, though I don't 
> have any numbers to back that claim.
> 
> 
>>> @@ -425,7 +465,18 @@ NTSTATUS wg_transform_read_data(void *args)
>>>           return STATUS_SUCCESS;
>>>       }
>>> -    if ((status = 
>>> read_transform_output_data(transform->output_buffer, sample)))
>>> +    if (sample->format && (caps = 
>>> gst_sample_get_caps(transform->output_sample)))
>>
>> This bothers me. Patch ordering problems aside, the way this reads 
>> implies that the winegstreamer API consumer is free to ignore sample 
>> format changes, which doesn't seem right.
>>

Actually, I just reread this, and no, it doesn't really imply that. So 
forget what I said...