[FFmpeg-devel] [PATCH 01/18] cbs: Allow non-blank packets in ff_cbs_write_packet
jamrial at gmail.com
Mon Jun 17 15:54:58 EEST 2019
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
>> 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.rheinhardt at 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,
More information about the ffmpeg-devel