[FFmpeg-devel] [PATCH] libavcodec/avpacket: add av_packet_remove_side_data
"zhilizhao(赵志立)"
quinkblack at foxmail.com
Tue Oct 12 05:42:07 EEST 2021
> On Oct 12, 2021, at 2:20 AM, Marton Balint <cus at 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 at ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at 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 at ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
More information about the ffmpeg-devel
mailing list