[FFmpeg-devel] [PATCH 01/10] zlib decoder
Michael Niedermayer
michaelni
Sun Jul 15 23:14:47 CEST 2007
Hi
On Sun, Jul 15, 2007 at 09:12:21PM +0100, Mans Rullgard wrote:
[...]
> +struct AVInflateContext {
> + unsigned int container;
> +
> + enum AVInflateState state;
> +
> + GetBitContext gb;
> + unsigned int skip;
> +
> + VLC cl_vlc;
> + VLC ll_vlc;
> + VLC dist_vlc;
> +
> + unsigned int bfinal;
> + unsigned int btype;
> +
> + unsigned int nclen;
> +
> + uint8_t ll_len[288 + 32];
> +
> + unsigned int hlit;
> + unsigned int hdist;
> + unsigned int hclen;
> +
> + uint8_t cl_len[19];
> + unsigned int ncl;
> +
> + unsigned int clv;
> + unsigned int clr;
> +
> + unsigned int i;
> +
> + unsigned int code;
> + unsigned int len;
> + unsigned int dist;
> +
> + unsigned int gzflags;
> + unsigned int gzcomp;
> + unsigned int gzskip;
> +
> + unsigned int tailsize;
> + unsigned int tailbits;
> + uint8_t tailbuf[8];
> +
> + unsigned int cpoff;
> + unsigned int cplen;
> +
> + unsigned int bufsize;
> + uint8_t *buf;
> +};
what about adding some doxygen comments which describe what all these
variables mean?
[...]
> +static const unsigned short len_tab[29][2] = {
> + { 0, 3 },
> + { 0, 4 },
> + { 0, 5 },
> + { 0, 6 },
> + { 0, 7 },
> + { 0, 8 },
> + { 0, 9 },
> + { 0, 10 },
> + { 1, 11 },
> + { 1, 13 },
> + { 1, 15 },
> + { 1, 17 },
> + { 2, 19 },
> + { 2, 23 },
> + { 2, 27 },
> + { 2, 31 },
> + { 3, 35 },
> + { 3, 43 },
> + { 3, 51 },
> + { 3, 59 },
> + { 4, 67 },
> + { 4, 83 },
> + { 4, 99 },
> + { 4, 115 },
> + { 5, 131 },
> + { 5, 163 },
> + { 5, 195 },
> + { 5, 227 },
> + { 0, 258 },
> +};
what about subtracting 3 from [1], that way this table would need 58 bytes
less
> +
> +static const unsigned short dist_tab[32][2] = {
> + { 0, 1 },
> + { 0, 2 },
> + { 0, 3 },
> + { 0, 4 },
> + { 1, 5 },
> + { 1, 7 },
> + { 2, 9 },
> + { 2, 13 },
> + { 3, 17 },
> + { 3, 25 },
> + { 4, 33 },
> + { 4, 49 },
> + { 5, 65 },
> + { 5, 97 },
> + { 6, 129 },
> + { 6, 193 },
> + { 7, 257 },
> + { 7, 385 },
> + { 8, 513 },
> + { 8, 769 },
> + { 9, 1025 },
> + { 9, 1537 },
> + { 10, 2049 },
> + { 10, 3073 },
> + { 11, 4097 },
> + { 11, 6145 },
> + { 12, 8193 },
> + { 12, 12289 },
> + { 13, 16385 },
> + { 13, 24577 },
> +};
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]);
[...]
> +#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 ...
then i would suggest to rename this to something like GOTO_OUTBITS_IF_END()
or another name which says what is actually done
[...]
> +#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?
read_vlc() itself also checks blindly for 16bit and not the proper vlc len
[...]
> +#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
[...]
> +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
[...]
> + 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()
[...]
> +#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?
[...]
> + 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
[...]
> + 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
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- 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/20070715/b6f1c809/attachment.pgp>
More information about the ffmpeg-devel
mailing list