[Ffmpeg-devel] Incorrect JPEG decoding with patch

[Michael Niedermayer]

Hi

On Fri, Nov 17, 2006 at 05:25:02PM +0100, Cyril Russo wrote:
> Hi all,
> 
> I've reported earlier that there was a bug with ffmpeg while decoding 
> my pictures.
> I've dug into mjpeg.c file, and I've found that ffmpeg didn't support 
> multiple scans JPEG.
> 
> Well, not yet ;-)
> 
> I've modified the JPEG decoder so it can handle multiple scans JPEG too.
> Currently, 95% of JPEG file have only one interleaved scan.
> Bad luck, the JPEG stream I must decode are 3 single scans (Y, Cr, Cb).
> 
> Multiple scans are allowed in JFIF specification 1.02, but lacked until 
> now in current mjpeg.c implementation.
> I've attached the patch, and I ask the devs for review.
> For testing the new features, download and decode from 
> http://xryl669ny.free.fr/MyFile000.jpg
> 
> BTW, grep for "Cyril Russo" for all modifications I've added in final 
> patched file.

as diego said dont add your name all over the place, if every of the
hundreads of contributors did that we would end up with more names then
code :)

svn annotate mjpeg.c + svn log mjpeg.c will provide the same info

also as diego said feel free to add yourself to the top like alex did with
 * Support for external huffman table, various fixes (AVID workaround),
 * aspecting, new decode_frame mechanism and apple mjpeg-b support
 *                                  by Alex Beregszaszi <alex at naxine.org>


> 
> I've tried to follow ffmpeg coding guidelines. I haven't subscribe to 
> the list, so for reply use cyril dot russo AT nexvision in france ;-)
[...]

 
> @@ -1532,6 +1533,46 @@ static int mjpeg_decode_scan(MJpegDecode
>      return 0;
>  }
>  
> +/* Added by Cyril Russo, this functions decode a scan when multiple scan are present inside the JPEG stream */
> +static int mjpeg_decode_scan(MJpegDecodeContext *s, int id){
> +    int mb_x, mb_y;
> +	int c = s->comp_index[id]; 
> +    for(mb_y = 0; mb_y < s->mb_height; mb_y++) {
> +        for(mb_x = 0; mb_x < s->mb_width; mb_x++) {
> +            uint8_t *ptr;
> +
> +            if (s->restart_interval && !s->restart_count)
> +                s->restart_count = s->restart_interval;
> +
> +            memset(s->block, 0, sizeof(s->block));
> +            if (decode_block(s, s->block, id,
> +                             s->dc_index[c], s->ac_index[c],
> +                             s->quant_matrixes[ s->quant_index[c]]) < 0) {
> +                dprintf("error y=%d x=%d\n", mb_y, mb_x);
> +                return -1;
> +            }
> +//          dprintf("mb: %d %d processed\n", mb_y, mb_x);
> +            ptr = s->picture.data[id] +
> +                (((s->linesize[id] * mb_y * 8) +
> +                mb_x * 8) >> s->avctx->lowres);
> +            if (s->interlaced && s->bottom_field)
> +                ptr += s->linesize[id] >> 1;
> +//av_log(NULL, AV_LOG_DEBUG, "%d %d %d %d %d %d %d %d \n", mb_x, mb_y, x, y, c, s->bottom_field, (v * mb_y + y) * 8, (h * mb_x + x) * 8);
> +            s->idct_put(ptr, s->linesize[id], s->block);
> +            
> +            /* (< 1350) buggy workaround for Spectralfan.mov, should be fixed */
> +            if (s->restart_interval && (s->restart_interval < 1350) &&
> +                !--s->restart_count) {
> +                align_get_bits(&s->gb);
> +                skip_bits(&s->gb, 16); /* skip RSTn */
> +                s->last_dc[id] = 1024;
> +            }
> +        }
> +    }
> +    return 0;
> +}

can the normal mjpeg_decode_scan() with nb_components=1 instead of 3 not
be used?


> +
> +
>  static int mjpeg_decode_sos(MJpegDecodeContext *s)
>  {
>      int len, nb_components, i, h, v, predictor, point_transform;
[...]
>      /* XXX: only interleaved scan accepted */
>      if ((nb_components != s->nb_components) && !s->ls)
>      {
> -        dprintf("decode_sos: components(%d) mismatch\n", nb_components);
> -        return -1;
> +        dprintf("decode_sos: multiple scan detected, id of current ");
> +        /* return -1; */

if something is wrong remove it, dont comment it out


>      }
>      vmax = 0;
>      hmax = 0;
> @@ -1605,14 +1649,23 @@ static int mjpeg_decode_sos(MJpegDecodeC
>      skip_bits(&s->gb, 4); /* Ah */
>      point_transform= get_bits(&s->gb, 4); /* Al */
>  
> -    for(i=0;i<nb_components;i++)
> -        s->last_dc[i] = 1024;
>  
> +	/* CR, Accept multiple components */
>      if (nb_components > 1) {
>          /* interleaved stream */
> +		for(i=0;i<nb_components;i++) s->last_dc[i] = 1024;

cant the last_dc thing be moved into the loop above (would avoid it being
duplicated if its possible)?

[...]
> @@ -1620,8 +1673,9 @@ static int mjpeg_decode_sos(MJpegDecodeC
>          s->nb_blocks[0] = 1;
>          s->h_scount[0] = 1;
>          s->v_scount[0] = 1;
> -    }
>  
> +	}
> +	

cosmetical change


[...]
> @@ -1648,8 +1702,9 @@ static int mjpeg_decode_sos(MJpegDecodeC
>              }
>          }
>      }else{
> -        if(mjpeg_decode_scan(s) < 0)
> -            return -1;
> +		if (nb_components == 3)		 { if(mjpeg_decode_iscan(s) < 0) return -1; }
> +		else if (nb_components == 1) { if(mjpeg_decode_scan(s, index) < 0) return -1; }
> +		else { dprintf("invalid number of components : %d\n", nb_components); return -1; }

please dont put that all on one line its as readable as
pleasedontputthatallononelineitsasreadableasthisline :)

[...]

also tabs are forbidden in ffmpeg, please replace them by spaces, a simple
search and replace should do the trick

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is