On 6/17/2019 11:34 AM, Andreas Rheinhardt wrote: > 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.
No, it's fine as is. > > - 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". > _______________________________________________ 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".
