alsa-project / alsa-lib

The Advanced Linux Sound Architecture (ALSA) - library
GNU Lesser General Public License v2.1
366 stars 177 forks source link

Severe regression between 1.2.3.2 and 1.2.4 #107

Closed mirabilos closed 3 years ago

mirabilos commented 3 years ago

Original bugreport: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=976895

Upgrading libasound2 from 1.2.3.2 to 1.2.4 introduces a severe regression affecting MuseScore (both 2 and 3):

Starting musescore3 and loading a score, e.g. like this…

    $ musescore3 /usr/share/mscore3-3.2/demos/Reunion.mscz

… quickly brings the whole system down because it spits tons of lines…

    Alsa_driver: recover: pcm_status(): Broken pipe

… to the xterm in which it was invoked.

mirabilos commented 3 years ago

git bisect shows that reverting precisely commit 4f90392f07e8822d1984ed990f622ad36022a4a3 with no other change fixes this bug.

perexg commented 3 years ago

The application should be fixed. Report this issue for the application.

mirabilos commented 3 years ago

Jaroslav Kysela dixit:

The application should be fixed. Report this issue for the application.

WTF, fixed in what way?

Can you at least help with getting that fixed?

bye, //mirabilos -- 15:39⎜«mika:#grml» mira|AO: "mit XFree86® wär’ das nicht passiert" - muhaha 15:48⎜<thkoehler:#grml> also warum machen die xorg Jungs eigentlich alles kaputt? :) 15:49⎜<novoid:#grml> thkoehler: weil sie als Kinder nie den gebauten Turm selber umschmeissen durften? -- ~/.Xmodmap wonders…

mirabilos commented 3 years ago

After lots of grepping around to find the caller, it seems like the “problem” is with this part of the code:

  if ((err = snd_pcm_status (_play_handle, stat)) < 0) {
        qDebug("Alsa_driver: recover: pcm_status(): %s",  snd_strerror (err));
        return false;
  }

What is the code supposed to do in such an error case (I’m assuming EPIPE)? Then I can propose this as fix in MuseScore.

perexg commented 3 years ago

Refer the documentation: https://www.alsa-project.org/alsa-doc/alsa-lib/pcm.html . -EPIPE means that the application is not able to feed the PCM samples in time. The PCM handle should be recovered in this case. But the error code may be just a consequence of a wrong buffering. Again, report this to the application developers.

tiwai commented 3 years ago

I guess this coming from the commit 4f90392f07e8822d1984ed990f622ad36022a4a3. Do we need to return for the error from snd_pcm_plug_update_avail()? Since the error state is stored in the next snd_pcm_status() call as XRUN state, we may not need to return -EPIPE error.

In general, it makes more sense if snd_pcm_status() returns a proper status record as much as possible even if it's in XRUN state, IMO.

perexg commented 3 years ago

Thanks @tiwai . Looking to this change the second time, this change is really unwanted (although it looks good at first glance).

perexg commented 3 years ago

Fixed via afe6ff3b33ee6e5ea3511fe458bfd4e516b10bcf .

mirabilos commented 3 years ago

Thanks! The diff…

$ git diff 4f90392f^..afe6ff3b src/pcm/pcm_plugin.c 
diff --git a/src/pcm/pcm_plugin.c b/src/pcm/pcm_plugin.c
index ea60eb98..76a524fa 100644
--- a/src/pcm/pcm_plugin.c
+++ b/src/pcm/pcm_plugin.c
@@ -551,6 +551,8 @@ static int snd_pcm_plugin_status(snd_pcm_t *pcm, snd_pcm_status_t * status)
                return err;
        status->appl_ptr = *pcm->appl.ptr;
        status->hw_ptr = *pcm->hw.ptr;
+       status->avail = snd_pcm_mmap_avail(pcm);
+       status->delay = snd_pcm_mmap_delay(pcm);
        return 0;
 }

… now looks exactly like the initial submission https://lore.kernel.org/alsa-devel/d9c1f37e-5c8d-f289-270e-c6cda7a56ce3@axis.com/ linked in commit 4f90392 (i.e. what was submitted differed from what was committed).

This will also fix my problem.

Update: Fix tested and uploaded to Debian DELAYED/5 as NMU. I assume your next release (1.2.4.1? 1.2.5?) will also contain it.