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

[Rémi Bernon]

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 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.
> 


Format change is really only used by H264. I initially had it 
non-optional, but it means every transform needs to keep a wg_format 
too, as well as passing it around. None of the other transforms I've 
tested (4-5 so far) will generate stream change events.

If the client uses a fixed format, it will be translated to a fixed caps 
and I believe no stream change can happen.

Only the H264 client, which clears its width / height before creating 
the wg_transform, is expecting format (but actually only frame size) to 
change.

-- 
Rémi Bernon <rbernon at codeweavers.com>