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

[Michael Niedermayer]

On Thu, Dec 25, 2008 at 08:19:25AM +0200, Kostya wrote:
> On Wed, Dec 24, 2008 at 11:00:08PM +0100, Michael Niedermayer wrote:
> > On Wed, Dec 24, 2008 at 08:06:13AM +0200, Kostya wrote:
> > > On Tue, Dec 23, 2008 at 08:27:31PM +0100, Michael Niedermayer wrote:
> > > > 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 int find_group3_syncmarker(GetBitContext *gb, int srcsize)
> > > +{
> > > +    int state = get_bits(gb, 12);
> > > +    int rem = srcsize - get_bits_count(gb);
> > > +    while((state & 0xFFF) != 1){
> > > +        state = (state << 1) | get_bits1(gb);
> > > +        if(--rem <= 0)
> > > +            return -1;
> > > +    }
> > > +    return 0;
> > > +}
> > 
> > you only need 1 get_bits call in this function
> > besides this code is buggy, it will miss the sync code when its at
> > the end, that is it will never check the last read bit
> > Ive already provided a working implementation, why dont you use it?
> 

> In most cases EOL will follow line data immediately, so the loop won't
> be entered at all instead of 12 iterations in your case. 

it really doesnt matter speedwise, this code isnt executed often enough
but if its important for you, leave it there.


> Also it does
> not matter for decoding if there's EOL right one bit before line end -
> there won't be sufficient data to decode that line anyway (for 2d-case
> that single bit is read to determine line coding mode and for 1d case
> first codeword should have length >= 4 bits).

its imporant to detect errors, for that it is important to find out if
a sync marker is missing, this also includes the last sync marker.

i can offer to cleanup, commit and maintain CCITT fax support myself if
you dont want to fix the issues?

[...]
> > btw, do you have some url where i can find files to test this code?
> 
> http://samples.mplayerhq.hu/image-samples/tiff/
>  

thanks


> > PS: merry christmas :)
> 
> Well, I live in a country where church calendar uses incorrect rounding
> and was tied to the standard one only in XXth century with 13 days of difference
> we have New Year first, then Christmas on Jan 7th and then Old New Year on
> Jan 14th (i.e.  New Year in Old calendar). Pretty amusing, eh?

a few days off isnt that bad ...
keep in mind that the whole calendar the world is using is a few years off,
wiki says something between 2 and 7 years.


[...]
> @@ -103,6 +105,26 @@
>              return -1;
>          }
>      }
> +    if(s->compr == TIFF_CCITT_RLE || s->compr == TIFF_G3 || s->compr == TIFF_G4){
> +        int i, ret = 0;
> +        uint8_t *src2 = av_malloc(size);
> +
> +        for(i = 0; i < size; i++)
> +            src2[i] = ff_reverse[src[i]];

out of mem check missing


[...]
> +static int decode_group3_1d_line(AVCodecContext *avctx, GetBitContext *gb,
> +                                 int pix_left, int *runs)
> +{
> +    int mode = 0, run = 0;
> +    unsigned int t;
> +    for(;;){
> +        t = get_vlc2(gb, ccitt_vlc[mode].table, 9, 2);
> +        run += t;
> +        if(t < 64){

first


> +            pix_left -= run;
> +            *runs++ = run;

> +            if(!pix_left){

second


> +                break;
> +            }else if(pix_left < 0){

third

still 3

its really easy to just merge the 2nd and 3rd

if(pix_left <= 0){
    if(!pix_left)
        break;
    ...
}

just think of how often each side is true

if(!pix_left)     true maximally once at the end of a undamaged line
if(pix_left<0)    true maximally once in case of an overrun of a damaged line

while the code in the if(t<64) branch is executed once per run, and there
can be 1 run per pixel, thats for a 1024x768 image 1024 times more
often (in worst case) and still equally often if you encode just a empty
white page

if you write
if(!pix_left){
...
else if(pix_left < 0){

then there are 2 if() that is 2 for each run has to be checked

if you write 

if(pix_left <= 0){
    if(!pix_left)
        break;
    ...
}

there are just half as many

also, what about runs of 0 ?
if they are not allowed (except at pos 0) that should be checked for and an
error be printed. Its easy to check for 0 before the loop and remove the 0
code from the table ...
Alternatively, if 0 is allowed at any place that should be supported, iam
not sure if it is currently ...

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

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- 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/20081225/ba9f8591/attachment.pgp>