[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: 196

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

[gpz]

dennisat, on 26 January 2015 - 08:18 AM, said:

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.

This part is not really clear to me, why you insist on removing the NeedsFrequentUpdate flag. Those sound sources not marked with this really not needed to be updated more frequently. (It shouldn't lead to stuttering, because the sound process is not the sound renderer process, it is just for "preparation".) Of course, apart from position update of the moving sources and moving camera. Those properties need to be updated without any thread sleep. Not even with reduced thread sleep, but without any. Unfortunately I don't have too much experience with thread programming, so I don't know what the real solution would be...

[James Ross]

gpz, on 26 January 2015 - 09:00 AM, said:

Unfortunately I don't have too much experience with thread programming, so I don't know what the real solution would be...

So, I think there are two kinds of solution here:

• We could adjust the code so that changes to sound source lists are updated in a thread-safe way, which will be more complex for removal but still possible, and continue setting the position/velocity in Updater process and leaving the other updates to the Sound process.
  
• We could have the Updater process save the minimum data needed to a shared location to update the position/velocity in the Sound process, which will need updating to do this constantly while also performing its other sound duties.

[dennisat]

James Ross, on 26 January 2015 - 01:19 PM, said:

[*]We could have the Updater process save the minimum data needed to a shared location to update the position/velocity in the Sound process, which will need updating to do this constantly while also performing its other sound duties.
[/list]

In the thread safe version this data is already available and used to update the sound position and velocity. At the moment, however, the sound process only cycles every 50ms and this does not update every sound source. All sound sources are updated approximately every 200ms. The sound/position update code is very simple and on the surface it appears it could be formed into a method which is cycled through at a much faster rate while maintaining the slow cycle for the major updates.

My personal mods adopt the brute force technique of updating everything every 50ms but this is expensive on resources and is probably not a good solution for lower powered systems.

Dennis

[gpz]

James Ross, on 26 January 2015 - 01:19 PM, said:

So, I think there are two kinds of solution here:

• We could adjust the code so that changes to sound source lists are updated in a thread-safe way, which will be more complex for removal but still possible, and continue setting the position/velocity in Updater process and leaving the other updates to the Sound process.
  
• We could have the Updater process save the minimum data needed to a shared location to update the position/velocity in the Sound process, which will need updating to do this constantly while also performing its other sound duties.

I think any of these methods would be sufficient. It is no problem updating the positions/velocities from the Sound process, provided it is done fast enough.

[Csantucci]

Today I at last had time to have a comparison check (with earphones) of the actual OR version and of one with Dennis' patch applied.
I found one difference: if you are in view 4 and a train at 150 Km/h passes by with the horn pressed, you have a nice Doppler effect with smoothly descending sound frequency with the actual OR version, while in Dennis' version you hear a stepwise sound frequency reduction: not a tragic problem, but a problem, that MSTS does not have. This probably arises from the fact that the position and speed (in this case I think position is what counts) calculation is done max 20 times per second in case of Dennis' version, while it is done FPS times per second in the actual OR version.
I am wondering if there is a more satisfying solution (suggestions possibly not requiring a master degree in computer science are welcome). I thought about something that is not a lock, but a skip mechanism: here we have sound process and updater process concurring; when neeeded each of them declares itself as needing to use the common resource; after that it tests if the other process is already using the common resource; if not it performs the sound calculations, if yes it skips the sound calculations (it will perform them at next update); in both cases it frees then the common resource. I will have some test.