| Message ID | 20180214024324.10260-1-muken.the.vfrmaniac@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 2/13/2018 11:43 PM, Yusuke Nakamura wrote: > This makes you need not call av_init_packet before av_packet_copy_props like the following. > > AVPacket dst; > av_packet_copy_props(&dst, &src); In this scenario, dst->side_data is uninitialized, and bad things can happen when av_packet_copy_props calls av_packet_new_side_data. For something like this, it's best to instead just zero initialize dst. Also, av_packet_init should *always* be called before a packet stored on stack is used, regardless of it being zero initialized or not. > --- > libavcodec/avpacket.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c > index 90b8215928..1a9be60e20 100644 > --- a/libavcodec/avpacket.c > +++ b/libavcodec/avpacket.c > @@ -571,6 +571,7 @@ FF_ENABLE_DEPRECATION_WARNINGS > dst->flags = src->flags; > dst->stream_index = src->stream_index; > > + dst->side_data_elems = 0; > for (i = 0; i < src->side_data_elems; i++) { > enum AVPacketSideDataType type = src->side_data[i].type; > int size = src->side_data[i].size; > Afaik, the intended behavior of this function was to merge the side data in dst with that of src, and this patch would break that. It's admittedly not really defined and can get confusing, especially when the old deprecated API (av_copy_packet, av_copy_packet_side_data, av_dup_packet) do seem to just completely overwrite rather than merge. IMO, we should first define what should happen with side data in this function before we make any further changes to it.
2018-02-14 12:11 GMT+09:00 James Almer <jamrial@gmail.com>: > On 2/13/2018 11:43 PM, Yusuke Nakamura wrote: > > This makes you need not call av_init_packet before av_packet_copy_props > like the following. > > > > AVPacket dst; > > av_packet_copy_props(&dst, &src); > > In this scenario, dst->side_data is uninitialized, and bad things can > happen when av_packet_copy_props calls av_packet_new_side_data. > > For something like this, it's best to instead just zero initialize dst. > Also, av_packet_init should *always* be called before a packet stored on > stack is used, regardless of it being zero initialized or not. > Ooops! Sorry, originally it is for the case using av_packet_ref not av_packet_copy_props. av_packet_ref expects the dst packet is inited by av_packet_init? If so, should be documented on av_packet_ref docs I think. I'm trapped that. > > --- > > libavcodec/avpacket.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c > > index 90b8215928..1a9be60e20 100644 > > --- a/libavcodec/avpacket.c > > +++ b/libavcodec/avpacket.c > > @@ -571,6 +571,7 @@ FF_ENABLE_DEPRECATION_WARNINGS > > dst->flags = src->flags; > > dst->stream_index = src->stream_index; > > > > + dst->side_data_elems = 0; > > for (i = 0; i < src->side_data_elems; i++) { > > enum AVPacketSideDataType type = src->side_data[i].type; > > int size = src->side_data[i].size; > > > > Afaik, the intended behavior of this function was to merge the side data > in dst with that of src, and this patch would break that. > It's admittedly not really defined and can get confusing, especially > when the old deprecated API (av_copy_packet, av_copy_packet_side_data, > av_dup_packet) do seem to just completely overwrite rather than merge. > > IMO, we should first define what should happen with side data in this > function before we make any further changes to it. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On 2/14/2018 12:22 AM, Yusuke Nakamura wrote: > 2018-02-14 12:11 GMT+09:00 James Almer <jamrial@gmail.com>: > >> On 2/13/2018 11:43 PM, Yusuke Nakamura wrote: >>> This makes you need not call av_init_packet before av_packet_copy_props >> like the following. >>> >>> AVPacket dst; >>> av_packet_copy_props(&dst, &src); >> >> In this scenario, dst->side_data is uninitialized, and bad things can >> happen when av_packet_copy_props calls av_packet_new_side_data. >> >> For something like this, it's best to instead just zero initialize dst. >> Also, av_packet_init should *always* be called before a packet stored on >> stack is used, regardless of it being zero initialized or not. >> > > Ooops! Sorry, originally it is for the case using av_packet_ref not > av_packet_copy_props. > av_packet_ref expects the dst packet is inited by av_packet_init? It needs to be in a "clean" state, be it because it was just allocated by av_packet_alloc, or right after having zero initialized it or called av_packet_init or av_packet_unref, because otherwise dst->buf will be either uninitialized or wrongly pointing to something (Apparently only an issue if the source packet is not ref counted, though). Similarly, what i said above regarding dst->side_data. > If so, should be documented on > av_packet_ref docs I think. I'm trapped that. I don't know if this is the intended behavior, but that's how it currently works. Maybe that should be fixed rather than the documentation, after it's properly defined. > > >>> --- >>> libavcodec/avpacket.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c >>> index 90b8215928..1a9be60e20 100644 >>> --- a/libavcodec/avpacket.c >>> +++ b/libavcodec/avpacket.c >>> @@ -571,6 +571,7 @@ FF_ENABLE_DEPRECATION_WARNINGS >>> dst->flags = src->flags; >>> dst->stream_index = src->stream_index; >>> >>> + dst->side_data_elems = 0; >>> for (i = 0; i < src->side_data_elems; i++) { >>> enum AVPacketSideDataType type = src->side_data[i].type; >>> int size = src->side_data[i].size; >>> >> >> Afaik, the intended behavior of this function was to merge the side data >> in dst with that of src, and this patch would break that. >> It's admittedly not really defined and can get confusing, especially >> when the old deprecated API (av_copy_packet, av_copy_packet_side_data, >> av_dup_packet) do seem to just completely overwrite rather than merge. >> >> IMO, we should first define what should happen with side data in this >> function before we make any further changes to it. >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Wed, 14 Feb 2018 00:11:32 -0300 James Almer <jamrial@gmail.com> wrote: > > --- > > libavcodec/avpacket.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c > > index 90b8215928..1a9be60e20 100644 > > --- a/libavcodec/avpacket.c > > +++ b/libavcodec/avpacket.c > > @@ -571,6 +571,7 @@ FF_ENABLE_DEPRECATION_WARNINGS > > dst->flags = src->flags; > > dst->stream_index = src->stream_index; > > > > + dst->side_data_elems = 0; > > for (i = 0; i < src->side_data_elems; i++) { > > enum AVPacketSideDataType type = src->side_data[i].type; > > int size = src->side_data[i].size; > > > > Afaik, the intended behavior of this function was to merge the side data > in dst with that of src, and this patch would break that. > It's admittedly not really defined and can get confusing, especially > when the old deprecated API (av_copy_packet, av_copy_packet_side_data, > av_dup_packet) do seem to just completely overwrite rather than merge. > > IMO, we should first define what should happen with side data in this > function before we make any further changes to it. If you ask me, merging the side data is under-defined at best. What happens if there are side data elements of the same type in src and dst? It looks like dst currently overwrites src. Does this even make sense? You could as well argue that src should be preserved (because it could mean that dst is supposed to provide fallbacks for missing info in src). So in my opinion, code which requires "merging" needs to be conscious about the required semantics and is best off doing it manually.
On 2/14/2018 2:25 AM, wm4 wrote: > On Wed, 14 Feb 2018 00:11:32 -0300 > James Almer <jamrial@gmail.com> wrote: > >>> --- >>> libavcodec/avpacket.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c >>> index 90b8215928..1a9be60e20 100644 >>> --- a/libavcodec/avpacket.c >>> +++ b/libavcodec/avpacket.c >>> @@ -571,6 +571,7 @@ FF_ENABLE_DEPRECATION_WARNINGS >>> dst->flags = src->flags; >>> dst->stream_index = src->stream_index; >>> >>> + dst->side_data_elems = 0; >>> for (i = 0; i < src->side_data_elems; i++) { >>> enum AVPacketSideDataType type = src->side_data[i].type; >>> int size = src->side_data[i].size; >>> >> >> Afaik, the intended behavior of this function was to merge the side data >> in dst with that of src, and this patch would break that. >> It's admittedly not really defined and can get confusing, especially >> when the old deprecated API (av_copy_packet, av_copy_packet_side_data, >> av_dup_packet) do seem to just completely overwrite rather than merge. >> >> IMO, we should first define what should happen with side data in this >> function before we make any further changes to it. > > If you ask me, merging the side data is under-defined at best. What > happens if there are side data elements of the same type in src and > dst? It looks like dst currently overwrites src. Does this even make > sense? You could as well argue that src should be preserved (because it > could mean that dst is supposed to provide fallbacks for missing info > in src). av_packet_add_side_data() used to add whatever new element you feed it at the end of the array without question. This meant that av_packet_get_side_data() would never actually get to them if another of the same types existed beforehand, as it returns the first element of the requested type it finds while looping through the array. I changed this in 28f60eeabb to instead replace the existing side data, so only the last one to be added is actually present in the packet. This is further enforced by making sure side_data_elems <= AV_PKT_DATA_NB when adding new elements. In the case of av_packet_copy_props(), the resulting merge prioritizes the elements from src over those in dst. Before, the elements from src would be added at the end of dst and potentially never be returned by av_packet_get_side_data(). So what do you suggest to do in av_packet_copy_props()? Call av_packet_free_side_data() on dst before copying elements from src so effectively generates an actual mirror of properties from src? Also, it would be great if someone could write a new generic and well defined side data API already. PS: frame side data still adds new elements at the end without question at the request of Ronald. > So in my opinion, code which requires "merging" needs to be > conscious about the required semantics and is best off doing it > manually. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Wed, 14 Feb 2018 13:14:19 -0300 James Almer <jamrial@gmail.com> wrote: > On 2/14/2018 2:25 AM, wm4 wrote: > > On Wed, 14 Feb 2018 00:11:32 -0300 > > James Almer <jamrial@gmail.com> wrote: > > > >>> --- > >>> libavcodec/avpacket.c | 1 + > >>> 1 file changed, 1 insertion(+) > >>> > >>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c > >>> index 90b8215928..1a9be60e20 100644 > >>> --- a/libavcodec/avpacket.c > >>> +++ b/libavcodec/avpacket.c > >>> @@ -571,6 +571,7 @@ FF_ENABLE_DEPRECATION_WARNINGS > >>> dst->flags = src->flags; > >>> dst->stream_index = src->stream_index; > >>> > >>> + dst->side_data_elems = 0; > >>> for (i = 0; i < src->side_data_elems; i++) { > >>> enum AVPacketSideDataType type = src->side_data[i].type; > >>> int size = src->side_data[i].size; > >>> > >> > >> Afaik, the intended behavior of this function was to merge the side data > >> in dst with that of src, and this patch would break that. > >> It's admittedly not really defined and can get confusing, especially > >> when the old deprecated API (av_copy_packet, av_copy_packet_side_data, > >> av_dup_packet) do seem to just completely overwrite rather than merge. > >> > >> IMO, we should first define what should happen with side data in this > >> function before we make any further changes to it. > > > > If you ask me, merging the side data is under-defined at best. What > > happens if there are side data elements of the same type in src and > > dst? It looks like dst currently overwrites src. Does this even make > > sense? You could as well argue that src should be preserved (because it > > could mean that dst is supposed to provide fallbacks for missing info > > in src). > > av_packet_add_side_data() used to add whatever new element you feed it > at the end of the array without question. This meant that > av_packet_get_side_data() would never actually get to them if another of > the same types existed beforehand, as it returns the first element of > the requested type it finds while looping through the array. > I changed this in 28f60eeabb to instead replace the existing side data, > so only the last one to be added is actually present in the packet. This > is further enforced by making sure side_data_elems <= AV_PKT_DATA_NB > when adding new elements. > In the case of av_packet_copy_props(), the resulting merge prioritizes > the elements from src over those in dst. Before, the elements from src > would be added at the end of dst and potentially never be returned by > av_packet_get_side_data(). Yeah, I switched src/dst at some point, resulting in confusing text. Anyway, you could argue it should work both ways, and considering the past confusion, I don't think it'd be a problem to always strictly overwrite dst side data like the patch suggests. It would have the advantage of having clearer semantics. (If side data gets "merged", you could still argue it should merge the contents in a clever way instead of just overwriting side data types that in both src and dst. Making a strict copy of the metadata would have more predictable semantics. > Also, it would be great if someone could write a new generic and well > defined side data API already. I thought about it, but I don't know how I'd make old and new API compatible. The API user is completely free to manipulate the current side data management structures manually, so old and new side data could get out of sync without a way to know which is the correct version. And obviously we can't just break the old API now. Maybe if the new side data API used the old fields to store the data, but that'd be a pain too. I'm open for suggestions. > PS: frame side data still adds new elements at the end without question > at the request of Ronald. That sounds weird. What was the reason?
On 2/14/2018 4:21 PM, wm4 wrote: > On Wed, 14 Feb 2018 13:14:19 -0300 > James Almer <jamrial@gmail.com> wrote: > >> On 2/14/2018 2:25 AM, wm4 wrote: >>> On Wed, 14 Feb 2018 00:11:32 -0300 >>> James Almer <jamrial@gmail.com> wrote: >>> >>>>> --- >>>>> libavcodec/avpacket.c | 1 + >>>>> 1 file changed, 1 insertion(+) >>>>> >>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c >>>>> index 90b8215928..1a9be60e20 100644 >>>>> --- a/libavcodec/avpacket.c >>>>> +++ b/libavcodec/avpacket.c >>>>> @@ -571,6 +571,7 @@ FF_ENABLE_DEPRECATION_WARNINGS >>>>> dst->flags = src->flags; >>>>> dst->stream_index = src->stream_index; >>>>> >>>>> + dst->side_data_elems = 0; >>>>> for (i = 0; i < src->side_data_elems; i++) { >>>>> enum AVPacketSideDataType type = src->side_data[i].type; >>>>> int size = src->side_data[i].size; >>>>> >>>> >>>> Afaik, the intended behavior of this function was to merge the side data >>>> in dst with that of src, and this patch would break that. >>>> It's admittedly not really defined and can get confusing, especially >>>> when the old deprecated API (av_copy_packet, av_copy_packet_side_data, >>>> av_dup_packet) do seem to just completely overwrite rather than merge. >>>> >>>> IMO, we should first define what should happen with side data in this >>>> function before we make any further changes to it. >>> >>> If you ask me, merging the side data is under-defined at best. What >>> happens if there are side data elements of the same type in src and >>> dst? It looks like dst currently overwrites src. Does this even make >>> sense? You could as well argue that src should be preserved (because it >>> could mean that dst is supposed to provide fallbacks for missing info >>> in src). >> >> av_packet_add_side_data() used to add whatever new element you feed it >> at the end of the array without question. This meant that >> av_packet_get_side_data() would never actually get to them if another of >> the same types existed beforehand, as it returns the first element of >> the requested type it finds while looping through the array. >> I changed this in 28f60eeabb to instead replace the existing side data, >> so only the last one to be added is actually present in the packet. This >> is further enforced by making sure side_data_elems <= AV_PKT_DATA_NB >> when adding new elements. >> In the case of av_packet_copy_props(), the resulting merge prioritizes >> the elements from src over those in dst. Before, the elements from src >> would be added at the end of dst and potentially never be returned by >> av_packet_get_side_data(). > > Yeah, I switched src/dst at some point, resulting in confusing text. > Anyway, you could argue it should work both ways, and considering the > past confusion, I don't think it'd be a problem to always strictly > overwrite dst side data like the patch suggests. It would have the > advantage of having clearer semantics. (If side data gets "merged", you > could still argue it should merge the contents in a clever way instead > of just overwriting side data types that in both src and dst. Making a > strict copy of the metadata would have more predictable semantics. > Ok, will apply a slightly modified version of Yusuke's patch then, by also setting dst->side_data to NULL to avoid issues in av_packet_add_side_data's av_realloc call if the field was uninitialized. Is that ok? >> Also, it would be great if someone could write a new generic and well >> defined side data API already. > > I thought about it, but I don't know how I'd make old and new API > compatible. The API user is completely free to manipulate the current > side data management structures manually, so old and new side data > could get out of sync without a way to know which is the correct > version. And obviously we can't just break the old API now. > > Maybe if the new side data API used the old fields to store the data, > but that'd be a pain too. > > I'm open for suggestions. > >> PS: frame side data still adds new elements at the end without question >> at the request of Ronald. > > That sounds weird. What was the reason? He said he personally saw the benefit of having more than one element of the same type in certain frames (don't remember what element type), and that he could manually fetch them where get_side_data() would not.
On Wed, 14 Feb 2018 18:57:37 -0300 James Almer <jamrial@gmail.com> wrote: > On 2/14/2018 4:21 PM, wm4 wrote: > > On Wed, 14 Feb 2018 13:14:19 -0300 > > James Almer <jamrial@gmail.com> wrote: > > > >> On 2/14/2018 2:25 AM, wm4 wrote: > >>> On Wed, 14 Feb 2018 00:11:32 -0300 > >>> James Almer <jamrial@gmail.com> wrote: > >>> > >>>>> --- > >>>>> libavcodec/avpacket.c | 1 + > >>>>> 1 file changed, 1 insertion(+) > >>>>> > >>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c > >>>>> index 90b8215928..1a9be60e20 100644 > >>>>> --- a/libavcodec/avpacket.c > >>>>> +++ b/libavcodec/avpacket.c > >>>>> @@ -571,6 +571,7 @@ FF_ENABLE_DEPRECATION_WARNINGS > >>>>> dst->flags = src->flags; > >>>>> dst->stream_index = src->stream_index; > >>>>> > >>>>> + dst->side_data_elems = 0; > >>>>> for (i = 0; i < src->side_data_elems; i++) { > >>>>> enum AVPacketSideDataType type = src->side_data[i].type; > >>>>> int size = src->side_data[i].size; > >>>>> > >>>> > >>>> Afaik, the intended behavior of this function was to merge the side data > >>>> in dst with that of src, and this patch would break that. > >>>> It's admittedly not really defined and can get confusing, especially > >>>> when the old deprecated API (av_copy_packet, av_copy_packet_side_data, > >>>> av_dup_packet) do seem to just completely overwrite rather than merge. > >>>> > >>>> IMO, we should first define what should happen with side data in this > >>>> function before we make any further changes to it. > >>> > >>> If you ask me, merging the side data is under-defined at best. What > >>> happens if there are side data elements of the same type in src and > >>> dst? It looks like dst currently overwrites src. Does this even make > >>> sense? You could as well argue that src should be preserved (because it > >>> could mean that dst is supposed to provide fallbacks for missing info > >>> in src). > >> > >> av_packet_add_side_data() used to add whatever new element you feed it > >> at the end of the array without question. This meant that > >> av_packet_get_side_data() would never actually get to them if another of > >> the same types existed beforehand, as it returns the first element of > >> the requested type it finds while looping through the array. > >> I changed this in 28f60eeabb to instead replace the existing side data, > >> so only the last one to be added is actually present in the packet. This > >> is further enforced by making sure side_data_elems <= AV_PKT_DATA_NB > >> when adding new elements. > >> In the case of av_packet_copy_props(), the resulting merge prioritizes > >> the elements from src over those in dst. Before, the elements from src > >> would be added at the end of dst and potentially never be returned by > >> av_packet_get_side_data(). > > > > Yeah, I switched src/dst at some point, resulting in confusing text. > > Anyway, you could argue it should work both ways, and considering the > > past confusion, I don't think it'd be a problem to always strictly > > overwrite dst side data like the patch suggests. It would have the > > advantage of having clearer semantics. (If side data gets "merged", you > > could still argue it should merge the contents in a clever way instead > > of just overwriting side data types that in both src and dst. Making a > > strict copy of the metadata would have more predictable semantics. > > > > Ok, will apply a slightly modified version of Yusuke's patch then, by > also setting dst->side_data to NULL to avoid issues in > av_packet_add_side_data's av_realloc call if the field was > uninitialized. Is that ok? What is our goal here: changing the semantics, or enabling this function to be called on uninitialized packets? If it's the latter, it should probably call av_init_packet() (and also set data/size to 0). To be honest I'm not sure if that change won't cause other problems, but all these packet functions are so messy, so it's hard to tell how it should work.
On 2/14/2018 7:54 PM, wm4 wrote: > On Wed, 14 Feb 2018 18:57:37 -0300 > James Almer <jamrial@gmail.com> wrote: > >> On 2/14/2018 4:21 PM, wm4 wrote: >>> On Wed, 14 Feb 2018 13:14:19 -0300 >>> James Almer <jamrial@gmail.com> wrote: >>> >>>> On 2/14/2018 2:25 AM, wm4 wrote: >>>>> On Wed, 14 Feb 2018 00:11:32 -0300 >>>>> James Almer <jamrial@gmail.com> wrote: >>>>> >>>>>>> --- >>>>>>> libavcodec/avpacket.c | 1 + >>>>>>> 1 file changed, 1 insertion(+) >>>>>>> >>>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c >>>>>>> index 90b8215928..1a9be60e20 100644 >>>>>>> --- a/libavcodec/avpacket.c >>>>>>> +++ b/libavcodec/avpacket.c >>>>>>> @@ -571,6 +571,7 @@ FF_ENABLE_DEPRECATION_WARNINGS >>>>>>> dst->flags = src->flags; >>>>>>> dst->stream_index = src->stream_index; >>>>>>> >>>>>>> + dst->side_data_elems = 0; >>>>>>> for (i = 0; i < src->side_data_elems; i++) { >>>>>>> enum AVPacketSideDataType type = src->side_data[i].type; >>>>>>> int size = src->side_data[i].size; >>>>>>> >>>>>> >>>>>> Afaik, the intended behavior of this function was to merge the side data >>>>>> in dst with that of src, and this patch would break that. >>>>>> It's admittedly not really defined and can get confusing, especially >>>>>> when the old deprecated API (av_copy_packet, av_copy_packet_side_data, >>>>>> av_dup_packet) do seem to just completely overwrite rather than merge. >>>>>> >>>>>> IMO, we should first define what should happen with side data in this >>>>>> function before we make any further changes to it. >>>>> >>>>> If you ask me, merging the side data is under-defined at best. What >>>>> happens if there are side data elements of the same type in src and >>>>> dst? It looks like dst currently overwrites src. Does this even make >>>>> sense? You could as well argue that src should be preserved (because it >>>>> could mean that dst is supposed to provide fallbacks for missing info >>>>> in src). >>>> >>>> av_packet_add_side_data() used to add whatever new element you feed it >>>> at the end of the array without question. This meant that >>>> av_packet_get_side_data() would never actually get to them if another of >>>> the same types existed beforehand, as it returns the first element of >>>> the requested type it finds while looping through the array. >>>> I changed this in 28f60eeabb to instead replace the existing side data, >>>> so only the last one to be added is actually present in the packet. This >>>> is further enforced by making sure side_data_elems <= AV_PKT_DATA_NB >>>> when adding new elements. >>>> In the case of av_packet_copy_props(), the resulting merge prioritizes >>>> the elements from src over those in dst. Before, the elements from src >>>> would be added at the end of dst and potentially never be returned by >>>> av_packet_get_side_data(). >>> >>> Yeah, I switched src/dst at some point, resulting in confusing text. >>> Anyway, you could argue it should work both ways, and considering the >>> past confusion, I don't think it'd be a problem to always strictly >>> overwrite dst side data like the patch suggests. It would have the >>> advantage of having clearer semantics. (If side data gets "merged", you >>> could still argue it should merge the contents in a clever way instead >>> of just overwriting side data types that in both src and dst. Making a >>> strict copy of the metadata would have more predictable semantics. >>> >> >> Ok, will apply a slightly modified version of Yusuke's patch then, by >> also setting dst->side_data to NULL to avoid issues in >> av_packet_add_side_data's av_realloc call if the field was >> uninitialized. Is that ok? > > What is our goal here: changing the semantics, or enabling this > function to be called on uninitialized packets? In a way, both. Make it actually copy side data rather than merging it, which by extension lets you use it on an uninitialized packet. > > If it's the latter, it should probably call av_init_packet() (and also > set data/size to 0). av_init_packet() sets buf to NULL, and av_packet_copy_props() is meant to copy properties and touch nothing else. It's stated in the doxy. > > To be honest I'm not sure if that change won't cause other problems, > but all these packet functions are so messy, so it's hard to tell how > it should work. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Wed, 14 Feb 2018 19:59:32 -0300 James Almer <jamrial@gmail.com> wrote: > On 2/14/2018 7:54 PM, wm4 wrote: > > On Wed, 14 Feb 2018 18:57:37 -0300 > > James Almer <jamrial@gmail.com> wrote: > > > >> On 2/14/2018 4:21 PM, wm4 wrote: > >>> On Wed, 14 Feb 2018 13:14:19 -0300 > >>> James Almer <jamrial@gmail.com> wrote: > >>> > >>>> On 2/14/2018 2:25 AM, wm4 wrote: > >>>>> On Wed, 14 Feb 2018 00:11:32 -0300 > >>>>> James Almer <jamrial@gmail.com> wrote: > >>>>> > >>>>>>> --- > >>>>>>> libavcodec/avpacket.c | 1 + > >>>>>>> 1 file changed, 1 insertion(+) > >>>>>>> > >>>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c > >>>>>>> index 90b8215928..1a9be60e20 100644 > >>>>>>> --- a/libavcodec/avpacket.c > >>>>>>> +++ b/libavcodec/avpacket.c > >>>>>>> @@ -571,6 +571,7 @@ FF_ENABLE_DEPRECATION_WARNINGS > >>>>>>> dst->flags = src->flags; > >>>>>>> dst->stream_index = src->stream_index; > >>>>>>> > >>>>>>> + dst->side_data_elems = 0; > >>>>>>> for (i = 0; i < src->side_data_elems; i++) { > >>>>>>> enum AVPacketSideDataType type = src->side_data[i].type; > >>>>>>> int size = src->side_data[i].size; > >>>>>>> > >>>>>> > >>>>>> Afaik, the intended behavior of this function was to merge the side data > >>>>>> in dst with that of src, and this patch would break that. > >>>>>> It's admittedly not really defined and can get confusing, especially > >>>>>> when the old deprecated API (av_copy_packet, av_copy_packet_side_data, > >>>>>> av_dup_packet) do seem to just completely overwrite rather than merge. > >>>>>> > >>>>>> IMO, we should first define what should happen with side data in this > >>>>>> function before we make any further changes to it. > >>>>> > >>>>> If you ask me, merging the side data is under-defined at best. What > >>>>> happens if there are side data elements of the same type in src and > >>>>> dst? It looks like dst currently overwrites src. Does this even make > >>>>> sense? You could as well argue that src should be preserved (because it > >>>>> could mean that dst is supposed to provide fallbacks for missing info > >>>>> in src). > >>>> > >>>> av_packet_add_side_data() used to add whatever new element you feed it > >>>> at the end of the array without question. This meant that > >>>> av_packet_get_side_data() would never actually get to them if another of > >>>> the same types existed beforehand, as it returns the first element of > >>>> the requested type it finds while looping through the array. > >>>> I changed this in 28f60eeabb to instead replace the existing side data, > >>>> so only the last one to be added is actually present in the packet. This > >>>> is further enforced by making sure side_data_elems <= AV_PKT_DATA_NB > >>>> when adding new elements. > >>>> In the case of av_packet_copy_props(), the resulting merge prioritizes > >>>> the elements from src over those in dst. Before, the elements from src > >>>> would be added at the end of dst and potentially never be returned by > >>>> av_packet_get_side_data(). > >>> > >>> Yeah, I switched src/dst at some point, resulting in confusing text. > >>> Anyway, you could argue it should work both ways, and considering the > >>> past confusion, I don't think it'd be a problem to always strictly > >>> overwrite dst side data like the patch suggests. It would have the > >>> advantage of having clearer semantics. (If side data gets "merged", you > >>> could still argue it should merge the contents in a clever way instead > >>> of just overwriting side data types that in both src and dst. Making a > >>> strict copy of the metadata would have more predictable semantics. > >>> > >> > >> Ok, will apply a slightly modified version of Yusuke's patch then, by > >> also setting dst->side_data to NULL to avoid issues in > >> av_packet_add_side_data's av_realloc call if the field was > >> uninitialized. Is that ok? > > > > What is our goal here: changing the semantics, or enabling this > > function to be called on uninitialized packets? > > In a way, both. Make it actually copy side data rather than merging it, > which by extension lets you use it on an uninitialized packet. > > > > > If it's the latter, it should probably call av_init_packet() (and also > > set data/size to 0). > > av_init_packet() sets buf to NULL, and av_packet_copy_props() is meant > to copy properties and touch nothing else. It's stated in the doxy. OK then. (What a mess.) Possibly it would still make sense to trivially factor the code to reset the fields into a separate function, just so it's not forgotten should new AVPacket fields be added (but feel free to ignore this).
On 2/14/2018 8:13 PM, wm4 wrote: > On Wed, 14 Feb 2018 19:59:32 -0300 > James Almer <jamrial@gmail.com> wrote: > >> On 2/14/2018 7:54 PM, wm4 wrote: >>> On Wed, 14 Feb 2018 18:57:37 -0300 >>> James Almer <jamrial@gmail.com> wrote: >>> >>>> On 2/14/2018 4:21 PM, wm4 wrote: >>>>> On Wed, 14 Feb 2018 13:14:19 -0300 >>>>> James Almer <jamrial@gmail.com> wrote: >>>>> >>>>>> On 2/14/2018 2:25 AM, wm4 wrote: >>>>>>> On Wed, 14 Feb 2018 00:11:32 -0300 >>>>>>> James Almer <jamrial@gmail.com> wrote: >>>>>>> >>>>>>>>> --- >>>>>>>>> libavcodec/avpacket.c | 1 + >>>>>>>>> 1 file changed, 1 insertion(+) >>>>>>>>> >>>>>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c >>>>>>>>> index 90b8215928..1a9be60e20 100644 >>>>>>>>> --- a/libavcodec/avpacket.c >>>>>>>>> +++ b/libavcodec/avpacket.c >>>>>>>>> @@ -571,6 +571,7 @@ FF_ENABLE_DEPRECATION_WARNINGS >>>>>>>>> dst->flags = src->flags; >>>>>>>>> dst->stream_index = src->stream_index; >>>>>>>>> >>>>>>>>> + dst->side_data_elems = 0; >>>>>>>>> for (i = 0; i < src->side_data_elems; i++) { >>>>>>>>> enum AVPacketSideDataType type = src->side_data[i].type; >>>>>>>>> int size = src->side_data[i].size; >>>>>>>>> >>>>>>>> >>>>>>>> Afaik, the intended behavior of this function was to merge the side data >>>>>>>> in dst with that of src, and this patch would break that. >>>>>>>> It's admittedly not really defined and can get confusing, especially >>>>>>>> when the old deprecated API (av_copy_packet, av_copy_packet_side_data, >>>>>>>> av_dup_packet) do seem to just completely overwrite rather than merge. >>>>>>>> >>>>>>>> IMO, we should first define what should happen with side data in this >>>>>>>> function before we make any further changes to it. >>>>>>> >>>>>>> If you ask me, merging the side data is under-defined at best. What >>>>>>> happens if there are side data elements of the same type in src and >>>>>>> dst? It looks like dst currently overwrites src. Does this even make >>>>>>> sense? You could as well argue that src should be preserved (because it >>>>>>> could mean that dst is supposed to provide fallbacks for missing info >>>>>>> in src). >>>>>> >>>>>> av_packet_add_side_data() used to add whatever new element you feed it >>>>>> at the end of the array without question. This meant that >>>>>> av_packet_get_side_data() would never actually get to them if another of >>>>>> the same types existed beforehand, as it returns the first element of >>>>>> the requested type it finds while looping through the array. >>>>>> I changed this in 28f60eeabb to instead replace the existing side data, >>>>>> so only the last one to be added is actually present in the packet. This >>>>>> is further enforced by making sure side_data_elems <= AV_PKT_DATA_NB >>>>>> when adding new elements. >>>>>> In the case of av_packet_copy_props(), the resulting merge prioritizes >>>>>> the elements from src over those in dst. Before, the elements from src >>>>>> would be added at the end of dst and potentially never be returned by >>>>>> av_packet_get_side_data(). >>>>> >>>>> Yeah, I switched src/dst at some point, resulting in confusing text. >>>>> Anyway, you could argue it should work both ways, and considering the >>>>> past confusion, I don't think it'd be a problem to always strictly >>>>> overwrite dst side data like the patch suggests. It would have the >>>>> advantage of having clearer semantics. (If side data gets "merged", you >>>>> could still argue it should merge the contents in a clever way instead >>>>> of just overwriting side data types that in both src and dst. Making a >>>>> strict copy of the metadata would have more predictable semantics. >>>>> >>>> >>>> Ok, will apply a slightly modified version of Yusuke's patch then, by >>>> also setting dst->side_data to NULL to avoid issues in >>>> av_packet_add_side_data's av_realloc call if the field was >>>> uninitialized. Is that ok? >>> >>> What is our goal here: changing the semantics, or enabling this >>> function to be called on uninitialized packets? >> >> In a way, both. Make it actually copy side data rather than merging it, >> which by extension lets you use it on an uninitialized packet. >> >>> >>> If it's the latter, it should probably call av_init_packet() (and also >>> set data/size to 0). >> >> av_init_packet() sets buf to NULL, and av_packet_copy_props() is meant >> to copy properties and touch nothing else. It's stated in the doxy. > > OK then. (What a mess.) Possibly it would still make sense to trivially > factor the code to reset the fields into a separate function, just so > it's not forgotten should new AVPacket fields be added (but feel free > to ignore this). That would result in a reset then copy in this function, so kinda redundant IMO. In any case, in the unlikely case a new field is added to AVPacket (its size is part of the ABI, so that can only happen after a major bump), i doubt anyone would forget to add it to both init() and copy_props(). Patch amended and pushed.
diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c index 90b8215928..1a9be60e20 100644 --- a/libavcodec/avpacket.c +++ b/libavcodec/avpacket.c @@ -571,6 +571,7 @@ FF_ENABLE_DEPRECATION_WARNINGS dst->flags = src->flags; dst->stream_index = src->stream_index; + dst->side_data_elems = 0; for (i = 0; i < src->side_data_elems; i++) { enum AVPacketSideDataType type = src->side_data[i].type; int size = src->side_data[i].size;