Issue 1419593007: Add an ALSA output to the motown audio server.

Issue 1419593007: Add an ALSA output to the motown audio server. (Closed)

Created:
5 years, 1 month ago by johngro

Modified:
5 years, 1 month ago

Reviewers:
dalesat, jamesr, jeffbrown

CC:
mojo-reviews_chromium.org, qsr+mojo_chromium.org

Base URL:
https://github.com/domokit/mojo.git@change5

Target Ref:
refs/heads/master

Project:
mojo

Visibility:
Public.

More Reviews

Description

Add an ALSA output to the motown audio server. Add an implementation for linux platforms of an ALSA audio output for the motown audio server. R=jeffbrown@google.com, jamesr@chromium.org BUG= Committed: https://chromium.googlesource.com/external/mojo/+/cffee4a8b7fa62691efe0ee8354a3c81f48eca51

Patch Set 5 : fixing android build #

Created: 5 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+497 lines, -0 lines)			Patch
M	services/media/audio/BUILD.gn	View	1 2 3 4	1 chunk	+7 lines, -0 lines	0 comments	Download
M	services/media/audio/audio_output_manager.h	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	services/media/audio/audio_output_manager.cc	View	1 2 3	2 chunks	+7 lines, -0 lines	0 comments	Download
A	services/media/audio/platform/linux/alsa_output.h	View	1 2	1 chunk	+65 lines, -0 lines	0 comments	Download
A	services/media/audio/platform/linux/alsa_output.cc	View	1 2 3	1 chunk	+399 lines, -0 lines	0 comments	Download
A	services/media/audio/platform/stubs/alsa_output_stub.cc	View	1 2 3 4	1 chunk	+17 lines, -0 lines	0 comments	Download

Messages

Total messages: 9 (2 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

jeffbrown

https://codereview.chromium.org/1419593007/diff/20001/services/media/audio/audio_output_manager.cc File services/media/audio/audio_output_manager.cc (right): https://codereview.chromium.org/1419593007/diff/20001/services/media/audio/audio_output_manager.cc#newcode34 services/media/audio/audio_output_manager.cc:34: #if defined(OS_LINUX) This to me looks like an opportunity ...

5 years, 1 month ago (2015-11-04 20:33:43 UTC) #4

johngro

5 years, 1 month ago (2015-11-06 20:22:07 UTC) #5

https://codereview.chromium.org/1419593007/diff/20001/services/media/audio/au...
File services/media/audio/audio_output_manager.cc (right):

https://codereview.chromium.org/1419593007/diff/20001/services/media/audio/au...
services/media/audio/audio_output_manager.cc:34: #if defined(OS_LINUX)
On 2015/11/04 20:33:42, jeffbrown wrote:
> This to me looks like an opportunity to define a new class to abstract
platform
> audio sink concerns.

Acknowledged.

We should talk sometime.  I'd like to understand more about your approach to
doing this.

https://codereview.chromium.org/1419593007/diff/20001/services/media/audio/au...
services/media/audio/audio_output_manager.cc:46:
static_cast<audio::AlsaOutput*>(audio_out.get());
On 2015/11/04 20:33:42, jeffbrown wrote:
> Did you "git cl format"?  I think this is supposed to be 4 spaces or reflow
into
> something equally hideous.

Done.

Solved this a different way.  A long time ago, I used to have the output manager
in just mojo::media, and the audio:: scope was needed, but it no longer is. 
Removing that makes stuff fit on one line.

Oddly enough, I cannot find clear guidance in the public style guide about what
to do if an assignment (or any other operation) exceeds the 80 col length.  I'm
not sure what git CL format chooses to do, but AFAICT there are not a lot of
strict rules here besides use your best judgement and be consistent.

https://codereview.chromium.org/1419593007/diff/20001/services/media/audio/au...
services/media/audio/audio_output_manager.cc:50: DCHECK(config);
On 2015/11/04 20:33:42, jeffbrown wrote:
> Might be able to omit this DCHECK.  Seems like the sort of thing the Mojo
> bindings should do internally whenever New() fails.  (I really hate checking
the
> result of new...)

The problem here is that this is not a call to operator new().  It's a call to a
function someone wrote called New, which (in theory) could have extra side
effects.  IOW - its great that the mojo/chrome global new has been overloaded to
DCHECK if allocation fails, but this function could succeed to allocate and
still return NULL.

In this case, however, I know that this is machine generated code which should
Never Do This, so I'll remove the check.

https://codereview.chromium.org/1419593007/diff/20001/services/media/audio/pl...
File services/media/audio/platform/linux/alsa_output.cc (right):

https://codereview.chromium.org/1419593007/diff/20001/services/media/audio/pl...
services/media/audio/platform/linux/alsa_output.cc:15: static constexpr
LocalDuration TGT_LATENCY = local_time::from_msec(35);
On 2015/11/04 20:33:42, jeffbrown wrote:
> TGT is pretty opaque as acronyms go, prefer TARGET_LATENCY

Done.

https://codereview.chromium.org/1419593007/diff/20001/services/media/audio/pl...
services/media/audio/platform/linux/alsa_output.cc:25: -EINTR, -EPIPE,
-ESTRPIPE,
On 2015/11/04 20:33:42, jeffbrown wrote:
> Kind of odd to put these in a set<> given that it'll take longer to evaluate
> than a switch statement and that the response to each of these errors may
> differ.  Particularly EINTR.
> 
> (oh man, can we make out C library just deal with EINTR internally?  I'm so
sick
> of it with Bionic.  For the one or two times I really care... ugh.)

It just makes the code a bit more readable and maintainable than a switch IMO.
I'll replace with a switch instead, it just turns 3 lines of code into 10.

https://codereview.chromium.org/1419593007/diff/20001/services/media/audio/pl...
services/media/audio/platform/linux/alsa_output.cc:33: // anyway, just in case
something got missed.
On 2015/11/04 20:33:42, jeffbrown wrote:
> Why not just CHECK in release mode?  Yeah we'll crash but it's better than
> having dead code.

A call to cleanup might (someday) do more than just close the device.  I don't
consider it to be dead code, I consider it to be structure provided so that
future maintainers have a clear place to put idempotent cleanup/shutdown tasks.

re: checking in release mode.
If a failed check here posed a potential security risk, or implied some form
lingering corruption which was going to blow up in a strange way hours from now,
I think a CHECK would be warranted.  As it stands, I don't think that either is
the case.  This is strange, and it should not happen, but we can probably
soldier on and be OK.  The DCHECK should catch most mistakes during internal
testing.

https://codereview.chromium.org/1419593007/diff/20001/services/media/audio/pl...
services/media/audio/platform/linux/alsa_output.cc:102: // TODO(johngro): log
something here?
On 2015/11/04 20:33:42, jeffbrown wrote:
> Yeah.  LOG(ERROR) << "You're hosed."  ;)

Done.

https://codereview.chromium.org/1419593007/diff/20001/services/media/audio/pl...
services/media/audio/platform/linux/alsa_output.cc:103: DCHECK(!alsa_device_);
On 2015/11/04 20:33:42, jeffbrown wrote:
> This DCHECK seems unnecessary given your test at the top of this function.

Assuming that ALSA never has any bugs, but yes...

https://codereview.chromium.org/1419593007/diff/20001/services/media/audio/pl...
services/media/audio/platform/linux/alsa_output.cc:112: 1,   // allow ALSA
resample
On 2015/11/04 20:33:42, jeffbrown wrote:
> Hmm.  Guess we'll disable this in the future.

how about right now?  Not sure how this got set TBH.  Probably cut-n-paste from
my earlier proto-server.

https://codereview.chromium.org/1419593007/diff/20001/services/media/audio/pl...
services/media/audio/platform/linux/alsa_output.cc:115: // TODO(johngro): log
something here?
On 2015/11/04 20:33:42, jeffbrown wrote:
> Please do.  ALSA is such a pain to debug when bringing up devices.

