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

[Rémi Bernon (@rbernon)]

> 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.


> >> 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.


> > 
> > 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).

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/197#note_1726