[FFmpeg-devel] [PATCH] Doxument some of SwsContext.

Stefano Sabatini stefano.sabatini-lala
Sun Jan 17 01:02:34 CET 2010


On date Saturday 2010-01-16 20:40:42 -0200, Ramiro Polla encoded:
[...]
> diff --git a/swscale_internal.h b/swscale_internal.h
> index 65174fd..8053bcc 100644
> --- a/swscale_internal.h
> +++ b/swscale_internal.h
> @@ -82,51 +82,81 @@ typedef struct SwsContext {
>      int chrSrcH;                  ///< Height of source      chroma     planes.
>      int chrDstW;                  ///< Width  of destination chroma     planes.
>      int chrDstH;                  ///< Height of destination chroma     planes.

General comment, our convention for the doxy of a field is: "upcased
if not a complete sentence or an incomplete sentence followed by a
complete sentence" so these doxies should be downcased and without the
final point, as they are not complete sentences.

> -    int lumXInc, chrXInc;
> -    int lumYInc, chrYInc;
> +    int lumXInc;                  ///< Horizontal scale factor between source and destination luma/alpha planes.
> +    int chrXInc;                  ///< Horizontal scale factor between source and destination chroma     planes.
> +    int lumYInc;                  ///< Vertical   scale factor between source and destination luma/alpha planes.
> +    int chrYInc;                  ///< Vertical   scale factor between source and destination chroma     planes.

While these doxies are a big step for improving general understanding
of the fields, I still cannot perfectly understand what these fields
are for.

What is a scale factor between a source and a destination plane? I
suppose here it is meant the size, that is e.g.:
dstLumPlaneSize = lumXInc * srcLumPlaneSize

also is this field only meaningful for planar and not for
paletted/packed formats?

Also I'm not happy about the "Inc" in the fields names, maybe the doxy
should make clear what that suffix means (I suppose it means "Increment").

>      enum PixelFormat dstFormat;   ///< Destination pixel format.
>      enum PixelFormat srcFormat;   ///< Source      pixel format.
> -    int chrSrcHSubSample, chrSrcVSubSample;
> -    int chrDstHSubSample, chrDstVSubSample;
> -    int vChrDrop;
> -    int sliceDir;
> +    int chrSrcHSubSample;         ///< Binary logarithm of horizontal subsampling factor between luma/alpha and chroma planes in source      image.
> +    int chrSrcVSubSample;         ///< Binary logarithm of vertical   subsampling factor between luma/alpha and chroma planes in source      image.
> +    int chrDstHSubSample;         ///< Binary logarithm of horizontal subsampling factor between luma/alpha and chroma planes in destination image.
> +    int chrDstVSubSample;         ///< Binary logarithm of vertical   subsampling factor between luma/alpha and chroma planes in destination image.
> +    int vChrDrop;                 ///< Binary logarithm of extra vertical subsampling factor in source image chroma planes specified by user.
> +    int sliceDir;                 ///< Direction that slices are fed to the scaler (1 = top-to-bottom, -1 = bottom-to-top).

Maybe we could define some enum, e.g. enum SWSSliceDirection {
SWS_SLICE_DIR_TOP_DOWN = -1,
SWS_SLICE_DIR_NONE = 0,
SWS_SLICE_DIR_BOTTOM_UP = 1
};

I'm not sure if it is possible to have headache-lessly negative values
in an enumerated type.

>      double param[2];              ///< Input parameters for scaling algorithms that need them.
>  
> -    uint32_t pal_yuv[256];
> -    uint32_t pal_rgb[256];
> -
> -    int16_t **lumPixBuf;
> -    int16_t **chrPixBuf;
> -    int16_t **alpPixBuf;
> -    int16_t *hLumFilter;
> -    int16_t *hLumFilterPos;
> -    int16_t *hChrFilter;
> -    int16_t *hChrFilterPos;
> -    int16_t *vLumFilter;
> -    int16_t *vLumFilterPos;
> -    int16_t *vChrFilter;
> -    int16_t *vChrFilterPos;
> -
> -    uint8_t formatConvBuffer[VOF]; //FIXME dynamic allocation, but we have to change a lot of code for this to be useful
> -
> -    int hLumFilterSize;
> -    int hChrFilterSize;
> -    int vLumFilterSize;
> -    int vChrFilterSize;
> -    int vLumBufSize;
> -    int vChrBufSize;
> +    uint32_t pal_yuv[256];        ///< YUV   values corresponding to palettized data from source pixel format.
> +    uint32_t pal_rgb[256];        ///< RGB32 values corresponding to palettized data from source pixel format.
> +
> +    /**
> +     * @name Scaled horizontal lines ring buffer.
> +     * The horizontal scaler keeps just enough scaled lines in a ring buffer
> +     * so they may be passed to the vertical scaler. The pointers to the
> +     * allocated buffers for each line are duplicated in sequence in the ring
> +     * buffer to simplify indexing and avoid wrapping around between lines
> +     * inside the vertical scaler code. The wrapping is done before the
> +     * vertical scaler is called.
> +     */
> +    //@{
> +    int16_t **lumPixBuf;          ///< Ring buffer for scaled horizontal luma   plane lines to be fed to the vertical scaler.
> +    int16_t **chrPixBuf;          ///< Ring buffer for scaled horizontal chroma plane lines to be fed to the vertical scaler.
> +    int16_t **alpPixBuf;          ///< Ring buffer for scaled horizontal alpha  plane lines to be fed to the vertical scaler.
> +    int       vLumBufSize;        ///< Number of vertical luma/alpha lines allocated in the ring buffer.
> +    int       vChrBufSize;        ///< Number of vertical chroma     lines allocated in the ring buffer.

> +    int       lastInLumBuf;       ///< Last scaled horizontal luma/alpha line from source in the ring buffer.
> +    int       lastInChrBuf;       ///< Last scaled horizontal chroma     line from source in the ring buffer.

Are these lines or indexes? In the latter case I suggest to use
explicitely the term "index" in the name.

> +    int       lumBufIndex;        ///< Index in ring buffer of the last scaled horizontal luma/alpha line from source.
> +    int       chrBufIndex;        ///< Index in ring buffer of the last scaled horizontal chroma     line from source.

Maybe it's just me, but I cannot understand the difference between
these fields and the previous ones, seems to me that the
{lum,chr}BufIndex doxies are just a re-formulation of the
lastIn{Lum,Chr}Buf doxies, but with a mention to the term "index".

> +    //@}
> +
> +    //FIXME dynamic allocation, but we have to change a lot of code for this to be useful
> +    uint8_t formatConvBuffer[VOF]; ///< Intermediate horizontal scaler buffer used for unscaled conversion of input data to YV12.
> +
> +    /**
> +     * @name Horizontal and vertical filters.
> +     * To better understand the following fields, here is a pseudo-code of
> +     * their usage in filtering a horizontal line:
> +     * @code
> +     * for (i = 0; i < width; i++) {
> +     *     dst[i] = 0;
> +     *     for (j = 0; j < filterSize; j++)
> +     *         dst[i] += src[ filterPos[i] + j ] * filter[ filterSize * i + j ];
> +     * }
> +     * @endcode
> +     */
> +    //@{
> +    int16_t *hLumFilter;          ///< Array of horizontal filter coefficients for luma/alpha planes.
> +    int16_t *hChrFilter;          ///< Array of horizontal filter coefficients for chroma     planes.
> +    int16_t *vLumFilter;          ///< Array of vertical   filter coefficients for luma/alpha planes.
> +    int16_t *vChrFilter;          ///< Array of vertical   filter coefficients for chroma     planes.
> +    int16_t *hLumFilterPos;       ///< Array of horizontal filter starting positions for each dst[i] for luma/alpha planes.
> +    int16_t *hChrFilterPos;       ///< Array of horizontal filter starting positions for each dst[i] for chroma     planes.
> +    int16_t *vLumFilterPos;       ///< Array of vertical   filter starting positions for each dst[i] for luma/alpha planes.
> +    int16_t *vChrFilterPos;       ///< Array of vertical   filter starting positions for each dst[i] for chroma     planes.
> +    int      hLumFilterSize;      ///< Horizontal filter size for luma/alpha pixels.
> +    int      hChrFilterSize;      ///< Horizontal filter size for chroma     pixels.
> +    int      vLumFilterSize;      ///< Vertical   filter size for luma/alpha pixels.
> +    int      vChrFilterSize;      ///< Vertical   filter size for chroma     pixels.
> +    //@}

Fine and much needed explanation.

>      int lumMmx2FilterCodeSize;    ///< Runtime-generated MMX2 horizontal fast bilinear scaler code size for luma/alpha planes.
>      int chrMmx2FilterCodeSize;    ///< Runtime-generated MMX2 horizontal fast bilinear scaler code size for chroma     planes.
>      uint8_t *lumMmx2FilterCode;   ///< Runtime-generated MMX2 horizontal fast bilinear scaler code for luma/alpha planes.
>      uint8_t *chrMmx2FilterCode;   ///< Runtime-generated MMX2 horizontal fast bilinear scaler code for chroma     planes.
>  
> -    int canMMX2BeUsed;
> +    int canMMX2BeUsed;            ///< Whether the runtime-generated MMX2 horizontal fast bilinear scaler can be used.
>  
> -    int lastInLumBuf;
> -    int lastInChrBuf;
> -    int lumBufIndex;
> -    int chrBufIndex;
>      int dstY;                     ///< Last destination vertical line output from last slice.
>      int flags;                    ///< Flags passed by the user to select scaler algorithm, optimizations, subsampling, etc...
>      void * yuvTable;            // pointer to the yuv->rgb table start so it can be freed()
> @@ -183,7 +213,7 @@ typedef struct SwsContext {
>      DECLARE_ALIGNED(8, uint64_t, vOffset);
>      int32_t  lumMmxFilter[4*MAX_FILTER_SIZE];
>      int32_t  chrMmxFilter[4*MAX_FILTER_SIZE];
> -    int dstW;
> +    int dstW;                     ///< Width  of destination luma/alpha planes.
>      DECLARE_ALIGNED(8, uint64_t, esp);
>      DECLARE_ALIGNED(8, uint64_t, vRounder);
>      DECLARE_ALIGNED(8, uint64_t, u_temp);
> @@ -221,16 +251,26 @@ typedef struct SwsContext {
>  #endif
>  
>      /* function pointers for swScale() */
> +
> +    /**
> +     * Generic vertical YV12 to NV12 scaler.
> +     */

I'd like to avoid the term "YV12", as it is an MPlayer-ism, for
example there is no mention of it in libavutil/pixfmt.h.

I suggest to use instead "YUV" (I may be wrong here).

>      void (*yuv2nv12X  )(struct SwsContext *c,
>                          const int16_t *lumFilter, const int16_t **lumSrc, int lumFilterSize,
>                          const int16_t *chrFilter, const int16_t **chrSrc, int chrFilterSize,
>                          uint8_t *dest, uint8_t *uDest,
>                          int dstW, int chrDstW, int dstFormat);
> +    /**
> +     * Vertical YV12 to YV12 converter without scaling or interpolating.
> +     */
>      void (*yuv2yuv1   )(struct SwsContext *c,
>                          const int16_t *lumSrc, const int16_t *chrSrc, const int16_t *alpSrc,
>                          uint8_t *dest,
>                          uint8_t *uDest, uint8_t *vDest, uint8_t *aDest,
>                          long dstW, long chrDstW);
> +    /**
> +     * Generic vertical YV12 to YV12 scaler.
> +     */
>      void (*yuv2yuvX   )(struct SwsContext *c,
>                          const int16_t *lumFilter, const int16_t **lumSrc, int lumFilterSize,
>                          const int16_t *chrFilter, const int16_t **chrSrc, int chrFilterSize,
> @@ -238,39 +278,67 @@ typedef struct SwsContext {
>                          uint8_t *dest,
>                          uint8_t *uDest, uint8_t *vDest, uint8_t *aDest,
>                          long dstW, long chrDstW);
> +    /**
> +     * Vertical YV12 to RGB converter interpolating chrominance for every second line.
> +     */

Please use always the same terminology, is a "converter" a "scaler" or
are they different things?

>      void (*yuv2packed1)(struct SwsContext *c,
>                          const uint16_t *buf0,
>                          const uint16_t *uvbuf0, const uint16_t *uvbuf1,
>                          const uint16_t *abuf0,
>                          uint8_t *dest,
>                          int dstW, int uvalpha, int dstFormat, int flags, int y);
> +    /**
> +     * Vertical bilinear YV12 to RGB scaler.
> +     */
>      void (*yuv2packed2)(struct SwsContext *c,
>                          const uint16_t *buf0, const uint16_t *buf1,
>                          const uint16_t *uvbuf0, const uint16_t *uvbuf1,
>                          const uint16_t *abuf0, const uint16_t *abuf1,
>                          uint8_t *dest,
>                          int dstW, int yalpha, int uvalpha, int y);
> +    /**
> +     * Generic vertical YV12 to RGB scaler.
> +     */
>      void (*yuv2packedX)(struct SwsContext *c,
>                          const int16_t *lumFilter, const int16_t **lumSrc, int lumFilterSize,
>                          const int16_t *chrFilter, const int16_t **chrSrc, int chrFilterSize,
>                          const int16_t **alpSrc, uint8_t *dest,
>                          long dstW, long dstY);
>  

> +    /**
> +     * Unscaled conversion of luma plane to YV12 for horizontal scaler.
> +     */
>      void (*lumToYV12)(uint8_t *dst, const uint8_t *src,
> -                      long width, uint32_t *pal); ///< Unscaled conversion of luma plane to YV12 for horizontal scaler.
> +                      long width, uint32_t *pal);
> +    /**
> +     * Unscaled conversion of alpha plane to YV12 for horizontal scaler.
> +     */
>      void (*alpToYV12)(uint8_t *dst, const uint8_t *src,
> -                      long width, uint32_t *pal); ///< Unscaled conversion of alpha plane to YV12 for horizontal scaler.
> +                      long width, uint32_t *pal);
> +    /**
> +     * Unscaled conversion of chroma planes to YV12 for horizontal scaler.
> +     */
>      void (*chrToYV12)(uint8_t *dstU, uint8_t *dstV,
>                        const uint8_t *src1, const uint8_t *src2,
> -                      long width, uint32_t *pal); ///< Unscaled conversion of chroma planes to YV12 for horizontal scaler.
> +                      long width, uint32_t *pal);

"Unscaled conversion" -> scaler or converter if they are such.

Also I'm not much happy about this YV12, also for some fields we use
"To", for other ones "To", this leads to confusion, maybe a general
review on the names should be done.

> +
> +    /**
> +     * Fast bilinear horizontal scaler for luma/alpha planes.
> +     */
>      void (*hyscale_fast)(struct SwsContext *c,
>                           int16_t *dst, long dstWidth,
>                           const uint8_t *src, int srcW, int xInc);
> +    /**
> +     * Fast bilinear horizontal scaler for chroma planes.
> +     */
>      void (*hcscale_fast)(struct SwsContext *c,
>                           int16_t *dst, long dstWidth,
>                           const uint8_t *src1, const uint8_t *src2,
>                           int srcW, int xInc);
>  
> +    /**
> +     * Generic horizontal scaler.
> +     */
>      void (*hScale)(int16_t *dst, int dstW, const uint8_t *src, int srcW,
>                     int xInc, const int16_t *filter, const int16_t *filterPos,
>                     long filterSize);
> diff --git a/swscale_template.c b/swscale_template.c
> index 76297d6..6e3690d 100644
> --- a/swscale_template.c
> +++ b/swscale_template.c
> @@ -1019,9 +1019,6 @@ static inline void RENAME(yuv2yuv1)(SwsContext *c, const int16_t *lumSrc, const
>  }
>  
>  
> -/**
> - * vertical scale YV12 to RGB
> - */
>  static inline void RENAME(yuv2packedX)(SwsContext *c, const int16_t *lumFilter, const int16_t **lumSrc, int lumFilterSize,
>                                         const int16_t *chrFilter, const int16_t **chrSrc, int chrFilterSize,
>                                         const int16_t **alpSrc, uint8_t *dest, long dstW, long dstY)
> @@ -1204,9 +1201,6 @@ static inline void RENAME(yuv2packedX)(SwsContext *c, const int16_t *lumFilter,
>                         alpSrc, dest, dstW, dstY);
>  }
>  
> -/**
> - * vertical bilinear scale YV12 to RGB
> - */
>  static inline void RENAME(yuv2packed2)(SwsContext *c, const uint16_t *buf0, const uint16_t *buf1, const uint16_t *uvbuf0, const uint16_t *uvbuf1,
>                            const uint16_t *abuf0, const uint16_t *abuf1, uint8_t *dest, int dstW, int yalpha, int uvalpha, int y)
>  {
> @@ -1353,9 +1347,6 @@ static inline void RENAME(yuv2packed2)(SwsContext *c, const uint16_t *buf0, cons
>      YSCALE_YUV_2_ANYRGB_C(YSCALE_YUV_2_RGB2_C, YSCALE_YUV_2_PACKED2_C(void,0), YSCALE_YUV_2_GRAY16_2_C, YSCALE_YUV_2_MONO2_C)
>  }
>  
> -/**
> - * YV12 to RGB without scaling or interpolating
> - */
>  static inline void RENAME(yuv2packed1)(SwsContext *c, const uint16_t *buf0, const uint16_t *uvbuf0, const uint16_t *uvbuf1,
>                            const uint16_t *abuf0, uint8_t *dest, int dstW, int uvalpha, enum PixelFormat dstFormat, int flags, int y)
>  {

Regards and thanks for the much needed doxumentation job.
-- 
FFmpeg = Fantastic & Friendly Minimalistic Ponderous Extravagant Gymnast



More information about the ffmpeg-devel mailing list