[FFmpeg-devel] [PATCH 2/2] Change filter type if/else to a switch.

[Måns Rullgård]

Justin Ruggles <justin.ruggles at gmail.com> writes:

> Simplifies error handling and makes it easier to add additional filter types.
> ---
>  libavcodec/iirfilter.c |   26 ++++++++++++++------------
>  1 files changed, 14 insertions(+), 12 deletions(-)
>
>
> diff --git a/libavcodec/iirfilter.c b/libavcodec/iirfilter.c
> index 0854820..8aa177b 100644
> --- a/libavcodec/iirfilter.c
> +++ b/libavcodec/iirfilter.c
> @@ -164,6 +164,7 @@ av_cold struct FFIIRFilterCoeffs* ff_iir_filter_init_coeffs(void *avc,
>                                                  float stopband, float ripple)
>  {
>      FFIIRFilterCoeffs *c;
> +    int ret;
>  
>      if (order <= 0 || order > MAXORDER || cutoff_ratio >= 1.0)
>          return NULL;
> @@ -176,20 +177,21 @@ av_cold struct FFIIRFilterCoeffs* ff_iir_filter_init_coeffs(void *avc,
>                        init_fail);
>      c->order = order;
>  
> -    if (filt_type == FF_FILTER_TYPE_BUTTERWORTH) {
> -        if (butterworth_init_coeffs(avc, c, filt_mode, order, cutoff_ratio,
> -            stopband)) {
> -            goto init_fail;
> -        }
> -    } else if (filt_type == FF_FILTER_TYPE_BIQUAD) {
> -        if (biquad_init_coeffs(avc, c, filt_mode, order, cutoff_ratio,
> -            stopband)) {
> -            goto init_fail;
> -        }
> -    } else {
> +    ret = 0;

Initialising ret in the declaration looks nicer, I think.

> +    switch (filt_type) {
> +    case FF_FILTER_TYPE_BUTTERWORTH:
> +        ret = butterworth_init_coeffs(avc, c, filt_mode, order, cutoff_ratio,
> +                                      stopband);
> +        break;
> +    case FF_FILTER_TYPE_BIQUAD:
> +        ret = biquad_init_coeffs(avc, c, filt_mode, order, cutoff_ratio,
> +                                 stopband);

Missing break.

> +    default:
>          av_log(avc, AV_LOG_ERROR, "filter type is not currently implemented\n");
> -        goto init_fail;
> +        ret = -1;

Why not leave the goto there,

>      }
> +    if (ret)
> +        goto init_fail;

and make this "if (!ret) return c;"?

>      return c;

Maybe that's not any nicer though.  Just a thought.

-- 
M?ns Rullg?rd
mans at mansr.com