| Message ID | MRkdyHg--3-2@lynne.ee |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avpacket: RFC for ABI bump additions | expand |
| 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 |
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
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
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.
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".
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.
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
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.
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.
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?
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.
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.
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.
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).
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.
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.
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.
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 --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 {