| Message ID | 20161227180210.23100-6-george@nsup.org |
|---|---|
| State | New |
| Headers | show |
On 12/27/2016 3:02 PM, Nicolas George wrote: > AVFilterLink contains fields of internal types that are better > isolated. Furthermore, with the prospect of inter-filters > threading, it is better if filters do not access links directly. > > Signed-off-by: Nicolas George <george@nsup.org> > --- > libavfilter/Makefile | 1 + > libavfilter/avfilter.h | 217 +-------------------------------------- > libavfilter/avfilterlink.h | 248 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 250 insertions(+), 216 deletions(-) > create mode 100644 libavfilter/avfilterlink.h > > > That way, making it public or private is just a matter of changing one line > in the Makefile. > [...] > diff --git a/libavfilter/avfilterlink.h b/libavfilter/avfilterlink.h > new file mode 100644 > index 0000000000..51ab322ae9 > --- /dev/null > +++ b/libavfilter/avfilterlink.h > @@ -0,0 +1,248 @@ > +/* > + * AVFilterLink definition > + * Copyright (c) 2007 Bobby Bingham > + * > + * 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_AVFILTERLINK_H > +#define AVFILTER_AVFILTERLINK_H > + > +#include "avfilter.h" > + > +#ifdef FF_INTERNAL_FIELDS > +# include "framequeue.h" > +#endif > + > +/** > + * A link between two filters. This contains pointers to the source and > + * destination filters between which this link exists, and the indexes of > + * the pads involved. In addition, this link also contains the parameters > + * which have been negotiated and agreed upon between the filter, such as > + * image dimensions, format, etc. > + */ > +struct AVFilterLink { > + AVFilterContext *src; ///< source filter > + AVFilterPad *srcpad; ///< output pad on the source filter > + > + AVFilterContext *dst; ///< dest filter > + AVFilterPad *dstpad; ///< input pad on the dest filter > + > + enum AVMediaType type; ///< filter media type > + > + /* These parameters apply only to video */ > + int w; ///< agreed upon image width > + int h; ///< agreed upon image height > + AVRational sample_aspect_ratio; ///< agreed upon sample aspect ratio > + /* These parameters apply only to audio */ > + uint64_t channel_layout; ///< channel layout of current buffer (see libavutil/channel_layout.h) > + int sample_rate; ///< samples per second > + > + int format; ///< agreed upon media format > + > + /** > + * Define the time base used by the PTS of the frames/samples > + * which will pass through this link. > + * During the configuration stage, each filter is supposed to > + * change only the output timebase, while the timebase of the > + * input link is assumed to be an unchangeable property. > + */ > + AVRational time_base; > + > + /***************************************************************** > + * All fields below this line are not part of the public API. They > + * may not be used outside of libavfilter and can be changed and > + * removed at will. > + * New public fields should be added right above. > + ***************************************************************** > + */ > + /** > + * Lists of formats and channel layouts supported by the input and output > + * filters respectively. These lists are used for negotiating the format > + * to actually be used, which will be loaded into the format and > + * channel_layout members, above, when chosen. > + * > + */ > + AVFilterFormats *in_formats; > + AVFilterFormats *out_formats; > + > + /** > + * Lists of channel layouts and sample rates used for automatic > + * negotiation. > + */ > + AVFilterFormats *in_samplerates; > + AVFilterFormats *out_samplerates; > + struct AVFilterChannelLayouts *in_channel_layouts; > + struct AVFilterChannelLayouts *out_channel_layouts; > + > + /** > + * Audio only, the destination filter sets this to a non-zero value to > + * request that buffers with the given number of samples should be sent to > + * it. AVFilterPad.needs_fifo must also be set on the corresponding input > + * pad. > + * Last buffer before EOF will be padded with silence. > + */ > + int request_samples; > + > + /** stage of the initialization of the link properties (dimensions, etc) */ > + enum { > + AVLINK_UNINIT = 0, ///< not started > + AVLINK_STARTINIT, ///< started, but incomplete > + AVLINK_INIT ///< complete > + } init_state; > + > + /** > + * Graph the filter belongs to. > + */ > + struct AVFilterGraph *graph; > + > + /** > + * Current timestamp of the link, as defined by the most recent > + * frame(s), in link time_base units. > + */ > + int64_t current_pts; > + > + /** > + * Current timestamp of the link, as defined by the most recent > + * frame(s), in AV_TIME_BASE units. > + */ > + int64_t current_pts_us; > + > + /** > + * Index in the age array. > + */ > + int age_index; > + > + /** > + * Frame rate of the stream on the link, or 1/0 if unknown or variable; > + * if left to 0/0, will be automatically copied from the first input > + * of the source filter if it exists. > + * > + * Sources should set it to the best estimation of the real frame rate. > + * If the source frame rate is unknown or variable, set this to 1/0. > + * Filters should update it if necessary depending on their function. > + * Sinks can use it to set a default output frame rate. > + * It is similar to the r_frame_rate field in AVStream. > + */ > + AVRational frame_rate; > + > + /** > + * Buffer partially filled with samples to achieve a fixed/minimum size. > + */ > + AVFrame *partial_buf; > + > + /** > + * Size of the partial buffer to allocate. > + * Must be between min_samples and max_samples. > + */ > + int partial_buf_size; > + > + /** > + * Minimum number of samples to filter at once. If filter_frame() is > + * called with fewer samples, it will accumulate them in partial_buf. > + * This field and the related ones must not be changed after filtering > + * has started. > + * If 0, all related fields are ignored. > + */ > + int min_samples; > + > + /** > + * Maximum number of samples to filter at once. If filter_frame() is > + * called with more samples, it will split them. > + */ > + int max_samples; > + > + /** > + * Number of channels. > + */ > + int channels; > + > + /** > + * Link processing flags. > + */ > + unsigned flags; > + > + /** > + * Number of past frames sent through the link. > + */ > + int64_t frame_count_in, frame_count_out; > + > + /** > + * A pointer to a FFVideoFramePool struct. > + */ > + void *video_frame_pool; > + > + /** > + * 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. > + */ > + int frame_wanted_out; > + > + /** > + * For hwaccel pixel formats, this should be a reference to the > + * 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. How is this any different than the above notice? Everything below time_base is also meant for internal use and shouldn't be accessed by applications or other ffmpeg libraries. Is extending the scope of FF_INTERNAL_FIELDS to cover the above fields your intention for the next major bump? > + */ > + char reserved[0xF000]; What's the point in reserving space like this? And isn't 60kb a bit overkill? I insist this is just ugly and unconventional. > + > +#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 */ > + > +}; > + > +#endif /* AVFILTER_AVFILTERLINK_H */ >
Le septidi 7 nivôse, an CCXXV, James Almer a écrit : > How is this any different than the above notice? Everything below time_base > is also meant for internal use and shouldn't be accessed by applications or > other ffmpeg libraries. > Is extending the scope of FF_INTERNAL_FIELDS to cover the above fields your > intention for the next major bump? > What's the point in reserving space like this? And isn't 60kb a bit overkill? > > I insist this is just ugly and unconventional. You are late, this is not new code, just moved. Regards,
On 12/27/2016 3:38 PM, Nicolas George wrote: > Le septidi 7 nivôse, an CCXXV, James Almer a écrit : >> How is this any different than the above notice? Everything below time_base >> is also meant for internal use and shouldn't be accessed by applications or >> other ffmpeg libraries. >> Is extending the scope of FF_INTERNAL_FIELDS to cover the above fields your >> intention for the next major bump? > >> What's the point in reserving space like this? And isn't 60kb a bit overkill? >> >> I insist this is just ugly and unconventional. > > You are late, this is not new code, just moved. It may not be something added with this patchset, but It's pretty much new and recent code, questioned by more people than just me prior to it being pushed. I think making questions and complaining about its ugly and unconventional nature is something i'm allowed to do, especially when it was pushed while blocking technical reviews were purposely unaddressed at the time. It's clear you're on the defensive and think I'm just attacking you for the sake of it, so trying to discuss with you will be pointless. Nothing good will come out of this at this point, from either of us.
On Tue, Dec 27, 2016 at 07:02:07PM +0100, Nicolas George wrote: > AVFilterLink contains fields of internal types that are better > isolated. Furthermore, with the prospect of inter-filters > threading, it is better if filters do not access links directly. > > Signed-off-by: Nicolas George <george@nsup.org> > --- > libavfilter/Makefile | 1 + > libavfilter/avfilter.h | 217 +-------------------------------------- > libavfilter/avfilterlink.h | 248 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 250 insertions(+), 216 deletions(-) > create mode 100644 libavfilter/avfilterlink.h > > > That way, making it public or private is just a matter of changing one line > in the Makefile. LGTM thx [...]
Le septidi 7 nivôse, an CCXXV, James Almer a écrit : > It may not be something added with this patchset, but It's pretty much new and > recent code, questioned by more people than just me prior to it being pushed. There was a discussion, then there was a consensus. > It's clear you're on the defensive and think I'm just attacking you for the sake > of it, so trying to discuss with you will be pointless. Self-fulfilling prophecy. Regards,
On 12/27/2016 5:09 PM, Nicolas George wrote: > Le septidi 7 nivôse, an CCXXV, James Almer a écrit : >> It may not be something added with this patchset, but It's pretty much new and >> recent code, questioned by more people than just me prior to it being pushed. > > There was a discussion, then there was a consensus. Where have i heard this before? Something something ffserver. Not to mention it's a lie. There was a review you purposely (and proudly!) ignored. I think that's as far as "consensus" as it can get. > >> It's clear you're on the defensive and think I'm just attacking you for the sake >> of it, so trying to discuss with you will be pointless. > > Self-fulfilling prophecy. I have no fucking idea what you're even trying to imply here. At this point i don't think you're even addressing what I'm saying, but what the evil version of myself that resides in your mind says instead. Learn to handle criticism to your code already and stop taking everything as a personal attack. It will help people tolerate you.
Le septidi 7 nivôse, an CCXXV, James Almer a écrit : > Where have i heard this before? Something something ffserver. Actually, for ffserver, it turned out that there was no decision at all. And for ffserver, the situation changed between the pretended decision and the revoke. No situation changed here. > Not to mention it's a lie. There was a review you purposely (and proudly!) ignored. > I think that's as far as "consensus" as it can get. The "review" you are referring to was for another patch entirely. I will see if you do the right thing about that in your next message. > >> It's clear you're on the defensive and think I'm just attacking you for the sake > >> of it, so trying to discuss with you will be pointless. > > Self-fulfilling prophecy. > I have no fucking idea what you're even trying to imply here. At this point i don't > think you're even addressing what I'm saying, but what the evil version of myself > that resides in your mind says instead. Usually, when someone says "trying to discuss with you will be pointless", the problem lies with the one who is saying that, not the other one. > Learn to handle criticism to your code already and stop taking everything as a > personal attack. It will help people tolerate you. I can take criticism, but I do not like nagging.
On 12/27/2016 5:40 PM, Nicolas George wrote: > Le septidi 7 nivôse, an CCXXV, James Almer a écrit : >> Where have i heard this before? Something something ffserver. > > Actually, for ffserver, it turned out that there was no decision at all. There sure was. A discussion, a decision, and a patch, all without opposition until months later when they were going to be made effective. Unlike some of these avfilter changes you're pushing, which went in despite having people questioning them. > > And for ffserver, the situation changed between the pretended decision > and the revoke. No situation changed here. > >> Not to mention it's a lie. There was a review you purposely (and proudly!) ignored. >> I think that's as far as "consensus" as it can get. > > The "review" you are referring to was for another patch entirely. > That patch was a dependency for the one we're talking about here. With the dependency blocked, the following one is just as blocked. Last i checked, you didn't re-implement it without the FFFrameQueue api to free it from said blockage. > I will see if you do the right thing about that in your next message. More "I'm right, you're wrong, and I'm simply letting you know it" attitude. That sure helps you argument. > >>>> It's clear you're on the defensive and think I'm just attacking you for the sake >>>> of it, so trying to discuss with you will be pointless. >>> Self-fulfilling prophecy. >> I have no fucking idea what you're even trying to imply here. At this point i don't >> think you're even addressing what I'm saying, but what the evil version of myself >> that resides in your mind says instead. > > Usually, when someone says "trying to discuss with you will be > pointless", the problem lies with the one who is saying that, not the > other one. I made three questions. All of them where ignored, and what i got instead was a "You are late, this is not new code". Do you honestly consider discussing with you will get us anywhere if that's your first reaction at a normal and completely valid reply to your patch? > >> Learn to handle criticism to your code already and stop taking everything as a >> personal attack. It will help people tolerate you. > > I can take criticism, but I do not like nagging. What nagging? You got dissenting opinions and technical concerns about your code from more than one developer. You in turn ignored and belittled most if not all of them, and those behind them as well. You're the only person on this list that constantly reacts like this. This project would be dead and lacking contributors if every single person handled reviews even remotely like you.
Le septidi 7 nivôse, an CCXXV, James Almer a écrit : > I made three questions. All of them where ignored, and what i got instead was a "You > are late, this is not new code". All three of them have answers in the original discussion. You can read it yourself. If you want more courteous answers from me, well, you should not have spent all week attacking me. The fact that you do not realize they are attacks compounds the problem. An apology for having accused me of lying would be a good start.
On 12/27/16, James Almer <jamrial@gmail.com> wrote: > On 12/27/2016 3:02 PM, Nicolas George wrote: >> AVFilterLink contains fields of internal types that are better >> isolated. Furthermore, with the prospect of inter-filters >> threading, it is better if filters do not access links directly. >> >> Signed-off-by: Nicolas George <george@nsup.org> >> --- >> libavfilter/Makefile | 1 + >> libavfilter/avfilter.h | 217 +-------------------------------------- >> libavfilter/avfilterlink.h | 248 >> +++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 250 insertions(+), 216 deletions(-) >> create mode 100644 libavfilter/avfilterlink.h >> >> >> That way, making it public or private is just a matter of changing one >> line >> in the Makefile. >> > > [...] > >> diff --git a/libavfilter/avfilterlink.h b/libavfilter/avfilterlink.h >> new file mode 100644 >> index 0000000000..51ab322ae9 >> --- /dev/null >> +++ b/libavfilter/avfilterlink.h >> @@ -0,0 +1,248 @@ >> +/* >> + * AVFilterLink definition >> + * Copyright (c) 2007 Bobby Bingham >> + * >> + * 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_AVFILTERLINK_H >> +#define AVFILTER_AVFILTERLINK_H >> + >> +#include "avfilter.h" >> + >> +#ifdef FF_INTERNAL_FIELDS >> +# include "framequeue.h" >> +#endif >> + >> +/** >> + * A link between two filters. This contains pointers to the source and >> + * destination filters between which this link exists, and the indexes of >> + * the pads involved. In addition, this link also contains the parameters >> + * which have been negotiated and agreed upon between the filter, such as >> + * image dimensions, format, etc. >> + */ >> +struct AVFilterLink { >> + AVFilterContext *src; ///< source filter >> + AVFilterPad *srcpad; ///< output pad on the source filter >> + >> + AVFilterContext *dst; ///< dest filter >> + AVFilterPad *dstpad; ///< input pad on the dest filter >> + >> + enum AVMediaType type; ///< filter media type >> + >> + /* These parameters apply only to video */ >> + int w; ///< agreed upon image width >> + int h; ///< agreed upon image height >> + AVRational sample_aspect_ratio; ///< agreed upon sample aspect ratio >> + /* These parameters apply only to audio */ >> + uint64_t channel_layout; ///< channel layout of current buffer >> (see libavutil/channel_layout.h) >> + int sample_rate; ///< samples per second >> + >> + int format; ///< agreed upon media format >> + >> + /** >> + * Define the time base used by the PTS of the frames/samples >> + * which will pass through this link. >> + * During the configuration stage, each filter is supposed to >> + * change only the output timebase, while the timebase of the >> + * input link is assumed to be an unchangeable property. >> + */ >> + AVRational time_base; >> + >> + /***************************************************************** >> + * All fields below this line are not part of the public API. They >> + * may not be used outside of libavfilter and can be changed and >> + * removed at will. >> + * New public fields should be added right above. >> + ***************************************************************** >> + */ >> + /** >> + * Lists of formats and channel layouts supported by the input and >> output >> + * filters respectively. These lists are used for negotiating the >> format >> + * to actually be used, which will be loaded into the format and >> + * channel_layout members, above, when chosen. >> + * >> + */ >> + AVFilterFormats *in_formats; >> + AVFilterFormats *out_formats; >> + >> + /** >> + * Lists of channel layouts and sample rates used for automatic >> + * negotiation. >> + */ >> + AVFilterFormats *in_samplerates; >> + AVFilterFormats *out_samplerates; >> + struct AVFilterChannelLayouts *in_channel_layouts; >> + struct AVFilterChannelLayouts *out_channel_layouts; >> + >> + /** >> + * Audio only, the destination filter sets this to a non-zero value >> to >> + * request that buffers with the given number of samples should be >> sent to >> + * it. AVFilterPad.needs_fifo must also be set on the corresponding >> input >> + * pad. >> + * Last buffer before EOF will be padded with silence. >> + */ >> + int request_samples; >> + >> + /** stage of the initialization of the link properties (dimensions, >> etc) */ >> + enum { >> + AVLINK_UNINIT = 0, ///< not started >> + AVLINK_STARTINIT, ///< started, but incomplete >> + AVLINK_INIT ///< complete >> + } init_state; >> + >> + /** >> + * Graph the filter belongs to. >> + */ >> + struct AVFilterGraph *graph; >> + >> + /** >> + * Current timestamp of the link, as defined by the most recent >> + * frame(s), in link time_base units. >> + */ >> + int64_t current_pts; >> + >> + /** >> + * Current timestamp of the link, as defined by the most recent >> + * frame(s), in AV_TIME_BASE units. >> + */ >> + int64_t current_pts_us; >> + >> + /** >> + * Index in the age array. >> + */ >> + int age_index; >> + >> + /** >> + * Frame rate of the stream on the link, or 1/0 if unknown or >> variable; >> + * if left to 0/0, will be automatically copied from the first input >> + * of the source filter if it exists. >> + * >> + * Sources should set it to the best estimation of the real frame >> rate. >> + * If the source frame rate is unknown or variable, set this to 1/0. >> + * Filters should update it if necessary depending on their function. >> + * Sinks can use it to set a default output frame rate. >> + * It is similar to the r_frame_rate field in AVStream. >> + */ >> + AVRational frame_rate; >> + >> + /** >> + * Buffer partially filled with samples to achieve a fixed/minimum >> size. >> + */ >> + AVFrame *partial_buf; >> + >> + /** >> + * Size of the partial buffer to allocate. >> + * Must be between min_samples and max_samples. >> + */ >> + int partial_buf_size; >> + >> + /** >> + * Minimum number of samples to filter at once. If filter_frame() is >> + * called with fewer samples, it will accumulate them in partial_buf. >> + * This field and the related ones must not be changed after >> filtering >> + * has started. >> + * If 0, all related fields are ignored. >> + */ >> + int min_samples; >> + >> + /** >> + * Maximum number of samples to filter at once. If filter_frame() is >> + * called with more samples, it will split them. >> + */ >> + int max_samples; >> + >> + /** >> + * Number of channels. >> + */ >> + int channels; >> + >> + /** >> + * Link processing flags. >> + */ >> + unsigned flags; >> + >> + /** >> + * Number of past frames sent through the link. >> + */ >> + int64_t frame_count_in, frame_count_out; >> + >> + /** >> + * A pointer to a FFVideoFramePool struct. >> + */ >> + void *video_frame_pool; >> + >> + /** >> + * 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. >> + */ >> + int frame_wanted_out; >> + >> + /** >> + * For hwaccel pixel formats, this should be a reference to the >> + * 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. > > How is this any different than the above notice? Everything below time_base > is also meant for internal use and shouldn't be accessed by applications or > other ffmpeg libraries. > Is extending the scope of FF_INTERNAL_FIELDS to cover the above fields your > intention for the next major bump? > >> + */ >> + char reserved[0xF000]; > > What's the point in reserving space like this? And isn't 60kb a bit > overkill? > > I insist this is just ugly and unconventional. > Yes, what's point of that?
On 12/27/2016 6:27 PM, Nicolas George wrote: > Le septidi 7 nivôse, an CCXXV, James Almer a écrit : >> I made three questions. All of them where ignored, and what i got instead was a "You >> are late, this is not new code". > > All three of them have answers in the original discussion. You can read > it yourself. If that's true, why didn't you point it out in your first reply? Was that dismissive answer you gave me how you handle people making questions that you know were already answered elsewhere? > If you want more courteous answers from me, well, you should not have > spent all week attacking me. The fact that you do not realize they are > attacks compounds the problem. No, I have heavily criticized your attitude and behavior, as have others. You should be able to see it for what it is. But at least i now have the confirmation your answers have been purposely aggressive. > > An apology for having accused me of lying would be a good start. I'm not going to apologize for stating facts. Especially when you cut them out in this latest reply of yours.
Le septidi 7 nivôse, an CCXXV, Paul B Mahol a écrit :
> Yes, what's point of that?
The answer is there:
https://ffmpeg.org/pipermail/ffmpeg-devel/2016-November/202772.html
Regards,
On 12/28/16, Nicolas George <george@nsup.org> wrote: > Le septidi 7 nivose, an CCXXV, Paul B Mahol a ecrit : >> Yes, what's point of that? > > The answer is there: > > https://ffmpeg.org/pipermail/ffmpeg-devel/2016-November/202772.html How that plays out with filtergraphs with 1000 filter links?
L'octidi 8 nivôse, an CCXXV, Paul B Mahol a écrit :
> How that plays out with filtergraphs with 1000 filter links?
I think you need to read the code more carefully to realize it does not
change anything at all. The "reserved[a lot]" field only makes a
difference for applications that incorrectly allocate an AVFilterLink
themselves.
Regards,
On Tue, 27 Dec 2016 21:09:26 +0100 Nicolas George <george@nsup.org> wrote: > Le septidi 7 nivôse, an CCXXV, James Almer a écrit : > > It may not be something added with this patchset, but It's pretty much new and > > recent code, questioned by more people than just me prior to it being pushed. > > There was a discussion, then there was a consensus. As we recently established with the ffserver discussion, this means nothing in ffmpeg development.
diff --git a/libavfilter/Makefile b/libavfilter/Makefile index 9ab65eb891..f4e8893b3c 100644 --- a/libavfilter/Makefile +++ b/libavfilter/Makefile @@ -4,6 +4,7 @@ NAME = avfilter HEADERS = avfilter.h \ avfiltergraph.h \ + avfilterlink.h \ buffersink.h \ buffersrc.h \ version.h \ diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h index 828b270b6c..f219e2ad34 100644 --- a/libavfilter/avfilter.h +++ b/libavfilter/avfilter.h @@ -377,222 +377,7 @@ struct AVFilterContext { unsigned ready; }; -/** - * A link between two filters. This contains pointers to the source and - * destination filters between which this link exists, and the indexes of - * the pads involved. In addition, this link also contains the parameters - * which have been negotiated and agreed upon between the filter, such as - * image dimensions, format, etc. - */ -struct AVFilterLink { - AVFilterContext *src; ///< source filter - AVFilterPad *srcpad; ///< output pad on the source filter - - AVFilterContext *dst; ///< dest filter - AVFilterPad *dstpad; ///< input pad on the dest filter - - enum AVMediaType type; ///< filter media type - - /* These parameters apply only to video */ - int w; ///< agreed upon image width - int h; ///< agreed upon image height - AVRational sample_aspect_ratio; ///< agreed upon sample aspect ratio - /* These parameters apply only to audio */ - uint64_t channel_layout; ///< channel layout of current buffer (see libavutil/channel_layout.h) - int sample_rate; ///< samples per second - - int format; ///< agreed upon media format - - /** - * Define the time base used by the PTS of the frames/samples - * which will pass through this link. - * During the configuration stage, each filter is supposed to - * change only the output timebase, while the timebase of the - * input link is assumed to be an unchangeable property. - */ - AVRational time_base; - - /***************************************************************** - * All fields below this line are not part of the public API. They - * may not be used outside of libavfilter and can be changed and - * removed at will. - * New public fields should be added right above. - ***************************************************************** - */ - /** - * Lists of formats and channel layouts supported by the input and output - * filters respectively. These lists are used for negotiating the format - * to actually be used, which will be loaded into the format and - * channel_layout members, above, when chosen. - * - */ - AVFilterFormats *in_formats; - AVFilterFormats *out_formats; - - /** - * Lists of channel layouts and sample rates used for automatic - * negotiation. - */ - AVFilterFormats *in_samplerates; - AVFilterFormats *out_samplerates; - struct AVFilterChannelLayouts *in_channel_layouts; - struct AVFilterChannelLayouts *out_channel_layouts; - - /** - * Audio only, the destination filter sets this to a non-zero value to - * request that buffers with the given number of samples should be sent to - * it. AVFilterPad.needs_fifo must also be set on the corresponding input - * pad. - * Last buffer before EOF will be padded with silence. - */ - int request_samples; - - /** stage of the initialization of the link properties (dimensions, etc) */ - enum { - AVLINK_UNINIT = 0, ///< not started - AVLINK_STARTINIT, ///< started, but incomplete - AVLINK_INIT ///< complete - } init_state; - - /** - * Graph the filter belongs to. - */ - struct AVFilterGraph *graph; - - /** - * Current timestamp of the link, as defined by the most recent - * frame(s), in link time_base units. - */ - int64_t current_pts; - - /** - * Current timestamp of the link, as defined by the most recent - * frame(s), in AV_TIME_BASE units. - */ - int64_t current_pts_us; - - /** - * Index in the age array. - */ - int age_index; - - /** - * Frame rate of the stream on the link, or 1/0 if unknown or variable; - * if left to 0/0, will be automatically copied from the first input - * of the source filter if it exists. - * - * Sources should set it to the best estimation of the real frame rate. - * If the source frame rate is unknown or variable, set this to 1/0. - * Filters should update it if necessary depending on their function. - * Sinks can use it to set a default output frame rate. - * It is similar to the r_frame_rate field in AVStream. - */ - AVRational frame_rate; - - /** - * Buffer partially filled with samples to achieve a fixed/minimum size. - */ - AVFrame *partial_buf; - - /** - * Size of the partial buffer to allocate. - * Must be between min_samples and max_samples. - */ - int partial_buf_size; - - /** - * Minimum number of samples to filter at once. If filter_frame() is - * called with fewer samples, it will accumulate them in partial_buf. - * This field and the related ones must not be changed after filtering - * has started. - * If 0, all related fields are ignored. - */ - int min_samples; - - /** - * Maximum number of samples to filter at once. If filter_frame() is - * called with more samples, it will split them. - */ - int max_samples; - - /** - * Number of channels. - */ - int channels; - - /** - * Link processing flags. - */ - unsigned flags; - - /** - * Number of past frames sent through the link. - */ - int64_t frame_count_in, frame_count_out; - - /** - * A pointer to a FFVideoFramePool struct. - */ - void *video_frame_pool; - - /** - * 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. - */ - int frame_wanted_out; - - /** - * For hwaccel pixel formats, this should be a reference to the - * 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 */ - -}; +#include "avfilterlink.h" /** * Link two filters together. diff --git a/libavfilter/avfilterlink.h b/libavfilter/avfilterlink.h new file mode 100644 index 0000000000..51ab322ae9 --- /dev/null +++ b/libavfilter/avfilterlink.h @@ -0,0 +1,248 @@ +/* + * AVFilterLink definition + * Copyright (c) 2007 Bobby Bingham + * + * 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_AVFILTERLINK_H +#define AVFILTER_AVFILTERLINK_H + +#include "avfilter.h" + +#ifdef FF_INTERNAL_FIELDS +# include "framequeue.h" +#endif + +/** + * A link between two filters. This contains pointers to the source and + * destination filters between which this link exists, and the indexes of + * the pads involved. In addition, this link also contains the parameters + * which have been negotiated and agreed upon between the filter, such as + * image dimensions, format, etc. + */ +struct AVFilterLink { + AVFilterContext *src; ///< source filter + AVFilterPad *srcpad; ///< output pad on the source filter + + AVFilterContext *dst; ///< dest filter + AVFilterPad *dstpad; ///< input pad on the dest filter + + enum AVMediaType type; ///< filter media type + + /* These parameters apply only to video */ + int w; ///< agreed upon image width + int h; ///< agreed upon image height + AVRational sample_aspect_ratio; ///< agreed upon sample aspect ratio + /* These parameters apply only to audio */ + uint64_t channel_layout; ///< channel layout of current buffer (see libavutil/channel_layout.h) + int sample_rate; ///< samples per second + + int format; ///< agreed upon media format + + /** + * Define the time base used by the PTS of the frames/samples + * which will pass through this link. + * During the configuration stage, each filter is supposed to + * change only the output timebase, while the timebase of the + * input link is assumed to be an unchangeable property. + */ + AVRational time_base; + + /***************************************************************** + * All fields below this line are not part of the public API. They + * may not be used outside of libavfilter and can be changed and + * removed at will. + * New public fields should be added right above. + ***************************************************************** + */ + /** + * Lists of formats and channel layouts supported by the input and output + * filters respectively. These lists are used for negotiating the format + * to actually be used, which will be loaded into the format and + * channel_layout members, above, when chosen. + * + */ + AVFilterFormats *in_formats; + AVFilterFormats *out_formats; + + /** + * Lists of channel layouts and sample rates used for automatic + * negotiation. + */ + AVFilterFormats *in_samplerates; + AVFilterFormats *out_samplerates; + struct AVFilterChannelLayouts *in_channel_layouts; + struct AVFilterChannelLayouts *out_channel_layouts; + + /** + * Audio only, the destination filter sets this to a non-zero value to + * request that buffers with the given number of samples should be sent to + * it. AVFilterPad.needs_fifo must also be set on the corresponding input + * pad. + * Last buffer before EOF will be padded with silence. + */ + int request_samples; + + /** stage of the initialization of the link properties (dimensions, etc) */ + enum { + AVLINK_UNINIT = 0, ///< not started + AVLINK_STARTINIT, ///< started, but incomplete + AVLINK_INIT ///< complete + } init_state; + + /** + * Graph the filter belongs to. + */ + struct AVFilterGraph *graph; + + /** + * Current timestamp of the link, as defined by the most recent + * frame(s), in link time_base units. + */ + int64_t current_pts; + + /** + * Current timestamp of the link, as defined by the most recent + * frame(s), in AV_TIME_BASE units. + */ + int64_t current_pts_us; + + /** + * Index in the age array. + */ + int age_index; + + /** + * Frame rate of the stream on the link, or 1/0 if unknown or variable; + * if left to 0/0, will be automatically copied from the first input + * of the source filter if it exists. + * + * Sources should set it to the best estimation of the real frame rate. + * If the source frame rate is unknown or variable, set this to 1/0. + * Filters should update it if necessary depending on their function. + * Sinks can use it to set a default output frame rate. + * It is similar to the r_frame_rate field in AVStream. + */ + AVRational frame_rate; + + /** + * Buffer partially filled with samples to achieve a fixed/minimum size. + */ + AVFrame *partial_buf; + + /** + * Size of the partial buffer to allocate. + * Must be between min_samples and max_samples. + */ + int partial_buf_size; + + /** + * Minimum number of samples to filter at once. If filter_frame() is + * called with fewer samples, it will accumulate them in partial_buf. + * This field and the related ones must not be changed after filtering + * has started. + * If 0, all related fields are ignored. + */ + int min_samples; + + /** + * Maximum number of samples to filter at once. If filter_frame() is + * called with more samples, it will split them. + */ + int max_samples; + + /** + * Number of channels. + */ + int channels; + + /** + * Link processing flags. + */ + unsigned flags; + + /** + * Number of past frames sent through the link. + */ + int64_t frame_count_in, frame_count_out; + + /** + * A pointer to a FFVideoFramePool struct. + */ + void *video_frame_pool; + + /** + * 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. + */ + int frame_wanted_out; + + /** + * For hwaccel pixel formats, this should be a reference to the + * 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 */ + +}; + +#endif /* AVFILTER_AVFILTERLINK_H */
AVFilterLink contains fields of internal types that are better isolated. Furthermore, with the prospect of inter-filters threading, it is better if filters do not access links directly. Signed-off-by: Nicolas George <george@nsup.org> --- libavfilter/Makefile | 1 + libavfilter/avfilter.h | 217 +-------------------------------------- libavfilter/avfilterlink.h | 248 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 250 insertions(+), 216 deletions(-) create mode 100644 libavfilter/avfilterlink.h That way, making it public or private is just a matter of changing one line in the Makefile.