Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

shmem input is broken #375

Closed
atisharma opened this issue Aug 15, 2020 · 20 comments · Fixed by #376
Closed

shmem input is broken #375

atisharma opened this issue Aug 15, 2020 · 20 comments · Fixed by #376

Comments

@atisharma
Copy link

I complied cava from source (0.7.2) on raspbian and tried from the void repo (v0.7.2) on x64.

Starting cava to use the shmem input with squeezelite fails with

higher cuttoff frequency can't be higher then sample rate / 2input_shmem: source: /squeezelite-dc:##:##:##:##:##:##

I checked and /dev/shm/... is there.

This seems to be because line 510 of cava.c has this line commented out:
// audio.rate = 44100;

As a result, audio.rate defaults to 0 and cava exits with failure.

I uncommented this line and compiled, and it runs, and seems to be OK.

@atisharma
Copy link
Author

atisharma commented Aug 17, 2020

To add to this, it works, but is pegging a core to 100% or thereabouts.
I tested this on both rpi 4 64-bit debian aarch64, Linux 5.4.51-v8+, and on Void on an i9 x86_64, Linux 5.7.15_1.

@karlstav
Copy link
Owner

ih @atisharma

the main loop did not wait for the shmem input module to set the rate. should be fixed now.

for the cpu usage there is some logic missing in the shmem input module.

there should probably some checking of whether or not this variable changes:

time_t updated;

and then sleep for some ms if there are no update.

@atisharma
Copy link
Author

Excellent! I can confirm your fix works.

Would you like me to add the timing check you mention to cava/input/shmem.c ?

@karlstav
Copy link
Owner

I made an attempt, but I have no way of testing it.

If it doesn't work, please feel free to have a go at it.

@atisharma
Copy link
Author

Thanks for the very fast response!
Unfortunately that broke it. The vis looks like a bowl shape indicating it's not reading the memory correctly any more.
I'll have a further look at the commit and have a go.

@karlstav
Copy link
Owner

does it work better after latest commit:

dc77e9e

I tried to ref this issue but i hit the '%' instead of the '#' and did not notice.

karlstav added a commit that referenced this issue Aug 17, 2020
@atisharma
Copy link
Author

atisharma commented Aug 17, 2020

It does, thanks.

I've been having a look at the squeezelite source that writes to shmem.
It doesn't update updated so far as I can tell.
But, buf_index is incremented on every frame (and reset to zero occasionally).
So I tried

while (!audio->terminate) {
    if (mmap_area->running) {
        if (mmap_area->buf_index != last_write) {
            write_to_fftw_input_buffers(mmap_area->buffer, BUFSIZE, audio);
            last_write = mmap_area->buf_index;
        }
        nanosleep(&req, NULL);
    } else {
        write_to_fftw_input_buffers(mmap_area->buffer, BUFSIZE, audio);
        // write zeros to fftw here....
        nanosleep(&req, NULL);
    }
}

