Libsndfile Sink and Source

[Hrick87]

#246

[github-actions]

🤖 Upon creation, pull request description does not have a link to an issue. If there is a related issue, please add it to the description using any of the supported formats.

[Hrick87]

@gavv

I was hoping you might have an idea as to why the macos13-x86_64/build-3rdparty.sh, macos13-x86_64/standard-build.sh and macos/universal-binaries.sh scripts are failing in my latest commit 471615c? The log and error code returned has not been helpful. Commit 779319d passed macos13-x86_64/standard-build.sh, but 471615c has identical code.

I apologize for all the commits of me testing minor changes to the scripts, but I was unable to figure out how to run the macOS scripts locally like I did with the other environments. Also, GitHub Actions runs the build scripts MUCH faster than my Virtual Box instance can. I can squash them once this is resolved if that is preferred.

Once passing, my code will be ready for review.

Thank you!

[gavv]

Sorry for delay. I triggered build on develop & master, and the same issue is reproduced there, so it's no specific to your PR.

https://github.com/roc-streaming/roc-toolkit/actions/runs/7242028057/job/19726898808

I'll take a look.

[gavv]

I apologize for all the commits of me testing minor changes to the scripts, but I was unable to figure out how to run the macOS scripts locally like I did with the other environments. Also, GitHub Actions runs the build scripts MUCH faster than my Virtual Box instance can. I can squash them once this is resolved if that is preferred.

No worries, github allows to squash commits into one when merging PR, I can just use that button.

[gavv]

This issue with macos checks is gone.

I think the reason was this: actions/setup-python#577

It's not reproducing now, but I've pushed a fix that I hope will prevent it from appearing again: c55443c

[Hrick87]

This issue with macos checks is gone.

I think the reason was this: actions/setup-python#577

It's not reproducing now, but I've pushed a fix that I hope will prevent it from appearing again: c55443c

The python issue was the only error message I was seeing on my end too, so the override argument should resolve it, but this is a bit beyond me. I have a lot to learn about containerization and GitHub Actions still. Anyways, as of now all the builds are passing, so I'm going to open up my PR for review. Thank you for your help.

[gavv]

Thanks for PR, that's big!

Here is the first part of PR, related to build system.

I'll review the remaining part in upcoming days.

[gavv]

I believe it's "libsndfile" or "sndfile", not "SndFile".

[gavv]

Since --disable-tools is present, I think sndfile is not needed.

[gavv]

Thanks again for the patch! Here is the remaining review.

[gavv]

Could you please rebase on fresh develop?

[gavv]

Hi, I've pushed commit 03d29eb to develop branch that adds new fields sample_format and pcm_format to SampleSpec.

It mostly does not affect this PR, but SampleSpec ctor now has new parameter pcm_format, which you can just set to audio::Sample_RawFormat (which means that source/sink produces/consumes samples in "raw" format - 32-bit PCM floats).

So please rebase on fresh develop.

[github-actions]

🤖 The latest upstream change made this pull request unmergeable. Please resolve the merge conflicts.

[Hrick87]

@gavv I was hoping to get some advice on one of the tests. I am trying to remove the use of core::Arrayaudio::sample_t buffer_ and size_t buffer_size_, and am working on refactoring the read() function for sndfile_source.

I am failing this test at the line CHECK(!sndfile_source.read(frame));

TEST(sndfile_source, eof_restart) {
    core::TempFile file("test.wav");

    {
        test::MockSource mock_source;
        mock_source.add(FrameSize * NumChans * 2);

        SndfileSink sndfile_sink(arena, sink_config);
        CHECK(sndfile_sink.open(NULL, file.path()));

        Pump pump(buffer_factory, mock_source, NULL, sndfile_sink, FrameDuration,
                  SampleSpecs, Pump::ModeOneshot);
        CHECK(pump.is_valid());
        CHECK(pump.run());
    }

    SndfileSource sndfile_source(arena, source_config);

    CHECK(sndfile_source.open(NULL, file.path()));

    audio::sample_t frame_data[FrameSize * NumChans] = {};
    audio::Frame frame(frame_data, FrameSize * NumChans);

    for (int i = 0; i < 3; i++) {
        CHECK(sndfile_source.read(frame));
        CHECK(sndfile_source.read(frame));
        CHECK(!sndfile_source.read(frame)); //This line fails

        CHECK(sndfile_source.restart());
    }
}

