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

[Michael Niedermayer]

On Tue, Dec 23, 2008 at 09:00:57AM +0200, Kostya wrote:
> On Mon, Dec 22, 2008 at 09:06:37PM +0100, Michael Niedermayer wrote:
> > 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.
> [...]
> > > +{
> > > +    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.
>  
> now it should be thread safe
> 
> [...]
> > 
> > > +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?
> 
> Your code was wrong since it could not check for incorrect code (t = -1 was
> falling into t<64 condition). 

iam pretty sure my t was unsigned and unsigned -1 is not <64


> 
> > And just the clarify, the "continue" path can only be executed once every
> > 64 pixels and thus is of less relevance speedwise.
> 
> ?
> no, it's not a counter. For example, white run of 2000 pixels will be coded
> as two values: <1984><16>. See ITU T.4

yes, that calls it twice for 2000 pixels and 2/2000 < 1/64
so my argument stands the continue cannot happen more than once every 64
pixels


>  
> > also the code can end in a "infinite" and then segfaulting loop through the
> > continue
> 
> Probably not. When bitreader is out of data it will return zeroes and there's no
> all-zero code in the list, so get_vlc2() will return -1 eventually.

ok, ive missed that
i guess though this should be documented to stop people from "fixing" it

will review the patch later today

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

In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- 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/20081223/7f4f78af/attachment.pgp>