[FFmpeg-devel] MJPEG bug

Michael Niedermayer michaelni at gmx.at
Fri Oct 7 16:48:50 CEST 2011


On Thu, Oct 06, 2011 at 08:25:04PM -0700, Aaron Miller wrote:
> Michael,
> 
> This appears to fix it.  I did not get any errors when running it on the
> problematic file I've been working with, but I have yet to test it on other
> files.  The output plays fine.
> 
> This uses the state64 field of the ParseContext structure in a non-standard
> way, that invalidates the comment in the header file: it stores two 16 bit
> variables in the low 32 bits of state64.

Please add a MjpegParser context with 2 seperate variables


> 
> Cheers,
> Aaron
> 
> On Mon, Oct 3, 2011 at 9:33 AM, Michael Niedermayer <michaelni at gmx.at>wrote:
> 
> > On Sun, Oct 02, 2011 at 11:56:44PM -0700, Aaron Miller wrote:
> > > Michael,
> > >
> > > Here is one problem frame from the (large) raw MJPEG stream I have.
> > >
> > > And thanks for the tip about mjpeg_parser.c! I believe that's what I need
> > to
> > > change.
> >
> > ok, if you manage to solve it please send a patch, if not please tell
> > me and ill fix it
> >
> > [...]
> >
> > --
> > Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> >
> > Freedom in capitalist society always remains about the same as it was in
> > ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> >

>  mjpeg_parser.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 51 insertions(+), 8 deletions(-)
> a2b4a2cf894114f40780cc14338b84384232d7b3  mjpeg_parser.c.diff
> --- libavcodec_mjpeg_parser.c	2011-10-06 18:52:01.000000000 -0700
> +++ mjpeg_parser.c	2011-10-06 18:55:14.000000000 -0700
> @@ -36,18 +36,41 @@
>  static int find_frame_end(ParseContext *pc, const uint8_t *buf, int buf_size){
>      int vop_found, i;
>      uint16_t state;
> +    uint16_t bytes_to_length; // number of bytes before the 2-byte length has been read into state (above)
> +    uint16_t payload_remaining; // number of bytes left in the payload
>  
>      vop_found= pc->frame_start_found;
>      state= pc->state;
> +    bytes_to_length = pc->state64 >> 16;
> +    payload_remaining = pc->state64 & 0xffff;
>  
>      i=0;
>      if(!vop_found){
>          for(i=0; i<buf_size; i++){
>              state= (state<<8) | buf[i];
> -            if(state == 0xFFD8){
> -                i++;
> -                vop_found=1;
> -                break;
> +            if (bytes_to_length) {
> +                if (bytes_to_length == 1) {
> +                    bytes_to_length = 0;
> +                    payload_remaining = state - 1; // 2-byte length includes itself (but not the marker just passed)
> +                } else {
> +                    bytes_to_length--;
> +                }
> +            } else {
> +                if (payload_remaining) {
> +                    payload_remaining--;
> +                    continue;

running this complicated loop per byte is quite slow. The payload could
in many cases just be skiped at least partially


> +                }
> +                if ((0xffc0 <= state) && (state <= 0xfffe)) {
> +                    if(state == 0xFFD8){
> +                        i++;
> +                        vop_found=1;
> +                        pc->state64=0;  // not sure if necessary
> +                        break;
> +                    } else if ((state <= 0xffcf) || (state >= 0xffda)) {
> +                        // has 2-byte length and payload
> +                        bytes_to_length = 2;
> +                    }
> +                }
>              }
>          }
>      }
> @@ -58,15 +81,35 @@
>              return 0;
>          for(; i<buf_size; i++){
>              state= (state<<8) | buf[i];
> -            if(state == 0xFFD8){
> -                pc->frame_start_found=0;
> -                pc->state=0;
> -                return i-1;
> +            if (bytes_to_length) {
> +                if (bytes_to_length == 1) {
> +                    bytes_to_length = 0;
> +                    payload_remaining = state - 1; // 2-byte length includes itself (but not the marker just passed)
> +                } else {
> +                    bytes_to_length--;
> +                }
> +            } else {
> +                if (payload_remaining) {
> +                    payload_remaining--;
> +                    continue;
> +                }
> +                if ((0xffc0 <= state) && (state <= 0xfffe)) {
> +                    if(state == 0xFFD8){
> +                        pc->frame_start_found=0;
> +                        pc->state=0;
> +                        pc->state64=0;  // bytes_to_length and payload_remaining
> +                        return i-1;
> +                    } else if ((state <= 0xffcf) || (state >= 0xffda)) {
> +                        // has 2-byte length and payload
> +                        bytes_to_length = 2;
> +                    }
> +                }
>              }
>          }
>      }
>      pc->frame_start_found= vop_found;
>      pc->state= state;
> +    pc->state64 = (bytes_to_length << 16) | payload_remaining;
>      return END_NOT_FOUND;
>  }
>  

>  mjpegdec.new.c |   25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 5fdd950e91159e7ee0f464155504fd31805a8b51  mjpegdec.c.diff
> --- /Users/atlas/mjpegdec.old.c	2011-10-06 19:42:05.000000000 -0700
> +++ /Users/atlas/mjpegdec.new.c	2011-10-06 20:02:29.000000000 -0700
> @@ -1271,7 +1271,8 @@
>  
>  /* return the 8 bit start code value and update the search
>     state. Return -1 if no start code found */
> -static int find_marker(const uint8_t **pbuf_ptr, const uint8_t *buf_end)
> +static int find_marker(const uint8_t **pbuf_ptr, const uint8_t *buf_end,
> +                       const uint8_t **payload_end)
>  {
>      const uint8_t *buf_ptr;
>      unsigned int v, v2;
> @@ -1281,6 +1282,10 @@
>  #endif
>  
>      buf_ptr = *pbuf_ptr;
> +    if (*payload_end && *payload_end > buf_ptr) {
> +        buf_ptr = *payload_end;
> +    }
> +    *payload_end = 0;
>      while (buf_ptr < buf_end) {
>          v = *buf_ptr++;
>          v2 = *buf_ptr;

> @@ -1301,10 +1306,12 @@
>  
>  int ff_mjpeg_find_marker(MJpegDecodeContext *s,
>                           const uint8_t **buf_ptr, const uint8_t *buf_end,
> -                         const uint8_t **unescaped_buf_ptr, int *unescaped_buf_size)
> +                         const uint8_t **unescaped_buf_ptr, int *unescaped_buf_size,

> +                         const uint8_t **payload_end) /* AARON addition 2011-09-30 */

git log and blame keep track of who added what and when theres no need
to use comments for that
also please generate patches with git format-patch or equivalent



>  {
>      int start_code;
> -    start_code = find_marker(buf_ptr, buf_end);
> +    unsigned int payload_size;
> +    start_code = find_marker(buf_ptr, buf_end, payload_end);
>  
>                  if ((buf_end - *buf_ptr) > s->buffer_size)
>                  {
> @@ -1315,6 +1322,14 @@
>                          s->buffer_size);
>                  }
>  
> +                if (start_code >= 0xd0 && start_code <= 0xd9) {
> +                    payload_size = 0;
> +                } else {
> +                    /* variable size */

> +                    payload_size = ((**buf_ptr)<<8) | *(*buf_ptr + 1);

see AV_RB16()


[...]


-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Avoid a single point of failure, be that a person or equipment.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20111007/8eed459f/attachment.asc>


More information about the ffmpeg-devel mailing list