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

Sound support is still awful #184

Open
pvcraven opened this Issue Mar 19, 2018 · 15 comments

Comments

Projects
None yet
4 participants
@pvcraven
Owner

pvcraven commented Mar 19, 2018

Bug Report

Most recent version of Arcade doesn't use AvBin. (yay). But using "mciSendStringA" only supports wav, mp3, and a max of 8 character file names.

pvcraven added a commit that referenced this issue Apr 1, 2018

Issue #184, get sound hopefully working on the Mac.
Related to #180, using code from dlanghorne0428.
@dlanghorne0428

This comment has been minimized.

Contributor

dlanghorne0428 commented Apr 4, 2018

Hi Prof. Craven:

I confirmed your recent commit works correctly on my Mac. Splitting the code into loadsound() and playsound() eliminates the delay on playing the first sound.

pvcraven pushed a commit that referenced this issue Apr 5, 2018

pvcraven added a commit that referenced this issue Apr 8, 2018

pvcraven added a commit that referenced this issue May 5, 2018

More work on sound support. #184.
Loading avbin again. Can now play ogg, mp3, and wav on windows.

pvcraven added a commit that referenced this issue May 5, 2018

@dangillet

This comment has been minimized.

Collaborator

dangillet commented Jun 15, 2018

Hello,

I've made some work on pyglet regarding FFmpeg usage. I tried to address the problem you mentioned on the Google Group regarding the binary filename.

Here is my pyglet fork: https://bitbucket.org/dangillet/pyglet

You would need to check-out the ffmpeg4 branch for our current work. This also means you need to use the FFmpeg 4 binaries for testing.

The binary filenames are hard-coded because we can only support one major version of FFmpeg at a time. When FFmpeg 5 will come out, if someone volunteers to review the ctypes wrapping, Pyglet would then support FFmpeg 5, but not FFmpeg 4 anymore. This also means Pyglet version would be bumped up by 1 minor version.

I don't have access to a Mac. So I would appreciate if you could test it out for me and let me know if it works or not.

Obviously this branch will finally be merged back into Pyglet when we're done.

@pauleveritt

This comment has been minimized.

Collaborator

pauleveritt commented Jun 18, 2018

Sorry for the delay in looking at this. I got your pyglet branch and used it with a master checkout of Arcade. I'm on High Sierra (latest) and installed ffmpeg4 from Homebrew with:

$ brew install ffmpeg --with-libass --with-fontconfig

Using the Python from my virtualenv, here is the output when doing setup.py develop:

$ python3 ./setup.py develop
running develop
running egg_info
creating pyglet.egg-info
writing pyglet.egg-info/PKG-INFO
writing dependency_links to pyglet.egg-info/dependency_links.txt
writing top-level names to pyglet.egg-info/top_level.txt
writing manifest file 'pyglet.egg-info/SOURCES.txt'
reading manifest file 'pyglet.egg-info/SOURCES.txt'
reading manifest template 'MANIFEST.in'
warning: no files found matching 'CHANGELOG'
no previously-included directories found matching 'examples/**/dist'
warning: no previously-included files matching '*.png' found under directory 'tests/regression/images'
warning: no previously-included files matching '*.log' found under directory 'tests'
no previously-included directories found matching '**/.svn'
warning: no previously-included files matching '*.pyc' found under directory '*'
warning: no previously-included files matching '*.pyo' found under directory '*'
writing manifest file 'pyglet.egg-info/SOURCES.txt'
running build_ext
Creating /Users/pauleveritt/projects/pauleveritt/arcade/arcade_sound/env3/lib/python3.6/site-packages/pyglet.egg-link (link to .)
Adding pyglet 1.4.0a1 to easy-install.pth file

Installed /Users/pauleveritt/projects/pauleveritt/arcade/pyglet
Processing dependencies for pyglet==1.4.0a1
Finished processing dependencies for pyglet==1.4.0a1

...which doesn't really indicate where it got the ffmpeg4 binaries from.