My understanding of the test is that the second read() call is expected to reach eof and set eof_ to true, so that when the third read() call happens in the for loop, the call will return false. I think the issue is that the second read() call is not reaching the eof_ = true line and so the third call fails. I cannot find the issue in my code that is causing this. I've been stuck on this problem for quite some time.

I currently have:

bool SndfileSource::read(audio::Frame& frame) {
    roc_panic_if(!valid_);

    if (eof_) {
        return false;
    }

    if (!file_) {
        roc_panic("sndfile source: read: non-open input file");
    }
    audio::sample_t * frame_data = frame.samples();
    sf_count_t frame_left = (sf_count_t)frame.num_samples();
    sf_count_t buffer_size = frame_left;

    while (frame_left > 0) {
        sf_count_t n_samples = sf_read_float(file_, frame_data, buffer_size);

        printf("read %ld bytes\n", n_samples);

        if (n_samples == 0) {
            printf("eof, n_samples = 0\n");
            roc_log(LogDebug, "sndfile source: got eof from sndfile");
            eof_ = true;
            break;
        }
        
        frame_left -= n_samples;
    }

    if (frame_left == (sf_count_t)frame.num_samples()) {
        return false;
    }

    if (frame_left > 0) {
        memset(frame_data, 0, (size_t)frame_left * sizeof(audio::sample_t));
    }

    return true;
}

EDIT: I am able to pass the test by adding return false after the eof_ = true line but this does not seem like a good solution. From my understanding, this is not the intended behavior, as SoX is able to pass its respective test without that addition.

[gavv]

sf_count_t n_samples = sf_read_float(file_, frame_data, buffer_size);

First, seems that you request the whole buffer_size from sndfile, but I think you need to request only frame_left?

Second, I think you don't need a loop at all?

Maybe you can do something like:

samples_per_ch = frame.num_samples() / num_channels;

n_read = sf_read_float(file_, frame.samples(), samples_per_ch);

if (n_read == 0) {
    eof = true;
}

if (n_read < samples_per_ch) {
    memset(frame.samples() + n_read * num_channels, 0,
        (samples_per_ch - n_read) * num_channels * sizeof(sample_t));
}

return !eof;

BTW note that sf_read_float() works with number of samples per channel (a.k.a. number of frames, not to be confused with audio::Frame in roc) and frame.num_samples() is number of samples for all channels.

Also I think we need to check for error here too using sf_error_number(). If there is error, we can also add a panic with TODO comment.

[gavv]

If you'll have troubles you could push your code somewhere (to this PR or other branch) and I'll take a look.

[Hrick87]

sf_count_t n_samples = sf_read_float(file_, frame_data, buffer_size);

First, seems that you request the whole buffer_size from sndfile, but I think you need to request only frame_left?

Second, I think you don't need a loop at all?

Maybe you can do something like:

samples_per_ch = frame.num_samples() / num_channels;

n_read = sf_read_float(file_, frame.samples(), samples_per_ch);

if (n_read == 0) {
    eof = true;
}

if (n_read < samples_per_ch) {
    memset(frame.samples() + n_read * num_channels, 0,
        (samples_per_ch - n_read) * num_channels * sizeof(sample_t));
}

return !eof;

BTW note that sf_read_float() works with number of samples per channel (a.k.a. number of frames, not to be confused with audio::Frame in roc) and frame.num_samples() is number of samples for all channels.

Also I think we need to check for error here too using sf_error_number(). If there is error, we can also add a panic with TODO comment.

Your code greatly simplified things for me, enough to see the problem! The check should not be if(n_samples == 0) but instead if(n_samples < frame_left) because eof can be signified by 0 or less than read's count argument. I was not accounting for that possibility. Its been fixed. Thank you!

[gavv]

The check should not be if(n_samples == 0) but instead if(n_samples < frame_left) because eof can be signified by 0 or less than read's count argument

Oh, really. Great!

[Hrick87]

@gavv I've been having some trouble refactoring the test cases to be done in a for loop. I'm only attempting to change one test as of now in test_pump.cpp. The print statements and new name method I implemented in IBackend are just for debugging purposes. This refactoring:

