[PATCH] Re: [Ffmpeg-devel] FFmpeg: H.264 decoding issue

[Matthias Hopf]

Hey,

On Feb 20, 07 19:57:46 +0100, Michael Niedermayer wrote:
> > Index: libavcodec/utils.c
> > ===================================================================
> > --- libavcodec/utils.c	(revision 8020)
> > +++ libavcodec/utils.c	(working copy)
> > @@ -278,7 +278,7 @@
> >  
> >          if(!(s->flags&CODEC_FLAG_EMU_EDGE)){
> >              w+= EDGE_WIDTH*2;
> > -            h+= EDGE_WIDTH*2;
> > +            h+= EDGE_WIDTH*2+1;		// +1 for potential interlace (MPV_frame_start)
> 
> why?

MPV_frame_start:

    if(s->picture_structure!=PICT_FRAME){
        int i;
        for(i=0; i<4; i++){
            if(s->picture_structure == PICT_BOTTOM_FIELD){
                 s->current_picture.data[i] += s->current_picture.linesize[i];
            }
            s->current_picture.linesize[i] *= 2;
            s->last_picture.linesize[i] *=2;
            s->next_picture.linesize[i] *=2;
        }
    }

If the frames are still used in a progressive fashion afterwards (e.g.
because PAFF isn't implemented), a buffer overflow occurs.
But the crash I've seen so far happens in MPV_frame_end, which is AFAICS
decoder independent (well, MPEG, but no further diversification). So
IMHO this could even happen with MPEG2.

I think, changing the base data pointer and linesize *without* changing
the height of the picture and related parameters just calls for trouble.
Though I guess this assumption is reflected in the code in a lot of
places and I don't really want to change anything here.


Given that linesize is duplicated as well, my patch is certainly a
half-hearted approach. It probably only fixed the problem for me because
MPV_frame_end used s->linesize and not s->current_picture.linesize:

    draw_edges(s->current_picture.data[0], s->linesize,
               s->h_edge_pos, s->v_edge_pos, EDGE_WIDTH);

So why is this using s->current_picture.data, but s->linesize etc.?
There's also a comment in MPV_frame_start (//FIXME use only the vars
from current_pic).
Should this be split for interlaced frames (so we have interlaced lines
on the edges as well)? Are completely filled edges necessary, or only
the lines reflecting the current field?

How do I actually decide whether I have a frame or a field?
s->current_picture_ptr->interlaced_frame?
Or s->picture_structure!=PICT_FRAME?
s->current_picture.linesize is duplicated in the later case.
But s->picture_structure is not a per-picture field...


So we would either have something like

    draw_edges(s->current_picture.data[0], s->current_picture.linesize,
               s->h_edge_pos, s->current_picture.v_edge_pos, EDGE_WIDTH);

with a per-picture v_edge_pos (this will only fill the edges for lines
that reflect the current field). Alternatively, without introducing a
new field one could reuse the known parameters as in

    draw_edges(s->current_picture.data[0], s->current_picture.linesize,
               s->h_edge_pos, v_edge_pos * s->linesize / s->current_picture.linesize,
               EDGE_WIDTH);

but IMHO that looks ugly / broken.

The other solution would be something like

	if (s->picture_structure==PICT_FRAME) {
	  draw_edges(s->current_picture.data[0], s->linesize, s->h_edge_pos, s->v_edge_pos, EDGE_WIDTH);
	  [...]
	} else {
	  int o = (s->picutre_structure==PICT_BOTTOM_FIELD ? s->current_picture.linesize[i]/2 : 0);
	  draw_edges(s->current_picture.data[0], s->linesize, s->h_edge_pos, s->v_edge_pos, EDGE_WIDTH);
	  [...]
	}

Yes, this is dead ugly and potentially harmful, with its broken mixture
of current_picuture and MpegEncContext accesses.


> [...]
> > Index: libavcodec/mpegvideo.c
> > ===================================================================
> > --- libavcodec/mpegvideo.c	(revision 8020)
> > +++ libavcodec/mpegvideo.c	(working copy)
[snip]
> this change is wrong, the bug is outside MPV_decode_mb(), its not
> MPV_decode_mb() job to check the bitstreams validity

Ok, will trace this more.

CU

Matthias

-- 
Matthias Hopf <mhopf at suse.de>      __        __   __
Maxfeldstr. 5 / 90409 Nuernberg   (_   | |  (_   |__          mat at mshopf.de
Phone +49-911-74053-715           __)  |_|  __)  |__  R & D   www.mshopf.de