[FFmpeg-devel] avpacket: Set dst->side_data_elems to 0 within av_packet_copy_props.

Submitted by Yusuke Nakamura on Feb. 14, 2018, 2:43 a.m.

Details

Message ID 20180214024324.10260-1-muken.the.vfrmaniac@gmail.com
State New
Headers show

Commit Message

Yusuke Nakamura Feb. 14, 2018, 2:43 a.m.
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);
---
 libavcodec/avpacket.c | 1 +
 1 file changed, 1 insertion(+)

Comments

James Almer Feb. 14, 2018, 3:11 a.m.
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.
Yusuke Nakamura Feb. 14, 2018, 3:22 a.m.
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
>
James Almer Feb. 14, 2018, 3:44 a.m.
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
>
wm4 Feb. 14, 2018, 5:25 a.m.
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.
James Almer Feb. 14, 2018, 4:14 p.m.
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
>
wm4 Feb. 14, 2018, 7:21 p.m.
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?
James Almer Feb. 14, 2018, 9:57 p.m.
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.
wm4 Feb. 14, 2018, 10:54 p.m.
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.
James Almer Feb. 14, 2018, 10:59 p.m.
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
>
wm4 Feb. 14, 2018, 11:13 p.m.
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).
James Almer Feb. 15, 2018, 12:06 a.m.
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.

Patch hide | download patch | download mbox

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;