flibitijibibo / FNA-MGHistory

FNA - Accuracy-focused XNA4 reimplementation for open platforms
http://fna-xna.github.io/
246 stars 37 forks source link

Access Violation Exception when using MediaPlayer.Play(aSong) #290

Closed zolrath closed 9 years ago

zolrath commented 9 years ago

While trying to add music to my game I found that it played about 5 seconds of the song before abruptly cutting off and spitting out an Access Violation Exception.
If MediaPlayer.IsRepeating = true the application crashes due to a Stack Overflow Exception. .
I've made a simple as possible test case which results in the same issue:

protected override void LoadContent()
 {
        Song test = Content.Load<Song>("test");
        MediaPlayer.Play(test);
 }

This is using an ogg vorbis sample from Wikipedia set to Copy if Newer and no Build Action in the Content Pipeline and using the FNA lib pack.

An unhandled exception of type 'System.AccessViolationException' occurred in FNA.dll
Additional information: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.

FNA.dll!Microsoft.Xna.Framework.Media.Song.QueueBuffer(object sender, System.EventArgs args)
FNA.dll!Microsoft.Xna.Framework.Audio.DynamicSoundEffectInstance.Update()
FNA.dll!Microsoft.Xna.Framework.Media.Song.SongThread()
mscorlib.dll!System.Threading.ThreadHelper.ThreadStart_Context(object state)
mscorlib.dll!System.Threading.ExecutionContext.RunInternal(System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, object state, bool preserveSyncCtx)
mscorlib.dll!System.Threading.ExecutionContext.Run(System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, object state, bool preserveSyncCtx)
mscorlib.dll!System.Threading.ExecutionContext.Run(System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, object state)
mscorlib.dll!System.Threading.ThreadHelper.ThreadStart()

And just for good measure if IsRepeating = true

System.StackOverflowException was unhandled
Message: An unhandled exception of type 'System.StackOverflowException' occurred in FNA.dll

FNA.dll!Microsoft.Xna.Framework.Media.Song.QueueBuffer(object sender, System.EventArgs args)
FNA.dll!Microsoft.Xna.Framework.Media.Song.Play()
FNA.dll!Microsoft.Xna.Framework.Media.MediaPlayer.PlaySong(Microsoft.Xna.Framework.Media.Song song)
FNA.dll!Microsoft.Xna.Framework.Media.MediaPlayer.NextSong(int direction)
FNA.dll!Microsoft.Xna.Framework.Media.MediaPlayer.MoveNext()
FNA.dll!Microsoft.Xna.Framework.Media.MediaPlayer.OnSongFinishedPlaying(object sender, system.EventArgs args)
FNA.dll!Microsoft.Xna.Framework.Media.Song.OnFinishedPlaying()
FNA.dll!Microsoft.Xna.Framework.Media.Song.QueueBuffer(object sender, System.EventArgs args)
FNA.dll!Microsoft.Xna.Framework.Media.Song.Play()
... repeat for all of time ..
The maximum number of stack frames supported by Visual Studio has been exceeded.

This doesn't seem like it should cause any issues but is there something different and special that must be done in FNA?

flibitijibibo commented 9 years ago

Here's a fix for the stack overflow, at least:

https://github.com/flibitijibibo/FNA/commit/d0a14fa56d84eec81dcf38cc988d3982388e0b80 https://github.com/flibitijibibo/FNA/commit/14245b4e18de356d84907bd20fec28356a0d9547

But this would only happen if the Vorbis file was totally empty...? Good to be robust I guess.

The native segfault's a bit too vague to fix though. Do we have information from a real debugger somewhere? For all that tells us it could just be fixed by using the NO_STREAM_THREAD option at the top of Song.

zolrath commented 9 years ago

Switched over to a Debug build of FNA.dll for a more specific trace, should have been using that to begin with, doh. Unfortunately the Vorbis file isn't empty, I used this sample clip from Wikipedia for ease of reproduction.

The audio cuts out about 4 seconds into the 22 second clip and there seems to be a 50% chance to either simply remain silent or to throw this error a few seconds later:

