[FFmpeg-devel] [PATCH 01/10] zlib decoder

[Måns Rullgård]

Michael Niedermayer <michaelni at gmx.at> writes:

> Hi
>
> On Sun, Jul 15, 2007 at 09:12:21PM +0100, Mans Rullgard wrote:
> [...]
>> +struct AVInflateContext {
[...]
>> +};
>
> what about adding some doxygen comments which describe what all these
> variables mean?

Good idea I guess.

>> +static const unsigned short len_tab[29][2] = {
>
> what about subtracting 3 from [1], that way this table would need 58 bytes
> less

True, didn't think of that.

>> +static const unsigned short dist_tab[32][2] = {
>
> dist_tab[x][1] could theoretically be replaced by
> (dist_tab[x][1]<<dist_tab[x][0])+1
> then the table would also fit in 8bit but maybe thats not worth it



>> +        if (clen[i] > max_bits)
>> +            max_bits = clen[i];
>
> max_bits= FFMAX(max_bits, clen[i]);

Indeed.

>> +#define check_bits(label, gb, n)                                        \
>> +    case AV_INFLATE_ ## label:                                          \
>> +        ctx->state = AV_INFLATE_ ## label;                              \
>> +        if ((n) > (gb)->size_in_bits - get_bits_count(gb) && insize) {  \
>> +            needbits = n;                                               \
>> +            goto outbits;                                               \
>> +        }
>
> maybe macros should be all uppercase
> and iam tempted to suggest that the case AV_INFLATE_* and 
> ctx->state = AV_INFLATE_* could be added directly into the code without
> any macros, this would be more readable i think though it would increase
> the number of lines of source significantly, so iam not sure if its a
> good idea ...

Well, I found it more readable to hide the buffer switching code away
from the main decoding.

> then i would suggest to rename this to something like GOTO_OUTBITS_IF_END()
> or another name which says what is actually done

What?  Have we turned into Java programmers needing entire essays for
variable names?

>> +#define decode_code_lens()                                              \
>> +do {                                                                    \
>> +    ctx->i = 0;                                                         \
>> +    while (ctx->i < ctx->hlit + 257 + ctx->hdist + 1) {                 \
>> +        read_vlc(CLV, ctx->clv, &ctx->gb, cl_vlc);                      \
>> +                                                                        \
>> +        if (ctx->clv < 16) {                                            \
>> +            ctx->ll_len[ctx->i++] = ctx->clv;                           \
>> +        } else if (ctx->clv < 19) {                                     \
>> +            ctx->clr = 0;                                               \
>> +                                                                        \
>> +            if (ctx->clv == 16) {                                       \
>> +                read_bits(CLR1, ctx->clr, &ctx->gb, 2);                 \
>> +                ctx->clr += 3;                                          \
>> +                ctx->clv = ctx->ll_len[ctx->i-1];                       \
>> +            } else if (ctx->clv == 17) {                                \
>> +                read_bits(CLR2, ctx->clr, &ctx->gb, 3);                 \
>> +                ctx->clr += 3;                                          \
>> +                ctx->clv = 0;                                           \
>> +            } else if (ctx->clv == 18) {                                \
>> +                read_bits(CLR3, ctx->clr, &ctx->gb, 7);                 \
>> +                ctx->clr += 11;                                         \
>> +                ctx->clv = 0;                                           \
>> +            }                                                           \
>
> the above code contains 4 checks, case labels and all the other related
> code, thats alot even if its not vissible due to the macros ...
> why not check if theres 16+7 or whatever the max is bits available at one
> spot?

Probably no reason at all.

> read_vlc() itself also checks blindly for 16bit and not the proper vlc len

Yes, I know.  Thanks for reminding me.

>> +#define build_vlc_dynamic()                                             \
>> +do {                                                                    \
>> +    read_bits(HLIT, ctx->hlit, &ctx->gb, 5);                            \
>> +    read_bits(HDIST, ctx->hdist, &ctx->gb, 5);                          \
>> +    read_bits(HCLEN, ctx->hclen, &ctx->gb, 4);                          \
>
> 1 check is enough, theres no sense IMHO in being able to break out and
> continue between these 3

I guess not.

>> +static void
>> +copy_bytes(uint8_t *dst, uint8_t *src, unsigned int len)
>> +{
>> +    while (len--)
>> +        *dst++ = *src++;
>> +}
>
> i think we have such a copy routine somewhere already, but i dont remember
> where

Reimar said it looked familiar too.  Wherever it is, it's not in a
central location.  Where would be the appropriate place for a function
like this?

>> +        if (bp > ctx->bufsize) {
>> +            av_log(NULL, AV_LOG_ERROR, "offset too large: %d > %d\n",
>> +                   offset, os + ctx->bufsize);
>
> please add the needed AVClass or whatever to AVInflateContext or another
> appropriate place and use it for av_log()

Will do.

>> +#define STATE_START switch (ctx->state) { case AV_INFLATE_INIT:
>> +#define STATE(n)    case AV_INFLATE_ ## n:
>> +#define STATE_END   }
>
> what sense do these macros have?
> why not replace them by the single line they represent?

IIRC I was thinking of putting more stuff there at some point but
never needed to.

>> +        read_bits(BFINAL, ctx->bfinal, &ctx->gb, 1);
>> +        read_bits(BTYPE, ctx->btype, &ctx->gb, 2);
>
> these dont need the full case state check goto logic twice
>
> [...]
>> +            read_bits(NCLEN, ctx->nclen, &ctx->gb, 16);
>> +            read_bits(NCNLEN, nlen, &ctx->gb, 16);
>
> same as above

Yes.

>> +        if (ctx->container == CONTAINER_GZIP) {
>> +            read_bits(GZCRC, tmp, &ctx->gb, 16);
>> +            read_bits(GZISIZE, tmp, &ctx->gb, 16);
>> +        } else if (ctx->container == CONTAINER_ZLIB) {
>> +            read_bits(ZLIB_ADLER32_1, tmp, &ctx->gb, 16);
>> +            read_bits(ZLIB_ADLER32_2, tmp, &ctx->gb, 16);
>> +        }
>
> a single if(ctx->container != CONTAINER_RAW) check(32)_set_state_goto_magic
> seems enough, no need to do it 4 times

Yes.  It would also be good to actually compute and verify the checksums.

Thanks for your comments.  I initially focused on getting correct
operation at all.  Now I can look at simplifying bits like these.

-- 
M?ns Rullg?rd
mans at mansr.com