lmms icon indicating copy to clipboard operation
lmms copied to clipboard

SID XRun bug: pause on 1-st SID note played.

Open firewall1110 opened this issue 1 year ago • 2 comments

System Information

GNU/Linux Debian stable

LMMS Version(s)

LMMS 1.3.0-alpha.1.681+gbda1a9c37 (master build 2024.08.14, using clang14)

Most Recent Working Version

lmms-1.3.0-alpha.1.102+g89fc6c9-linux-x86_64.AppImage

Bug Summary

When 1-st note played by SID all project paused for short time (if using jack audio back-end : XRun messages). After 1-st note played (even in export mode) - no more problems.

https://github.com/user-attachments/assets/5ea45a10-bce9-4ad2-9623-b3467c533b17

Expected Behaviour

No unexpected pauses.

Steps To Reproduce

Open project, containing SID. When 1-st SID note played will be pause (happens only once !).

Logs

Click to expand
  

Screenshots / Minimum Reproducible Project

SID_bug_demo.mmpz.zip

Please search the issue tracker for existing bug reports before submitting your own.

  • [X] I have searched all existing issues and confirmed that this is not a duplicate.

firewall1110 avatar Aug 19 '24 05:08 firewall1110

The problem can also be reproduced with demos/Greippi - Krem Kaakkuja (Second Flight Remix).mmpz. Steps to reproduce:

  1. Start a debug build of LMMS.
  2. Open the demo song demos/Greippi - Krem Kaakkuja (Second Flight Remix).mmpz.
  3. Play the song.

Towards the end of the fourth measure you should experience a short dropout.

michaelgregorius avatar Aug 25 '24 19:08 michaelgregorius

1-st negative result: I tried to simply make new reSID::SID() once in SidInstrument::SidInstrument :

  • no pause on 1-rst note;
  • Segmentation Fault on 2-nd note.

Segmentation Fault in line ~426: const auto num = static_cast<f_cnt_t>(sid_fillbuffer(sidreg.data(), sid, delta_t, buf, frames));

P.S. @michaelgregorius ! Thank You for bug confirmation!

firewall1110 avatar Aug 26 '24 07:08 firewall1110

I treat issue #7638 as a hint:

libsid.so!reSID::EnvelopeGenerator::clock(reSID::EnvelopeGenerator * const this, reSID::cycle_count delta_t) (/home/michael/Eigene Dateien/Development/LMMS/lmms-dev-mg/lmms/plugins/Sid/resid/resid/envelope.h:269)
libsid.so!reSID::SID::clock(reSID::SID * const this, reSID::cycle_count delta_t) (/home/michael/Eigene Dateien/Development/LMMS/lmms-dev-mg/lmms/plugins/Sid/resid/resid/sid.cc:758)
libsid.so!reSID::SID::clock_fast(reSID::SID * const this, reSID::cycle_count & delta_t, short * buf, int n, int interleave) (/home/michael/Eigene Dateien/Development/LMMS/lmms-dev-mg/lmms/plugins/Sid/resid/resid/sid.cc:867)
libsid.so!reSID::SID::clock(reSID::SID * const this, reSID::cycle_count & delta_t, short * buf, int n, int interleave) (/home/michael/Eigene Dateien/Development/LMMS/lmms-dev-mg/lmms/plugins/Sid/resid/resid/sid.cc:841)
libsid.so!lmms::sid_fillbuffer(unsigned char * sidreg, reSID::SID * sid, int tdelta, short * ptr, int samples) (/home/michael/Eigene Dateien/Development/LMMS/lmms-dev-mg/lmms/plugins/Sid/SidInstrument.cpp:273)

(Edited:) This is wrong: So may be in my solution (make memory allocation for emulator in SID constructor) I simply expose another bug ... (Edited:) I has not understood how SID is working - it need new instance at every new note played, and this instance is deleted after note off (but this delete is not in SidInstrument.cpp)

P.S. I hope to fix this bug but now I am "paralyzed" with my semi professional musician work ...

firewall1110 avatar Jan 20 '25 07:01 firewall1110

New observation:

