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

[Zuxy Meng]

2011/3/20 Reimar Döffinger <Reimar.Doeffinger at gmx.de>:
> 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.

Applied.

-- 
Zuxy
Beauty is truth,
While truth is beauty.
PGP KeyID: E8555ED6