[Ffmpeg-devel] [PATCH] TIFF encoder (Google SoC qualification task)

[Rich Felker]

On Mon, Apr 02, 2007 at 03:01:29PM +0200, Kamil Nowosad wrote:
> Hi
> 
> On Sun, Apr 01, 2007 at 08:23:41PM +0200, Michael Niedermayer wrote:
> > > -                *out++ = 0x80 | (count - 1);
> > > +                if (style == FF_RLE_SETBIT)
> > > +                    *out++ = 0x80 | (count - 1);
> > > +                else
> > > +                    *out++ = -count + 1;
> > 
> > this can be done without a branch, also it can be done more generically
> 
> Is this sufficent:
> 
> a = (uint8_t[]{   1,-1})[style];
> b = (uint8_t[]{0x80, 0})[style];

Syntax here is wrong... You meant:

a = ((uint8_t[]){   1,-1})[style];
b = ((uint8_t[]){0x80, 0})[style];

Better (cleaner syntax, less dependence on newer C features) would be
to declare a static const array explicitly and index into it.

> > > +#define COMPRESS(func) \
> > > +        for (y = 0; y < avctx->height; y += s->lines_per_strip) {\
> > > +            int i;\
> > > +            s->strip_offset[s->strip++] = CURR_OFFSET(s);\
> > > +            for (i = 0; (i < s->lines_per_strip) && (y + i < avctx->height); i++) {\
> > > +                func;\
> > > +                src += p->linesize[0];\
> > > +            }\
> > > +        }
> > > +    switch (s->compression) {
> > > +    case TIFF_RAW:
> > > +        if (s->buf_end - s->buf < avctx->height * s->bytes_per_line) {
> > > +            av_log(avctx, AV_LOG_ERROR, "the buffer given is too short\n");
> > > +            return -1;
> > > +        }
> > > +        COMPRESS(bytestream_put_buffer(&s->buf, src, s->bytes_per_line));
> > > +        break;
> > > +
> > > +    case TIFF_PACKBITS:
> > > +        COMPRESS(ret =
> > > +                 ff_encode_rle(s->buf, s->buf_end - s->buf, src,
> > > +                               s->bytes_per_line, 1, FF_RLE_NEG);
> > > +                 if (ret == -1) {
> > > +                     av_log(avctx, AV_LOG_ERROR,
> > > +                        "the buffer given is too short\n");
> > > +                     return -1;
> > > +                 }
> > > +                 s->buf += ret;);
> > > +        break;
> > 
> > hmm isnt this a little bit obfuscated? if theres no way to cleanly simplify
> > it then it might be better to leave it ...
> 
> I'll try to something with that.

Making a function-like macro that does not behave like a function is
bad. Put do { } while (0) around it so that it can be terminated with
; properly. Otherwise it mysteriously breaks when someone tries e.g.

if (foo) COMPRESS(...);
else ...;

Notice that your above code has a spurious semicolon (null statement)
after each 'call' to COMPRESS.

Rich