Re: [FFmpeg-devel] [PATCH 1/4] libswscale: Re-factor ff_shuffle_filter_coefficients.

[Alan Kelly <alankelly-at-google.com@ffmpeg.org>]

Hi Michael,

Thanks for your feedback. I have updated the patches and split this patch
into two, one with cosmetic fixes and one propagating the errors. Since
there is now an extra patch in the set and the commit messages have
changed, new threads have been started.

Alan

On Thu, Feb 3, 2022 at 3:11 PM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Mon, Jan 10, 2022 at 03:58:33PM +0100, Alan Kelly wrote:
> > Make the code more readable, follow the style guide and propagate memory
> > allocation errors.
>
> Cosmetics and bugfixes should not be in the same patch
>
>
> > ---
> >  libswscale/swscale_internal.h |  2 +-
> >  libswscale/utils.c            | 68 ++++++++++++++++++++---------------
> >  2 files changed, 40 insertions(+), 30 deletions(-)
> >
> > diff --git a/libswscale/swscale_internal.h
> b/libswscale/swscale_internal.h
> > index 3a78d95ba6..26d28d42e6 100644
> > --- a/libswscale/swscale_internal.h
> > +++ b/libswscale/swscale_internal.h
> > @@ -1144,5 +1144,5 @@ void ff_sws_slice_worker(void *priv, int jobnr,
> int threadnr,
> >  #define MAX_LINES_AHEAD 4
> >
> >  //shuffle filter and filterPos for hyScale and hcScale filters in avx2
> > -void ff_shuffle_filter_coefficients(SwsContext *c, int* filterPos, int
> filterSize, int16_t *filter, int dstW);
> > +int ff_shuffle_filter_coefficients(SwsContext *c, int* filterPos, int
> filterSize, int16_t *filter, int dstW);
> >  #endif /* SWSCALE_SWSCALE_INTERNAL_H */
> > diff --git a/libswscale/utils.c b/libswscale/utils.c
> > index c5ea8853d5..52f07e1661 100644
> > --- a/libswscale/utils.c
> > +++ b/libswscale/utils.c
> > @@ -278,39 +278,47 @@ static const FormatEntry format_entries[] = {
> >      [AV_PIX_FMT_P416LE]      = { 1, 1 },
> >  };
> >
> > -void ff_shuffle_filter_coefficients(SwsContext *c, int *filterPos, int
> filterSize, int16_t *filter, int dstW){
> > +int ff_shuffle_filter_coefficients(SwsContext *c, int *filterPos,
> > +                                   int filterSize, int16_t *filter,
> > +                                   int dstW)
> > +{
> >  #if ARCH_X86_64
>
> > -    int i, j, k, l;
> > +    int i = 0, j = 0, k = 0;
>
> why?
> they are set when used if iam not mistaken
>
>
> >      int cpu_flags = av_get_cpu_flags();
>
> > +    if (!filter || dstW % 16 != 0) return 0;
>
> please add \n also a comment what the dstW & 16 case exactly does and why
>
>
> [...]
> >  int sws_isSupportedInput(enum AVPixelFormat pix_fmt)
> > @@ -1836,7 +1844,8 @@ av_cold int sws_init_context(SwsContext *c,
> SwsFilter *srcFilter,
> >                             get_local_pos(c, 0, 0, 0),
> >                             get_local_pos(c, 0, 0, 0))) < 0)
> >                  goto fail;
> > -            ff_shuffle_filter_coefficients(c, c->hLumFilterPos,
> c->hLumFilterSize, c->hLumFilter, dstW);
> > +            if ((ret = ff_shuffle_filter_coefficients(c,
> c->hLumFilterPos, c->hLumFilterSize, c->hLumFilter, dstW)) != 0)
> > +                goto nomem;
>
> This is confusing as ret is never used, also error codes are <0
>
> thx
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Those who are best at talking, realize last or never when they are wrong.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".