On 3/2/2018 6:27 PM, wm4 wrote: > On Fri, 2 Mar 2018 18:17:30 -0300 > James Almer <jamr...@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 <jamr...@gmail.com> wrote: >>>>> >>>>>> On 3/2/2018 1:47 PM, wm4 wrote: >>>>>>> On Fri, 2 Mar 2018 13:11:35 -0300 >>>>>>> James Almer <jamr...@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. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