TEST(pump, write_read) {
{ 

    enum { NumSamples = BufSize * 10 };
    
    for(size_t n_backend = 0; n_backend < BackendMap::instance().num_backends(); n_backend++){
        test::MockSource mock_source;
        mock_source.add(NumSamples);
        core::TempFile file("test.wav");
        

        IBackend &backend = BackendMap::instance().nth_backend(n_backend);
        printf("Currently on: %s\n", backend.name());
        fflush(stdout);
        
        {
            IDevice *backend_device = backend.open_device(DeviceType_Sink, DriverType_File, "wav", file.path(), config, arena);
            if(backend_device == NULL){
                printf("Failing sink test: %s\n", backend.name());
                fflush(stdout);
            }
            else{
                printf("Passing sink test: %s\n", backend.name());
                fflush(stdout);
                ISink *backend_sink = backend_device->to_sink();
                Pump pump(buffer_factory, mock_source, NULL, *backend_sink, BufDuration, SampleSpecs,
                        Pump::ModeOneshot);
                CHECK(pump.is_valid());
                CHECK(pump.run());

                CHECK(mock_source.num_returned() >= NumSamples - BufSize);
            }
        }
        printf("File path: %s\n", file.path());
        fflush(stdout);
        
        IDevice *backend_device = backend.open_device(DeviceType_Source, DriverType_File, "wav", file.path(), config, arena);
        if(backend_device == NULL){
            printf("Failing source test: %s\n", backend.name());
            fflush(stdout);
                continue;
        }

        printf("Passing source test: %s\n\n", backend.name());
        
        ISource * backend_source = backend_device->to_source();

        test::MockSink mock_writer;

        Pump pump(buffer_factory,
                *backend_source,
                NULL,
                mock_writer,
                BufDuration,
                SampleSpecs,
                Pump::ModePermanent);
        CHECK(pump.is_valid());
        CHECK(pump.run());

        mock_writer.check(0, mock_source.num_returned());
    }
}
} // namespace roc

results in this output:

Currently on: wav
Passing sink test: wav
File path: /tmp/rocr4GtK2/test.wav
Failing source test: wav
Currently on: PulseAudio
Failing sink test: PulseAudio
File path: /tmp/rocIyNcKx/test.wav
Failing source test: PulseAudio
Currently on: sndfile
Passing sink test: sndfile
File path: /tmp/rocXWcJjr/test.wav
Passing source test: sndfile

Currently on: SoX
Passing sink test: SoX
File path: /tmp/rocNy0rfi/test.wav
Passing source test: SoX


src/tests/roc_sndio/test_pump.cpp:58: error: Failure in TEST(pump, write_read)
src/tests/roc_sndio/test_helpers/mock_sink.h:84: error:
        expected <0>
        but was  <0.921875> threshold used was <0.0001>

If I disable SoX in my build, the test passes. I also tried reverting the test cases back to their individual formats as I have them in my initial pull request and both sndfile and SoX pass. This leads me to believe something about how my for loop works is causing SoX to fail. Any ideas would be much appreciated!

[gavv]

The test will pass if you do this change:

// from:
ISink *backend_sink = backend_device->to_sink();
// to:
core::ScopedPtr<ISink> backend_sink(backend_device->to_sink(), arena);

and:

// from:
ISource * backend_source = backend_device->to_source();
// to:
core::ScopedPtr<ISource> backend_source(backend_device->to_source(), arena);

Sox sink uses buffering, and last part of buffer may not be flushed until destructor is called. By using ScopedPtr, we ensure that destructor is called in the end of the block.

Actually, I think this behavior is very unexpected. I think we should add ISink::flush() and modify Pump to call flush() when it exits from loop. For most sinks, flush() will be just no-op, but sinks that use buffering will implement it.

I've created an issue: #703

Note that, apparently, you can't use:

core::ScopedPtr<IDevice> backend_device(backend.open_device(...))

and instead have to use core::ScopedPtr<ISink> after you call to_sink(). That's because ISink/ISource uses virtual inheritance to derive IDevice, and pointer to IDevice has different address than pointer to ISink/ISource.

Usually we create sinks/sources via BackendDispatcher, which returns ISink/ISource, not IDevice, so this problem usually not present. However this test is a bit more low-level as it uses BackendMap directly,

Another note: in your test, WAV backend is failing because WavSource forbids providing non-empty sample spec. I guess we should use empty sample spec when constructing WavSource/SoxSource/SndfileSource, and SndfileSource should forbid non-empty spec as well.

It corresponds to this previous discussion from chat, because --rate is stored in sample spec:

