[FFmpeg-devel] [PATCH 01/18] cbs: Allow non-blank packets in ff_cbs_write_packet

[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.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,
>>
>