HEnquist / camillagui

GNU General Public License v3.0
9 stars 1 forks source link

Configs with $samplerate$ are invalid in the camillagui editor. #27

Closed bitkeeper closed 3 years ago

bitkeeper commented 3 years ago

In the past I would also get that error and just as workarround press "Load from CDSP" and the error is fixed. (Down side was that if you now saved the file you lost the $samplerate$ var, because the full name was used.

image

The correct validation would be to take the samplerate on top of the camillagui and substitute for the $samplerate$ to test if the coeff file is present.

JWahle commented 3 years ago

I'll see, if this can be fixed on the GUI part - we might need to change CDSP to get the validation right.

bitkeeper commented 3 years ago

I think before calling the validation function replacing the $samplerate$ with the sample rate from the configurationn would be enough ?

Funny thing if I call camilladsp -c with the same config, this is exact the behaviour of camilladsp. It searches for the conv file where $samplerate$ is the samplerate at top of the configuration file. So validating with camilladsp -c vs camillagui results in other tests.

JWahle commented 3 years ago

The GUI does not have its own validation logic, it just invokes CamillaDSP - just not through command line, but through a websocket. We could build a workaround into the GUI, but I think this is better fixed in CDSP. I created https://github.com/HEnquist/camilladsp/issues/95 for this.

bitkeeper commented 3 years ago

The perceived behaviour of validating with camilladsp -c and camillagui differs. Please read the story below:

Used setup:

Used config

Part of the config file:

devices:
  samplerate: 44100
  ...
  playback:
    type: Alsa
    channels: 2
    device: "hw:2,0"
    format: S32LE
filters:
    ir_left:
      type: Conv
      parameters:
        type: File
        filename: ../coeffs/test_samplerate_$samplerate$Hz.wav
        format: S16LE
 ...

Validating with cmdline

Validating with cmdline with a samplerate which match an existing filter file: This will match the 44100 samplerate from the config

pi@moodedev:/usr/share/camilladsp/configs $ camilladsp -c test_samplerate.yml
Config is valid

This expected.

Validating with cmdline with missing filte filer: For this I change the samplerate in the config to 96000:

devices:
  samplerate: 96000
  ...
pi@moodedev:/usr/share/camilladsp/configs $ camilladsp -c test_samplerate.yml
Config is not valid
Invalid filter 'ir_left'. Reason: Could not open coefficient file '../coeffs/test_samplerate_96000Hz.wav'. Error: No such file or directory (os error 2)

This is also expected, the config is not valid because it could not find the matching filter file. Also proves that camilladsp -c substitute the samplerate from the config when validating.

Validating with camillagui

Now we test the same config as with the commandline.

After opening with camillagui (while playing music) This time the validation fails and the error message show a filter file which doesn't substitute the $samplerate$: image

After pressing 'Load from CDSP' (while playing music) If you press the 'Load from CDSP' the config is valid, because CDSP has substitute the $samplerate$. image

If you now look at the filter settings you see indeed the substitute $samplerate$:

image If you now save 'Apply the config' again you have overwriten your config and did loose all the substitute vars.

After pressing 'Load from CDSP' (while being offline, no music) This time the behaviour is again different, again error message about invalid filter file. image

Conclusion

It is clear that camillagui config validation behaves different then from the cmdline. Also the behaviour between online differs from offline.

JWahle commented 3 years ago

Thanks for the detailed explanation. Don't worry - we understand the problem and this WILL get fixed :)

HEnquist commented 3 years ago

The develop branch of camilladsp has a fix for this now!

JWahle commented 3 years ago

Fixed, as HEnquist/camilladsp#95 is fixed.