[PATCH 5/5] winegstreamer: Support zero-copy in wg_transform_push_data.

[Rémi Bernon]

On 2/23/22 03:09, Zebediah Figura (she/her) wrote:
> On 2/22/22 16:48, Rémi Bernon wrote:
>> This wraps the wg_sample struct in a PE-side struct to add a list entry
>> without exposing it to the unix-side.
>>
>> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51931
>> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52391
>> Signed-off-by: Rémi Bernon <rbernon at codeweavers.com>
>> ---
>>   dlls/winegstreamer/gst_private.h  |  3 ++-
>>   dlls/winegstreamer/mfplat.c       | 36 ++++++++++++++++++++++++-------
>>   dlls/winegstreamer/unixlib.h      |  2 ++
>>   dlls/winegstreamer/wg_transform.c | 15 +++++++++++--
>>   dlls/winegstreamer/wma_decoder.c  | 13 +++++------
>>   5 files changed, 52 insertions(+), 17 deletions(-)
>>
> 
> This patch is kind of hard to review (or test) without ProcessOutput()
> implemented. Is there a chance that you can send that first?
> 


Well ProcessOutput would actually do nothing unless there's buffer 
pushed first, so it'd be kind of dead code but probably, yes.


>> +void mf_reap_wg_samples(struct list *sample_list, bool force)
>> +{
>> +    struct wg_sample_entry *entry, *next;
>> +    LIST_FOR_EACH_ENTRY_SAFE(entry, next, sample_list, struct wg_sample_entry, entry)
>> +        if (!entry->sample.refcount || force)
>> +            mf_destroy_wg_sample(&entry->sample);
>>   }
> 
> "force" seems awkward here. Since it should never happen anyway, it
> strikes me as better just to print an ERR here [and probably here rather
> than in mf_destroy_wg_sample()] and leak it.
> 


wg_sample_destroy could also separately and incorrectly be called with a 
non zero refcount, so I think the ERR is better to have it in it. 
Otherwise yeah leaking the sample might be better.


>> diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h
>> index 0162ed8f550..5666149a6a1 100644
>> --- a/dlls/winegstreamer/unixlib.h
>> +++ b/dlls/winegstreamer/unixlib.h
>> @@ -106,6 +106,8 @@ struct wg_format
>>   
>>   struct wg_sample
>>   {
>> +    volatile LONG refcount; /* unix refcount */
> 
> Does this need to be a refcount? We only ever incref once in this patch;
> this could just as easily be a "free" member instead.
> 
> It doesn't make much of a difference, but when I see "refcount" I find
> myself thinking about why it's a refcount...
> 


Yes, the same sample may be pushed multiple times. Not with WMA, because 
it doesn't accept input as soon as a has been provided, but other 
transform may and do.


>> +    const LONG __pad;
>>       UINT32 max_size;
>>       UINT32 size;
>>       BYTE *data;
> 
> Also: do the rest of these members really need to be a part of struct
> wg_sample per se? It seems they could just as easily be passed to
> wg_transform_push_data() and then forgotten about. Unless I'm missing
> something, the only thing that really needs to be shared is the refcount
> (or free flag).
> 
> Obviously it's convenient to put the other members here, but I think it
> makes the code less clear.
> 


I think it makes the code clearer as it saves us tons of free parameters 
passed around.

We need at least refcount and size to be kept so that we can update the 
size of the sample buffer when it's released (used for output). Later we 
will also need a flags member, wg_format, timestamp and duration.

All are properties of the sample / buffer we've wrapped and imho makes 
sense to keep here.

Also later, in ProcessOutput I re-use the struct to provide the unix 
output buffer from the sink_chain_cb to read_data function.

It could be another struct but as it shares most of the properties here 
this struct fits very well.


>> @@ -318,11 +325,15 @@ NTSTATUS wg_transform_push_data(void *args)
>>       GstFlowReturn ret;
>>       GstBuffer *buffer;
>>   
>> -    buffer = gst_buffer_new_and_alloc(sample->size);
>> +    InterlockedIncrement(&sample->refcount);
>> +    buffer = gst_buffer_new_wrapped_full(GST_MEMORY_FLAG_READONLY, sample->data, sample->max_size,
>> +            0, sample->size, sample, wg_sample_free_notify);
>>       if (!buffer)
>> +    {
>> +        InterlockedDecrement(&sample->refcount);
>>           return STATUS_NO_MEMORY;
>> +    }
> 
> We don't need to incref before calling gst_buffer_new_wrapped_full(), do
> we? We haven't passed the buffer to anything yet.
> 


Sure, it seemed more correct to increment refount before any use, as 
gst_buffer_new_wrapped_full could do anything, and decrement after it 
failed to wrap it.


>> @@ -537,14 +536,16 @@ static HRESULT WINAPI transform_ProcessInput(IMFTransform *iface, DWORD id, IMFS
>>       if (!decoder->wg_transform)
>>           return MF_E_TRANSFORM_TYPE_NOT_SET;
>>   
>> -    if (decoder->input_sample)
>> +    if (!list_empty(&decoder->input_samples))
>>           return MF_E_NOTACCEPTING;
> 
> Why store samples in a list, if we're still only going to limit it to
> one at a time?


Right now we do, and with WMA it'll probably always be the case, but 
it's not a requirement and using a list makes the queueing mechanism 
reusable in other transforms which do not share the same pattern.

Later the check will also be moved to the unix side and return 
MF_E_NOTACCEPTING if there's output sample read, which may not 
correspond to a single input sample being queued.

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