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