| Message ID | 20230130122937.12258-1-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] lavfi: get rid of FF_INTERNAL_FIELDS | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On 1/30/23, Anton Khirnov <anton@khirnov.net> wrote: > This hack is used to limit the visibility of some AVFilterLink fields to > only certain files. Replace it with the same pattern that is used e.g. > in lavf AVStream/FFstream and avoid exposing these internal fields in a > public header completely. Looks fine on quick look, but wait for Nicolas comment.
Anton Khirnov (12023-01-30): > This hack is used to limit the visibility of some AVFilterLink fields to > only certain files. Replace it with the same pattern that is used e.g. > in lavf AVStream/FFstream and avoid exposing these internal fields in a > public header completely. > --- > libavfilter/avfilter.c | 191 +++++++++++++++++++++-------------- > libavfilter/avfilter.h | 45 --------- > libavfilter/avfiltergraph.c | 9 +- > libavfilter/buffersink.c | 8 +- > libavfilter/link_internal.h | 69 +++++++++++++ > libavfilter/tests/filtfmts.c | 9 +- > 6 files changed, 196 insertions(+), 135 deletions(-) > create mode 100644 libavfilter/link_internal.h This makes the code more verbose, less readable and harder to maintain, so no thanks. If you find a solution that does not require us to remember which field is private and with field is public to prefix them with link-> or li->, it would not have this issue; avoiding this requirement was a prime goal of the current implementation. At least you did not add an indirection like on some other places.
Quoting Nicolas George (2023-01-30 13:40:06) > Anton Khirnov (12023-01-30): > > This hack is used to limit the visibility of some AVFilterLink fields to > > only certain files. Replace it with the same pattern that is used e.g. > > in lavf AVStream/FFstream and avoid exposing these internal fields in a > > public header completely. > > --- > > libavfilter/avfilter.c | 191 +++++++++++++++++++++-------------- > > libavfilter/avfilter.h | 45 --------- > > libavfilter/avfiltergraph.c | 9 +- > > libavfilter/buffersink.c | 8 +- > > libavfilter/link_internal.h | 69 +++++++++++++ > > libavfilter/tests/filtfmts.c | 9 +- > > 6 files changed, 196 insertions(+), 135 deletions(-) > > create mode 100644 libavfilter/link_internal.h > > This makes the code more verbose, less readable and harder to maintain, I find this a significant improvement in code quality, making it easier to maintain. > so no thanks. > > If you find a solution that does not require us to remember which field > is private and with field is public to prefix them with link-> or li->, > it would not have this issue; avoiding this requirement was a prime goal > of the current implementation. At least you did not add an indirection > like on some other places. Making it obvious which field is private and which is public is a feature, not a bug. I'll also note that - we've been switching to this precise pattern everywhere else in the project - the most prolific lavfi contributor recently (Paul) has no objections to this patch - the second most prolific lavfi contributor recently (Andreas) told me on IRC he planned to write this same patch himself So if you insist on objecting to this patch, it seems to me that a vote would be in order.
Anton Khirnov (12023-01-31): > I find this a significant improvement in code quality, making it easier > to maintain. You can say that, you do not maintain it. > Making it obvious which field is private and which is public is a > feature, not a bug. Then I do not want this feature. Making people expend effort for no reason at all. > I'll also note that > - we've been switching to this precise pattern everywhere else in the > project Well, too bad. > - the most prolific lavfi contributor recently (Paul) has no objections > to this patch > - the second most prolific lavfi contributor recently (Andreas) told me > on IRC he planned to write this same patch himself It is not a matter of volume, it is a matter of complexity. Since Stefano is no longer involved in the coding, I am the only one who knows how the tricky bits of libavfilter work. > So if you insist on objecting to this patch, it seems to me that a vote > would be in order. Well, go ahead, but please be aware that a vote cannot force me to maintain that code. If this project goes over my objections “that patch makes my maintenance work harder”, then be very careful to involve in your plans “find somebody else willing to debug the code of libavfilter”. Good luck. In fact, I think I will take it very easy with maintenance duty until further notice. I am really fed up with the direction this project is taking. Authoritarianism, that was the other side of the fork, and we all know what happened there. And who is to blame.
On 1/31/23, Nicolas George <george@nsup.org> wrote: > Anton Khirnov (12023-01-31): >> I find this a significant improvement in code quality, making it easier >> to maintain. > > You can say that, you do not maintain it. > >> Making it obvious which field is private and which is public is a >> feature, not a bug. > > Then I do not want this feature. Making people expend effort for no > reason at all. > >> I'll also note that >> - we've been switching to this precise pattern everywhere else in the >> project > > Well, too bad. > >> - the most prolific lavfi contributor recently (Paul) has no objections >> to this patch >> - the second most prolific lavfi contributor recently (Andreas) told me >> on IRC he planned to write this same patch himself > > It is not a matter of volume, it is a matter of complexity. Since > Stefano is no longer involved in the coding, I am the only one who knows > how the tricky bits of libavfilter work. No, you do not. Calling your libavfilter framework code "complex" is disgrace to really complex code in non-framework part of libavfilter or else in FFmpeg libraries. libavfilter framework needs serious overhaul. It have numerous limitations and other stuff too much exposed to user, that really should not be. You do not maintain code at all, you just block patches and never contribute anymore anything useful besides blocking patches. > >> So if you insist on objecting to this patch, it seems to me that a vote >> would be in order. > > Well, go ahead, but please be aware that a vote cannot force me to > maintain that code. > > If this project goes over my objections “that patch makes my maintenance > work harder”, then be very careful to involve in your plans “find > somebody else willing to debug the code of libavfilter”. Good luck. > > In fact, I think I will take it very easy with maintenance duty until > further notice. I am really fed up with the direction this project is > taking. Authoritarianism, that was the other side of the fork, and we > all know what happened there. And who is to blame. > > -- > Nicolas George > _______________________________________________ > 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". >
Quoting Nicolas George (2023-01-31 15:03:34) > Anton Khirnov (12023-01-31): > > I find this a significant improvement in code quality, making it easier > > to maintain. > > You can say that, you do not maintain it. Do you? You keep implying in your emails that you are the authority on lavfi, but I see no objective support for this claim. MAINTAINERS only lists you as the maintainer for graphdump.c and two filters. And taken e.g. by the number of commits touching libavfilter/, mine is similar to yours, and both are far behind other people. > > - the most prolific lavfi contributor recently (Paul) has no objections > > to this patch > > - the second most prolific lavfi contributor recently (Andreas) told me > > on IRC he planned to write this same patch himself > > It is not a matter of volume, it is a matter of complexity. Since > Stefano is no longer involved in the coding, I am the only one who knows > how the tricky bits of libavfilter work. Maybe someone should replace those tricky bits with something simpler then. > > So if you insist on objecting to this patch, it seems to me that a vote > > would be in order. > > Well, go ahead, but please be aware that a vote cannot force me to > maintain that code. > > If this project goes over my objections “that patch makes my maintenance > work harder”, then be very careful to involve in your plans “find > somebody else willing to debug the code of libavfilter”. Good luck. > > In fact, I think I will take it very easy with maintenance duty until > further notice. I am really fed up with the direction this project is > taking. Authoritarianism, that was the other side of the fork, and we > all know what happened there. And who is to blame. Ah yes, preventing one person from enforcing his opinion over those of multiple other people is "authoritarianism". I don't think that word means what you think it means. So take your pick - either stop opposing this patch or we have a vote over it and see what other people think.
Anton Khirnov (12023-01-31): > Do you? > > You keep implying in your emails that you are the authority on lavfi, > but I see no objective support for this claim. MAINTAINERS only lists > you as the maintainer for graphdump.c and two filters. > And taken e.g. by the number of commits touching libavfilter/, mine is > similar to yours, and both are far behind other people. I know the code, how it works. I know how the code needs to evolve to achieve new features without breaking existing. I review and apply patches to fix bugs when they come; fortunately it does not happen frequently. > Maybe someone should replace those tricky bits with something simpler > then. Yes, do that, and I will laugh. The code is tricky because the needs are tricky. “Something simpler” is precisely what came from libav and I needed to fix a decade ago. > Ah yes, preventing one person from enforcing his opinion over those of > multiple other people is "authoritarianism". I don't think that word > means what you think it means. > > So take your pick - either stop opposing this patch or we have a vote > over it and see what other people think. Go ahead, have a vote. I no longer care. If it goes against me, that will make one less thing to consider my responsibility. We should never have asked ourselves “how should we change to entice forkers back”, we should have asked “how must the forkers change if they want to be accepted back”. You did not change.
Paul B Mahol (12023-01-31): > No, you do not. Calling your libavfilter framework code "complex" is disgrace > to really complex code in non-framework part of libavfilter or else in > FFmpeg libraries. > > libavfilter framework needs serious overhaul. > It have numerous limitations and other stuff too much exposed to user, > that really should not be. > > You do not maintain code at all, you just block patches and never > contribute anymore anything useful besides blocking patches. … says the person who does not know that the duration between pts=3 and pts=5 must be 2. This is a bad joke.
On 1/31/23, Nicolas George <george@nsup.org> wrote: > Paul B Mahol (12023-01-31): >> No, you do not. Calling your libavfilter framework code "complex" is >> disgrace >> to really complex code in non-framework part of libavfilter or else in >> FFmpeg libraries. >> >> libavfilter framework needs serious overhaul. >> It have numerous limitations and other stuff too much exposed to user, >> that really should not be. >> >> You do not maintain code at all, you just block patches and never >> contribute anymore anything useful besides blocking patches. > > … says the person who does not know that the duration between pts=3 and > pts=5 must be 2. > > This is a bad joke. The patch just returned duration of frame from framesync and not actual duration of frame. And I have not tested it extensively at all or even thought about it much. Why? Because I find whole API including libavfilter very silly and broken and limited. At least I contributed numerous filters used in production, otherwise libavfilter would probably not exist today at all in this form at least. It would be already rewritten several times and/or whole idea abandoned completely because of lack of interest to develop it or poor usage.
Quoting Nicolas George (2023-01-31 15:31:44) > Anton Khirnov (12023-01-31): > > Do you? > > > > You keep implying in your emails that you are the authority on lavfi, > > but I see no objective support for this claim. MAINTAINERS only lists > > you as the maintainer for graphdump.c and two filters. > > And taken e.g. by the number of commits touching libavfilter/, mine is > > similar to yours, and both are far behind other people. > > I know the code, how it works. > > I know how the code needs to evolve to achieve new features without > breaking existing. > > I review and apply patches to fix bugs when they come; fortunately it > does not happen frequently. I still see no objective facts supporting your claim of exclusive maintainership over the entirety of lavfi generic code and public API. Considering yourself the maintainer in your own head is not enough. So to avoid any further pointless bickering, I'm hereby asking the TC to resolve this. To summarize my view, this patch is an improvement because: * it prevents filterlink internals from being visible in a public header, where they have no business being * it is a step towards hiding more of lavfi internals from public headers * the same pattern is already and ever more widely used in the other libraries and ffmpeg CLI * it is supported by Andreas (who submitted a more general analogue of this patch over a year ago) and Paul * I am not aware of anyone other than Nicolas being against it
Anton Khirnov (12023-01-31): > I still see no objective facts supporting your claim of exclusive > maintainership over the entirety of lavfi generic code and public API. The fact is very simple: I am the only one who understand how this code works. > So to avoid any further pointless bickering, I'm hereby asking the TC to > resolve this. Just so we are clear: you are a party in this, you can therefore not be a judge. > To summarize my view, this patch is an improvement because: > * it prevents filterlink internals from being visible in a > public header, where they have no business being > * it is a step towards hiding more of lavfi internals from public > headers > * the same pattern is already and ever more widely used in the other > libraries and ffmpeg CLI > * it is supported by Andreas (who submitted a more general analogue of > this patch over a year ago) and Paul > * I am not aware of anyone other than Nicolas being against it It is a worsening because: * It requires the developers to remember which field is public and which field is private, which is not something relevant here (is is relevant elsewhere). Of course, if you think about it two seconds, you realize it affects the person who knows the code very well and used/wants to work on it intensively more than the developers who move from one part of the code to another and have to re-learn everything. But thinking two seconds how your changes affects other people does not seem like your forte either. Therefore, I add this point: * If this change is applied, FFmpeg needs to find somebody else to maintain the core of libavfilter. (And I predict you will not.)
Nicolas George (12023-01-31): > > * it prevents filterlink internals from being visible in a > > public header, where they have no business being > > * it is a step towards hiding more of lavfi internals from public > > headers > > * the same pattern is already and ever more widely used in the other Note to the TC who will decide: I do not oppose the efforts mentioned in these two points (that are actually the same points twice), I only oppose this particular solution because of this drawback: > * It requires the developers to remember which field is public and which > field is private, which is not something relevant here (is is relevant > elsewhere). Without looking very far, I can think of several different ways of hiding the internal fields better without requiring changes to the implementation. I would not oppose such a change.
Nicolas George: > Nicolas George (12023-01-31): >>> * it prevents filterlink internals from being visible in a >>> public header, where they have no business being >>> * it is a step towards hiding more of lavfi internals from public >>> headers >>> * the same pattern is already and ever more widely used in the other > > Note to the TC who will decide: I do not oppose the efforts mentioned in > these two points (that are actually the same points twice), I only > oppose this particular solution because of this drawback: > >> * It requires the developers to remember which field is public and which >> field is private, which is not something relevant here (is is relevant >> elsewhere). > > Without looking very far, I can think of several different ways of > hiding the internal fields better without requiring changes to the > implementation. I would not oppose such a change. > Details please. I can only think of the following: a) Allow the use of -fms-extensions. This allows structs with tags and typedefs thereof to be used as unnamed struct members with the members of the unnamed structure being treated as members of the enclosing structure. One could then use a pointer to the big internal structure internally and one does not need to remember whether a field is internal or not. There is still a problem, though: One needs to cast from AVFilterContext.(inputs|outputs). This should be done via dedicated inlined getters, but this is a bit more typing. E.g. "input_from_ctx(ctx, i)" instead of "ctx->inputs[i]". Of course, it might also be shorter if someone has a short name. GCC, Clang, MSVC and the IIRC the intel compilers support this. b) Add a big #define AVFILTERLINK in avfilter.h that expands to the public part of AVFilterLink and change the declaration of AVFilterLink to "struct AVFilterLink { AVFILTERLINK };" and use declare the internal struct via "struct FilterLinkInternal { AVFILTERLINK /* actual internal fields */ };" This has the downside of actually being an aliasing violation and of adding considerable ugliness to avfilter.h, in particular during deprecations (like with FF_API_OLD_CHANNEL_LAYOUT -- you can't check via #if in the implementation of a macro). I also don't know whether it plays nicely with tools that deal with source code annotations. c) Wrap the internal part in an #ifdef HAVE_AV_CONFIG_H, optionally using #if defined(HAVE_AV_CONFIG_H) && defined(BUILDING_avfilter). d) Same as c), but strip this stuff from installed headers. I consider b)-d) as inferior to a), which I consider superior to Anton's proposal, but the big drawback is its reliance on a compiler extension. - Andreas
Andreas Rheinhardt (12023-01-31): > Details please. I was not going to spend time explaining unless I had assurance it was because the issue of requiring changes in the internal implementation and developers habits just to hide some fields was taken into account. Coming from somebody else, I would have expected the question to be only to find the first excuses to shoot the proposal down. But you more or less found the same ideas I did anyway. > I can only think of the following: > a) Allow the use of -fms-extensions. This allows structs with tags and > typedefs thereof to be used as unnamed struct members with the members > of the unnamed structure being treated as members of the enclosing > structure. One could then use a pointer to the big internal structure > internally and one does not need to remember whether a field is internal > or not. There is still a problem, though: One needs to cast from > AVFilterContext.(inputs|outputs). This should be done via dedicated > inlined getters, but this is a bit more typing. E.g. > "input_from_ctx(ctx, i)" instead of "ctx->inputs[i]". Of course, it > might also be shorter if someone has a short name. > GCC, Clang, MSVC and the IIRC the intel compilers support this. > > b) Add a big #define AVFILTERLINK in avfilter.h that expands to the > public part of AVFilterLink and change the declaration of AVFilterLink > to "struct AVFilterLink { AVFILTERLINK };" and use declare the internal > struct via > "struct FilterLinkInternal { > AVFILTERLINK > /* actual internal fields */ > };" > This has the downside of actually being an aliasing violation and of > adding considerable ugliness to avfilter.h, in particular during > deprecations (like with FF_API_OLD_CHANNEL_LAYOUT -- you can't check via > #if in the implementation of a macro). I also don't know whether it > plays nicely with tools that deal with source code annotations. Or use a “#include "avfilter_link_internal_fields.h" instead of a macro. > c) Wrap the internal part in an #ifdef HAVE_AV_CONFIG_H, optionally > using #if defined(HAVE_AV_CONFIG_H) && defined(BUILDING_avfilter). > d) Same as c), but strip this stuff from installed headers. > > I consider b)-d) as inferior to a), which I consider superior to Anton's > proposal, but the big drawback is its reliance on a compiler extension. Once and for all: If the solution requires changing things in the internal implementation C files and changing the habits of the people who work on these files (that goes together), then it is inferior to a solution that does not require that. Your solution (a) has this flaw, plus relies on an extension that is probably not available on all supported platforms (Microsoft I am looking at you). My favor goes to (d). Probably with a stricter test for the (c) part because the internal fields are not necessary for the filters and therefore could be hidden there too. Regards,
Nicolas George: > Andreas Rheinhardt (12023-01-31): >> Details please. > > I was not going to spend time explaining unless I had assurance it was > because the issue of requiring changes in the internal implementation > and developers habits just to hide some fields was taken into account. > Coming from somebody else, I would have expected the question to be only > to find the first excuses to shoot the proposal down. > > But you more or less found the same ideas I did anyway. > >> I can only think of the following: >> a) Allow the use of -fms-extensions. This allows structs with tags and >> typedefs thereof to be used as unnamed struct members with the members >> of the unnamed structure being treated as members of the enclosing >> structure. One could then use a pointer to the big internal structure >> internally and one does not need to remember whether a field is internal >> or not. There is still a problem, though: One needs to cast from >> AVFilterContext.(inputs|outputs). This should be done via dedicated >> inlined getters, but this is a bit more typing. E.g. >> "input_from_ctx(ctx, i)" instead of "ctx->inputs[i]". Of course, it >> might also be shorter if someone has a short name. >> GCC, Clang, MSVC and the IIRC the intel compilers support this. >> >> b) Add a big #define AVFILTERLINK in avfilter.h that expands to the >> public part of AVFilterLink and change the declaration of AVFilterLink >> to "struct AVFilterLink { AVFILTERLINK };" and use declare the internal >> struct via >> "struct FilterLinkInternal { >> AVFILTERLINK >> /* actual internal fields */ >> };" >> This has the downside of actually being an aliasing violation and of >> adding considerable ugliness to avfilter.h, in particular during >> deprecations (like with FF_API_OLD_CHANNEL_LAYOUT -- you can't check via >> #if in the implementation of a macro). I also don't know whether it >> plays nicely with tools that deal with source code annotations. > > Or use a “#include "avfilter_link_internal_fields.h" instead of a macro. > >> c) Wrap the internal part in an #ifdef HAVE_AV_CONFIG_H, optionally >> using #if defined(HAVE_AV_CONFIG_H) && defined(BUILDING_avfilter). >> d) Same as c), but strip this stuff from installed headers. >> >> I consider b)-d) as inferior to a), which I consider superior to Anton's >> proposal, but the big drawback is its reliance on a compiler extension. > > Once and for all: If the solution requires changing things in the > internal implementation C files and changing the habits of the people > who work on these files (that goes together), then it is inferior to a > solution that does not require that. > > Your solution (a) has this flaw, plus relies on an extension that is > probably not available on all supported platforms (Microsoft I am > looking at you). > It's called -fms-extensions for a reason. > My favor goes to (d). Probably with a stricter test for the (c) part > because the internal fields are not necessary for the filters and > therefore could be hidden there too. > This can be accomplished with a), too. All one has to do is use multiple levels of extensions. - Andreas PS: Upon rethinking, it is not only b) that contains undefined behaviour; it is b)-d) as well as the current code. The reason is that the type of AVFilterLink as seen in the files with FF_INTERNAL_FIELDS is not compatible with the type of AVFilterLink from the files without FF_INTERNAL_FIELDS. This also makes derived types, like the types of function declarations containing pointers to AVFilterLink incompatible and this is a violation of 6.2.7(2) of C11. I wonder if this will become a real problem with lto someday.
Andreas Rheinhardt (12023-02-01): > PS: Upon rethinking, it is not only b) that contains undefined > behaviour; it is b)-d) as well as the current code. The reason is that > the type of AVFilterLink as seen in the files with FF_INTERNAL_FIELDS is > not compatible with the type of AVFilterLink from the files without > FF_INTERNAL_FIELDS. This also makes derived types, like the types of > function declarations containing pointers to AVFilterLink incompatible > and this is a violation of 6.2.7(2) of C11. I wonder if this will become > a real problem with lto someday. No: read the second half of the previous paragraph: two structures with common first fields are compatible types. What we have been using is a deliberately supported construct. Regards,
Nicolas George: > Andreas Rheinhardt (12023-02-01): >> PS: Upon rethinking, it is not only b) that contains undefined >> behaviour; it is b)-d) as well as the current code. The reason is that >> the type of AVFilterLink as seen in the files with FF_INTERNAL_FIELDS is >> not compatible with the type of AVFilterLink from the files without >> FF_INTERNAL_FIELDS. This also makes derived types, like the types of >> function declarations containing pointers to AVFilterLink incompatible >> and this is a violation of 6.2.7(2) of C11. I wonder if this will become >> a real problem with lto someday. > > No: read the second half of the previous paragraph: two structures with > common first fields are compatible types. What we have been using is a > deliberately supported construct. > "Moreover, two structure, union, or enumerated types declared in separate translation units are compatible if their tags and members satisfy the following requirements: If one is declared with a tag, the other shall be declared with the same tag. If both are completed anywhere within their respective translation units, then the following additional requirements apply: there shall be a one-to-one correspondence between their members such that each pair of corresponding members are declared with compatible types; if one member of the pair is declared with an alignment specifier, the other is declared with an equivalent alignment specifier; and if one member of the pair is declared with a name, the other is declared with the same name. For two structures, corresponding members shall be declared in the same order." 1. There is no one-to-one correspondence (aka a bijection) between the elements of the complete and of the public structure. 2. In the case of AVFilterLink, there is not even an injection from the public to the FF_INTERNAL_FIELDS structure (due to the former having a big reserved array). 3. The fact that these structures share a common initial sequence does not mean that this is legal. There are some guarantees regarding common initial sequences in 6.5.2.3 (6): "One special guarantee is made in order to simplify the use of unions: if a union contains several structures that share a common initial sequence (see below), and if the union object currently contains one of these structures, it is permitted to inspect the common initial part of any of them anywhere that a declaration of the completed type of the union is visible. Two structures share a common initial sequence if corresponding members have compatible types (and, for bit-fields, the same widths) for a sequence of one or more initial members." But there just is no union between the FF_INTERNAL_FIELDS !defined(FF_INTERNAL_FIELDS) structures in the whole codebase. Furthermore, said guarantee is only for inspecting, i.e. reading. For example, for the following two structs sharing a common initial sequence, struct Small { uint64_t foo; uint32_t bar; }; struct Big { uint64_t foo; uint32_t bar; uint32_t baz; }; if one had a union { struct Small; struct Big; }, a write to Small.bar may clobber Big.baz. - Andreas
Andreas Rheinhardt (12023-02-01): > "One special guarantee is made in order to simplify the use of unions: > if a union contains several structures that share a common initial > sequence (see below), and if the union object currently contains one of > these structures, it is permitted to inspect the common initial part of > any of them anywhere that a declaration of the completed type of the > union is visible. Two structures share a common initial sequence if > corresponding members have compatible types (and, for bit-fields, the > same widths) for a sequence of one or more initial members." > > But there just is no union between the FF_INTERNAL_FIELDS > !defined(FF_INTERNAL_FIELDS) structures in the whole codebase. It is not necessary that there is one: it is enough that there could be one in another compilation unit, the code that handles these structures does not know what exists in the rest of the code base. This guarantee was made FOR unions, but it does not REQUIRE unions, it applies to anything that is semantically equivalent to a union. > Furthermore, said guarantee is only for inspecting, i.e. reading. For > example, for the following two structs sharing a common initial sequence, > > struct Small { > uint64_t foo; > uint32_t bar; > }; > struct Big { > uint64_t foo; > uint32_t bar; > uint32_t baz; > }; > > if one had a union { struct Small; struct Big; }, a write to Small.bar > may clobber Big.baz. That is a good point. It does not apply when the first field of Big is Small itself, which is the case that I absolutely know is supported, because in that case Big will embed the padding of Small. I had not thought of this. Fortunately, even if we are not 100% in the case that is officially supported, there are multiple reasons that each should be enough to guarantee that our code will work: - Between the public part of the structure and the #ifede FF_INTERNAL_FIELDS, there are a lot of "not part of the public API" fields", changing ch_layout will not clobber incfg because incfg is known at this point. - Even if there was no fields in between, reserved[] offers the same protection. - And even if there was no gap, we are good because applications, and even filters, are not supposed to modify AVFilterLink, only the framework and the framework knows the whole structure. In fact, that last remark makes me think of another solution, for the slightly longer term: make AVFilterLink entirely opaque. The only documented reason for application to access AVFilterLink was to get the parameters of buffersink, but buffersink has a real API to get this since 2016. Anyway, if you prefer using a compilation option, I have no objection. My only concern is that when a developer writes: if (link->x == ... && link->y == ... && link->status_in == ... && link->min_samples == ..) they have to remember that no, status_in is different from all the others. And it is especially bad in this particular case because the distinction is not public / private, which makes some semantic sense and might be easier to remember, the distinction is actually public-or-private-because-of-a-comment / private-because-of-a-ifdef. But even if the distinction really was public / private, I consider it unacceptable if we can do otherwise. And we can. Regards,
Nicolas George: > Andreas Rheinhardt (12023-02-01): >> "One special guarantee is made in order to simplify the use of unions: >> if a union contains several structures that share a common initial >> sequence (see below), and if the union object currently contains one of >> these structures, it is permitted to inspect the common initial part of >> any of them anywhere that a declaration of the completed type of the >> union is visible. Two structures share a common initial sequence if >> corresponding members have compatible types (and, for bit-fields, the >> same widths) for a sequence of one or more initial members." >> >> But there just is no union between the FF_INTERNAL_FIELDS >> !defined(FF_INTERNAL_FIELDS) structures in the whole codebase. > > It is not necessary that there is one: it is enough that there could be > one in another compilation unit, the code that handles these structures > does not know what exists in the rest of the code base. This guarantee > was made FOR unions, but it does not REQUIRE unions, it applies to > anything that is semantically equivalent to a union. > 1. The common initial sequence rule indeed forced traditional compilers to use the same offsets for the members of the common initial sequence of two arbitrary structs, because there might be a union of these two in another translation unit. But this is no longer true for lto compilers that see the whole program at once (although I don't think that they will deviate from the behaviour of traditional compilers in this regard). 2. But even if the offsets are fine, does not mean that the accesses are fine. Notice the clause "anywhere that a declaration of the completed type of the union is visible" above? It is accompanied with the following example: "The following is not a valid fragment (because the union type is not visible within function f): struct t1 { int m; }; struct t2 { int m; }; int f(struct t1 *p1, struct t2 *p2) { if (p1->m < 0) p2->m = -p2->m; return p1->m; } int g() { union { struct t1 s1; struct t2 s2; } u; /* ... */ return f(&u.s1, &u.s2); }" >> Furthermore, said guarantee is only for inspecting, i.e. reading. For >> example, for the following two structs sharing a common initial sequence, >> >> struct Small { >> uint64_t foo; >> uint32_t bar; >> }; >> struct Big { >> uint64_t foo; >> uint32_t bar; >> uint32_t baz; >> }; >> >> if one had a union { struct Small; struct Big; }, a write to Small.bar >> may clobber Big.baz. > > That is a good point. It does not apply when the first field of Big is > Small itself, which is the case that I absolutely know is supported, > because in that case Big will embed the padding of Small. I had not > thought of this. > > Fortunately, even if we are not 100% in the case that is officially > supported, there are multiple reasons that each should be enough to > guarantee that our code will work: > I'd rather have something that is actually supported and spec-compliant. Anyway, I worry more about lto-compilers than about traditional compilers accidentally clobbering something (after all, most of our fields are sizeof(int)-sized or a multiple (double) thereof, so CPU instruction sets can write this natively and there is no advantage in touching padding). > - Between the public part of the structure and the #ifede > FF_INTERNAL_FIELDS, there are a lot of "not part of the public API" > fields", changing ch_layout will not clobber incfg because incfg is > known at this point. > > - Even if there was no fields in between, reserved[] offers the same > protection. > > - And even if there was no gap, we are good because applications, and > even filters, are not supposed to modify AVFilterLink, only the > framework and the framework knows the whole structure. > > In fact, that last remark makes me think of another solution, for the > slightly longer term: make AVFilterLink entirely opaque. The only > documented reason for application to access AVFilterLink was to get the > parameters of buffersink, but buffersink has a real API to get this > since 2016. > > Anyway, if you prefer using a compilation option, I have no objection. > My only concern is that when a developer writes: > > if (link->x == ... && > link->y == ... && > link->status_in == ... && > link->min_samples == ..) > > they have to remember that no, status_in is different from all the > others. > > And it is especially bad in this particular case because the distinction > is not public / private, which makes some semantic sense and might be > easier to remember, the distinction is actually > public-or-private-because-of-a-comment / private-because-of-a-ifdef. > > But even if the distinction really was public / private, I consider it > unacceptable if we can do otherwise. And we can. > > Regards, >
diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c index c2ecdffa6f5..2a1f99bd656 100644 --- a/libavfilter/avfilter.c +++ b/libavfilter/avfilter.c @@ -34,15 +34,14 @@ #include "libavutil/rational.h" #include "libavutil/samplefmt.h" -#define FF_INTERNAL_FIELDS 1 -#include "framequeue.h" - #include "audio.h" #include "avfilter.h" #include "filters.h" #include "formats.h" +#include "framequeue.h" #include "framepool.h" #include "internal.h" +#include "link_internal.h" static void tlog_ref(void *ctx, AVFrame *ref, int end) { @@ -148,6 +147,7 @@ int ff_append_outpad_free_name(AVFilterContext *f, AVFilterPad *p) int avfilter_link(AVFilterContext *src, unsigned srcpad, AVFilterContext *dst, unsigned dstpad) { + FilterLinkInternal *li; AVFilterLink *link; av_assert0(src->graph); @@ -166,9 +166,10 @@ int avfilter_link(AVFilterContext *src, unsigned srcpad, return AVERROR(EINVAL); } - link = av_mallocz(sizeof(*link)); - if (!link) + li = av_mallocz(sizeof(*li)); + if (!li) return AVERROR(ENOMEM); + link = &li->l; src->outputs[srcpad] = dst->inputs[dstpad] = link; @@ -179,17 +180,20 @@ int avfilter_link(AVFilterContext *src, unsigned srcpad, link->type = src->output_pads[srcpad].type; av_assert0(AV_PIX_FMT_NONE == -1 && AV_SAMPLE_FMT_NONE == -1); link->format = -1; - ff_framequeue_init(&link->fifo, &src->graph->internal->frame_queues); + ff_framequeue_init(&li->fifo, &src->graph->internal->frame_queues); return 0; } void avfilter_link_free(AVFilterLink **link) { + FilterLinkInternal *li; + if (!*link) return; + li = ff_link_internal(*link); - ff_framequeue_free(&(*link)->fifo); + ff_framequeue_free(&li->fifo); ff_frame_pool_uninit((FFFramePool**)&(*link)->frame_pool); av_channel_layout_uninit(&(*link)->ch_layout); @@ -209,29 +213,35 @@ static void filter_unblock(AVFilterContext *filter) { unsigned i; - for (i = 0; i < filter->nb_outputs; i++) - filter->outputs[i]->frame_blocked_in = 0; + for (i = 0; i < filter->nb_outputs; i++) { + FilterLinkInternal * const li = ff_link_internal(filter->outputs[i]); + li->frame_blocked_in = 0; + } } void ff_avfilter_link_set_in_status(AVFilterLink *link, int status, int64_t pts) { - if (link->status_in == status) + FilterLinkInternal * const li = ff_link_internal(link); + + if (li->status_in == status) return; - av_assert0(!link->status_in); - link->status_in = status; - link->status_in_pts = pts; + av_assert0(!li->status_in); + li->status_in = status; + li->status_in_pts = pts; link->frame_wanted_out = 0; - link->frame_blocked_in = 0; + li->frame_blocked_in = 0; filter_unblock(link->dst); ff_filter_set_ready(link->dst, 200); } void ff_avfilter_link_set_out_status(AVFilterLink *link, int status, int64_t pts) { + FilterLinkInternal * const li = ff_link_internal(link); + av_assert0(!link->frame_wanted_out); - av_assert0(!link->status_out); - link->status_out = status; + av_assert0(!li->status_out); + li->status_out = status; if (pts != AV_NOPTS_VALUE) ff_update_link_current_pts(link, pts); filter_unblock(link->dst); @@ -409,13 +419,15 @@ void ff_tlog_link(void *ctx, AVFilterLink *link, int end) int ff_request_frame(AVFilterLink *link) { + FilterLinkInternal * const li = ff_link_internal(link); + FF_TPRINTF_START(NULL, request_frame); ff_tlog_link(NULL, link, 1); av_assert1(!link->dst->filter->activate); - if (link->status_out) - return link->status_out; - if (link->status_in) { - if (ff_framequeue_queued_frames(&link->fifo)) { + if (li->status_out) + return li->status_out; + if (li->status_in) { + if (ff_framequeue_queued_frames(&li->fifo)) { av_assert1(!link->frame_wanted_out); av_assert1(link->dst->ready >= 300); return 0; @@ -423,8 +435,8 @@ int ff_request_frame(AVFilterLink *link) /* Acknowledge status change. Filters using ff_request_frame() will handle the change automatically. Filters can also check the status directly but none do yet. */ - ff_avfilter_link_set_out_status(link, link->status_in, link->status_in_pts); - return link->status_out; + ff_avfilter_link_set_out_status(link, li->status_in, li->status_in_pts); + return li->status_out; } } link->frame_wanted_out = 1; @@ -437,14 +449,18 @@ static int64_t guess_status_pts(AVFilterContext *ctx, int status, AVRational lin unsigned i; int64_t r = INT64_MAX; - for (i = 0; i < ctx->nb_inputs; i++) - if (ctx->inputs[i]->status_out == status) + for (i = 0; i < ctx->nb_inputs; i++) { + FilterLinkInternal * const li = ff_link_internal(ctx->inputs[i]); + if (li->status_out == status) r = FFMIN(r, av_rescale_q(ctx->inputs[i]->current_pts, ctx->inputs[i]->time_base, link_time_base)); + } if (r < INT64_MAX) return r; av_log(ctx, AV_LOG_WARNING, "EOF timestamp not reliable\n"); - for (i = 0; i < ctx->nb_inputs; i++) - r = FFMIN(r, av_rescale_q(ctx->inputs[i]->status_in_pts, ctx->inputs[i]->time_base, link_time_base)); + for (i = 0; i < ctx->nb_inputs; i++) { + FilterLinkInternal * const li = ff_link_internal(ctx->inputs[i]); + r = FFMIN(r, av_rescale_q(li->status_in_pts, ctx->inputs[i]->time_base, link_time_base)); + } if (r < INT64_MAX) return r; return AV_NOPTS_VALUE; @@ -452,17 +468,18 @@ static int64_t guess_status_pts(AVFilterContext *ctx, int status, AVRational lin static int ff_request_frame_to_filter(AVFilterLink *link) { + FilterLinkInternal * const li = ff_link_internal(link); int ret = -1; FF_TPRINTF_START(NULL, request_frame_to_filter); ff_tlog_link(NULL, link, 1); /* Assume the filter is blocked, let the method clear it if not */ - link->frame_blocked_in = 1; + li->frame_blocked_in = 1; if (link->srcpad->request_frame) ret = link->srcpad->request_frame(link); else if (link->src->inputs[0]) ret = ff_request_frame(link->src->inputs[0]); if (ret < 0) { - if (ret != AVERROR(EAGAIN) && ret != link->status_in) + if (ret != AVERROR(EAGAIN) && ret != li->status_in) ff_avfilter_link_set_in_status(link, ret, guess_status_pts(link->src, ret, link->time_base)); if (ret == AVERROR_EOF) ret = 0; @@ -977,6 +994,7 @@ fail: int ff_filter_frame(AVFilterLink *link, AVFrame *frame) { + FilterLinkInternal * const li = ff_link_internal(link); int ret; FF_TPRINTF_START(NULL, filter_frame); ff_tlog_link(NULL, link, 1); ff_tlog(NULL, " "); tlog_ref(NULL, frame, 1); @@ -1006,11 +1024,11 @@ int ff_filter_frame(AVFilterLink *link, AVFrame *frame) } } - link->frame_blocked_in = link->frame_wanted_out = 0; + li->frame_blocked_in = link->frame_wanted_out = 0; link->frame_count_in++; link->sample_count_in += frame->nb_samples; filter_unblock(link->dst); - ret = ff_framequeue_add(&link->fifo, frame); + ret = ff_framequeue_add(&li->fifo, frame); if (ret < 0) { av_frame_free(&frame); return ret; @@ -1023,16 +1041,17 @@ error: return AVERROR_PATCHWELCOME; } -static int samples_ready(AVFilterLink *link, unsigned min) +static int samples_ready(FilterLinkInternal *link, unsigned min) { return ff_framequeue_queued_frames(&link->fifo) && (ff_framequeue_queued_samples(&link->fifo) >= min || link->status_in); } -static int take_samples(AVFilterLink *link, unsigned min, unsigned max, +static int take_samples(FilterLinkInternal *li, unsigned min, unsigned max, AVFrame **rframe) { + AVFilterLink *link = &li->l; AVFrame *frame0, *frame, *buf; unsigned nb_samples, nb_frames, i, p; int ret; @@ -1040,9 +1059,9 @@ static int take_samples(AVFilterLink *link, unsigned min, unsigned max, /* Note: this function relies on no format changes and must only be called with enough samples. */ av_assert1(samples_ready(link, link->min_samples)); - frame0 = frame = ff_framequeue_peek(&link->fifo, 0); - if (!link->fifo.samples_skipped && frame->nb_samples >= min && frame->nb_samples <= max) { - *rframe = ff_framequeue_take(&link->fifo); + frame0 = frame = ff_framequeue_peek(&li->fifo, 0); + if (!li->fifo.samples_skipped && frame->nb_samples >= min && frame->nb_samples <= max) { + *rframe = ff_framequeue_take(&li->fifo); return 0; } nb_frames = 0; @@ -1055,9 +1074,9 @@ static int take_samples(AVFilterLink *link, unsigned min, unsigned max, } nb_samples += frame->nb_samples; nb_frames++; - if (nb_frames == ff_framequeue_queued_frames(&link->fifo)) + if (nb_frames == ff_framequeue_queued_frames(&li->fifo)) break; - frame = ff_framequeue_peek(&link->fifo, nb_frames); + frame = ff_framequeue_peek(&li->fifo, nb_frames); } buf = ff_get_audio_buffer(link, nb_samples); @@ -1071,7 +1090,7 @@ static int take_samples(AVFilterLink *link, unsigned min, unsigned max, p = 0; for (i = 0; i < nb_frames; i++) { - frame = ff_framequeue_take(&link->fifo); + frame = ff_framequeue_take(&li->fifo); av_samples_copy(buf->extended_data, frame->extended_data, p, 0, frame->nb_samples, link->ch_layout.nb_channels, link->format); p += frame->nb_samples; @@ -1079,10 +1098,10 @@ static int take_samples(AVFilterLink *link, unsigned min, unsigned max, } if (p < nb_samples) { unsigned n = nb_samples - p; - frame = ff_framequeue_peek(&link->fifo, 0); + frame = ff_framequeue_peek(&li->fifo, 0); av_samples_copy(buf->extended_data, frame->extended_data, p, 0, n, link->ch_layout.nb_channels, link->format); - ff_framequeue_skip_samples(&link->fifo, n, link->time_base); + ff_framequeue_skip_samples(&li->fifo, n, link->time_base); } *rframe = buf; @@ -1091,6 +1110,7 @@ static int take_samples(AVFilterLink *link, unsigned min, unsigned max, static int ff_filter_frame_to_filter(AVFilterLink *link) { + FilterLinkInternal * const li = ff_link_internal(link); AVFrame *frame = NULL; AVFilterContext *dst = link->dst; int ret; @@ -1111,7 +1131,7 @@ static int ff_filter_frame_to_filter(AVFilterLink *link) before the frame; ff_filter_frame_framed() will re-increment it. */ link->frame_count_out--; ret = ff_filter_frame_framed(link, frame); - if (ret < 0 && ret != link->status_out) { + if (ret < 0 && ret != li->status_out) { ff_avfilter_link_set_out_status(link, ret, AV_NOPTS_VALUE); } else { /* Run once again, to see if several frames were available, or if @@ -1121,18 +1141,21 @@ static int ff_filter_frame_to_filter(AVFilterLink *link) return ret; } -static int forward_status_change(AVFilterContext *filter, AVFilterLink *in) +static int forward_status_change(AVFilterContext *filter, FilterLinkInternal *li_in) { + AVFilterLink *in = &li_in->l; unsigned out = 0, progress = 0; int ret; - av_assert0(!in->status_out); + av_assert0(!li_in->status_out); if (!filter->nb_outputs) { /* not necessary with the current API and sinks */ return 0; } - while (!in->status_out) { - if (!filter->outputs[out]->status_in) { + while (!li_in->status_out) { + FilterLinkInternal *li_out = ff_link_internal(filter->outputs[out]); + + if (!li_out->status_in) { progress++; ret = ff_request_frame_to_filter(filter->outputs[out]); if (ret < 0) @@ -1142,7 +1165,7 @@ static int forward_status_change(AVFilterContext *filter, AVFilterLink *in) if (!progress) { /* Every output already closed: input no longer interesting (example: overlay in shortest mode, other input closed). */ - ff_avfilter_link_set_out_status(in, in->status_in, in->status_in_pts); + ff_avfilter_link_set_out_status(in, li_in->status_in, li_in->status_in_pts); return 0; } progress = 0; @@ -1158,19 +1181,22 @@ static int ff_filter_activate_default(AVFilterContext *filter) unsigned i; for (i = 0; i < filter->nb_inputs; i++) { - if (samples_ready(filter->inputs[i], filter->inputs[i]->min_samples)) { + if (samples_ready(ff_link_internal(filter->inputs[i]), + filter->inputs[i]->min_samples)) { return ff_filter_frame_to_filter(filter->inputs[i]); } } for (i = 0; i < filter->nb_inputs; i++) { - if (filter->inputs[i]->status_in && !filter->inputs[i]->status_out) { - av_assert1(!ff_framequeue_queued_frames(&filter->inputs[i]->fifo)); - return forward_status_change(filter, filter->inputs[i]); + FilterLinkInternal * const li = ff_link_internal(filter->inputs[i]); + if (li->status_in && !li->status_out) { + av_assert1(!ff_framequeue_queued_frames(&li->fifo)); + return forward_status_change(filter, li); } } for (i = 0; i < filter->nb_outputs; i++) { + FilterLinkInternal * const li = ff_link_internal(filter->outputs[i]); if (filter->outputs[i]->frame_wanted_out && - !filter->outputs[i]->frame_blocked_in) { + !li->frame_blocked_in) { return ff_request_frame_to_filter(filter->outputs[i]); } } @@ -1326,39 +1352,44 @@ int ff_filter_activate(AVFilterContext *filter) int ff_inlink_acknowledge_status(AVFilterLink *link, int *rstatus, int64_t *rpts) { + FilterLinkInternal * const li = ff_link_internal(link); *rpts = link->current_pts; - if (ff_framequeue_queued_frames(&link->fifo)) + if (ff_framequeue_queued_frames(&li->fifo)) return *rstatus = 0; - if (link->status_out) - return *rstatus = link->status_out; - if (!link->status_in) + if (li->status_out) + return *rstatus = li->status_out; + if (!li->status_in) return *rstatus = 0; - *rstatus = link->status_out = link->status_in; - ff_update_link_current_pts(link, link->status_in_pts); + *rstatus = li->status_out = li->status_in; + ff_update_link_current_pts(link, li->status_in_pts); *rpts = link->current_pts; return 1; } size_t ff_inlink_queued_frames(AVFilterLink *link) { - return ff_framequeue_queued_frames(&link->fifo); + FilterLinkInternal * const li = ff_link_internal(link); + return ff_framequeue_queued_frames(&li->fifo); } int ff_inlink_check_available_frame(AVFilterLink *link) { - return ff_framequeue_queued_frames(&link->fifo) > 0; + FilterLinkInternal * const li = ff_link_internal(link); + return ff_framequeue_queued_frames(&li->fifo) > 0; } int ff_inlink_queued_samples(AVFilterLink *link) { - return ff_framequeue_queued_samples(&link->fifo); + FilterLinkInternal * const li = ff_link_internal(link); + return ff_framequeue_queued_samples(&li->fifo); } int ff_inlink_check_available_samples(AVFilterLink *link, unsigned min) { - uint64_t samples = ff_framequeue_queued_samples(&link->fifo); + FilterLinkInternal * const li = ff_link_internal(link); + uint64_t samples = ff_framequeue_queued_samples(&li->fifo); av_assert1(min); - return samples >= min || (link->status_in && samples); + return samples >= min || (li->status_in && samples); } static void consume_update(AVFilterLink *link, const AVFrame *frame) @@ -1372,18 +1403,19 @@ static void consume_update(AVFilterLink *link, const AVFrame *frame) int ff_inlink_consume_frame(AVFilterLink *link, AVFrame **rframe) { + FilterLinkInternal * const li = ff_link_internal(link); AVFrame *frame; *rframe = NULL; if (!ff_inlink_check_available_frame(link)) return 0; - if (link->fifo.samples_skipped) { - frame = ff_framequeue_peek(&link->fifo, 0); + if (li->fifo.samples_skipped) { + frame = ff_framequeue_peek(&li->fifo, 0); return ff_inlink_consume_samples(link, frame->nb_samples, frame->nb_samples, rframe); } - frame = ff_framequeue_take(&link->fifo); + frame = ff_framequeue_take(&li->fifo); consume_update(link, frame); *rframe = frame; return 1; @@ -1392,6 +1424,7 @@ int ff_inlink_consume_frame(AVFilterLink *link, AVFrame **rframe) int ff_inlink_consume_samples(AVFilterLink *link, unsigned min, unsigned max, AVFrame **rframe) { + FilterLinkInternal * const li = ff_link_internal(link); AVFrame *frame; int ret; @@ -1399,9 +1432,9 @@ int ff_inlink_consume_samples(AVFilterLink *link, unsigned min, unsigned max, *rframe = NULL; if (!ff_inlink_check_available_samples(link, min)) return 0; - if (link->status_in) - min = FFMIN(min, ff_framequeue_queued_samples(&link->fifo)); - ret = take_samples(link, min, max, &frame); + if (li->status_in) + min = FFMIN(min, ff_framequeue_queued_samples(&li->fifo)); + ret = take_samples(li, min, max, &frame); if (ret < 0) return ret; consume_update(link, frame); @@ -1411,7 +1444,8 @@ int ff_inlink_consume_samples(AVFilterLink *link, unsigned min, unsigned max, AVFrame *ff_inlink_peek_frame(AVFilterLink *link, size_t idx) { - return ff_framequeue_peek(&link->fifo, idx); + FilterLinkInternal * const li = ff_link_internal(link); + return ff_framequeue_peek(&li->fifo, idx); } int ff_inlink_make_frame_writable(AVFilterLink *link, AVFrame **rframe) @@ -1497,29 +1531,32 @@ void ff_inlink_request_frame(AVFilterLink *link) void ff_inlink_set_status(AVFilterLink *link, int status) { - if (link->status_out) + FilterLinkInternal * const li = ff_link_internal(link); + if (li->status_out) return; link->frame_wanted_out = 0; - link->frame_blocked_in = 0; + li->frame_blocked_in = 0; ff_avfilter_link_set_out_status(link, status, AV_NOPTS_VALUE); - while (ff_framequeue_queued_frames(&link->fifo)) { - AVFrame *frame = ff_framequeue_take(&link->fifo); + while (ff_framequeue_queued_frames(&li->fifo)) { + AVFrame *frame = ff_framequeue_take(&li->fifo); av_frame_free(&frame); } - if (!link->status_in) - link->status_in = status; + if (!li->status_in) + li->status_in = status; } int ff_outlink_get_status(AVFilterLink *link) { - return link->status_in; + FilterLinkInternal * const li = ff_link_internal(link); + return li->status_in; } int ff_inoutlink_check_flow(AVFilterLink *inlink, AVFilterLink *outlink) { + FilterLinkInternal * const li_in = ff_link_internal(inlink); return ff_outlink_frame_wanted(outlink) || ff_inlink_check_available_frame(inlink) || - inlink->status_out; + li_in->status_out; } diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h index c2ec7a4b5fc..5ccab4a3f8f 100644 --- a/libavfilter/avfilter.h +++ b/libavfilter/avfilter.h @@ -668,51 +668,6 @@ struct AVFilterLink { * AVHWFramesContext describing the frames. */ AVBufferRef *hw_frames_ctx; - -#ifndef FF_INTERNAL_FIELDS - - /** - * Internal structure members. - * The fields below this limit are internal for libavfilter's use - * and must in no way be accessed by applications. - */ - char reserved[0xF000]; - -#else /* FF_INTERNAL_FIELDS */ - - /** - * Queue of frames waiting to be filtered. - */ - FFFrameQueue fifo; - - /** - * If set, the source filter can not generate a frame as is. - * The goal is to avoid repeatedly calling the request_frame() method on - * the same link. - */ - int frame_blocked_in; - - /** - * Link input status. - * If not zero, all attempts of filter_frame will fail with the - * corresponding code. - */ - int status_in; - - /** - * Timestamp of the input status change. - */ - int64_t status_in_pts; - - /** - * Link output status. - * If not zero, all attempts of request_frame will fail with the - * corresponding code. - */ - int status_out; - -#endif /* FF_INTERNAL_FIELDS */ - }; /** diff --git a/libavfilter/avfiltergraph.c b/libavfilter/avfiltergraph.c index 53f468494d3..c3280f5f8e7 100644 --- a/libavfilter/avfiltergraph.c +++ b/libavfilter/avfiltergraph.c @@ -31,13 +31,13 @@ #include "libavutil/opt.h" #include "libavutil/pixdesc.h" -#define FF_INTERNAL_FIELDS 1 -#include "framequeue.h" #include "avfilter.h" #include "buffersink.h" #include "formats.h" +#include "framequeue.h" #include "internal.h" +#include "link_internal.h" #include "thread.h" #define OFFSET(x) offsetof(AVFilterGraph, x) @@ -1326,10 +1326,11 @@ int avfilter_graph_request_oldest(AVFilterGraph *graph) av_assert1(oldest->age_index >= 0); frame_count = oldest->frame_count_out; while (frame_count == oldest->frame_count_out) { + FilterLinkInternal * const li = ff_link_internal(oldest); r = ff_filter_graph_run_once(graph); if (r == AVERROR(EAGAIN) && - !oldest->frame_wanted_out && !oldest->frame_blocked_in && - !oldest->status_in) + !oldest->frame_wanted_out && !li->frame_blocked_in && + !li->status_in) ff_request_frame(oldest); else if (r < 0) return r; diff --git a/libavfilter/buffersink.c b/libavfilter/buffersink.c index e269cf72d1b..e102bd22f38 100644 --- a/libavfilter/buffersink.c +++ b/libavfilter/buffersink.c @@ -30,14 +30,13 @@ #include "libavutil/internal.h" #include "libavutil/opt.h" -#define FF_INTERNAL_FIELDS 1 -#include "framequeue.h" - #include "audio.h" #include "avfilter.h" #include "buffersink.h" #include "filters.h" +#include "framequeue.h" #include "internal.h" +#include "link_internal.h" typedef struct BufferSinkContext { const AVClass *class; @@ -187,9 +186,10 @@ static av_cold int common_init(AVFilterContext *ctx) static int activate(AVFilterContext *ctx) { BufferSinkContext *buf = ctx->priv; + FilterLinkInternal * const li = ff_link_internal(ctx->inputs[0]); if (buf->warning_limit && - ff_framequeue_queued_frames(&ctx->inputs[0]->fifo) >= buf->warning_limit) { + ff_framequeue_queued_frames(&li->fifo) >= buf->warning_limit) { av_log(ctx, AV_LOG_WARNING, "%d buffers queued in %s, something may be wrong.\n", buf->warning_limit, diff --git a/libavfilter/link_internal.h b/libavfilter/link_internal.h new file mode 100644 index 00000000000..b5a8ac89ec2 --- /dev/null +++ b/libavfilter/link_internal.h @@ -0,0 +1,69 @@ +/* + * Internal filter link API + * + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#ifndef AVFILTER_LINK_INTERNAL_H +#define AVFILTER_LINK_INTERNAL_H + +#include <stdint.h> + +#include "avfilter.h" +#include "framequeue.h" + +typedef struct FilterLinkInternal { + AVFilterLink l; + + /** + * Queue of frames waiting to be filtered. + */ + FFFrameQueue fifo; + + /** + * If set, the source filter can not generate a frame as is. + * The goal is to avoid repeatedly calling the request_frame() method on + * the same link. + */ + int frame_blocked_in; + + /** + * Link input status. + * If not zero, all attempts of filter_frame will fail with the + * corresponding code. + */ + int status_in; + + /** + * Timestamp of the input status change. + */ + int64_t status_in_pts; + + /** + * Link output status. + * If not zero, all attempts of request_frame will fail with the + * corresponding code. + */ + int status_out; +} FilterLinkInternal; + +static inline FilterLinkInternal *ff_link_internal(AVFilterLink *link) +{ + return (FilterLinkInternal*)link; +} + +#endif /* AVFILTER_LINK_INTERNAL_H */ diff --git a/libavfilter/tests/filtfmts.c b/libavfilter/tests/filtfmts.c index 909c1e8dc96..3451ed891e5 100644 --- a/libavfilter/tests/filtfmts.c +++ b/libavfilter/tests/filtfmts.c @@ -25,12 +25,11 @@ #include "libavutil/pixdesc.h" #include "libavutil/samplefmt.h" -#define FF_INTERNAL_FIELDS 1 -#include "libavfilter/framequeue.h" - #include "libavfilter/avfilter.h" #include "libavfilter/formats.h" +#include "libavfilter/framequeue.h" #include "libavfilter/internal.h" +#include "libavfilter/link_internal.h" static void print_formats_internal(AVFilterLink **links, const AVFilterPad *pads, unsigned nb, size_t fmts_cfg_offset, @@ -123,7 +122,7 @@ int main(int argc, char **argv) /* create a link for each of the input pads */ for (i = 0; i < filter_ctx->nb_inputs; i++) { - AVFilterLink *link = av_mallocz(sizeof(AVFilterLink)); + AVFilterLink *link = av_mallocz(sizeof(FilterLinkInternal)); if (!link) { fprintf(stderr, "Unable to allocate memory for filter input link\n"); ret = 1; @@ -133,7 +132,7 @@ int main(int argc, char **argv) filter_ctx->inputs[i] = link; } for (i = 0; i < filter_ctx->nb_outputs; i++) { - AVFilterLink *link = av_mallocz(sizeof(AVFilterLink)); + AVFilterLink *link = av_mallocz(sizeof(FilterLinkInternal)); if (!link) { fprintf(stderr, "Unable to allocate memory for filter output link\n"); ret = 1;