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

[Andreas Rheinhardt]

James Almer:
> On 7/7/2021 3:12 PM, Marton Balint wrote:
>>
>>
>> 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.
> 
> Others may have not paid attention to this thread just yet. This looks
> like a considerable change, so it probably could use other people's
> opinions.
> 
I am not really convinced either; and as soon as this field is there,
users will (legitimately) expect it to be honoured by us; which is just
not true as no demuxer sets it and all muxers and codecs ignore it.

- Andreas