diff mbox series

[FFmpeg-devel] avpacket: RFC for ABI bump additions

Message ID MRkdyHg--3-2@lynne.ee
State New
Headers show
Series [FFmpeg-devel] avpacket: RFC for ABI bump additions
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Lynne Jan. 23, 2021, 7:10 p.m. UTC
This is an RFC about the upcoming additions to the AVPacket structure
(whose size is still part of the ABI, so we need to plan for any changes).

The current RFC patch adds 3 fields:
    - "void *opaque;" for the user to use as they wish, same as AVFrame.opaque
    - "void *opaque_ref;" for more permanent and propagating user data, same as AVFrame.opaque_ref
    - "AVRational time_base;" which will be set to indicate the time base of the packet's timestamps

Some devs (JEEB) wanted reception timestamps and original, overflowed timestamps for MPEG-TS.
I'd be willing to add a reception timestamp as long as we add an additional time_base field
and make it independent of the packet's pts field, since those timestamps are usually on
highly precise system clock time bases, and reducing their precision would be undesirable.

I'm not sure about overflowed original timestamps or how they would help.
Perhaps we should introduce an AVBufferRef *internal_ref like with AVFrame to
store such single library-only data?
Subject: [PATCH] avpacket: RFC for ABI bump additions

---
 doc/APIchanges        |  3 +++
 libavcodec/avpacket.c |  9 +++++++++
 libavcodec/packet.h   | 23 +++++++++++++++++++++++
 3 files changed, 35 insertions(+)

Comments

Gyan Doshi Jan. 23, 2021, 7:36 p.m. UTC | #1
On 24-01-2021 12:40 am, Lynne wrote:
> This is an RFC about the upcoming additions to the AVPacket structure
> (whose size is still part of the ABI, so we need to plan for any changes).
>
> The current RFC patch adds 3 fields:
>      - "void *opaque;" for the user to use as they wish, same as AVFrame.opaque
>      - "void *opaque_ref;" for more permanent and propagating user data, same as AVFrame.opaque_ref
>      - "AVRational time_base;" which will be set to indicate the time base of the packet's timestamps
>
> Some devs (JEEB) wanted reception timestamps and original, overflowed timestamps for MPEG-TS.
> I'd be willing to add a reception timestamp as long as we add an additional time_base field
> and make it independent of the packet's pts field, since those timestamps are usually on
> highly precise system clock time bases, and reducing their precision would be undesirable.
>
> I'm not sure about overflowed original timestamps or how they would help.

Reception timestamps sounds useful.

Overflow timestamps may be useful, Right now, lavf rolls over timestamps 
correctly only once, as the offset doesn't accommodate further loops.


Regards,
Gyan
Marton Balint Jan. 23, 2021, 8:04 p.m. UTC | #2
On Sat, 23 Jan 2021, Lynne wrote:

> This is an RFC about the upcoming additions to the AVPacket structure
> (whose size is still part of the ABI, so we need to plan for any changes).
>
> The current RFC patch adds 3 fields:
>     - "void *opaque;" for the user to use as they wish, same as AVFrame.opaque
>     - "void *opaque_ref;" for more permanent and propagating user data, same as AVFrame.opaque_ref

These seem legit.

>     - "AVRational time_base;" which will be set to indicate the time base of the packet's timestamps

Why? It seems redundant and it will not be clear when to use to use 
stream/bsf/etc time base and when embedded AVPacket timebase. So I don't 
think this is a good idea.

>
> Some devs (JEEB) wanted reception timestamps and original, overflowed timestamps for MPEG-TS.

MPEG-TS with timestamps has many issues and I am afraid the introduction 
of original/overflowed timestamps won't solve them:
- discontinuity - the discontinuity should be hidden from the output 
timestamps, but the discontinuity should be detected based on the PCR of 
the streams
- parsers - the parsers merge/split packets as they want, who knows what 
will happen to the timestamps...
- overflows - ffmpeg's "infrastructure" for packet timestamp overflows 
only handles a single wraparound, not multiple, so it is useless for 
anything serious.

> I'd be willing to add a reception timestamp as long as we add an additional time_base field
> and make it independent of the packet's pts field, since those timestamps are usually on
> highly precise system clock time bases, and reducing their precision would be undesirable.

I don't know, I'd really like to see some actual benefit of these extra
timestamps before we agree to add specific fields for it. Until then, maye 
just use side data?

>
> I'm not sure about overflowed original timestamps or how they would help.
> Perhaps we should introduce an AVBufferRef *internal_ref like with AVFrame to
> store such single library-only data?

I guess you mean private_ref. This can be added, if for nothing else, then 
for consistency. :)

Regards,
Marton
Lynne Jan. 23, 2021, 8:24 p.m. UTC | #3
Jan 23, 2021, 21:04 by cus@passwd.hu:

>
>
> On Sat, 23 Jan 2021, Lynne wrote:
>
>> This is an RFC about the upcoming additions to the AVPacket structure
>> (whose size is still part of the ABI, so we need to plan for any changes).
>>
>> The current RFC patch adds 3 fields:
>>     - "void *opaque;" for the user to use as they wish, same as AVFrame.opaque
>>     - "void *opaque_ref;" for more permanent and propagating user data, same as AVFrame.opaque_ref
>>
>
> These seem legit.
>
>>     - "AVRational time_base;" which will be set to indicate the time base of the packet's timestamps
>>
>
> Why? It seems redundant and it will not be clear when to use to use stream/bsf/etc time base and when embedded AVPacket timebase. So I don't think this is a good idea.
>

I'd like to switch to using this to avoid the dance you have to do with
avformat init, where you have to give it your packet's time_base in the stream time_base
then init, which then sets the time_base in the same field you provided your time_base,
and then you have to rescale the timestamps of packets to that timebase.
Wouldn't it be simpler to just give avformat packets and let it adjust the
packet time base internally as it sees fit and store it in a private_ref field?

It was a struggle figuring out how to init avformat properly since none of this
was documented when I was implementing it, and it was only thanks to
comments in mpv's source that I figured it out.
Marton Balint Jan. 23, 2021, 8:42 p.m. UTC | #4
On Sat, 23 Jan 2021, Lynne wrote:

> Jan 23, 2021, 21:04 by cus@passwd.hu:
>
>>
>>
>> On Sat, 23 Jan 2021, Lynne wrote:
>>
>>> This is an RFC about the upcoming additions to the AVPacket structure
>>> (whose size is still part of the ABI, so we need to plan for any changes).
>>>
>>> The current RFC patch adds 3 fields:
>>>     - "void *opaque;" for the user to use as they wish, same as AVFrame.opaque
>>>     - "void *opaque_ref;" for more permanent and propagating user data, same as AVFrame.opaque_ref
>>>
>>
>> These seem legit.
>>
>>>     - "AVRational time_base;" which will be set to indicate the time base of the packet's timestamps
>>>
>>
>> Why? It seems redundant and it will not be clear when to use to use stream/bsf/etc time base and when embedded AVPacket timebase. So I don't think this is a good idea.
>>
>
> I'd like to switch to using this to avoid the dance you have to do with
> avformat init, where you have to give it your packet's time_base in the stream time_base
> then init, which then sets the time_base in the same field you provided your time_base,
> and then you have to rescale the timestamps of packets to that timebase.

That is by design as far as I know, you set the timebase to your 
requested time base, if the format supports that then you are happy, if 
not, then you convert.

> Wouldn't it be simpler to just give avformat packets and let it adjust the
> packet time base internally as it sees fit and store it in a private_ref field?

I would not want avformat to rescale timestamps behind my back, that way I 
control how I lose precision.

Regards,
Marton

>
> It was a struggle figuring out how to init avformat properly since none of this
> was documented when I was implementing it, and it was only thanks to
> comments in mpv's source that I figured it out.
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Lynne Jan. 23, 2021, 9:17 p.m. UTC | #5
Jan 23, 2021, 21:42 by cus@passwd.hu:

>
>
> On Sat, 23 Jan 2021, Lynne wrote:
>
>> Jan 23, 2021, 21:04 by cus@passwd.hu:
>>
>>>
>>>
>>> On Sat, 23 Jan 2021, Lynne wrote:
>>>
>>>> This is an RFC about the upcoming additions to the AVPacket structure
>>>> (whose size is still part of the ABI, so we need to plan for any changes).
>>>>
>>>> The current RFC patch adds 3 fields:
>>>>     - "void *opaque;" for the user to use as they wish, same as AVFrame.opaque
>>>>     - "void *opaque_ref;" for more permanent and propagating user data, same as AVFrame.opaque_ref
>>>>
>>>
>>> These seem legit.
>>>
>>>>     - "AVRational time_base;" which will be set to indicate the time base of the packet's timestamps
>>>>
>>>
>>> Why? It seems redundant and it will not be clear when to use to use stream/bsf/etc time base and when embedded AVPacket timebase. So I don't think this is a good idea.
>>>
>>
>> I'd like to switch to using this to avoid the dance you have to do with
>> avformat init, where you have to give it your packet's time_base in the stream time_base
>> then init, which then sets the time_base in the same field you provided your time_base,
>> and then you have to rescale the timestamps of packets to that timebase.
>>
>
> That is by design as far as I know, you set the timebase to your requested time base, if the format supports that then you are happy, if not, then you convert.
>

You can still keep the mechanism, since it's init time, but what's
the problem with letting lavf convert the timestamps for you if they don't
match? It wouldn't break any behavior apart from what's currently 
improper API usage. And it would make the API much more usable, as
most users just want to mux at the FFMIN(muxer_max_tb, incoming_stream_tb)
rather than a specific timebase.
Marton Balint Jan. 23, 2021, 10:12 p.m. UTC | #6
On Sat, 23 Jan 2021, Lynne wrote:

