[PATCH 5/5] winegstreamer: Support zero-copy in wg_transform_push_data.
Zebediah Figura (she/her)
zfigura at codeweavers.com
Wed Feb 23 15:43:32 CST 2022
On 2/23/22 02:22, Rémi Bernon wrote:
> 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.
I meant just implementing sample processing first without the zero-copy
optimization, as you seem to have done in v2.
>
>
>>> +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.
Sure, on reflection I think that makes sense.
>
>
>>> 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.
Okay, makes sense.
>
>
>>> + 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.
I'll admit I'm not thrilled about it. I can see the logic behind making
it a single coherent object; what I think is made less clear this way is
the synchronization model (for those fields in particular). Still, I can
probably live with it...
>
>
>>> @@ -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.
>
I guess...
>
>>> @@ -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.
>
Okay, I guess I can live with that...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 495 bytes
Desc: OpenPGP digital signature
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20220223/5ff00505/attachment.sig>
More information about the wine-devel
mailing list