given it second thought... I think we could simplify convention and just forbid using --rate argument with files at all, and just fail if rate is non-zero. (we can do it in sox too, but only in case if we're opening a file)

And final note - instead of skipping backend if it fails to open file, it'd be better to check if wav is reported by discover_drivers(), and if yes, require backend to be able to open file and fail the test if open fails. Otherwise the test can succeed if backend supports wav but there is a bug in open().

BTW, nice idea to add name() to backends!

[gavv]

One more follow-up issue: #704

[Hrick87]

@gavv Hi, I've got a several last questions and then I'll be ready to submit my PR for review.

Questions about code

Should the auto sample rate of sndfile_sink be 48000?

roc-toolkit/src/internal_modules/roc_sndio/target_sndfile/roc_sndio/sndfile_sink.cpp

Lines 159 to 161 in e454729

	if (file_info_.samplerate == 0) {
	file_info_.samplerate = 48000;
	}

I admittedly picked a sample rate of 48,000 simply because its a pretty high quality while not generating too large a file. Also sndfile requires we set sample rate, channels, and format before sf_write() occurs. This leads me to my next question.

Should I have a guard in place for number of channels and format as well?

Currently I only handle the case of sample_rate being empty in sndfile_sink, and it seems none of the test cases currently in roc_sndio account for a possibility where the channels or format is not set. I'm wondering if this is because its not possible for it to happen, and I don't need to worry about this.

Is setting the format to a signed 32 bit integer PCM format for all writes ok?

roc-toolkit/src/internal_modules/roc_sndio/target_sndfile/roc_sndio/sndfile_sink.cpp

Lines 153 to 155 in e454729

	// file_info_.format = (int)config.sample_spec.pcm_format(); this needs to be
	// converted to corresponding enum of sndfile somehow
	file_info_.format = SF_FORMAT_PCM_32;

Sndfile_sink currently uses the SF_FORMAT_PCM_32 as its PCM format for all writes. I chose this because it has the highest compatibility with most file extensions versus the highest level of quality achievable. In map_to_sndfile() I have a section of code that essentially accounts for the only two file extensions that need a different sub format, and they first attempt the higher quality, then the lower quality, before giving up and returning false. (FORMAT_COUNT is 2).

roc-toolkit/src/internal_modules/roc_sndio/target_sndfile/roc_sndio/sndfile_sink.cpp

Lines 100 to 123 in e454729

	int temp_format = 0;
	for (int format_attempt = 0; format_attempt < FORMAT_COUNT; format_attempt++) {
	if (format_enum == SF_FORMAT_XI) {
	sfinfo.channels = 1;
	if (format_attempt == 0) {
	temp_format = format_enum | SF_FORMAT_DPCM_16;
	} else {
	temp_format = format_enum | SF_FORMAT_DPCM_8;
	}
	} else {
	if (format_attempt == 0) {
	temp_format = format_enum | SF_FORMAT_PCM_24;
	} else {
	temp_format = format_enum | SF_FORMAT_PCM_16;
	}
	}
	sfinfo.format = temp_format;
	if (sf_format_check(&sfinfo)) {
	return true;
	}
	}

This feels hacky and probably not intended. The issue is, the only alternative I can think of is mapping the PCM format enums of roc_toolkit to the PCM format enums of sndfile and then assigning it to file_info_.format. This would be quite a large table to map, unless there's a clever way to map without a table that I'm not thinking of. I'd like your thoughts on this.

Is this acceptable until we have is_state() implemented?

roc-toolkit/src/tests/roc_sndio/test_backend_source.cpp

Lines 340 to 358 in e454729

	if (strcmp(backend.name(), "SoX") == 0) {
	// TODO(gh-706): check state
	CHECK(!backend_source->read(frame2));
	CHECK(backend_source->restart());
	// TODO(gh-706): check state
	CHECK(backend_source->read(frame2));
	} else {
	// TODO(gh-706): check state
	CHECK(backend_source->read(frame2));
	CHECK(backend_source->restart());
	// TODO(gh-706): check state
	CHECK(backend_source->read(frame2));
	}

and

roc-toolkit/src/tests/roc_sndio/test_backend_source.cpp

Lines 270 to 288 in e454729

	if (strcmp(backend.name(), "SoX") == 0) {
	// TODO(gh-706): check state
	CHECK(!backend_source->read(frame2));
	CHECK(backend_source->resume());
	// TODO(gh-706): check state
	CHECK(backend_source->read(frame2));
	} else {
	// TODO(gh-706): check state
	CHECK(backend_source->read(frame2));
	CHECK(backend_source->resume());
	// TODO(gh-706): check state
	CHECK(!backend_source->read(frame2));
	}

SoX supports pausing, so even with a file it should still simulate the pause from my understanding. This means different behavior is expected between SoX and other backends from the read() between pause() and restart(). I know this doesn't keep things completely generic, but I'm thinking it will have to do until is_state() issue is closed. Let me know if you think of a better solution for now.

Is noop even necessary in source and sink tests anymore/is this proper implementation if it is?

roc-toolkit/src/tests/roc_sndio/test_backend_source.cpp

Lines 76 to 104 in e454729

	TEST(backend_source, noop) {
	for (size_t n_backend = 0; n_backend < BackendMap::instance().num_backends();
	n_backend++) {
	core::TempFile file("test.wav");
	IBackend& backend = BackendMap::instance().nth_backend(n_backend);
	if (!supports_wav(backend)) {
	continue;
	}
	{
	test::MockSource mock_source;
	IDevice* backend_device = backend.open_device(
	DeviceType_Sink, DriverType_File, NULL, file.path(), sink_config, arena);
	CHECK(backend_device != NULL);
	core::ScopedPtr<ISink> backend_sink(backend_device->to_sink(), arena);
	CHECK(backend_sink != NULL);
	Pump pump(buffer_factory, mock_source, NULL, *backend_sink, FrameDuration,
	SampleSpecs, Pump::ModeOneshot);
	CHECK(pump.is_valid());
	CHECK(pump.run());
	}
	IDevice* backend_device = backend.open_device(
	DeviceType_Source, DriverType_File, NULL, file.path(), source_config, arena);
	CHECK(backend_device != NULL);
	core::ScopedPtr<ISource> backend_source(backend_device->to_source(), arena);
	CHECK(backend_source != NULL);
	}
	}

and

roc-toolkit/src/tests/roc_sndio/test_backend_sink.cpp

Lines 56 to 70 in e454729

	TEST(backend_sink, noop) {
	for (size_t n_backend = 0; n_backend < BackendMap::instance().num_backends();
	n_backend++) {
	IBackend& backend = BackendMap::instance().nth_backend(n_backend);
	if (!supports_wav(backend)) {
	continue;
	}
	core::TempFile file("test.wav");
	IDevice* backend_device = backend.open_device(
	DeviceType_Sink, DriverType_File, NULL, file.path(), sink_config, arena);
	CHECK(backend_device != NULL);
	core::ScopedPtr<ISink> backend_sink(backend_device->to_sink(), arena);
	CHECK(backend_sink != NULL);
	}
	}

As you can see I tried to keep no-op in the test cases, but ran into a bit of a problem. When making them generic, this means I no longer have access to simply constructing them. backend_device only allows for opening, which is technically an operation... So essentially I just made the test to be opening and then closing the devices. Maybe a renaming of the test is in order if you do decide we should keep them like this?

I think this code is unnecessary, can I remove it?

roc-toolkit/src/internal_modules/roc_sndio/target_sndfile/roc_sndio/sndfile_source.cpp

Lines 198 to 202 in e454729

	if ((size_t)n_samples < samples_per_ch) {
	memset(frame.raw_samples() + (size_t)n_samples * num_channels, 0,
	(samples_per_ch - (size_t)n_samples) * num_channels
	* sizeof(audio::sample_t));
	}

At the time you were unaware of sf_read()'s behavior when you suggested this bit of code to me in an earlier comment. Due to the fact that sf_read() signifies end of file by returning less than the expected amount or 0, this now seems incorrect. Do I need some new condition for memset here, or is it safe to remove it entirely?

Build Errors

How can I resolve the build error for the two failing macOS checks?

After doing some research about carbon.h I found these two relevant posts:

• https://stackoverflow.com/questions/20990375/fatal-error-installing-libsndfile-on-os-x-mavericks
• https://trac.macports.org/ticket/33409
  It seems to me that this is a know issue with libsndfile version 1.0.25 on macOS that was patched in later versions of sndfile. It also looks like some people have created their own patches for version 1.0.25 (see second link). I am struggling with how I can go about fixing this issue within SConscript using this information. Let me know if you have any ideas.

How can I resolve the build error for the two failing ARM architecture checks?

sndfile.c contains an assert to verify sf_count_t is 8 bytes. sf_count_t resolves to int64_t, which further resolves to a signed long. On these two ARM architectures, it seems the assert fails. I'm guessing it is because they are both 32 bit architectures. I am at a loss as to what to do about this. My research has been unhelpful, as many suggestions were to typedef the signed long variable type to int64_t to stay consistent between 32 bit and 64 bit platforms, but obviously that's already being done. Let me know if you have any ideas.

Any other concerns?

If you see anything else in the code while going over these that looks concerning or you want explanations for, please let me know. I want to try and wrap this up in one go after these questions are answered. Thank you!

[gavv]

Should the auto sample rate of sndfile_sink be 48000?

I think right now 44100 is a better default for us, for two reasons:

• for now, CLI tools use static (predefined) RTP payload types and can't use dynamic types; there are static types for 44100, but not for 48k, so if sink/source is 48k, pipeline will do resampling (C API allows to use custom rates, but CLI tools can't do it yet)

• consumer sound cards like Intel HDA usually support 44100 natively

So if 44100 is default, then on the typical case when the user runs roc-recv/roc-send on laptop or raspberry, the whole chain between file on on side and speakers/mic on the other won't need resampling.

Should I have a guard in place for number of channels and format as well?

Yes, we need something like this:

roc-toolkit/src/internal_modules/roc_sndio/wav_sink.cpp

Lines 44 to 57 in e86ccda

	if (sample_spec_.sample_format() == audio::SampleFormat_Invalid) {
	sample_spec_.set_sample_format(audio::SampleFormat_Pcm);
	sample_spec_.set_pcm_format(audio::Sample_RawFormat);
	}
	if (sample_spec_.sample_rate() == 0) {
	sample_spec_.set_sample_rate(DefaultRate);
	}
	if (!sample_spec_.channel_set().is_valid()) {
	sample_spec_.channel_set().set_layout(audio::ChanLayout_Surround);
	sample_spec_.channel_set().set_order(audio::ChanOrder_Smpte);
	sample_spec_.channel_set().set_channel_range(0, DefaultChans, true);
	}

(Since we're saving sample_spec, I suggest to apply defaults to sample_spec before copying values to file_info_).

Is setting the format to a signed 32 bit integer PCM format for all writes ok?

Yes, ideally I think we need to do this:

• if format is not specified in sample spec, use whatever format we think is good
• if format is specified, query sndfile what subformats are supported for given file type, and map sample spec format to it (e.g. wav file type supports floats, integers, etc); if format can't be mapped, return error

For now CLI tools never set format in sample spec, so we don't need to handle it in this PR.

We have two issues for handling format in backends: #696, #608.

Would be nice if you add a comment like TOTO(gh-696): map format from sample_space.

Sndfile_sink currently uses the SF_FORMAT_PCM_32 as its PCM format for all writes. I chose this because it has the highest compatibility with most file extensions versus the highest level of quality achievable. In map_to_sndfile() I have a section of code that essentially accounts for the only two file extensions that need a different sub format, and they first attempt the higher quality, then the lower quality, before giving up and returning false. (FORMAT_COUNT is 2).

This feels hacky and probably not intended.

Actually I think this logic makes sense, though maybe we can modify it a bit: instead of hard-coding specific types like SF_FORMAT_XI, we instead can create a static array of subformats sorted from high priority (like float32 as you said) to low priority (like smaller integers).

Then, no matter of the file type, we can first try use format without subformat, and then try each subformat from the list, until first successful attempt.

For now this can be the only behavior we support. Later this behavior will be used only if format is not specified in sample spec.

Is this acceptable until we have is_state() implemented?

I think that's OK.

Is noop even necessary in source and sink tests anymore/is this proper implementation if it is?

The test probably still makes sense, it checks that after we created a file using sink, we can open it using source.

So yeah, maybe just rename it to something like write_open or something.

I think this code is unnecessary, can I remove it?

I think we need the following behavior: if sndfile returns less samples than requested, we should return a frame with what we got from sndfile, padded with zeros until frame end. On the next call we should return false.

Why:

• if we return false immediately, we'll loose last chunk instead of sending & playing it; that's not cool
• currently we can't report partial read, so we have to pad chunk with zeros up to the full frame size

I think same behavior is implemented in Sox.

[gavv]

How can I resolve the build error for the two failing ARM architecture checks?

If you look here: https://github.com/roc-streaming/roc-toolkit/actions/runs/8101072211/job/22140438520?pr=660

You can see:

     BUILD   build/3rdparty/arm-linux-gnueabihf/gcc-4.9.4-release/sndfile-1.0.25
[download] http://www.mega-nerd.com/libsndfile/files/libsndfile-1.0.25.tar.gz
[unpack] libsndfile-1.0.25.tar.gz

Now we can go to sndfile sources.

For 1.0.25: https://github.com/libsndfile/libsndfile/blob/1.0.25/src/sndfile.h.in#L318

For master: https://github.com/libsndfile/libsndfile/blob/master/include/sndfile.h#L368

So actually it's not int64_t in 1.0.25, and I guess this is a bug that was fixed in some later version.

So I think we can just bump libsndile version to the lowest one that doesn't have the bug.

Again, from this link: https://github.com/roc-streaming/roc-toolkit/actions/runs/8101072211/job/22140438520?pr=660

You can copy the very first command:

scripts/ci_checks/docker.sh rocstreaming/toolchain-arm-linux-gnueabihf:gcc-4.9 scripts/ci_checks/linux-arm/arm-linux-gnueabihf-gcc-4.9.sh

This way you can run exact same CI check locally (in docker). So you can try different sndfile versions and see which one would work.

My research has been unhelpful, as many suggestions were to typedef the signed long variable type to int64_t to stay consistent between 32 bit and 64 bit platforms, but obviously that's already being done. Let me know if you have any ideas.

Yes, that platform is 32-bit, it uses 32-bit pointers and size_t, but it still supports 64-bit integers and int64_t, they're just not as efficient as 32-bit.

So it looks like sndfile is inconsistent when it first resolves sf_count_t to a 32-bit type but then requires it to be 64-bit. The latest version that just uses int64_t should work fine on 32-bit platform.

[gavv]

How can I resolve the build error for the two failing macOS checks?

It seems to me that this is a know issue with libsndfile version 1.0.25 on macOS that was patched in later versions of sndfile. It also looks like some people have created their own patches for version 1.0.25 (see second link). I am struggling with how I can go about fixing this issue within SConscript using this information. Let me know if you have any ideas.

If it's fixed in later version, let's just bump to minimum version that works properly.

If you know what's that version, let's try it. If you need to do some testing and don't have macOS, let me know, I could do the testing. (I hate that you can't cross-compile to mac from other systems...)