> Jan 23, 2021, 21:42 by cus@passwd.hu:
>
>>
>>
>> On Sat, 23 Jan 2021, Lynne wrote:
>>
>>> Jan 23, 2021, 21:04 by cus@passwd.hu:
>>>
>>>>
>>>>
>>>> On Sat, 23 Jan 2021, Lynne wrote:
>>>>
>>>>> This is an RFC about the upcoming additions to the AVPacket structure
>>>>> (whose size is still part of the ABI, so we need to plan for any changes).
>>>>>
>>>>> The current RFC patch adds 3 fields:
>>>>>     - "void *opaque;" for the user to use as they wish, same as AVFrame.opaque
>>>>>     - "void *opaque_ref;" for more permanent and propagating user data, same as AVFrame.opaque_ref
>>>>>
>>>>
>>>> These seem legit.
>>>>
>>>>>     - "AVRational time_base;" which will be set to indicate the time base of the packet's timestamps
>>>>>
>>>>
>>>> Why? It seems redundant and it will not be clear when to use to use stream/bsf/etc time base and when embedded AVPacket timebase. So I don't think this is a good idea.
>>>>
>>>
>>> I'd like to switch to using this to avoid the dance you have to do with
>>> avformat init, where you have to give it your packet's time_base in the stream time_base
>>> then init, which then sets the time_base in the same field you provided your time_base,
>>> and then you have to rescale the timestamps of packets to that timebase.
>>>
>>
>> That is by design as far as I know, you set the timebase to your requested time base, if the format supports that then you are happy, if not, then you convert.
>>
>
> You can still keep the mechanism, since it's init time, but what's
> the problem with letting lavf convert the timestamps for you if they don't
> match?

And why do you need per-AVPacket time bases for that if your packets are 
in a fixed timestamp anyway?

> It wouldn't break any behavior apart from what's currently 
> improper API usage. And it would make the API much more usable, as

I don't see why, you only gain that you don't have to call 
av_packet_rescale_ts() yourself.

> most users just want to mux at the FFMIN(muxer_max_tb, incoming_stream_tb)
> rather than a specific timebase.

The user should preferably mux audio in 1/sample_rate time base, video in 
1/frame_rate time base, and that's what ffmpeg.c does AFAIK.

Regards,
Marton
Lynne Jan. 23, 2021, 10:30 p.m. UTC | #7
Jan 23, 2021, 23:12 by cus@passwd.hu:

>
>
> On Sat, 23 Jan 2021, Lynne wrote:
>
>> Jan 23, 2021, 21:42 by cus@passwd.hu:
>>
>>>
>>>
>>> On Sat, 23 Jan 2021, Lynne wrote:
>>>
>>>> Jan 23, 2021, 21:04 by cus@passwd.hu:
>>>>
>>>>>
>>>>>
>>>>> On Sat, 23 Jan 2021, Lynne wrote:
>>>>>
>>>>>> This is an RFC about the upcoming additions to the AVPacket structure
>>>>>> (whose size is still part of the ABI, so we need to plan for any changes).
>>>>>>
>>>>>> The current RFC patch adds 3 fields:
>>>>>>     - "void *opaque;" for the user to use as they wish, same as AVFrame.opaque
>>>>>>     - "void *opaque_ref;" for more permanent and propagating user data, same as AVFrame.opaque_ref
>>>>>>
>>>>>
>>>>> These seem legit.
>>>>>
>>>>>>     - "AVRational time_base;" which will be set to indicate the time base of the packet's timestamps
>>>>>>
>>>>>
>>>>> Why? It seems redundant and it will not be clear when to use to use stream/bsf/etc time base and when embedded AVPacket timebase. So I don't think this is a good idea.
>>>>>
>>>>
>>>> I'd like to switch to using this to avoid the dance you have to do with
>>>> avformat init, where you have to give it your packet's time_base in the stream time_base
>>>> then init, which then sets the time_base in the same field you provided your time_base,
>>>> and then you have to rescale the timestamps of packets to that timebase.
>>>>
>>>
>>> That is by design as far as I know, you set the timebase to your requested time base, if the format supports that then you are happy, if not, then you convert.
>>>
>>
>> You can still keep the mechanism, since it's init time, but what's
>> the problem with letting lavf convert the timestamps for you if they don't
>> match?
>>
>
> And why do you need per-AVPacket time bases for that if your packets are in a fixed timestamp anyway?
>

If we don't change anything regarding the lavf API, it's exactly why this field was added:
So you could rescale the packet timestamps without having to match them
up to a stream. In my code the muxer loop just gets packets from encoders via a FIFO
and has to match them up to whatever stream the encoder was registered to use,
then look up its time base and rescale from that timebase. Currently I just use some
leftover fields as a hack, and while I could just carry that info in an opaque_ref,
it would be neater to just have all the info necessary to rescale every packet's timestamps
in the packet itself.
Anton Khirnov Jan. 24, 2021, 4:05 p.m. UTC | #8
Quoting Lynne (2021-01-23 20:10:46)
> This is an RFC about the upcoming additions to the AVPacket structure
> (whose size is still part of the ABI, so we need to plan for any changes).
> 
> The current RFC patch adds 3 fields:
>     - "void *opaque;" for the user to use as they wish, same as AVFrame.opaque
>     - "void *opaque_ref;" for more permanent and propagating user data, same as AVFrame.opaque_ref

Generally in favor, but:
- do we really need both?
- ownership semantics should be specified more clearly, so there is no
  ambiguity over who is allowed to set it. Something like:
    whoever owns (i.e. is responsible for unreffing) the packet ref also
    owns opaque_ref

>     - "AVRational time_base;" which will be set to indicate the time base of the packet's timestamps

In favor - should reduce confusion and existing users can still rescale
timestamps manually before passing them to lavf. But then it should also be added to AVFrame.
And compatibility will be tricky - e.g. encoders setting this field will
break muxing in lavf (since current users all rescale manually).

> 
> Some devs (JEEB) wanted reception timestamps and original, overflowed timestamps for MPEG-TS.
> I'd be willing to add a reception timestamp as long as we add an additional time_base field
> and make it independent of the packet's pts field, since those timestamps are usually on
> highly precise system clock time bases, and reducing their precision would be undesirable.

This sounds like side data to me.

> 
> I'm not sure about overflowed original timestamps or how they would help.
> Perhaps we should introduce an AVBufferRef *internal_ref like with AVFrame to
> store such single library-only data?

I don't like treating the libraries specially, so -1 for me.
internal_ref should not exist either.
Lynne Jan. 24, 2021, 5:34 p.m. UTC | #9
Jan 24, 2021, 17:05 by anton@khirnov.net:

> Quoting Lynne (2021-01-23 20:10:46)
>
>> This is an RFC about the upcoming additions to the AVPacket structure
>> (whose size is still part of the ABI, so we need to plan for any changes).
>>
>> The current RFC patch adds 3 fields:
>>     - "void *opaque;" for the user to use as they wish, same as AVFrame.opaque
>>     - "void *opaque_ref;" for more permanent and propagating user data, same as AVFrame.opaque_ref
>>
>
> Generally in favor, but:
> - do we really need both?
> - ownership semantics should be specified more clearly, so there is no
>  ambiguity over who is allowed to set it. Something like:
>  whoever owns (i.e. is responsible for unreffing) the packet ref also
>  owns opaque_ref
>

I think the void * is useful for API users. They can set it to a local context
(such as their own muxing context), while the opaque_ref can be used for
more global info (internal metadata, reception timestamp for the source
so latency can be measured and such).
As for ownership sematics, the description copied verbatim from
libavutil/frame.h. To keep sync, I think that description should be expanded
first.



>>     - "AVRational time_base;" which will be set to indicate the time base of the packet's timestamps
>>
>
> In favor - should reduce confusion and existing users can still rescale
> timestamps manually before passing them to lavf. But then it should also be added to AVFrame.
>

Thought so too, will send a patch for AVFrame.time_base.



> And compatibility will be tricky - e.g. encoders setting this field will
> break muxing in lavf (since current users all rescale manually).
>

We discussed this in IRC, the conclusion was to add an optional
flag to every major context (bsfc, avc, lavf, lavfi*) which will
set the time_base value of produced avframes and avpacket,
and if it's present, then we can automagically rescale the timestamps
in cosumers/producers.
The flag will be deprecated once the bump happens. If the user
wishes to rescale themselves, they will still be able to do that
once the flag is removed.



>>
>> Some devs (JEEB) wanted reception timestamps and original, overflowed timestamps for MPEG-TS.
>> I'd be willing to add a reception timestamp as long as we add an additional time_base field
>> and make it independent of the packet's pts field, since those timestamps are usually on
>> highly precise system clock time bases, and reducing their precision would be undesirable.
>>
>
> This sounds like side data to me.
>
>>
>> I'm not sure about overflowed original timestamps or how they would help.
>> Perhaps we should introduce an AVBufferRef *internal_ref like with AVFrame to
>> store such single library-only data?
>>
>
> I don't like treating the libraries specially, so -1 for me.
> internal_ref should not exist either.
>

Yeah, tbh it's a single-library only field in avframes and is mainly used
for hardware API weirdness reasons.


Also another suggestion: should we add a few dozen bytes of padding at
the end of the avpacket structure so if anything were to happen
we wouldn't be block on waiting for a bump to add a field?
Anton Khirnov Jan. 26, 2021, 4:02 p.m. UTC | #10
Quoting Lynne (2021-01-24 18:34:36)
> >>
> >> Some devs (JEEB) wanted reception timestamps and original, overflowed timestamps for MPEG-TS.
> >> I'd be willing to add a reception timestamp as long as we add an additional time_base field
> >> and make it independent of the packet's pts field, since those timestamps are usually on
> >> highly precise system clock time bases, and reducing their precision would be undesirable.
> >>
> >
> > This sounds like side data to me.
> >
> >>
> >> I'm not sure about overflowed original timestamps or how they would help.
> >> Perhaps we should introduce an AVBufferRef *internal_ref like with AVFrame to
> >> store such single library-only data?
> >>
> >
> > I don't like treating the libraries specially, so -1 for me.
> > internal_ref should not exist either.
> >
> 
> Yeah, tbh it's a single-library only field in avframes and is mainly used
> for hardware API weirdness reasons.

It's not even necessary, that code can work without it.

> Also another suggestion: should we add a few dozen bytes of padding at
> the end of the avpacket structure so if anything were to happen
> we wouldn't be block on waiting for a bump to add a field?

Ugly, but I guess acceptable? Have to be pretty careful what you add
though.
James Almer Jan. 26, 2021, 4:07 p.m. UTC | #11
On 1/26/2021 1:02 PM, Anton Khirnov wrote:
> Quoting Lynne (2021-01-24 18:34:36)
>>>>
>>>> Some devs (JEEB) wanted reception timestamps and original, overflowed timestamps for MPEG-TS.
>>>> I'd be willing to add a reception timestamp as long as we add an additional time_base field
>>>> and make it independent of the packet's pts field, since those timestamps are usually on
>>>> highly precise system clock time bases, and reducing their precision would be undesirable.
>>>>
>>>
>>> This sounds like side data to me.
>>>
>>>>
>>>> I'm not sure about overflowed original timestamps or how they would help.
>>>> Perhaps we should introduce an AVBufferRef *internal_ref like with AVFrame to
>>>> store such single library-only data?
>>>>
>>>
>>> I don't like treating the libraries specially, so -1 for me.
>>> internal_ref should not exist either.
>>>
>>
>> Yeah, tbh it's a single-library only field in avframes and is mainly used
>> for hardware API weirdness reasons.
> 
> It's not even necessary, that code can work without it.
> 
>> Also another suggestion: should we add a few dozen bytes of padding at
>> the end of the avpacket structure so if anything were to happen
>> we wouldn't be block on waiting for a bump to add a field?
> 
> Ugly, but I guess acceptable? Have to be pretty careful what you add
> though.

