[FFmpeg-devel] [PATCH] CCITT Group 3 and 4 decompression

[Michael Niedermayer]

On Mon, Dec 22, 2008 at 08:31:55AM +0200, Kostya wrote:
> I need that for resolving issue 750 on roundup.

This is not a complete patch, it just adds unused functions.


[...]
> static const int ccitt_syms[CCITT_SYMS] = {
>     0,    1,    2,    3,    4,    5,    6,    7,    8,    9,   10,   11,   12,
>    13,   14,   15,   16,   17,   18,   19,   20,   21,   22,   23,   24,   25,
>    26,   27,   28,   29,   30,   31,   32,   33,   34,   35,   36,   37,   38,
>    39,   40,   41,   42,   43,   44,   45,   46,   47,   48,   49,   50,   51,
>    52,   53,   54,   55,   56,   57,   58,   59,   60,   61,   62,   63,   64,
>   128,  192,  256,  320,  384,  448,  512,  576,  640,  704,  768,  832,  896,
>   960, 1024, 1088, 1152, 1216, 1280, 1344, 1408, 1472, 1536, 1600, 1664, 1728,
>  1792, 1856, 1920, 1984, 2048, 2112, 2176, 2240, 2304, 2368, 2432, 2496, 2560,
>  CCITT_CODE_EOL
> };

uint16_t


[...]
> void ff_ccitt_unpack_init()
> {
>     static VLC_TYPE code_table1[536][2];
>     static VLC_TYPE code_table2[652][2];
>     int i;

>     if(ccitt_vlc[0].table)
>         return;
>     ccitt_vlc[0].table = code_table1;
>     ccitt_vlc[0].table_allocated = 536;

this is not thread safe, you must check the LAST entry changed


>     ccitt_vlc[1].table = code_table2;
>     ccitt_vlc[1].table_allocated = 652;
>     for(i = 0; i < 2; i++){
>         init_vlc_sparse(&ccitt_vlc[i], 9, CCITT_SYMS,
>                         ccitt_codes_lens[i], 1, 1,
>                         ccitt_codes_bits[i], 1, 1, 
>                         ccitt_syms, sizeof(ccitt_syms[0]), sizeof(ccitt_syms[0]),
>                         INIT_VLC_USE_NEW_STATIC);
>     }
>     INIT_VLC_STATIC(&ccitt_group3_2d_vlc, 9, 11,
>                     ccitt_group3_2d_lens, 1, 1,
>                     ccitt_group3_2d_bits, 1, 1, 512);
> }
> 
> 

> static inline int decode_group3_1d_line(AVCodecContext *avctx, GetBitContext *gb,
>                                         int pix_left, int *runs)
> {
>     int mode = 0, run = 0, t;
>     for(;;){
>         t = get_vlc2(gb, ccitt_vlc[mode].table, 9, 2);
>         if(t == -1){
>             av_log(avctx, AV_LOG_ERROR, "Incorrect code @ %d\n",avctx->width -pix_left);
>             return -1;
>         }
>         if(t == CCITT_CODE_EOL)
>             break;
>         run += t;
>         if(t < 64){
>             *runs++ = run;
>             if(pix_left < run){
>                 av_log(avctx, AV_LOG_ERROR, "Run went out of bounds\n");
>                 return -1;
>             }
>             pix_left -= run;
>             run = 0;
>             mode = !mode;
>             if(!pix_left)
>                 break;
>         }
>     }

dont put all the almost never true if() on the path that will be
excuted 99% of the time
also dont store invalid values in case an error has been detected


int mode = 0, run = 0;
for(;;){
    unsigned int t = get_vlc2(gb, ccitt_vlc[mode].table, 9, 2);
    run += t;
    if(t < 64){
        *runs++ = run;
        pix_left -= run;
        if(pix_left <= 0){
            if(!pix_left)
                break;
            runs[-1] += pix_left;
            av_log(avctx, AV_LOG_ERROR, "Run went out of bounds\n");
            return -1;
        }

        run = 0;
        mode = !mode;
    }else{
        if(t == CCITT_CODE_EOL) {
            break;
        } else if(t == -1){
            av_log(avctx, AV_LOG_ERROR, "Incorrect code @ %d\n",avctx->width -pix_left);
            return -1;
        }
    }
}

also there are 2 ways to break out of this without error, are both valid
according to specs?
if false the code is broken

Is it allowed to have a CCITT_CODE_EOL before actually reaching the end?
Either way the code looks broken if such a case is encountered

ill review the rest once this is a complete test that can be tested with
actual files and a fuzzer.


[...]
> int ff_ccitt_unpack_group3_2d(AVCodecContext *avctx,
>                               const uint8_t *src, int srcsize,
>                               uint8_t *dst, int height, int stride, int padded)
> {
>     int j;
>     GetBitContext gb;
>     int *runs, *ref;
>     int ret;
> 
>     runs = av_malloc(avctx->width * sizeof(runs[0]));
>     ref = av_malloc((avctx->width + 1) * sizeof(ref[0]));
>     init_get_bits(&gb, src, srcsize*8);
>     for(j = 0; j < height; j++){
>         if(padded){
>             int bits = get_bits_count(&gb) & 7;
>             if(bits != 4)
>                 skip_bits(&gb, (12 - bits) & 7);
>         }
>         if(get_bits(&gb, 12) != 1){
>             av_log(NULL,0,"EOL syncmarker not found\n");
>             return -1;
>         }
>         memset(runs, 0, avctx->width * sizeof(runs[0]));
>         if(get_bits1(&gb))
>             ret = decode_group3_1d_line(avctx, &gb, avctx->width, runs);
>         else
>             ret = decode_group3_2d_line(avctx, &gb, avctx->width, runs, ref);
>         if(ret < 0){
>             av_free(runs);
>             av_free(ref);
>             return -1;
>         }
>         put_line(dst, stride, avctx->width, runs);

>         memcpy(ref + 1, runs, avctx->width * sizeof(ref[0]));

this memcpy seems unneeded
and the code is very fragile, the format has syncmarkers, please use them if
there was an error


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

No great genius has ever existed without some touch of madness. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081222/962f367d/attachment.pgp>