Bug#578720: vorbis-tools: Sends invalid argument to ALSA when playing some files

[Ron]

On Sat, Dec 13, 2014 at 01:17:58AM +0100, Martin Steghöfer wrote:
> Hi Ron!
> 
> Thank you for your quick and comprehensive answer! :-)
> 
> Unfortunately, I think I should have pasted more information into my
> previous mail.

I did actually go read the whole bug report (since there indeed wasn't
quite enough quoted to get the whole picture of what this was about :)

> Most of your answer tries to argue away a point that I
> actually already ruled out in a message I posted earlier to this bug report:
> It looks like we both agree that doing the upmixing is out of the scope of
> both ogg123 and libao. I didn't suggest to do that. I thought the snippet I
> pasted into my message to you made that clear. But apparently it didn't, I'm
> sorry for not having made that more clear and therefore having wasted your
> time. I have a very compact way of writing things, but I try to be more
> verbose this time.

No, that's fine.  Most of that was just me "thinking out loud" as I traced
through the relevant code.  That we agree on that just means we came to the
same conclusion.  I didn't mean to suggest you thought otherwise, I was
just fleshing out the reasons I thought that was the right call to make.

Sorry if that wasn't clear (or too verbose to immediately make sense :)


> After upmixing is ruled out, the only thing left is to (maybe) improve the
> error message, if that's possible.
> 
> In ogg123 we don't have the information that would be necessary in order to
> tell what's going on. The only thing ogg123 knows is that for some reason
> the ALSA output couldn't be opened. And that's what ogg123 reports to the
> user ("Error: Cannot open device alsa."). I don't see any way to improve
> this.
> 
> So, if anything, we can only improve the *other* message. And that one
> "comes" from ALSA (the error code and its translation into a string) and is
> "emitted" (converted to string, composed and printed) by libao, like I tried
> to explain in my previous message:

I don't think we have enough in libao to do this either.  It would
need to be done in alsa-lib ...


> >>The string "invalid argument" comes from ALSA ("snd_strerror")
> >>and it's printed by libao (in "ao_plugin_open"). So either of
> >>them could improve the wording.
> 
> Since you were doubting this...
> 
> >The second to last line appears to have come from alsa-lib itself,
> >I don't see anything in libao that would emit that.
> 
> ...let me be more specific: The line in which libao emits this is located in
> ao_alsa09.c:391 in version 0.8.8 (which is the one the bug reporter used).
> In the most recent version (1.1.0) a similar output would be emitted by
> ao_alsa.c:521.

Ah, right.  That was the bit was missing, that this was against a much
older version of libao, and the code emitting that had changed quite a
bit since then (including the message that would be output).  I'd only
looked at the current code.

That doesn't substantially change what I was saying though.  In libao
(that we currently have), the code in question is this:

	err = snd_pcm_hw_params_set_channels(internal->pcm_handle,
			params, (unsigned int)device->output_channels);
	if (err < 0){
          adebug("snd_pcm_hw_params_set_channels() failed.\n");
          return err;
        }

And all we really know is that call into alsa-lib failed.  We don't
really know anything more about *why* it failed than the error code
it returns.  If all it returns is "Invalid argument", that's all we
have to go on.

We can't know that was because the hardware didn't support the
requested number of channels (if alsa doesn't explicitly say that)
it could be for any number of reasons.

All we really know is the snd_pcm_hw_params_set_channels call failed,
and that's what we report.


> >It [the error message] might be a little counter-intuitive
> >until you realise some hardware simply can't handle mono output
> 
> Exactly, *that* is the (only) point where I see potential for improvement.
> The function name "snd_pcm_hw_params_set_channels" probably doesn't mean
> much to the user. And "invalid parameters" sounds more like an internal
> software problem (one of those illegal function parameters that you try to
> detect with assertions) than like a lack of hardware features.

Right, but "Set hardware channels failed" doesn't explain it much
differently.  And "invalid parameters" is all alsa told us.

I'm not specifically objecting to improving that, but to do so
it would have to be in alsa-lib, not libao.  And then there'd
be some risk of breaking something else using alsa if it makes
assumptions about the error codes that might be returned.

The other option would be for ogg123 to try to query the output
capabilities before deciding whether it can legally specify them,
but it's not clear that added complexity is worth it either ...


> >but it's not clear to me that anything
> >above the level of alsa-lib can really
> >second-guess (or report) why it
> >failed in any more detailed way.
> 
> Not clear to me, either. That's why I'm coming to you because you know libao
> better. An idea from my side: libao *can* know what's going on by combining
> the following information: The fact that the error happened, when setting
> the output device parameters, and the return code (an integer value that
> means "invalid parameters"). The information in "internal->cmd" could even
> tell *which* parameter couldn't be set (in this case the number of
> channels).

No, we still don't know *why* it failed.  All we know is which alsa-lib
function we called, and the error code it returned.  Which is what we
already report.


> I'm not saying that anything *has* to be done, neither am I saying that
> libao is the right place to do say. I'm just saying that *if* we want to do
> anything to improve this, the only places that occur to me for doing this in
> a reasonable way are libao or ALSA. That's why I came to you, to hear your
> opinion, if you think this should be done in libao, in ALSA or not at all.

I don't think there's much we can do in libao, at least not without
making things a lot more complicated by querying capabilities rather
than just passing on the error if we aren't able to set what was
requested, and arguably that's a job for the app using libao to do
before it requests those capabilities from libao anyway (and even
then in neither case are you guaranteed that the call into alsa-lib
still won't fail for some other reason).

You might be able to do something more in ALSA, but whether that's
a good idea, and whether its maintainers would be keen to do it
is a question you'd have to run past them.


> >So sorry, I don't really see an easy out that just lets you punt this
> >one to someone else :)
> 
> I wasn't trying to. This is not about *who* does things (I'd be glad to
> contribute, be the "issue" in vorbis-tools or in libao; reassigning doesn't
> mean that I'm out), but *where* things are done. Since it was clear to me
> that the issue couldn't be helped in vorbis-tools, instead of just closing
> the issue I tried a final shot: Searching an external place, where the issue
> might be mitigated. And I still see a slight chance for this to happen in
> libao.

That wasn't meant to sound like an accusation :)  More like sympathy that
I don't really see a lot of practical ways to deal with this (aside from
what you'd already noted in the bug report).

We're basically talking about very simple low level tools, if you ask
them to do something they can't do, you get very simple low level errors
in response.  It's not a bad thing to try to improve that, but it becomes
a game of whack a mole, where by the time you've done everything you need
to produce a more detailed explanation of what they mean for a novice user,
you might as well add the extra code to actually fix that too ...

... and then before you know it, you no longer have a simple low level
tool, but a high level all-singing, all-dancing, audio player.

So it's mostly a question of where we draw the line, since players that
do that do already exist too.


> Thank you for your help! :-)

Thank you for trying to bring some closure to this 4 year old bug!

You could run it past the ALSA maintainers and see if they think they
could do something better here.  But if not, I'd probably just close
it now (or close it with a patch sent upstream to note this in the
ogg123 docs, which might be useful regardless of what the ALSA folk do).

It's not really "wontfix", and not even really a problem that will
affect all users.  If the original reporter had audio hardware that
did support mono streams (as some do), or if they'd been using a
different(ly configured) backend, they wouldn't have hit this with
that file either.

There are lots of potential 'solutions' to this, at lots of layers
in the audio stack, and what the right answer for individual users
will be is probably almost as varied.  Which is why all I can really
do is muse about options, there isn't an obvious trivial patch for
either ogg123 or libao that we can just apply to Make It All Better.

  Best,
  Ron