[vim/vim] [WIP] Sound APIs on Windows (#4522)
mattn
This is work in progress. Current status:
sound_playfile: done
sound_stop: done
sound_clear: done
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/4522
Commit Summary
- [WIP] Add sound on Windows
- Merge branch 'master' into sound-windows
- Implement sound_playfile, sound_stop, sound_clear
File Changes
- M src/Make_cyg_ming.mak (19)
- M src/sound.c (163)
Patch Links:
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
K.Takata
@k-takata commented on this pull request.
In src/sound.c:
> @@ -199,4 +205,151 @@ sound_free(void) } #endif -#endif // FEAT_SOUND && HAVE_CANBERRA +#elif defined(MSWIN) // MSWIN + +static HWND g_hWndSound = NULL; + + LRESULT CALLBACK
static might be added.
K.Takata
@k-takata commented on this pull request.
> @@ -106,6 +106,13 @@ else TERMINAL=no endif +# Set to yes to enable sound support. +ifeq (HUGE, $(FEATURES))
ifneq ($(findstring $(FEATURES),BIG HUGE),) might be better?
Sound is supported with the big features on Linux.
K.Takata
@k-takata commented on this pull request.
In src/sound.c:
> +{
+ int newid = sound_id + 1;
+ int len;
+ char_u *p, *esc;
+ soundcb_T *soundcb;
+
+ rettv->v_type = VAR_NUMBER;
+
+ esc = vim_strsave_shellescape(
+ tv_get_string(&argvars[0]), FALSE, FALSE);
+
+ len = STRLEN(esc) + 5 + 18 + 1;
+ p = alloc(len);
+ vim_snprintf((char *)p, len, "open %s alias sound%06d", esc, newid);
+ free(esc);
+ if (mciSendString((char*) p, NULL, 0, (HWND) sound_window()) != 0)
Don't we need to convert the encoding of the sound filename to UTF-16 and use the wide function?
Sound files with multibyte filename might not work with the current implementation.
K.Takata
@mattn I will create a patch for Make_mvc.mak later.
K.Takata
> + }
+
+ return g_hWndSound;
+}
+
+ void
+f_sound_playevent(typval_T *argvars, typval_T *rettv)
+{
+ smsg("sound_playevent() not implemented");
+}
+
+ void
+f_sound_playfile(typval_T *argvars, typval_T *rettv)
+{
+ int newid = sound_id + 1;
+ int len;
The type of len should be size_t, otherwise warning occurs at L282.
mattn
@mattn commented on this pull request.
In src/sound.c:
> +{
+ int newid = sound_id + 1;
+ int len;
+ char_u *p, *esc;
+ soundcb_T *soundcb;
+
+ rettv->v_type = VAR_NUMBER;
+
+ esc = vim_strsave_shellescape(
+ tv_get_string(&argvars[0]), FALSE, FALSE);
+
+ len = STRLEN(esc) + 5 + 18 + 1;
+ p = alloc(len);
+ vim_snprintf((char *)p, len, "open %s alias sound%06d", esc, newid);
+ free(esc);
+ if (mciSendString((char*) p, NULL, 0, (HWND) sound_window()) != 0)
Yes, will do. I will have to use wide string APIs and window proceudure too.
K.Takata
@k-takata commented on this pull request.
In src/sound.c:
> + int dummy; + + argv[0].v_type = VAR_NUMBER; + argv[0].vval.v_number = p->sound_id; + argv[1].v_type = VAR_NUMBER; + argv[1].vval.v_number = wParam == MCI_NOTIFY_SUCCESSFUL ? 0 + : wParam == MCI_NOTIFY_ABORTED ? 1 : 2; + argv[2].v_type = VAR_UNKNOWN; + + call_callback(&p->snd_callback, -1, + &rettv, 2, argv, NULL, 0L, 0L, &dummy, TRUE, NULL); + clear_tv(&rettv); + + delete_sound_callback(p); + redraw_after_callback(TRUE); +
Don't we need to close the sound file here? Or is it closed automatically?
In src/sound.c:
> + p = alloc(len);
+ vim_snprintf((char *)p, len, "open %s alias sound%06d", esc, newid);
+ free(esc);
+ if (mciSendString((char*) p, NULL, 0, (HWND) sound_window()) != 0)
+ {
+ free(p);
+ rettv->vval.v_number = 0;
+ return;
+ }
+ free(p);
+
+ len = 23 + 1;
+ p = alloc(len);
+ vim_snprintf((char *)p, len, "play sound%06d notify", newid);
+ if (mciSendString((char*) p, NULL, 0, (HWND) sound_window()) != 0)
+ {
Don't we need to close the file when failed to play?
In src/sound.c:
> @@ -199,4 +205,151 @@ sound_free(void)
}
#endif
-#endif // FEAT_SOUND && HAVE_CANBERRA
+#elif defined(MSWIN) // MSWIN
+
+static HWND g_hWndSound = NULL;
+
+ LRESULT CALLBACK
+sound_wndproc(HWND hwnd, UINT message, WPARAM wParam, LPARAM lParam)
+{
+ soundcb_T *p;
+
+ switch(message)
Space needed after switch.
K.Takata
@mattn Could you import this patch?
https://gist.github.com/k-takata/4a8c42377c3256f1e29d26183b4b2a39
mattn
@k-takata Will do tonight. Thank you.
mattn
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.
Bram Moolenaar
> I applied review fixes. I noticed that implementing sound_playevent()
> is bits hard. The reasons are:
>
> * 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.
Thanks for working on this. Unfortunately, the list of sound events
that is supported is unclear. There is a list from XDG, e.g. here:
http://0pointer.de/public/sound-naming-spec.html
But it contains many names that my Ubuntu system doesn't have, while the
simple "bell" sound isn't listed.
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.
--
hundred-and-one symptoms of being an internet addict:
152. You find yourself falling for someone you've never seen or hardly
know, but, boy can he/she TYPE!!!!!!
/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
mattn
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.
Bram Moolenaar
> >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.
thus block until it's finished. If we can do this in a new thread, then
it's possible to invoke the callback when finished. Is that doable?
--
Although the scythe isn't pre-eminent among the weapons of war, anyone who
has been on the wrong end of, say, a peasants' revolt will know that in
skilled hands it is fearsome.
-- (Terry Pratchett, Mort)
/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—
K.Takata
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.
K.Takata
> +
+ static LRESULT CALLBACK
+sound_wndproc(HWND hwnd, UINT message, WPARAM wParam, LPARAM lParam)
+{
+ soundcb_T *p;
+
+ switch (message)
+ {
+ case MM_MCINOTIFY:
+ for (p = first_callback; p != NULL; p = p->snd_next)
+ if (p->device_id == (MCIDEVICEID) lParam)
+ {
+ typval_T argv[3];
+ typval_T rettv;
+ int dummy;
+ char buf[16];
The buffer size looks insufficient.
In src/sound.c:
> + }
+
+ return g_hWndSound;
+}
+
+ void
+f_sound_playevent(typval_T *argvars, typval_T *rettv)
+{
+ smsg("sound_playevent() not implemented");
+}
+
+ void
+f_sound_playfile(typval_T *argvars, typval_T *rettv)
+{
+ int newid = sound_id + 1;
+ int len;
Not fixed yet. Warning occurs at L290.
In src/sound.c:
> + len = STRLEN(esc) + 5 + 18 + 1;
+ p = alloc(len);
+ if (p == NULL)
+ {
+ free(esc);
+ return;
+ }
+ vim_snprintf((char *)p, len, "open %s alias sound%06d", esc, newid);
+ free(esc);
+
+ wp = enc_to_utf16((char_u *)p, NULL);
+ free(p);
+ if (wp == NULL)
+ return;
+
+ err = mciSendStringW(wp, NULL, 0, (HWND) sound_window());
The cast (HWND) might not be necessary.
In src/sound.c:
> + } + vim_snprintf((char *)p, len, "open %s alias sound%06d", esc, newid); + free(esc); + + wp = enc_to_utf16((char_u *)p, NULL); + free(p); + if (wp == NULL) + return; + + err = mciSendStringW(wp, NULL, 0, (HWND) sound_window()); + free(wp); + if (err != 0) + return; + + vim_snprintf(buf, sizeof(buf), "play sound%06d notify", newid); + if (mciSendString(buf, NULL, 0, (HWND) sound_window()) != 0)
The cast (HWND) might not be necessary.
In src/sound.c:
> + rettv->vval.v_number = sound_id;
+
+ soundcb = get_sound_callback(&argvars[1]);
+ if (soundcb != NULL)
+ {
+ vim_snprintf(buf, sizeof(buf), "sound%06d", newid);
+ soundcb->sound_id = newid;
+ soundcb->device_id = mciGetDeviceID(buf);
+ }
+}
+
+ void
+f_sound_stop(typval_T *argvars, typval_T *rettv UNUSED)
+{
+ int id = tv_get_number(&argvars[0]);
+ char buf[16];
The buffer size looks insufficient.
In src/sound.c:
> + void
+f_sound_playfile(typval_T *argvars, typval_T *rettv)
+{
+ int newid = sound_id + 1;
+ int len;
+ char_u *p, *esc;
+ WCHAR *wp;
+ soundcb_T *soundcb;
+ char buf[32];
+ MCIERROR err;
+
+ rettv->v_type = VAR_NUMBER;
+ rettv->vval.v_number = 0;
+
+ esc = vim_strsave_shellescape(
+ tv_get_string(&argvars[0]), FALSE, FALSE);
This line might be able to join with the previous line. It will fit in the 80 columns.
Bram Moolenaar
Ken Takata wrote:
> > 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.
overlapping sounds is common. I assume the system sounds are fairly
short anyway. Stopping a sound file would be more common, e.g. if you
have an alarm clock sound or ringtone.
One thing to keep in mind: If the sound plays in a separate thread,
invoking the callback needs to be done in the main thread, thus it needs
to use some safe inter-thread flag. Invoking the callback from another
thread might break just about anything.
--
hundred-and-one symptoms of being an internet addict:
Codecov
Codecov Report
Merging #4522 into master will decrease coverage by
<.01%.
The diff coverage isn/a.
@@ Coverage Diff @@ ## master #4522 +/- ## ========================================== - Coverage 80.65% 80.64% -0.01% ========================================== Files 111 111 Lines 143838 143841 +3 ========================================== - Hits 116011 116002 -9 - Misses 27827 27839 +12
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/sound.c | 64.38% <ø> (ø) |
⬆️ |
| src/if_xcmdsrv.c | 83.48% <0%> (-1.08%) |
⬇️ |
| src/netbeans.c | 27.25% <0%> (-0.3%) |
⬇️ |
| src/ex_cmds.c | 82.18% <0%> (-0.25%) |
⬇️ |
| src/gui.c | 58.47% <0%> (-0.11%) |
⬇️ |
| src/gui_gtk_x11.c | 49.36% <0%> (-0.05%) |
⬇️ |
| src/terminal.c | 81.05% <0%> (-0.04%) |
⬇️ |
| src/version.c | 88.93% <0%> (ø) |
⬆️ |
| src/window.c | 87.21% <0%> (+0.03%) |
⬆️ |
| src/option.c | 86.28% <0%> (+0.04%) |
⬆️ |
| ... and 3 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...fdc8ee8. Read the comment docs.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
mattn
Implemented sound_playevent() using PlaySound(). sound_playevent() return ID, but this can not be used for sound_stop().
K.Takata
> +}
+
+ void
+f_sound_playevent(typval_T *argvars, typval_T *rettv)
+{
+ WCHAR *wp;
+
+ rettv->v_type = VAR_NUMBER;
+ rettv->vval.v_number = 0;
+
+ wp = enc_to_utf16(tv_get_string(&argvars[0]), NULL);
+ if (wp == NULL)
+ return;
+
+ PlaySoundW(wp, 0, SND_ASYNC | SND_ALIAS);
+
wp should be freed here.
mattn
@mattn commented on this pull request.
In src/sound.c:
> +}
+
+ void
+f_sound_playevent(typval_T *argvars, typval_T *rettv)
+{
+ WCHAR *wp;
+
+ rettv->v_type = VAR_NUMBER;
+ rettv->vval.v_number = 0;
+
+ wp = enc_to_utf16(tv_get_string(&argvars[0]), NULL);
+ if (wp == NULL)
+ return;
+
+ PlaySoundW(wp, 0, SND_ASYNC | SND_ALIAS);
+
Thank you.
—
You are receiving this because you are subscribed to this thread.
K.Takata
@mattn Could you check this additional patch?
https://gist.github.com/k-takata/5fb38f305408a4bfa35221a5aec0f8ab
K.Takata
@mattn AppVeyor is failing. We need to fix the test.
https://ci.appveyor.com/project/chrisbra/vim/builds/25258938/job/2h4no54pbcv7fert#L3620
—
You are receiving this because you are subscribed to this thread.
K.Takata
@k-takata commented on this pull request.
In src/sound.c:
> @@ -75,6 +76,11 @@ delete_sound_callback(soundcb_T *soundcb) } } +#if defined(HAVE_CANBERRA)
This line should be #if defined(HAVE_CANBERRA) || defined(PROTO), otherwise cproto won't be able to generate the prototypes.
In src/sound.c:
> +{
+ long id = tv_get_number(&argvars[0]);
+ char buf[32];
+
+ vim_snprintf(buf, sizeof(buf), "stop sound%06ld", id);
+ mciSendString(buf, NULL, 0, NULL);
+}
+
+ void
+f_sound_clear(typval_T *argvars UNUSED, typval_T *rettv UNUSED)
+{
+ PlaySound(NULL, NULL, 0);
+ mciSendString("close all", NULL, 0, NULL);
+}
+
+#if defined(EXITFREE) || defined(PROTO)
|| defined(PROTO) may not needed here, because cproto won't check the MSWIN part.
—
You are receiving this because you are subscribed to this thread.
K.Takata
We've done. I think this is ready to be merged.
—
You are receiving this because you are subscribed to this thread.