[FFmpeg-devel] [PATCH] swscale: Use function pointers inside swScale().

Michael Niedermayer michaelni
Fri Apr 3 05:38:55 CEST 2009


On Fri, Apr 03, 2009 at 12:07:10AM -0300, Ramiro Polla wrote:
> Hi,
> 
> 2009/4/2 M?ns Rullg?rd <mans at mansr.com>:
> > Michael Niedermayer <michaelni at gmx.at> writes:
> >> On Thu, Apr 02, 2009 at 05:09:49PM -0300, Ramiro Polla wrote:
> >>> One more patch (bundled with cosmetics.diff to be applied before) in
> >>> the series...
> >>>
> >> [...]
> >>> Index: swscale_internal.h
> >>> ===================================================================
> >>> --- swscale_internal.h ? ? ? (revision 29130)
> >>> +++ swscale_internal.h ? ? ? (working copy)
> >>> @@ -214,6 +214,37 @@
> >>> ? ? ?uint64_t sparc_coeffs[10] __attribute__((aligned(8)));
> >>> ?#endif
> >>>
> >>> + ? ?/* function pointers for swScale() */
> >>> + ? ?void (*yuv2nv12X ?)(struct SwsContext *, int16_t *, int16_t **, int,
> >>> + ? ? ? ? ? ? ? ? ? ? ? ?int16_t *, int16_t **, int, uint8_t *, uint8_t *, int,
> >>> + ? ? ? ? ? ? ? ? ? ? ? ?int, int);
> >>> + ? ?void (*yuv2yuv1 ? )(struct SwsContext *, int16_t *, int16_t *, int16_t *,
> >>> + ? ? ? ? ? ? ? ? ? ? ? ?uint8_t *, uint8_t *, uint8_t *, uint8_t *, long,
> >>> + ? ? ? ? ? ? ? ? ? ? ? ?long);
> >>> + ? ?void (*yuv2yuvX ? )(struct SwsContext *, int16_t *, int16_t **, int,
> >>> + ? ? ? ? ? ? ? ? ? ? ? ?int16_t *, int16_t **, int, int16_t **, uint8_t *,
> >>> + ? ? ? ? ? ? ? ? ? ? ? ?uint8_t *, uint8_t *, uint8_t *, long, long);
> >>> + ? ?void (*yuv2packed1)(struct SwsContext *, uint16_t *, uint16_t *,
> >>> + ? ? ? ? ? ? ? ? ? ? ? ?uint16_t *, uint16_t *, uint8_t *, int, int, int, int,
> >>> + ? ? ? ? ? ? ? ? ? ? ? ?int);
> >>> + ? ?void (*yuv2packed2)(struct SwsContext *, uint16_t *, uint16_t *,
> >>> + ? ? ? ? ? ? ? ? ? ? ? ?uint16_t *, uint16_t *, uint16_t *, uint16_t *,
> >>> + ? ? ? ? ? ? ? ? ? ? ? ?uint8_t *, int, int, int, int);
> >>> + ? ?void (*yuv2packedX)(struct SwsContext *c, int16_t *, int16_t **, int,
> >>> + ? ? ? ? ? ? ? ? ? ? ? ?int16_t *, int16_t **, int, int16_t **, uint8_t *,
> >>> + ? ? ? ? ? ? ? ? ? ? ? ?long, long);
> >>> +
> >>> + ? ?void (*hyscale_internal)(uint8_t *, uint8_t *, long, uint32_t *);
> >>> + ? ?void (*hcscale_internal)(uint8_t *, uint8_t *, uint8_t *, uint8_t *, long,
> >>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? uint32_t *);
> >>> + ? ?void (*hyscale_fast)(struct SwsContext *c, int16_t *, int, uint8_t *,
> >>> + ? ? ? ? ? ? ? ? ? ? ? ? int, int);
> >>> + ? ?void (*hcscale_fast)(struct SwsContext *c, int16_t *, int, uint8_t *,
> >>> + ? ? ? ? ? ? ? ? ? ? ? ? uint8_t *, int, int);
> >>> +
> >>> + ? ?void (*hScale)(int16_t *, int, uint8_t *, int , int, int16_t *, int16_t *,
> >>> + ? ? ? ? ? ? ? ? ? long);
> >>> +
> >>
> >> these require a clear documentation, that is the documentation must be
> >> sufficient to implement a working optimized function for each.
> >> otherwise it would be yet another undocumented API that would require
> >> global knowledge for any work
> 
> Yes, but that is a separate issue. There currently is no documentation
> and the patch only copies the function declarations. Besides, that
> would require a much closer inspection of each function and so I
> suspect the original authors (eg: you =) might be better suited for
> the job.

