bug#74863: 31.0.50; Problems with play-sound on MS-Windows

bug#74863: 31.0.50; Problems with play-sound on MS-Windows

[Cecilio Pardo]

There are some problems with play-sound, but I didn't find an active bug 
for them. Before working on them I have a couple of questions:

- Is sound playing synchronous on purpose?

- To support :data, we can use the PlaySound function, but we will lose
the ability to play files other than wav. We can maintain the current 
code for files, and use PlaySound for :data. Another option would be to 
simply save data to a temp file and play the file.

bug#74863: 31.0.50; Problems with play-sound on MS-Windows

[Eli Zaretskii]

> Date: Sat, 14 Dec 2024 00:50:34 +0100
> From: Cecilio Pardo <cpardo@imayhem.com>
> 
> There are some problems with play-sound, but I didn't find an active bug 
> for them. Before working on them I have a couple of questions:
> 
> - Is sound playing synchronous on purpose?

It's synchronous because AFAIK the equivalent features on GNU/Linux
play sound synchronously.  We have a policy of not installing features
that might put non-free systems at an advantage.  So we can make the
sound playing on Windows asynchronous (which is very easily done on
Windows) once there is such a capability on free systems.

> - To support :data, we can use the PlaySound function, but we will lose
> the ability to play files other than wav. We can maintain the current 
> code for files, and use PlaySound for :data. Another option would be to 
> simply save data to a temp file and play the file.

I'd say the former is preferable, but without losing the ability to
play the files we can today.  That is, maintain the current code for
files and add new code for :data.

Thanks.

bug#74863: 31.0.50; Problems with play-sound on MS-Windows

[Cecilio Pardo]

On 14/12/2024 9:42, Eli Zaretskii wrote:
>> - Is sound playing synchronous on purpose?
> 
> It's synchronous because AFAIK the equivalent features on GNU/Linux
> play sound synchronously.  We have a policy of not installing features
> that might put non-free systems at an advantage.  So we can make the
> sound playing on Windows asynchronous (which is very easily done on
> Windows) once there is such a capability on free systems.

Should we add support for PipeWire on GNU/Linux?

bug#74863: 31.0.50; Problems with play-sound on MS-Windows

[Eli Zaretskii]

> Date: Sat, 14 Dec 2024 15:07:39 +0100
> Cc: 74863@debbugs.gnu.org
> From: Cecilio Pardo <cpardo@imayhem.com>
> 
> On 14/12/2024 9:42, Eli Zaretskii wrote:
> >> - Is sound playing synchronous on purpose?
> > 
> > It's synchronous because AFAIK the equivalent features on GNU/Linux
> > play sound synchronously.  We have a policy of not installing features
> > that might put non-free systems at an advantage.  So we can make the
> > sound playing on Windows asynchronous (which is very easily done on
> > Windows) once there is such a capability on free systems.
> 
> Should we add support for PipeWire on GNU/Linux?

I don't know what PipeWire is, so I have no opinion on this.

bug#74863: 31.0.50; Problems with play-sound on MS-Windows

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> Should we add support for PipeWire on GNU/Linux?

ALSA clients can be configured to output via PipeWire, and the Pipewire
project itself recommends staying with the ALSA API, at least for now:

https://gitlab.freedesktop.org/pipewire/pipewire/-/wikis/FAQ#what-audio-api-do-you-recommend-to-use

> I don't know what PipeWire is, so I have no opinion on this.

It's a new multimedia server that replaces both Pulse Audio and JACK.
AFAIK, it's the default sound server on Debian when using Gnome, and on
Fedora 34.

bug#74863: 31.0.50; Problems with play-sound on MS-Windows

[Cecilio Pardo]

