[FFmpeg-devel] [PATCH] Added support for MB_INFO

[Timo Rothenpieler]

On 17.06.2022 12:59, Carotti, Elias wrote:
>>
>> Yes, exactly. It relies on x264 to free it.
> 
> Not really. It can rely on x264 if you explicitly want that behavior.
> If you do not set a deallocator, it remains the caller responsibility.
> 
>> What happens if x264 is not involved, but some other encoder, which
>> does
>> not check for that kind of side data?
>>
>> How does the caller know that the data was consumed, and the
>> ownership
>> passed on?
> 
> Again, you don't have to pass the ownership, and in fact in my use case
> I do not pass it since I actually recycle and update the same buffer
> for subsequent frames. If you do intentionally pass the ownership you
> need to be aware of what you are doing. As I said, I see a leak only in
> case of a programming error.
> Maybe we could explicitly state it in the comment section in mb_info.h:
> if you set the deallocator you're relinquishing ownership of the
> buffer.

I'm not sure if that's something you can sensibly do with side data?
What if it gets buffered, copied, and so on?

> Plus, there's one more flag (b_mb_info_update) in libx264 which allows
> to pass back information to the caller about which macroblocks among
> the flagged ones were actually encoded as P_SKIP. I did not add support
> for that though but in the future somebody may want to.

Yes, it's very x264 specific.
But the side data is generic. If for some reason x264 does not process a 
frame, or any other encoder ends up getting used, the data will leak if 
it relied on x264 to free it.

> In principle I could've put the buffer into the side information  and
> not just pass a pointer to it but I thought that it would require
> adding more semantics which would imply a stronger dependency on
> libx264.
> Right now, mb_info is a vector of uint8_t flags and the only possible
> value - to date - is X264_MBINFO_CONSTANT. What if, say, one day
> libx264 decides the buffer has to be a vector of uint16_t or still
> uint8_t but the flags are packed? On the other hand, this, AFAIK, is
> only supported by libx264. Other codecs may want to choose a different
> semantic for a similar field and the only possibility to make it
> generic is letting the caller handling the low level details.

I'm not aware of any other side data with a similar semantic. And I'm 
really not sure if it's sane or even valid to do it like that.
Can't the data be entirely contained within the side data?