Alternatively, we could follow through with the idea of making its size 
stop being part of the ABI.
It's a big change since AVPacket is kinda advertised as one of the few 
structs with ABI-tied size, and it wont happen until at least two years 
from now. But now that the old deprecated decode/encode API is being 
removed and non reference counted packets will no longer be a thing, 
it's probably a good time to do it.
Anton Khirnov Jan. 26, 2021, 4:17 p.m. UTC | #12
Quoting James Almer (2021-01-26 17:07:28)
> On 1/26/2021 1:02 PM, Anton Khirnov wrote:
> > Quoting Lynne (2021-01-24 18:34:36)
> >>>>
> >>>> Some devs (JEEB) wanted reception timestamps and original, overflowed timestamps for MPEG-TS.
> >>>> I'd be willing to add a reception timestamp as long as we add an additional time_base field
> >>>> and make it independent of the packet's pts field, since those timestamps are usually on
> >>>> highly precise system clock time bases, and reducing their precision would be undesirable.
> >>>>
> >>>
> >>> This sounds like side data to me.
> >>>
> >>>>
> >>>> I'm not sure about overflowed original timestamps or how they would help.
> >>>> Perhaps we should introduce an AVBufferRef *internal_ref like with AVFrame to
> >>>> store such single library-only data?
> >>>>
> >>>
> >>> I don't like treating the libraries specially, so -1 for me.
> >>> internal_ref should not exist either.
> >>>
> >>
> >> Yeah, tbh it's a single-library only field in avframes and is mainly used
> >> for hardware API weirdness reasons.
> > 
> > It's not even necessary, that code can work without it.
> > 
> >> Also another suggestion: should we add a few dozen bytes of padding at
> >> the end of the avpacket structure so if anything were to happen
> >> we wouldn't be block on waiting for a bump to add a field?
> > 
> > Ugly, but I guess acceptable? Have to be pretty careful what you add
> > though.
> 
> Alternatively, we could follow through with the idea of making its size 
> stop being part of the ABI.
> It's a big change since AVPacket is kinda advertised as one of the few 
> structs with ABI-tied size, and it wont happen until at least two years 
> from now. But now that the old deprecated decode/encode API is being 
> removed and non reference counted packets will no longer be a thing, 
> it's probably a good time to do it.

We could start by adding a field to AVPacket that would be set to a
magic value by av_packet_alloc().
Then have e.g. AVCodecContext/AVFormatContext warn when they see a
packet without this magic value.
James Almer Jan. 26, 2021, 7:11 p.m. UTC | #13
On 1/26/2021 1:17 PM, Anton Khirnov wrote:
> Quoting James Almer (2021-01-26 17:07:28)
>> On 1/26/2021 1:02 PM, Anton Khirnov wrote:
>>> Quoting Lynne (2021-01-24 18:34:36)
>>>>>>
>>>>>> Some devs (JEEB) wanted reception timestamps and original, overflowed timestamps for MPEG-TS.
>>>>>> I'd be willing to add a reception timestamp as long as we add an additional time_base field
>>>>>> and make it independent of the packet's pts field, since those timestamps are usually on
>>>>>> highly precise system clock time bases, and reducing their precision would be undesirable.
>>>>>>
>>>>>
>>>>> This sounds like side data to me.
>>>>>
>>>>>>
>>>>>> I'm not sure about overflowed original timestamps or how they would help.
>>>>>> Perhaps we should introduce an AVBufferRef *internal_ref like with AVFrame to
>>>>>> store such single library-only data?
>>>>>>
>>>>>
>>>>> I don't like treating the libraries specially, so -1 for me.
>>>>> internal_ref should not exist either.
>>>>>
>>>>
>>>> Yeah, tbh it's a single-library only field in avframes and is mainly used
>>>> for hardware API weirdness reasons.
>>>
>>> It's not even necessary, that code can work without it.
>>>
>>>> Also another suggestion: should we add a few dozen bytes of padding at
>>>> the end of the avpacket structure so if anything were to happen
>>>> we wouldn't be block on waiting for a bump to add a field?
>>>
>>> Ugly, but I guess acceptable? Have to be pretty careful what you add
>>> though.
>>
>> Alternatively, we could follow through with the idea of making its size
>> stop being part of the ABI.
>> It's a big change since AVPacket is kinda advertised as one of the few
>> structs with ABI-tied size, and it wont happen until at least two years
>> from now. But now that the old deprecated decode/encode API is being
>> removed and non reference counted packets will no longer be a thing,
>> it's probably a good time to do it.
> 
> We could start by adding a field to AVPacket that would be set to a
> magic value by av_packet_alloc().
> Then have e.g. AVCodecContext/AVFormatContext warn when they see a
> packet without this magic value.

