Created attachment 135744 [details] [review]
handle already pad_alloced buffers

This patch tracks pad_alloced buffers in gst_base_transform_buffer_alloc(). If an input buffer is pad_alloced we can skip the pad_alloc in gst_base_transform_prepare_output_buffer() as reverse negotiation has been already handled.

For the before mentioned example, this brings the unneeded pad_allocs down to:
1382

no ref is kept to the last allocated buffer, I guess you can have false positives if the buffer is deleted and a buffer is allocated that happens to have the same memory.

One of the ideas I had back in the days was to keep track of the proxy alloc buffers by making a subclass of them, but that did not seem to be working so well because of the subbuffer/subclassing limitations.

I guess we also don't really want to keep a ref to the alloc buffer to avoid keeping it around longer than needed. We also can't install a custom free function that invalidates the pointer we keep to the buffer because there is no room for user data.

So, I don't know.

I still have to explanation for the remaining 1382. I added some logging and get a lot of elements with same_output=1, but the pad_alloced buffer is not passed as an input:
<gain_0x8448198> same_output=1 last_buffer=0xb2936820 in_buf=0xb2902d58

the upstream called pad_alloc for volume's sink pad, but then pushes a different buffer. It might be caused be other unneeded pad_allocs from upstream. Adding a hashtable to track all pad_allocs and drop the ones which are seen in prepare output buffer sounds expensive.

Created attachment 139068 [details] [review]
track pad_alloced buffers in a hashtable

I am giving up on this for now. What we need here is beeing able to distinguish normal alloced buffers from pad_alloced buffers. If a buffer has been created via pad_alloc, reverse-negotiation would already have been taken care of.

In the new patch I replace the free_func in the buffer with an own and chain up on destruction. I can identify pad_alloced buffers by checking if the free-func is mine.

problems:
* in the free function I have no context (need to make the hash table global and protect with a lock).
* if an element overrides pad_alloc we don't mark it :/ Even though that basetransform could check if that method is overridden, it does not tell that it was used to create that buffer (we could if we would require to chain up).

In my tests this till catches 2/3 of the cases though.

One last idea would be adding a buffer flag, to indicate that the buffer has been allocated using pad_alloc. This would have the same effect as the current code, but is faster and does not leak buffers. The efficiency could be improved by adding usage of the flag to functions that override pad_alloc.

Closing this as a dupe, as the other bug has a working patch. The remaining pad_allocs should be fixed with negotiation events.

*** This bug has been marked as a duplicate of bug 642373 ***

Sorry for the noise, different problem.

Are you still going to work on this? In 0.11 this is no problem anymore

Unless someone has a simple enough idea how to fix it in 0.10 I am fine with WONTFIX. I read the "In 0.11 this is no problem anymore" as it does not happpen, right?

There's no gst_pad_alloc_buffer() in 0.11 anymore, this is done via buffer pools now. Buffer pools are distributed to elements by the ALLOCATION query.

And bufferpools/allocations/etc are separated from (re-)negotiation now, which was the main reason why basetransform was doing lots of unneeded pad-allocs