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?
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
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
participants (4)
-
Andrew A. Gill -
Diego Biurrun -
Ivo -
Reimar Döffinger