System.AccessViolationException was unhandled
  HResult=-2147467261
  Message=Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
  Source=FNA
  StackTrace:
       at Vorbisfile.ov_read(OggVorbis_File& vf, Byte[] buffer, Int32 length, Int32 bigendianp, Int32 word, Int32 sgned, Int32& current_section)
       at Microsoft.Xna.Framework.Media.Song.QueueBuffer(Object sender, EventArgs args) in C:\Users\zolrath\Documents\Code\ThirdParty\FNA\src\Media\Xiph\Song.cs:line 305
       at Microsoft.Xna.Framework.Audio.DynamicSoundEffectInstance.Update() in C:\Users\zolrath\Documents\Code\ThirdParty\FNA\src\Audio\DynamicSoundEffectInstance.cs:line 321
       at Microsoft.Xna.Framework.Media.Song.SongThread() in C:\Users\zolrath\Documents\Code\ThirdParty\FNA\src\Media\Xiph\Song.cs:line 356
       at System.Threading.ThreadHelper.ThreadStart_Context(Object state)
       at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
       at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
       at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state)
       at System.Threading.ThreadHelper.ThreadStart()
  InnerException: 

With MediaPlayer.IsRepeating = true:

System.InvalidOperationException was unhandled
  HResult=-2146233079
  Message=Queue empty.
  Source=System
  StackTrace:
       at System.ThrowHelper.ThrowInvalidOperationException(ExceptionResource resource)
       at System.Collections.Generic.Queue`1.Dequeue()
       at Microsoft.Xna.Framework.Audio.DynamicSoundEffectInstance.Update() in C:\Users\zolrath\Documents\Code\ThirdParty\FNA\src\Audio\DynamicSoundEffectInstance.cs:line 327
       at Microsoft.Xna.Framework.Media.Song.SongThread() in C:\Users\zolrath\Documents\Code\ThirdParty\FNA\src\Media\Xiph\Song.cs:line 356
       at System.Threading.ThreadHelper.ThreadStart_Context(Object state)
       at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
       at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
       at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state)
       at System.Threading.ThreadHelper.ThreadStart()
  InnerException: 
flibitijibibo commented 9 years ago

This should deal with BufferNeeded:

https://github.com/flibitijibibo/FNA/commit/d730a16b9872afe4612528f7f287071d7637b82a https://github.com/flibitijibibo/FNA/commit/56f1a21c7edc425b9f33570c2da9aa0fdee00142

As for the native segfault, we at least know that it's ov_read now... but why that is, I'm not sure. I would start poking things around that call to see what it's returning, what might be missing, or maybe there's an error code in Vorbisfile we need to look at...

zolrath commented 9 years ago

This may be less specifically about the Vorbisfile.ov_read method than it appeared. My patented "Keep running the same program over and over and see what happens" method spat out a new error pointing us to the AL10.alSourcef method in the Volume setter in SoundEffectInstance.cs when IsRepeating = true.

System.StackOverflowException was unhandled
  HResult=-2147023895
  Message=Exception of type 'System.StackOverflowException' was thrown.
  InnerException: 

if (INTERNAL_alSource != 0)
{
    AL10.alSourcef(
    INTERNAL_alSource,
    AL10.AL_GAIN,
    INTERNAL_volume * SoundEffect.MasterVolume
);

On another run the Stack Overflow occured in the Pan setter:

AL10.alSource3f(
    INTERNAL_alSource,
    AL10.AL_POSITION,
    INTERNAL_pan,
    0.0f,
    (float) Math.Sqrt(1 - Math.Pow(INTERNAL_pan, 2))
);

And another run gave us a Stack Overflow at the Vorbisfile.ov_read method. I'm using an audio file from vorbis.com to ensure that it's a well-formatted Vorbis file, just in case.

I've also tried a very small 9k vorbis file with similar results with IsRepeating = true. The sound file (1/8th of a second long) plays a single time followed by a few seconds of nothing before the Stack Overflow occurs.

flibitijibibo commented 9 years ago

I doubt we're getting stack overflows in AL Soft - for reference, here's where alSource3f will end up:

http://repo.or.cz/w/openal-soft.git/blob/HEAD:/OpenAL32/alSource.c#l386

These are calls on the main thread, and there is another thread being used for the Song stream... maybe this is just the debugger stopping wherever it was, even though it's not the crashing thread?

It may be worth the time to isolate the Vorbisfile# work into a separate program that just decodes the Vorbis data on another thread, using the same techniques we're using here (thread, prealloc'd byte array, the works). This is sounding more and more like a Win32-specific problem, which I have the least experience with.

flibitijibibo commented 9 years ago

Something else to consider, and I know C# loves doing this, is that the struct layouts for the Vorbisfile interop are wrong. This file is in lib/Vorbisfile-CS/:

https://github.com/flibitijibibo/Vorbisfile-CS/blob/master/Vorbisfile.cs

Previously I've had to do incredibly dumb things like making 32-bit versions of structs:

https://github.com/flibitijibibo/TheoraPlay-CS/blob/master/TheoraPlay.cs#L44

So maybe we need it here too. Try setting Pack = 1 for all the structs and see what happens.

flibitijibibo commented 9 years ago

I looked into this a little bit. This is the result:

https://github.com/flibitijibibo/Vorbisfile-CS/commit/9312aedd5673150312b6b86326e8be867fc455e8 https://github.com/flibitijibibo/FNA/commit/6881b67f17ae268cb2f6c51906e61772ca96d0e5

This is basically the same problem we had with TheoraPlay, but with way too many structures and varying alignments to just do the stupid thing we did there. So instead, we're doing a different stupid thing to get around it.

Instead of trying to recreate the structure, we're just going to alloc the size of the struct as it would have been in C/C++ (though you wouldn't have necessarily needed to malloc one like we have to now), then just send around this blob of bytes pretending it's the OggVorbis_File struct. Vorbisfile won't know, and C# clearly doesn't care. The sizes were determined by me printf'ing sizeof(OggVorbis_File) and marking the result down; Win64 is the only one missing since I didn't have it in me to wait for another Microsoft SDK to download/install.

In theory you could have fixed this just for 32-bit Unix by setting Pack = 1 for OggVorbis_File, vorbis_dsp_state, and vorbis_block, for example, but that of course messes up 64-bit since managed languages apparently can't let you pick the pack alignment based on architecture (which is determinable at runtime like every other platform attribute, including sizeof(void*)), nor is it willing to admit that it might be messing with your struct data in between the C#/C versions when using P/Invoke, even when the struct sizes are the same (we'll ignore for a minute that the language is supposed to be reproducing the structs given the element sizes as it would have in C anyway; that's why we have all these ridiculous keywords/attributes from InteropServices, right?).

Let me know if this doesn't take care of things. Now that we have the right block size and nothing's trying to realign things we should be all set, but maybe there's a detail I missed.

Either way, everyone in charge of C# runtimes is fired.

zolrath commented 9 years ago

Oh man! Wrapping these libraries is more complicated than I would have thought. I'm running Windows 8.1 x64, though this is being compiled for x86. Ugh, I hate to say it but unfortunately it still plays for a few seconds until we're still blasted in the face with a good ol':

System.AccessViolationException was unhandled
  HResult=-2147467261
  Message=Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
  Source=FNA
  StackTrace:
       at Vorbisfile.ov_read(Byte[] vf, Byte[] buffer, Int32 length, Int32 bigendianp, Int32 word, Int32 sgned, Int32& current_section)
       at Microsoft.Xna.Framework.Media.Song.QueueBuffer(Object sender, EventArgs args) in C:\Users\zolrath\Documents\Code\ThirdParty\FNA\src\Media\Xiph\Song.cs:line 305
       at Microsoft.Xna.Framework.Audio.DynamicSoundEffectInstance.Update() in C:\Users\zolrath\Documents\Code\ThirdParty\FNA\src\Audio\DynamicSoundEffectInstance.cs:line 321
       at Microsoft.Xna.Framework.Media.Song.SongThread() in C:\Users\zolrath\Documents\Code\ThirdParty\FNA\src\Media\Xiph\Song.cs:line 356
       at System.Threading.ThreadHelper.ThreadStart_Context(Object state)
       at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
       at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
       at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state)
       at System.Threading.ThreadHelper.ThreadStart()
  InnerException: 

Thanks for your tireless hammering, hopefully the problem will give up and retire to Florida soon.

flibitijibibo commented 9 years ago

Hm, I would verify the sizeof(OggVorbis_File) on Win32 if you happen to have a C compiler handy. Unfortunately without any real information from the debugger there's not much else I can do here, other than assume that the Vorbisfile handle is being mangled; the only other place that should be written to is buffer, and I don't think it reads from anywhere other than vf.

Also, see if disabling the thread helps (see top of Song.cs).

EDIT: Also, are you sure there's no error code from ov_read? It should be producing something like this; if it was just returning 0 we would be exiting without any problems.

zolrath commented 9 years ago

Uncommenting #define NO_STREAM_THREAD still crashes but it makes the error consistently Access Violation Exception instead of sometimes blowing the stack instead.

flibitijibibo commented 9 years ago

If you have a repro case somewhere, send it my way. I've cranked up the motor on my Windows drive so I should be able to look at it locally now.

zolrath commented 9 years ago

Instead of making a throw away repo I threw up a fairly isolated test case onto Dropbox for you!

flibitijibibo commented 9 years ago

Alright, I was able to reproduce it and I found a fix. Are you ready?

https://github.com/flibitijibibo/Vorbisfile-CS/commit/a08ca774a11def830fa6038a9bbf14cbdce62ded https://github.com/flibitijibibo/FNA/commit/f9871c4989ee8ec0c6d49e598b4b959d761d0a56

Yup. The problem is fixed by using malloc'd data rather than data allocated by C#'s new operator. Why does C# new not work? More like why is this glass of vodka on my desk not empty right now!

r2d2rigo commented 9 years ago

@flibitijibibo This is an unverified explanation (sorry, no time/tools to test it right now!) that comes from my limited experience of interop with native code: the crash is due to new reserving memory managed by the .NET runtime, and malloc returning a blob of unmanaged memory provided by the OS.

Due to .NET's memory management, I'm pretty sure there is a moment that due to the garbage collector compacting the used memory, the pointer data is moved to a new location. This somehow interferes with the synchronization with the native library, and the AccessViolationException is thrown.

Alternatively, read about GCHandle, go back to the managed memory implementation and try pinning the memory (GCHandle.Alloc(vf, GCHandleType.Pinned);) during native calls that use it (if this doesn't work just retain the GCHandle for the entire life of the object, although this would negatively impact performance due to the existence of memory blocks that can't be moved around by the compactor).

flibitijibibo commented 9 years ago

@r2d2rigo It's definitely an odd case, because as far as I know Vorbisfile doesn't care where the OggVorbis_File is located, as long as the data inside it is the same. Given how the timing is only slightly random (though the crash itself was definitely hit-and-miss) I'd be willing to buy that explanation though.

We do use GCHandles in FNA for texture/buffer data, but for this it seemed easier to just malloc/free, but maybe that's me as a C programmer taking priority.

AxiomVerge commented 9 years ago

Hey there ... I'm trying to port Axiom Verge to FNA and I ran into that same stack overflow issue in Song.QueueBuffer. Thing is, though I see you've fixed it in your post in this thread, I don't have the fix, even though cloned FNA for the first time today. I tried using git pull and git submodule update, but it says I am up to date. I'm not experienced with git. Is there a preferred way for me to get this fix?

(Also, while I'm at it ... the songs seem to be extremely quiet compared to their xna counterparts...just thought I'd mention)

EDIT: Nevermind, I DO have your fix, I was just reading the diffs backwards. Anyway, you still get a stack overflow when a repeating song ends. It just seems to repeatedly go into MediaPlayer.OnSongFinishedPlaying() until it crashes.

flibitijibibo commented 9 years ago

Just took care of the looping issue:

https://github.com/flibitijibibo/FNA/commit/12c7d63d923b830227d63ef50c3cd7b7b0a10d30

AxiomVerge commented 9 years ago

It appears that looping is now fixed, but now stopping a song and playing a new song causes it to hang or crash most of the time.

When it crashes, it's an AccessViolationException (read or write protected memory) on Song.cs line 291, in a worker thread:

            Vorbisfile.ov_time_seek(vorbisFile, 0.0);

Here's the call stack:

>   FNA.dll!Microsoft.Xna.Framework.Media.Song.Stop() Line 291  C#
    FNA.dll!Microsoft.Xna.Framework.Media.MediaPlayer.Stop() Line 242   C#
    FNA.dll!Microsoft.Xna.Framework.Media.MediaPlayer.NextSong(int direction) Line 282  C#
    FNA.dll!Microsoft.Xna.Framework.Media.MediaPlayer.MoveNext() Line 165   C#
    FNA.dll!Microsoft.Xna.Framework.Media.MediaPlayer.OnSongFinishedPlaying(object sender, System.EventArgs args) Line 273  C#
    FNA.dll!Microsoft.Xna.Framework.Media.Song.SongThread() Line 364    C#
    [External Code] 

Looking at my threads, I see that there's another thread doing this:

    [External Code] 
>   FNA.dll!Microsoft.Xna.Framework.Media.Song.QueueBuffer(object sender, System.EventArgs args) Line 307   C#
    FNA.dll!Microsoft.Xna.Framework.Media.Song.Play() Line 251  C#
    FNA.dll!Microsoft.Xna.Framework.Media.MediaPlayer.PlaySong(Microsoft.Xna.Framework.Media.Song song) Line 311    C#
    FNA.dll!Microsoft.Xna.Framework.Media.MediaPlayer.NextSong(int direction) Line 299  C#
    FNA.dll!Microsoft.Xna.Framework.Media.MediaPlayer.MoveNext() Line 165   C#
    FNA.dll!Microsoft.Xna.Framework.Media.MediaPlayer.OnSongFinishedPlaying(object sender, System.EventArgs args) Line 273  C#
    FNA.dll!Microsoft.Xna.Framework.Media.Song.SongThread() Line 364    C#
    [External Code] 

On the times that it hangs, it's also on line 291, but when I break takes me to the the main thread:

    [External Code] 
>   FNA.dll!Microsoft.Xna.Framework.Media.Song.Stop() Line 291  C#
    FNA.dll!Microsoft.Xna.Framework.Media.MediaPlayer.Stop() Line 242   C#
    AxiomVerge.exe!OuterBeyond.THAudio.PlaySong(string songName, bool repeat, bool beatBlend) Line 575  C#

--And I can see a worker thread with this call stack:

    [External Code] 
>   FNA.dll!Microsoft.Xna.Framework.Media.Song.Stop() Line 291  C#
    FNA.dll!Microsoft.Xna.Framework.Media.MediaPlayer.Stop() Line 242   C#
    FNA.dll!Microsoft.Xna.Framework.Media.MediaPlayer.NextSong(int direction) Line 282  C#
    FNA.dll!Microsoft.Xna.Framework.Media.MediaPlayer.MoveNext() Line 165   C#
    FNA.dll!Microsoft.Xna.Framework.Media.MediaPlayer.OnSongFinishedPlaying(object sender, System.EventArgs args) Line 273  C#
    FNA.dll!Microsoft.Xna.Framework.Media.Song.SongThread() Line 364    C#
    [External Code] 

So I'm thinking it's something like MediaPlayer.Stop() causes a thread to do OnSongFinishedPlaying(), but then MediaPlayer.Play() has some kind of contention going on where it also tries to stop the song and there is deadlock.

One time I had it work, when I went from the title screen into gameplay, but when I exited gameplay, the title music resumed playing where it left off (normally it starts over), but after a second or two it threw an exception on line 325 of DynamicSoundEffectInstance:

// Error check our queuedBuffers list.
            for (int i = 0; i < finishedBuffers; i += 1)
            {
                uint newBuf = queuedBuffers.Dequeue();
                if (newBuf != bufs[i])
                {
                    throw new Exception("Buffer desync!");
                }
                availableBuffers.Enqueue(newBuf);
            }

Here's the stack:

>   FNA.dll!Microsoft.Xna.Framework.Audio.DynamicSoundEffectInstance.Update() Line 325  C#
    FNA.dll!Microsoft.Xna.Framework.Media.Song.SongThread() Line 357    C#
    [External Code] 
flibitijibibo commented 9 years ago

So let me be sure of what the actual code is here... are you doing something like this?

MediaPlayer.Stop();
MediaPlayer.Play(someSong);

I don't think ActiveSongChanged even gets poked when you call Stop outright anyway, so we'll at least knock out that call:

https://github.com/flibitijibibo/FNA/commit/5a7f6812674bcc0c4a54890d7be0710dff517243 https://github.com/flibitijibibo/FNA/commit/7778491220cab37e45ba51bf2188cda4c6093e3a

That alone might actually knock out all of the contention. The buffer desync probably happens as a result of something halting the DSEI while the update is happening, so the lists go out of sync. Might be something that can be fixed with a lock, but I'm more concerned with why the desync is happening at all.

AxiomVerge commented 9 years ago

Okay, I tried this a bunch of different scenarios and it appears to work! Thanks!

flibitijibibo commented 9 years ago

Excellent, glad it finally stopped crashing everything...

To prevent a possible deadlock I slightly modified the thread routine to not assign exitThread, just in case the Update return value stomped on Stop's request to exit:

https://github.com/flibitijibibo/FNA/commit/dcca7b1f98701bd369d3edf4a59b7f2d6ba48153

I'd certainly feel silly if this broke what finally works, but you never know!

flibitijibibo commented 9 years ago

Going to close this since it sounds like we've got every case under the sun working, and on Windows (and for my titles, modified to get similar-ish cases). @zolrath, if this is still borked, feel free to shout mean things at me for closing early.