[alsa-devel] [PATCH 00/23] alsa-utils: rewrite aplay

[Takashi Iwai]

On Thu, 17 Aug 2017 13:59:41 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> I think all of you in this mailing list might have used the command 'aplay',
> at least once. This patchset is my attempt to rewrite it because current
> shape of code is not enough good. Especially, ALSA PCM interface have got
> extensions since aplay was firstly developed. One of my motivation is to
> refine the tool to test such extensions.
> 
> Below diagram illustrates new internal design of this program:
> 
>                                              <-> container <-> (file)
> (device) <-> (alsa-lib) <-> xfer <-> aligner <-> container <-> (file)
>                                              <-> container <-> (file)
> 
> For description of each modules, please read patch comment.
> 
> This patchset is early preview for developers to see the above design, thus
> some features of original aplay are not still implemented. Especially, some
> options have no actual effect even if they're parsed. Presently, I see below
> command lines run as I expected:
> 
> $ ./aplay -C (-v) (-M)(-N) -D [hw:0,0|default] record.wav 
> $ ./aplay -P (-v) (-M)(-N) -D [hw:0,0|default] playback.wav
> $ ./aplay --list-devices
> $ ./aplay --list-pcm
> $ ./aplay --version
> 
> Here, I work with Intel HDA (hw:0,0) and pulse PCM plugin (default).
> 
> It's my pleasure if received some comments from you.

Thanks for the effort, the resultant codes look neat.
But, my concern is that the incomplete implementation and the lack of
regression test thereof.

The most common tactics for the code refactoring is "divide and
conquer": i.e. split the lengthy codes at first, and rewrite each of
them.  The merit by this way is that you can keep the functionality
and check the regression test step-by-step.

Meanwhile the approach you're taking is rather a complete rewrite from
the scratch, and it's error-prone in that regard.  OTOH, a scratch
build may result in a cleaner, better structured code, eventually.

So, I find it's OK to go in this way, but we can put this as a
different command at first.  Once when the code reaches to the 100%
full compatibility with the existing aplay, we can obsolete the old
code and provide a symlink to the new command for the compatibility.

In that way, we can merge the codes gradually without too much worry.

About the code: I still had little time to read through the whole
codes, but though a very quick glance:

- Please put the good explanation as in the cover letter into the
  changelog.  A documentation for the big picture is missing.

- Try to avoid inventing a new term; e.g. aligner is a unique word,
  and difficult to understand (whether it's for teeth or what else).


thanks,

Takashi


> 
> Takashi Sakamoto (23):
>   aplay: add an abstraction of container to parse/build audio-specific
>     data format
>   aplay: add an implementation of container for Microsoft/IBM RIFF/Wave
>     format
>   aplay: add an implementation of container for Sparc AU format
>   aplay: add an implementation of container for Creative Tech. voice
>     format
>   aplay: add an implementation of container for raw format
>   aplay: aligner: add an abstraction to align buffers with different
>     data
>   aplay: add an implementation of aligner for single target
>   aplay: add an implementation of aligner for multiple target
>   aplay: add an abstruction of waiter for I/O event notification
>   aplay: add an implementation of waiter for poll(2)
>   aplay: add an implementation of waiter for epoll(7)
>   aplay: options: add a parser for command-line options
>   aplay: add an abstraction for transferring of PCM frames
>   aplay: add an implementation for transferring by ALSA PCM APIs
>   aplay: add implementation of I/O
>   aplay: add implementations to scheduling
>   aplay: add a sub-command to print list of PCMs/devices
>   aplay: add a sub-command to transfer data frames
>   aplay: obsolete main routine and introduce sub-command style
>   aplay: add an implementation for volume unit meter
>   aplay: add a parser for channel map API
>   aplay: add a handler for key events
>   aplay: add a feature to generate PID file
> 
>  aplay/Makefile.am             |   48 +-
>  aplay/aligner-multiple.c      |  337 ++++
>  aplay/aligner-single.c        |  256 +++
>  aplay/aligner.c               |  122 ++
>  aplay/aligner.h               |   88 ++
>  aplay/aplay.c                 | 3425 -----------------------------------------
>  aplay/container-au.c          |  203 +++
>  aplay/container-riff-wave.c   |  549 +++++++
>  aplay/container-voc.c         |  736 +++++++++
>  aplay/container.c             |  375 +++++
>  aplay/container.h             |  127 ++
>  aplay/formats.h               |  134 --
>  aplay/key-event.c             |  120 ++
>  aplay/key-event.h             |   21 +
>  aplay/main.c                  |  178 +++
>  aplay/main.h                  |   32 +
>  aplay/options.c               |  674 ++++++++
>  aplay/options.h               |   71 +
>  aplay/subcmd-list.c           |  233 +++
>  aplay/subcmd-transfer.c       |  431 ++++++
>  aplay/vumeter.c               |  299 ++++
>  aplay/vumeter.h               |   50 +
>  aplay/waiter-epoll.c          |   76 +
>  aplay/waiter-poll.c           |   66 +
>  aplay/waiter.c                |   61 +
>  aplay/waiter.h                |   59 +
>  aplay/xfer-alsa-io-mmap.c     |  135 ++
>  aplay/xfer-alsa-io-rw.c       |  370 +++++
>  aplay/xfer-alsa-sched-irq.c   |   18 +
>  aplay/xfer-alsa-sched-timer.c |   49 +
>  aplay/xfer-alsa.c             |  610 ++++++++
>  aplay/xfer-alsa.h             |   67 +
>  aplay/xfer.c                  |  124 ++
>  aplay/xfer.h                  |   74 +
>  configure.ac                  |    2 +-
>  35 files changed, 6655 insertions(+), 3565 deletions(-)
>  create mode 100644 aplay/aligner-multiple.c
>  create mode 100644 aplay/aligner-single.c
>  create mode 100644 aplay/aligner.c
>  create mode 100644 aplay/aligner.h
>  delete mode 100644 aplay/aplay.c
>  create mode 100644 aplay/container-au.c
>  create mode 100644 aplay/container-riff-wave.c
>  create mode 100644 aplay/container-voc.c
>  create mode 100644 aplay/container.c
>  create mode 100644 aplay/container.h
>  delete mode 100644 aplay/formats.h
>  create mode 100644 aplay/key-event.c
>  create mode 100644 aplay/key-event.h
>  create mode 100644 aplay/main.c
>  create mode 100644 aplay/main.h
>  create mode 100644 aplay/options.c
>  create mode 100644 aplay/options.h
>  create mode 100644 aplay/subcmd-list.c
>  create mode 100644 aplay/subcmd-transfer.c
>  create mode 100644 aplay/vumeter.c
>  create mode 100644 aplay/vumeter.h
>  create mode 100644 aplay/waiter-epoll.c
>  create mode 100644 aplay/waiter-poll.c
>  create mode 100644 aplay/waiter.c
>  create mode 100644 aplay/waiter.h
>  create mode 100644 aplay/xfer-alsa-io-mmap.c
>  create mode 100644 aplay/xfer-alsa-io-rw.c
>  create mode 100644 aplay/xfer-alsa-sched-irq.c
>  create mode 100644 aplay/xfer-alsa-sched-timer.c
>  create mode 100644 aplay/xfer-alsa.c
>  create mode 100644 aplay/xfer-alsa.h
>  create mode 100644 aplay/xfer.c
>  create mode 100644 aplay/xfer.h
> 
> -- 
> 2.11.0
>