libsndfile source and sink #246
Comments
gavv
added
enhancement
help wanted
|
This task is not very small, but it is quite straightforward and detaily explained, so I think it should be good enough for newcomers. |
|
Hello. I'm a student in Computer Science, in my fourth year of study and I'd like to help resolve this issue. This semester I need to choose a project to contribute to, and the help of a maintainer of the project, to ensure that my push requests will not just be ignored, or reviewed after the deadlines of the university. It shouldn't be much work to the maintainer, just adding constructive feedback on requests and such. Could you be this helper for the project? Thanks in advance |
|
@Billionai sure, you're welcome! |
|
See also #251. It provides an overview of roc_sndio concepts. |
|
Thank you! I'm at a little bit of a loss as to how your sox classes work, so that'll help a lot! |
gavv
added
category: android
|
I'm having some trouble finding the dependencies for libsndfile. In this page I found (http://www.linuxfromscratch.org/blfs/view/svn/multimedia/libsndfile.html) it only lists flac, ogg and vorbis, and says alsa and SQlite (?) are optional dependencies. Are those the ones used in SContruct? |
That's right. Actually all these dependencies are optional. If libsndfile was built with libFLAC, libogg, and libvorbis, it will use them to open corresponding file formats. Otherwise these formats will not be supported by libsndfile. ALSA and sqlite are needed only for libsndfile tools and tests. The library itself does not depend on them. Roc gives you two modes for every dependency:
The first mode is the default and it is generally preferred and recommended. The second mode is a kind of fallback that can be useful if you can't or don't want to build the dependencies by yourself and want to get something working and portable as fast as possible. It is particularly useful when cross-compiling. It's quite time-consuming to setup a sysroot and cross-compile dependencies, and by employing So, answering to your question. When the user will specify |
|
Our https://github.com/roc-project/roc/blob/master/scripts/3rdparty.py#L472-L474 |
|
Hey, I was coding the sink for sndfile and noticed that I need to pass a number as a format, that would be derived from the driver passed (I'm guessing). What formats should be supported, and what their strings will look like? ("wav" or "wavefile"?) This is the only thing left to do for the SndfileSink, and I can start working on the source. Sorry about the delay, the mixture of inexperience and difficult documentation along with university made the progress be much slower than I expected. |
Yep.
It would be great if we will support all formats supported by libsndfile.
For formats that are also supported by SoX, we should use the same string name as SoX uses, so that the For formats that are not supported by SoX (if any) we can use the (common) extension name as the format name. You can see the list of currently supported formats by running
Cool!
That's alright :) |
|
I have a couple of questions before I can move to the backend: First, about the 'is_file_' member: Second, most of the drivers from sox aren't format the way sndfile recognizes them, just wav and core audio. Should I just ignore the rest and use sndfile's extensions, or am I missing something? |
I don't quite understand what devices are you talking about. In SoX backend, input or output may be either a path on filesystem (e.g. .wav file) or a device name specific to some sound system, e.g. ALSA device or PulseAudio sink. If input or output is a device, it's not a path and it usually does not correspond to any file in Generally, this has nothing to do with UNIX device files that may be found in AFAIK, libsndfile does not support any audio devices, only files. Does it? If it doesn't,
Do you mean driver names? As for the names, I guess we should map the file format name to It seems there most formats are supported both by libsndfile and SoX, e.g.: Actually it's hard to find formats that are supported by libsndfile but not supported by SoX. But it seems there are some, e.g.: Here is what SoX supports on my system: Maybe I misunderstood the question :) |
As I'm used to linux, I figured that you'd handle a device in a similar way to a file with libsndfile, except it wouldn't be seekable. Otherwise I can't really see a reason for a music file to not be seekable, but maybe it's just inexperience. If libsndfile does, indeed, only support files I'll just set
Yes, that's what I meant. I had some trouble because That mapping feels like it should be part of the backend file, as both the source and sink should use it. Is that so? Also, the simple mapping i'm thinking of (without using STL) would be 2 constant arrays, one of strings which contain the common extension or the same name as in SoX, and the other one with the driver code. Not necessarily the fastest method, but if we order the arrays by usage, it might be faster (on average) than using a binary search. Thoughts? Thanks for the fast responses! |
I guess the only case when it's not seekable is a pipe, e.g.:
Yep.
Yeah, it was added recently. BTW don't forget to rebase on fresh You can also use:
Agree. Backend can map string to constant and pass it to sink/source constructor.
I think this is perfectly OK. This is not a critical path and there is no need to optimize it. Also, on small array sizes there will be very little difference between linear and binary search. |
|
@Billionai Hi, do you still have plans to finish this? |
|
@gavv While working on sndfile_source I came across an issue of data types and casting. Inside the audio namespace many of the functions defined that are necessary for libsndfile's implementation return data types of size_t (long unsigned int). libsndfile's C API consistently wants to work with sf_count_t (signed int 64 bit) data types. Right now I am casting between the two types in order to get things to compile but I worry about casting between a signed and an unsigned integer in the case of overflow or negative values being lost. Do you have a suggested way to deal with this safely or is this actually not an issue at all? |
|
It depends on context. E.g. if you have size_t with frame size, you can be sure that it can't be huge (gigabytes), so you can just blindly cast to sf_count_t. On the other hand, if you have sf_count_t value returned from library, and the library can return negative values (e.g. to indicate error), you need to check for negative values first, and only cast to size_t if it's positive. |
|
@gavv I've been working on sndfile_backend but something I don't understand is the purpose of the discover_drivers function in IBackend. Looking at the sox_backend implementation of it hasn't made it any clearer for me. Could I get an explanation? Thanks. |
|
Each backend may support multiple drivers. One driver corresponds to one audio system or audio file format. For example, drivers supported by sox backend are: pulse, alsa, wav, mp3, ... Source or sink is identified by driver name + device or file name, e.g. driver is "alsa" and device/file is "hw:0", or driver is "wav" and device/file is "test.wav". Devices are described by io uri like "alsa://hw:0". When backend dispatcher sees this uri, it selects backend that supports "alsa" driver and uses it. Files are described by io uri in form "file://path". Which driver to use, i.e. what is the file format (wav, mp3, ...) is usually auto-detected from file extension. However, the user can also manually specify file format (driver) via separate cli option. For sndfile backend, all drivers are file formats (no devices). So its discover_drivers method should return a list of supported file formats. See comment above: #246 (comment) |
Your response has immensely clarified things for me. The documentation states most of what you said, but the way you put it into your own words was so much easier to follow. Thank you for the fast response. I believe I understand now. |
|
Great, I've added documentation based on my comments above: https://github.com/roc-streaming/roc-toolkit/blob/develop/docs/sphinx/internals/audio_backends.rst (it will appear on web site after next release) |
|
@gavv I have implemented the probe() function in sndfile_backend but I have some questions related to it. 1. Where should I be declaring ProbeFlag, where should I be defining it, and what determines if it is set? I'm thinking it needs to be set if the driver_type is a file and it should be declared and defined somewhere inside backend_dispatcher before probe() is called. 2. When you say probe() should "fail" if the ProbeFile flag is not set, do you mean it should return false or roc_panic and terminate the program? Right now I have probe() calling roc_log(debug) and returning false if ProbeFlag is not set. 3. Where should probe() be called? I am thinking it must be called somewhere with a guarded if statement that determines if sndfile or sox will be used assuming both are present. If it returns true, then sndfile will be used. It is unclear to me where this would be, but my guess is somewhere in backend_dispatcher.cpp before an open_device() call. Your guidance is appreciated, thank you. |
|
@Hrick87 Oh, actually probe functionality was removed, and I forgot to remove it from task description, sorry! Earlier, to decide which backend to use, backed dispatcher called probe() method on each backend, until one of them succeeds. However later we realized that for most backends probe() will be implemented using open + check + close, so using probe() + open() leads to open + close + open again. So probe logic was removed, and now backend dispatcher just tries to open file/device using each backend, until it succeeds. |
|
So all you need is to implement open_device() method. If driver_type is DriverType_Device, it should just return NULL. If driver_type is DriverType_File, it should try to open file using libsndfile, and construct sink/source in case of success, or return NULL in case of failure. |
No worries! I thought this might be the case because as I was looking through commit history and saw that probe() used to be included in some of the backends, but wasn't anymore. I should've checked in before attempting to implement it. |
|
@gavv When running roc-recv using my sndfile implementation, watchdog reports all blank frames except for a single dropped frame (b's and a single i). My roc-send sndfile implementation works when roc-recv is using pulse audio as its backend. Thus I'm assuming two things, my roc-send implementation is correct, and my roc-recv implementation is done incorrectly. I decided the best place to start debugging would be in my sndfile sink's write() function. I was using SoX's write() function as a reference and that's when I noticed I can't find a single instance of SoX's write() function being called anywhere. This leaves me completely confused how writing from the buffer to the newly created file or stream is occurring when using SoX. Am I simply missing write()'s call somewhere in the code base? |
|
write() is called from sndio::Pump, see also https://github.com/roc-streaming/roc-toolkit/blob/develop/docs%2Fsphinx%2Finternals%2Faudio_backends.rst Watchdog monitors audio from network, before it goes to your source, so it's strange. Note that some blank frames at the beginning of the session is normal. |
|
@gavv I have a full working implementation of sndfile and am nearly ready to make my PR, but the build fails on github actions for every single linux distro. I believe this is due to the fact that the Scons -Q command's build-3rdparty argument lacks "sndfile" on every docker image. I am quite new to github actions so maybe I am misunderstanding the issue and there is something I am doing wrong. Please let me know if it's on my end or something that needs to be fixed on your end. Thank you! EDIT: I see that the CI build tests are stored in the repository and that I could possibly go in and change each of them for you. I am happy to do this, or if because its beyond the scope of this issue/you want this solved a different way I won't change them. |
|
@Hrick87 Indeed, I forgot about that. Yes, if you could add sndfile to docker images, it would be great. We use github actions, but actual checks are run inside custom docker images, which are build from dockerfiles in this repo: https://github.com/roc-streaming/dockerfiles. You can even run those checks locally. We'd need to add sndfile to all Here are some docs about CI: https://roc-streaming.org/toolkit/docs/development/continuous_integration.html |
- unify formatting - a few renames - add a few comments - initialize all vars - remove some overly verbose checks inherited from sox source/sink - remove unused members - use non-default rate (48000) in tests (just to be sure the rate passed from test is really applied)
- Use SampleSpec::use_defaults() for simpler code. - Simplify how SampleSpec is populated. - In SndfileSink, don't call seek, because some formats are not seekable and will fail. - In SndfileSource, check if file is seekable. If yes, implement restart() using seek, otherwise implement it by reopening file. - In SndfileSource, use StringBuffer for path, because provided const char* may point to a temporary string. - If close fails, print error instead of panic.
- Pass driver type to SoxSource/SoxSink ctor. - Check or populate SampleSpec in ctor depending on driver type (file or device). - In SoxSink, populate with defaults all fields (format, channels, rate) for files, and all except rate for devices. (If we don't popolate rate for device, sox will pick a rate natively supported by device, which is good. But if we don't populate channels, sox will pick mono, which is a bad default. So if channels are not specified, we select stereo). - In SoxSource, forbid non-empty SampleSpec for files. For devices, populate all fields except rate. - Remove panic.
- unify formatting - a few renames - add a few comments - initialize all vars - remove some overly verbose checks inherited from sox source/sink - remove unused members - use non-default rate (48000) in tests (just to be sure the rate passed from test is really applied)
- Use SampleSpec::use_defaults() for simpler code. - Simplify how SampleSpec is populated. - In SndfileSink, don't call seek, because some formats are not seekable and will fail. - In SndfileSource, check if file is seekable. If yes, implement restart() using seek, otherwise implement it by reopening file. - In SndfileSource, use StringBuffer for path, because provided const char* may point to a temporary string. - If close fails, print error instead of panic.
- Pass driver type to SoxSource/SoxSink ctor. - Check or populate SampleSpec in ctor depending on driver type (file or device). - In SoxSink, populate with defaults all fields (format, channels, rate) for files, and all except rate for devices. (If we don't popolate rate for device, sox will pick a rate natively supported by device, which is good. But if we don't populate channels, sox will pick mono, which is a bad default. So if channels are not specified, we select stereo). - In SoxSource, forbid non-empty SampleSpec for files. For devices, populate all fields except rate. - Remove panic.
|
Landed. |
Last revised: Oct 2023
Update: Issue is still relevant. Updated to reflect nowadays codebase.
Intro
libsndfile is a minimalistic cross-platform C library allowing to read and write audio files in several audio formats.
Currently roc-send can read files using SoxSource and roc-recv can write files using SoxSink. These source and sink implementation use SoX, which can work both with audio files and devices.
However, SoX has some disadvantages:
In other issues like #231 and #232 we will implement sources and sinks for ALSA, PulseAudio, CoreAudio, and Android.
It would be also nice to be able to read and write files using libsndfile instead of SoX:
It will allow us to have file support in roc-recv and roc-send on Android.
It will also reduce number of dependencies because we anyway depend on libsndfile indirectly. SoX will become an optional dependency needed only for some exotic audio backends like sndio.
Implementation
So in this issue, we should:
implement
sndio::SndfileSource, similar to SoxSource, but working only with files using libsndfileimplement
sndio::SndfileSink, similar to SoxSink, but working only with files using libsndfileimplement
sndio::SndfileBackend, similar to SoxBackend andsndio::PulseaudioBackendregister
sndio::SndfileBackendin BackendDispatcherImplementation notes
SndfileSource, SndfileSink, and SndfileBackend should be placed under target_sndfile directory (similar to target_sox and target_pulseaudio)
SndfileBackend::probe() should check whether the requested driver/input or driver/output combination can be opened using sndfile; in case of regular files, driver will be the file type (e.g. "wav") and input/output will be the file name (e.g. "test.wav"). probe() should return true only if the file format (driver) is supported by sndfile
SndfileBackend::probe() should auto-detect driver by file extension if it is empty, so that probe(NULL, "test.wav", ProbeFile) will be equal to probe("wav", "test.wav", ProbeFile)
SndfileBackend::probe() should also respect the flags given to it: it should fail unless ProbeFile is set
we should ensure that if both SndfileBackend and SoxBackend are present, SndfileBackend is preferred for file formats supported by libsndfile (e.g. WAV); libsndfile will be more efficient in this case
Build system
We should also add sndfile support to scons scripts, basically just duplicate what we're doing for sox and adjust it for sndfile, specifically:
in SConstruct, add --disable-sndfile (like --disable-sox)
in SConstruct, automatically append target_sndfile to ROC_TARGETS if --disable-sndfile is not provided (like we do it for target_sox)
in 3rdparty/SConscript, check whether sndfile is available on system (like we do it in if 'target_sox' in
system_dependenciesbranch for sox)in 3rdparty/SConscript, download and build libsndfile if it's present in --build-3rdparty option (like we do it in
if 'target_sox' in download_dependencies:branch for sox); our build-3rdparty.py script already supports libsndfile so we should just callenv.BuildThirdPartyin 3rdparty/SConscript and it will do all the workUnit tests
We already have unit tests for SoxSource and SoxSink. It would be nice to reuse them for SndfileSource and SndfileSink.
This would be easy: we already have ISource and ISink interfaces, so we should just make every test to run on two backends (sox and sndfile).
We already have an example of such tests -- here every test has a for-loop iterating over all supported FEC schemes.
The text was updated successfully, but these errors were encountered: