[MPlayer-dev-eng] Segfault caused by the "expand" filter

[Reimar Döffinger]

On Sun, Mar 20, 2011 at 09:49:29PM +0800, Zuxy Meng wrote:
> 2011/3/20 Reimar Döffinger <Reimar.Doeffinger at gmx.de>:
> > On 20 Mar 2011, at 13:27, Zuxy Meng <zuxy.meng at gmail.com> wrote:
> >> 2011/3/19 Reimar Döffinger <Reimar.Doeffinger at gmx.de>:
> >>> On Sat, Mar 19, 2011 at 10:30:14PM +0800, Zuxy Meng wrote:
> >>>> 2011/3/17 Reimar Döffinger <Reimar.Doeffinger at gmx.de>:
> >>>>> On Wed, Mar 16, 2011 at 06:19:04PM +0800, Zuxy Meng wrote:
> >>>>>> 2011/3/16 Reimar Döffinger <Reimar.Doeffinger at gmx.de>:
> >>>>>>> On 15 Mar 2011, at 03:57, Zuxy Meng <zuxy.meng at gmail.com> wrote:
> >>>>>>>> Hope here's still atmosphere for technical discussions:
> >>>>>>>>
> >>>>>>>> I met segfaults triggered by unaligned accesses of SSE instructions.
> >>>>>>>> The root cause is within the get_image() function of the "expand"
> >>>>>>>> filter, in lines like:
> >>>>>>>>
> >>>>>>>>    if(mpi->flags&MP_IMGFLAG_PLANAR){
> >>>>>>>>        mpi->planes[0]=vf->dmpi->planes[0]+
> >>>>>>>>        vf->priv->exp_y*vf->dmpi->stride[0]+vf->priv->exp_x;
> >>>>>>>>
> >>>>>>>> Here vf->dmpi->planes[0] is 16-byte aligned, but mpi->planes[0] may not be.
> >>>>>>>>
> >>>>>>>> I'm not sure what's the correct fix here. Would a simple forced align
> >>>>>>>> before the assignment work (e.g. ((vf->dmpi->planes[0] + ...)&~15))?
> >>>>>>>
> >>>>>>> To solve it properly you have to allocate a new image and memcpy instead of using EXPORT image type (or is this the DR path? Disable DR in that case).
> >>>>>>
> >>>>>> Yes I guess it's the DR path.
> >>>>>>
> >>>>>>> Alternatively filters requiring the alignment could do the memcpy, but I think that's more effort for little gain.
> >>>>>>
> >>>>>> The problem happens when expand isn't the last filter, e.g. vf=expand...,pp=ac
> >>>>>
> >>>>> That doesn't make sense, the line you quoted only causes the filters (and decoder)
> >>>>> _before_ to get unaligned pointers.
> >>>>
> >>>> I'm puzzled too. But in my case vf=pp=ac,expand=::::1:8/5 works and
> >>>> vf=expand=::::1:8/5,pp=ac crashes.
> >>>
> >>> That doesn't say anything at all about where and why the crash happens.
> >>> Probably the pp filter allocates a new frame to pass to the decoder whereas
> >>> it itself doesn't need the pointers to be aligned
> >>
> >> This is the log and traceback, from which you can see the problematic
> >> address 0x181c705 was allocated by the expand filter.
> >
> > I don't see that. I see pp allocating an image and expand modifying it, and then the decoder crashing because it isn't aligned.
> > Which is not an error in expand, since vd_ffmpeg does _not_ ask for something aligned.
> > So could you please try the change for vd_ffmpeg I suggested?
> 
> Indeed! The attached patch works.

Seems good to me (except for ugly indentation), even though it might give worse performance in some cases.
It would be good to look for cases where performance issues can be avoided,
e.g. making expand also work for aligned strides at least in some cases,
but that is also ok to leave for later, crashes are more important.