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

[Andreas Rheinhardt]

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