> 
> I updated the patch to take the parameters' names as well.

good


> 
> > The important things to know is guaranteed alignment of data and other
> > restrictions like width/height always multiple of N. ?What the
> > function actually does can hopefully be determined by reading the C
> > code.
> 
> This also requires a closer inspection (and better understand of the
> underlying asm). I don't know much about alignments.
> 

> If anyone cares to jump in and help on documenting the parameters and
> the alignments, that would be really appreciated.

yes i also would greatly appreciate it.


[...]

> Index: swscale_internal.h
> ===================================================================
> --- swscale_internal.h	(revision 29130)
> +++ swscale_internal.h	(working copy)
> @@ -214,6 +214,55 @@
>      uint64_t sparc_coeffs[10] __attribute__((aligned(8)));
>  #endif
>  
> +    /* function pointers for swScale() */
> +    void (*yuv2nv12X  )(struct SwsContext *c,
> +                        int16_t *lumFilter, int16_t **lumSrc, int lumFilterSize,
> +                        int16_t *chrFilter, int16_t **chrSrc, int chrFilterSize,
> +                        uint8_t *dest, uint8_t *uDest,
> +                        int dstW, int chrDstW, int dstFormat);
> +    void (*yuv2yuv1   )(struct SwsContext *c,
> +                        int16_t *lumSrc, int16_t *chrSrc, int16_t *alpSrc,
> +                        uint8_t *dest,
> +                        uint8_t *uDest, uint8_t *vDest, uint8_t *aDest,
> +                        long dstW, long chrDstW);
> +    void (*yuv2yuvX   )(struct SwsContext *c,
> +                        int16_t *lumFilter, int16_t **lumSrc, int lumFilterSize,
> +                        int16_t *chrFilter, int16_t **chrSrc, int chrFilterSize,
> +                        int16_t **alpSrc,
> +                        uint8_t *dest,
> +                        uint8_t *uDest, uint8_t *vDest, uint8_t *aDest,
> +                        long dstW, long chrDstW);
> +    void (*yuv2packed1)(struct SwsContext *c,
> +                        uint16_t *buf0, uint16_t *uvbuf0, uint16_t *uvbuf1,
> +                        uint16_t *abuf0, uint8_t *dest,
> +                        int dstW, int uvalpha, int dstFormat, int flags, int y);
> +    void (*yuv2packed2)(struct SwsContext *c,
> +                        uint16_t *buf0, uint16_t *buf1,
> +                        uint16_t *uvbuf0, uint16_t *uvbuf1,
> +                        uint16_t *abuf0, uint16_t *abuf1,
> +                        uint8_t *dest,
> +                        int dstW, int yalpha, int uvalpha, int y);
> +    void (*yuv2packedX)(struct SwsContext *c,
> +                        int16_t *lumFilter, int16_t **lumSrc, int lumFilterSize,
> +                        int16_t *chrFilter, int16_t **chrSrc, int chrFilterSize,
> +                        int16_t **alpSrc, uint8_t *dest,
> +                        long dstW, long dstY);
> +
> +    void (*hyscale_internal)(uint8_t *dst, uint8_t *src,
> +                             long width, uint32_t *pal);
> +    void (*hcscale_internal)(uint8_t *dstU, uint8_t *dstV,
> +                             uint8_t *src1, uint8_t *src2,
> +                             long width, uint32_t *pal);
> +    void (*hyscale_fast)(struct SwsContext *c,
> +                         int16_t *dst, int dstWidth,
> +                         uint8_t *src, int srcW, int xInc);
> +    void (*hcscale_fast)(struct SwsContext *c,
> +                         int16_t *dst, int dstWidth,
> +                         uint8_t *src1, uint8_t *src2, int srcW, int xInc);
> +
> +    void (*hScale)(int16_t *dst, int dstW, uint8_t *src, int srcW,
> +                   int xInc, int16_t *filter, int16_t *filterPos, long filterSize);
> +

all the parameters that have src in their name should be const but they likely
arent the only ones 


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
then the original author, trying to rewrite it will not make it better.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090403/7e1ff399/attachment.pgp>



More information about the ffmpeg-devel mailing list