[FFmpeg-devel] [PATCH] Move link_dpad and link_spad macros from avfilter.c to internal.h, so

[Stefano Sabatini]

On date Saturday 2010-09-04 13:54:12 +0200, Michael Niedermayer encoded:
> On Sat, Sep 04, 2010 at 01:43:27PM +0200, Stefano Sabatini wrote:
> > On date Saturday 2010-09-04 01:38:38 +0200, Michael Niedermayer encoded:
> > > On Fri, Sep 03, 2010 at 04:47:36PM +0200, Stefano Sabatini wrote:
> > > > On date Tuesday 2010-08-24 12:26:05 +0200, Stefano Sabatini encoded:
> > > > > On date Tuesday 2010-07-20 18:42:44 +0200, Michael Niedermayer encoded:
> > > > > > On Mon, Jul 19, 2010 at 06:19:56PM +0200, Stefano Sabatini wrote:
> > > > > > > ---
> > > > > > >  libavfilter/avfilter.c |    4 ----
> > > > > > >  libavfilter/internal.h |    4 ++++
> > > > > > >  2 files changed, 4 insertions(+), 4 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> > > > > > > index dc8f90d..e81fa48 100644
> > > > > > > --- a/libavfilter/avfilter.c
> > > > > > > +++ b/libavfilter/avfilter.c
> > > > > > > @@ -41,10 +41,6 @@ const char *avfilter_license(void)
> > > > > > >      return LICENSE_PREFIX FFMPEG_LICENSE + sizeof(LICENSE_PREFIX) - 1;
> > > > > > >  }
> > > > > > >  
> > > > > > > -/** helper macros to get the in/out pad on the dst/src filter */
> > > > > > > -#define link_dpad(link)     link->dst-> input_pads[link->dstpad]
> > > > > > > -#define link_spad(link)     link->src->output_pads[link->srcpad]
> > > > > > > -
> > > > > > 
> > > > > > iam against such obfuscation macros
> > > > > 
> > > > > So you either remove them or make them available in other files as
> > > > > well.
> > > > > 
> > > > > Maybe the link_dstpad / link_srcpad names would read better?
> > > > > 
> > > > > As for me I surely find more readable:
> > > > > AVFilterPad *pad = link_dstpad(link);
> > > > > rather than:
> > > > > AVFilterPad *pad = link->dst->input_pads[link->dstpad];
> > > > 
> > > > Updated with the names: FF_LINK_DSTPAD/SRCPAD.
> > > > 
> > > > I'll apply in three days if I see no objections.
> > > 
> > > i already said iam against the macros
> > > also iam against such macros in general not just here, this is not clean
> > > code.
> > > 
> > > If accessing a field requires a huge line of code and this is used
> > > hundereds of times then there is a problem with the design, and the
> > > fix is not to hide this behind a macro.  And besides it should be a
> > > static inline function not a macro if the design cant be fixed or
> > > the fix has worse side effects than this
> > 
> > This is not a problem of efficiency, it is a problem of readability.
> 
> you say this as if it makes a difference. Iam failing to see a difference
> though, if a design requires huge lines the design possibly needs to be
> improved. If theres a speed or rather a readability problem with that doesnt
> really change it.
> 
> 
> > So I suggest to take the static inline path, I don't think the design
> > should be changed.
> 
> i will consider this once i see arguments in favor of it compared to
> replacing the array index int by a pointer

I guess the main reason of the current design is to simplify the
operation of filter auto-insertion, check avfilter_insert_filter().

Allowing the filter to know the input and output index pads (rather
than the corresponding pointers) is convenient, e.g. it allows to
directly do:

src->outputs[link->srcpad] = new_lnk;
new_lnk->srcpad = link->srcpad;

rather than having to iterate through all the src->outputs in
order to find the one which is equal to the pointer pointed by
link->srcpad.

for (padidx = 0; i < src->output_count; padidx++)
    if (src->output_pads[padidx] == link->srcpad) {
        link->srcpad = NULL;
        src->outputs[padidx] = new_lnk;
        new_lnk->srcpad = src->output_pads[padidx];    
        break;
    }

I'll look in the direction of using a pointer though, regards.
-- 
FFmpeg = Friendly and Fundamental Mystic Pacific Excellent Generator