[FFmpeg-devel] [patch] mjpeg restart marker parsing

[Michael Niedermayer]

On Sat, Mar 27, 2010 at 11:44:00PM -0700, Tom Harper wrote:
> in the mjpeg code the parsing code is not checking to see if the proper 
> next restart marker is present at the assumed restart marker boundary- if 
> there are stuffing bits for example present instead of the marker the 
> following error is often seen:
> [mjpeg @ 6B0445C4]
> sequencial DCT  p:0 >>:0 ilv:63 bits:8
> [mjpeg @ 6B0445C4]
> mjpeg_decode_dc: bad vlc: 0:0 (01BBDAA0)
>
> so this error causes the decoder to bail out, resulting in the picture not 
> being fully decoded.  The restart markers are supposed to provide recovery 
> points but that is another patch for another day.
>
> In any case I fixed the basic case, which is to look for the actual restart 
> marker at or near where the next marker is supposed to be.  So that is the 
> purpose of this patch.
>
> Feedback appreciated, I think particularly for mjpeg web cameras this is a 
> helpful patch.
>
> Tom

>  mjpegdec.c |   16 +++++++++++++---
>  mjpegdec.h |    1 +
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 1a8023786b4937e226756792716660615ad4749a  ffmpeg_mjpeg_restart_marker_patch.diff
> Index: mjpegdec.c
> ===================================================================
> --- mjpegdec.c	(revision 1316)
> +++ mjpegdec.c	(working copy)
> @@ -764,7 +764,7 @@
>  }
>  
>  static int mjpeg_decode_scan(MJpegDecodeContext *s, int nb_components, int Ah, int Al){
> -    int i, mb_x, mb_y;
> +    int i, mb_x, mb_y, marker;
>      uint8_t* data[MAX_COMPONENTS];
>      int linesize[MAX_COMPONENTS];
>  

> @@ -828,10 +828,19 @@
>                      }
>                  }
>              }
> -
>              if (s->restart_interval && !--s->restart_count) {

cosmetic


>                  align_get_bits(&s->gb);
> -                skip_bits(&s->gb, 16); /* skip RSTn */
> +                marker = 0xFF;
> +				/* remove stuffing bits */

tabs

> +                while (marker == 0xFF) 
> +                {
> +                    marker = get_bits(&s->gb, 8);
> +                }

do{}while()
and you should possibly check for the end of the buffer


> +                if (marker != (s->next_restart_marker+RST0))
> +                {
> +                    av_log(s->avctx, AV_LOG_ERROR, "unexpected restart marker %d vs %d\n",marker,s->next_restart_marker+RST0);
> +                }
> +                s->next_restart_marker = (s->next_restart_marker + 1) & 7;

if its a differnt number this fails, this seems not ideal

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

Good people do not need laws to tell them to act responsibly, while bad
people will find a way around the laws. -- Plato
-------------- 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/20100328/94f10a77/attachment.pgp>