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

Michael Niedermayer michaelni
Mon Dec 22 21:06:37 CET 2008


On Mon, Dec 22, 2008 at 08:15:06PM +0200, Kostya wrote:
> On Mon, Dec 22, 2008 at 01:13:53PM +0100, Michael Niedermayer wrote:
> > On Mon, Dec 22, 2008 at 08:31:55AM +0200, Kostya wrote:
> > > I need that for resolving issue 750 on roundup.
[...]

> [...] 
> [RLE decoding code]
> >
> > 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.
> 
> full patch attached 

thank you


>  
> > [...]
> > > 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
> 
> added resyncs and replaced memcpy() with pointer swap 

[..]

> +void ff_ccitt_unpack_init()

av_cold


> +{
> +    static VLC_TYPE code_table1[528][2];
> +    static VLC_TYPE code_table2[648][2];
> +    int i;
> +    if(ccitt_group3_2d_vlc.table)
> +        return;

this is not thread safe, this is not the last value written.

and yes these functions can be called from several threads from
av_find_stream_info() ...
alternative solutions are welcome as well but as long as av_find_stream_info
does what it does all init must be thread safe.


> +    ccitt_vlc[0].table = code_table1;
> +    ccitt_vlc[0].table_allocated = 528;
> +    ccitt_vlc[1].table = code_table2;
> +    ccitt_vlc[1].table_allocated = 648;
> +    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, 2, 2,
> +                        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;
> +    while(pix_left){
> +        t = get_vlc2(gb, ccitt_vlc[mode].table, 9, 2);
> +        run += t;
> +        if(t == -1){
> +            av_log(avctx, AV_LOG_ERROR, "Incorrect code @ %d\n",avctx->width -pix_left);
> +            return -1;
> +        }
> +        if(t >= 64)
> +            continue;
> +        pix_left -= run;
> +        if(pix_left < 0){
> +            av_log(avctx, AV_LOG_ERROR, "Run went out of bounds\n");
> +            return -1;
> +        }
> +        *runs++ = run;
> +        run = 0;
> +        mode = !mode;
> +    }
> +    *runs++ = 0;
> +    return 0;
> +}

This still has 4 ifs on the most commonly excuted path, the code i provided
has 2, is there any reason why you do not use something based on just 2?

And just the clarify, the "continue" path can only be executed once every
64 pixels and thus is of less relevance speedwise.

also the code can end in a "infinite" and then segfaulting loop through the
continue


> +
> +static inline int decode_group3_2d_line(AVCodecContext *avctx, GetBitContext *gb,
> +                                        int pix_left, int *runs, const int *ref)
> +{

the function does not look speed critical enough for the inline ...


[...]

> +
> +static void put_line(uint8_t *dst, int size, int width, const int *runs)
> +{
> +    PutBitContext pb;
> +    int run, t, mode = 0, pix_left = width;
> +
> +    init_put_bits(&pb, dst, size*8);
> +    while(pix_left > 0){
> +        run = *runs++;
> +        pix_left -= run;

> +        while(run){
> +            t = FFMIN(run, 16);
> +            put_sbits(&pb, t, -mode);
> +            run -= t;
> +        }

for(; run>=16; run-=16)
    put_sbits(&pb, 16, -mode);
put_sbits(&pb, run, -mode);


[...]
> +int ff_ccitt_unpack_group3_1d(AVCodecContext *avctx,
> +                              const uint8_t *src, int srcsize,
> +                              uint8_t *dst, int height, int stride, int padded)
> +{
> +    int j;
> +    GetBitContext gb;
> +    int *runs;
> +
> +    runs = av_malloc(avctx->width * sizeof(runs[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(avctx, AV_LOG_ERROR, "EOL syncmarker not found, resyncing\n");
> +            do{
> +                skip_bits1(&gb);
> +            }while(get_bits_count(&gb) < srcsize*8 && show_bits(&gb, 12) != 1);
> +            if(get_bits_count(&gb) >= srcsize*8)
> +                return -1;
> +        }

how is this supposed to work?
once a line is damaged and lost there can be 1 line less thus this can
still run out of data and fail.


[...]
> +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 + 1) * sizeof(runs[0]));
> +    ref  = av_malloc((avctx->width + 1) * sizeof(ref[0]));
> +    runs[0] = 0;
> +    ref[0] = 0;
> +    ref[1] = avctx->width;
> +    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(avctx, AV_LOG_ERROR, "EOL syncmarker not found, resyncing\n");
> +            do{
> +                skip_bits1(&gb);
> +            }while(get_bits_count(&gb) < srcsize*8 && show_bits(&gb, 12) != 1);
> +            if(get_bits_count(&gb) >= srcsize*8)
> +                return -1;
> +        }

duplicate

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

He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
-------------- 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/d17f316f/attachment.pgp>



More information about the ffmpeg-devel mailing list