Done.

https://codereview.chromium.org/1419593007/diff/20001/services/media/audio/pl...
services/media/audio/platform/linux/alsa_output.cc:177: int64_t now_ticks =
now.time_since_epoch().count();
On 2015/11/04 20:33:42, jeffbrown wrote:
> Looking at this conversion to int64_t, I wonder whether it would make sense to
> specialize LinearTransform in relation to std::chrono::duration types.  Kind
of
> weird maybe and it would make the math more annoying.  Anyhow, something to
keep
> in mind for later if you like.

yeah, an adapter might be a good idea.  If we end up really running with the
idea of having identifiers for the timelines which define the domains on either
side of the transformation, we could have the specialization DCHECK that the
domain matches the chrono clock source.

https://codereview.chromium.org/1419593007/diff/20001/services/media/audio/pl...
services/media/audio/platform/linux/alsa_output.cc:181: while
(++local_to_output_gen_ == MixJob::INVALID_GENERATION) {}
On 2015/11/04 20:33:42, jeffbrown wrote:
> A little weird but ok.

Acknowledged.

I could wrap this in a method if you think it would help clarity.

https://codereview.chromium.org/1419593007/diff/20001/services/media/audio/pl...
services/media/audio/platform/linux/alsa_output.cc:194: bool trans_ok =
local_to_output_.DoReverseTransform(frames_sent_,
On 2015/11/04 20:33:42, jeffbrown wrote:
> Am I correct in thinking that the reverse transform isn't always well-defined?

> (scale factor might be zero)
> 
> Do we need to check for singularity?

No, outputs cannot be paused.  They are either running or not, and when they are
not, there is no transformation (it needs to be primed).

Related:  This is why we always boost into our track's domain and perform
sampling there.  Even when pause, transformations into the tracks domain are
non-singular.  In the paused case, they are also non-1-to-1; all points in the
real world map to the paused point in the track.  Still, the transformation is
meaningful for doing things like trimming pending queues.

re: checking for singularity.
This is handled by the transform class.  It will never divide by zero.  If
boosting from one domain to the other fails (either because of overflow or
because of singularity) it will return false.  This should never happen for us,
which is why we DCHECK that result.

https://codereview.chromium.org/1419593007/diff/20001/services/media/audio/pl...
services/media/audio/platform/linux/alsa_output.cc:196: DCHECK(trans_ok);
On 2015/11/04 20:33:43, jeffbrown wrote:
> Ahh, here's our check for singularity.
> 
> Odd thought, but what if DoReverseTransform checked this already for you? 
> Essentially say: "don't ever call this function is the transform is singular".

> Seems like a reasonable precondition.

This also checks for overflow, not just singularity.

https://codereview.chromium.org/1419593007/diff/20001/services/media/audio/pl...
services/media/audio/platform/linux/alsa_output.cc:204: // is the case, just try
again a short amt of time in the future.
On 2015/11/04 20:33:42, jeffbrown wrote:
> amt -> amount

Done.

https://codereview.chromium.org/1419593007/diff/20001/services/media/audio/pl...
services/media/audio/platform/linux/alsa_output.cc:227: DCHECK_LE(fill_amt,
std::numeric_limits<int32_t>::max());
On 2015/11/04 20:33:42, jeffbrown wrote:
> Are we guaranteed that numerator <= denominator?  Otherwise we might still
> overflow.

We should be.  If N > D, it implies that our audio frame rate is greater than
our local clock frequency.  If this is the case, our local clock probably does
not have sufficient resolution to be used for media (that, or we are running our
audio output at something like 1GHz :O)

The real solution here is to expose a scale method from LinearTransform::Ratio
which (just like LT itself) returns an error code in case the transformation
either overflows or is singular.  I mention that I really need to do this in the
few other places in the server where I do a by-hand scale operation.  It's on my
list of things to do.

Still, I would just do it and DCHECK.  We should never overflow here.

https://codereview.chromium.org/1419593007/diff/20001/services/media/audio/pl...
services/media/audio/platform/linux/alsa_output.cc:228: fill_amt *=
frames_per_tick_.numerator;
On 2015/11/04 20:33:43, jeffbrown wrote:
> I might recommend that we create a for rounded scaling.

good idea.  See above; its the type of thing which belongs as part of the
utility methods for LT::Ratio

https://codereview.chromium.org/1419593007/diff/20001/services/media/audio/pl...
services/media/audio/platform/linux/alsa_output.cc:247: silence_byte_,
On 2015/11/04 20:33:43, jeffbrown wrote:
> Technically silence might not always be a byte.  An unsigned 16-bit format,
for
> instance.  Or floats.  If you believe in them.  :)

