[PATCH 0/5] MR197: winegstreamer: Add support for zero-copy in wg_transform.

[Zebediah Figura]

On 6/8/22 03:37, Rémi Bernon (@rbernon) wrote:
>> Could we do this without the callback? Just thinking out loud, maybe
>> something like
>>
>> void wg_allocator_add_sample(WgAllocator *allocator,
>>           struct wg_sample *sample);
>> void wg_allocator_remove_sample(WgAllocator *allocator,
>>           struct wg_sample *sample);
>>
>> which manipulate an internal wg_sample pointer, instead of having them
>> access the wg_transform's sample pointer. Then
>> wg_allocator_remove_sample() would also end up calling
>> wg_allocator_release_sample().
>>
>> I suspect I'm missing something, though, given my below comment...
> 
> The callback is with a future use from wg_parser in mind. In that case
> we would forward allocation requests to the read thread (for instance).

Ah, in order to allocate ad-hoc samples, since we don't have the same 
requirement for the output buffer?

It seems plausible, but I was also inclined to avoid this from the 
beginning due to the increased code complexity. Setting up a pool of 
wg_samples when connecting seems like a potentially better option, for 
instance.

>> Had to look this up, but apparently the default GstBufferPool will throw
>> away buffers if they're modified. I thought the "empty" buffers would
>> pile up and never be freed, but evidently not.
>>
>> I've probably had this question answered before, but can we replace the
>> buffer pool itself, instead of just the allocator?
> 
> We could reimplement a custom buffer pool that always free buffers, but
> that would be more work and still won't solve the underlying problem.
> 
> The problem the allocator is solving is that some decoders don't release
> their buffers to the pool. The gst_buffer_is_writable check catches that
> and gst_buffer_replace_all_memory would fail anyway if we're not the
> only ones holding a ref on the buffer. So instead of discarding the data
> we copy it back into the buffer unix memory.

Right, but that's the problem that the allocator is solving on unmap, 
which is orthogonal to the "don't reuse this buffer" problem.

I guess that overriding unmap requires having a custom allocator, so I 
should ask if we can replace the pool *as well as* the allocator.

> 
> Later, when the decoder will eventually release the buffer to the pool,
> it will be untainted, and so not freed until the pool is destroyed, and
> reusable without going through the allocator. After a few iterations the
> pool has enough allocated buffers, we won't get any allocation requests
> anymore and our samples are never used directly and we copy the data
> normally.
> 

Actually, wait, isn't this a problem we'd be able to fix *only* by using 
a custom pool? Why doesn't the current patch 4/5 suffer from this problem?