[Csantucci]

I would like to get a bit of attention to better programmers than myself about this bug report: https://bugs.launchp...or/+bug/1406179 .
It occurred also to me today , and luckily enough it happened during a debug session. I was passing from one train to another with the Alt-9 command: the crash occurred in the updater process during these instructions of TrainCar.cs (#1453), and in particular in the first OpenAL... instruction:

            var soundSourceIDs = SoundSourceIDs.ToArray();
            foreach (var soundSourceID in soundSourceIDs)
            {
                OpenAL.alSourcefv(soundSourceID, OpenAL.AL_POSITION, position);
                OpenAL.alSourcefv(soundSourceID, OpenAL.AL_VELOCITY, Velocity);
            }

At the moment of the crash there are three soundSourceIDs: 47, 48 and 49.

As I was in debug I could check what the sound process was doing at the same moment: it was in line 628 (it's the third OpenAL... line) of ALSoundHelper.cs, within this block:

            if (Ignore3D)
            {
                OpenAL.alSourcei(SoundSourceID, OpenAL.AL_SOURCE_RELATIVE, OpenAL.AL_TRUE);
                OpenAL.alSourcef(SoundSourceID, OpenAL.AL_DOPPLER_FACTOR, 0);
                OpenAL.alSource3f(SoundSourceID, OpenAL.AL_POSITION, 0, 0, 0);
                OpenAL.alSource3f(SoundSourceID, OpenAL.AL_VELOCITY, 0, 0, 0);
            }

At the moment of the crashe the SoundSourceID is 55, so one not existing within soundSourceIDs.

so there is a conflict in dealing with a SoundSourceID. Again, moving the SoundSourceIDs in a new array as done in TrainCar.cs does not seem to solve the problem.

Solutions, apart another lock, that James does not like? Am I authorized to insert a lock? Would it be better to move the loop from TrainCar.cs to the sound process?

[dennisat]

 Csantucci, on 16 January 2015 - 09:18 AM, said:

I would like to get a bit of attention to better programmers than myself about this bug report: https://bugs.launchp...or/+bug/1406179 .
It occurred also to me today , and luckily enough it happened during a debug session. I was passing from one train to another with the Alt-9 command: the crash occurred in the updater process during these instructions of TrainCar.cs (#1453), and in particular in the first OpenAL... instruction:

            var soundSourceIDs = SoundSourceIDs.ToArray();
            foreach (var soundSourceID in soundSourceIDs)
            {
                OpenAL.alSourcefv(soundSourceID, OpenAL.AL_POSITION, position);
                OpenAL.alSourcefv(soundSourceID, OpenAL.AL_VELOCITY, Velocity);
            }

At the moment of the crash there are three soundSourceIDs: 47, 48 and 49.

As I was in debug I could check what the sound process was doing at the same moment: it was in line 628 (it's the third OpenAL... line) of ALSoundHelper.cs, within this block:

            if (Ignore3D)
            {
                OpenAL.alSourcei(SoundSourceID, OpenAL.AL_SOURCE_RELATIVE, OpenAL.AL_TRUE);
                OpenAL.alSourcef(SoundSourceID, OpenAL.AL_DOPPLER_FACTOR, 0);
                OpenAL.alSource3f(SoundSourceID, OpenAL.AL_POSITION, 0, 0, 0);
                OpenAL.alSource3f(SoundSourceID, OpenAL.AL_VELOCITY, 0, 0, 0);
            }

At the moment of the crashe the SoundSourceID is 55, so one not existing within soundSourceIDs.

so there is a conflict in dealing with a SoundSourceID. Again, moving the SoundSourceIDs in a new array as done in TrainCar.cs does not seem to solve the problem.

Solutions, apart another lock, that James does not like? Am I authorized to insert a lock? Would it be better to move the loop from TrainCar.cs to the sound process?

I've found that as well a long time ago (a year?). I never reported it because no-one else seemed to have the problem. I've fixed and forgotten it. It's in one of my private patches somewhere. You don't need a lock, just moving the code to another thread cures it. I'll try and find the relevant code, it's amongst several patches I've got on the sound process. Unfortunately, once again I'm pressed for time, another holiday starts late tomorrow.

Dennis

[gpz]

 Csantucci, on 16 January 2015 - 09:18 AM, said:

Solutions, apart another lock, that James does not like? Am I authorized to insert a lock? Would it be better to move the loop from TrainCar.cs to the sound process?

If I remember correctly, this step would highly degrade sound quality in cases e.g. an electric locomotive is passing by a camera 4 viewer, because of the sound process update time, so please test it carefully. (This type of degradation is heard best at electric-static type sounds and high locomotive speeds, less at diesels or low speeds.)

[Csantucci]

Dennis,
thank you, I'll test your patch against Peter's concerns when you will have time to provide it. I am also a bit concerned on sound synchronization issues if that block changes process.

[James Ross]

 Csantucci, on 16 January 2015 - 09:18 AM, said:

Solutions, apart another lock, that James does not like? Am I authorized to insert a lock? Would it be better to move the loop from TrainCar.cs to the sound process?

Under no circumstances is a lock to be added to the Render or Updater processes; their unblocked forward progress is required for any chance of getting a smooth frame rate. :)

I would suggest here that the problem is that the Updater process is touching sounds in a non-trivial way. It should only be signalling changes to the Sound process ideally. I don't know if these OpenAL calls are thread-safe but even if they are, we shouldn't make them outside of the Sound process.

The Updater is allowed to prepare information for consumption elsewhere (that's how stuff is passed to the Render process). Would that be appropriate here?

[dennisat]

 James Ross, on 16 January 2015 - 12:50 PM, said:

Under no circumstances is a lock to be added to the Render or Updater processes; their unblocked forward progress is required for any chance of getting a smooth frame rate. :bigboss:

I would suggest here that the problem is that the Updater process is touching sounds in a non-trivial way. It should only be signalling changes to the Sound process ideally.

My cure was to move the sound update and position calculation logic to the Sound process and read the velocity from the Updater process. The Sound Process works out position as well so there didn't seem to be any need to take the position from the Updater process. Definitely no lock needed, I've run this patch for at least a year. I'm afraid I won't be able to provide my patch for another week (see my previous post) - it's buried in a lot of other patches I've made to the Sound process. From my description of the patch you can probably see what is to be done, it wasn't difficult.

Dennis

[James Ross]

 dennisat, on 17 January 2015 - 12:32 AM, said:

My cure was to move the sound update and position calculation logic to the Sound process and read the velocity from the Updater process. The Sound Process works out position as well so there didn't seem to be any need to take the position from the Updater process. Definitely no lock needed, I've run this patch for at least a year. I'm afraid I won't be able to provide my patch for another week (see my previous post) - it's buried in a lot of other patches I've made to the Sound process. From my description of the patch you can probably see what is to be done, it wasn't difficult.

Sounds like an improvement. Reading data owned by one thread from another thread can still be problematic but is usually much easier to make safe. Let us know when you have found your patch.

[gpz]

 dennisat, on 17 January 2015 - 12:32 AM, said:

My cure was to move the sound update and position calculation logic to the Sound process and read the velocity from the Updater process. The Sound Process works out position as well so there didn't seem to be any need to take the position from the Updater process.

This is what I was afraid of, and I disagree with a simple solution like this. Current sound process is not frequent enough for position calculation. Sound sources position setting must be done with high frequency, without any thread sleeping involved for the moving sounds/moving camera to be smooth enough. Otherwise the discrete position setting will surely be audible. (At least I can hear it.)

For a proper patch, position data must be taken from the updater process and be set with high frequency. Velocity setting is not important so much, because that doesn't change so fast, so that can be set from secondary sources, if more appropriate.

[Csantucci]

Peter,
if you have in mind a solution that you think is OK, pls. apply it. Being the person that has most experience with OR sound, you are for sure the first candidate as upgrader (which of course does not mean that Dennis' solution is not good).

[dennisat]

I'm back from my skiing holiday in one piece and attached is the patch which restores thread safety to the sound process.

 Position_Velocity.zip (1.12K)
Number of downloads: 154

This is largely a reversion to the code that was in use up to X1848. As Peter feared, the sound performance is not good; he won't be happy. I hadn't noticed a problem with the X1848 code because of my other patches. I'd modified soundprocess.cs to update every sound source every cycle instead of the one update in four cycles that applied to most. In addition, if the sound response time started to exceed 50ms, I dynamically reduced the sound thread sleeptime. The one in four cycle with a 50ms sleeptime means that most sound sources only get updated every 200ms at best which is probably why considerable stuttering could sometimes be heard when the sound load was heavy. I probably get away with my patches because I have a four core processor so loading up the sound thread doesn't affect other threads too much. A two core processor might perform badly using my patches.

A way to improve the performance with the sound process doing the position/velocity update might be to have a flag similar to the NeedsFrequentUpdate flag that already exists. This new flag would indicate that only the position/velocity update method was to be executed in a sound process update cycle. I haven't had the time to experiment with this.

I pointed out that the move of the position/velocity update from the sound process to the update process was not thread safe here. I think it must only be coming to light now because of a patch that James applied because of an observation I made here. This patch has so vastly improved the clean up of sound sources that now I can run MEP activities without the LAA program. Because some sound source clean up was a bit tardy or non-existant prior to this patch, and the sound process updates are so infrequent, I imagine that the position/velocity update placed in the update process stood very little chance of throwing an exception. The potential was always there, though.

I apologise for not having a ready made elegant solution for this problem but until I tested my stripped down patch I hadn't realised quite how bad the sound stuttering could be prior to X1849. With all my personal patches applied, I get a pretty good quality of sound, but this doesn't help anyone else.

Dennis