If I place in constructor SidInstrument::SidInstrument( InstrumentTrack * _instrument_track )

line auto sid = new reSID::SID(); (and after line if (sid) {fprintf(stderr, "\nnew reSID::SID(); :: Done\n");} for no compiler warning)

It seems bug not noticed (but anyway SidInstrument implementation is not RT-safe) So real problem is then auto sid = new reSID::SID(); called 1-st time.

firewall1110 avatar Jan 23 '25 12:01 firewall1110

New observation:

(1) In SidInstrument::SidInstrument( InstrumentTrack * _instrument_track ) I can put line: reSID::SID sid;

And this fix XRun bug

(2) In SID::SID() I can (with (1)) remove line: set_sampling_parameters(985248, SAMPLE_FAST, 44100);

And this fix XRun bug

So it seems, that calling SidInstrument::SidInstrument 1-st time some file is loaded from disk .

firewall1110 avatar Jan 24 '25 09:01 firewall1110

So it seems, that calling SidInstrument::SidInstrument 1-st time some file is loaded from disk .

It actually seems like its calculating tables (which are then later cached), based on the code in sid.cc.

sakertooth avatar Jan 31 '25 22:01 sakertooth

Though, its still somewhat confusing because its presumably being recalculated for each note being played.

sakertooth avatar Jan 31 '25 22:01 sakertooth

FYI, the actual place where a cached calculation occurs is reSID::Filter::Filter, which has a initialization guarded with class_init.

PhysSong avatar Feb 01 '25 02:02 PhysSong

FYI, the actual place where a cached calculation occurs is reSID::Filter::Filter, which has a initialization guarded with class_init.

I was referring to the code in SID::set_sampling_parameters, which is also called on construction. In there, there's some kind of caching near the end. Seemed like a possible reason for a dropout. Could be wrong though. I was expecting some kind of static initialization somewhere if the solution was to infact do the initialization first before trying to play notes, so this might not be the right place to look, however the code in reSID::SID doesn't seem to be doing any of that.


  /* Determine if we need to recalculate table, or whether we can reuse earlier cached copy.
   * This pays off on slow hardware such as current Android devices.
   */
  if (fir && fir_RES_new == fir_RES && fir_N_new == fir_N && beta == fir_beta && f_cycles_per_sample == fir_f_cycles_per_sample && fir_filter_scale == filter_scale) {
      return true;
  }
  fir_RES = fir_RES_new;
  fir_N = fir_N_new;
  fir_beta = beta;
  fir_f_cycles_per_sample = f_cycles_per_sample;
  fir_filter_scale = filter_scale;

  // Allocate memory for FIR tables.
  delete[] fir;
  fir = new short[fir_N*fir_RES];

  // Calculate fir_RES FIR tables for linear interpolation.

sakertooth avatar Feb 01 '25 02:02 sakertooth

however the code in reSID::SID doesn't seem to be doing any of that.

