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

Ramiro Polla ramiro.polla
Wed Jan 20 04:30:17 CET 2010


On Sun, Jan 17, 2010 at 12:16 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> 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

Ok, applied.

> of course that doesnt mean it cant be improved further ...

Suggestions are welcome (by anyone else who wants to crosscheck as well =).

>> 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 ?

I don't understand what you mean by this.

>> ? ? ?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

Applied.

>> + ? ?/**
>> + ? ? * @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

I wanted to keep this more generic, but I added the shift and a
comment explaining the actual implementation is fixed-point.

>> @@ -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

Applied.

I'll send an updated patch later.



More information about the ffmpeg-devel mailing list