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. > > 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. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel