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

[gpz]

I also hear the wrong doppler effect for electric locomotives motor sound at lower speeds too. Your described solution is the same as James' 1st. The problem with Dennis' patch is that it is incomplete. The balancing of updates was removed, just because it is easier to code, and that computer is fast enough, which is fine for a single person, but not a general solution. Probably the faster update time for positions could be achieved by James' 2nd solution.

[dennisat]

Something you might like to try, that I have implemented, is to mark mobile sound sources as "NeedsFrequentUpdate" when they exceed 15m/sec. I think this improves things a lot, certainly on a 4 core processor.

Dennis

[gpz]

The drawbacks of this is, that
1. Even NeedsFrequentUpdate-flagged sound sources' positions are updated too slowly for a fast moving sound,
2. A full update on sounds sources is more than just the needed simple position update. This is just for different purposes.

[Csantucci]

This is what I have prepared.

This is the patch:

There is a hypothesis under this patch: the crashes were due because some SoundSourceID's were deleted by the sound process while still being accessed by the updater process. Only uploading and having people running this will show if this is the only case that can cause crashes.

and these are the two runactivities:

I have put them here to allow checking if the sound quality has remained substantially the same.

After (hopefully) getting comments on patch and runactivities I will decide if the patch is worth uploading.

Files removed because new release available.

[dennisat]

Isn't there likely to be an exposure around the new flag "updaterWorking"?

Let's say that the Sound process has reached the position where it checks "updaterWorking". Let's say "updaterWorking" is set false. The loop to update the sound streams then starts. At this point, the Updater process could reach the local copy of "SoundSourceIDs" and then set flag "updaterWorking" to true. The Updater process then starts trying to update "SoundSourceIDs", via its local copy, while the Sound process is still likely to be removing some of them.

If I read this right, "updaterWorking" should be protected by Interlocked Compare/Exchange.

Dennis

Edit:
I think there is a bigger potential problem here. It is highly unlikely that the Sound source being processed by the Sound process actually relates to the Car being processed by the Updater.

[gpz]

Just a note about what I read on the net: by OpenAL spec, OpenAL is thread-safe, so it must be safe to send commands to it from different threads.
Is it possible, that C# invoke itself is not thread-safe?

[Csantucci]

Dennis,
to avoid your first objection I have added in TrainCar.cs the test if the soundsource is still valid or not

if (OpenAL.alIsSource(soundSourceID))

just before working on it.
As it is now, I see only a possible uncovered case:
-sound process starts, finds the updater process flag free, and is interrupted before removing a sound source from SoundSourceIDs
-updater enters, works and is interrupted within the above if block
-sound process enters, and removes sound source from SoundSourceIDs and clears the sound source
-updater process enters and works now on a not existing sound source.
I think that the combination of these events is very unlikely.
I first had on the above "if" also a test if the sound process was in the critical section; this covered the above case, but caused short sound interruptions.
I am studying if and how to overcome the uncovered case.
A possibility is to split the work within the sound process in two: on an update SoundSourceIDs is updated removing the dead entries, and on a subsequent one the sound sources are cleared; in both updates a check is first done on the updater process flag.

I didn't understand your second objection.

[dennisat]

Csantucci, on 08 February 2015 - 02:46 AM, said:

I didn't understand your second objection.

The update of sound source IDs in TrainCar.cs is working on the sounds for a particular car. The update of sound sources in the sound process is looping through all sound sources, at its own rate, with (normally) no synchronisation to any other processing of the owner of the source. Sound Trigger synchronisation is handled by an Events system and source Add / Remove by locks (which James requires to be replaced as soon as a reliable procedure is devised). It is therefore highly unlikely that the car sounds being processed at any one time by TrainCar.cs will coincide with those being processed in the sound process.

Dennis

[gpz]

I don't think, that possibly passing a non-existent (destroyed) sound source ID integer number to OpenAL would lead to a crash. It would mean OpenAL was very poorly coded, which is highly unlikely. So just by checking if these IDs are in sync, this problem would not be solved. I assume it is absolutely safe to pass a false ID to OpenAL.

[dennisat]

I don't think I've seen a crash actually in OpenAl. What we are crashing on are sound related lists that are being modified by one thread while another is trying to read / modify it at the same time. I'm sure all the crashes have been of the "Collection modified while being processed in a loop" type. This is a problem with our thread synchronisation, not OpenAl

Dennis