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

[Ron]

On Sat, Dec 13, 2014 at 09:12:38PM +0100, Martin Steghöfer wrote:
> Ron write:
> >The problem is, that even if this is the case with the current alsa
> >code, we have no guarantee it would always remain true.  And it may
> >in turn pass back error codes from the kernel, which could include
> >this one too.
> 
> Technically it *can* do a lot of things, but it isn't allowed to. Passing on
> an "EINVAL" error code from somewhere internal is something I would consider
> a bug. If a callee returns an "invalid arguments" code to the caller, that
> information has to refer to the actual call at hand, instead of some call
> somewhere under the hood.

No, what I mean is we pass more than one parameter in these function
calls.  It would be perfectly fine for alsa-lib to sanity check all
of them and return EINVAL if any of them were invalid, for whatever
reasons.

It would likewise be fine for it to pass them further up the chain
to the kernel (which it should do in the final snd_pcm_hw_params()
call, if not also before), and for *it* to return EINVAL if some
part of that is invalid (and for alsa-lib to then return that to
us again).

I'm not saying any of these things are currently the case, but they
seem like perfectly reasonable things that any future implementation
might begin to do (or past implementation used to do) without having
specific API guarantees to the contrary.


> If a function "set number of channels" reports an invalid argument, to me
> the only correct interpretation of this is that the number of channels that
> I've asked for isn't allowed.

Then what would it return if one of the other arguments passed to that
function were invalid?


> >(which is why I didn't bother looking at the alsa-lib code to see
> >if there was a way we could do something 'clever' here.  Its next
> >release could look nothing like the code in the current one in its
> >internals, even if it's otherwise API/ABI compatible)
> 
> You are right that whatever is under the hood should be irrelevant to us. I
> shouldn't have mentioned that I looked at the implementation. That was more
> of a side note than an argument. I'm just discussing the meaning of the API
> (even though I know what is under the hood).

It's fine to look.  That's why having the source is important :)
That's just different from what we can safely assume will remain
true into the future.

I just didn't look, because I don't think we can safely assume
anything except "it may fail for any number of reasons, and if
it does we'll get an error code back".  I think trying to layer
extra meaning on that purely by making assumptions of our own
would be a somewhat fragile thing to do.


> >And there's still a long list of things other than number of channels
> >which this could fail on in much the same way (sample rate, sample
> >format, sample size, etc.)
> 
> They could fail with the same error code, but not in the same function call.
> If the callee tells us that the argument was invalid, that should (and does
> in this case) refer to the actual argument at hand.

Right, but now we need to make magic assumptions about the return codes
of *many* functions.  Otherwise the first person to try to play a file
with a 12345Hz sample rate, that their hardware doesn't support, will
be right back where we started with this report.


> >I think anything which wants to verify those things in a user friendly
> >manner really needs to check them for validity specifically itself,
> >rather than just throwing them at libao to see if they'll stick, and
> >hoping it will explain why not in a way suitable for them.
> 
> That's a different path to solve the same issue. I didn't know that the
> libao API allowed us to do so. In any case, this would probably be an
> excessive effort for this issue.

I don't think libao actually does have an interface for that :)
But alsa does.  I think it was one of the complaints of pulse that it
makes determining this sort of thing nearly (or totally) impossible
too - and since libao is a generic front end, it gets a bit tricky
too (though you can at least query which backend libao is using).

It is the right way to do this if you want to be able to give detailed
user friendly reporting of what is valid or not though, or if you want
to ensure this really can play any file regardless of what the hardware
or its drivers support.

But yes, it seems like overkill for ogg123.  If you want a more full
featured player there are plenty of those to choose from.


> But in general, independently from the quality of ALSA/libao's error
> message, implementing this would be a good thing. Can you point me in the
> right direction? At first sight I haven't found a libao function to check
> that.
> 
> Or were you thinking about checking device capabilities using ALSA directly?
> That would somehow foil the point of using libao as intermediary in the
> first place.

libao itself is also just meant to be a very simple abstraction.
I think Monty has been hoping for years that someone would write
something to replace it that doesn't suck so we could kill it off.

We're, uh ...  still waiting.  Unfortunately.

He might take patches to add this, but it could be tricky to do for
all of the backends in a generic way.


