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. > >> + >> pkt->buf = buf; >> pkt->data = frag->data; >> pkt->size = frag->data_size; >> diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h >> index 967dcd1468..5260a39c63 100644 >> --- a/libavcodec/cbs.h >> +++ b/libavcodec/cbs.h >> @@ -297,7 +297,8 @@ int ff_cbs_write_fragment_data(CodedBitstreamContext >> *ctx, >> /** >> * Write the bitstream of a fragment to the extradata in codec parameters. >> * >> - * This replaces any existing extradata in the structure. >> + * Modifies context and fragment as ff_cbs_write_fragment_data does and >> + * replaces any existing extradata in the structure. >> */ >> int ff_cbs_write_extradata(CodedBitstreamContext *ctx, >> AVCodecParameters *par, >> @@ -305,6 +306,13 @@ int ff_cbs_write_extradata(CodedBitstreamContext *ctx, >> >> /** >> * Write the bitstream of a fragment to a packet. >> + * >> + * Modifies context and fragment as ff_cbs_write_fragment_data does. >> + * >> + * On success, the packet's buf is unreferenced and its buf, data and >> + * size fields are set to the corresponding values from the newly updated >> + * fragment; other fields are not touched. On failure, the packet is not >> + * touched at all. >> */ >> int ff_cbs_write_packet(CodedBitstreamContext *ctx, >> AVPacket *pkt, >> > _______________________________________________ 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".
