| Message ID | 8eda0a9a-b4ea-18c6-231c-668509b17e9b@gmail.com |
|---|---|
| State | Withdrawn |
| Headers | show |
On Fri, 2 Mar 2018 14:30:28 -0300 James Almer <jamrial@gmail.com> wrote: > On 3/2/2018 1:47 PM, wm4 wrote: > > On Fri, 2 Mar 2018 13:11:35 -0300 > > James Almer <jamrial@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. 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.
On 3/2/2018 3:19 PM, wm4 wrote: > On Fri, 2 Mar 2018 14:30:28 -0300 > James Almer <jamrial@gmail.com> wrote: > >> On 3/2/2018 1:47 PM, wm4 wrote: >>> On Fri, 2 Mar 2018 13:11:35 -0300 >>> James Almer <jamrial@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.
On Fri, 2 Mar 2018 15:36:02 -0300 James Almer <jamrial@gmail.com> wrote: > On 3/2/2018 3:19 PM, wm4 wrote: > > On Fri, 2 Mar 2018 14:30:28 -0300 > > James Almer <jamrial@gmail.com> wrote: > > > >> On 3/2/2018 1:47 PM, wm4 wrote: > >>> On Fri, 2 Mar 2018 13:11:35 -0300 > >>> James Almer <jamrial@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. I disagree. It appears the user is free to do with the side data whatever he pleases, as long as he doesn't violate the underlying ABI constraints (such as changing the sizer of side data backed by a struct). But since all side data comes with a size, the data can be freely copied around. The side data merging I mentioned is an use case of this. > > > > 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. If you think it's OK and others agree, I don't mind. Better to have a single QP side data than 2. If we do this, we should probably add a note to this effect.
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@gmail.com> wrote: > > > >> On 3/2/2018 1:47 PM, wm4 wrote: > >>> On Fri, 2 Mar 2018 13:11:35 -0300 > >>> James Almer <jamrial@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 ? 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 [...]
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@gmail.com> wrote: >>> >>>> On 3/2/2018 1:47 PM, wm4 wrote: >>>>> On Fri, 2 Mar 2018 13:11:35 -0300 >>>>> James Almer <jamrial@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? > 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.
On Fri, 2 Mar 2018 18:17:30 -0300 James Almer <jamrial@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@gmail.com> wrote: > >>> > >>>> On 3/2/2018 1:47 PM, wm4 wrote: > >>>>> On Fri, 2 Mar 2018 13:11:35 -0300 > >>>>> James Almer <jamrial@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.
On Fri, Mar 02, 2018 at 06:17:30PM -0300, James Almer 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@gmail.com> wrote: > >>> > >>>> On 3/2/2018 1:47 PM, wm4 wrote: > >>>>> On Fri, 2 Mar 2018 13:11:35 -0300 > >>>>> James Almer <jamrial@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? > > > 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. AVPacket between the (de)mxuer/(de)coder, AVFrames between the remaining parts. I worded this inprecisse without realizing > > 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. I dont think treating side data for AVPackets differently than AVFrames is a good idea. That would be unexpected, confusing, inconsistent and when the same side data is passed through both it could bite if a design that works only in one case was already part of the puiblic API [...]
On 3/2/2018 6:27 PM, wm4 wrote: > On Fri, 2 Mar 2018 18:17:30 -0300 > James Almer <jamrial@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@gmail.com> wrote: >>>>> >>>>>> On 3/2/2018 1:47 PM, wm4 wrote: >>>>>>> On Fri, 2 Mar 2018 13:11:35 -0300 >>>>>>> James Almer <jamrial@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.
On 3/2/2018 6:38 PM, Michael Niedermayer wrote: > On Fri, Mar 02, 2018 at 06:17:30PM -0300, James Almer 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@gmail.com> wrote: >>>>> >>>>>> On 3/2/2018 1:47 PM, wm4 wrote: >>>>>>> On Fri, 2 Mar 2018 13:11:35 -0300 >>>>>>> James Almer <jamrial@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? >> >>> 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. > > AVPacket between the (de)mxuer/(de)coder, AVFrames between the remaining > parts. I worded this inprecisse without realizing > > >> >> 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. > > I dont think treating side data for AVPackets differently than AVFrames > is a good idea. > That would be unexpected, confusing, inconsistent and > when the same side data is passed through both it could bite if a design > that works only in one case was already part of the puiblic API What can't be done with frame side data is write your own implementation of functions to add, copy or remove elements, etc. If you had one back when ref counting was added, you'd have suddenly found yourself with non working code. Packet side data exist as a pointer to an array of consecutive AVPacketSideData inside the packet, whereas frame side data exist as pointer to an array of AVFrameSideData pointers inside the frame. The size of the former is part of the ABI, whereas the latter isn't. We really need a single, generic side data handling API.
On Fri, 2 Mar 2018 18:39:27 -0300 James Almer <jamrial@gmail.com> wrote: > On 3/2/2018 6:27 PM, wm4 wrote: > > On Fri, 2 Mar 2018 18:17:30 -0300 > > James Almer <jamrial@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@gmail.com> wrote: > >>>>> > >>>>>> On 3/2/2018 1:47 PM, wm4 wrote: > >>>>>>> On Fri, 2 Mar 2018 13:11:35 -0300 > >>>>>>> James Almer <jamrial@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. I think there's a real issue with API users potentially "transporting" side data across their own framework's data types, and assuming that side data contains just flat byte arrays. After all, AVPacket side data doesn't even _have_ an AVBufferRef, so why should AVFrame side data subtly and dangerously be different? Maybe it doesn't matter, and we should redefine it. So we say that the side data could contain internal pointers, and everyone has to use AVBufferRef refcounting to avoid dangling pointers or memory leaks. This is sort of an API change, but if nothing actually relied on this (I don't know), we could get away with claiming it's an ABI change. I could send such a patch, and adjust the QP patch to use your suggestion (with a nested QP table AVBufferRef), if everyone agrees? This issue is so annoying. It's 100% trivial, but unclear API docs, code duplication, and lack of consistency makes it a mess. Like you said, a generic side data thing would be good, but then I don't know how to make it not conflict with the old API. Also, this is probably already more time I'd like to have spent on this QP stuff. Might as well have modified the few codecs to setup the QP buffer with the metadata (type/stride/offset) included inline. > 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? Do you mean in AVBuffer, as in the argument to the free function? I don't think that should hold anything other than memory management stuff (so it doesn't apply here). > > > >>> 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.
diff --git a/libavutil/frame.c b/libavutil/frame.c index 0db2a2d57b..b349ff84fe 100644 --- a/libavutil/frame.c +++ b/libavutil/frame.c @@ -46,8 +46,28 @@ MAKE_ACCESSORS(AVFrame, frame, enum AVColorRange, color_range) av_get_channel_layout_nb_channels((frame)->channel_layout)) #if FF_API_FRAME_QP +struct qp_properties { + AVBufferRef *buf; + int stride; + int type; +}; + +// Free the qp_properties struct and the QP data buffer +// To be used as the free() function for the side data buffer. +static void frame_qp_free(void *opaque, uint8_t *data) +{ + struct qp_properties *p = (struct qp_properties *)data; + + av_buffer_unref(&p->buf); + av_free(data); +} + int av_frame_set_qp_table(AVFrame *f, AVBufferRef *buf, int stride, int qp_type) { + struct qp_properties *p = NULL; + AVFrameSideData *sd; + AVBufferRef *sd_buf = NULL, *ref = NULL; + FF_DISABLE_DEPRECATION_WARNINGS av_buffer_unref(&f->qp_table_buf); @@ -57,20 +77,69 @@ FF_DISABLE_DEPRECATION_WARNINGS f->qscale_type = qp_type; FF_ENABLE_DEPRECATION_WARNINGS + av_frame_remove_side_data(f, AV_FRAME_DATA_QP_TABLE); + + // Create a new QP data table ref for the side data + ref = av_buffer_ref(buf); + if (!ref) + return AVERROR(ENOMEM); + + // Alloc the qp_properties struct + p = av_malloc(sizeof(struct qp_properties)); + if (!p) + goto fail; + + // Create a buffer to be used in side data, containing the qp_properties struct. + // Use a custom free() function to properly unref the QP table buffer when the side data + // is removed. + sd_buf = av_buffer_create((uint8_t *)p, sizeof(struct qp_properties), frame_qp_free, NULL, 0); + if (!sd_buf) + goto fail; + + // Add the buffer containing the qp_properties struct as side data + sd = av_frame_new_side_data_from_buf(f, AV_FRAME_DATA_QP_TABLE, sd_buf); + if (!sd) + goto fail; + + // Fill the prop fields and the QP table buffer. + p->stride = stride; + p->type = qp_type; + p->buf = ref; + return 0; +fail: + av_buffer_unref(&ref); + av_buffer_unref(&sd_buf); + av_free(p); + return AVERROR(ENOMEM); } int8_t *av_frame_get_qp_table(AVFrame *f, int *stride, int *type) { -FF_DISABLE_DEPRECATION_WARNINGS - *stride = f->qstride; - *type = f->qscale_type; + AVBufferRef *buf = NULL; - if (!f->qp_table_buf) - return NULL; + *stride = 0; + *type = 0; - return f->qp_table_buf->data; +FF_DISABLE_DEPRECATION_WARNINGS + if (f->qp_table_buf) { + *stride = f->qstride; + *type = f->qscale_type; + buf = f->qp_table_buf; FF_ENABLE_DEPRECATION_WARNINGS + } else { + AVFrameSideData *sd; + struct qp_properties *p; + sd = av_frame_get_side_data(f, AV_FRAME_DATA_QP_TABLE); + if (!sd) + return NULL; + p = (struct qp_properties *)sd->data; + *stride = p->stride; + *type = p->type; + buf = p->buf; + } + + return buf ? buf->data : NULL; } #endif @@ -787,6 +856,7 @@ const char *av_frame_side_data_name(enum AVFrameSideDataType type) case AV_FRAME_DATA_CONTENT_LIGHT_LEVEL: return "Content light level metadata"; case AV_FRAME_DATA_GOP_TIMECODE: return "GOP timecode"; case AV_FRAME_DATA_ICC_PROFILE: return "ICC profile"; + case AV_FRAME_DATA_QP_TABLE: return "QP table"; } return NULL; } diff --git a/libavutil/frame.h b/libavutil/frame.h index ddbac3156d..dd1917882b 100644 --- a/libavutil/frame.h +++ b/libavutil/frame.h @@ -141,6 +141,16 @@ enum AVFrameSideDataType { * metadata key entry "name". */ AV_FRAME_DATA_ICC_PROFILE, + +#if FF_API_FRAME_QP + /** + * QP table data. + * The contents of this side data are undocumented and internal; use + * av_frame_set_qp_table() and av_frame_get_qp_table() to access this in a + * meaningful way instead. + */ + AV_FRAME_DATA_QP_TABLE, +#endif }; enum AVActiveFormatDescription {