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

[Zebediah Figura]

On 6/7/22 03:43, Rémi Bernon wrote:
> +static void wg_sample_queue_append(struct wg_sample_queue *queue, struct wg_sample *wg_sample)
> +{
> - struct mf_sample *mf_sample = CONTAINING_RECORD(wg_sample, struct mf_sample, wg_sample);
> -
> - /* make sure a concurrent wg_sample_queue_release call won't release the sample until we're done */
> - InterlockedIncrement(&wg_sample->refcount);
> - mf_sample->wg_sample.flags |= WG_SAMPLE_FLAG_HAS_REFCOUNT;
> -
> - EnterCriticalSection(&queue->cs);
> - list_add_tail(&queue->samples, &mf_sample->entry);
> - LeaveCriticalSection(&queue->cs);
>
> +}
> -
>
> +void wg_sample_queue_release(struct wg_sample_queue *queue, struct wg_sample *wg_sample, bool all)
>   {
> - struct mf_sample *mf_sample = CONTAINING_RECORD(sample, struct mf_sample, wg_sample);
> - struct mf_sample *mf_sample, *next;
> -
> - /* release temporary ref taken in wg_sample_queue_append */
> - if (wg_sample)
> -
>    InterlockedDecrement(&wg_sample->refcount);
> -
> - EnterCriticalSection(&queue->cs);
> -
> - LIST_FOR_EACH_ENTRY_SAFE(mf_sample, next, &queue->samples, struct mf_sample, entry)
> - {
> -
>    if (!InterlockedOr(&mf_sample->wg_sample.refcount, 0) || all)
> -
>    {
> -
>        list_remove(&mf_sample->entry);
> -
>        wg_sample_release(&mf_sample->wg_sample);
> -
>    }
> - }
> -
> - LeaveCriticalSection(&queue->cs);
>
> +}

This feels awkward, partly because it does two things at once, partly 
because of the lack of symmetry. But the lack of symmetry is kind of 
necessary...

(Maybe bring the incref/decref into the caller?)

I'd at least suggest to make most of that a separate helper like 
"wg_sample_queue_flush()" which doesn't take the specific sample 
argument, and have wg_sample_queue_release() decref the sample and then 
call wg_sample_queue_flush().