I don't like much the idea of adding a public field just to emit a 
deprecation warning. And adding something like an internal pointer 
(Alongside a struct like AVPacketInternal) or an analog to 
AVFrame.private_ref would be an allocation overhead per packet, at least 
during the deprecation period (assuming it's not kept in place for other 
uses after that).
Anton Khirnov Jan. 27, 2021, 9:16 a.m. UTC | #14
Quoting James Almer (2021-01-26 20:11:16)
> On 1/26/2021 1:17 PM, Anton Khirnov wrote:
> > We could start by adding a field to AVPacket that would be set to a
> > magic value by av_packet_alloc().
> > Then have e.g. AVCodecContext/AVFormatContext warn when they see a
> > packet without this magic value.
> 
> I don't like much the idea of adding a public field just to emit a 
> deprecation warning.


int internal_do_not_touch; // do not touch

is not really public. It is visible in the public headers, but so are
all the AVFooInternal. I agree that it is not the prettiest thing ever,
but it's not too bad.

And I believe it would solve a real problem, since we have few other
ways to let our users know they need to change something. Most of them
do not follow development closely, I'd think.
James Almer Jan. 27, 2021, 3:07 p.m. UTC | #15
On 1/27/2021 6:16 AM, Anton Khirnov wrote:
> Quoting James Almer (2021-01-26 20:11:16)
>> On 1/26/2021 1:17 PM, Anton Khirnov wrote:
>>> We could start by adding a field to AVPacket that would be set to a
>>> magic value by av_packet_alloc().
>>> Then have e.g. AVCodecContext/AVFormatContext warn when they see a
>>> packet without this magic value.
>>
>> I don't like much the idea of adding a public field just to emit a
>> deprecation warning.
> 
> 
> int internal_do_not_touch; // do not touch
> 
> is not really public. It is visible in the public headers, but so are
> all the AVFooInternal. I agree that it is not the prettiest thing ever,
> but it's not too bad.
> 
> And I believe it would solve a real problem, since we have few other
> ways to let our users know they need to change something. Most of them
> do not follow development closely, I'd think.

If sizeof(AVPacket) stops being part of the ABI, then av_init_packet() 
becomes unnecessary, right? av_packet_unref() will be the only valid way 
for the user to reuse the AVPacket. So that deprecation warning might be 
enough for stack users.

Regarding a new internal field that lavf could check, i don't think it's 
enough since it may be uninitialized for packets on stack. And you can't 
make av_init_packet() set it to 0/NULL because then av_packet_unref() 
will also reset it, and lavf will start printing bogus warnings for 
allocated packet users.
Lynne Jan. 27, 2021, 5:40 p.m. UTC | #16
Jan 27, 2021, 16:07 by jamrial@gmail.com:

> On 1/27/2021 6:16 AM, Anton Khirnov wrote:
>
>> Quoting James Almer (2021-01-26 20:11:16)
>>
>>> On 1/26/2021 1:17 PM, Anton Khirnov wrote:
>>>
>>>> We could start by adding a field to AVPacket that would be set to a
>>>> magic value by av_packet_alloc().
>>>> Then have e.g. AVCodecContext/AVFormatContext warn when they see a
>>>> packet without this magic value.
>>>>
>>>
>>> I don't like much the idea of adding a public field just to emit a
>>> deprecation warning.
>>>
>>
>>
>> int internal_do_not_touch; // do not touch
>>
>> is not really public. It is visible in the public headers, but so are
>> all the AVFooInternal. I agree that it is not the prettiest thing ever,
>> but it's not too bad.
>>
>> And I believe it would solve a real problem, since we have few other
>> ways to let our users know they need to change something. Most of them
>> do not follow development closely, I'd think.
>>
>
> If sizeof(AVPacket) stops being part of the ABI, then av_init_packet() becomes unnecessary, right? av_packet_unref() will be the only valid way for the user to reuse the AVPacket. So that deprecation warning might be enough for stack users.
>

I think just a compile-time deprecation on av_init_packet() would be enough.



> Regarding a new internal field that lavf could check, i don't think it's enough since it may be uninitialized for packets on stack. And you can't make av_init_packet() set it to 0/NULL because then av_packet_unref() will also reset it, and lavf will start printing bogus warnings for allocated packet users.
>

