[FFmpeg-devel] avcodec/avpacket: add av_packet_remove_side_data()

This helper removes a side data entry from the packet, maintaining the
integrity and order of the remaining entries, if any.

Signed-off-by: James Almer <jamrial@gmail.com>
---
Missing APIChanges entry and version bump.

Couldn't find a place in the tree where it could be used right now, but
it makes the API be more in line with the AVFrame one, so i decided to
write it.

 libavcodec/avpacket.c | 24 ++++++++++++++++++++++++
 libavcodec/packet.h   |  8 ++++++++
 2 files changed, 32 insertions(+)

James Almer:
> This helper removes a side data entry from the packet, maintaining the
> integrity and order of the remaining entries, if any.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> Missing APIChanges entry and version bump.
> 
> Couldn't find a place in the tree where it could be used right now, but
> it makes the API be more in line with the AVFrame one, so i decided to
> write it.
> 
>  libavcodec/avpacket.c | 24 ++++++++++++++++++++++++
>  libavcodec/packet.h   |  8 ++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> index 4801163227..61ea81698c 100644
> --- a/libavcodec/avpacket.c
> +++ b/libavcodec/avpacket.c
> @@ -367,6 +367,30 @@ uint8_t *av_packet_get_side_data(const AVPacket *pkt, enum AVPacketSideDataType
>      return NULL;
>  }
>  
> +void av_packet_remove_side_data(AVPacket *pkt, enum AVPacketSideDataType type)
> +{
> +    int i, elems;
> +
> +    for (i = 0; i < pkt->side_data_elems; i++) {
> +        if (pkt->side_data[i].type != type)
> +            continue;
> +
> +        elems = pkt->side_data_elems - 1;
> +        av_freep(&pkt->side_data[i].data);
> +        pkt->side_data[i].size = 0;
> +
> +        if (i < elems) {
> +            memmove(&pkt->side_data[i], &pkt->side_data[i + 1],
> +                    (elems - i) * sizeof(*pkt->side_data));
> +        }
> +        if (!elems)
> +            av_freep(&pkt->side_data);
> +        pkt->side_data_elems = elems;
> +
> +        break;
> +    }
> +}
> +
>  const char *av_packet_side_data_name(enum AVPacketSideDataType type)
>  {
>      switch(type) {
> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
> index 0a19a0eff3..6ce3c91c07 100644
> --- a/libavcodec/packet.h
> +++ b/libavcodec/packet.h
> @@ -595,6 +595,14 @@ int av_packet_shrink_side_data(AVPacket *pkt, enum AVPacketSideDataType type,
>  uint8_t* av_packet_get_side_data(const AVPacket *pkt, enum AVPacketSideDataType type,
>                                   int *size);
>  
> +/**
> + * Remove and free a side data entry of the given type.
> + *
> + * @param pkt packet
> + * @param type side data type to be removed
> + */
> +void av_packet_remove_side_data(AVPacket *pkt, enum AVPacketSideDataType type);

How about extending the functionality to optionally pass the side data
to the caller instead of freeing it? All it needs are two pointer
parameters for data and size. Then this function could be used to
simplify
https://ffmpeg.org/pipermail/ffmpeg-devel/2020-September/269319.html

- Andreas

On 9/8/2020 7:42 PM, Andreas Rheinhardt wrote:
> James Almer:
>> This helper removes a side data entry from the packet, maintaining the
>> integrity and order of the remaining entries, if any.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> Missing APIChanges entry and version bump.
>>
>> Couldn't find a place in the tree where it could be used right now, but
>> it makes the API be more in line with the AVFrame one, so i decided to
>> write it.
>>
>>  libavcodec/avpacket.c | 24 ++++++++++++++++++++++++
>>  libavcodec/packet.h   |  8 ++++++++
>>  2 files changed, 32 insertions(+)
>>
>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>> index 4801163227..61ea81698c 100644
>> --- a/libavcodec/avpacket.c
>> +++ b/libavcodec/avpacket.c
>> @@ -367,6 +367,30 @@ uint8_t *av_packet_get_side_data(const AVPacket *pkt, enum AVPacketSideDataType
>>      return NULL;
>>  }
>>  
>> +void av_packet_remove_side_data(AVPacket *pkt, enum AVPacketSideDataType type)
>> +{
>> +    int i, elems;
>> +
>> +    for (i = 0; i < pkt->side_data_elems; i++) {
>> +        if (pkt->side_data[i].type != type)
>> +            continue;
>> +
>> +        elems = pkt->side_data_elems - 1;
>> +        av_freep(&pkt->side_data[i].data);
>> +        pkt->side_data[i].size = 0;
>> +
>> +        if (i < elems) {
>> +            memmove(&pkt->side_data[i], &pkt->side_data[i + 1],
>> +                    (elems - i) * sizeof(*pkt->side_data));
>> +        }
>> +        if (!elems)
>> +            av_freep(&pkt->side_data);
>> +        pkt->side_data_elems = elems;
>> +
>> +        break;
>> +    }
>> +}
>> +
>>  const char *av_packet_side_data_name(enum AVPacketSideDataType type)
>>  {
>>      switch(type) {
>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
>> index 0a19a0eff3..6ce3c91c07 100644
>> --- a/libavcodec/packet.h
>> +++ b/libavcodec/packet.h
>> @@ -595,6 +595,14 @@ int av_packet_shrink_side_data(AVPacket *pkt, enum AVPacketSideDataType type,
>>  uint8_t* av_packet_get_side_data(const AVPacket *pkt, enum AVPacketSideDataType type,
>>                                   int *size);
>>  
>> +/**
>> + * Remove and free a side data entry of the given type.
>> + *
>> + * @param pkt packet
>> + * @param type side data type to be removed
>> + */
>> +void av_packet_remove_side_data(AVPacket *pkt, enum AVPacketSideDataType type);
> 
> How about extending the functionality to optionally pass the side data
> to the caller instead of freeing it? All it needs are two pointer
> parameters for data and size. Then this function could be used to
> simplify
> https://ffmpeg.org/pipermail/ffmpeg-devel/2020-September/269319.html

I thought about it, but doing so would be tailoring the function for
this specific scenario, turning it into a combination of get and remove
rather than a simple and API consistent (with AVFrame) fire-and-forget
call to remove a side data type if present in the packet.

That one case above is also discarding the packet altogether immediately
after stealing its side data, so it will not even gain anything from the
extra measures to rearrange the resulting array post removal.

> 
> - Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>

On Wed, Sep 9, 2020 at 6:59 AM James Almer <jamrial@gmail.com> wrote:

> On 9/8/2020 7:42 PM, Andreas Rheinhardt wrote:
> > James Almer:
> >> This helper removes a side data entry from the packet, maintaining the
> >> integrity and order of the remaining entries, if any.
> >>
> >> Signed-off-by: James Almer <jamrial@gmail.com>
> >> ---
> >> Missing APIChanges entry and version bump.
> >>
> >> Couldn't find a place in the tree where it could be used right now, but
> >> it makes the API be more in line with the AVFrame one, so i decided to
> >> write it.
> >>
> >>  libavcodec/avpacket.c | 24 ++++++++++++++++++++++++
> >>  libavcodec/packet.h   |  8 ++++++++
> >>  2 files changed, 32 insertions(+)
> >>
> >> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> >> index 4801163227..61ea81698c 100644
> >> --- a/libavcodec/avpacket.c
> >> +++ b/libavcodec/avpacket.c
> >> @@ -367,6 +367,30 @@ uint8_t *av_packet_get_side_data(const AVPacket
> *pkt, enum AVPacketSideDataType
> >>      return NULL;
> >>  }
> >>
> >> +void av_packet_remove_side_data(AVPacket *pkt, enum
> AVPacketSideDataType type)
> >> +{
> >> +    int i, elems;
> >> +
> >> +    for (i = 0; i < pkt->side_data_elems; i++) {
> >> +        if (pkt->side_data[i].type != type)
> >> +            continue;
> >> +
> >> +        elems = pkt->side_data_elems - 1;
> >> +        av_freep(&pkt->side_data[i].data);
> >> +        pkt->side_data[i].size = 0;
> >> +
> >> +        if (i < elems) {
> >> +            memmove(&pkt->side_data[i], &pkt->side_data[i + 1],
> >> +                    (elems - i) * sizeof(*pkt->side_data));
> >> +        }
> >> +        if (!elems)
> >> +            av_freep(&pkt->side_data);
> >> +        pkt->side_data_elems = elems;
> >> +
> >> +        break;
> >> +    }
> >> +}
> >> +
> >>  const char *av_packet_side_data_name(enum AVPacketSideDataType type)
> >>  {
> >>      switch(type) {
> >> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
> >> index 0a19a0eff3..6ce3c91c07 100644
> >> --- a/libavcodec/packet.h
> >> +++ b/libavcodec/packet.h
> >> @@ -595,6 +595,14 @@ int av_packet_shrink_side_data(AVPacket *pkt, enum
> AVPacketSideDataType type,
> >>  uint8_t* av_packet_get_side_data(const AVPacket *pkt, enum
> AVPacketSideDataType type,
> >>                                   int *size);
> >>
> >> +/**
> >> + * Remove and free a side data entry of the given type.
> >> + *
> >> + * @param pkt packet
> >> + * @param type side data type to be removed
> >> + */
> >> +void av_packet_remove_side_data(AVPacket *pkt, enum
> AVPacketSideDataType type);
> >
> > How about extending the functionality to optionally pass the side data
> > to the caller instead of freeing it? All it needs are two pointer
> > parameters for data and size. Then this function could be used to
> > simplify
> > https://ffmpeg.org/pipermail/ffmpeg-devel/2020-September/269319.html
>
> I thought about it, but doing so would be tailoring the function for
> this specific scenario, turning it into a combination of get and remove
> rather than a simple and API consistent (with AVFrame) fire-and-forget
> call to remove a side data type if present in the packet.
>
> That one case above is also discarding the packet altogether immediately
> after stealing its side data, so it will not even gain anything from the
> extra measures to rearrange the resulting array post removal.
>

Since we have av_packet_add_side_data, it will transfer side data ownership
from the user to AVPacket.
Is it possible to add  av_packet_steal_side_data to transfer the side data
ownership from AVPacket to the user?
Then  av_packet_remove_side_data can call  av_packet_steal_side_data and
free the stealed data.


> >
> > - Andreas
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> >
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

On Wed, Sep 9, 2020 at 6:36 AM James Almer <jamrial@gmail.com> wrote:

> This helper removes a side data entry from the packet, maintaining the
> integrity and order of the remaining entries, if any.
>
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> Missing APIChanges entry and version bump.
>
> Couldn't find a place in the tree where it could be used right now, but
> it makes the API be more in line with the AVFrame one, so i decided to
> write it.
>
>  libavcodec/avpacket.c | 24 ++++++++++++++++++++++++
>  libavcodec/packet.h   |  8 ++++++++
>  2 files changed, 32 insertions(+)
>
> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> index 4801163227..61ea81698c 100644
> --- a/libavcodec/avpacket.c
> +++ b/libavcodec/avpacket.c
> @@ -367,6 +367,30 @@ uint8_t *av_packet_get_side_data(const AVPacket *pkt,
> enum AVPacketSideDataType
>      return NULL;
>  }
>
> +void av_packet_remove_side_data(AVPacket *pkt, enum AVPacketSideDataType
> type)
> +{
> +    int i, elems;
> +
> +    for (i = 0; i < pkt->side_data_elems; i++) {
> +        if (pkt->side_data[i].type != type)
> +            continue;
> +
> +        elems = pkt->side_data_elems - 1;
> +        av_freep(&pkt->side_data[i].data);
> +        pkt->side_data[i].size = 0;
>

Nit: This not needed since we will overwrite pkg->side_data[i] or shrink
the  side_data_elems later.


> +
> +        if (i < elems) {
> +            memmove(&pkt->side_data[i], &pkt->side_data[i + 1],
> +                    (elems - i) * sizeof(*pkt->side_data));
> +        }
> +        if (!elems)
> +            av_freep(&pkt->side_data);
> +        pkt->side_data_elems = elems;
> +
> +        break;
> +    }
> +}
> +
>  const char *av_packet_side_data_name(enum AVPacketSideDataType type)
>  {
>      switch(type) {
> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
> index 0a19a0eff3..6ce3c91c07 100644
> --- a/libavcodec/packet.h
> +++ b/libavcodec/packet.h
> @@ -595,6 +595,14 @@ int av_packet_shrink_side_data(AVPacket *pkt, enum
> AVPacketSideDataType type,
>  uint8_t* av_packet_get_side_data(const AVPacket *pkt, enum
> AVPacketSideDataType type,
>                                   int *size);
>
> +/**
> + * Remove and free a side data entry of the given type.
> + *
> + * @param pkt packet
> + * @param type side data type to be removed
> + */
> +void av_packet_remove_side_data(AVPacket *pkt, enum AVPacketSideDataType
> type);
> +
>  #if FF_API_MERGE_SD_API
>  attribute_deprecated
>  int av_packet_merge_side_data(AVPacket *pkt);
> --
> 2.27.0
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

On Tue, 8 Sep 2020, James Almer wrote:

> This helper removes a side data entry from the packet, maintaining the
> integrity and order of the remaining entries, if any.
>
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> Missing APIChanges entry and version bump.
>
> Couldn't find a place in the tree where it could be used right now, but
> it makes the API be more in line with the AVFrame one, so i decided to
> write it.
>
> libavcodec/avpacket.c | 24 ++++++++++++++++++++++++
> libavcodec/packet.h   |  8 ++++++++
> 2 files changed, 32 insertions(+)
>
> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> index 4801163227..61ea81698c 100644
> --- a/libavcodec/avpacket.c
> +++ b/libavcodec/avpacket.c
> @@ -367,6 +367,30 @@ uint8_t *av_packet_get_side_data(const AVPacket *pkt, enum AVPacketSideDataType
>     return NULL;
> }
> 
> +void av_packet_remove_side_data(AVPacket *pkt, enum AVPacketSideDataType type)
> +{
> +    int i, elems;
> +
> +    for (i = 0; i < pkt->side_data_elems; i++) {
> +        if (pkt->side_data[i].type != type)
> +            continue;
> +
> +        elems = pkt->side_data_elems - 1;
> +        av_freep(&pkt->side_data[i].data);
> +        pkt->side_data[i].size = 0;
> +
> +        if (i < elems) {
> +            memmove(&pkt->side_data[i], &pkt->side_data[i + 1],
> +                    (elems - i) * sizeof(*pkt->side_data));
> +        }
> +        if (!elems)
> +            av_freep(&pkt->side_data);
> +        pkt->side_data_elems = elems;
> +
> +        break;
> +    }
> +}

Why not use the same algorightm that is used in av_frame_remove_side_data?
Shorter and does not need memmove...

Thanks,
Marton

> +
> const char *av_packet_side_data_name(enum AVPacketSideDataType type)
> {
>     switch(type) {
> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
> index 0a19a0eff3..6ce3c91c07 100644
> --- a/libavcodec/packet.h
> +++ b/libavcodec/packet.h
> @@ -595,6 +595,14 @@ int av_packet_shrink_side_data(AVPacket *pkt, enum AVPacketSideDataType type,
> uint8_t* av_packet_get_side_data(const AVPacket *pkt, enum AVPacketSideDataType type,
>                                  int *size);
> 
> +/**
> + * Remove and free a side data entry of the given type.
> + *
> + * @param pkt packet
> + * @param type side data type to be removed
> + */
> +void av_packet_remove_side_data(AVPacket *pkt, enum AVPacketSideDataType type);
> +
> #if FF_API_MERGE_SD_API
> attribute_deprecated
> int av_packet_merge_side_data(AVPacket *pkt);
> -- 
> 2.27.0
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

On 9/9/2020 12:47 PM, Marton Balint wrote:
> 
> 
> On Tue, 8 Sep 2020, James Almer wrote:
> 
>> This helper removes a side data entry from the packet, maintaining the
>> integrity and order of the remaining entries, if any.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> Missing APIChanges entry and version bump.
>>
>> Couldn't find a place in the tree where it could be used right now, but
>> it makes the API be more in line with the AVFrame one, so i decided to
>> write it.
>>
>> libavcodec/avpacket.c | 24 ++++++++++++++++++++++++
>> libavcodec/packet.h   |  8 ++++++++
>> 2 files changed, 32 insertions(+)
>>
>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>> index 4801163227..61ea81698c 100644
>> --- a/libavcodec/avpacket.c
>> +++ b/libavcodec/avpacket.c
>> @@ -367,6 +367,30 @@ uint8_t *av_packet_get_side_data(const AVPacket
>> *pkt, enum AVPacketSideDataType
>>     return NULL;
>> }
>>
>> +void av_packet_remove_side_data(AVPacket *pkt, enum
>> AVPacketSideDataType type)
>> +{
>> +    int i, elems;
>> +
>> +    for (i = 0; i < pkt->side_data_elems; i++) {
>> +        if (pkt->side_data[i].type != type)
>> +            continue;
>> +
>> +        elems = pkt->side_data_elems - 1;
>> +        av_freep(&pkt->side_data[i].data);
>> +        pkt->side_data[i].size = 0;
>> +
>> +        if (i < elems) {
>> +            memmove(&pkt->side_data[i], &pkt->side_data[i + 1],
>> +                    (elems - i) * sizeof(*pkt->side_data));
>> +        }
>> +        if (!elems)
>> +            av_freep(&pkt->side_data);
>> +        pkt->side_data_elems = elems;
>> +
>> +        break;
>> +    }
>> +}
> 
> Why not use the same algorightm that is used in av_frame_remove_side_data?
> Shorter and does not need memmove...

Frame side data and packet side data are different.

AVFrameSideData **side_data;

vs

AVPacketSideData *side_data;

You can also see the difference in the functions adding side data.

On Wed, 9 Sep 2020, James Almer wrote:

> On 9/9/2020 12:47 PM, Marton Balint wrote:
>> 
>> 
>> On Tue, 8 Sep 2020, James Almer wrote:
>> 
>>> This helper removes a side data entry from the packet, maintaining the
>>> integrity and order of the remaining entries, if any.
>>>
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>> Missing APIChanges entry and version bump.
>>>
>>> Couldn't find a place in the tree where it could be used right now, but
>>> it makes the API be more in line with the AVFrame one, so i decided to
>>> write it.
>>>
>>> libavcodec/avpacket.c | 24 ++++++++++++++++++++++++
>>> libavcodec/packet.h   |  8 ++++++++
>>> 2 files changed, 32 insertions(+)
>>>
>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>>> index 4801163227..61ea81698c 100644
>>> --- a/libavcodec/avpacket.c
>>> +++ b/libavcodec/avpacket.c
>>> @@ -367,6 +367,30 @@ uint8_t *av_packet_get_side_data(const AVPacket
>>> *pkt, enum AVPacketSideDataType
>>>     return NULL;
>>> }
>>>
>>> +void av_packet_remove_side_data(AVPacket *pkt, enum
>>> AVPacketSideDataType type)
>>> +{
>>> +    int i, elems;
>>> +
>>> +    for (i = 0; i < pkt->side_data_elems; i++) {
>>> +        if (pkt->side_data[i].type != type)
>>> +            continue;
>>> +
>>> +        elems = pkt->side_data_elems - 1;
>>> +        av_freep(&pkt->side_data[i].data);
>>> +        pkt->side_data[i].size = 0;
>>> +
>>> +        if (i < elems) {
>>> +            memmove(&pkt->side_data[i], &pkt->side_data[i + 1],
>>> +                    (elems - i) * sizeof(*pkt->side_data));
>>> +        }
>>> +        if (!elems)
>>> +            av_freep(&pkt->side_data);
>>> +        pkt->side_data_elems = elems;
>>> +
>>> +        break;
>>> +    }
>>> +}
>> 
>> Why not use the same algorightm that is used in av_frame_remove_side_data?
>> Shorter and does not need memmove...
>
> Frame side data and packet side data are different.
>
> AVFrameSideData **side_data;
>
> vs
>
> AVPacketSideData *side_data;

Ok, but the algorithm still works, only it operates on structs and not 
pointers, right?

static void av_packet_remove_side_data(AVPacket *pkt, enum AVPacketSideDataType type)
{
     for (int i = pkt->side_data_elems - 1; i >= 0; i--) {
         AVPacketSideData *sd = &pkt->side_data[i];
         if (sd->type == type) {
             av_freep(&sd->data);
             pkt->side_data[i] = pkt->side_data[pkt->side_data_elems - 1];
             pkt->side_data_elems--;
         }
     }
}

Regards,
Marton