[vlc-devel] [PATCH] Let Render() throw an error message and return right away if the SPU region height turns out to be zero.

[Fabian Keil]

Fabian Keil <freebsd-listen at fabiankeil.de> wrote:

> "Rémi Denis-Courmont" <remi at remlab.net> wrote:
> 
> > Le dimanche 5 juillet 2009 11:38:51 Fabian Keil, vous avez écrit :
> > > The attached patch fixes a problem on FreeBSD 8.0 where vlc
> > > triggers an assertion in jemalloc if the SPU region height
> > > is zero.
> > 
> > I fail to see the link between a zero SPU region height and a crash an 
> > assertion failure in libc.
> > 
> > > The backtrace looks like this:
> > > #0  0x0000000801118eac in thr_kill () from /lib/libc.so.7
> > > #1  0x00000008011afa0b in abort () from /lib/libc.so.7
> > > #2  0x0000000801196e45 in __assert () from /lib/libc.so.7
> > > #3  0x000000080113ae81 in malloc_usable_size () from /lib/libc.so.7
> > > #4  0x000000080113e098 in posix_memalign () from /lib/libc.so.7
> > > #5  0x00000008007cb842 in __vout_AllocatePicture (p_this=Variable "p_this"
> > > is not available.) at vout_pictures.h:126
> > 
> > From the stack trace, I guess the allocated size is invalid. *But* I do not 
> > understand why the problem would be _specific_ to zero height SPU region in 
> > the SPU decoder. If vout_AllocatePicture() or whatever has an integer 
> > underflow or some other kind of arithmetic error, I would rather have it fixed 
> > that worked around.
> 
> It's not specific to a zero height SPU region, but would
> happen if any of the factors used to calculate i_bytes in
> src/video_output/vout_pictures.c is zero:
> 
>    570      /* Calculate how big the new image should be */
>    571      i_bytes = p_pic->format.i_bits_per_pixel *
>    572          i_width_aligned * i_height_aligned / 8;
>    573
> 
> I assume any of those factors being zero would be an error,
> but the zero height is the only problem I've seen on actual
> DVDs so far.

I changed the patch to check fmt.i_width as well.

Skipping invalid SPUs seems reasonable in general to me,
even if it also happens to be a workaround for the crashes
described above.

> > In principle, while useless, there is no reason why allocating an empty 
> > picture should not be possible. posix_memalign() does support zero-length 
> > allocation anyway:
> > http://www.opengroup.org/onlinepubs/9699919799/functions/posix_memalign.html
> > | If the size of the space requested is 0, the behavior is
> > | implementation-defined; the value returned in memptr shall be either a
> > | null pointer or a unique pointer.
> 
> I assume posix_memalign() on FreeBSD 8.0 currently
> isn't as compliant as the man page says it is, then.

This will be fixed:
http://docs.freebsd.org/cgi/getmsg.cgi?fetch=28852+0+current/freebsd-hackers

Fabian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20090710/463f634c/attachment.sig>