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

[wm4]

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.

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