[FFmpeg-devel] libavcodec/avpacket: add av_packet_remove_side_data

---
 doc/APIchanges              |  3 +++
 libavcodec/avpacket.c       | 15 +++++++++++++++
 libavcodec/packet.h         |  5 +++++
 libavcodec/tests/avpacket.c |  9 +++++++++
 libavcodec/version.h        |  2 +-
 5 files changed, 33 insertions(+), 1 deletion(-)

On Mon, 11 Oct 2021, Zhao Zhili wrote:

> ---
> doc/APIchanges              |  3 +++
> libavcodec/avpacket.c       | 15 +++++++++++++++
> libavcodec/packet.h         |  5 +++++
> libavcodec/tests/avpacket.c |  9 +++++++++
> libavcodec/version.h        |  2 +-
> 5 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 7b267a79ac..2c6b369ea9 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -14,6 +14,9 @@ libavutil:     2021-04-27
>
> API changes, most recent first:
>
> +2021-10-11 - xxxxxxxxxx - lavc 59.13.100 - packet.h
> +  Add av_packet_remove_side_data()
> +
> 2021-09-21 - xxxxxxxxxx - lavu 57.7.100 - pixfmt.h
>   Add AV_PIX_FMT_X2BGR10.
>
> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> index d8d8fef3b9..2a9123e5fa 100644
> --- a/libavcodec/avpacket.c
> +++ b/libavcodec/avpacket.c
> @@ -179,6 +179,21 @@ int av_packet_from_data(AVPacket *pkt, uint8_t *data, int size)
>     return 0;
> }
>
> +void av_packet_remove_side_data(AVPacket *pkt, enum AVPacketSideDataType type)
> +{
> +    for (int i = 0; i < pkt->side_data_elems; i++) {
> +        if (pkt->side_data[i].type == type) {
> +            av_freep(&pkt->side_data[i].data);
> +            pkt->side_data[i] = pkt->side_data[pkt->side_data_elems - 1];
> +            pkt->side_data_elems--;
> +            /* Better keep side_data sync to side_data_elems */
> +            if (!pkt->side_data_elems)
> +                av_freep(&pkt->side_data);
> +            break;
> +        }
> +    }
> +}

I suggest you use the same algorigthm which is used for avframe side data 
removal, because as far as I know it is still not explicitly documented 
that multiple types of the same side data is not allowed... So IMHO it is 
better if this works in all cases.

Thanks,
Marton

