[FFmpeg-devel] [PATCH 2/7] decode: add a method for attaching lavc-internal data to frames

[Tobias Rapp]

On 04.10.2017 11:34, wm4 wrote:
> On Wed, 4 Oct 2017 11:22:37 +0200
> Michael Niedermayer <michael at niedermayer.cc> wrote:
> 
>> On Wed, Oct 04, 2017 at 09:12:29AM +0200, wm4 wrote:
>>> On Tue, 3 Oct 2017 21:40:58 +0200
>>> Michael Niedermayer <michael at niedermayer.cc> wrote:
>>>    
>>>> On Tue, Oct 03, 2017 at 03:15:13PM +0200, wm4 wrote:
>>>>> From: Anton Khirnov <anton at khirnov.net>
>>>>>      
>>>>    
>>>>> Use the AVFrame.opaque_ref field. The original user's opaque_ref is
>>>>> wrapped in the lavc struct and then unwrapped before the frame is
>>>>> returned to the caller.
>>>>
>>>> this is a ugly hack
>>>>
>>>> one and the same field should not be used to hold both the
>>>> users opaque_ref as well as a structure which is itself not a user
>>>> opaque_ref
>>>
>>> While the AVFrame is within libavcodec, it's libavcodec's frame, not
>>> the user's. Thus your claim doesn't make too much sense. libavcodec
>>> fully controls the meaning for its own AVFrames' opaque_ref, but
>>> reconstruct the user's value when returning it.
>>
>> i disagree
> 
> Well, you're wrong anyway.
> 
>> such hacks should not be added, we do have enough hacks already
> 
> It's not a hack.

Changing the semantics of a field during its lifetime, even when only 
done within private code, is at least unexpected behavior.

>> AVFrames are not really seperated into isolated classes
>> There arent "the users AVFrames" vs. "the internal AVFrames"
> 
> Oh yes, there is the concept of an "owner" of an AVFrame, and the owner
> of the AVFrame decides what opaque_ref means. It's quite literally for
> free use by the owner of the reference.
> 
> That the code goes through some acrobatics to preserve opaque_ref as
> passed in by get_buffer is just a feature.
> 
>> its fragile to create and maintain such seperation with interfaces
>> that all wrap and unwrap the opaque_ref. Any mistake being a potential
>> security issue and or crash
> 
> This is done strictly when returning a valid AVFrame, so there is no
> room for mistakes.

The room for mistake might not increase for external developers but it 
increases for internal developers (maintenance cost).

>> Its MUCH more robust and also easier to understand to use a sperate
>> field
> 
> No, it's not. If you fail to do call post_process() on returned
> AVFrames (which is done by the same code which exchanges opaque_ref)
> you have bugs that violate the API or crash anyway.
> 
>> more so, opaque_ref is used in only 5 lines in the whole codebase,
>> so there is not much code to consider when using a different solution
> 
> We shouldn't add such special fields, we have enough hacks already. Is
> that your only suggestion how to do this? Because it's a bad one.

What would be the drawback of using a separate field?

And please try to reduce the amount of emotions in this thread, and 
increase the amount of objective reasons.

Regards,
Tobias