Skip to content
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 APIs on Windows #4522

Closed
wants to merge 16 commits into from

Conversation

Projects
None yet
4 participants
@mattn
Copy link

commented Jun 10, 2019

This is work in progress. Current status:

sound_playfile: done
sound_stop: done
sound_clear: done

mattn added some commits Jun 10, 2019

Show resolved Hide resolved src/sound.c Outdated
Show resolved Hide resolved src/Make_cyg_ming.mak Outdated
Show resolved Hide resolved src/sound.c Outdated
@k-takata

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

@mattn I will create a patch for Make_mvc.mak later.

Show resolved Hide resolved src/sound.c Outdated
Show resolved Hide resolved src/sound.c
Show resolved Hide resolved src/sound.c Outdated
Show resolved Hide resolved src/sound.c Outdated
@k-takata

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

@mattn

This comment has been minimized.

Copy link
Author

commented Jun 11, 2019

@k-takata Will do tonight. Thank you.

k-takata and others added some commits Jun 11, 2019

@mattn

This comment has been minimized.

Copy link
Author

commented Jun 11, 2019

I applied review fixes. I noticed that implementing sound_playevent() is bits hard. The reasons are:

  • mciSendString does not allow alias name like "SystemAlert".
  • PlaySound() that can allow filename or "SystemAlert" does not work with callback.
  • PlaySound() can not stop sound indivisually.
  • "SystemAlert" is embeded in "mcires.dll".
  • Windows does not have XDG sound list.
@brammool

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

@mattn

This comment has been minimized.

Copy link
Author

commented Jun 11, 2019

Does MS-Windows install some of these sounds, so that they can be played
as sound files? Then we could map some event names to them. I rather
not include sound files in the distribution, except some very basic ones
like "bell". I assume plugins can include files in a way they play on
any system with the +sound feature. .wav files should work, libcanberra
also handles .ogg and other formats.

Older version of Windows install them. But Windows 10 does not install them. All of sound resource are bundled in mcires.dll as resource file. PlaySound() API can play the resource sound. But PlaySound() is not used with callback as I mentioned in above.

@brammool

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

@k-takata

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Is that doable?

I think it is possible, but the problem is that PlaySound() cannot stop the sounds individually.
It can stop all the sounds only. Another way is playing only one sound at once. If there is a new call of PlaySound(), stop the old sound.

Show resolved Hide resolved src/sound.c Outdated
Show resolved Hide resolved src/sound.c Outdated
Show resolved Hide resolved src/sound.c Outdated
Show resolved Hide resolved src/sound.c Outdated
Show resolved Hide resolved src/sound.c Outdated
Show resolved Hide resolved src/sound.c Outdated
@codecov-io

This comment has been minimized.

Copy link

commented Jun 12, 2019

Codecov Report

Merging #4522 into master will increase coverage by 0.17%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4522      +/-   ##
==========================================
+ Coverage   80.65%   80.82%   +0.17%     
==========================================
  Files         111      111              
  Lines      143838   144338     +500     
==========================================
+ Hits       116011   116662     +651     
+ Misses      27827    27676     -151
Impacted Files Coverage Δ
src/sound.c 64.38% <ø> (ø) ⬆️
src/if_xcmdsrv.c 83.66% <0%> (-0.9%) ⬇️
src/gui.c 58.05% <0%> (-0.52%) ⬇️
src/screen.c 81.95% <0%> (-0.41%) ⬇️
src/dict.c 85.58% <0%> (-0.4%) ⬇️
src/netbeans.c 27.25% <0%> (-0.3%) ⬇️
src/gui_gtk_x11.c 49.16% <0%> (-0.25%) ⬇️
src/ex_cmds.c 82.18% <0%> (-0.25%) ⬇️
src/os_unix.c 60.63% <0%> (-0.05%) ⬇️
src/evalfunc.c 89.27% <0%> (-0.05%) ⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12ee7ff...825abec. Read the comment docs.

mattn added some commits Jun 12, 2019

@mattn

This comment has been minimized.

Copy link
Author

commented Jun 12, 2019

Implemented sound_playevent() using PlaySound(). sound_playevent() return ID, but this can not be used for sound_stop().

@mattn mattn changed the title [WIP] Sound APIs on Windows Sound APIs on Windows Jun 12, 2019

Show resolved Hide resolved src/sound.c
@k-takata

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

@mattn Could you check this additional patch?
https://gist.github.com/k-takata/5fb38f305408a4bfa35221a5aec0f8ab

sound: Fix some issues
* Update document about sound_playevent() on Windows.
* Some lines exceeded 80 columns.
* Types of some variables were wrong.
@k-takata

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

Show resolved Hide resolved src/sound.c Outdated
Show resolved Hide resolved src/sound.c Outdated
@k-takata

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

We've done. I think this is ready to be merged.

@brammool brammool closed this in 9b28352 Jun 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.