Acknowledged.

This only works because we (currently) hardcode our format to signed 16 bit.  As
things develop, this will need to be updated.

https://codereview.chromium.org/1419593007/diff/20001/services/media/audio/pl...
services/media/audio/platform/linux/alsa_output.cc:295: ::memset(mix_buf_.get(),
silence_byte_, mix_buf_bytes_);
On 2015/11/04 20:33:42, jeffbrown wrote:
> This has shown up twice now.  Consider making a function to fill a buffer with
N
> frames of silence.  (Dealing with bytes and frames in the same function is a
> little confusing anyhow.)

Sure; this (moving to frames only) means performing a multiply which we don't
strictly need to do.  It makes me a bit sad inside, but only because of the
unhealthy aspects of my OCD.  In reality, its not a big deal.

https://codereview.chromium.org/1419593007/diff/20001/services/media/audio/pl...
services/media/audio/platform/linux/alsa_output.cc:308: void
AlsaOutput::HandleAsError(snd_pcm_sframes_t code) {
On 2015/11/04 20:33:42, jeffbrown wrote:
> The name of this function is a little too similar to HandleAlsaError for
casual
> inspection.

Permission to say HandleALSAError instead of HandleAlsaError?  I think this
would make things much more clear.  Failing that, I am open to suggestions.

https://codereview.chromium.org/1419593007/diff/20001/services/media/audio/pl...
services/media/audio/platform/linux/alsa_output.cc:313: if
(RECOVERABLE_ALSA_ERRORS.find(code) != RECOVERABLE_ALSA_ERRORS.end()) {
On 2015/11/04 20:33:42, jeffbrown wrote:
> If we got EINTR then the correct response was to retry immediately.  Probably
> want to use TEMP_FAILURE_RETRY for that one instead of delaying.

I'm not sure I understand your request here.  The logic here is supposed to
be...

+ We have received an error from ALSA which is not underflow.
+ If this error is not on the short list which ALSA says it can attempt to
recover from, then LOG and shutdown.
+ If we can recover from it, then attempt to do so immediately.
+ If recovery succeeds, then wait our ERROR_RECOVERY_TIME (so speakers can
settle and we don't end up in a rapid, and unpleasant sounding,
start/fail/recover loop), then start the system back up again.
+ If recovery fails, treat this as an unrecoverable error (Apparently it is) and
shut ourselves down.

https://codereview.chromium.org/1419593007/diff/20001/services/media/audio/pl...
File services/media/audio/platform/linux/alsa_output.h (right):

https://codereview.chromium.org/1419593007/diff/20001/services/media/audio/pl...
services/media/audio/platform/linux/alsa_output.h:25: MediaResult
Configure(LpcmMediaTypeDetailsPtr config);
On 2015/11/04 20:33:43, jeffbrown wrote:
> I wonder whether these Mojo objects should be flowing down to this level of
> abstraction.  Does it make sense to keep them at the interface boundary alone
> and unpack the relevant bits into other structures you use internally?  (I was
> debating this earlier with the View Manager too and ended up doing essentially
> what you did here and not caring about the boundary.  Still not sure what's
> best.)

I think that this is proper.  LpcmMediaTypeDetails is currently a structure
which is defined in mojom and meant to be part of the formal interface
definition for the media system.  It seems natural to use the same
structures/constants which you defined for your external interfaces for your
internal interfaces.  If you cannot, then it seems to imply that something is
either wrong with your interface, or the bindings generated by your IDL.

This said, these structures are a PITA to use (because our C++ structure
bindings are generally a PITA to use).  I *really* didn't want to use them at
all because they annoy me so much.  In the end, I did, because I decided that
while these are a pain; duplicating the structure contents and maintaining both
sides as the structure evolves was actually a sin.

Note: I have committed this sin when describing format capabilities in
audio_track_impl.cc.  My confession is in a comment at the top and my penance is
to replace all of that with something better (around the topic of format
negotiation) someday.

jeffbrown

LGTM with minor comment https://codereview.chromium.org/1419593007/diff/40001/services/media/audio/platform/linux/alsa_output.cc File services/media/audio/platform/linux/alsa_output.cc (right): https://codereview.chromium.org/1419593007/diff/40001/services/media/audio/platform/linux/alsa_output.cc#newcode334 services/media/audio/platform/linux/alsa_output.cc:334: code = snd_pcm_recover(alsa_device_, code, true); ...

5 years, 1 month ago (2015-11-10 20:13:25 UTC) #6

johngro

https://codereview.chromium.org/1419593007/diff/40001/services/media/audio/platform/linux/alsa_output.cc File services/media/audio/platform/linux/alsa_output.cc (right): https://codereview.chromium.org/1419593007/diff/40001/services/media/audio/platform/linux/alsa_output.cc#newcode334 services/media/audio/platform/linux/alsa_output.cc:334: code = snd_pcm_recover(alsa_device_, code, true); On 2015/11/10 20:13:25, jeffbrown ...

5 years, 1 month ago (2015-11-10 20:32:02 UTC) #7

https://codereview.chromium.org/1419593007/diff/40001/services/media/audio/pl...
File services/media/audio/platform/linux/alsa_output.cc (right):

https://codereview.chromium.org/1419593007/diff/40001/services/media/audio/pl...
services/media/audio/platform/linux/alsa_output.cc:334: code =
snd_pcm_recover(alsa_device_, code, true);
On 2015/11/10 20:13:25, jeffbrown wrote:
> This could fail with EINTR too I think and the correct thing to do would be to
> loop immediately.

Docs are fuzzy on this issue.  They basically say that if the device cannot be
recovered, that it will return the value that you passed for 'code' to the
recover call.  They do not seem to give any indication of why the system could
not be recovered (were we EINTR'ed again, or is there something else going on?)

The procedure for recovering from an error which cannot be snd_pcm_recovered
from is also unclear.  I suppose that the nuclear option is to close the device
and attempt to re-open it; I'd need to refactor some code to take that approach.
 For now, I'm just shutting down.

I would assume that the #1 reason we would see EINTR is because either someone
CTRL-C'ed us, or because someone SIGKILL'ed us, and its probably time to shut
down anyway.  For that reason, and because generally I don't want to end up with
any unbound operations on a thread like this (high priority, currently a member
of a pool) I would prefer to yield to the outer-engine.  This gives the system a
chance to re-evaluate the should-I-shutdown-now question, and also will throttle
the system so that it does not melt down.

I assume that at this point, all bets are off and that we have damaged
presentation already, so working extra hard to recover super fast (for a sound
architecture we are only casually supporting to begin with) is probably not
worth it.

I'll retry in this case, but with the same delay I use for recovered errors. 
I'll also leave a comment about needing to exercise this behavior if we plan on
officially supporting ALSA as a backend.

johngro

Notes on PS4, In addition to the EINTR tweak, I also refactored the hack I ...

5 years, 1 month ago (2015-11-11 01:32:41 UTC) #8

johngro

5 years, 1 month ago (2015-11-12 01:18:51 UTC) #9

Message was sent while issue was closed.

Committed patchset #5 (id:80001) manually as
cffee4a8b7fa62691efe0ee8354a3c81f48eca51 (presubmit successful).

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages