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

[Zebediah Figura]

On 6/8/22 17:10, Rémi Bernon (@rbernon) wrote:
>> 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.
> 
> I don't think so, we simply cannot know how many buffer GStreamer is
> going to need. Allocating them when it requests them is the right
> solution here, it's also simpler.

Sure, conceptually it's simpler, but the callbacks make it really ugly. 
(Also the fact that it's so different from the API for the transform...)

We can't exactly predict how many buffers a GStreamer pipeline needs 
(well, unless it tells us through the buffer pool config), but we don't 
necessarily need to, either; we can allocate new buffers and blit if our 
pool wasn't large enough, and just make a point of always allocating 
"enough" PE buffers in practice.

> 
> 
>>>> 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.
> 
> Re-implementing a pool doesn't help anything. The default implementation
> already frees the buffers when their memory is tainted, we don't need
> anything more.

Well, sure. I just ask partly because tainting memory is not the most 
obvious way of accomplishing our goals, and I wanted to see if other 
options were considered. (Also, at this point, because of the problem 
described below.)

> 
> 
>>>
>>> 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?
> 
> I don't understand, it's not really a problem and the code handles that
> case with the discard_data option at the same time as it handles format
> changes and partial reads (which should not happen anyway).
> 

If we read an entire buffer but the decoder is still holding onto the 
GstBuffer, we won't be able to "taint" it because it's not writable, but 
we'll still remove the wg_sample. Later when the decoder does free the 
GstBuffer it'll be untainted and hence can be reused. Hence the buffer 
pool will pick up that buffer instead of the zero-copy buffer we want.