[MPlayer-dev-eng] MPlayer/MEncoder crash on using -vf scale and expand together. Fix attached.

[Ivan Kalvachev]

On 10/28/11, Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
> On Fri, Oct 28, 2011 at 07:53:40PM +0300, Ivan Kalvachev wrote:
>> On 10/28/11, Alex C <alexc.xander at yahoo.in> wrote:
>> > Problem: SIGSEGV on mplayer -vf scale=720:480,expand=720:480 test.mkv
>> >
>> > Sample test file:
>> > https://ffmpeg.org/trac/ffmpeg/attachment/ticket/594/rotate_x.mp4
>> >
>> > Backtrace on: http://pastebin.com/uHPRtswz
>> >
>> > Similar to ffmpeg bug:
>> > https://ffmpeg.org/trac/ffmpeg/ticket/594?cversion=1&cnum_hist=4
>> >
>> > Patched in ffmpeg by michaelni:
>> > http://git.videolan.org/gitweb.cgi?p=ffmpeg.git;a=commitdiff;h=3e4375833d964ebb5d38816126ff0101ae696cf9;hp=d7dcd96a2377835a20c3d23b3bd476ddcf1baebb
>> >
>> > I have applied a similar patch for mplayer. Patch is attached. The crash
>> > is
>> > averted as a result.
>>
>> Your patch is reverted (aka the newer file is at ---).
>> I'm also keeping the MP_IMGFLAG_ACCEPT_STRIDE, it is needed for the xv
>> direct rendering. I'm also applying this to the second vf_get_image()
>> at line 379 in start_slice() function.
>>
>> No need to resend the patch, I'll commit in a minutes.
>> I'll use this commit message.
>> --------------
>> Fix segfault in scale due to unaligned stride.
>>
>> The MP_IMGFLAG_PREFER_ALIDNED_STRIDE is honored only
>> in MP_IMGTYPE_EXPORT, but vf_scale uses it only with MP_IMGTYPE_TEMP.
>> As result stride is not aligned and this causes misalignment for SSE code.
>>
>> Use MP_IMGFLAG_ACCEPT_ALIDNED_STRIDE instead, that aligns it in all cases.
>>
>> Solution and initial patch by alexc.xander.yahoo.in.
>
> The part that is missing is why libswscale requires alignment (also, how
> much? After this change it's still pure chance if our provided stride
> and what libswscale needs match) and why that should be acceptable.
> So far it looks to me like libswscale just has a single broken SSE3
> function that requires more alignment than it may.

I'll be glad if the yuv2yuvX_sse3() is fixed so it doesn't mandate
aligned access. However this makes the sse code slower and it is never
an easy solution to go with. The fact that Michael didn't fix it this
way indicates he is not quite eager to do the trade. Still if you have
a fix, I'll support you in keeping swscale alignment free.

On the other side.

The current use of MP_IMGFLAG_PREFER_ALIDNED_STRIDE is definitely
wrong. It doesn't have any effect and is misleading.

About the enforcing of the aligned stride, take a look of
vf.c:vf_get_image(). The ACCEPT_ causes calculation of aligned "w2"
(aligned at 16 bytes) , that is used by TYPE_TEMP for system memory
allocation. The case that you describe is possible if the filter takes
buffer from the following vf/vo, that however happens only with
TYPE_EXPORT and is exactly where PREFER_ is involved into. Of course
the code first checks if the chain supports stride arithmetic .
(There is one exception, if put_image is not implemented, but in that
case the original width is always used.)

In short, we should either change to FLAG_ACCEPT_ or to TYPE_EXPORT.

Having lines aligned to dword/cacheline/sse is speed optimization and
is good to have even if there is no sse code crashing..


If I've messed something, feel free to correct me :)