| Message ID | 20210305163339.63164-2-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | deprecate av_init_packet() and sizeof(AVPacket) as part of the ABI | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make_warn | warning | New warnings during build |
| 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 |
Quoting James Almer (2021-03-05 17:32:52) > diff --git a/libavformat/avformat.h b/libavformat/avformat.h > index 7da2f3d98e..783cc1b591 100644 > --- a/libavformat/avformat.h > +++ b/libavformat/avformat.h > @@ -954,7 +954,11 @@ typedef struct AVStream { > * decoding: set by libavformat, must not be modified by the caller. > * encoding: unused > */ > +#if FF_API_INIT_PACKET > AVPacket attached_pic; > +#else > + AVPacket *attached_pic; > +#endif Sorry I'm late to the party, but as we are changing the type of an existing field, we need to explicitly spell out a way for the callers to make their code forward compatible. E.g. similarly to what I made for thread_safe_callbacks deprecation.
On 3/21/2021 9:28 AM, Anton Khirnov wrote: > Quoting James Almer (2021-03-05 17:32:52) >> diff --git a/libavformat/avformat.h b/libavformat/avformat.h >> index 7da2f3d98e..783cc1b591 100644 >> --- a/libavformat/avformat.h >> +++ b/libavformat/avformat.h >> @@ -954,7 +954,11 @@ typedef struct AVStream { >> * decoding: set by libavformat, must not be modified by the caller. >> * encoding: unused >> */ >> +#if FF_API_INIT_PACKET >> AVPacket attached_pic; >> +#else >> + AVPacket *attached_pic; >> +#endif > > Sorry I'm late to the party, but as we are changing the type of an > existing field, we need to explicitly spell out a way for the callers to > make their code forward compatible. E.g. similarly to what I made for > thread_safe_callbacks deprecation. What do you suggest? The field is not printing deprecation warnings, so would a doxy comment be enough for people to notice it? Have we done a similar change before? I know at least for symbols like public functions we never change their signature in an incompatible way, and instead replace them altogether. Maybe we could add the deprecated attribute to attached_pic, introduce a temporary getter function that returns a pointer to the packet, mention in the doxy that the field is not going away, is just changing types, and they have the option of using the getter for a fire-and-forget migration process, then maybe deprecate and eventually remove the getter after the switch is done.
Quoting James Almer (2021-03-21 14:54:22) > On 3/21/2021 9:28 AM, Anton Khirnov wrote: > > Quoting James Almer (2021-03-05 17:32:52) > >> diff --git a/libavformat/avformat.h b/libavformat/avformat.h > >> index 7da2f3d98e..783cc1b591 100644 > >> --- a/libavformat/avformat.h > >> +++ b/libavformat/avformat.h > >> @@ -954,7 +954,11 @@ typedef struct AVStream { > >> * decoding: set by libavformat, must not be modified by the caller. > >> * encoding: unused > >> */ > >> +#if FF_API_INIT_PACKET > >> AVPacket attached_pic; > >> +#else > >> + AVPacket *attached_pic; > >> +#endif > > > > Sorry I'm late to the party, but as we are changing the type of an > > existing field, we need to explicitly spell out a way for the callers to > > make their code forward compatible. E.g. similarly to what I made for > > thread_safe_callbacks deprecation. > > What do you suggest? The field is not printing deprecation warnings, so > would a doxy comment be enough for people to notice it? > Have we done a similar change before? I know at least for symbols like > public functions we never change their signature in an incompatible way, > and instead replace them altogether. > > Maybe we could add the deprecated attribute to attached_pic, introduce a > temporary getter function that returns a pointer to the packet, mention > in the doxy that the field is not going away, is just changing types, > and they have the option of using the getter for a fire-and-forget > migration process, then maybe deprecate and eventually remove the getter > after the switch is done. This seems like a lot of hoops to jump through. Wouldn't it then be better to just add a new field with a new name?
On 3/21/2021 11:16 AM, Anton Khirnov wrote: > Quoting James Almer (2021-03-21 14:54:22) >> On 3/21/2021 9:28 AM, Anton Khirnov wrote: >>> Quoting James Almer (2021-03-05 17:32:52) >>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h >>>> index 7da2f3d98e..783cc1b591 100644 >>>> --- a/libavformat/avformat.h >>>> +++ b/libavformat/avformat.h >>>> @@ -954,7 +954,11 @@ typedef struct AVStream { >>>> * decoding: set by libavformat, must not be modified by the caller. >>>> * encoding: unused >>>> */ >>>> +#if FF_API_INIT_PACKET >>>> AVPacket attached_pic; >>>> +#else >>>> + AVPacket *attached_pic; >>>> +#endif >>> >>> Sorry I'm late to the party, but as we are changing the type of an >>> existing field, we need to explicitly spell out a way for the callers to >>> make their code forward compatible. E.g. similarly to what I made for >>> thread_safe_callbacks deprecation. >> >> What do you suggest? The field is not printing deprecation warnings, so >> would a doxy comment be enough for people to notice it? >> Have we done a similar change before? I know at least for symbols like >> public functions we never change their signature in an incompatible way, >> and instead replace them altogether. >> >> Maybe we could add the deprecated attribute to attached_pic, introduce a >> temporary getter function that returns a pointer to the packet, mention >> in the doxy that the field is not going away, is just changing types, >> and they have the option of using the getter for a fire-and-forget >> migration process, then maybe deprecate and eventually remove the getter >> after the switch is done. > > This seems like a lot of hoops to jump through. Wouldn't it then be > better to just add a new field with a new name? A name change just because it's now a pointer seems overkill. Library users that want to support both lavc 59 and 60 will have to add preprocessor checks to their code anyway, so using the same name or another will not make any difference in that regard. Only a getter would prevent ifdeffery, but i agree it's also annoying, and we're about to get rid of a dozen of those in the upcoming bump. It also couldn't be added to 4.4 since that was already branched out. How about adding the deprecation attribute to prompt people to read the doxy, where we state the field is not going away, just changing types? Otherwise i don't think people will notice.
Quoting James Almer (2021-03-21 15:24:35) > On 3/21/2021 11:16 AM, Anton Khirnov wrote: > > Quoting James Almer (2021-03-21 14:54:22) > >> On 3/21/2021 9:28 AM, Anton Khirnov wrote: > >>> Quoting James Almer (2021-03-05 17:32:52) > >>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h > >>>> index 7da2f3d98e..783cc1b591 100644 > >>>> --- a/libavformat/avformat.h > >>>> +++ b/libavformat/avformat.h > >>>> @@ -954,7 +954,11 @@ typedef struct AVStream { > >>>> * decoding: set by libavformat, must not be modified by the caller. > >>>> * encoding: unused > >>>> */ > >>>> +#if FF_API_INIT_PACKET > >>>> AVPacket attached_pic; > >>>> +#else > >>>> + AVPacket *attached_pic; > >>>> +#endif > >>> > >>> Sorry I'm late to the party, but as we are changing the type of an > >>> existing field, we need to explicitly spell out a way for the callers to > >>> make their code forward compatible. E.g. similarly to what I made for > >>> thread_safe_callbacks deprecation. > >> > >> What do you suggest? The field is not printing deprecation warnings, so > >> would a doxy comment be enough for people to notice it? > >> Have we done a similar change before? I know at least for symbols like > >> public functions we never change their signature in an incompatible way, > >> and instead replace them altogether. > >> > >> Maybe we could add the deprecated attribute to attached_pic, introduce a > >> temporary getter function that returns a pointer to the packet, mention > >> in the doxy that the field is not going away, is just changing types, > >> and they have the option of using the getter for a fire-and-forget > >> migration process, then maybe deprecate and eventually remove the getter > >> after the switch is done. > > > > This seems like a lot of hoops to jump through. Wouldn't it then be > > better to just add a new field with a new name? > > A name change just because it's now a pointer seems overkill. Library > users that want to support both lavc 59 and 60 will have to add > preprocessor checks to their code anyway, so using the same name or > another will not make any difference in that regard. Only a getter would > prevent ifdeffery, but i agree it's also annoying, and we're about to > get rid of a dozen of those in the upcoming bump. It also couldn't be > added to 4.4 since that was already branched out. > > How about adding the deprecation attribute to prompt people to read the > doxy, where we state the field is not going away, just changing types? > Otherwise i don't think people will notice. That is also an acceptable solution.
On Sun, 21 Mar 2021, James Almer wrote: > On 3/21/2021 11:16 AM, Anton Khirnov wrote: >> Quoting James Almer (2021-03-21 14:54:22) >>> On 3/21/2021 9:28 AM, Anton Khirnov wrote: >>>> Quoting James Almer (2021-03-05 17:32:52) >>>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h >>>>> index 7da2f3d98e..783cc1b591 100644 >>>>> --- a/libavformat/avformat.h >>>>> +++ b/libavformat/avformat.h >>>>> @@ -954,7 +954,11 @@ typedef struct AVStream { >>>>> * decoding: set by libavformat, must not be modified by the > caller. >>>>> * encoding: unused >>>>> */ >>>>> +#if FF_API_INIT_PACKET >>>>> AVPacket attached_pic; >>>>> +#else >>>>> + AVPacket *attached_pic; >>>>> +#endif >>>> >>>> Sorry I'm late to the party, but as we are changing the type of an >>>> existing field, we need to explicitly spell out a way for the callers to >>>> make their code forward compatible. E.g. similarly to what I made for >>>> thread_safe_callbacks deprecation. >>> >>> What do you suggest? The field is not printing deprecation warnings, so >>> would a doxy comment be enough for people to notice it? >>> Have we done a similar change before? I know at least for symbols like >>> public functions we never change their signature in an incompatible way, >>> and instead replace them altogether. >>> >>> Maybe we could add the deprecated attribute to attached_pic, introduce a >>> temporary getter function that returns a pointer to the packet, mention >>> in the doxy that the field is not going away, is just changing types, >>> and they have the option of using the getter for a fire-and-forget >>> migration process, then maybe deprecate and eventually remove the getter >>> after the switch is done. >> >> This seems like a lot of hoops to jump through. Wouldn't it then be >> better to just add a new field with a new name? > > A name change just because it's now a pointer seems overkill. I don't think it is. A name change guarantees that existing code won't compile to something that will surely crash. IMHO the clean solution is to keep the old field with deprecation, and add a new field. Same what I did with AVFormat->filename / AVFormat->url. Regards, Marton
Marton Balint (12021-03-21):
> Same what I did with AVFormat->filename / AVFormat->url.
By the way, about that: at some point, we will need to clarify what part
of our API uses real URLs and what parts uses fake URLs.
Because as it is, a lot of code looks like it uses URLs but really it
does not. The worst offender is "file:" it looks like an URL, the prefix
is a 100% standard URL scheme. Yet, it will not find
file:///home/user/Some%20Movie.mkv because it does not obey to the
standard syntax of file:// URLs.
I had the vague intent of proposing a migration path, but never time to
work on it. It involves:
1. Introduce the fs: protocol as a synonym for file:.
2. For some time, detect if the behavior is likely to change in the next
step and warn the user.
3. Detect file:// and treat it like a standard file:// URL, including
skipping the server, query and fragment part and performing
%-unescaping; keep file: without the double slash as an alias for
fs:.
4. For some more time, warn users if they are using file: but not
file://.
5. Remove the compatibility file: → fs: alias.
Regards,
On 3/21/2021 3:15 PM, Marton Balint wrote: > > > On Sun, 21 Mar 2021, James Almer wrote: > >> On 3/21/2021 11:16 AM, Anton Khirnov wrote: >>> Quoting James Almer (2021-03-21 14:54:22) >>>> On 3/21/2021 9:28 AM, Anton Khirnov wrote: >>>>> Quoting James Almer (2021-03-05 17:32:52) >>>>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h >>>>>> index 7da2f3d98e..783cc1b591 100644 >>>>>> --- a/libavformat/avformat.h >>>>>> +++ b/libavformat/avformat.h >>>>>> @@ -954,7 +954,11 @@ typedef struct AVStream { >>>>>> * decoding: set by libavformat, must not be modified by the >> caller. >>>>>> * encoding: unused >>>>>> */ >>>>>> +#if FF_API_INIT_PACKET >>>>>> AVPacket attached_pic; >>>>>> +#else >>>>>> + AVPacket *attached_pic; >>>>>> +#endif >>>>> >>>>> Sorry I'm late to the party, but as we are changing the type of an >>>>> existing field, we need to explicitly spell out a way for the >>>>> callers to >>>>> make their code forward compatible. E.g. similarly to what I made for >>>>> thread_safe_callbacks deprecation. >>>> >>>> What do you suggest? The field is not printing deprecation warnings, so >>>> would a doxy comment be enough for people to notice it? >>>> Have we done a similar change before? I know at least for symbols like >>>> public functions we never change their signature in an incompatible >>>> way, >>>> and instead replace them altogether. >>>> >>>> Maybe we could add the deprecated attribute to attached_pic, >>>> introduce a >>>> temporary getter function that returns a pointer to the packet, mention >>>> in the doxy that the field is not going away, is just changing types, >>>> and they have the option of using the getter for a fire-and-forget >>>> migration process, then maybe deprecate and eventually remove the >>>> getter >>>> after the switch is done. >>> >>> This seems like a lot of hoops to jump through. Wouldn't it then be >>> better to just add a new field with a new name? >> >> A name change just because it's now a pointer seems overkill. > > I don't think it is. A name change guarantees that existing code won't > compile to something that will surely crash. IMHO the clean solution is > to keep the old field with deprecation, and add a new field. Same what I > did with AVFormat->filename / AVFormat->url. Compilation will fail if your code doesn't treat the field as a pointer after the switch, printing something like error: 'st->attached_pic' is a pointer; did you mean to use '->'? 123 | st->attached_pic.flags |= AV_PKT_FLAG_KEY; | ^ So it's not like it will compile and then crash at runtime by accessing the wrong thing. But if you still feel more inclined to replace the field, can you suggest a name for the new one?
Nicolas George (12021-03-21): > By the way, about that: at some point, we will need to clarify what part > of our API uses real URLs and what parts uses fake URLs. > > Because as it is, a lot of code looks like it uses URLs but really it > does not. The worst offender is "file:" it looks like an URL, the prefix > is a 100% standard URL scheme. Yet, it will not find > file:///home/user/Some%20Movie.mkv because it does not obey to the > standard syntax of file:// URLs. This is now Trac ticket #9157: https://trac.ffmpeg.org/ticket/9157 Note that it is a rather simple task for somebody who wants to get involved in the project. Regards,
On 3/21/2021 3:37 PM, James Almer wrote: > On 3/21/2021 3:15 PM, Marton Balint wrote: >> >> >> On Sun, 21 Mar 2021, James Almer wrote: >> >>> On 3/21/2021 11:16 AM, Anton Khirnov wrote: >>>> Quoting James Almer (2021-03-21 14:54:22) >>>>> On 3/21/2021 9:28 AM, Anton Khirnov wrote: >>>>>> Quoting James Almer (2021-03-05 17:32:52) >>>>>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h >>>>>>> index 7da2f3d98e..783cc1b591 100644 >>>>>>> --- a/libavformat/avformat.h >>>>>>> +++ b/libavformat/avformat.h >>>>>>> @@ -954,7 +954,11 @@ typedef struct AVStream { >>>>>>> * decoding: set by libavformat, must not be modified by the >>> caller. >>>>>>> * encoding: unused >>>>>>> */ >>>>>>> +#if FF_API_INIT_PACKET >>>>>>> AVPacket attached_pic; >>>>>>> +#else >>>>>>> + AVPacket *attached_pic; >>>>>>> +#endif >>>>>> >>>>>> Sorry I'm late to the party, but as we are changing the type of an >>>>>> existing field, we need to explicitly spell out a way for the >>>>>> callers to >>>>>> make their code forward compatible. E.g. similarly to what I made for >>>>>> thread_safe_callbacks deprecation. >>>>> >>>>> What do you suggest? The field is not printing deprecation >>>>> warnings, so >>>>> would a doxy comment be enough for people to notice it? >>>>> Have we done a similar change before? I know at least for symbols like >>>>> public functions we never change their signature in an incompatible >>>>> way, >>>>> and instead replace them altogether. >>>>> >>>>> Maybe we could add the deprecated attribute to attached_pic, >>>>> introduce a >>>>> temporary getter function that returns a pointer to the packet, >>>>> mention >>>>> in the doxy that the field is not going away, is just changing types, >>>>> and they have the option of using the getter for a fire-and-forget >>>>> migration process, then maybe deprecate and eventually remove the >>>>> getter >>>>> after the switch is done. >>>> >>>> This seems like a lot of hoops to jump through. Wouldn't it then be >>>> better to just add a new field with a new name? >>> >>> A name change just because it's now a pointer seems overkill. >> >> I don't think it is. A name change guarantees that existing code won't >> compile to something that will surely crash. IMHO the clean solution >> is to keep the old field with deprecation, and add a new field. Same >> what I did with AVFormat->filename / AVFormat->url. > > Compilation will fail if your code doesn't treat the field as a pointer > after the switch, printing something like > > error: 'st->attached_pic' is a pointer; did you mean to use '->'? > 123 | st->attached_pic.flags |= AV_PKT_FLAG_KEY; > | ^ > > So it's not like it will compile and then crash at runtime by accessing > the wrong thing. > > But if you still feel more inclined to replace the field, can you > suggest a name for the new one? Also, i wont be able to add a new field until after the bump because of the libavdevice franken-ABI situation.
On 3/21/2021 3:37 PM, James Almer wrote: > On 3/21/2021 3:15 PM, Marton Balint wrote: >> >> >> On Sun, 21 Mar 2021, James Almer wrote: >> >>> On 3/21/2021 11:16 AM, Anton Khirnov wrote: >>>> Quoting James Almer (2021-03-21 14:54:22) >>>>> On 3/21/2021 9:28 AM, Anton Khirnov wrote: >>>>>> Quoting James Almer (2021-03-05 17:32:52) >>>>>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h >>>>>>> index 7da2f3d98e..783cc1b591 100644 >>>>>>> --- a/libavformat/avformat.h >>>>>>> +++ b/libavformat/avformat.h >>>>>>> @@ -954,7 +954,11 @@ typedef struct AVStream { >>>>>>> * decoding: set by libavformat, must not be modified by the >>> caller. >>>>>>> * encoding: unused >>>>>>> */ >>>>>>> +#if FF_API_INIT_PACKET >>>>>>> AVPacket attached_pic; >>>>>>> +#else >>>>>>> + AVPacket *attached_pic; >>>>>>> +#endif >>>>>> >>>>>> Sorry I'm late to the party, but as we are changing the type of an >>>>>> existing field, we need to explicitly spell out a way for the >>>>>> callers to >>>>>> make their code forward compatible. E.g. similarly to what I made for >>>>>> thread_safe_callbacks deprecation. >>>>> >>>>> What do you suggest? The field is not printing deprecation >>>>> warnings, so >>>>> would a doxy comment be enough for people to notice it? >>>>> Have we done a similar change before? I know at least for symbols like >>>>> public functions we never change their signature in an incompatible >>>>> way, >>>>> and instead replace them altogether. >>>>> >>>>> Maybe we could add the deprecated attribute to attached_pic, >>>>> introduce a >>>>> temporary getter function that returns a pointer to the packet, >>>>> mention >>>>> in the doxy that the field is not going away, is just changing types, >>>>> and they have the option of using the getter for a fire-and-forget >>>>> migration process, then maybe deprecate and eventually remove the >>>>> getter >>>>> after the switch is done. >>>> >>>> This seems like a lot of hoops to jump through. Wouldn't it then be >>>> better to just add a new field with a new name? >>> >>> A name change just because it's now a pointer seems overkill. >> >> I don't think it is. A name change guarantees that existing code won't >> compile to something that will surely crash. IMHO the clean solution >> is to keep the old field with deprecation, and add a new field. Same >> what I did with AVFormat->filename / AVFormat->url. > > Compilation will fail if your code doesn't treat the field as a pointer > after the switch, printing something like > > error: 'st->attached_pic' is a pointer; did you mean to use '->'? > 123 | st->attached_pic.flags |= AV_PKT_FLAG_KEY; > | ^ > > So it's not like it will compile and then crash at runtime by accessing > the wrong thing. After looking a bit more, it will fail if you try to access fields like i mentioned above, but if you pass it as argument to some function it will only warn about mismatching types. So i guess yeah, a new field with a new name would be the cleanest solution. So as i said, field name suggestions welcome. > > But if you still feel more inclined to replace the field, can you > suggest a name for the new one?
On Sun, 21 Mar 2021, James Almer wrote: > On 3/21/2021 3:37 PM, James Almer wrote: >> On 3/21/2021 3:15 PM, Marton Balint wrote: >>> >>> >>> On Sun, 21 Mar 2021, James Almer wrote: >>> >>>> On 3/21/2021 11:16 AM, Anton Khirnov wrote: >>>>> Quoting James Almer (2021-03-21 14:54:22) >>>>>> On 3/21/2021 9:28 AM, Anton Khirnov wrote: >>>>>>> Quoting James Almer (2021-03-05 17:32:52) >>>>>>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h >>>>>>>> index 7da2f3d98e..783cc1b591 100644 >>>>>>>> --- a/libavformat/avformat.h >>>>>>>> +++ b/libavformat/avformat.h >>>>>>>> @@ -954,7 +954,11 @@ typedef struct AVStream { >>>>>>>> * decoding: set by libavformat, must not be modified by the >>>> caller. >>>>>>>> * encoding: unused >>>>>>>> */ >>>>>>>> +#if FF_API_INIT_PACKET >>>>>>>> AVPacket attached_pic; >>>>>>>> +#else >>>>>>>> + AVPacket *attached_pic; >>>>>>>> +#endif >>>>>>> >>>>>>> Sorry I'm late to the party, but as we are changing the type of an >>>>>>> existing field, we need to explicitly spell out a way for the >>>>>>> callers to >>>>>>> make their code forward compatible. E.g. similarly to what I made for >>>>>>> thread_safe_callbacks deprecation. >>>>>> >>>>>> What do you suggest? The field is not printing deprecation >>>>>> warnings, so >>>>>> would a doxy comment be enough for people to notice it? >>>>>> Have we done a similar change before? I know at least for symbols like >>>>>> public functions we never change their signature in an incompatible >>>>>> way, >>>>>> and instead replace them altogether. >>>>>> >>>>>> Maybe we could add the deprecated attribute to attached_pic, >>>>>> introduce a >>>>>> temporary getter function that returns a pointer to the packet, >>>>>> mention >>>>>> in the doxy that the field is not going away, is just changing types, >>>>>> and they have the option of using the getter for a fire-and-forget >>>>>> migration process, then maybe deprecate and eventually remove the >>>>>> getter >>>>>> after the switch is done. >>>>> >>>>> This seems like a lot of hoops to jump through. Wouldn't it then be >>>>> better to just add a new field with a new name? >>>> >>>> A name change just because it's now a pointer seems overkill. >>> >>> I don't think it is. A name change guarantees that existing code won't >>> compile to something that will surely crash. IMHO the clean solution >>> is to keep the old field with deprecation, and add a new field. Same >>> what I did with AVFormat->filename / AVFormat->url. >> >> Compilation will fail if your code doesn't treat the field as a pointer >> after the switch, printing something like >> >> error: 'st->attached_pic' is a pointer; did you mean to use '->'? >> 123 | st->attached_pic.flags |= AV_PKT_FLAG_KEY; >> | ^ >> >> So it's not like it will compile and then crash at runtime by accessing >> the wrong thing. > > After looking a bit more, it will fail if you try to access fields like > i mentioned above, but if you pass it as argument to some function it > will only warn about mismatching types. > > So i guess yeah, a new field with a new name would be the cleanest > solution. So as i said, field name suggestions welcome. attached_pic_pkt or attached_pkt maybe. Regards, Marton
On Sun, Mar 21, 2021 at 3:24 PM James Almer <jamrial@gmail.com> wrote: > > How about adding the deprecation attribute to prompt people to read the > doxy, where we state the field is not going away, just changing types? > Otherwise i don't think people will notice. I really don't like such solutions. You get warning spam in user code with basically no way to remedy them. - Hendrik
On 3/21/2021 8:13 PM, Hendrik Leppkes wrote: > On Sun, Mar 21, 2021 at 3:24 PM James Almer <jamrial@gmail.com> wrote: >> >> How about adding the deprecation attribute to prompt people to read the >> doxy, where we state the field is not going away, just changing types? >> Otherwise i don't think people will notice. > > I really don't like such solutions. You get warning spam in user code > with basically no way to remedy them. > > - Hendrik I'm going to add a new field in the end. A change of type can't ensure projects that don't migrate stop compiling (Like it happens when a field is removed), and instead will unexpectedly crash if they try to access it the wrong way. But i can't do it until after the bump because libavdevice is locking every AVStream field offset.
diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c index e4ba403cf6..ae0cbfb9f9 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,16 @@ FF_ENABLE_DEPRECATION_WARNINGS pkt->side_data = NULL; pkt->side_data_elems = 0; } +#endif + +static void get_packet_defaults(AVPacket *pkt) +{ + memset(pkt, 0, sizeof(*pkt)); + + pkt->pts = AV_NOPTS_VALUE; + pkt->dts = AV_NOPTS_VALUE; + pkt->pos = -1; +} AVPacket *av_packet_alloc(void) { @@ -56,7 +67,7 @@ AVPacket *av_packet_alloc(void) if (!pkt) return pkt; - av_init_packet(pkt); + get_packet_defaults(pkt); return pkt; } @@ -92,7 +103,7 @@ int av_new_packet(AVPacket *pkt, int size) if (ret < 0) return ret; - av_init_packet(pkt); + get_packet_defaults(pkt); pkt->buf = buf; pkt->data = buf->data; pkt->size = size; @@ -607,9 +618,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; + get_packet_defaults(pkt); } int av_packet_ref(AVPacket *dst, const AVPacket *src) @@ -664,9 +673,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; + get_packet_defaults(src); } int av_packet_make_refcounted(AVPacket *pkt) diff --git a/libavcodec/packet.h b/libavcodec/packet.h index b9d4c9c2c8..f07ca4936c 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 */ @@ -393,10 +395,13 @@ typedef struct AVPacket { #endif } AVPacket; +#if FF_API_INIT_PACKET +attribute_deprecated typedef struct AVPacketList { AVPacket pkt; struct AVPacketList *next; } AVPacketList; +#endif #define AV_PKT_FLAG_KEY 0x0001 ///< The packet contains a keyframe #define AV_PKT_FLAG_CORRUPT 0x0002 ///< The packet content is corrupted @@ -460,6 +465,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 +473,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 2b3757fa07..163dbc1cbf 100644 --- a/libavcodec/version.h +++ b/libavcodec/version.h @@ -28,7 +28,7 @@ #include "libavutil/version.h" #define LIBAVCODEC_VERSION_MAJOR 58 -#define LIBAVCODEC_VERSION_MINOR 128 +#define LIBAVCODEC_VERSION_MINOR 129 #define LIBAVCODEC_VERSION_MICRO 100 #define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \ @@ -159,5 +159,8 @@ #ifndef FF_API_GET_FRAME_CLASS #define FF_API_GET_FRAME_CLASS (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/libavformat/avformat.h b/libavformat/avformat.h index 7da2f3d98e..783cc1b591 100644 --- a/libavformat/avformat.h +++ b/libavformat/avformat.h @@ -954,7 +954,11 @@ typedef struct AVStream { * decoding: set by libavformat, must not be modified by the caller. * encoding: unused */ +#if FF_API_INIT_PACKET AVPacket attached_pic; +#else + AVPacket *attached_pic; +#endif /** * An array of side data that applies to the whole stream (i.e. the
Once removed, sizeof(AVPacket) will stop being a part of the public ABI. Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/avpacket.c | 23 +++++++++++++++-------- libavcodec/packet.h | 22 ++++++++++++++++++---- libavcodec/version.h | 5 ++++- libavformat/avformat.h | 4 ++++ 4 files changed, 41 insertions(+), 13 deletions(-)