[FFmpeg-devel] [PATCH] libavfilter-soc: make hflip use pixdesc stuff

[Benoit Fouet]

Hi Stefano,

On 04/02/2009 01:12 AM, Stefano Sabatini wrote:
> Hi,
>
> just a little sample of the new colorspace conversion system, this
> works with all formats!
>
>   

(I'm reviewing it, just in case the write_line appears some time)

> Regards.
>   

> Index: libavfilter-soc/ffmpeg/libavfilter/vf_hflip.c
> ===================================================================
> --- libavfilter-soc.orig/ffmpeg/libavfilter/vf_hflip.c    2009-04-02
00:54:53.000000000 +0200
> +++ libavfilter-soc/ffmpeg/libavfilter/vf_hflip.c    2009-04-02
01:07:02.000000000 +0200
>
> [...]
>
> +    if (!(flip->line = av_malloc(sizeof(int16_t) * link->w))) {
> +        av_log(ctx, AV_LOG_ERROR, "Memory problem, too bad");
> +        return -1;
> +    }
> 

sizeof(*flip->line)
no need for av_log()
return AVERROR(ENOMEM);

> +static inline void write_line_flipped(const uint16_t *src, uint8_t
*data[4], const int linesize[4],
> +                                      const AVPixFmtDescriptor *desc,
int x, int y, int c, int w)
>

please keep lines under 80 characters

> +    AVComponentDescriptor comp= desc->comp[c];
> +    int plane= comp.plane;
> +    int depth= comp.depth_minus1+1;
> +    int mask = (1<<depth)-1;
> +    int shift= comp.shift;
> +    int step = comp.step_minus1+1;
> +    int flags= desc->flags;
>

please keep a space between name and '=', e.g. int plane = comp.plane
(same applies in other places as well)
depth and shift declaration could be moved in the non bitstream case

> +    if (flags & PIX_FMT_BITSTREAM){
> +        PutBitContext pb;
> +        int skip;
> +
> +        init_put_bits(&pb, data[plane] + y*linesize[plane],
linesize[plane]*8);
> +        skip = x*step + comp.offset_plus1-1;
>

merge with declaration

> +        if (skip) skip_put_bits(&pb, skip);
> +

if (skip)
    skip_put_bits(...);

> +        while(w--){
> +            int val = *src--;
>

add a blank line here

> +            put_bits(&pb, depth, val);
> +            if (step-depth) skip_put_bits(&pb, step-depth);
>

if (step - depth)
    skip_put_bits(...);

> +        }
> +    } else {
> +        uint8_t *p = data[plane]+ y*linesize[plane] + x*step +
comp.offset_plus1-1;
>

add a blank line here

> +        while(w--){
> +            uint16_t val;
> +
> +            if (flags & PIX_FMT_BE) val= AV_RB16(p);
> +            else                    val= AV_RL16(p);
> +            val = (val & (~(mask<<shift))) | *src--<<shift;
> +
> +            if (flags & PIX_FMT_BE) AV_WB16(p, val);
> +            else                    AV_WL16(p, val);
> +            p+= step;
> +        }
>      }
>

I'd prefer to see:
if (flags & PIX_FMT_BE) {
    while (w--) {
        uint16_t val = (AV_RB16(p) & ~(mask<<shift)) | (*src--<<shift);

        AV_WB16(p, val);
        p += step;
    }
} else {
    while (w--) {
        /* same with LE */
    }
}

This (sort of) duplicates some code, but might also be faster...
(I just don't really like avoidable branch conditions in inner loops :) )

> +        for (i = y1; i < h1; i++) {
> +            read_line( flip->line,
>

drop the extra space

Ben