[FFmpeg-devel] [PATCH] Yet another stab at RGB48 support

[Diego Biurrun]

On Sat, May 09, 2009 at 02:28:18PM +0300, Kostya wrote:
> $subj

Changelog update is missing.

$nits below

> --- hires.c	(revision 0)
> +++ hires.c	(revision 0)
> @@ -0,0 +1,527 @@
> +#include <inttypes.h>

I think just stdint.h is enough.

> +static int gray16torgb48(SwsContext *c, uint8_t *src[], int srcStride[],
> +                         int srcSliceY, int srcSliceH, uint8_t *dst[],
> +                         int dstStride[]) {

K&R please, you already use it in other parts.

same below

> +static inline void rgb48togray16row_template(const uint16_t *src, uint16_t *dst, int length, int srcLE, int dstLE)

long line

> +static int rgb48togray16_template(SwsContext *c, uint8_t *src[],
> +                                         int srcStride[], int srcSliceY,
> +                                         int srcSliceH, uint8_t *dst[],
> +                                         int dstStride[], const int srcLE,
> +                                         const int dstLE) {

weird indentation, not K&R

> +#if HAVE_MMX
> +    __asm __volatile(

__asm__ volatile

> +#if HAVE_MMX
> +    __asm __volatile(

ditto, same below

> +SwsRGBFunc sws_hires_getRGBFunc(SwsContext *c)
> +{
> +    const enum PixelFormat srcFormat= c->srcFormat;
> +    const enum PixelFormat dstFormat= c->dstFormat;
> +    const int srcCbe= srcFormat==PIX_FMT_RGB48BE; /* components big-endian */
> +    const int dstCbe= dstFormat==PIX_FMT_RGB48BE;
> +    const int srcBpp= (fmt_depth(srcFormat) + 7) >> 3;
> +    const int dstBpp= (fmt_depth(dstFormat) + 7) >> 3;
> +    const int srcId= (fmt_depth(srcFormat) >> 2) - (srcBpp > 4); /* 1:0, 4:1, 8:2, 15:3, 16:4, 24:6, 32:8, 48:11 */
> +    const int dstId= (fmt_depth(dstFormat) >> 2) - (dstBpp > 4);
> +    SwsRGBFunc conv = NULL;

extra good karma for spaces around =, same below

> +    if (  (isBGR(srcFormat) && isBGR(dstFormat))
> +       || (isRGB(srcFormat) && isRGB(dstFormat))){

> +    }else if (  (isBGR(srcFormat) && isRGB(dstFormat))
> +             || (isRGB(srcFormat) && isBGR(dstFormat))){
> +        switch(srcId | (dstId<<4) | (srcCbe<<8) | (dstCbe<<12)){

> +    }else{

space around { }

> +    if (isGray16(srcFormat) && (dstFormat == PIX_FMT_RGB48BE ||
> +                                dstFormat == PIX_FMT_RGB48LE    ))
> +    {

> +    if (isGray16(dstFormat) && (srcFormat == PIX_FMT_RGB48BE ||
> +                                srcFormat == PIX_FMT_RGB48LE    ))
> +    {

{ on the same line

> --- hires.h	(revision 0)
> +++ hires.h	(revision 0)
> @@ -0,0 +1,33 @@
> +
> +#include <inttypes.h>

stdint.h should be enough.

> +#include "swscale.h"
> +#include "swscale_internal.h"

Why do you need both?

> --- swscale_template.c	(revision 29274)
> +++ swscale_template.c	(working copy)
> @@ -2130,6 +2130,86 @@
>  
> +static inline void RENAME(rgb48beToUV)(uint8_t *dstU, uint8_t *dstV, uint8_t *src1, uint8_t *src2, int width)

long line, same below

Diego