[FFmpeg-devel] [PATCH] avpacket: ABI bump additions
James Almer
jamrial at gmail.com
Wed Jul 7 21:16:50 EEST 2021
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.
>
> Regards,
> Marton
> _______________________________________________
> 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".
More information about the ffmpeg-devel
mailing list