Yes I have tested (#7673) with one line removed:


SID::SID()
{
  // Initialize pointers.
  sample = 0;
  fir = 0;
  fir_N = 0;
  fir_RES = 0;
  fir_beta = 0;
  fir_f_cycles_per_sample = 0;
  fir_filter_scale = 0;

  sid_model = MOS6581;
  voice[0].set_sync_source(&voice[2]);
  voice[1].set_sync_source(&voice[0]);
  voice[2].set_sync_source(&voice[1]);

 // set_sampling_parameters(985248, SAMPLE_FAST, 44100); // REMOVED LINE

  bus_value = 0;
  bus_value_ttl = 0;
  write_pipeline = 0;

  databus_ttl = 0;

  raw_debug_output = false;
}

And all work, and this is very strange ...

So I do not understand , how #7673 fix bug, but it fix ...

P.S. There are a lot of not RT safe code , and I plane to make it better (or even RT-safe) in #7638 context. But #7638 is not critical bug (my opinion).

firewall1110 avatar Feb 01 '25 07:02 firewall1110

If insert line (in SID::set_sampling_parameters)


  // FIR initialization is only necessary for resampling.
  if (method != SAMPLE_RESAMPLE && method != SAMPLE_RESAMPLE_FASTMEM)
  {
    delete[] sample;
    delete[] fir;
    sample = 0;
    fir = 0;
    return true;
  }

fprintf(stderr, "SID::set_sampling_parameters - get after\n"); // DEBUG LINE INSERTED

fprintf(stderr, "SID::set_sampling_parameters - get after\n"); is never called. So all after (in SID::set_sampling_parameters) seems be a "dead code" ...

firewall1110 avatar Feb 01 '25 07:02 firewall1110

So I do not understand , how https://github.com/LMMS/lmms/pull/7673 fix bug, but it fix ...

SID has member variables with type Filter, so creating a SID instance will ensure the initialization code in Filter's constructor is executed, which runs only once.

PhysSong avatar Feb 01 '25 10:02 PhysSong

SID has member variables with type Filter, so creating a SID instance will ensure the initialization code in Filter's constructor is executed, which runs only once.

I can confirm that this is most likely the reason after debugging with GDB and breaking execution exactly when the dropout happens. In which case we should only need to create a Filter object instead of a SID if that is really the case.

Thread 17 (Thread 0x7e251affe6c0 (LWP 658639) "lmms::AudioEngi"):
#0  0x00007e24c5e1b5cc in reSID::Filter::solve_gain (this=0x7e250c0018e0, opamp=0x7e250c0419f0, n=384, vi=42950, x=@0x7e251aff55d0: 21827, mf=...) at /home/saker/projects/lmms/plugins/Sid/resi
d/resid/filter.h:1371
#1  0x00007e24c5e1a4c6 in reSID::Filter::Filter (this=0x7e250c0018e0) at /home/saker/projects/lmms/plugins/Sid/resid/resid/filter.cc:331
#2  0x00007e24c5e13cfe in reSID::SID::SID (this=0x7e250c0016b0) at /home/saker/projects/lmms/plugins/Sid/resid/resid/sid.cc:56
#3  0x00007e24c5e1e924 in lmms::SidInstrument::playNote (this=0x5d1a243a1190, _n=0x5d1a1d5cbbc0, _working_buffer=0x7e24bc0033c0) at /home/saker/projects/lmms/plugins/Sid/SidInstrument.cpp:296
#4  0x00005d1a0fa5376d in lmms::InstrumentTrack::playNote (this=0x5d1a23ee11d0, n=0x5d1a1d5cbbc0, workingBuffer=0x7e24bc0033c0) at /home/saker/projects/lmms/src/tracks/InstrumentTrack.cpp:584
#5  0x00005d1a0f840d42 in lmms::NotePlayHandle::play (this=0x5d1a1d5cbbc0, _working_buffer=0x7e24bc0033c0) at /home/saker/projects/lmms/src/core/NotePlayHandle.cpp:263
#6  0x00005d1a0f84f5f9 in lmms::PlayHandle::doProcessing (this=0x5d1a1d5cbbc0) at /home/saker/projects/lmms/src/core/PlayHandle.cpp:59
#7  0x00005d1a0f7a40ef in lmms::ThreadableJob::process (this=0x5d1a1d5cbbc0) at /home/saker/projects/lmms/include/ThreadableJob.h:77
#8  0x00005d1a0f7a3760 in lmms::AudioEngineWorkerThread::JobQueue::run (this=0x5d1a0fbaf338 <lmms::AudioEngineWorkerThread::globalJobQueue>) at /home/saker/projects/lmms/src/core/AudioEngineWo
rkerThread.cpp:87
#9  0x00005d1a0f7a3ace in lmms::AudioEngineWorkerThread::run (this=0x5d1a1da05ca0) at /home/saker/projects/lmms/src/core/AudioEngineWorkerThread.cpp:176
#10 0x00007e25792f295b in ?? () from /usr/lib/libQt5Core.so.5
#11 0x00007e25784a32ce in ?? () from /usr/lib/libc.so.6
#12 0x00007e257852829c in ?? () from /usr/lib/libc.so.6

sakertooth avatar Feb 02 '25 15:02 sakertooth