PATCH: FPE in demux_audio.c
--- BUG DESCRIPTION --- There is a division by zero problem (two, actually) in main/libmpdemux/demux_audio.c. When playing MP3 or FLAC, it attempts to divide by sh_audio->i_bps which is unset. This doesn't happen when using a container, so presumably bps is defined by the container. For some reason, this doesn't cause FPE on IEEE 754-conformant processors (e.g. x86, unlike DEC Alpha). --- BUG DESCRIPTION --- --- RESOLUTION --- A patch is attached. The patch resolves the issue quite well by adding a switch-case statement to the affected lines. It is, however, evaluated every time that the audio buffer is filled, so more efficient solutions may be preferable. Also, I suggest that the code be checked for arithmetic traps. (if there's a test suite, I'd be happy to do it) --- RESOLUTION --- -- |Andrew A. Gill |I posted to Silent-Tristero and| |<superluser@frontiernet.net> |all I got was this stupid sig! | |alt.tv.simpsons CBG-FAQ author | | | (Report all obscene mail to Le Maitre Pots)| |Yet: <http://www.needsfoodbadly.com> Temporary sig: -- Yay advusers!
On Thursday 28 July 2005 02:38, Andrew A. Gill wrote:
--- main/libmpdemux/demux_audio.c 2005-04-18 16:51:34.000000000 -0400 +++ main/libmpdemux/demux_audio.c 2005-07-27 20:03:00.000000000 -0400 @@ -415,7 +415,15 @@ l = 65535; dp = new_demux_packet(l); l = stream_read(s,dp->buffer,l); - priv->last_pts = priv->last_pts < 0 ? 0 : priv->last_pts + l/(float)sh_audio->i_bps; + switch (sh_audio->i_bps) + { + case 0: + priv->last_pts = 0; + break; + default: + priv->last_pts = priv->last_pts < 0 ? 0 : priv->last_pts + l/(float)sh_audio->i_bps; + break; + } break; } default:
Why use a switch statement? You can also do: + if (sh_audio->i_bps) priv->last_pts = priv->last_pts < 0 ? 0 : priv->last_pts + l/(float)sh_audio->i_bps; + else priv->last_pts = 0; Same for the other switch. It's a lot shorter and also has no cosmetic changes (which are mostly forbidden, see DOCS/tech/patches.txt). BTW It's better if you post your patches to mplayer-dev-eng. --Ivo
On Thu, 28 Jul 2005, Ivo wrote:
Why use a switch statement? You can also do:
I thought that switches were more efficient. Brief googling informs me that in this case, they are not. I shall change the patch, recompile and send it along. OK. It works and is attached.
Same for the other switch. It's a lot shorter and also has no cosmetic changes (which are mostly forbidden, see DOCS/tech/patches.txt).
Well, you're going to need cosmetic changes to get rid of the goto statements eventually. ;-P
BTW It's better if you post your patches to mplayer-dev-eng.
I would, but I figured that this list would be better to ask about code efficiency and exception handling. Most coders couldn't care less about efficiency or exceptions. -- |Andrew A. Gill |I posted to Silent-Tristero and| |<superluser@frontiernet.net> |all I got was this stupid sig! | |alt.tv.simpsons CBG-FAQ author | | | (Report all obscene mail to Le Maitre Pots)| |Yet: <http://www.needsfoodbadly.com> Temporary sig: -- Why do you have those goto statements?
On Wed, Jul 27, 2005 at 10:27:21PM -0400, Andrew A. Gill wrote:
On Thu, 28 Jul 2005, Ivo wrote:
BTW It's better if you post your patches to mplayer-dev-eng.
I would, but I figured that this list would be better to ask about code efficiency and exception handling. Most coders couldn't care less about efficiency or exceptions.
mplayer-dev-eng is better, post there. Diego
Hi, On Wed, Jul 27, 2005 at 08:38:57PM -0400, Andrew A. Gill wrote:
The patch resolves the issue quite well by adding a switch-case statement to the affected lines. It is, however, evaluated every time that the audio buffer is filled, so more efficient solutions may be preferable.
I am against it, i_bps should not be 0, this might have other negative side effects, e.g. in mencoder. Would be better to find out why iti isn't set... Greetings, Reimar Döffinger
participants (4)
-
Andrew A. Gill -
Diego Biurrun -
Ivo -
Reimar Döffinger