[FFmpeg-devel] [PATCH] avpacket: ABI bump additions
Lynne
dev at lynne.ee
Fri Jul 9 05:42:07 EEST 2021
8 Jul 2021, 23:20 by andreas.rheinhardt at outlook.com:
> 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.
>
I can change the patch to either initialize it as an invalid value (which would
signal the user to instead get the timebase elsewhere) or set its value
when the packet passes through the common demuxing function.
Having this field in does not imply it's used during muxing at all, and in fact
makes it easier to figure out how to properly mux since we can add the info
to the comment (which we will have to, regardless of how we decide to implement
the automatic method for timestamp rescaling, if we ever do decide to).
More information about the ffmpeg-devel
mailing list