[PATCH 1/3] winepulse: Use handles to represent the stream.

[Huw Davies]

Signed-off-by: Huw Davies <huw(a)codeweavers.com>
---
 dlls/winepulse.drv/mmdevdrv.c |   8 +--
 dlls/winepulse.drv/pulse.c    | 122 ++++++++++++++++++++++++++++------
 dlls/winepulse.drv/unixlib.h  |  40 ++++++-----
 3 files changed, 125 insertions(+), 45 deletions(-)

diff --git a/dlls/winepulse.drv/mmdevdrv.c b/dlls/winepulse.drv/mmdevdrv.c
index 7b5ef7cfe39..96b8b28d1b5 100644
--- a/dlls/winepulse.drv/mmdevdrv.c
+++ b/dlls/winepulse.drv/mmdevdrv.c
@@ -156,7 +156,7 @@ struct ACImpl {
     UINT32 channel_count;
     HANDLE timer;
 
-    struct pulse_stream *pulse_stream;
+    unsigned int pulse_stream;
 
     AudioSession *session;
     AudioSessionWrapper *session_wrapper;
@@ -228,7 +228,7 @@ static void pulse_call(enum unix_funcs code, void *params)
     assert(!status);
 }
 
-static void pulse_release_stream(struct pulse_stream *stream, HANDLE timer)
+static void pulse_release_stream(unsigned int stream, HANDLE timer)
 {
     struct release_stream_params params;
     params.stream = stream;
@@ -668,7 +668,7 @@ static ULONG WINAPI AudioClient_Release(IAudioClient3 *iface)
     if (!ref) {
         if (This->pulse_stream) {
             pulse_release_stream(This->pulse_stream, This->timer);
-            This->pulse_stream = NULL;
+            This->pulse_stream = 0;
             EnterCriticalSection(&session_cs);
             list_remove(&This->entry);
             LeaveCriticalSection(&session_cs);
@@ -822,7 +822,7 @@ static HRESULT WINAPI AudioClient_Initialize(IAudioClient3 *iface,
     ACImpl *This = impl_from_IAudioClient3(iface);
     struct create_stream_params params;
     unsigned int i, channel_count;
-    struct pulse_stream *stream;
+    unsigned int stream;
     char *name;
     HRESULT hr;
 
diff --git a/dlls/winepulse.drv/pulse.c b/dlls/winepulse.drv/pulse.c
index 901e054f6d0..5e3ddcc9b6e 100644
--- a/dlls/winepulse.drv/pulse.c
+++ b/dlls/winepulse.drv/pulse.c
@@ -108,6 +108,18 @@ static const REFERENCE_TIME DefaultPeriod = 100000;
 static pthread_mutex_t pulse_mutex;
 static pthread_cond_t pulse_cond = PTHREAD_COND_INITIALIZER;
 
+struct stream_entry
+{
+    union
+    {
+        struct pulse_stream *stream;
+        unsigned int next_free;
+    };
+};
+static struct stream_entry *handles;
+static unsigned int next_free = ~0u, next_unused, num_handles;
+static pthread_mutex_t handle_lock = PTHREAD_MUTEX_INITIALIZER;
+
 UINT8 mult_alaw_sample(UINT8, float);
 UINT8 mult_ulaw_sample(UINT8, float);
 
@@ -131,6 +143,73 @@ static void pulse_broadcast(void)
     pthread_cond_broadcast(&pulse_cond);
 }
 
+static unsigned int handle_alloc(struct pulse_stream *stream)
+{
+    unsigned int index, count;
+    struct stream_entry *new_handles;
+
+    pthread_mutex_lock(&handle_lock);
+
+    index = next_free;
+    if(index != ~0u) next_free = handles[index].next_free;
+    else if(next_unused < num_handles) index = next_unused++;
+    else{
+        count = max(num_handles * 2, 256);
+        new_handles = realloc(handles, count);
+        if(!new_handles){
+            pthread_mutex_unlock(&handle_lock);
+            return 0;
+        }
+        handles = new_handles;
+        num_handles = count;
+        index = next_unused++;
+    }
+    handles[index].stream = stream;
+
+    pthread_mutex_unlock(&handle_lock);
+
+    return index + 1;
+}
+
+static struct stream_entry *handle_entry(unsigned int h)
+{
+    if(!h || h > num_handles){
+        ERR("Invalid handle %08x\n", h);
+        return NULL;
+    }
+    return handles + h - 1;
+}
+
+static void handle_free(unsigned int h)
+{
+    struct stream_entry *entry;
+
+    pthread_mutex_lock(&handle_lock);
+
+    entry = handle_entry(h);
+    if(entry){
+        entry->next_free = next_free;
+        next_free = h - 1;
+    }
+
+    pthread_mutex_unlock(&handle_lock);
+}
+
+static struct pulse_stream *handle_get_stream(unsigned int h)
+{
+    struct stream_entry *entry;
+    struct pulse_stream *stream = NULL;
+
+    pthread_mutex_lock(&handle_lock);
+
+    entry = handle_entry(h);
+    if(entry) stream = entry->stream;
+
+    pthread_mutex_unlock(&handle_lock);
+
+    return stream;
+}
+
 static void dump_attr(const pa_buffer_attr *attr)
 {
     TRACE("maxlength: %u\n", attr->maxlength);
@@ -1067,7 +1146,8 @@ static NTSTATUS pulse_create_stream(void *args)
     }
 
     *params->channel_count = stream->ss.channels;
-    *params->stream = stream;
+    *params->stream = handle_alloc(stream);
+    if (!*params->stream) params->result = E_OUTOFMEMORY;
 
 exit:
     if (FAILED(params->result = hr)) {
@@ -1086,7 +1166,7 @@ exit:
 static NTSTATUS pulse_release_stream(void *args)
 {
     struct release_stream_params *params = args;
-    struct pulse_stream *stream = params->stream;
+    struct pulse_stream *stream = handle_get_stream(params->stream);
     SIZE_T size;
 
     if(params->timer) {
@@ -1114,6 +1194,7 @@ static NTSTATUS pulse_release_stream(void *args)
         NtFreeVirtualMemory(GetCurrentProcess(), (void **)&stream->local_buffer,
                             &size, MEM_RELEASE);
     }
+    handle_free(params->stream);
     free(stream->peek_buffer);
     free(stream);
     return STATUS_SUCCESS;
@@ -1407,7 +1488,7 @@ static void pulse_read(struct pulse_stream *stream)
 static NTSTATUS pulse_timer_loop(void *args)
 {
     struct timer_loop_params *params = args;
-    struct pulse_stream *stream = params->stream;
+    struct pulse_stream *stream = handle_get_stream(params->stream);
     LARGE_INTEGER delay;
     pa_usec_t last_time;
     UINT32 adv_bytes;
@@ -1515,7 +1596,7 @@ static NTSTATUS pulse_timer_loop(void *args)
 static NTSTATUS pulse_start(void *args)
 {
     struct start_params *params = args;
-    struct pulse_stream *stream = params->stream;
+    struct pulse_stream *stream = handle_get_stream(params->stream);
     int success;
     pa_operation *o;
 
@@ -1571,7 +1652,7 @@ static NTSTATUS pulse_start(void *args)
 static NTSTATUS pulse_stop(void *args)
 {
     struct stop_params *params = args;
-    struct pulse_stream *stream = params->stream;
+    struct pulse_stream *stream = handle_get_stream(params->stream);
     pa_operation *o;
     int success;
 
@@ -1614,7 +1695,7 @@ static NTSTATUS pulse_stop(void *args)
 static NTSTATUS pulse_reset(void *args)
 {
     struct reset_params *params = args;
-    struct pulse_stream *stream = params->stream;
+    struct pulse_stream *stream = handle_get_stream(params->stream);
 
     pulse_lock();
     if (!pulse_stream_valid(stream))
@@ -1720,7 +1801,7 @@ static UINT32 pulse_capture_padding(struct pulse_stream *stream)
 static NTSTATUS pulse_get_render_buffer(void *args)
 {
     struct get_render_buffer_params *params = args;
-    struct pulse_stream *stream = params->stream;
+    struct pulse_stream *stream = handle_get_stream(params->stream);
     size_t bytes;
     UINT32 wri_offs_bytes;
 
@@ -1799,7 +1880,7 @@ static void pulse_wrap_buffer(struct pulse_stream *stream, BYTE *buffer, UINT32
 static NTSTATUS pulse_release_render_buffer(void *args)
 {
     struct release_render_buffer_params *params = args;
-    struct pulse_stream *stream = params->stream;
+    struct pulse_stream *stream = handle_get_stream(params->stream);
     UINT32 written_bytes;
     BYTE *buffer;
 
@@ -1856,7 +1937,7 @@ static NTSTATUS pulse_release_render_buffer(void *args)
 static NTSTATUS pulse_get_capture_buffer(void *args)
 {
     struct get_capture_buffer_params *params = args;
-    struct pulse_stream *stream = params->stream;
+    struct pulse_stream *stream = handle_get_stream(params->stream);
     ACPacket *packet;
 
     pulse_lock();
@@ -1902,7 +1983,7 @@ static NTSTATUS pulse_get_capture_buffer(void *args)
 static NTSTATUS pulse_release_capture_buffer(void *args)
 {
     struct release_capture_buffer_params *params = args;
-    struct pulse_stream *stream = params->stream;
+    struct pulse_stream *stream = handle_get_stream(params->stream);
 
     pulse_lock();
     if (!stream->locked && params->done)
@@ -1937,14 +2018,15 @@ static NTSTATUS pulse_release_capture_buffer(void *args)
 static NTSTATUS pulse_get_buffer_size(void *args)
 {
     struct get_buffer_size_params *params = args;
+    struct pulse_stream *stream = handle_get_stream(params->stream);
 
     params->result = S_OK;
 
     pulse_lock();
-    if (!pulse_stream_valid(params->stream))
+    if (!pulse_stream_valid(stream))
         params->result = AUDCLNT_E_DEVICE_INVALIDATED;
     else
-        *params->size = params->stream->bufsize_frames;
+        *params->size = stream->bufsize_frames;
     pulse_unlock();
 
     return STATUS_SUCCESS;
@@ -1953,7 +2035,7 @@ static NTSTATUS pulse_get_buffer_size(void *args)
 static NTSTATUS pulse_get_latency(void *args)
 {
     struct get_latency_params *params = args;
-    struct pulse_stream *stream = params->stream;
+    struct pulse_stream *stream = handle_get_stream(params->stream);
     const pa_buffer_attr *attr;
     REFERENCE_TIME lat;
 
@@ -1978,7 +2060,7 @@ static NTSTATUS pulse_get_latency(void *args)
 static NTSTATUS pulse_get_current_padding(void *args)
 {
     struct get_current_padding_params *params = args;
-    struct pulse_stream *stream = params->stream;
+    struct pulse_stream *stream = handle_get_stream(params->stream);
 
     pulse_lock();
     if (!pulse_stream_valid(stream))
@@ -2003,7 +2085,7 @@ static NTSTATUS pulse_get_current_padding(void *args)
 static NTSTATUS pulse_get_next_packet_size(void *args)
 {
     struct get_next_packet_size_params *params = args;
-    struct pulse_stream *stream = params->stream;
+    struct pulse_stream *stream = handle_get_stream(params->stream);
 
     pulse_lock();
     pulse_capture_padding(stream);
@@ -2020,7 +2102,7 @@ static NTSTATUS pulse_get_next_packet_size(void *args)
 static NTSTATUS pulse_get_frequency(void *args)
 {
     struct get_frequency_params *params = args;
-    struct pulse_stream *stream = params->stream;
+    struct pulse_stream *stream = handle_get_stream(params->stream);
 
     pulse_lock();
     if (!pulse_stream_valid(stream))
@@ -2041,7 +2123,7 @@ static NTSTATUS pulse_get_frequency(void *args)
 static NTSTATUS pulse_get_position(void *args)
 {
     struct get_position_params *params = args;
-    struct pulse_stream *stream = params->stream;
+    struct pulse_stream *stream = handle_get_stream(params->stream);
 
     pulse_lock();
     if (!pulse_stream_valid(stream))
@@ -2079,7 +2161,7 @@ static NTSTATUS pulse_get_position(void *args)
 static NTSTATUS pulse_set_volumes(void *args)
 {
     struct set_volumes_params *params = args;
-    struct pulse_stream *stream = params->stream;
+    struct pulse_stream *stream = handle_get_stream(params->stream);
     unsigned int i;
 
     for (i = 0; i < stream->ss.channels; i++)
@@ -2091,7 +2173,7 @@ static NTSTATUS pulse_set_volumes(void *args)
 static NTSTATUS pulse_set_event_handle(void *args)
 {
     struct set_event_handle_params *params = args;
-    struct pulse_stream *stream = params->stream;
+    struct pulse_stream *stream = handle_get_stream(params->stream);
     HRESULT hr = S_OK;
 
     pulse_lock();
@@ -2112,7 +2194,7 @@ static NTSTATUS pulse_set_event_handle(void *args)
 static NTSTATUS pulse_is_started(void *args)
 {
     struct is_started_params *params = args;
-    struct pulse_stream *stream = params->stream;
+    struct pulse_stream *stream = handle_get_stream(params->stream);
 
     pulse_lock();
     params->started = pulse_stream_valid(stream) && stream->started;
diff --git a/dlls/winepulse.drv/unixlib.h b/dlls/winepulse.drv/unixlib.h
index 9089e2829fc..e09362f0751 100644
--- a/dlls/winepulse.drv/unixlib.h
+++ b/dlls/winepulse.drv/unixlib.h
@@ -21,8 +21,6 @@
 
 #define MAX_PULSE_NAME_LEN 256
 
-struct pulse_stream;
-
 enum phys_device_bus_type {
     phys_device_bus_invalid = -1,
     phys_device_bus_pci,
@@ -71,42 +69,42 @@ struct create_stream_params
     const WAVEFORMATEX *fmt;
     HRESULT result;
     UINT32 *channel_count;
-    struct pulse_stream **stream;
+    unsigned int *stream;
 };
 
 struct release_stream_params
 {
-    struct pulse_stream *stream;
+    unsigned int stream;
     HANDLE timer;
     HRESULT result;
 };
 
 struct start_params
 {
-    struct pulse_stream *stream;
+    unsigned int stream;
     HRESULT result;
 };
 
 struct stop_params
 {
-    struct pulse_stream *stream;
+    unsigned int stream;
     HRESULT result;
 };
 
 struct reset_params
 {
-    struct pulse_stream *stream;
+    unsigned int stream;
     HRESULT result;
 };
 
 struct timer_loop_params
 {
-    struct pulse_stream *stream;
+    unsigned int stream;
 };
 
 struct get_render_buffer_params
 {
-    struct pulse_stream *stream;
+    unsigned int stream;
     UINT32 frames;
     HRESULT result;
     BYTE **data;
@@ -114,7 +112,7 @@ struct get_render_buffer_params
 
 struct release_render_buffer_params
 {
-    struct pulse_stream *stream;
+    unsigned int stream;
     UINT32 written_frames;
     DWORD flags;
     HRESULT result;
@@ -122,7 +120,7 @@ struct release_render_buffer_params
 
 struct get_capture_buffer_params
 {
-    struct pulse_stream *stream;
+    unsigned int stream;
     HRESULT result;
     BYTE **data;
     UINT32 *frames;
@@ -133,49 +131,49 @@ struct get_capture_buffer_params
 
 struct release_capture_buffer_params
 {
-    struct pulse_stream *stream;
+    unsigned int stream;
     BOOL done;
     HRESULT result;
 };
 
 struct get_buffer_size_params
 {
-    struct pulse_stream *stream;
+    unsigned int stream;
     HRESULT result;
     UINT32 *size;
 };
 
 struct get_latency_params
 {
-    struct pulse_stream *stream;
+    unsigned int stream;
     HRESULT result;
     REFERENCE_TIME *latency;
 };
 
 struct get_current_padding_params
 {
-    struct pulse_stream *stream;
+    unsigned int stream;
     HRESULT result;
     UINT32 *padding;
 };
 
 struct get_next_packet_size_params
 {
-    struct pulse_stream *stream;
+    unsigned int stream;
     HRESULT result;
     UINT32 *frames;
 };
 
 struct get_frequency_params
 {
-    struct pulse_stream *stream;
+    unsigned int stream;
     HRESULT result;
     UINT64 *freq;
 };
 
 struct get_position_params
 {
-    struct pulse_stream *stream;
+    unsigned int stream;
     BOOL device;
     HRESULT result;
     UINT64 *pos;
@@ -184,7 +182,7 @@ struct get_position_params
 
 struct set_volumes_params
 {
-    struct pulse_stream *stream;
+    unsigned int stream;
     float master_volume;
     const float *volumes;
     const float *session_volumes;
@@ -192,7 +190,7 @@ struct set_volumes_params
 
 struct set_event_handle_params
 {
-    struct pulse_stream *stream;
+    unsigned int stream;
     HANDLE event;
     HRESULT result;
 };
@@ -206,7 +204,7 @@ struct test_connect_params
 
 struct is_started_params
 {
-    struct pulse_stream *stream;
+    unsigned int stream;
     BOOL started;
 };
 
-- 
2.25.1

[Jacek Caban]

Hi Huw,
> @@ -131,6 +143,73 @@ static void pulse_broadcast(void)
>       pthread_cond_broadcast(&pulse_cond);
>   }
>
> +static unsigned int handle_alloc(struct pulse_stream *stream)
> +{
> +    unsigned int index, count;
> +    struct stream_entry *new_handles;
> +
> +    pthread_mutex_lock(&handle_lock);
> +
> +    index = next_free;
> +    if(index != ~0u) next_free = handles[index].next_free;
> +    else if(next_unused < num_handles) index = next_unused++;
> +    else{
> +        count = max(num_handles * 2, 256);
> +        new_handles = realloc(handles, count);

realloc() size is missing multiplication by sizeof(*handles).


Other than that, the patch looks fine to me. One thing I'd mention for 
consideration: while proper handle table is the right thing for true 
kernel interface in general, I'm not sure if we need it in such cases. 
We could just make the handle 64-bit and use Unix pointer as a value of 
the handle. It would simplify things, but I can see how such "leak" of 
Unix pointers may be considered less elegant, so it's fine with me both 
ways.


Thanks,

Jacek

[Huw Davies]

Hi Jacek,

Thanks for looking at this.

On Thu, Apr 21, 2022 at 12:13:22PM +0200, Jacek Caban wrote:
> > +    else{
> > +        count = max(num_handles * 2, 256);
> > +        new_handles = realloc(handles, count);
>
> realloc() size is missing multiplication by sizeof(*handles).

Argh, good catch.  I'd even tested this by deliberately leaking
handles - I guess I got lucky (or rather, unlucky).
> Other than that, the patch looks fine to me. One thing I'd mention for
> consideration: while proper handle table is the right thing for true kernel
> interface in general, I'm not sure if we need it in such cases. We could
> just make the handle 64-bit and use Unix pointer as a value of the handle.
> It would simplify things, but I can see how such "leak" of Unix pointers may
> be considered less elegant, so it's fine with me both ways.

Yes, let's go with this.  It's simpler, which probably makes sense.

I've sent in v2 with this change and an additional patch to set
zero_bits for the Wow64 buffer allocations.

Thanks,
Huw.