The asteroid smasher game played the audio but it isn't clear to me that it's using what you'd like used.

@dangillet

This comment has been minimized.

Collaborator

dangillet commented Jun 18, 2018

Thanks for taking the time to try this out. I realise I should have done more work to have something you can properly test. The idea is to use pyglet media library and have the binary files available within arcade.

I've forked the repo and made a crude working example - on Windows at least. You can find my fork here : https://github.com/dangillet/arcade. From there I've created a ffmpeg branch. In the example folder, I've changed the sound.py file. It's a small demo where you can press any key and it plays in sequence the sound files found in the sounds sub-folder.

I've also left a print statement showing if FFmpeg binaries can be found or not. On my machine it says True. I would be interested to know if it works on Mac OS too.

This is really just a proof of concept. It's nothing like polished and will probably crash in flames. 🔥 But it's probably a good start to see if this is the right way to go.

@pauleveritt

This comment has been minimized.

Collaborator

pauleveritt commented Jun 18, 2018

NULL pointer:

/Users/pauleveritt/projects/pauleveritt/arcade/arcade_dangillet/env3/bin/python /Users/pauleveritt/projects/pauleveritt/arcade/arcade_dangillet/arcade/examples/sound.py
Have FFmpeg True
Traceback (most recent call last):
  File "/Users/pauleveritt/projects/pauleveritt/arcade/arcade_dangillet/arcade/examples/sound.py", line 26, in <module>
    window = Window()
  File "/Users/pauleveritt/projects/pauleveritt/arcade/arcade_dangillet/arcade/examples/sound.py", line 15, in __init__
    sound = arcade.load_sound('sounds/' + file)
  File "/Users/pauleveritt/projects/pauleveritt/arcade/arcade_dangillet/arcade/sound.py", line 77, in _load_sound_win
    s = pyglet.media.load(sound, streaming=False)
  File "/Users/pauleveritt/projects/pauleveritt/arcade/pyglet/pyglet/media/__init__.py", line 132, in load
    loaded_source = decoder.decode(file, filename, streaming)
  File "/Users/pauleveritt/projects/pauleveritt/arcade/pyglet/pyglet/media/codecs/ffmpeg.py", line 1040, in decode
    return StaticSource(FFmpegSource(filename, file))
  File "/Users/pauleveritt/projects/pauleveritt/arcade/pyglet/pyglet/media/codecs/ffmpeg.py", line 559, in __init__
    self.start_time = self._get_start_time()
  File "/Users/pauleveritt/projects/pauleveritt/arcade/pyglet/pyglet/media/codecs/ffmpeg.py", line 1008, in _get_start_time
    return max(start_times(streams()))
  File "/Users/pauleveritt/projects/pauleveritt/arcade/pyglet/pyglet/media/codecs/ffmpeg.py", line 998, in start_times
    for stream in streams:
  File "/Users/pauleveritt/projects/pauleveritt/arcade/pyglet/pyglet/media/codecs/ffmpeg.py", line 993, in streams
    stream = format_context.contents.streams[idx].contents
ValueError: NULL pointer access
@pauleveritt

This comment has been minimized.

Collaborator

pauleveritt commented Jun 18, 2018

On the bright side:

Have FFmpeg True

:)

@dangillet

This comment has been minimized.

Collaborator

dangillet commented Jun 19, 2018

Thanks for the test! I'm amazed this worked on my computer and not on yours. I found a stupid bug in pyglet and I've just pushed a bug fix on my local repo. If you get the chance to update pyglet and let me know how it goes. 🙇

@pauleveritt

This comment has been minimized.

Collaborator

pauleveritt commented Jun 19, 2018

@dangillet

This comment has been minimized.

Collaborator

dangillet commented Jun 19, 2018

Awesome news!

The message is just an information from the codec. I had the same message. I'm not an expert in mp3 format and maybe I'm doing something wrong when decoding mp3 files. But it's working which is the good news!

I'm now looking at cleaning up the code and adding binaries for linux. Then tests seem like a good place to start.

