[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