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

[Cecilio Pardo <cpardo@imayhem.com>]

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

On 15/12/2024 13:50, Eli Zaretskii wrote:

>> (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?

Nothing. So many functions...

> 
>> +  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?

I think this limitation applies only when the argument points to a 
string, and not when using SND_MEMORY, which is documented with: "The 
pszSound parameter points to a sound loaded in memory."

It's true the documentation doesn't say so, and I haven't found any 
clarification by Microsoft, but SND_MEMORY would be useless otherwise. 
Also I see normal usage searching the internet.

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

Yes, done in the attached patch.


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

From b749e748672dea5e429f168e531de33f499f2b93 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  | 242 ++++++++++++++++++++++++++-------------------------
 3 files changed, 127 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..843d05d2349 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>
@@ -91,6 +89,11 @@ Copyright (C) 1998-1999, 2001-2024 Free Software Foundation, Inc.
 #include "w32.h"
 /* END: Windows Specific Includes */
 
+/* Missing in mingw32.  */
+#ifndef SND_SENTRY
+#define SND_SENTRY 0x00080000
+#endif
+
 #endif /* WINDOWSNT */
 
 /* BEGIN: Common Definitions */
@@ -278,7 +281,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 +369,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 +1217,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 +1228,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 +1253,105 @@ 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)
     {
-      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;
+      int flags = SND_MEMORY;
+      if (w32_major_version >= 6) /* Vista and later */
+	flags |= SND_SENTRY;
+      i_result = !PlaySound (psz_file_or_data, NULL, flags);
     }
-  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);
+  else
+    {
+      /* 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);
+    }
+
   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 +1369,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 +1436,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