I would say that we should only test arcade, stopping at the boundary with pyglet. This means I would mock both the pyglet.media.Player and the pyglet.media.Source (with its StaticSource and StreamingSource), using maybe spec_set to make sure only the right attributes are used. Pyglet functions like pyglet.media.load would be mocked to return a MockStreamingSource or MockStaticSource depending on the kwargs streaming.

@pauleveritt

This comment has been minimized.

Collaborator

pauleveritt commented Jun 19, 2018

I wonder if those informational messages can be routed through Python logging.

Sounds good on where to mock. If there are any platform-level differences on the Arcade side (e.g. how it sniffs for OS and does different things) then we should either arrange to replace sys.platform (and reset) in the tests, or refactor sound.py to make that surface area tiny. Or perhaps your part is independent of that part.

One last point...the tests folder has a file that plays sounds, for a human (not test runner) to just listen to it. It's not in tests/unit so it doesn't get run on Travis or locally. Makes for a good sanity check.

@dangillet

This comment has been minimized.

Collaborator

dangillet commented Jun 19, 2018

Those message can most probably be redirect to whatever we want. According to the documentation, they're sent to stderr. I tried to simply use contextlib.redirect_stderr but failed to make it work. Probably because FFmpeg does not write to stderr through the Python interpreter.

I've pushed some more commits on my fork. I've also added binaries for Win32 but I have no clue what should be done for Linux. If someone can help here, it would be appreciated.

Thanks for pointing me to the test_sound.py. I've changed it a bit so that each test waits for the sound to finish playing. It works on my machine. Also the test was initially failing because I was trying to launch the test from within the unit folder. So the sound files reference was not properly found. I've also added some code to make sure we refer to this folder relative to the test code, not to the current working directory.

I'll start to work on some mocks as described above. But before rushing head down into this, I'd rather have somebody else opinion if this is the desired direction, ie. using pyglet ffmpeg.

@pauleveritt

This comment has been minimized.

Collaborator

pauleveritt commented Jun 19, 2018

Re: "somebody else's opinion", I wonder if we need to have a real-time discussion e.g. Slack or Discourse. Arcade is starting to get some more folks involved. And sound has been such a bandaid-on-bandaid for us, it would be nice to load all the issues in our head, clear time, and get it in a good spot.

@pauleveritt

This comment has been minimized.

Collaborator

pauleveritt commented Jun 19, 2018

@dangillet Is your importlib.resource suggestion on the critical path? If so, perhaps you and I can start a ticket for just that part, I give you access to the repo, we work on a branch.

@dangillet

This comment has been minimized.

Collaborator

dangillet commented Jun 19, 2018

Regarding real-time discussion, I think it's a good idea. It's true that there are probably many moving parts and it's not easy for a single person to have the overview - or at least that's how I feel.

Now Pyglet media support is obviously the root cause of most arcade sound problems. I volunteered to write some code for Pyglet which wraps FFmpeg as it seems to be more maintained than AVlib. The code might still be buggy but I have the feeling it's heading in a better direction than before. The nagging bug I can't solve at the moment is the PusleAudio driver. As I said, I'm not a Linux expert. I've tried to tackle down the problem but could not find it. Now the alternative is to use OpenAL for MacOS and Linux and DirectSound for Windows.

importlib.resource is what I would use to locate all needed resources for the application (the game). Regarding FFmpeg binaries, they're loaded automatically by Pyglet at import time. This is maybe a bad thing in case our binaries end up in a zip file due to some freezing application (PyInstaller). As a work-around, I could imagine to first use importlib.resource to get an actual file path to the binaries and then set the required environment variable to that temp directory before importing pyglet.media.

Anyway all this needs indeed to be discussed.

@pvcraven

This comment has been minimized.

Owner

pvcraven commented Jun 19, 2018

Discord - try this. Should expire in 24 hrs, so ping me or e-mail if you need another invite: https://discord.gg/UctfSf

@pvcraven pvcraven added this to the ffmpeg milestone Jun 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment