> On Oct 12, 2021, at 2:20 AM, Marton Balint <c...@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-quinkbl...@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-requ...@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-requ...@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-requ...@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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".