> +
> void av_packet_free_side_data(AVPacket *pkt)
> {
>     int i;
> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
> index 9baff24635..85edf87211 100644
> --- a/libavcodec/packet.h
> +++ b/libavcodec/packet.h
> @@ -571,6 +571,11 @@ uint8_t* av_packet_new_side_data(AVPacket *pkt, enum AVPacketSideDataType type,
> int av_packet_add_side_data(AVPacket *pkt, enum AVPacketSideDataType type,
>                             uint8_t *data, size_t size);
>
> +/**
> + * Remove and free side data instances of the given type.
> + */
> +void av_packet_remove_side_data(AVPacket *pkt, enum AVPacketSideDataType type);
> +
> /**
>  * Shrink the already allocated side data buffer
>  *
> diff --git a/libavcodec/tests/avpacket.c b/libavcodec/tests/avpacket.c
> index 7a70ade4c3..710a964915 100644
> --- a/libavcodec/tests/avpacket.c
> +++ b/libavcodec/tests/avpacket.c
> @@ -124,6 +124,15 @@ int main(void)
>                 "when \"size\" parameter is too large.\n" );
>         ret = 1;
>     }
> +    /* test remove side data */
> +    av_packet_remove_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA);
> +    for (int i = 0; i < avpkt->side_data_elems; i++) {
> +        if (avpkt->side_data[i].type == AV_PKT_DATA_NEW_EXTRADATA) {
> +            printf("av_packet_remove_side_data failed to remove side data");
> +            ret = 1;
> +        }
> +    }
> +
>     /*clean up*/
>     av_packet_free(&avpkt_clone);
>     av_packet_free(&avpkt);
> diff --git a/libavcodec/version.h b/libavcodec/version.h
> index 74b8baa5f3..76af066d32 100644
> --- a/libavcodec/version.h
> +++ b/libavcodec/version.h
> @@ -28,7 +28,7 @@
> #include "libavutil/version.h"
>
> #define LIBAVCODEC_VERSION_MAJOR  59
> -#define LIBAVCODEC_VERSION_MINOR  12
> +#define LIBAVCODEC_VERSION_MINOR  13
> #define LIBAVCODEC_VERSION_MICRO 100
>
> #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
> -- 
> 2.31.1
>
> _______________________________________________
> 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 Oct 12, 2021, at 2:20 AM, Marton Balint <cus@passwd.hu> wrote:
> 
> On Mon, 11 Oct 2021, Zhao Zhili wrote:
> 
>> ---
>> doc/APIchanges              |  3 +++
>> libavcodec/avpacket.c       | 15 +++++++++++++++
>> libavcodec/packet.h         |  5 +++++
>> libavcodec/tests/avpacket.c |  9 +++++++++
>> libavcodec/version.h        |  2 +-
>> 5 files changed, 33 insertions(+), 1 deletion(-)
>> 
>> diff --git a/doc/APIchanges b/doc/APIchanges
>> index 7b267a79ac..2c6b369ea9 100644
>> --- a/doc/APIchanges
>> +++ b/doc/APIchanges
>> @@ -14,6 +14,9 @@ libavutil:     2021-04-27
>> 
>> API changes, most recent first:
>> 
>> +2021-10-11 - xxxxxxxxxx - lavc 59.13.100 - packet.h
>> +  Add av_packet_remove_side_data()
>> +
>> 2021-09-21 - xxxxxxxxxx - lavu 57.7.100 - pixfmt.h
>>  Add AV_PIX_FMT_X2BGR10.
>> 
>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>> index d8d8fef3b9..2a9123e5fa 100644
>> --- a/libavcodec/avpacket.c
>> +++ b/libavcodec/avpacket.c
>> @@ -179,6 +179,21 @@ int av_packet_from_data(AVPacket *pkt, uint8_t *data, int size)
>>    return 0;
>> }
>> 
>> +void av_packet_remove_side_data(AVPacket *pkt, enum AVPacketSideDataType type)
>> +{
>> +    for (int i = 0; i < pkt->side_data_elems; i++) {
>> +        if (pkt->side_data[i].type == type) {
>> +            av_freep(&pkt->side_data[i].data);
>> +            pkt->side_data[i] = pkt->side_data[pkt->side_data_elems - 1];
>> +            pkt->side_data_elems--;
>> +            /* Better keep side_data sync to side_data_elems */
>> +            if (!pkt->side_data_elems)
>> +                av_freep(&pkt->side_data);
>> +            break;
>> +        }
>> +    }
>> +}
> 
> I suggest you use the same algorigthm which is used for avframe side data removal, because as far as I know it is still not explicitly documented that multiple types of the same side data is not allowed... So IMHO it is better if this works in all cases.
> 

I have noticed the difference, av_packet_add_side_data() doesn’t allow
multiple instance side data of the same type, while
av_frame_new_side_data_from_buf() allows that for history reasons
and too late to be fixed.

As you said in https://patchwork.ffmpeg.org/project/ffmpeg/patch/20191101120314.88956-1-quinkblack@foxmail.com/

> all our API around side data (get/remove) is based on the 
> assumption that a single entry of a type exists. Also for packet/stream 
> side data it is already assumed as far as I see.

I think the behavior of avpacket is unlikely to change. So for the sake
of consistent to av_packet_add_side_data() and
av_packet_shrink_side_data(), and a little performance reason, I
prefer return early. But I’m fine to continue the loop if you thought
safety and forward compatibility is more important.

