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

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

Closed
1 task done
firewall1110 opened this issue Aug 19, 2024 · 13 comments · Fixed by #7673
Closed
1 task done

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

firewall1110 opened this issue Aug 19, 2024 · 13 comments · Fixed by #7673
Labels

Comments

@firewall1110
Copy link
Contributor

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.

SID_bug.mp4

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.

  • I have searched all existing issues and confirmed that this is not a duplicate.
@michaelgregorius
Copy link
Contributor

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.

@firewall1110
Copy link
Contributor Author

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
Copy link
Contributor Author

firewall1110 commented Jan 20, 2025

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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 added a commit to firewall1110/lmms that referenced this issue Jan 30, 2025
@sakertooth
Copy link
Contributor

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
Copy link
Contributor

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

@PhysSong
Copy link
Member

PhysSong commented Feb 1, 2025

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

@sakertooth
Copy link
Contributor

sakertooth commented Feb 1, 2025

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.

@firewall1110
Copy link
Contributor Author

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
Copy link
Contributor Author

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" ...

@PhysSong
Copy link
Member

PhysSong commented Feb 1, 2025

So I do not understand , how #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 PhysSong linked a pull request Feb 1, 2025 that will close this issue
@sakertooth
Copy link
Contributor

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

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

Successfully merging a pull request may close this issue.

4 participants