diff mbox series

[FFmpeg-devel] libavcodec/avpacket: add av_packet_remove_side_data

Message ID tencent_C8A5C265B4EC6872025568227E9AAD623B0A@qq.com
State New
Headers show
Series [FFmpeg-devel] libavcodec/avpacket: add av_packet_remove_side_data
Related show

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

zhilizhao(赵志立) Oct. 11, 2021, 7:41 a.m. UTC
---
 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(-)

Comments

Marton Balint Oct. 11, 2021, 6:20 p.m. UTC | #1
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".
>
zhilizhao(赵志立) Oct. 12, 2021, 2:42 a.m. UTC | #2
> 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".
diff mbox series

Patch

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;
+        }
+    }
+}
+
 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, \