| Message ID | 20161113101021.19812-2-george@nsup.org |
|---|---|
| State | Superseded |
| Headers | show |
On 11/13/2016 7:10 AM, Nicolas George wrote: > A lot of changes happen at the same time: > > - Add private_fields.h to devine private fields in AVFilterLink. > It allows to have the private fields directly in structured types > without needing an indirection for an opaque structure nor the > structure definition in public headers. I don't really like this method. This kind of ifdeffery is very uncommon and feels dirty. Just look at avutil/common.h to see how bad a public header can get this way. I'd prefer if we can stick with the usual internal opaque structs instead. The plan was to remove as many internal-but-not-really fields across the codebase as possible on the next major bump. Ideally, we'd be consistent with whatever method we choose to achieve this, and opaque structs are already being used everywhere, including avfilter.h [...] > diff --git a/libavfilter/private_fields.h b/libavfilter/private_fields.h > new file mode 100644 > index 0000000..5a3df68 > --- /dev/null > +++ b/libavfilter/private_fields.h > @@ -0,0 +1,43 @@ > +/* > + * Generic frame queue > + * Copyright (c) 2016 Nicolas George > + * > + * 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 > + */ > + > +#include "framequeue.h" > + > +#define AVFILTER_LINK_INTERNAL_FIELDS \ > +\ > + FFFrameQueue fifo; \ > +\ > + int frame_blocked_in; \ > +\ > + /** \ > + * Link input status. \ > + */ \ > + int status_in; \ > + 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; \ > + \ In any case, this is missing the usual header guards. > +/* define AVFILTER_LINK_INTERNAL_FIELDS */ > diff --git a/libavfilter/split.c b/libavfilter/split.c > index 6cf4ab1..89e3604 100644 > --- a/libavfilter/split.c > +++ b/libavfilter/split.c > @@ -30,6 +30,7 @@ > #include "libavutil/mem.h" > #include "libavutil/opt.h" > > +#include "private_fields.h" > #include "avfilter.h" > #include "audio.h" > #include "formats.h" > @@ -78,7 +79,7 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *frame) > for (i = 0; i < ctx->nb_outputs; i++) { > AVFrame *buf_out; > > - if (ctx->outputs[i]->status) > + if (ctx->outputs[i]->status_in) > continue; > buf_out = av_frame_clone(frame); > if (!buf_out) { > diff --git a/libavfilter/tests/filtfmts.c b/libavfilter/tests/filtfmts.c > index 46a2d94..682af8c 100644 > --- a/libavfilter/tests/filtfmts.c > +++ b/libavfilter/tests/filtfmts.c > @@ -25,6 +25,7 @@ > #include "libavutil/pixdesc.h" > #include "libavutil/samplefmt.h" > > +#include "libavfilter/private_fields.h" > #include "libavfilter/avfilter.h" > #include "libavfilter/formats.h" > > diff --git a/libavfilter/vf_extractplanes.c b/libavfilter/vf_extractplanes.c > index a23f838..c4b01e0 100644 > --- a/libavfilter/vf_extractplanes.c > +++ b/libavfilter/vf_extractplanes.c > @@ -22,6 +22,7 @@ > #include "libavutil/imgutils.h" > #include "libavutil/opt.h" > #include "libavutil/pixdesc.h" > +#include "private_fields.h" > #include "avfilter.h" > #include "drawutils.h" > #include "internal.h" > @@ -245,7 +246,7 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *frame) > const int idx = s->map[i]; > AVFrame *out; > > - if (outlink->status) > + if (outlink->status_in) > continue; > > out = ff_get_video_buffer(outlink, outlink->w, outlink->h); > diff --git a/tests/ref/fate/source b/tests/ref/fate/source > index 4ee8a58..23ffd8a 100644 > --- a/tests/ref/fate/source > +++ b/tests/ref/fate/source > @@ -29,3 +29,4 @@ compat/cuda/nvcuvid.h > compat/float/float.h > compat/float/limits.h > compat/nvenc/nvEncodeAPI.h > +libavfilter/private_fields.h This shouldn't happen. Guess it's because of the missing guards?
Le tridi 23 brumaire, an CCXXV, James Almer a écrit : > I don't really like this method. This kind of ifdeffery is very > uncommon and feels dirty. Just look at avutil/common.h to see how bad > a public header can get this way. > I'd prefer if we can stick with the usual internal opaque structs > instead. What is the exact solution you would propose instead? Please give a thought to the drawbacks of any alternative solution compared to this one, including indirection overhead. > The plan was to remove as many internal-but-not-really fields across > the codebase as possible on the next major bump. Ideally, we'd be > consistent with whatever method we choose to achieve this, and opaque > structs are already being used everywhere, including avfilter.h Almost everything in AVFilterLink is internal, but it is still needed there, it can not be removed. I think you are confusing with per-codec fields: fields that serve only for a few codecs are moved in private structures. But fields that are common to most codecs stay in the context. To hide private fields from the public API is a different issue altogether. There are several ways of doing that, the one I used was the one I found that minimizes first overhead, then brittleness, then ugliness. But I am open to other suggestions. > In any case, this is missing the usual header guards. This is intentional: this is not a normal header, it should not be included the normal way, and in particular never twice. The absence of guards is meant as a hint in that direction. > This shouldn't happen. Guess it's because of the missing guards? I do not mind adding the guards to avoid that, if that it what people prefer. Regards,
On 11/13/2016 6:57 PM, Nicolas George wrote: > Le tridi 23 brumaire, an CCXXV, James Almer a écrit : >> I don't really like this method. This kind of ifdeffery is very >> uncommon and feels dirty. Just look at avutil/common.h to see how bad >> a public header can get this way. >> I'd prefer if we can stick with the usual internal opaque structs >> instead. > > What is the exact solution you would propose instead? Please give a > thought to the drawbacks of any alternative solution compared to this > one, including indirection overhead. The same method used almost everywhere else. public header: typedef struct AVSomethingInternal AVSomethingInternal; typedef struct AVSomething { int public_field1; int public_field2; AVSomethingInternal *internal; } AVSomething; AVSomething *av_something_alloc(); private header: struct AVSomethingInternal { int private_field1; int private_field2; }; AVSomething *av_something_alloc() { AVSomething *something = malloc(sizeof(*something)); something->internal = malloc(sizeof(*something->internal)); return something; } No technical advantages i can think of compared to your method, but it's thoroughly tested, clean, and above all ubiquitous. See for example AVFilterInternal and AVCodecInternal. As i said, what I'm mostly looking for is consistency across the codebase and to reduce ifdeffery. > >> The plan was to remove as many internal-but-not-really fields across >> the codebase as possible on the next major bump. Ideally, we'd be >> consistent with whatever method we choose to achieve this, and opaque >> structs are already being used everywhere, including avfilter.h > > Almost everything in AVFilterLink is internal, but it is still needed > there, it can not be removed. That's the other method currently used for internal fields. Left in the public struct, with a huge message saying they are internal and shouldn't be accessed. That has proven to be problematic before, between merges adding fields at the wrong offset by accident, or downstream projects ignoring the big warnings and accessing them anyway. They are the ones planned to be made actually internal in the next bump. > > I think you are confusing with per-codec fields: fields that serve only > for a few codecs are moved in private structures. But fields that are > common to most codecs stay in the context. AVCodecInternal is explicitly not codec-specific. > > To hide private fields from the public API is a different issue > altogether. There are several ways of doing that, the one I used was the > one I found that minimizes first overhead, then brittleness, then > ugliness. But I am open to other suggestions. > >> In any case, this is missing the usual header guards. > > This is intentional: this is not a normal header, it should not be > included the normal way, and in particular never twice. The absence of > guards is meant as a hint in that direction. Every header is meant to be included once. That's why guards are put in place. > >> This shouldn't happen. Guess it's because of the missing guards? > > I do not mind adding the guards to avoid that, if that it what people > prefer. If this ends up being implemented this way, then yes, please include them.
Le tridi 23 brumaire, an CCXXV, James Almer a écrit : > AVSomethingInternal *internal; > No technical advantages i can think of compared to your method, but it's > thoroughly tested, clean, and above all ubiquitous. See for example > AVFilterInternal and AVCodecInternal. It has technical DISadvantages, and therefore needs a good reason to choose. I would really appreciate that objections acknowledge and discuss them, however minimally. In other words, unless your message contains a sense that looks like that: Compared to yours, my solution has the drawback of <fill in here>, but I still think it is better because <fill in here>. ... I consider that the advice was given without having thought about all the implications and therefore cannot be really valuable. > As i said, what I'm mostly looking for is consistency across the codebase > and to reduce ifdeffery. For consistency, I would prefer removing the indirections where they already are and replace them with invisible fields. And I consider this kind of ifdeffery, short and isolated, to be a non-issue. And most of all, efficiency should be the primary concern. Aesthetics comes only second. > That has proven to be problematic before, between merges adding fields at > the wrong offset by accident, IIRC, you are mistaken, we only had that kind of issue about public fields. > or downstream projects ignoring the big > warnings and accessing them anyway. Indeed. But what I propose prevents that too. > AVCodecInternal is explicitly not codec-specific. I forgot this one. > Every header is meant to be included once. That's why guards are put in > place. No, you have it the wrong way. The guards are there to allow the headers to be included several times. Without the guards, the compiler would issue errors for duplicated definitions. Of course, including the same header twice explicitly would be rather stupid. But that is forgetting indirect inclusions. A lot of the core headers end up being included many times like that. The guards are completely necessary for them. Furthermore, including useless headers has usually no worse consequences than a few milliseconds on the build time. This header is different, though: it cannot, must not, be included indirectly, and including it uselessly could have minor negative consequences. > If this ends up being implemented this way, then yes, please include them. If you insist about guards, they should be negative, i.e.: #ifdef AV_PRIVATE_FIELDS_H # error "private_fields.h must not be included indirectly." #endif #define AV_PRIVATE_FIELDS_H Positive guards would be useless, and useless code is harmful. Regards,
On Sun, Nov 13, 2016 at 11:54 PM, Nicolas George <george@nsup.org> wrote: >> As i said, what I'm mostly looking for is consistency across the codebase >> and to reduce ifdeffery. > > For consistency, I would prefer removing the indirections where they > already are and replace them with invisible fields. > > And I consider this kind of ifdeffery, short and isolated, to be a > non-issue. > > And most of all, efficiency should be the primary concern. Aesthetics > comes only second. I disagree with that, this is generic utility code, not a tight algorithm loop, where absolute efficiency is not necessarily required, and disregarding code simplicity amd consistency will just harm in the long-run. > >> That has proven to be problematic before, between merges adding fields at >> the wrong offset by accident, > > IIRC, you are mistaken, we only had that kind of issue about public > fields. No, this happened in 3.1 with "private" fields in public headers (in lavfi among other things), which is what sparked the discussion to get rid of all these in the future, either by making them proper public if appropriate, or hiding them entirely from the publics eye - and the existing concept we have for that is *Internal contexts. - Hendrik
Le quartidi 24 brumaire, an CCXXV, Hendrik Leppkes a écrit : > I disagree with that, this is generic utility code, not a tight > algorithm loop, where absolute efficiency is not necessarily required, > and disregarding code simplicity amd consistency will just harm in the > long-run. We can discuss that. Remember that they are fields in a structure, not code: if these fields are accessed in a tight loop, your argument vanishes. I agree that most of these fields will likely not be used in very tight loops. But "most", "likely", "very", because of these caveats, the concern can not be dismissed entirely. And remember that the indirection also harms the code simplicity, greatly. I claim that my solution yields overall simpler code. And more efficient. (I could play and say that if you do not see how the indirection harms the simplicity of the code, your advice is worthless on the matter, but that would be vile. If you do not see, ask and I will explain; just not right now because I need to wake up early tomorrow.) Regards,
2016-11-14 0:11 GMT+01:00 Hendrik Leppkes <h.leppkes@gmail.com>: >>> That has proven to be problematic before, between merges >>> adding fields at the wrong offset by accident, >> >> IIRC, you are mistaken, we only had that kind of issue about >> public fields. > > No, this happened in 3.1 with "private" fields in public headers (in > lavfi among other things), which is what sparked the discussion to get > rid of all these in the future, either by making them proper public if > appropriate, or hiding them entirely from the publics eye - and the > existing concept we have for that is *Internal contexts. Could you point me to this discussion? Thank you, Carl Eugen
On Mon, Nov 14, 2016 at 12:21 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2016-11-14 0:11 GMT+01:00 Hendrik Leppkes <h.leppkes@gmail.com>: >>>> That has proven to be problematic before, between merges >>>> adding fields at the wrong offset by accident, >>> >>> IIRC, you are mistaken, we only had that kind of issue about >>> public fields. >> >> No, this happened in 3.1 with "private" fields in public headers (in >> lavfi among other things), which is what sparked the discussion to get >> rid of all these in the future, either by making them proper public if >> appropriate, or hiding them entirely from the publics eye - and the >> existing concept we have for that is *Internal contexts. > > Could you point me to this discussion? > It was on IRC shortly after the 3.1 release when we were fixing the ABI "breakage" of some private fields that affected some downstreams. Probably 06/29 and surrounding days. This is in two parts, deprecate (and eventually remove) the getter/setter functions which only existed to keep ABI compat with libav, which we discontinued, so we can allow direct access to these fields again. And in the same process, check which fields marked as "private" or "no direct access" might be better hidden from the user entirely, or promoted to proper public fields with a stable ABI guarantee. - Hendrik
On 11/13/2016 7:54 PM, Nicolas George wrote: > Le tridi 23 brumaire, an CCXXV, James Almer a écrit : >> AVSomethingInternal *internal; > >> No technical advantages i can think of compared to your method, but it's >> thoroughly tested, clean, and above all ubiquitous. See for example >> AVFilterInternal and AVCodecInternal. > > It has technical DISadvantages, and therefore needs a good reason to > choose. I would really appreciate that objections acknowledge and > discuss them, however minimally. In other words, unless your message > contains a sense that looks like that: > > Compared to yours, my solution has the drawback of <fill in > here>, but I still think it is better because <fill in here>. > > ... I consider that the advice was given without having thought about > all the implications and therefore cannot be really valuable. Why do you always only accept or consider replies and arguments from other people when they exclusively fit the narrative you expect them to be? You're literally telling me what i said is worth nothing because it wasn't, or you assumed it wasn't, exactly what you wanted. > >> As i said, what I'm mostly looking for is consistency across the codebase >> and to reduce ifdeffery. > > For consistency, I would prefer removing the indirections where they > already are and replace them with invisible fields. > > And I consider this kind of ifdeffery, short and isolated, to be a > non-issue. > > And most of all, efficiency should be the primary concern. Aesthetics > comes only second. I very much rather stick with a clean, longstanding and proven to work method that doesn't fill the public headers with ifdeffery, doesn't require extra care to avoid including a new special kind of volatile header that should only be used in a specific way, and that doesn't make future merges more a pain in the ass than they already are. One indirection is not going to make a difference, even if we catalog it as a considerable disadvantage for the sake of this discussion. It's not enough of an argument in favor or replacing it with your design when you weight it with the above. And "Aesthetics comes second" many times ends up in spaghetti and nigh unreadable and maintainable code, so lets try to not go that route if possible. Ask Ronald about his libswr simd refactor work if you want anecdotal evidence, or refer to avutil/common.h > >> That has proven to be problematic before, between merges adding fields at >> the wrong offset by accident, > > IIRC, you are mistaken, we only had that kind of issue about public > fields. > >> or downstream projects ignoring the big >> warnings and accessing them anyway. > > Indeed. But what I propose prevents that too. > >> AVCodecInternal is explicitly not codec-specific. > > I forgot this one. > >> Every header is meant to be included once. That's why guards are put in >> place. > > No, you have it the wrong way. The guards are there to allow the headers > to be included several times. Without the guards, the compiler would > issue errors for duplicated definitions. > > Of course, including the same header twice explicitly would be rather > stupid. But that is forgetting indirect inclusions. A lot of the core > headers end up being included many times like that. The guards are > completely necessary for them. > > Furthermore, including useless headers has usually no worse consequences > than a few milliseconds on the build time. > > This header is different, though: it cannot, must not, be included > indirectly, and including it uselessly could have minor negative > consequences. > >> If this ends up being implemented this way, then yes, please include them. > > If you insist about guards, they should be negative, i.e.: > > #ifdef AV_PRIVATE_FIELDS_H > # error "private_fields.h must not be included indirectly." > #endif > #define AV_PRIVATE_FIELDS_H > > Positive guards would be useless, and useless code is harmful. > > Regards, > > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Mon, Nov 14, 2016 at 12:18:12AM +0100, Nicolas George wrote: > Le quartidi 24 brumaire, an CCXXV, Hendrik Leppkes a écrit : > > I disagree with that, this is generic utility code, not a tight > > algorithm loop, where absolute efficiency is not necessarily required, > > and disregarding code simplicity amd consistency will just harm in the > > long-run. > > We can discuss that. Remember that they are fields in a structure, not > code: if these fields are accessed in a tight loop, your argument > vanishes. > You can dereference once before entering the loop
Le quartidi 24 brumaire, an CCXXV, Clement Boesch a écrit :
> You can dereference once before entering the loop
That requires one extra variable, and therefore leaves one less register
for the loop itself.
And of course, all these efficiency problems correspond directly to
readability issues. Now that I think of it (I spent almost a year on
this thing, and that part was at the very beginning), I remember that my
reasons for choosing this solution were first readability, and only
second efficiency.
Regards,
Le tridi 23 brumaire, an CCXXV, James Almer a écrit : > Why do you always only accept or consider replies and arguments from other > people when they exclusively fit the narrative you expect them to be? You're > literally telling me what i said is worth nothing because it wasn't, or you > assumed it wasn't, exactly what you wanted. Your goal is to convince me, is it not? Well, I am telling you what kind of argument is, I expect, necessary to do so. Is it not better that I tell you instead of just arguing and letting you figure it by yourself? It is not specific to me: if you want to convince anyone that A is better than B, you have to address the reasons they have to think that B is better than A. Otherwise, you just show them that you do not understand the issue as well as them. The only thing about me is that I am more conscious of these phenomenons than others, having spent so much of my youth bikeshedding trivial matters on NNTP forums. > I very much rather stick with a clean, longstanding and proven to work method > that doesn't fill the public headers with ifdeffery, doesn't require extra care > to avoid including a new special kind of volatile header that should only be > used in a specific way, and that doesn't make future merges more a pain in the > ass than they already are. It is the second time you invoke the "longstanding and proven to work" argument. Do you have any reason to think that my proposal does not work? If not, withdraw this argument, because it only amounts to FUD. For the merges, it has been a long time since merges on the lavfi framework were mostly impossible. The fork has the scheduling in lavfi completely broken. > One indirection is not going to make a difference, even if we catalog it as a > considerable disadvantage for the sake of this discussion. It's not enough of > an argument in favor or replacing it with your design when you weight it with > the above. > > And "Aesthetics comes second" many times ends up in spaghetti and nigh > unreadable and maintainable code, so lets try to not go that route if possible. > Ask Ronald about his libswr simd refactor work if you want anecdotal evidence, > or refer to avutil/common.h As I said to Clément a few minutes ago, I remembered that my reasons for choosing this solution were mostly readability. The extra efficiency is just a bonus. You seem to consider consistency in the code to be a goal by itself. It is not, it is only a means to an end, the end being simplicity, readability, maintenability. The version with the private structure and an indirection leads to more complex, less readable code. I say we throw consistency away, we go for the simpler and more readable code, as it happens it is more efficient too. And when everybody is convinced that it is satisfactory, we bring back consistency the other way around, porting other parts of the code to that new, better pattern. I can explain why the indirection gives more complex code, but not right now because the time I have is elapsed. Regards,
On Sun, Nov 13, 2016 at 11:54:23PM +0100, Nicolas George wrote: > Le tridi 23 brumaire, an CCXXV, James Almer a écrit : > > AVSomethingInternal *internal; > > > No technical advantages i can think of compared to your method, but it's > > thoroughly tested, clean, and above all ubiquitous. See for example > > AVFilterInternal and AVCodecInternal. > > It has technical DISadvantages, and therefore needs a good reason to > choose. I would really appreciate that objections acknowledge and > discuss them, however minimally. In other words, unless your message > contains a sense that looks like that: > > Compared to yours, my solution has the drawback of <fill in > here>, but I still think it is better because <fill in here>. > > ... I consider that the advice was given without having thought about > all the implications and therefore cannot be really valuable. > The pros for an internal fields are, for me: - consistency across the codebase - definitions more readable (if you're advocating the fact that a multiline define is as readable I'll stop reading right now) - common C pattern, accessible to other developers - tools will likely like it more (doxygen, looking up a field within your IDE/editor, GDB, or any other tool really) - we can still embed various ifdefery in the structure (for FF_API ifdefery, or simply defining constants above a field) So please, I don't want FFmpeg to be again the laughing stock of readability on the Internet just because a structure dereferencing was considered too much overhead. Regards,
Le quartidi 24 brumaire, an CCXXV, Clement Boesch a écrit : > The pros for an internal fields are, for me: > > - consistency across the codebase > - definitions more readable (if you're advocating the fact that a > multiline define is as readable I'll stop reading right now) > - common C pattern, accessible to other developers > - tools will likely like it more (doxygen, looking up a field within your > IDE/editor, GDB, or any other tool really) > - we can still embed various ifdefery in the structure (for FF_API > ifdefery, or simply defining constants above a field) I do not disagree with all that. But you are speaking of the readability of the structure definition, which is rather minor. Your forget another much more important point : with the indirection, the CODE that USES the structure becomes much less readable. Making the structure definition a little less readable to make the actual code a lot simpler, I say it is a very interesting compromise. Code needs readability more than definitions. I can propose another approach to avoid the indirection but address a few of your points : struct AVFilterLink { int public1; int public2; ... #ifdef FF_PRIVATE_FIELDS int private1; int private2; ... #endif }; In particular, I think it will work perfectly fine with Doxygen, IDEs, etc. > So please, I don't want FFmpeg to be again the laughing stock of > readability on the Internet Please keep the emotional arguments out of it. Regards,
On Mon, Nov 14, 2016 at 01:06:30PM +0100, Nicolas George wrote: > Le quartidi 24 brumaire, an CCXXV, Clement Boesch a écrit : > > The pros for an internal fields are, for me: > > > > - consistency across the codebase > > - definitions more readable (if you're advocating the fact that a > > multiline define is as readable I'll stop reading right now) > > - common C pattern, accessible to other developers > > - tools will likely like it more (doxygen, looking up a field within your > > IDE/editor, GDB, or any other tool really) > > - we can still embed various ifdefery in the structure (for FF_API > > ifdefery, or simply defining constants above a field) > > I do not disagree with all that. But you are speaking of the readability > of the structure definition, which is rather minor. > > Your forget another much more important point : with the indirection, > the CODE that USES the structure becomes much less readable. > I don't see how. You add a pointer at the beginning of your function pointing on that internal structure and use that. > Making the structure definition a little less readable to make the > actual code a lot simpler, I say it is a very interesting compromise. > Code needs readability more than definitions. > > I can propose another approach to avoid the indirection but address a > few of your points : > > struct AVFilterLink { > int public1; > int public2; > ... > #ifdef FF_PRIVATE_FIELDS > int private1; > int private2; > ... > #endif > }; > > In particular, I think it will work perfectly fine with Doxygen, IDEs, > etc. > That might not prevent tools from wrongly detecting ABI breakage from headers. The best way to have private fields is to make them private for real, so I would still prefer an opaque pointer, "leaking" zero information. > > So please, I don't want FFmpeg to be again the laughing stock of > > readability on the Internet > > Please keep the emotional arguments out of it. > It's not an emotional argument, it's a political one. "look at the shit FFmpeg developers still do" does not help the project from getting contributors/contributions.
Le quartidi 24 brumaire, an CCXXV, Clement Boesch a écrit : > I don't see how. You add a pointer at the beginning of your function > pointing on that internal structure and use that. That makes extra code. Remember we are talking about lavfi: we frequently have inlink1, inlink2, outlink. If we must have inlink1priv, inlink2priv, outlinkpriv on top of that, that is starting to make a lot of namespace pollution. Also, good luck keeping the variables names in sync. Someone will rename link to inlink someday, but not the corresponding internal variable, and the code will become an unreadable mess because variable that mean the same thing do not have a similar name. Plus, even with a single link, whether we access it using the indirection explicitly "link->internal->x" or with a dedicated variable "linkpriv->x", we still have to remember, for every damn bloody field, whether it is public or private. For codec/filter private context, there is a rather simple rule: if it is specific to the filter, it is private, if it is common to all filters it is in the common context. But it is still pretty annoying to have two variables and to must decide every time which one to use. And I waste a non-negligible amount of time getting it wrong somehow (for example after a copy-paste) and fixing it. For private/public fields, the rule is much more ambiguous, a lot of fields are visible but could be internal, or are in the public structure below the "/*private*/" line. If we were to move a field from visible to internal, or the other way around, we would have to fix all the code that uses it. Sorry, this is bad design, I refuse to have that in my code. I want to write link->fifo the same way I write link->src or link->time_base. You can reject my proposals, sure, but I can reject any proposal that do not verify this property. > That might not prevent tools from wrongly detecting ABI breakage from > headers. Can you explain? > The best way to have private fields is to make them private for > real, so I would still prefer an opaque pointer, "leaking" zero > information. And making the corresponding code unreadable, as I just explained. Rejected. > It's not an emotional argument, it's a political one. "look at the shit > FFmpeg developers still do" does not help the project from getting > contributors/contributions. Or maybe it help the project not getting bad contributors. Regards,
On Mon, Nov 14, 2016 at 04:52:01PM +0100, Nicolas George wrote: > Le quartidi 24 brumaire, an CCXXV, Clement Boesch a écrit : > > I don't see how. You add a pointer at the beginning of your function > > pointing on that internal structure and use that. > > That makes extra code. Remember we are talking about lavfi: we > frequently have inlink1, inlink2, outlink. If we must have inlink1priv, > inlink2priv, outlinkpriv on top of that, that is starting to make a lot > of namespace pollution. > > Also, good luck keeping the variables names in sync. Someone will rename > link to inlink someday, but not the corresponding internal variable, and > the code will become an unreadable mess because variable that mean the > same thing do not have a similar name. > > Plus, even with a single link, whether we access it using the > indirection explicitly "link->internal->x" or with a dedicated variable > "linkpriv->x", we still have to remember, for every damn bloody field, > whether it is public or private. > > For codec/filter private context, there is a rather simple rule: if it > is specific to the filter, it is private, if it is common to all filters > it is in the common context. But it is still pretty annoying to have two > variables and to must decide every time which one to use. And I waste a > non-negligible amount of time getting it wrong somehow (for example > after a copy-paste) and fixing it. > > For private/public fields, the rule is much more ambiguous, a lot of > fields are visible but could be internal, or are in the public structure > below the "/*private*/" line. If we were to move a field from visible to > internal, or the other way around, we would have to fix all the code > that uses it. > > Sorry, this is bad design, I refuse to have that in my code. I want to > write link->fifo the same way I write link->src or link->time_base. You > can reject my proposals, sure, but I can reject any proposal that do not > verify this property. > You could also make a local copy of the fields you need; it is -in my opinion- not that much noise to add one or a few more lines of declaration. > > That might not prevent tools from wrongly detecting ABI breakage from > > headers. > > Can you explain? > Some tool parsing the public header for analyzing structure layout changes in public structure to detect ABI and API breakage would not be able to make a difference in which fields are public or private. BTW, it also means you're showing all the internals and their doc to the users, which is a lot of noise for them. > > It's not an emotional argument, it's a political one. "look at the shit > > FFmpeg developers still do" does not help the project from getting > > contributors/contributions. > > Or maybe it help the project not getting bad contributors. > Then I guess I'm a bad contributor. Your private ifdefery suggestion is probably the least worst, but I still prefer the common paradigm for previously said reasons that IMO still stand. Anyway, I have your points and you have mine, and my care security fuse has blown, so I'm out of the debate. Regards,
Le quartidi 24 brumaire, an CCXXV, Clement Boesch a écrit : > You could also make a local copy of the fields you need; it is -in my > opinion- not that much noise to add one or a few more lines of > declaration. When the fields are used read-only, probably. Still more code for no good reason. When the fields are used read-write, it would require also storing them at the end. And of course it does not work in more complex cases, such as when one of the field is accessed concurrently by threads. > Some tool parsing the public header for analyzing structure layout changes > in public structure to detect ABI and API breakage would not be able to > make a difference in which fields are public or private. Ok. Obviously you are speaking about third-party tools. These tools must rely on the public headers. And, of course, they have to use the preprocessor correctly, otherwise all matter of libraries, not just ffmpeg, will not work. > BTW, it also means you're showing all the internals and their doc to the > users, which is a lot of noise for them. I agree, it is not ideal. I can see a few ways of mitigating that, not very elegant. But I think even that way it is still acceptable. > Then I guess I'm a bad contributor. That is not what I meant, and obviously you were not disgusted to the point of leaving. > Your private ifdefery suggestion is probably the least worst, but I still > prefer the common paradigm for previously said reasons that IMO still > stand. What about: struct AVFilterLink { ... #ifdef FF_INTERNAL_API #inclde "avfilterlink_internal.h" #endif That avoids your objection about showing the internal fields to the users, while avoiding multiline macro definitions and Doxygen trouble. As a side note, I notice that I have quite a few "defined(__KERNEL__)" lying around in /usr/include/. Regards,
On Mon, Nov 14, 2016 at 05:42:51PM +0100, Nicolas George wrote: [...] > What about: > > struct AVFilterLink { > ... > #ifdef FF_INTERNAL_API > #inclde "avfilterlink_internal.h" > #endif > I'm OK to make an exception for filter links that way if it's really important for you, but I don't think that's a good way of dealing with the problem in the rest of the codebase.
On 14.11.2016 17:42, Nicolas George wrote: > What about: > > struct AVFilterLink { > ... > #ifdef FF_INTERNAL_API > #inclde "avfilterlink_internal.h" > #endif I suspect that having different sizes for the same struct in different parts of the code base will upset some static analyzers. (I'm thinking of cbmc.) And rightly so, I'd say, because this can easily cause big trouble. Consider the (not so) theoretical case of API users simply (and wrongly) using the structs on the stack instead of dynamically allocating them. With the internal pointer the internal struct gets allocated in the respective init function and things mostly work fine, while with your proposal this would cause out-of-bounds reads/writes, which likely means an arbitrary code execution vulnerability. Thus I strongly advise against using this approach. On 14.11.2016 16:52, Nicolas George wrote: > Sorry, this is bad design, I refuse to have that in my code. I want to > write link->fifo the same way I write link->src or link->time_base. You > can reject my proposals, sure, but I can reject any proposal that do not > verify this property. If you really care so much about this, you can make the struct completely private and add getter/setter functions for those elements that need public access. Best regards, Andreas
Le quartidi 24 brumaire, an CCXXV, Andreas Cadhalpun a écrit : > I suspect that having different sizes for the same struct in different > parts of the code base will upset some static analyzers. Still, it is completely legal wrt the C standard. > Consider the (not so) theoretical case of API users simply (and wrongly) > using the structs on the stack instead of dynamically allocating them. > > With the internal pointer the internal struct gets allocated in the > respective init function and things mostly work fine, while with > your proposal this would cause out-of-bounds reads/writes, which > likely means an arbitrary code execution vulnerability. I think it will likely result in their application not working at all very soon, and therefore them realizing their mistake. This is exactly how I noticed that filfmts allocates the links itself. I would rather have found a way of making the structure unallocatable, but apparently the C standard refuses undefined structs even when it and the fields after it are not accessed. > If you really care so much about this, you can make the struct completely > private and add getter/setter functions for those elements that need > public access. I would be ok with that. But that is a big incompatible API change. I could argue that lavfi's API is currently unusable anyway. Regards,
Le quartidi 24 brumaire, an CCXXV, Clement Boesch a écrit : > I'm OK to make an exception for filter links that way if it's really > important for you, but I don't think that's a good way of dealing with the > problem in the rest of the codebase. Why? The arguments I gave for AVFilterLink apply to a lot of similar structures. It would make the actual code in a lot of places simpler. Regards,
On Mon, Nov 14, 2016 at 9:39 PM, Nicolas George <george@nsup.org> wrote: > Le quartidi 24 brumaire, an CCXXV, Clement Boesch a écrit : >> I'm OK to make an exception for filter links that way if it's really >> important for you, but I don't think that's a good way of dealing with the >> problem in the rest of the codebase. > > Why? The arguments I gave for AVFilterLink apply to a lot of similar > structures. It would make the actual code in a lot of places simpler. > If anything this discussion has shown that there are quite different priorities for many different developers. A key priority for many of us here seems to be to have a clean separation in public and private fields and keeping the public API clean and respectable (no weird ifdeffery in public headers, for example). We've had such a messy API for such a long time, so if we define any new solutions for the future, it should be one we can all be happy with. You seem to prefer a bit more internal simplicity over external interfaces, which is fine, but you should not expect everyone to share that preference. - Hendrik
On 14.11.2016 21:38, Nicolas George wrote: > Le quartidi 24 brumaire, an CCXXV, Andreas Cadhalpun a écrit : >> Consider the (not so) theoretical case of API users simply (and wrongly) >> using the structs on the stack instead of dynamically allocating them. >> >> With the internal pointer the internal struct gets allocated in the >> respective init function and things mostly work fine, while with >> your proposal this would cause out-of-bounds reads/writes, which >> likely means an arbitrary code execution vulnerability. > > I think it will likely result in their application not working at all > very soon, and therefore them realizing their mistake. This is exactly > how I noticed that filfmts allocates the links itself. It might work at first by accident and then some change in the internal API could cause problems. If the API usage is wrong, it should fail at build-time and not introduce grave security vulnerabilities at run-time. >> If you really care so much about this, you can make the struct completely >> private and add getter/setter functions for those elements that need >> public access. > > I would be ok with that. But that is a big incompatible API change. I > could argue that lavfi's API is currently unusable anyway. API changes in libavfilter are less of a problem, since not that many programs use it. For example, the codecpar change has orders of magnitude more impact. However, ideally any incompatible API change should be coordinated with Libav. Best regards, Andreas
Le quartidi 24 brumaire, an CCXXV, Andreas Cadhalpun a écrit : > It might work at first by accident and then some change in the internal > API could cause problems. > If the API usage is wrong, it should fail at build-time and not introduce > grave security vulnerabilities at run-time. That would be best, indeed. But sometimes you have to do with the limitations of the language and tools and choose the lesser of two evils. > API changes in libavfilter are less of a problem, since not that many > programs use it. For example, the codecpar change has orders of magnitude > more impact. I mostly agree. If people are ok with a sudden API change in lavfi, then I can rework my patch for that. But I want confirmation beforehand. A scheduled API change would be better, but I do not want to wait for it to apply the patch, so you would have to accept a temporary solution. > However, ideally any incompatible API change should be coordinated with Libav. I have been spurned in the past in my attempts to coordinate lavfi evolution with libav, I will not make efforts again for that. Regards,
Le quartidi 24 brumaire, an CCXXV, Hendrik Leppkes a écrit : > If anything this discussion has shown that there are quite different > priorities for many different developers. > > A key priority for many of us here seems to be to have a clean > separation in public and private fields and keeping the public API > clean and respectable (no weird ifdeffery in public headers, for > example). > We've had such a messy API for such a long time, so if we define any > new solutions for the future, it should be one we can all be happy > with. > > You seem to prefer a bit more internal simplicity over external > interfaces, which is fine, but you should not expect everyone to share > that preference. I know all that. But that road works both ways. You can notice that I did not sty to sneak that change within a huge complex patch, I specifically called attention on it. I realize that my proposal is not elegant. More importantly, I realize it has practical drawbacks. I had not thought of all that were raised in the discussion, and I thank the people who did. As a result, I have proposed various ideas for enhancing the proposal and mitigating the drawbacks, while keeping the benefits I want from it. I would appreciate of the other side would extend the same courtesy to me. Actually, no, that is a gross understatement. The real statement would be: a sane discussion can not happen unless the other side makes the same efforts. Here are three ideas to further reduce the drawbacks of my proposal. All these are an alternative to the other suggestion of making AVFilterLink suddenly opaque. 1. Filter the public headers before installing them, in order to completely remove the internal fields. That way, the installed headers are clean and not confusing for the users. 2. In the public view of the structure, instead of just removing the internal fields, replace them with "char reserved[0xE000];". That would completely close the risk of overflow for applications that incorrectly allocate the structure themselves (which I do not think exist, actually). 3. For compilers where we know that works, replace that 0xE000 with SIZE_MAX*7/16, making the structure impossible to allocate, thus forbidding the invalid uses of the API. Now, I realize that a lot of people will find these suggestions ugly. They are, there is no doubt about it. But this is irrelevant: the question is not "are they ugly?" but "are they uglier than the alternative?", and your opinion on the matter can only be relevant if you actually gave a solid amount of thought to the alternative. Regards,
On 17.11.2016 17:27, Nicolas George wrote: > Le quartidi 24 brumaire, an CCXXV, Andreas Cadhalpun a écrit : >> API changes in libavfilter are less of a problem, since not that many >> programs use it. For example, the codecpar change has orders of magnitude >> more impact. > > I mostly agree. If people are ok with a sudden API change in lavfi, then > I can rework my patch for that. But I want confirmation beforehand. I don't think a sudden API change would be good. > A scheduled API change would be better, but I do not want to wait for it > to apply the patch, so you would have to accept a temporary solution. I understand that and think a temporary solution would be OK if it's clear that it will be removed in say 2 years. >> However, ideally any incompatible API change should be coordinated with Libav. > > I have been spurned in the past in my attempts to coordinate lavfi > evolution with libav, I will not make efforts again for that. That's sad. Maybe someone who uses the libavfilter API from both forks can help and mediate in this matter? On 17.11.2016 17:47, Nicolas George wrote: > Here are three ideas to further reduce the drawbacks of my proposal. > > All these are an alternative to the other suggestion of making > AVFilterLink suddenly opaque. > > 1. Filter the public headers before installing them, in order to > completely remove the internal fields. That way, the installed > headers are clean and not confusing for the users. > > 2. In the public view of the structure, instead of just removing the > internal fields, replace them with "char reserved[0xE000];". That > would completely close the risk of overflow for applications that > incorrectly allocate the structure themselves (which I do not think > exist, actually). > > 3. For compilers where we know that works, replace that 0xE000 with > SIZE_MAX*7/16, making the structure impossible to allocate, thus > forbidding the invalid uses of the API. These are good ideas for mitigating the problems with your temporary solution. > Now, I realize that a lot of people will find these suggestions ugly. > They are, there is no doubt about it. But this is irrelevant: the > question is not "are they ugly?" but "are they uglier than the > alternative?", and your opinion on the matter can only be relevant if > you actually gave a solid amount of thought to the alternative. Sure this is somewhat "ugly", but as long as it's only a temporary solution I find that is not really a problem. Best regards, Andreas
On 11/17/16, Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> wrote: > On 17.11.2016 17:27, Nicolas George wrote: >> Le quartidi 24 brumaire, an CCXXV, Andreas Cadhalpun a ecrit : >>> API changes in libavfilter are less of a problem, since not that many >>> programs use it. For example, the codecpar change has orders of magnitude >>> more impact. >> >> I mostly agree. If people are ok with a sudden API change in lavfi, then >> I can rework my patch for that. But I want confirmation beforehand. > > I don't think a sudden API change would be good. > >> A scheduled API change would be better, but I do not want to wait for it >> to apply the patch, so you would have to accept a temporary solution. > > I understand that and think a temporary solution would be OK if it's clear > that it will be removed in say 2 years. > >>> However, ideally any incompatible API change should be coordinated with >>> Libav. >> >> I have been spurned in the past in my attempts to coordinate lavfi >> evolution with libav, I will not make efforts again for that. > > That's sad. Maybe someone who uses the libavfilter API from both forks can > help and mediate in this matter? Don't hold your breath, Libav interest in lavfi is very limited imho.
tks 2016-11-18 2:21 GMT-06:00 Paul B Mahol <onemda@gmail.com>: > On 11/17/16, Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> wrote: > > On 17.11.2016 17:27, Nicolas George wrote: > >> Le quartidi 24 brumaire, an CCXXV, Andreas Cadhalpun a ecrit : > >>> API changes in libavfilter are less of a problem, since not that many > >>> programs use it. For example, the codecpar change has orders of > magnitude > >>> more impact. > >> > >> I mostly agree. If people are ok with a sudden API change in lavfi, then > >> I can rework my patch for that. But I want confirmation beforehand. > > > > I don't think a sudden API change would be good. > > > >> A scheduled API change would be better, but I do not want to wait for it > >> to apply the patch, so you would have to accept a temporary solution. > > > > I understand that and think a temporary solution would be OK if it's > clear > > that it will be removed in say 2 years. > > > >>> However, ideally any incompatible API change should be coordinated with > >>> Libav. > >> > >> I have been spurned in the past in my attempts to coordinate lavfi > >> evolution with libav, I will not make efforts again for that. > > > > That's sad. Maybe someone who uses the libavfilter API from both forks > can > > help and mediate in this matter? > > Don't hold your breath, Libav interest in lavfi is very limited imho. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c index 662f933..9df4a1c 100644 --- a/libavfilter/avfilter.c +++ b/libavfilter/avfilter.c @@ -34,6 +34,7 @@ #include "libavutil/rational.h" #include "libavutil/samplefmt.h" +#include "private_fields.h" #include "audio.h" #include "avfilter.h" #include "formats.h" @@ -135,6 +136,10 @@ int avfilter_link(AVFilterContext *src, unsigned srcpad, { AVFilterLink *link; + av_assert0(src->graph); + av_assert0(dst->graph); + av_assert0(src->graph == dst->graph); + if (src->nb_outputs <= srcpad || dst->nb_inputs <= dstpad || src->outputs[srcpad] || dst->inputs[dstpad]) return AVERROR(EINVAL); @@ -147,6 +152,9 @@ int avfilter_link(AVFilterContext *src, unsigned srcpad, return AVERROR(EINVAL); } +#ifndef AVFILTER_LINK_INTERNAL_FIELDS +# error AVFilterLink internal fields not defined +#endif link = av_mallocz(sizeof(*link)); if (!link) return AVERROR(ENOMEM); @@ -160,6 +168,7 @@ 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); return 0; } @@ -170,6 +179,7 @@ void avfilter_link_free(AVFilterLink **link) return; av_frame_free(&(*link)->partial_buf); + ff_framequeue_free(&(*link)->fifo); ff_video_frame_pool_uninit((FFVideoFramePool**)&(*link)->video_frame_pool); av_freep(link); @@ -180,16 +190,46 @@ int avfilter_link_get_channels(AVFilterLink *link) return link->channels; } +static void ff_filter_set_ready(AVFilterContext *filter, unsigned priority) +{ + filter->ready = FFMAX(filter->ready, priority); +} + +/** + * Clear frame_blocked_in on all outputs. + * This is necessary whenever something changes on input. + */ +static void filter_unblock(AVFilterContext *filter) +{ + unsigned i; + + for (i = 0; i < filter->nb_outputs; i++) + filter->outputs[i]->frame_blocked_in = 0; +} + + void ff_avfilter_link_set_in_status(AVFilterLink *link, int status, int64_t pts) { - ff_avfilter_link_set_out_status(link, status, pts); + if (link->status_in == status) + return; + av_assert0(!link->status_in); + link->status_in = status; + link->status_in_pts = pts; + link->frame_wanted_out = 0; + link->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) { - link->status = status; - link->frame_wanted_in = link->frame_wanted_out = 0; - ff_update_link_current_pts(link, pts); + av_assert0(!link->frame_wanted_out); + av_assert0(!link->status_out); + link->status_out = status; + if (pts != AV_NOPTS_VALUE) + ff_update_link_current_pts(link, pts); + filter_unblock(link->dst); + ff_filter_set_ready(link->src, 200); } void avfilter_link_set_closed(AVFilterLink *link, int closed) @@ -370,10 +410,23 @@ int ff_request_frame(AVFilterLink *link) { FF_TPRINTF_START(NULL, request_frame); ff_tlog_link(NULL, link, 1); - if (link->status) - return link->status; - link->frame_wanted_in = 1; + if (link->status_out) + return link->status_out; + if (link->status_in) { + if (ff_framequeue_queued_frames(&link->fifo)) { + av_assert1(!link->frame_wanted_out); + av_assert1(link->dst->ready >= 300); + return 0; + } else { + /* 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; + } + } link->frame_wanted_out = 1; + ff_filter_set_ready(link->src, 100); return 0; } @@ -382,22 +435,17 @@ int ff_request_frame_to_filter(AVFilterLink *link) int ret = -1; FF_TPRINTF_START(NULL, request_frame_to_filter); ff_tlog_link(NULL, link, 1); - link->frame_wanted_in = 0; + /* Assume the filter is blocked, let the method clear it if not */ + link->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 == AVERROR_EOF && link->partial_buf) { - AVFrame *pbuf = link->partial_buf; - link->partial_buf = NULL; - ret = ff_filter_frame_framed(link, pbuf); - ff_avfilter_link_set_in_status(link, AVERROR_EOF, AV_NOPTS_VALUE); - link->frame_wanted_out = 0; - return ret; - } if (ret < 0) { - if (ret != AVERROR(EAGAIN) && ret != link->status) + if (ret != AVERROR(EAGAIN) && ret != link->status_in) ff_avfilter_link_set_in_status(link, ret, AV_NOPTS_VALUE); + if (ret == AVERROR_EOF) + ret = 0; } return ret; } @@ -1056,11 +1104,6 @@ static int ff_filter_frame_framed(AVFilterLink *link, AVFrame *frame) AVFilterCommand *cmd= link->dst->command_queue; int64_t pts; - if (link->status) { - av_frame_free(&frame); - return link->status; - } - if (!(filter_frame = dst->filter_frame)) filter_frame = default_filter_frame; @@ -1142,52 +1185,9 @@ fail: return ret; } -static int ff_filter_frame_needs_framing(AVFilterLink *link, AVFrame *frame) -{ - int insamples = frame->nb_samples, inpos = 0, nb_samples; - AVFrame *pbuf = link->partial_buf; - int nb_channels = av_frame_get_channels(frame); - int ret = 0; - - /* Handle framing (min_samples, max_samples) */ - while (insamples) { - if (!pbuf) { - AVRational samples_tb = { 1, link->sample_rate }; - pbuf = ff_get_audio_buffer(link, link->partial_buf_size); - if (!pbuf) { - av_log(link->dst, AV_LOG_WARNING, - "Samples dropped due to memory allocation failure.\n"); - return 0; - } - av_frame_copy_props(pbuf, frame); - pbuf->pts = frame->pts; - if (pbuf->pts != AV_NOPTS_VALUE) - pbuf->pts += av_rescale_q(inpos, samples_tb, link->time_base); - pbuf->nb_samples = 0; - } - nb_samples = FFMIN(insamples, - link->partial_buf_size - pbuf->nb_samples); - av_samples_copy(pbuf->extended_data, frame->extended_data, - pbuf->nb_samples, inpos, - nb_samples, nb_channels, link->format); - inpos += nb_samples; - insamples -= nb_samples; - pbuf->nb_samples += nb_samples; - if (pbuf->nb_samples >= link->min_samples) { - ret = ff_filter_frame_framed(link, pbuf); - pbuf = NULL; - } else { - if (link->frame_wanted_out) - link->frame_wanted_in = 1; - } - } - av_frame_free(&frame); - link->partial_buf = pbuf; - return ret; -} - int ff_filter_frame(AVFilterLink *link, AVFrame *frame) { + int ret; FF_TPRINTF_START(NULL, filter_frame); ff_tlog_link(NULL, link, 1); ff_tlog(NULL, " "); ff_tlog_ref(NULL, frame, 1); /* Consistency checks */ @@ -1220,23 +1220,329 @@ int ff_filter_frame(AVFilterLink *link, AVFrame *frame) } } - link->frame_wanted_out = 0; + link->frame_blocked_in = link->frame_wanted_out = 0; link->frame_count_in++; - /* Go directly to actual filtering if possible */ - if (link->type == AVMEDIA_TYPE_AUDIO && - link->min_samples && - (link->partial_buf || - frame->nb_samples < link->min_samples || - frame->nb_samples > link->max_samples)) { - return ff_filter_frame_needs_framing(link, frame); - } else { - return ff_filter_frame_framed(link, frame); + filter_unblock(link->dst); + ret = ff_framequeue_add(&link->fifo, frame); + if (ret < 0) { + av_frame_free(&frame); + return ret; } + ff_filter_set_ready(link->dst, 300); + return 0; + error: av_frame_free(&frame); return AVERROR_PATCHWELCOME; } +static int samples_ready(AVFilterLink *link) +{ + return ff_framequeue_queued_frames(&link->fifo) && + (ff_framequeue_queued_samples(&link->fifo) >= link->min_samples || + link->status_in); +} + +static int take_samples(AVFilterLink *link, unsigned min, unsigned max, + AVFrame **rframe) +{ + AVFrame *frame0, *frame, *buf; + unsigned nb_samples, nb_frames, i, p; + int ret; + + /* Note: this function relies on no format changes and must only be + called with enough samples. */ + av_assert1(samples_ready(link)); + frame0 = frame = ff_framequeue_peek(&link->fifo, 0); + if (frame->nb_samples >= min && frame->nb_samples < max) { + *rframe = ff_framequeue_take(&link->fifo); + return 0; + } + nb_frames = 0; + nb_samples = 0; + while (1) { + if (nb_samples + frame->nb_samples > max) { + if (nb_samples < min) + nb_samples = max; + break; + } + nb_samples += frame->nb_samples; + nb_frames++; + if (nb_frames == ff_framequeue_queued_frames(&link->fifo)) + break; + frame = ff_framequeue_peek(&link->fifo, nb_frames); + } + + buf = ff_get_audio_buffer(link, nb_samples); + if (!buf) + return AVERROR(ENOMEM); + ret = av_frame_copy_props(buf, frame0); + if (ret < 0) { + av_frame_free(&buf); + return ret; + } + buf->pts = frame0->pts; + + p = 0; + for (i = 0; i < nb_frames; i++) { + frame = ff_framequeue_take(&link->fifo); + av_samples_copy(buf->extended_data, frame->extended_data, p, 0, + frame->nb_samples, link->channels, link->format); + p += frame->nb_samples; + } + if (p < nb_samples) { + unsigned n = nb_samples - p; + frame = ff_framequeue_peek(&link->fifo, 0); + av_samples_copy(buf->extended_data, frame->extended_data, p, 0, n, + link->channels, link->format); + frame->nb_samples -= n; + av_samples_copy(frame->extended_data, frame->extended_data, 0, n, + frame->nb_samples, link->channels, link->format); + if (frame->pts != AV_NOPTS_VALUE) + frame->pts += av_rescale_q(n, av_make_q(1, link->sample_rate), link->time_base); + ff_framequeue_update_peeked(&link->fifo, 0); + ff_framequeue_skip_samples(&link->fifo, n); + } + + *rframe = buf; + return 0; +} + +int ff_filter_frame_to_filter(AVFilterLink *link) +{ + AVFrame *frame; + AVFilterContext *dst = link->dst; + int ret; + + av_assert1(ff_framequeue_queued_frames(&link->fifo)); + if (link->min_samples) { + int min = link->min_samples; + if (link->status_in) + min = FFMIN(min, ff_framequeue_queued_samples(&link->fifo)); + ret = take_samples(link, min, link->max_samples, &frame); + if (ret < 0) + return ret; + } else { + frame = ff_framequeue_take(&link->fifo); + } + /* The filter will soon have received a new frame, that may allow it to + produce one or more: unblock its outputs. */ + filter_unblock(dst); + ret = ff_filter_frame_framed(link, frame); + if (ret < 0 && ret != link->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 + the input status has also changed, or any other reason. */ + ff_filter_set_ready(dst, 300); + } + return ret; +} + +static int forward_status_change(AVFilterContext *filter, AVFilterLink *in) +{ + unsigned out = 0, progress = 0; + int ret; + + av_assert0(!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) { + progress++; + ret = ff_request_frame_to_filter(filter->outputs[out]); + if (ret < 0) + return ret; + } + if (++out == filter->nb_outputs) { + 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); + return 0; + } + progress = 0; + out = 0; + } + } + ff_filter_set_ready(filter, 200); + return 0; +} + +#define FFERROR_NOT_READY FFERRTAG('N','R','D','Y') + +static int ff_filter_activate_default(AVFilterContext *filter) +{ + unsigned i; + + for (i = 0; i < filter->nb_inputs; i++) { + if (samples_ready(filter->inputs[i])) { + 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]); + } + } + for (i = 0; i < filter->nb_outputs; i++) { + if (filter->outputs[i]->frame_wanted_out && + !filter->outputs[i]->frame_blocked_in) { + return ff_request_frame_to_filter(filter->outputs[i]); + } + } + return FFERROR_NOT_READY; +} + +/* + Filter scheduling and activation + + When a filter is activated, it must: + - if possible, output a frame; + - else, if relevant, forward the input status change; + - else, check outputs for wanted frames and forward the requests. + + The following AVFilterLink fields are used for activation: + + - frame_wanted_out: + + This field indicates if a frame is needed on this input of the + destination filter. A positive value indicates that a frame is needed + to process queued frames or internal data or to satisfy the + application; a zero value indicates that a frame is not especially + needed but could be processed anyway; a negative value indicates that a + frame would just be queued. + + It is set by filters using ff_request_frame() or ff_request_no_frame(), + when requested by the application through a specific API or when it is + set on one of the outputs. + + It is cleared when a frame is sent from the source using + ff_filter_frame(). + + It is also cleared when a status change is sent from the source using + ff_avfilter_link_set_in_status(). + + - frame_blocked_in: + + This field means that the source filter can not generate a frame as is. + Its goal is to avoid repeatedly calling the request_frame() method on + the same link. + + It is set by the framework on all outputs of a filter before activating it. + + It is automatically cleared by ff_filter_frame(). + + It is also automatically cleared by ff_avfilter_link_set_in_status(). + + It is also cleared on all outputs (using filter_unblock()) when + something happens on an input: processing a frame or changing the + status. + + - fifo: + + Contains the frames queued on a filter input. If it contains frames and + frame_wanted_out is not set, then the filter can be activated. If that + result in the filter not able to use these frames, the filter must set + frame_wanted_out to ask for more frames. + + - status_in and status_in_pts: + + Status (EOF or error code) of the link and timestamp of the status + change (in link time base, same as frames) as seen from the input of + the link. The status change is considered happening after the frames + queued in fifo. + + It is set by the source filter using ff_avfilter_link_set_in_status(). + + - status_out: + + Status of the link as seen from the output of the link. The status + change is considered having already happened. + + It is set by the destination filter using + ff_avfilter_link_set_out_status(). + + Filters are activated according to the ready field, set using the + ff_filter_set_ready(). Eventually, a priority queue will be used. + ff_filter_set_ready() is called whenever anything could cause progress to + be possible. Marking a filter ready when it is not is not a problem, + except for the small overhead it causes. + + Conditions that cause a filter to be marked ready are: + + - frames added on an input link; + + - changes in the input or output status of an input link; + + - requests for a frame on an output link; + + - after any actual processing using the legacy methods (filter_frame(), + and request_frame() to acknowledge status changes), to run once more + and check if enough input was present for several frames. + + Exemples of scenarios to consider: + + - buffersrc: activate if frame_wanted_out to notify the application; + activate when the application adds a frame to push it immediately. + + - testsrc: activate only if frame_wanted_out to produce and push a frame. + + - concat (not at stitch points): can process a frame on any output. + Activate if frame_wanted_out on output to forward on the corresponding + input. Activate when a frame is present on input to process it + immediately. + + - framesync: needs at least one frame on each input; extra frames on the + wrong input will accumulate. When a frame is first added on one input, + set frame_wanted_out<0 on it to avoid getting more (would trigger + testsrc) and frame_wanted_out>0 on the other to allow processing it. + + Activation of old filters: + + In order to activate a filter implementing the legacy filter_frame() and + request_frame() methods, perform the first possible of the following + actions: + + - If an input has frames in fifo and frame_wanted_out == 0, dequeue a + frame and call filter_frame(). + + Ratinale: filter frames as soon as possible instead of leaving them + queued; frame_wanted_out < 0 is not possible since the old API does not + set it nor provides any similar feedback; frame_wanted_out > 0 happens + when min_samples > 0 and there are not enough samples queued. + + - If an input has status_in set but not status_out, try to call + request_frame() on one of the outputs in the hope that it will trigger + request_frame() on the input with status_in and acknowledge it. This is + awkward and fragile, filters with several inputs or outputs should be + updated to direct activation as soon as possible. + + - If an output has frame_wanted_out > 0 and not frame_blocked_in, call + request_frame(). + + Rationale: checking frame_blocked_in is necessary to avoid requesting + repeatedly on a blocked input if another is not blocked (example: + [buffersrc1][testsrc1][buffersrc2][testsrc2]concat=v=2). + + TODO: respect needs_fifo and remove auto-inserted fifos. + + */ + +int ff_filter_activate(AVFilterContext *filter) +{ + int ret; + + filter->ready = 0; + ret = ff_filter_activate_default(filter); + if (ret == FFERROR_NOT_READY) + ret = 0; + return ret; +} + const AVClass *avfilter_get_class(void) { return &avfilter_class; diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h index d21b144..3629d5b 100644 --- a/libavfilter/avfilter.h +++ b/libavfilter/avfilter.h @@ -368,6 +368,13 @@ struct AVFilterContext { * Overrides global number of threads set per filter graph. */ int nb_threads; + + /** + * Ready status of the filter. + * A non-0 value means that the filter needs activating; + * a higher value suggests a more urgent activation. + */ + unsigned ready; }; /** @@ -509,18 +516,6 @@ struct AVFilterLink { int max_samples; /** - * Link status. - * If not zero, all attempts of filter_frame or request_frame - * will fail with the corresponding code, and if necessary the reference - * will be destroyed. - * If request_frame returns an error, the status is set on the - * corresponding link. - * It can be set also be set by either the source or the destination - * filter. - */ - int status; - - /** * Number of channels. */ int channels; @@ -541,13 +536,6 @@ struct AVFilterLink { void *video_frame_pool; /** - * True if a frame is currently wanted on the input of this filter. - * Set when ff_request_frame() is called by the output, - * cleared when the request is handled or forwarded. - */ - int frame_wanted_in; - - /** * True if a frame is currently wanted on the output of this filter. * Set when ff_request_frame() is called by the output, * cleared when a frame is filtered. @@ -559,6 +547,11 @@ struct AVFilterLink { * AVHWFramesContext describing the frames. */ AVBufferRef *hw_frames_ctx; + +#ifdef AVFILTER_LINK_INTERNAL_FIELDS + AVFILTER_LINK_INTERNAL_FIELDS +#endif + }; /** diff --git a/libavfilter/avfiltergraph.c b/libavfilter/avfiltergraph.c index 3af698d..0a6431f 100644 --- a/libavfilter/avfiltergraph.c +++ b/libavfilter/avfiltergraph.c @@ -32,6 +32,7 @@ #include "libavutil/opt.h" #include "libavutil/pixdesc.h" +#include "private_fields.h" #include "avfilter.h" #include "formats.h" #include "internal.h" @@ -87,6 +88,7 @@ AVFilterGraph *avfilter_graph_alloc(void) ret->av_class = &filtergraph_class; av_opt_set_defaults(ret); + ff_framequeue_global_init(&ret->internal->frame_queues); return ret; } @@ -1377,10 +1379,10 @@ void ff_avfilter_graph_update_heap(AVFilterGraph *graph, AVFilterLink *link) heap_bubble_down(graph, link, link->age_index); } - int avfilter_graph_request_oldest(AVFilterGraph *graph) { AVFilterLink *oldest = graph->sink_links[0]; + int64_t frame_count; int r; while (graph->sink_links_count) { @@ -1400,7 +1402,8 @@ int avfilter_graph_request_oldest(AVFilterGraph *graph) if (!graph->sink_links_count) return AVERROR_EOF; av_assert1(oldest->age_index >= 0); - while (oldest->frame_wanted_out) { + frame_count = oldest->frame_count_out; + while (frame_count == oldest->frame_count_out) { r = ff_filter_graph_run_once(graph); if (r < 0) return r; @@ -1408,41 +1411,17 @@ int avfilter_graph_request_oldest(AVFilterGraph *graph) return 0; } -static AVFilterLink *graph_run_once_find_filter(AVFilterGraph *graph) -{ - unsigned i, j; - AVFilterContext *f; - - /* TODO: replace scanning the graph with a priority list */ - for (i = 0; i < graph->nb_filters; i++) { - f = graph->filters[i]; - for (j = 0; j < f->nb_outputs; j++) - if (f->outputs[j]->frame_wanted_in) - return f->outputs[j]; - } - for (i = 0; i < graph->nb_filters; i++) { - f = graph->filters[i]; - for (j = 0; j < f->nb_outputs; j++) - if (f->outputs[j]->frame_wanted_out) - return f->outputs[j]; - } - return NULL; -} - int ff_filter_graph_run_once(AVFilterGraph *graph) { - AVFilterLink *link; - int ret; - - link = graph_run_once_find_filter(graph); - if (!link) { - av_log(NULL, AV_LOG_WARNING, "Useless run of a filter graph\n"); + AVFilterContext *filter; + unsigned i; + + av_assert0(graph->nb_filters); + filter = graph->filters[0]; + for (i = 1; i < graph->nb_filters; i++) + if (graph->filters[i]->ready > filter->ready) + filter = graph->filters[i]; + if (!filter->ready) return AVERROR(EAGAIN); - } - ret = ff_request_frame_to_filter(link); - if (ret == AVERROR_EOF) - /* local EOF will be forwarded through request_frame() / - set_status() until it reaches the sink */ - ret = 0; - return ret < 0 ? ret : 1; + return ff_filter_activate(filter); } diff --git a/libavfilter/buffersink.c b/libavfilter/buffersink.c index 2feb56d..e0af091 100644 --- a/libavfilter/buffersink.c +++ b/libavfilter/buffersink.c @@ -31,6 +31,7 @@ #include "libavutil/mathematics.h" #include "libavutil/opt.h" +#include "private_fields.h" #include "audio.h" #include "avfilter.h" #include "buffersink.h" @@ -129,18 +130,26 @@ int attribute_align_arg av_buffersink_get_frame_flags(AVFilterContext *ctx, AVFr { BufferSinkContext *buf = ctx->priv; AVFilterLink *inlink = ctx->inputs[0]; - int ret; + int peek_in_framequeue = 0, ret; + int64_t frame_count; AVFrame *cur_frame; /* no picref available, fetch it from the filterchain */ while (!av_fifo_size(buf->fifo)) { - if (inlink->status) - return inlink->status; - if (flags & AV_BUFFERSINK_FLAG_NO_REQUEST) + /* if peek_in_framequeue is true later, then ff_request_frame() and + the ff_filter_graph_run_once() loop will take a frame from it and + move it to the internal fifo, ending the global loop */ + av_assert0(!peek_in_framequeue); + if (inlink->status_out) + return inlink->status_out; + peek_in_framequeue = ff_framequeue_queued_frames(&inlink->fifo) && + ff_framequeue_queued_samples(&inlink->fifo) >= inlink->min_samples; + if ((flags & AV_BUFFERSINK_FLAG_NO_REQUEST) && !peek_in_framequeue) return AVERROR(EAGAIN); if ((ret = ff_request_frame(inlink)) < 0) return ret; - while (inlink->frame_wanted_out) { + frame_count = inlink->frame_count_out; + while (frame_count == inlink->frame_count_out) { ret = ff_filter_graph_run_once(ctx->graph); if (ret < 0) return ret; diff --git a/libavfilter/buffersrc.c b/libavfilter/buffersrc.c index 9294811..1314397 100644 --- a/libavfilter/buffersrc.c +++ b/libavfilter/buffersrc.c @@ -184,6 +184,7 @@ static int av_buffersrc_add_frame_internal(AVFilterContext *ctx, if (!frame) { s->eof = 1; + ff_avfilter_link_set_in_status(ctx->outputs[0], AVERROR_EOF, AV_NOPTS_VALUE); return 0; } else if (s->eof) return AVERROR(EINVAL); @@ -235,9 +236,8 @@ static int av_buffersrc_add_frame_internal(AVFilterContext *ctx, return ret; } - if ((flags & AV_BUFFERSRC_FLAG_PUSH)) - if ((ret = ctx->output_pads[0].request_frame(ctx->outputs[0])) < 0) - return ret; + if ((ret = ctx->output_pads[0].request_frame(ctx->outputs[0])) < 0) + return ret; return 0; } diff --git a/libavfilter/f_interleave.c b/libavfilter/f_interleave.c index 422f2bf..44ee11b 100644 --- a/libavfilter/f_interleave.c +++ b/libavfilter/f_interleave.c @@ -26,6 +26,7 @@ #include "libavutil/avassert.h" #include "libavutil/avstring.h" #include "libavutil/opt.h" +#include "private_fields.h" #include "avfilter.h" #include "bufferqueue.h" #include "formats.h" @@ -59,7 +60,7 @@ inline static int push_frame(AVFilterContext *ctx) for (i = 0; i < ctx->nb_inputs; i++) { struct FFBufQueue *q = &s->queues[i]; - if (!q->available && !ctx->inputs[i]->status) + if (!q->available && !ctx->inputs[i]->status_out) return 0; if (q->available) { frame = ff_bufqueue_peek(q, 0); @@ -190,7 +191,7 @@ static int request_frame(AVFilterLink *outlink) int i, ret; for (i = 0; i < ctx->nb_inputs; i++) { - if (!s->queues[i].available && !ctx->inputs[i]->status) { + if (!s->queues[i].available && !ctx->inputs[i]->status_out) { ret = ff_request_frame(ctx->inputs[i]); if (ret != AVERROR_EOF) return ret; diff --git a/libavfilter/internal.h b/libavfilter/internal.h index 3856012..a8b69fd 100644 --- a/libavfilter/internal.h +++ b/libavfilter/internal.h @@ -29,6 +29,7 @@ #include "avfiltergraph.h" #include "formats.h" #include "framepool.h" +#include "framequeue.h" #include "thread.h" #include "version.h" #include "video.h" @@ -147,6 +148,7 @@ struct AVFilterPad { struct AVFilterGraphInternal { void *thread; avfilter_execute_func *thread_execute; + FFFrameQueueGlobal frame_queues; }; struct AVFilterInternal { @@ -336,6 +338,8 @@ int ff_request_frame(AVFilterLink *link); int ff_request_frame_to_filter(AVFilterLink *link); +int ff_filter_frame_to_filter(AVFilterLink *link); + #define AVFILTER_DEFINE_CLASS(fname) \ static const AVClass fname##_class = { \ .class_name = #fname, \ @@ -376,6 +380,8 @@ int ff_filter_frame(AVFilterLink *link, AVFrame *frame); */ AVFilterContext *ff_filter_alloc(const AVFilter *filter, const char *inst_name); +int ff_filter_activate(AVFilterContext *filter); + /** * Remove a filter from a graph; */ diff --git a/libavfilter/private_fields.h b/libavfilter/private_fields.h new file mode 100644 index 0000000..5a3df68 --- /dev/null +++ b/libavfilter/private_fields.h @@ -0,0 +1,43 @@ +/* + * Generic frame queue + * Copyright (c) 2016 Nicolas George + * + * 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 + */ + +#include "framequeue.h" + +#define AVFILTER_LINK_INTERNAL_FIELDS \ +\ + FFFrameQueue fifo; \ +\ + int frame_blocked_in; \ +\ + /** \ + * Link input status. \ + */ \ + int status_in; \ + 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; \ + \ +/* define AVFILTER_LINK_INTERNAL_FIELDS */ diff --git a/libavfilter/split.c b/libavfilter/split.c index 6cf4ab1..89e3604 100644 --- a/libavfilter/split.c +++ b/libavfilter/split.c @@ -30,6 +30,7 @@ #include "libavutil/mem.h" #include "libavutil/opt.h" +#include "private_fields.h" #include "avfilter.h" #include "audio.h" #include "formats.h" @@ -78,7 +79,7 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *frame) for (i = 0; i < ctx->nb_outputs; i++) { AVFrame *buf_out; - if (ctx->outputs[i]->status) + if (ctx->outputs[i]->status_in) continue; buf_out = av_frame_clone(frame); if (!buf_out) { diff --git a/libavfilter/tests/filtfmts.c b/libavfilter/tests/filtfmts.c index 46a2d94..682af8c 100644 --- a/libavfilter/tests/filtfmts.c +++ b/libavfilter/tests/filtfmts.c @@ -25,6 +25,7 @@ #include "libavutil/pixdesc.h" #include "libavutil/samplefmt.h" +#include "libavfilter/private_fields.h" #include "libavfilter/avfilter.h" #include "libavfilter/formats.h" diff --git a/libavfilter/vf_extractplanes.c b/libavfilter/vf_extractplanes.c index a23f838..c4b01e0 100644 --- a/libavfilter/vf_extractplanes.c +++ b/libavfilter/vf_extractplanes.c @@ -22,6 +22,7 @@ #include "libavutil/imgutils.h" #include "libavutil/opt.h" #include "libavutil/pixdesc.h" +#include "private_fields.h" #include "avfilter.h" #include "drawutils.h" #include "internal.h" @@ -245,7 +246,7 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *frame) const int idx = s->map[i]; AVFrame *out; - if (outlink->status) + if (outlink->status_in) continue; out = ff_get_video_buffer(outlink, outlink->w, outlink->h); diff --git a/tests/ref/fate/source b/tests/ref/fate/source index 4ee8a58..23ffd8a 100644 --- a/tests/ref/fate/source +++ b/tests/ref/fate/source @@ -29,3 +29,4 @@ compat/cuda/nvcuvid.h compat/float/float.h compat/float/limits.h compat/nvenc/nvEncodeAPI.h +libavfilter/private_fields.h
A lot of changes happen at the same time: - Add private_fields.h to devine private fields in AVFilterLink. It allows to have the private fields directly in structured types without needing an indirection for an opaque structure nor the structure definition in public headers. - Add a framequeue fifo to AVFilterLink. - split AVFilterLink.status into status_in and status_out: requires changes to the few filters and programs that use it directly (f_interleave, split, filtfmts). - Add a field ready to AVFilterContext, marking when the filter is ready and its activation priority. - Add flags to mark blocked links. - Change ff_filter_frame() to enqueue the frame. - Change all filtering functions to update the ready field and the blocked flags. - Update ff_filter_graph_run_once() to use the ready field. - buffersrc: always push the frame immediately. Signed-off-by: Nicolas George <george@nsup.org> --- libavfilter/avfilter.c | 458 ++++++++++++++++++++++++++++++++++------- libavfilter/avfilter.h | 31 ++- libavfilter/avfiltergraph.c | 51 ++--- libavfilter/buffersink.c | 19 +- libavfilter/buffersrc.c | 6 +- libavfilter/f_interleave.c | 5 +- libavfilter/internal.h | 6 + libavfilter/private_fields.h | 43 ++++ libavfilter/split.c | 3 +- libavfilter/tests/filtfmts.c | 1 + libavfilter/vf_extractplanes.c | 3 +- tests/ref/fate/source | 1 + 12 files changed, 484 insertions(+), 143 deletions(-) create mode 100644 libavfilter/private_fields.h