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