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

[Zebediah Figura (she/her)]

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
>
> 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?
> +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.
> 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...
> - 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.
> @@ -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.
> @@ -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?