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

Michael Niedermayer michaelni
Sun Jan 17 15:16:31 CET 2010


On Sat, Jan 16, 2010 at 08:40:42PM -0200, Ramiro Polla wrote:
> On Thu, Jan 14, 2010 at 10:56 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Thu, Jan 14, 2010 at 04:13:36AM -0200, Ramiro Polla wrote:
> [...]
> 
> OK'd hunks applied.
> 
> [...]
> >> + ? ?int chrSrcHSubSample; ? ? ? ? ///< Binary logarithm of horizontal subsampling factor between luma/alpha and chroma planes in source ? ? ?image.
> >
> > log2 is shorter
> 
> Hmm, I prefer the more explicative "Binary logarithm". This is more
> easily searchable on Wikipedia for example.
> 
> >> + ? ?int sliceDir; ? ? ? ? ? ? ? ? ///< Direction that slices are fed to the scaler (top-to-bottom or bottom-to-top).
> >
> > its a int not a string ;) which is which
> 
> Values enumerated.
> 
> >> + ? ?/**
> >> + ? ? * @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.
> >> + ? ?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.
> >> + ? ?//@}
> >
> > sounds probable, you know its long ago that i worked much on sws
> > i could miss some wrong but probable sounding things ...
> 
> I didn't know if this meant ok or not, so not applied for now.

it means apply it you think its correct, it sounds correct to me but iam
too busy to crosscheck each line against the code atm
of course that doesnt mean it cant be improved further ...


> 
> By the way this should make it easier for other people to understand
> the code. Could other people please take some time to check if it
> becomes more understandable how the ring buffer and filters work with
> this patch?
> 
> >> @@ -231,39 +280,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 without scaling or interpolating.
> >> + ? ? */
> >> ? ? ?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);
> >
> > theres a check for uvalpha < 2048 and interpolation in there
> 
> Hmm, then the comment on the function in swscale_template.c was wrong.
> Should the comments be on the function pointers or on the functions
> themselves? I removed them from the functions.
> 
> New patch attached a little bit cleaner for review.
> 
> Ramiro Polla

>  swscale_internal.h |  144 +++++++++++++++++++++++++++++++++++++++--------------
>  swscale_template.c |    9 ---
>  2 files changed, 106 insertions(+), 47 deletions(-)
> 86325f9c5ef95abc3bef9318b92a1b260095ae06  doxument_2.diff
> 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.
> -    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.

so we support scaling by x1, x2, x3 ?


>      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).
>      double param[2];              ///< Input parameters for scaling algorithms that need them.

ok

>  
> -    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.
> +    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.
> +    //@}
> +
> +    //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.
> +    //@}
>  

ok, but you miss scale down >>X in your code


>      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);

ok


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

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- 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/20100117/a1f0374f/attachment.pgp>



More information about the ffmpeg-devel mailing list