[FFmpeg-devel] [PATCH v2] lavu/frame: add QP side data

[James Almer]

On 3/2/2018 6:27 PM, wm4 wrote:
> On Fri, 2 Mar 2018 18:17:30 -0300
> James Almer <jamrial at gmail.com> wrote:
> 
>> On 3/2/2018 5:54 PM, Michael Niedermayer wrote:
>>> On Fri, Mar 02, 2018 at 03:36:02PM -0300, James Almer wrote:  
>>>> On 3/2/2018 3:19 PM, wm4 wrote:  
>>>>> On Fri, 2 Mar 2018 14:30:28 -0300
>>>>> James Almer <jamrial at gmail.com> wrote:
>>>>>  
>>>>>> On 3/2/2018 1:47 PM, wm4 wrote:  
>>>>>>> On Fri, 2 Mar 2018 13:11:35 -0300
>>>>>>> James Almer <jamrial at gmail.com> wrote:
>>>>>>>  
>>>>>>>> On 3/2/2018 8:16 AM, wm4 wrote:  
>>>>>>>>> This adds a way for an API user to transfer QP data and metadata without
>>>>>>>>> having to keep the reference to AVFrame, and without having to
>>>>>>>>> explicitly care about QP APIs. It might also provide a way to finally
>>>>>>>>> remove the deprecated QP related fields. In the end, the QP table should
>>>>>>>>> be handled in a very similar way to e.g. AV_FRAME_DATA_MOTION_VECTORS.
>>>>>>>>>
>>>>>>>>> There are two side data types, because I didn't care about having to
>>>>>>>>> repack the QP data so the table and the metadata are in a single
>>>>>>>>> AVBufferRef. Otherwise it would have either required a copy on decoding
>>>>>>>>> (extra slowdown for something as obscure as the QP data), or would have
>>>>>>>>> required making intrusive changes to the codecs which support export of
>>>>>>>>> this data.    
>>>>>>>>
>>>>>>>> Why not add an AVBufferRef pointer to the qp_properties struct instead?
>>>>>>>> You don't need to merge the properties fields into the buffer data.  
>>>>>>>
>>>>>>> Not sure what you mean. The whole purpose of this is _not_ to add new
>>>>>>> pointers because it'd require an API user to deal with extra fields
>>>>>>> just for QP. I want to pretend that QP doesn't exist.  
>>>>>>
>>>>>> I mean keep the buffer and the int fields all in a single opaque (for
>>>>>> the user) struct handled by a single side data type. The user still only
>>>>>> needs to worry about using the get/set functions and nothing else.
>>>>>>
>>>>>> See the attached, untested PoC to get an idea of what i mean.
>>>>>>
>>>>>> If I'm really missing the entire point of this patch (Which i don't
>>>>>> discard may be the case), then ignore this.  
>>>>>
>>>>> That would be nice, but unfortunately it's not allowed. An API user can
>>>>> treat side data as byte arrays, and e.g. copy & restore it somewhere
>>>>> (presumably even if the data is opaque and implementation defined).
>>>>>
>>>>> So the side data can't contain any pointers. The user could copy
>>>>> the byte data, unref the AVBufferRef, and later add it back as side
>>>>> data using the copied bytes. Then it'd contain a dangling pointer.  
>>>>
>>>> Afaik, ref counting was added for frame side data because
>>>> sizeof(AVFrameSideData) is not part of the ABI, meaning that users are
>>>> not supposed to manipulate side data with anything except the provided
>>>> API functions (new, remove, get, and now new_from_buf). Or at least that
>>>> was agreed in the relevant thread from some time ago.
>>>> This is not the case for AVPacketSideData, which is probably why it
>>>> never got a ref count upgrade.
>>>>  
>>>>>
>>>>> The side data merging (which we even still provide as API) would be an
>>>>> application for that - it's for AVPacket, but there's nothing that
>>>>> prevents the same assumptions with AVFrames.
>>>>>
>>>>> Unless we decide that at least AVFrame side data can contain pointers,
>>>>> and the user must strictly use the AVBufferRef to manage the life time
>>>>> of the data. Maybe I'm just overthinking this.  
>>>>
>>>> As i said, since the user is not expected to manipulate the side data
>>>> manually, then i don't see why it can't have pointers of any kind.  
>>>
>>> The user has to pass the data she gets from the demuxer to the decoder,
>>> from the decoder to filters from there to an encoder and then to a muxer.
>>>
>>> If byte per byte copying of side data is not possible anymore how would
>>> the user do this ?  
>>
>> AVFrame side data is supposedly ref counted. Shouldn't av_frame_ref()
>> and av_frame_copy_props() both create new references of all source side
>> data as they do for the actual frame data and countless other
>> AVBufferRefs defined in AVFrame, and not do a memcpy of
>> AVFrameSideData->data from src to dst frame?
> 
> Whether it's refcounted or not doesn't matter at all.

I guess I'm missing something here, but if everyone else thinks having
pointers in side data is not possible then I'm not going to argue any
further.
With the new av_frame_new_side_data_from_buf() allowing us to create
side data using AVBufferRef with custom free() functions, this seems
like a no brainer now.

There's also the opaque pointer in AVBufferRef to hold custom
data/structs. Would that be good to work around the problems or side
data copying you're worried about?

> 
>>> Consider the user is most likely not basing her whole app on AVPackets
>>> So she has to turn an AVPacket into something that can be passed within
>>> the framework and language the application uses. 
>>> So some form of generic array <-> side data copy or (de)serialization
>>> would likely be needed  
>>
>> This is not about AVPackets. Those currently can be freely manipulated
>> as the user wishes.
>>
>> It was established back when AVFrame refcounting was introduced that
>> users are not to manipulate frame side data manually, and must only use
>> the provided API.
> 
> Is that even documented anywhere? Sigh.

No. The side data fields have no doxy, even.