[gavv]

Any other concerns?

Three small comments.

---

        if ((strcmp(format_info.extension, "wav") == 0)
            || (strcmp(format_info.extension, "mat") == 0)) {

Seems that we can remove this check and just always use table? (file_type_map) If there is an entry for the format, use table, otherwise use format_info.extension.

This way the knowledge about extensions & driver will be completely isolated in the table.

---

And the second note, map_to_sndfile looks a bit too long, maybe we can split it into two functions? E.g. one to select format, another to select subformat.

---

Finally, regarding FileMap:

• it would be great to add a comment briefly explaining why we need it and who this mappings are used
• we don't use #pragma once
• I think it makes sense to move table definition to a .cpp file so that it's not duplicated in every .cpp that uses this header

[Hrick87]

If it's fixed in later version, let's just bump to minimum version that works properly.

If you know what's that version, let's try it. If you need to do some testing and don't have macOS, let me know, I could do the testing. (I hate that you can't cross-compile to mac from other systems...)

version 1.0.26 resolved both build issues! Everything you mentioned has been fixed. Ready for a (hopefully) final review and merge :)

[gavv]

Quick question.

In SndfileSink::open, you have:

sample_spec_.set_sample_rate((unsigned long)file_info_.samplerate);

In which case opening file for writing may change requested rate?

(Note that the rate is never zero, we explicitly set it to 44100 in ctor if it's zero).

[gavv]

And another one.

Is there a reason to have this code:

    if (!eof_) {
        if (!seek_(0)) {
            roc_log(LogError, "sndfile source: seek failed when restarting");
            return false;
        }
    } else {
        if (file_) {
            close_();
        }

        if (!open_(path_)) {
            roc_log(LogError, "sndfile source: open failed when restarting");
            return false;
        }
    }

which may reopen file, instead of just always using seek?

[gavv]

which may reopen file, instead of just always using seek?

Oh, I see, some file types are unseekable.

[gavv]

Thanks a lot for PR! That was big!

I have pushed a few follow-up patches:

• ebd0c7a
• 19c77d3 - cosmetic changes
• 35c87b9 - fix a few bugs in sndfile backend
• 608963d - cleanup sox backend a bit
• 71b4749 - better channel count handling