[FFmpeg-devel] [Patch] QT RLE encoder, bis

[Michael Niedermayer]

Hi

On Wed, Jun 27, 2007 at 08:30:59AM +0200, Alexis Ballier wrote:
> Hi,
> 
> >[...]
> >> +/**
> >
> >trailing whitespace
> >may i suggest that you try grep ' $'
> 
> 
> Hmm good idea, I didn't even think about doing it like that, I was
> stupidly manually searching for them while rereading the code :/
> 
> 
> >[...]
> >> +    if((start_line==0 && end_line == s->avctx->height) || 
> >start_line==s->avctx->height)
> >
> >the () here is superfluos too, though its less annoying than (a*b)+c
> >so if you prefer the extra () here i wont mind keeping that though 
> >personally
> >id remove it if it where my code
> 
> 
> removed
> 
> 
> >[...]
> >> +static int qtrle_encode_frame(AVCodecContext *avctx, uint8_t *buf, int 
> >buf_size, void *data)
> >> +{
> >> +    QtrleEncContext * const s = (QtrleEncContext *)avctx->priv_data;
> >
> >unneeded cast
> 
> Indeed, removed all of them
> 
> Also changed to "temp_cost = 1 + s->pixel_size + !i;", with some luck
> gcc might optimize it better
> 
> 
> I tried to improve style consistency with things like :
> - a space after a comma in function calls, space after ';' in for's
> - a space around '+', '-', '>' & friends, '=', but not '*', which I
> find more readable (but this is discussable with the "space after a
> comma" in function calls)
> - if/for/while *space* (condition) *space* {
> 
> There was something I wasn't sure :
> 
> if (!s->frame.key_frame && !memcmp(this_line, prev_line, s->pixel_size))
>    skipcount  = FFMIN(skipcount + 1, MAX_RLE_SKIP);
> else skipcount = 0;
> 
> I didn't know if you prefered 4 spaces indentation or better
> alignment, I assumed 4 spaces had preference

if (!s->frame.key_frame && !memcmp(this_line, prev_line, s->pixel_size))
    skipcount = FFMIN(skipcount + 1, MAX_RLE_SKIP);
else
    skipcount = 0;

4 space and aligned, sadly its 1 line longer


[...]
> +    s->length_table  = av_mallocz(s->avctx->width*sizeof(int));

[...]
> +    uint8_t *this_line = p->data[0] + line*p->linesize[0] + (width - 1)*s->pixel_size;
> +    uint8_t *prev_line = s->previous_frame.data[0] + line*p->linesize[0] + (width - 1)*s->pixel_size;

uint8_t *this_line = p->               data[0] + line*p->linesize[0] + (width - 1)*s->pixel_size;
uint8_t *prev_line = s->previous_frame.data[0] + line*p->linesize[0] + (width - 1)*s->pixel_size;


> +
> +    s->length_table[width] = 0;

isnt the table just width entries long?


[...]
> +    this_line = p->data[0] + line*p->linesize[0];
> +    prev_line = s->previous_frame.data[0] + line*p->linesize[0];

this_line = p->               data[0] + line*p->linesize[0];
prev_line = s->previous_frame.data[0] + line*p->linesize[0];

[...]
> +    bytestream_put_byte(&buf, 0);                     // zero skip code = frame finished
> +    AV_WB32(orig_buf, buf - orig_buf);      // patch the chunk size

the // could be aligned vertically

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

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070629/ec7c7500/attachment.pgp>