which seems also to work. (I don't know how to write zeros on silence though...)

I have a question: how did you calculate req.tv_nsec in the previous try? Why does BUFSIZE appear?

edit: updated is set to time(NULL) in squeezelite, no idea how I missed it.

@atisharma
Copy link
Author

Since shmem input is working now, and since the questions about BUFSIZE and writing zeros are not directly relevant to the issue, I'll close the issue.
Thanks again!

@karlstav
Copy link
Owner

no problem, I did brake this a while back, so it's ok to get it fixed.

req.tv_nsec = (1000000 / mmap_area->rate) * BUFSIZE;

this is essentially the time of one whole buffer (around 40ms with 44100 rate). I set it to sleep that long since there is no need to write data any faster than that. But we should probably include your changes regarding the buf_indexas well.

you could create some kind of dummy "silence" buffer before the while loop like this:

int16_t silence_buffer[BUFSIZE];

for (int i = 0; i < BUFSIZE; i++)
    silence_buffer[i] = 0;

and use it like this:

write_to_fftw_input_buffers(silence_buffer, BUFSIZE, audio);

But is it necessary? What happens if there is no audio? Is it uninitialized data in the buffers (the bowl shape you mentioned earlier)?

one more thing I don't really understand is the

#define VIS_BUF_SIZE 16384
#define VB_OFFSET 8192+4096

the original commit by @dnalor we only use the data in the mmap buffer from index 12288 to 14336 (2048 16 bit samples). But I have no idea why. Maybe we should use the whole buffer? Maybe the buffer also could be smaller?

Is there any documentation on this? I can't seem to find it.

You could try to play around with those number, see what difference it makes.

@atisharma
Copy link
Author

Thanks for the explanation. That seems reasonable.

What happens if there is no audio? Is it uninitialized data in the buffers (the bowl shape you mentioned earlier)?

Basically it just leave the spectrum as it was (whatever shape it was when it's paused or stopped, for instance) since I think squeezelite doesn't clear the buffer, it just sets running to false. See here.

That link may also explain the offset. Actually, I think my understanding of buf_index was wrong. Is it a counter over the number of frames in the buffer?

I tried doubling BUFSIZE and I get a dirty signal (a mix of a reasonable fft and the bowl noise). I think only a certain number of frames are written. Reducing to 1024 looks OK.

I tried writing silence as you suggest above, and the array of zeros appears as the bowl shape (default config). It's fair to say I don't know why. Playing silent tracks looks all flat.

As for documentation on squeezelite's shmem feature, I don't think there is any. The original source code I linked to above is about it.

@atisharma
Copy link
Author

I think I have solved both the buf_index question, the buffer size question (almost) and the silence question.
The relevant changes are here:

use the whole buffer:

#define BUFSIZE 8192
#define VIS_BUF_SIZE 16384

silence_buffer was the wrong type, needs to be s16_t and different length:

    s16_t silence_buffer[VIS_BUF_SIZE];
    for (int i = 0; i < VIS_BUF_SIZE; i++)
        silence_buffer[i] = 0;

Wait for the buffer index to pass the amount we are to read, then pass to fft:

    while (!audio->terminate) {
        if (mmap_area->running) {
            if (mmap_area->buf_size > BUFSIZE) {
                write_to_fftw_input_buffers(mmap_area->buffer, BUFSIZE, audio);
                nanosleep(&req, NULL);
            }
        } else {
            write_to_fftw_input_buffers(silence_buffer, BUFSIZE, audio);
            nanosleep(&req, NULL);
        }
    }

I am still not sure why BUFSIZE has to be 1/2 of VIS_BUF_SIZE (stereo channels perhaps?), but increasing it beyond 8192 fails.

@atisharma atisharma reopened this Aug 18, 2020
@karlstav
Copy link
Owner

do you have stereo enabled in your cava config? can you confirm that it is actually left and right channel that you see?

@atisharma
Copy link
Author

I have stereo enabled (channels = stereo). I can confirm that with the code above left and right are correctly separated. I checked with this mp3.

atisharma pushed a commit to atisharma/cava that referenced this issue Aug 19, 2020
@atisharma atisharma mentioned this issue Aug 19, 2020
@karlstav
Copy link
Owner

karlstav commented Sep 8, 2020

Hi @atisharma , could you test shmem with the latest commits? I think we might need to reduce the number of bytes read on each iteration. It should be no more than 512 (fft treble input buffer size) frames (preferably equal) written to the fft buffers.

BTW is there an easy way to test shmem? I am thinking about implementing some kind of automated testing to the github actions.

@docgalaxyblock
Copy link

Hi guys I like to aks first here before open a new issue
I purchased a Pirate Audio Hat by Pimoroni and started searching alternative software for the lcd
Now I tried cava but I got the error (squeezelite -v is set)

root@DietPi:~# cava -p /root/.config/cava/config
malloc(): corrupted top size

Have you any idear?
Running squeezelite 1.8-4.1+b1 (Pi 3 A+, DietPi)
and server 8.1.0 - 1608700893 (Odroid XU4, DietPi)
Greetings

@karlstav
Copy link
Owner

@docgalaxyblock running as root looks a bit strange. Are you sure it's necessary?

@Jocker666z
Copy link
Contributor

Hi guys I like to aks first here before open a new issue I purchased a Pirate Audio Hat by Pimoroni and started searching alternative software for the lcd Now I tried cava but I got the error (squeezelite -v is set)

root@DietPi:~# cava -p /root/.config/cava/config
malloc(): corrupted top size

Have you any idear? Running squeezelite 1.8-4.1+b1 (Pi 3 A+, DietPi) and server 8.1.0 - 1608700893 (Odroid XU4, DietPi) Greetings

Same error here
This error appears at version 0.7.3.
The version 0.7.2 with this patch -> 693543f - Work.
My system:
Raspberry 3
Raspbian GNU/Linux 11 (bullseye)
Squeezelite v1.9.8-1317

@karlstav
Copy link
Owner

hi @Jocker666z,

sorry about this, but the smem input is indeed broken. this is actually the same as #418. I haven't been maintaining this input module properly for a while, but I think I have a fix ready, will push it now.

@Jocker666z
Copy link
Contributor

Jocker666z commented Jan 29, 2022

hi @Jocker666z,

sorry about this, but the smem input is indeed broken. this is actually the same as #418. I haven't been maintaining this input module properly for a while, but I think I have a fix ready, will push it now.

Thanks the malloc bug fixed, but now noise bug appear. It seems to me that it has something to do with this exchange #375 (comment)

As for the background noise when there is silence, a step backwards solves the problem:

    s16_t silence_buffer[VIS_BUF_SIZE];
    for (int i = 0; i < VIS_BUF_SIZE; i++)
        silence_buffer[i] = 0;

I don't know how to solve the problem of background noise while playing music (My knowledge in C is very limited). But I can test all the modifications you want ;)

@karlstav
Copy link
Owner

Yes looks like something is wrong. I will look at it soon.

Please move the discussion over to #418 as this issue was originally for a completely different thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants