[FFmpeg-devel] [PATCH] fix H.264 crash (issue 706)

[Michael Niedermayer]

On Thu, Apr 09, 2009 at 10:33:49PM +0200, Reimar D?ffinger wrote:
> On Thu, Apr 09, 2009 at 10:00:00PM +0200, Michael Niedermayer wrote:
> > On Thu, Apr 09, 2009 at 09:51:56PM +0200, Reimar D?ffinger wrote:
> > > On Thu, Apr 09, 2009 at 09:03:31PM +0200, Michael Niedermayer wrote:
> > > > On Thu, Apr 09, 2009 at 07:46:19PM +0200, Reimar D?ffinger wrote:
> > > > > Index: libavcodec/error_resilience.c
> > > > > ===================================================================
> > > > > --- libavcodec/error_resilience.c	(revision 18389)
> > > > > +++ libavcodec/error_resilience.c	(working copy)
> > > > > @@ -343,6 +343,7 @@
> > > > >  
> > > > >                  if(IS_INTRA(s->current_picture.mb_type[mb_xy]))  continue;
> > > > >                  if(!(s->error_status_table[mb_xy]&MV_ERROR)) continue;
> > > > > +                if(!s->last_picture.data[0]) continue;
> > > > >  
> > > > >                  s->mv_dir = MV_DIR_FORWARD;
> > > > >                  s->mb_intra=0;
> > > > 
> > > > this should use MV_DIR_BACKWARD i suspect instead of skiping it completely
> > > 
> > > Do I need to flip the sign of the mvs or is that correct as-is (well, in
> > > one of those cases those mvs are 0 anyway).
> > 
> > you should use the other MV, MBs have 2 (its likely a [0] that should be [1])
> 
> You mean e.g.
> s->current_picture.motion_val[0][ mb_x*2 + mb_y*2*s->b8_stride ][0];
> should be
> s->current_picture.motion_val[dir][ mb_x*2 + mb_y*2*s->b8_stride ][0];
> With dir = !s->last_picture.data[0]; ?

yes


> 
> > > @@ -850,6 +850,12 @@
> > >      /* set unknown mb-type to most likely */
> > >      for(i=0; i<s->mb_num; i++){
> > >          const int mb_xy= s->mb_index2xy[i];
> > > +        // change inter with no references to intra
> > > +        if (!IS_INTRA(s->current_picture.mb_type[mb_xy]) &&
> > > +            !s->last_picture.data[0] && !s->next_picture.data[0]) {
> > > +            s->current_picture.mb_type[mb_xy]= MB_TYPE_INTRA4x4;
> > > +            continue;
> > > +        }
> > >          error= s->error_status_table[mb_xy];
> > >          if(!((error&DC_ERROR) && (error&MV_ERROR)))
> > >              continue;
> > 
> > is_intra_more_likely() should return 1
> > and that should cause all to be set to intra
> 
> Well, but that code will only run if there is either either a DC or a MV
> error (see above), whereas the problematic code will only run if there
> is no MV error. So if there is only an AC error I think the code I added
> above would be necessary I think...

short awnser
our h264 decoder doesnt set AC_ERROR without MV_ERROR

long one:
if the MV is correct then it should be used,now it could be that the
reference frame is plain unavailable and we thus might pick a different
reference with scaled MV or change to intra but
the code is called "/* set unknown mb-type to most likely */"
MB is not unknown if MV is known
but for all this we first need support for partitioned h264 and we
also need a partitioned h264 file (we have neither ...)

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

I am the wisest man alive, for I know one thing, and that is that I know
nothing. -- Socrates
-------------- 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/20090409/32de58ab/attachment.pgp>