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

[Ivan Kalvachev]

On 10/28/11, Ivan Kalvachev <ikalvachev at gmail.com> wrote:
> 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 :)

I don't hear any objections, so in few hours I'll commit it with the
following message:
------------------------
Ensure aligned stride for vf_scale output temp buffer.

The MP_IMGFLAG_PREFER_ALIDNED_STRIDE is honored only
with MP_IMGTYPE_EXPORT, but vf_scale uses only MP_IMGTYPE_TEMP.
As result stride won't be rounded up to a multiple of 16.

Use MP_IMGFLAG_ACCEPT_ALIDNED_STRIDE instead, as it works for
all memory allocated image types..

Discovery and initial patch by alexc.xander.yahoo.in.
Workarounds segfault in swscale sse3 code due to unaligned access.
---