[FFmpeg-devel] [PATCH 01/10] zlib decoder
Vadim Lebedev
vadim
Mon Jul 16 01:37:10 CEST 2007
M?ns Rullg?rd wrote:
>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?
>
>
>
Why don't you simply use memcpy in this case?
>>>+ 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.
>
>
>
More information about the ffmpeg-devel
mailing list