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

[James Almer]

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

No, it's fine as is.

> 
> - Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>