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

Lynne dev at lynne.ee
Wed Jul 7 02:40:38 EEST 2021


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.


More information about the ffmpeg-devel mailing list