> >In the current libao code, I don't think this will even output a
> >message at all unless you've turned the verbosity up (which is
> >different to the code that this was originally seen on).
> 
> I'm going to check that suspicion. If it is correct, that would render our
> hole discussion pointless. :-(

 #define adebug(format, args...) {\
    if(device->verbose==2){                                             \
      if(strcmp(format,"\n")){                                          \
        if(device->funcs->driver_info()->short_name){                   \
          fprintf(stderr,"ao_%s debug: " format,device->funcs->driver_info()->short_name,## args); \
        }else{                                                          \
          fprintf(stderr,"debug: " format,## args);                     \
        }                                                               \
      }else{                                                            \
        fprintf(stderr,"\n");                                           \
      }                                                                 \
    }                                                                   \
  }


> >>And then there's the risk of breaking other software by changing the error code, like you said. :-(
> >Right, including breaking libao if we were to do the trick you proposed
> >above :)
> 
> No, our check for the EINVAL return code would just never become "true" and
> therefore we would be back to using the strings handed to us by
> "snd_strerror". Our "trick" (as you call it) wouldn't be outside the
> specification. But the ALSA return code change could break applications that
> consider EINVAL the *only* valid error code in the case (which they
> shouldn't).

Which would then leave us with an awful mess of having to do different
things based on different versions of alsa etc.  Making it even more
difficult for someone trying to work backward through the magic to
find out what really failed.

This "probably won't happen", but it's the kind of risk we expose
ourselves to if we try to be "clever" here.  And it's still only
speculation that rewording the error message a bit will actually
really help anyone.


At the end of the day, it's still not going to play files that aren't
supported by the hardware when using the direct ALSA backend.  Which
is what most people will probably consider "the bug" to be if they
encounter it.

The original reporter of this bug already concluded that the mono
file was most probably their problem.  So the existing message was
sufficient for that it would seem.

What they apparently didn't know is that this was "expected" in the
configuration they were using, and not just some oversight.  I don't
think we can fix that by changing the error message as such.

"Error: successfully failed to play 1 channel file on your hardware"
isn't really much of a consolation if you were expecting that to work :)


> >Well, the ones who don't read documentation (or source :) are probably
> >still going to think/report that ogg123 is broken even if it tells them
> >"Sorry I can't play 1 channel audio on your system!", so I'm not sure
> >we can really ever win that game.
> 
> Unfortunately that's true... ;-(
> 
> We should wrap this up before we get lost in a too general discussion: While
> I still think that "mode not supported by ALSA device" is the only valid
> conclusion that can be drawn from receiving an "invalid parameter" error on
> that concrete function call,

Except even that is misleading if installing the remixing plugin, or using
some other front end to alsa as the backend to libao would still let this
work (regardless of the case where it failed due to the other parameters
we passed to the function).

Which is why talking through this is basically pushing me toward "note this
stuff in the documentation" as about the only viable solution.

Linux audio is still a mess in many ways.  Of the dozens of ways any given
user can set it up to work for themselves, each has their own set of pros
and cons still, and will vary from user to user depending on what hardware
they have.


> So I will check, if I report this to ALSA (probably a pointless exercise)
> and if some documentation improvements can still be made in ogg123 (not
> sure) and then close this "bug" (which it probably never really was).

I think we've pretty much already arrived at what the right answer for
ALSA will be (don't change anything or you'll break stuff!), so yeah,
I think being a bit more explicit in the ogg123 docs is probably the
best bet at this stage.

That nobody has complained about this a lot since 2010, might mean they
aren't using the raw ALSA backend so much anymore anyway, and this either
isn't actually a real problem in practice very much anymore - or that
people using raw ALSA still actually do understand it and aren't confused.


> Thank you for having made the effort of discussing this minor issue
> in such detail!

"It doesn't work" isn't really a minor problem for the person it doesn't
work for if they can't figure out why.  So I am interested in improving
that in any reasonable way we can.

Going back to the original report again though (rather than the proposed
solutions to it), it does look more like the problem was not in knowing
what didn't work - but rather in knowing what was *expected* to work in
that configuration, and what other options there were to make it work.

A different error message can really only tackle the "what" (non-)problem
part.  If we need to explain "why", that seems like a job for documentation.


  Cheers,
  Ron