[MPlayer-dev-eng] [PATCH] ALSA: get rid of potentially endless loop

[Reimar Döffinger]

On Tue, Sep 29, 2009 at 03:12:01PM +0200, Clemens Ladisch wrote:
> Reimar Döffinger wrote:
> > Since MPlayer will try writing audio again sooner or later if the play
> > function returns 0, I don't see much value in that loop, particularly
> > since if for some reason a broken snd_pcm_writei returns 0 all the time
> > it will be an endless loop and MPlayer will be stuck completely.
> 
> snd_pcm_writei will return 0 only in non-blocking mode.  If the
> device does not advance at all, ALSA will eventually return an error.
> 
> As far as I can see, mplayer never calls play with a bigger amount of
> data than previously returned by get_space, so snd_pcm_writei will
> actually never return 0.
> 
> The (only) purpose of the loop is to retry writing immediately if error
> recovery has been necessary.
> 
> > I have to add I do not like the overall code at all, if snd_pcm_resume
> > or snd_pcm_prepare ever return something > 0 total chaos will result.
> 
> Those functions never return > 0.

Well, more or less my point: I think this code is full of obfuscation
(a while (res == 0) that actually is not about the res == 0 case but to
handle the some error (all res < 0) cases? Seriously?) and unnecessary
assumptions about the ALSA API (what is the advantage of assuming that
snd_pcm_resume will never, ever in any ALSA version and with any kind of
buggy hardware drivers return something > 0? Particularly since I don't
know if it might even cause a crash if it does.).
Any why does the code have to rely on "ALSA will eventually return an
error"? Why should MPlayer code assume that ALSA is not buggy when it is
no more effort to code in a robust way (obviously I could be missing
1000s of reasons why my proposed patch is a bad idea, but arguments
along the line "as long as ALSA is working perfectly the current code is
not wrong" don't really convince me.)
Also in case it is unclear what the point is: if this is gone, the next
time such an "high CPU usage" issue comes up, there is one less place
that could be at fault (it could be the case that for some reason
snd_pcm_writei does return 0, or maybe it returns for each try a 1000
times -EINTR before finally succeeding - none of these are possible to
see currently and removing the loop seems like an easy way to get
problems that are easier to interpret, particularly since it would show
up in a general debugging log message for the audio_out->play function -
which I admit currently does not seem to exist).