> Thanks,
> Marton
> 
>> +
>> void av_packet_free_side_data(AVPacket *pkt)
>> {
>>    int i;
>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
>> index 9baff24635..85edf87211 100644
>> --- a/libavcodec/packet.h
>> +++ b/libavcodec/packet.h
>> @@ -571,6 +571,11 @@ uint8_t* av_packet_new_side_data(AVPacket *pkt, enum AVPacketSideDataType type,
>> int av_packet_add_side_data(AVPacket *pkt, enum AVPacketSideDataType type,
>>                            uint8_t *data, size_t size);
>> 
>> +/**
>> + * Remove and free side data instances of the given type.
>> + */
>> +void av_packet_remove_side_data(AVPacket *pkt, enum AVPacketSideDataType type);
>> +
>> /**
>> * Shrink the already allocated side data buffer
>> *
>> diff --git a/libavcodec/tests/avpacket.c b/libavcodec/tests/avpacket.c
>> index 7a70ade4c3..710a964915 100644
>> --- a/libavcodec/tests/avpacket.c
>> +++ b/libavcodec/tests/avpacket.c
>> @@ -124,6 +124,15 @@ int main(void)
>>                "when \"size\" parameter is too large.\n" );
>>        ret = 1;
>>    }
>> +    /* test remove side data */
>> +    av_packet_remove_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA);
>> +    for (int i = 0; i < avpkt->side_data_elems; i++) {
>> +        if (avpkt->side_data[i].type == AV_PKT_DATA_NEW_EXTRADATA) {
>> +            printf("av_packet_remove_side_data failed to remove side data");
>> +            ret = 1;
>> +        }
>> +    }
>> +
>>    /*clean up*/
>>    av_packet_free(&avpkt_clone);
>>    av_packet_free(&avpkt);
>> diff --git a/libavcodec/version.h b/libavcodec/version.h
>> index 74b8baa5f3..76af066d32 100644
>> --- a/libavcodec/version.h
>> +++ b/libavcodec/version.h
>> @@ -28,7 +28,7 @@
>> #include "libavutil/version.h"
>> 
>> #define LIBAVCODEC_VERSION_MAJOR  59
>> -#define LIBAVCODEC_VERSION_MINOR  12
>> +#define LIBAVCODEC_VERSION_MINOR  13
>> #define LIBAVCODEC_VERSION_MICRO 100
>> 
>> #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
>> -- 
>> 2.31.1
>> 
>> _______________________________________________
>> 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 Mon, 11 Oct 2021, Zhao Zhili wrote:
> 
>> ---
>> doc/APIchanges              |  3 +++
>> libavcodec/avpacket.c       | 15 +++++++++++++++
>> libavcodec/packet.h         |  5 +++++
>> libavcodec/tests/avpacket.c |  9 +++++++++
>> libavcodec/version.h        |  2 +-
>> 5 files changed, 33 insertions(+), 1 deletion(-)
>> 
>> diff --git a/doc/APIchanges b/doc/APIchanges
>> index 7b267a79ac..2c6b369ea9 100644
>> --- a/doc/APIchanges
>> +++ b/doc/APIchanges
>> @@ -14,6 +14,9 @@ libavutil:     2021-04-27
>> 
>> API changes, most recent first:
>> 
>> +2021-10-11 - xxxxxxxxxx - lavc 59.13.100 - packet.h
>> +  Add av_packet_remove_side_data()
>> +
>> 2021-09-21 - xxxxxxxxxx - lavu 57.7.100 - pixfmt.h
>>  Add AV_PIX_FMT_X2BGR10.
>> 
>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>> index d8d8fef3b9..2a9123e5fa 100644
>> --- a/libavcodec/avpacket.c
>> +++ b/libavcodec/avpacket.c
>> @@ -179,6 +179,21 @@ int av_packet_from_data(AVPacket *pkt, uint8_t *data, int size)
>>    return 0;
>> }
>> 
>> +void av_packet_remove_side_data(AVPacket *pkt, enum AVPacketSideDataType type)
>> +{
>> +    for (int i = 0; i < pkt->side_data_elems; i++) {
>> +        if (pkt->side_data[i].type == type) {
>> +            av_freep(&pkt->side_data[i].data);
>> +            pkt->side_data[i] = pkt->side_data[pkt->side_data_elems - 1];
>> +            pkt->side_data_elems--;
>> +            /* Better keep side_data sync to side_data_elems */
>> +            if (!pkt->side_data_elems)
>> +                av_freep(&pkt->side_data);
>> +            break;
>> +        }
>> +    }
>> +}
> 
> I suggest you use the same algorigthm which is used for avframe side data removal, because as far as I know it is still not explicitly documented that multiple types of the same side data is not allowed... So IMHO it is better if this works in all cases.
> 
> Thanks,
> Marton
> 
>> +
>> void av_packet_free_side_data(AVPacket *pkt)
>> {
>>    int i;
>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
>> index 9baff24635..85edf87211 100644
>> --- a/libavcodec/packet.h
>> +++ b/libavcodec/packet.h
>> @@ -571,6 +571,11 @@ uint8_t* av_packet_new_side_data(AVPacket *pkt, enum AVPacketSideDataType type,
>> int av_packet_add_side_data(AVPacket *pkt, enum AVPacketSideDataType type,
>>                            uint8_t *data, size_t size);
>> 
>> +/**
>> + * Remove and free side data instances of the given type.
>> + */
>> +void av_packet_remove_side_data(AVPacket *pkt, enum AVPacketSideDataType type);
>> +
>> /**
>> * Shrink the already allocated side data buffer
>> *
>> diff --git a/libavcodec/tests/avpacket.c b/libavcodec/tests/avpacket.c
>> index 7a70ade4c3..710a964915 100644
>> --- a/libavcodec/tests/avpacket.c
>> +++ b/libavcodec/tests/avpacket.c
>> @@ -124,6 +124,15 @@ int main(void)
>>                "when \"size\" parameter is too large.\n" );
>>        ret = 1;
>>    }
>> +    /* test remove side data */
>> +    av_packet_remove_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA);
>> +    for (int i = 0; i < avpkt->side_data_elems; i++) {
>> +        if (avpkt->side_data[i].type == AV_PKT_DATA_NEW_EXTRADATA) {
>> +            printf("av_packet_remove_side_data failed to remove side data");
>> +            ret = 1;
>> +        }
>> +    }
>> +
>>    /*clean up*/
>>    av_packet_free(&avpkt_clone);
>>    av_packet_free(&avpkt);
>> diff --git a/libavcodec/version.h b/libavcodec/version.h
>> index 74b8baa5f3..76af066d32 100644
>> --- a/libavcodec/version.h
>> +++ b/libavcodec/version.h
>> @@ -28,7 +28,7 @@
>> #include "libavutil/version.h"
>> 
>> #define LIBAVCODEC_VERSION_MAJOR  59
>> -#define LIBAVCODEC_VERSION_MINOR  12
>> +#define LIBAVCODEC_VERSION_MINOR  13
>> #define LIBAVCODEC_VERSION_MICRO 100
>> 
>> #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
>> -- 
>> 2.31.1
>> 
>> _______________________________________________
>> 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 Oct 12, 2021, at 10:42 AM, zhilizhao(赵志立) <quinkblack@foxmail.com> wrote:
> 
>> On Oct 12, 2021, at 2:20 AM, Marton Balint <cus@passwd.hu> wrote:
>> 
>> On Mon, 11 Oct 2021, Zhao Zhili wrote:
>> 
>>> ---
>>> doc/APIchanges              |  3 +++
>>> libavcodec/avpacket.c       | 15 +++++++++++++++
>>> libavcodec/packet.h         |  5 +++++
>>> libavcodec/tests/avpacket.c |  9 +++++++++
>>> libavcodec/version.h        |  2 +-
>>> 5 files changed, 33 insertions(+), 1 deletion(-)
>>> 
> 

Ping.

I searched and surprised to find out there is another version of patch
from James earlier than this one.

https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200908223510.7839-1-jamrial@gmail.com/

There’re some discussion and then stopped. Any way to make some progress
before v3?

> On Nov 4, 2021, at 4:02 PM, zhilizhao(赵志立) <quinkblack@foxmail.com> wrote:
> 
> 
> 
>> On Oct 12, 2021, at 10:42 AM, zhilizhao(赵志立) <quinkblack@foxmail.com> wrote:
>> 
>>> On Oct 12, 2021, at 2:20 AM, Marton Balint <cus@passwd.hu> wrote:
>>> 
>>> On Mon, 11 Oct 2021, Zhao Zhili wrote:
>>> 
>>>> ---
>>>> doc/APIchanges              |  3 +++
>>>> libavcodec/avpacket.c       | 15 +++++++++++++++
>>>> libavcodec/packet.h         |  5 +++++
>>>> libavcodec/tests/avpacket.c |  9 +++++++++
>>>> libavcodec/version.h        |  2 +-
>>>> 5 files changed, 33 insertions(+), 1 deletion(-)
>>>> 
>> 
> 
> Ping.
> _______________________________________________
> 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".