[FFmpeg-devel] [PATCH] avpacket: ABI bump additions

[Marton Balint]

On Wed, 7 Jul 2021, Lynne wrote:

> 6 Jul 2021, 21:57 by cus at passwd.hu:
>
>>
>>
>> On Tue, 6 Jul 2021, Lynne wrote:
>>
>>> 3 Jun 2021, 06:58 by dev at lynne.ee:
>>>
>>>> Apr 26, 2021, 03:27 by andreas.rheinhardt at outlook.com:
>>>>
>>>>> Lynne:
>>>>>
>>>>>> From 097aed2ac33dda0bb2052d8b0402711ce95079ba Mon Sep 17 00:00:00 2001
>>>>>> From: Lynne <dev at lynne.ee>
>>>>>> Date: Sat, 23 Jan 2021 19:56:18 +0100
>>>>>> Subject: [PATCH] avpacket: ABI bump additions
>>>>>>
>>>>>> ---
>>>>>>  libavcodec/avpacket.c |  5 +++++
>>>>>>  libavcodec/packet.h   | 21 +++++++++++++++++++++
>>>>>>  2 files changed, 26 insertions(+)
>>>>>>
>>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>>>>>> index e32c467586..03b73b3b53 100644
>>>>>> --- a/libavcodec/avpacket.c
>>>>>> +++ b/libavcodec/avpacket.c
>>>>>> @@ -382,6 +382,10 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src)
>>>>>>  dst->flags                = src->flags;
>>>>>>  dst->stream_index         = src->stream_index;
>>>>>>
>>>>>> +    i = av_buffer_replace(&dst->opaque_ref, src->opaque_ref);
>>>>>> +    if (i < 0)
>>>>>> +        return i;
>>>>>>
>>>>>
>>>>> 1. Don't use i here; add a new variable.
>>>>> 2. Up until now, av_packet_ref() and av_packet_copy_props() treat the
>>>>> destination packet as uninitialized and make no attempt at unreferencing
>>>>> its content; yet you try to reuse opaque_ref. Even worse, you might
>>>>> return potentially dangerous packets: If the properties were
>>>>> uninitialized and av_packet_copy_props() failed, then the caller were
>>>>> not allowed to unreference the packet even when the non-properties were
>>>>> set to sane values. The easiest way to fix this is to move setting
>>>>> opaque ref to the place after initializing side_data below and either
>>>>> set dst->opaque_ref to NULL before av_buffer_replace() or to not use
>>>>> av_buffer_replace(). It may also be best to unref it again if copying
>>>>> side data fails.
>>>>>
>>>>>> +
>>>>>>  dst->side_data            = NULL;
>>>>>>  dst->side_data_elems      = 0;
>>>>>>  for (i = 0; i < src->side_data_elems; i++) {
>>>>>> @@ -403,6 +407,7 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src)
>>>>>>  void av_packet_unref(AVPacket *pkt)
>>>>>>  {
>>>>>>  av_packet_free_side_data(pkt);
>>>>>> +    av_buffer_unref(&pkt->opaque_ref);
>>>>>>  av_buffer_unref(&pkt->buf);
>>>>>>  get_packet_defaults(pkt);
>>>>>>  }
>>>>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
>>>>>> index fad8341c12..c29ad18a2b 100644
>>>>>> --- a/libavcodec/packet.h
>>>>>> +++ b/libavcodec/packet.h
>>>>>> @@ -383,6 +383,27 @@ typedef struct AVPacket {
>>>>>>  int64_t duration;
>>>>>>
>>>>>>  int64_t pos;                            ///< byte position in stream, -1 if unknown
>>>>>> +
>>>>>> +    /**
>>>>>> +     * for some private data of the user
>>>>>> +     */
>>>>>> +    void *opaque;
>>>>>>
>>>>>
>>>>> The corresponding AVFrame field is copied when copying props.
>>>>>
>>>>
>>>> Fixed both, thanks. Also copied the time_base field and made av_init_packet()
>>>> initialize all fields.
>>>> Patch attached. Sorry it's taking me so long to work on this and the Vulkan ABI changes.
>>>>
>>>
>>> Since no one yet has objected, will push this in 3 days unless someone does.
>>>
>>
>> Can you reply this this please?
>>
>> http://ffmpeg.org/pipermail/ffmpeg-devel/2021-May/279855.html
>>
>
>> You wrote earlier that you don't like that you have to pass packets to the
>> muxer in a timebase as set by the muxer's init function. Solving this by
>> adding a muxer open flag which saves the preferred time base of the user
>> and rescales all packets from the user's preferred time base to the real
>> time base before processing seems much more managable than introducing the
>> AVPacket->time_base support everywhere and as far as I see it solves this
>> problem just the same.
>
> I'm mainly introducing the field for myself. I have some (de)muxer code that's
> timestamp dependent, and timestamps don't make sense without knowing the
> time base. Since multiple streams go into that code, having a sideband way
> to plumb the timebases made the code very messy. The fact that they can
> also be used to save on an awkward and complicated mechanism to
> negotiate just comes as a bonus. Honestly, I had to read the mpv source
> code to realize that this is the correct sequence that has to happen,
> when none of this is even necessary. I'm sure other API users will find the
> field useful.
>
> I don't mind having a way to set the preferred time base, but I do think it'll
> be even more confusing if it converts time bases as well. We need a way
> to give a hint to muxers what time base you'd like, but I'd rather have it
> just remain a hint rather than also do the conversion, since it'll limit the
> usability to just your stream's input timebase. And if you have to specify your
> stream input timebase somewhere, then this would be better, where it's
> relevant.
>
>> Are there similar problems elsewhere? If there are, then is it not more
>> managable to allow the user to specify a preferred input or output
>> timebase during init instead of allowing per-packet timebases? By adding
>> time_base to AVPacket you basically say that it is possible
>> that each packet of a stream have a different time base. But in realitly,
>> this never happens.
>
> We can add a comment saying this will never happen. Although if
> we do decide to allow packet timebase to dynamically change,
> and we add a similar field to frames (which I plan to do after this, but since
> we can add fields to AVFrames easily, I left it for later), then we would be
> able to dynamically handle more without effort from the user.
> For example, streams could be switched and concatenated in a way
> that doesn't break demuxing. elenril has some plans on writing a concat bsf,
> so he can say whether that could be useful there.

OK, thanks. I can't say I am fully convinced, but apparently nobody shares 
my concenrs, so feel free to go ahead with it.

Regards,
Marton