I'd really rather not spam the log of users with API deprecation messages. We're
already guilty enough of doing that with color_range.
James Almer Jan. 27, 2021, 7:37 p.m. UTC | #17
On 1/27/2021 2:40 PM, Lynne wrote:
> Jan 27, 2021, 16:07 by jamrial@gmail.com:
> 
>> On 1/27/2021 6:16 AM, Anton Khirnov wrote:
>>
>>> Quoting James Almer (2021-01-26 20:11:16)
>>>
>>>> On 1/26/2021 1:17 PM, Anton Khirnov wrote:
>>>>
>>>>> We could start by adding a field to AVPacket that would be set to a
>>>>> magic value by av_packet_alloc().
>>>>> Then have e.g. AVCodecContext/AVFormatContext warn when they see a
>>>>> packet without this magic value.
>>>>>
>>>>
>>>> I don't like much the idea of adding a public field just to emit a
>>>> deprecation warning.
>>>>
>>>
>>>
>>> int internal_do_not_touch; // do not touch
>>>
>>> is not really public. It is visible in the public headers, but so are
>>> all the AVFooInternal. I agree that it is not the prettiest thing ever,
>>> but it's not too bad.
>>>
>>> And I believe it would solve a real problem, since we have few other
>>> ways to let our users know they need to change something. Most of them
>>> do not follow development closely, I'd think.
>>>
>>
>> If sizeof(AVPacket) stops being part of the ABI, then av_init_packet() becomes unnecessary, right? av_packet_unref() will be the only valid way for the user to reuse the AVPacket. So that deprecation warning might be enough for stack users.
>>
> 
> I think just a compile-time deprecation on av_init_packet() would be enough.
> 
> 
> 
>> Regarding a new internal field that lavf could check, i don't think it's enough since it may be uninitialized for packets on stack. And you can't make av_init_packet() set it to 0/NULL because then av_packet_unref() will also reset it, and lavf will start printing bogus warnings for allocated packet users.
>>
> 
> I'd really rather not spam the log of users with API deprecation messages. We're
> already guilty enough of doing that with color_range.

Ok, something like this patch, then?

Assuming we go through with it, I don't know what would be better, if to 
push it now for ffmpeg 4.4 (Which should be branched out before the 
bump), or after the bump.
The former would give a lot more time for downstreams to adapt, since it 
will be in a release for the majority of the deprecation period rather 
than exclusively on the master branch until the first post-bump release.

There's also a lot of stack usage for AVPacket within libav*, so we will 
have to either remove them all alongside the deprecation, or at least 
silence the av_init_packet() warnings while we slowly go through all of 
them.
From 75d82c56aca9d1905865e0b1e40258ec0da61e6b Mon Sep 17 00:00:00 2001
From: James Almer <jamrial@gmail.com>
Date: Wed, 27 Jan 2021 16:24:10 -0300
Subject: [PATCH] avcodec/packet: deprecate av_init_packet()

Once removed, sizeof(AVPacket) will stop being a part of the public ABI.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/avpacket.c | 34 ++++++++++++++++++++++++++--------
 libavcodec/packet.h   | 19 +++++++++++++++----
 libavcodec/version.h  |  3 +++
 3 files changed, 44 insertions(+), 12 deletions(-)

diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
index e4ba403cf6..e6cae39c81 100644
--- a/libavcodec/avpacket.c
+++ b/libavcodec/avpacket.c
@@ -32,6 +32,7 @@
 #include "packet.h"
 #include "packet_internal.h"
 
+#if FF_API_INIT_PACKET
 void av_init_packet(AVPacket *pkt)
 {
     pkt->pts                  = AV_NOPTS_VALUE;
@@ -49,6 +50,27 @@ FF_ENABLE_DEPRECATION_WARNINGS
     pkt->side_data            = NULL;
     pkt->side_data_elems      = 0;
 }
+#endif
+
+static void packet_init(AVPacket *pkt)
+{
+    pkt->pts             = AV_NOPTS_VALUE;
+    pkt->dts             = AV_NOPTS_VALUE;
+    pkt->pos             = -1;
+    pkt->duration        = 0;
+#if FF_API_CONVERGENCE_DURATION
+FF_DISABLE_DEPRECATION_WARNINGS
+    pkt->convergence_duration = 0;
+FF_ENABLE_DEPRECATION_WARNINGS
+#endif
+    pkt->flags           = 0;
+    pkt->stream_index    = 0;
+    pkt->buf             = NULL;
+    pkt->data            = NULL;
+    pkt->side_data       = NULL;
+    pkt->side_data_elems = 0;
+    pkt->size            = 0;
+}
 
 AVPacket *av_packet_alloc(void)
 {
@@ -56,7 +78,7 @@ AVPacket *av_packet_alloc(void)
     if (!pkt)
         return pkt;
 
-    av_init_packet(pkt);
+    packet_init(pkt);
 
     return pkt;
 }
@@ -92,7 +114,7 @@ int av_new_packet(AVPacket *pkt, int size)
     if (ret < 0)
         return ret;
 
-    av_init_packet(pkt);
+    packet_init(pkt);
     pkt->buf      = buf;
     pkt->data     = buf->data;
     pkt->size     = size;
@@ -607,9 +629,7 @@ void av_packet_unref(AVPacket *pkt)
 {
     av_packet_free_side_data(pkt);
     av_buffer_unref(&pkt->buf);
-    av_init_packet(pkt);
-    pkt->data = NULL;
-    pkt->size = 0;
+    packet_init(pkt);
 }
 
 int av_packet_ref(AVPacket *dst, const AVPacket *src)
@@ -664,9 +684,7 @@ AVPacket *av_packet_clone(const AVPacket *src)
 void av_packet_move_ref(AVPacket *dst, AVPacket *src)
 {
     *dst = *src;
-    av_init_packet(src);
-    src->data = NULL;
-    src->size = 0;
+    packet_init(src);
 }
 
 int av_packet_make_refcounted(AVPacket *pkt)
