James Almer: > On 6/17/2019 9:44 AM, James Almer wrote: >> On 6/17/2019 12:42 AM, Andreas Rheinhardt wrote: >>> Up until now, ff_cbs_write_packet always initialized the packet >>> structure it received without documenting this behaviour; furthermore, >>> the packet's buffer would (on success) be overwritten with the new >>> buffer without unreferencing the old. This meant that the input packet >>> had to be either clean (otherwise there would be memleaks) in which case >>> the initialization is redundant or uninitialized. ff_cbs_write_packet >>> was never used with uninitialized packets, so the initialization was >>> redundant. Worse yet, it forced callers to use more than one packet and >>> made it difficult to add side-data to a packet designated for output, >>> because said side-data could only be attached after the call to >>> ff_cbs_write_packet. >>> >>> This has been changed. It is now allowed to use a non-blank packet. >>> The currently existing buffer will be unreferenced and replaced by >>> the new one, as will be the accompanying fields (i.e. data and size). >>> The rest isn't touched at all. >>> >>> This change will enable us to use only one packet in the bitstream >>> filters that rely on CBS. >>> >>> This commit also updates the documentation of ff_cbs_write_extradata >>> and ff_cbs_write_packet (to better describe existing behaviour and in >>> the latter case to also describe the new behaviour). >>> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinha...@gmail.com> >>> --- >>> I could also have made it unref the packet's buffer at the beginning; >>> this would have the advantage that the packet's buffer would be freed >>> after the units have been rewritten (if they are rewritten) and after >>> the fragment's buffer has been unreferenced, so that maximum memory >>> consumption would decrease. It would also be in line with all current >>> callers of ff_cbs_write_packet, but maybe it wouldn't be what a future >>> caller wants. What do you think? >>> libavcodec/cbs.c | 3 ++- >>> libavcodec/cbs.h | 10 +++++++++- >>> 2 files changed, 11 insertions(+), 2 deletions(-) >>> >>> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c >>> index 0260ba6f67..47679eca1b 100644 >>> --- a/libavcodec/cbs.c >>> +++ b/libavcodec/cbs.c >>> @@ -357,7 +357,8 @@ int ff_cbs_write_packet(CodedBitstreamContext *ctx, >>> if (!buf) >>> return AVERROR(ENOMEM); >>> >>> - av_init_packet(pkt); >>> + av_buffer_unref(&pkt->buf); >> >> You should probably unref the packet, not just the AVBufferRef. > > Right, i see in patch 2 why you did this. I don't like much how with > this change ff_cbs_write_packet would leave the packet in a weird state > of having the filtered data alongside unrelated props already in packet > provided by the caller. If CBS is ever made public, i'm not sure it's a > behavior we should keep. > > But if right now it allows us to use ff_bsf_get_packet_ref() and remove > av_packet_copy_props() calls, then it's good. > Would you prefer if ff_cbs_write_packet gets renamed to ff_cbs_update_packet_data? Anyway, thanks for taking your time to review this.
- 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-requ...@ffmpeg.org with subject "unsubscribe".