[-- Attachment #1: Type: text/plain, Size: 1042 bytes --]

On 14/12/2024 9:42, Eli Zaretskii wrote:
>> - To support :data, we can use the PlaySound function, but we will lose
>> the ability to play files other than wav. We can maintain the current
>> code for files, and use PlaySound for :data. Another option would be to
>> simply save data to a temp file and play the file.
> 
> I'd say the former is preferable, but without losing the ability to
> play the files we can today.  That is, maintain the current code for
> files and add new code for :data.

This patch adds support for :data using PlaySound, keeping the current 
code for files.

It also fixes a problem in the handling of the volume. Let me know if I 
have to make a separate patch/bug.

Tested with mingw64 and mingw32.

To test:

(defun load-file-into-unibyte-string (file-path)
   (with-temp-buffer
     (set-buffer-multibyte nil)
     (insert-file-contents file-path)
     (buffer-string)))

(play-sound `(sound :data ,(load-file-into-unibyte-string "awav.wav") 
:volume 100))

(play-sound '(sound :file "awav.wav" :volume 100))

[-- Attachment #2: 0001-Add-support-for-the-data-keyword-for-play-sound-in-M.patch --]
[-- Type: text/plain, Size: 13423 bytes --]

From 27225d244bad747e7f2469afe6d8ad0d374e2910 Mon Sep 17 00:00:00 2001
From: Cecilio Pardo <cpardo@imayhem.com>
Date: Sun, 15 Dec 2024 01:13:16 +0100
Subject: [PATCH] Add support for the ':data' keyword for play-sound in
 MS-Windows.

Also fix an error on volume handling.

* etc/NEWS: Add entry for this change.
* etc/PROBLEMS: Remove entry about missing support for :data.
* src/sound.c (parse_sound): Check that either :file or :data is
present.
(do_play_sound): Added parameter to select file or data, and code to
play from data.
(Fplay_sound_internal): Fixed volume format, and send file or data to
do_play_sound.
---
 etc/NEWS     |   5 ++
 etc/PROBLEMS |   5 --
 src/sound.c  | 232 +++++++++++++++++++++++++--------------------------
 3 files changed, 117 insertions(+), 125 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 48546a2d916..b07d75a665a 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1061,6 +1061,11 @@ current buffer, if the major mode supports it.  (Support for
 Transformed images are smoothed using the bilinear interpolation by
 means of the GDI+ library.
 
+---
+** Emacs on MS-Windows now supports the ':data' keyword for 'play-sound'.
+In addition to ':file FILE' for playing a sound from a file, ':data
+DATA' can now be used to play a sound from memory.
+
 \f
 ----------------------------------------------------------------------
 This file is part of GNU Emacs.
diff --git a/etc/PROBLEMS b/etc/PROBLEMS
index 8de12a78613..c1745e8d18f 100644
--- a/etc/PROBLEMS
+++ b/etc/PROBLEMS
@@ -3278,11 +3278,6 @@ Files larger than 4GB cause overflow in the size (represented as a
 well, since the Windows port uses a Lisp emulation of 'ls', which relies
 on 'file-attributes'.
 
-** Playing sound doesn't support the :data method
-
-Sound playing is not supported with the ':data DATA' key-value pair.
-You _must_ use the ':file FILE' method.
-
 ** Typing Alt-Shift has strange effects on MS-Windows.
 
 This combination of keys is a command to change keyboard layout.  If
diff --git a/src/sound.c b/src/sound.c
index 004015fc936..2cc0e71f998 100644
--- a/src/sound.c
+++ b/src/sound.c
@@ -26,14 +26,12 @@ Copyright (C) 1998-1999, 2001-2024 Free Software Foundation, Inc.
   implementation of the play-sound specification for Windows.
 
   Notes:
-  In the Windows implementation of play-sound-internal only the
-  :file and :volume keywords are supported.  The :device keyword,
-  if present, is ignored.  The :data keyword, if present, will
-  cause an error to be generated.
+  In the Windows implementation of play-sound-internal the :device
+  keyword, if present, is ignored.
 
   The Windows implementation of play-sound is implemented via the
-  Windows API functions mciSendString, waveOutGetVolume, and
-  waveOutSetVolume which are exported by Winmm.dll.
+  Windows API functions mciSendString, waveOutGetVolume,
+  waveOutSetVolume and PlaySound which are exported by Winmm.dll.
 */
 
 #include <config.h>
@@ -278,7 +276,7 @@ #define MAX_SOUND_HEADER_BYTES \
 #else /* WINDOWSNT */
 
 /* BEGIN: Windows Specific Definitions */
-static int do_play_sound (const char *, unsigned long);
+static int do_play_sound (const char *, unsigned long, bool);
 /*
   END: Windows Specific Definitions */
 #endif /* WINDOWSNT */
@@ -366,21 +364,10 @@ parse_sound (Lisp_Object sound, Lisp_Object *attrs)
   attrs[SOUND_DEVICE] = plist_get (sound, QCdevice);
   attrs[SOUND_VOLUME] = plist_get (sound, QCvolume);
 
-#ifndef WINDOWSNT
   /* File name or data must be specified.  */
   if (!STRINGP (attrs[SOUND_FILE])
       && !STRINGP (attrs[SOUND_DATA]))
     return 0;
-#else /* WINDOWSNT */
-  /*
-    Data is not supported in Windows.  Therefore a
-    File name MUST be supplied.
-  */
-  if (!STRINGP (attrs[SOUND_FILE]))
-    {
-      return 0;
-    }
-#endif /* WINDOWSNT */
 
   /* Volume must be in the range 0..100 or unspecified.  */
   if (!NILP (attrs[SOUND_VOLUME]))
@@ -1225,7 +1212,7 @@ #define SOUND_WARNING(func, error, text)		\
   } while (0)
 
 static int
-do_play_sound (const char *psz_file, unsigned long ui_volume)
+do_play_sound (const char *psz_file_or_data, unsigned long ui_volume, bool in_memory)
 {
   int i_result = 0;
   MCIERROR mci_error = 0;
@@ -1236,65 +1223,7 @@ do_play_sound (const char *psz_file, unsigned long ui_volume)
   BOOL b_reset_volume = FALSE;
   char warn_text[560];
 
-  /* Since UNICOWS.DLL includes only a stub for mciSendStringW, we
-     need to encode the file in the ANSI codepage on Windows 9X even
-     if w32_unicode_filenames is non-zero.  */
-  if (w32_major_version <= 4 || !w32_unicode_filenames)
-    {
-      char fname_a[MAX_PATH], shortname[MAX_PATH], *fname_to_use;
-
-      filename_to_ansi (psz_file, fname_a);
-      fname_to_use = fname_a;
-      /* If the file name is not encodable in ANSI, try its short 8+3
-	 alias.  This will only work if w32_unicode_filenames is
-	 non-zero.  */
-      if (_mbspbrk ((const unsigned char *)fname_a,
-		    (const unsigned char *)"?"))
-	{
-	  if (w32_get_short_filename (psz_file, shortname, MAX_PATH))
-	    fname_to_use = shortname;
-	  else
-	    mci_error = MCIERR_FILE_NOT_FOUND;
-	}
-
-      if (!mci_error)
-	{
-	  memset (sz_cmd_buf_a, 0, sizeof (sz_cmd_buf_a));
-	  memset (sz_ret_buf_a, 0, sizeof (sz_ret_buf_a));
-	  sprintf (sz_cmd_buf_a,
-		   "open \"%s\" alias GNUEmacs_PlaySound_Device wait",
-		   fname_to_use);
-	  mci_error = mciSendStringA (sz_cmd_buf_a,
-				      sz_ret_buf_a, sizeof (sz_ret_buf_a), NULL);
-	}
-    }
-  else
-    {
-      wchar_t sz_cmd_buf_w[520];
-      wchar_t sz_ret_buf_w[520];
-      wchar_t fname_w[MAX_PATH];
-
-      filename_to_utf16 (psz_file, fname_w);
-      memset (sz_cmd_buf_w, 0, sizeof (sz_cmd_buf_w));
-      memset (sz_ret_buf_w, 0, sizeof (sz_ret_buf_w));
-      /* _swprintf is not available on Windows 9X, so we construct the
-	 UTF-16 command string by hand.  */
-      wcscpy (sz_cmd_buf_w, L"open \"");
-      wcscat (sz_cmd_buf_w, fname_w);
-      wcscat (sz_cmd_buf_w, L"\" alias GNUEmacs_PlaySound_Device wait");
-      mci_error = mciSendStringW (sz_cmd_buf_w,
-				  sz_ret_buf_w, ARRAYELTS (sz_ret_buf_w) , NULL);
-    }
-  if (mci_error != 0)
-    {
-      strcpy (warn_text,
-	      "mciSendString: 'open' command failed to open sound file ");
-      strcat (warn_text, psz_file);
-      SOUND_WARNING (mciGetErrorString, mci_error, warn_text);
-      i_result = (int) mci_error;
-      return i_result;
-    }
-  if ((ui_volume > 0) && (ui_volume != UINT_MAX))
+  if (ui_volume > 0)
     {
       mm_result = waveOutGetVolume ((HWAVEOUT) WAVE_MAPPER, &ui_volume_org);
       if (mm_result == MMSYSERR_NOERROR)
@@ -1319,34 +1248,100 @@ do_play_sound (const char *psz_file, unsigned long ui_volume)
                          " not be used.");
         }
     }
-  memset (sz_cmd_buf_a, 0, sizeof (sz_cmd_buf_a));
-  memset (sz_ret_buf_a, 0, sizeof (sz_ret_buf_a));
-  strcpy (sz_cmd_buf_a, "play GNUEmacs_PlaySound_Device wait");
-  mci_error = mciSendStringA (sz_cmd_buf_a, sz_ret_buf_a, sizeof (sz_ret_buf_a),
-			      NULL);
-  if (mci_error != 0)
+
+  if (in_memory)
+    i_result = !PlaySound (psz_file_or_data, NULL, SND_MEMORY);
+  else
     {
-      strcpy (warn_text,
-	      "mciSendString: 'play' command failed to play sound file ");
-      strcat (warn_text, psz_file);
-      SOUND_WARNING (mciGetErrorString, mci_error, warn_text);
-      i_result = (int) mci_error;
+      /* Since UNICOWS.DLL includes only a stub for mciSendStringW, we
+	 need to encode the file in the ANSI codepage on Windows 9X even
+	 if w32_unicode_filenames is non-zero.  */
+      if (w32_major_version <= 4 || !w32_unicode_filenames)
+	{
+	  char fname_a[MAX_PATH], shortname[MAX_PATH], *fname_to_use;
+
+	  filename_to_ansi (psz_file_or_data, fname_a);
+	  fname_to_use = fname_a;
+	  /* If the file name is not encodable in ANSI, try its short 8+3
+	     alias.  This will only work if w32_unicode_filenames is
+	     non-zero.  */
+	  if (_mbspbrk ((const unsigned char *)fname_a,
+			(const unsigned char *)"?"))
+	    {
+	      if (w32_get_short_filename (psz_file_or_data, shortname, MAX_PATH))
+		fname_to_use = shortname;
+	      else
+		mci_error = MCIERR_FILE_NOT_FOUND;
+	    }
+
+	  if (!mci_error)
+	    {
+	      memset (sz_cmd_buf_a, 0, sizeof (sz_cmd_buf_a));
+	      memset (sz_ret_buf_a, 0, sizeof (sz_ret_buf_a));
+	      sprintf (sz_cmd_buf_a,
+		       "open \"%s\" alias GNUEmacs_PlaySound_Device wait",
+		       fname_to_use);
+	      mci_error = mciSendStringA (sz_cmd_buf_a,
+					  sz_ret_buf_a, sizeof (sz_ret_buf_a), NULL);
+	    }
+	}
+      else
+	{
+	  wchar_t sz_cmd_buf_w[520];
+	  wchar_t sz_ret_buf_w[520];
+	  wchar_t fname_w[MAX_PATH];
+
+	  filename_to_utf16 (psz_file_or_data, fname_w);
+	  memset (sz_cmd_buf_w, 0, sizeof (sz_cmd_buf_w));
+	  memset (sz_ret_buf_w, 0, sizeof (sz_ret_buf_w));
+	  /* _swprintf is not available on Windows 9X, so we construct the
+	     UTF-16 command string by hand.  */
+	  wcscpy (sz_cmd_buf_w, L"open \"");
+	  wcscat (sz_cmd_buf_w, fname_w);
+	  wcscat (sz_cmd_buf_w, L"\" alias GNUEmacs_PlaySound_Device wait");
+	  mci_error = mciSendStringW (sz_cmd_buf_w,
+				      sz_ret_buf_w, ARRAYELTS (sz_ret_buf_w) , NULL);
+	}
+      if (mci_error != 0)
+	{
+	  strcpy (warn_text,
+		  "mciSendString: 'open' command failed to open sound file ");
+	  strcat (warn_text, psz_file_or_data);
+	  SOUND_WARNING (mciGetErrorString, mci_error, warn_text);
+	  i_result = (int) mci_error;
+	  return i_result;
+	}
+      memset (sz_cmd_buf_a, 0, sizeof (sz_cmd_buf_a));
+      memset (sz_ret_buf_a, 0, sizeof (sz_ret_buf_a));
+      strcpy (sz_cmd_buf_a, "play GNUEmacs_PlaySound_Device wait");
+      mci_error = mciSendStringA (sz_cmd_buf_a, sz_ret_buf_a, sizeof (sz_ret_buf_a),
+				  NULL);
+      if (mci_error != 0)
+	{
+	  strcpy (warn_text,
+		  "mciSendString: 'play' command failed to play sound file ");
+	  strcat (warn_text, psz_file_or_data);
+	  SOUND_WARNING (mciGetErrorString, mci_error, warn_text);
+	  i_result = (int) mci_error;
+	}
+      memset (sz_cmd_buf_a, 0, sizeof (sz_cmd_buf_a));
+      memset (sz_ret_buf_a, 0, sizeof (sz_ret_buf_a));
+      strcpy (sz_cmd_buf_a, "close GNUEmacs_PlaySound_Device wait");
+      mci_error = mciSendStringA (sz_cmd_buf_a, sz_ret_buf_a, sizeof (sz_ret_buf_a),
+				  NULL);
     }
-  memset (sz_cmd_buf_a, 0, sizeof (sz_cmd_buf_a));
-  memset (sz_ret_buf_a, 0, sizeof (sz_ret_buf_a));
-  strcpy (sz_cmd_buf_a, "close GNUEmacs_PlaySound_Device wait");
-  mci_error = mciSendStringA (sz_cmd_buf_a, sz_ret_buf_a, sizeof (sz_ret_buf_a),
-			      NULL);
+
   if (b_reset_volume == TRUE)
     {
       mm_result = waveOutSetVolume ((HWAVEOUT) WAVE_MAPPER, ui_volume_org);
       if (mm_result != MMSYSERR_NOERROR)
-        {
-          SOUND_WARNING (waveOutGetErrorText, mm_result,
+	{
+	  SOUND_WARNING (waveOutGetErrorText, mm_result,
 			 "waveOutSetVolume: failed to reset the original"
-                         " volume level of the WAVE_MAPPER device.");
-        }
+			 " volume level of the WAVE_MAPPER device.");
+	}
     }
+
   return i_result;
 }
 
@@ -1364,8 +1359,7 @@ DEFUN ("play-sound-internal", Fplay_sound_internal, Splay_sound_internal, 1, 1,
   specpdl_ref count = SPECPDL_INDEX ();
 
 #ifdef WINDOWSNT
-  unsigned long ui_volume_tmp = UINT_MAX;
-  unsigned long ui_volume = UINT_MAX;
+  unsigned long ui_volume = 0;
 #endif /* WINDOWSNT */
 
   /* Parse the sound specification.  Give up if it is invalid.  */
@@ -1432,33 +1426,31 @@ DEFUN ("play-sound-internal", Fplay_sound_internal, Splay_sound_internal, 1, 1,
 
 #else /* WINDOWSNT */
 
-  file = Fexpand_file_name (attrs[SOUND_FILE], Vdata_directory);
-  file = ENCODE_FILE (file);
+
   if (FIXNUMP (attrs[SOUND_VOLUME]))
-    {
-      ui_volume_tmp = XFIXNAT (attrs[SOUND_VOLUME]);
-    }
+    ui_volume = XFIXNAT (attrs[SOUND_VOLUME]);
   else if (FLOATP (attrs[SOUND_VOLUME]))
-    {
-      ui_volume_tmp = XFLOAT_DATA (attrs[SOUND_VOLUME]) * 100;
-    }
+    ui_volume = XFLOAT_DATA (attrs[SOUND_VOLUME]) * 100;
+
+  if (ui_volume > 100)
+    ui_volume = 100;
+
+  /* For volume (32 bits), low order 16 bits are the value for left
+     channel, and high order 16 bits for the right channel.  We use the
+     specified volume on both channels.  */
+  ui_volume = ui_volume * 0xFFFF / 100;
+  ui_volume = (ui_volume << 16) + ui_volume;
 
   CALLN (Frun_hook_with_args, Qplay_sound_functions, sound);
 
-  /*
-    Based on some experiments I have conducted, a value of 100 or less
-    for the sound volume is much too low.  You cannot even hear it.
-    A value of UINT_MAX indicates that you wish for the sound to played
-    at the maximum possible volume.  A value of UINT_MAX/2 plays the
-    sound at 50% maximum volume.  Therefore the value passed to do_play_sound
-    (and thus to waveOutSetVolume) must be some fraction of UINT_MAX.
-    The following code adjusts the user specified volume level appropriately.
-  */
-  if ((ui_volume_tmp > 0) && (ui_volume_tmp <= 100))
+  if (STRINGP (attrs[SOUND_FILE]))
     {
-      ui_volume = ui_volume_tmp * (UINT_MAX / 100);
+      file = Fexpand_file_name (attrs[SOUND_FILE], Vdata_directory);
+      file = ENCODE_FILE (file);
+      do_play_sound (SSDATA (file), ui_volume, false);
     }
-  (void)do_play_sound (SSDATA (file), ui_volume);
+  else
+    do_play_sound (SDATA (attrs[SOUND_DATA]), ui_volume, true);
 
 #endif /* WINDOWSNT */
 
-- 
2.35.1.windows.2

bug#74863: 31.0.50; Problems with play-sound on MS-Windows

[Eli Zaretskii]

> Date: Sun, 15 Dec 2024 12:55:35 +0100
> Cc: 74863@debbugs.gnu.org
> From: Cecilio Pardo <cpardo@imayhem.com>
> 
> This patch adds support for :data using PlaySound, keeping the current 
> code for files.

Thanks.

> It also fixes a problem in the handling of the volume. Let me know if I 
> have to make a separate patch/bug.

No need.

> To test:
> 
> (defun load-file-into-unibyte-string (file-path)
>    (with-temp-buffer
>      (set-buffer-multibyte nil)
>      (insert-file-contents file-path)
>      (buffer-string)))
> 
> (play-sound `(sound :data ,(load-file-into-unibyte-string "awav.wav") 
> :volume 100))

What's wrong with insert-file-contents-literally?

> +  if (in_memory)
> +    i_result = !PlaySound (psz_file_or_data, NULL, SND_MEMORY);

AFAIU, the documentation seems to say that the string passed as the
first argument to PlaySound is limited to 256 characters (i.e. bytes)?
If so, how do we play longer sounds?

Should we also use SND_SENTRY flag (on Vista and later)?