From 0753e992b82adb46c1cc11a34225b6ed9f2c8335 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Barnab=C3=A1s=20P=C5=91cze?= Date: Wed, 17 Nov 2021 11:30:39 +0100 Subject: [PATCH] pulse-server: improve reference counting of samples Previously, when a sample is "committed" from an upload stream, its reference count is set to 1. This is problematic if, when the sample is committed for a second time, there are streams playing the sample as the reference count will go out of sync. The problem can be easily triggered, especially with longer samples: pactl upload-sample a-long-sample.ogg pactl play-sample a-long-sample pactl play-sample a-long-sample pactl upload-sample a-long-sample.ogg # while playing When the first stream finishes playing, it will free the sample, which can cause problems e.g. for the second stream playing the sample at that very moment. Fix that by decoupling the buffer from the sample and making the buffer reference counted. And remove the reference counting from the samples as it is no longer needed. Futhermore, previously, the reference counts were ignored when the removal of a sample was requested. That is fixed as well. The previous issue can be triggered easily as well: pactl upload-sample a-long-sample.ogg pactl play-sample a-long-sample pactl remove-sample a-long-sample # while playing Fixes #1953 --- .../module-protocol-pulse/pulse-server.c | 41 +++++++++++++------ .../module-protocol-pulse/sample-play.c | 13 +++--- src/modules/module-protocol-pulse/sample.h | 12 ++++++ 3 files changed, 48 insertions(+), 18 deletions(-) diff --git a/src/modules/module-protocol-pulse/pulse-server.c b/src/modules/module-protocol-pulse/pulse-server.c index 9fa0f2098..b308aa30a 100644 --- a/src/modules/module-protocol-pulse/pulse-server.c +++ b/src/modules/module-protocol-pulse/pulse-server.c @@ -2110,7 +2110,7 @@ static int do_finish_upload_stream(struct client *client, uint32_t command, uint { struct impl *impl = client->impl; uint32_t channel, event; - struct stream *stream = NULL; + struct stream *stream; struct sample *sample; const char *name; int res; @@ -2134,22 +2134,36 @@ static int do_finish_upload_stream(struct client *client, uint32_t command, uint client->name, commands[command].name, tag, channel, name); - sample = find_sample(impl, SPA_ID_INVALID, name); - if (sample == NULL) { - sample = calloc(1, sizeof(struct sample)); + struct sample *old = find_sample(impl, SPA_ID_INVALID, name); + if (old == NULL || (old != NULL && old->ref > 1)) { + sample = calloc(1, sizeof(*sample)); if (sample == NULL) goto error_errno; - sample->index = pw_map_insert_new(&impl->samples, sample); - if (sample->index == SPA_ID_INVALID) - goto error_errno; + if (old != NULL) { + sample->index = old->index; + spa_assert_se(pw_map_insert_at(&impl->samples, sample->index, sample) == 0); - event = SUBSCRIPTION_EVENT_NEW; + old->index = SPA_ID_INVALID; + sample_unref(old); + } else { + sample->index = pw_map_insert_new(&impl->samples, sample); + if (sample->index == SPA_ID_INVALID) + goto error_errno; + } } else { - pw_properties_free(sample->props); - free(sample->buffer); - event = SUBSCRIPTION_EVENT_CHANGE; + pw_properties_free(old->props); + free(old->buffer); + impl->stat.sample_cache -= old->length; + + sample = old; } + + if (old != NULL) + event = SUBSCRIPTION_EVENT_CHANGE; + else + event = SUBSCRIPTION_EVENT_NEW; + sample->ref = 1; sample->impl = impl; sample->name = name; @@ -2442,7 +2456,10 @@ static int do_remove_sample(struct client *client, uint32_t command, uint32_t ta SUBSCRIPTION_EVENT_SAMPLE_CACHE, sample->index); - sample_free(sample); + pw_map_remove(&impl->samples, sample->index); + sample->index = SPA_ID_INVALID; + + sample_unref(sample); return reply_simple_ack(client, tag); } diff --git a/src/modules/module-protocol-pulse/sample-play.c b/src/modules/module-protocol-pulse/sample-play.c index cc5a95011..b0554912c 100644 --- a/src/modules/module-protocol-pulse/sample-play.c +++ b/src/modules/module-protocol-pulse/sample-play.c @@ -77,12 +77,11 @@ static void sample_play_stream_destroy(void *data) struct sample_play *p = data; pw_log_info("destroy %s", p->sample->name); + spa_hook_remove(&p->listener); - - if (--p->sample->ref == 0) - sample_free(p->sample); - p->stream = NULL; + + sample_unref(p->sample); p->sample = NULL; } @@ -173,9 +172,11 @@ struct sample_play *sample_play_new(struct pw_core *core, goto error_free; } - p->sample = sample; + /* safe to increment the reference count here because it will be decreased + by the stream's 'destroy' event handler, which will be called + (even if `pw_stream_connect()` fails) */ + p->sample = sample_ref(sample); p->stride = sample_spec_frame_size(&sample->ss); - sample->ref++; pw_stream_add_listener(p->stream, &p->listener, diff --git a/src/modules/module-protocol-pulse/sample.h b/src/modules/module-protocol-pulse/sample.h index 424ac57f8..db347eb9f 100644 --- a/src/modules/module-protocol-pulse/sample.h +++ b/src/modules/module-protocol-pulse/sample.h @@ -46,4 +46,16 @@ struct sample { void sample_free(struct sample *sample); +static inline struct sample *sample_ref(struct sample *sample) +{ + sample->ref++; + return sample; +} + +static inline void sample_unref(struct sample *sample) +{ + if (--sample->ref == 0) + sample_free(sample); +} + #endif /* PULSE_SERVER_SAMPLE_H */