diff --git a/libavcodec/packet.h b/libavcodec/packet.h
index b9d4c9c2c8..d7f34d109e 100644
--- a/libavcodec/packet.h
+++ b/libavcodec/packet.h
@@ -319,10 +319,6 @@ typedef struct AVPacketSideData {
  * packets, with no compressed data, containing only side data
  * (e.g. to update some stream parameters at the end of encoding).
  *
- * AVPacket is one of the few structs in FFmpeg, whose size is a part of public
- * ABI. Thus it may be allocated on stack and no new fields can be added to it
- * without libavcodec and libavformat major bump.
- *
  * The semantics of data ownership depends on the buf field.
  * If it is set, the packet data is dynamically allocated and is
  * valid indefinitely until a call to av_packet_unref() reduces the
@@ -334,6 +330,12 @@ typedef struct AVPacketSideData {
  * The side data is always allocated with av_malloc(), copied by
  * av_packet_ref() and freed by av_packet_unref().
  *
+ * sizeof(AVPacket) being a part of the public ABI is deprecated. once
+ * av_init_packet() is removed, new packets will only be able to be allocated
+ * with av_packet_alloc(), and new fields may be added to the end of the struct
+ * with a minor bump.
+ *
+ * @see av_packet_alloc
  * @see av_packet_ref
  * @see av_packet_unref
  */
@@ -460,6 +462,7 @@ AVPacket *av_packet_clone(const AVPacket *src);
  */
 void av_packet_free(AVPacket **pkt);
 
+#if FF_API_INIT_PACKET
 /**
  * Initialize optional fields of a packet with default values.
  *
@@ -467,8 +470,16 @@ void av_packet_free(AVPacket **pkt);
  * initialized separately.
  *
  * @param pkt packet
+ *
+ * @see av_packet_alloc
+ * @see av_packet_unref
+ *
+ * @deprecated This function is deprecated. Once it's removed,
+               sizeof(AVPacket) will not be a part of the ABI anymore.
  */
+attribute_deprecated
 void av_init_packet(AVPacket *pkt);
+#endif
 
 /**
  * Allocate the payload of a packet and initialize its fields with
diff --git a/libavcodec/version.h b/libavcodec/version.h
index 1e1bedfce6..af8487a4c1 100644
--- a/libavcodec/version.h
+++ b/libavcodec/version.h
@@ -153,5 +153,8 @@
 #ifndef FF_API_DEBUG_MV
 #define FF_API_DEBUG_MV          (LIBAVCODEC_VERSION_MAJOR < 60)
 #endif
+#ifndef FF_API_INIT_PACKET
+#define FF_API_INIT_PACKET         (LIBAVCODEC_VERSION_MAJOR < 60)
+#endif
 
 #endif /* AVCODEC_VERSION_H */
diff mbox series

Patch

diff --git a/doc/APIchanges b/doc/APIchanges
index bbf56a5385..d239ef7627 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -15,6 +15,9 @@  libavutil:     2017-10-21
 
 API changes, most recent first:
 
+2021-xx-xx - xxxxxxxxxx - lavc 59.100.100 - avpacket.h
+  Add AVPacket.opaque, AVPacket.opaque_ref and AVPacket.time_base
+
 2021-01-11 - xxxxxxxxxx - lavc 58.116.100 - avcodec.h
   Add FF_PROFILE_VVC_MAIN_10 and FF_PROFILE_VVC_MAIN_10_444.
 
diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
index e4ba403cf6..cf2c5efb66 100644
--- a/libavcodec/avpacket.c
+++ b/libavcodec/avpacket.c
@@ -585,6 +585,12 @@  FF_ENABLE_DEPRECATION_WARNINGS
     dst->flags                = src->flags;
     dst->stream_index         = src->stream_index;
 
+#if LIBAVCODEC_VERSION_MAJOR > 58
+    i = av_buffer_replace(&dst->opaque_ref, src->opaque_ref);
+    if (i < 0)
+        return i;
+#endif
+
     dst->side_data            = NULL;
     dst->side_data_elems      = 0;
     for (i = 0; i < src->side_data_elems; i++) {
@@ -606,6 +612,9 @@  FF_ENABLE_DEPRECATION_WARNINGS
 void av_packet_unref(AVPacket *pkt)
 {
     av_packet_free_side_data(pkt);
+#if LIBAVCODEC_VERSION_MAJOR > 58
+    av_buffer_unref(&pkt->opaque_ref);
+#endif
     av_buffer_unref(&pkt->buf);
     av_init_packet(pkt);
     pkt->data = NULL;
diff --git a/libavcodec/packet.h b/libavcodec/packet.h
index b9d4c9c2c8..8e3f95e6ae 100644
--- a/libavcodec/packet.h
+++ b/libavcodec/packet.h
@@ -391,6 +391,29 @@  typedef struct AVPacket {
     attribute_deprecated
     int64_t convergence_duration;
 #endif
+
+#if LIBAVCODEC_VERSION_MAJOR > 58
+    /**
+     * for some private data of the user
+     */
+    void *opaque;
+
+    /**
+     * AVBufferRef for free use by the API user. FFmpeg will never check the
+     * contents of the buffer ref. FFmpeg calls av_buffer_unref() on it when
+     * the packet is unreferenced. av_packet_copy_props() calls create a new
+     * reference with av_buffer_ref() for the target packet's opaque_ref field.
+     *
+     * This is unrelated to the opaque field, although it serves a similar
+     * purpose.
+     */
+    AVBufferRef *opaque_ref;
+
+    /**
+     * Time base of the packet's timestamps.
+     */
+    AVRational time_base;
+#endif
 } AVPacket;
 
 typedef struct AVPacketList {