From 36e036db228a9a8b8bff084ec7c5d5bf232b0eb4 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Tue, 19 Jan 2021 23:05:22 +0100 Subject: [PATCH 1/8] bluetooth: Do not re-retrieve codecs in switch_codec The desired list of capabilities is provided by the caller; if an endpoint is successfully selected that endpoint resides in this hashmap. Perhaps choose_remote_endpoint should return the key-value pair instead? --- src/modules/bluetooth/bluez5-util.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/modules/bluetooth/bluez5-util.c b/src/modules/bluetooth/bluez5-util.c index 2431b3831..76e20f487 100644 --- a/src/modules/bluetooth/bluez5-util.c +++ b/src/modules/bluetooth/bluez5-util.c @@ -447,7 +447,6 @@ bool pa_bluetooth_device_switch_codec(pa_bluetooth_device *device, pa_bluetooth_ uint8_t config[MAX_A2DP_CAPS_SIZE]; uint8_t config_size; bool is_a2dp_sink; - pa_hashmap *all_endpoints; char *pa_endpoint; const char *endpoint; @@ -462,13 +461,8 @@ bool pa_bluetooth_device_switch_codec(pa_bluetooth_device *device, pa_bluetooth_ is_a2dp_sink = profile == PA_BLUETOOTH_PROFILE_A2DP_SINK; - all_endpoints = NULL; - all_endpoints = pa_hashmap_get(is_a2dp_sink ? device->a2dp_sink_endpoints : device->a2dp_source_endpoints, - &endpoint_conf->id); - pa_assert(all_endpoints); - pa_assert_se(endpoint = endpoint_conf->choose_remote_endpoint(capabilities_hashmap, &device->discovery->core->default_sample_spec, is_a2dp_sink)); - pa_assert_se(capabilities = pa_hashmap_get(all_endpoints, endpoint)); + pa_assert_se(capabilities = pa_hashmap_get(capabilities_hashmap, endpoint)); config_size = endpoint_conf->fill_preferred_configuration(&device->discovery->core->default_sample_spec, capabilities->buffer, capabilities->size, config); From 90fd392101ee56b35e2dc0bb723d2b589f04baca Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Sun, 24 Jan 2021 15:09:17 +0100 Subject: [PATCH 2/8] bluetooth: Rename default_sample_spec to spec This is not always the default; rather what is requested based on application. --- src/modules/bluetooth/a2dp-codec-api.h | 4 ++-- src/modules/bluetooth/a2dp-codec-aptx-gst.c | 16 ++++++++-------- src/modules/bluetooth/a2dp-codec-ldac-gst.c | 10 +++++----- src/modules/bluetooth/a2dp-codec-sbc.c | 8 ++++---- 4 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/modules/bluetooth/a2dp-codec-api.h b/src/modules/bluetooth/a2dp-codec-api.h index e46981836..90d8d38f4 100644 --- a/src/modules/bluetooth/a2dp-codec-api.h +++ b/src/modules/bluetooth/a2dp-codec-api.h @@ -52,13 +52,13 @@ typedef struct pa_a2dp_endpoint_conf { * (const char *endpoint -> const pa_a2dp_codec_capabilities *capability) * and returns corresponding endpoint key (or NULL when there is no valid), * for_encoder is true when capabilities hash map is used for encoding */ - const char *(*choose_remote_endpoint)(const pa_hashmap *capabilities_hashmap, const pa_sample_spec *default_sample_spec, bool for_encoding); + const char *(*choose_remote_endpoint)(const pa_hashmap *capabilities_hashmap, const pa_sample_spec *spec, bool for_encoding); /* Fill codec capabilities, returns size of filled buffer */ uint8_t (*fill_capabilities)(uint8_t capabilities_buffer[MAX_A2DP_CAPS_SIZE]); /* Validate codec configuration, returns true on success */ bool (*is_configuration_valid)(const uint8_t *config_buffer, uint8_t config_size); /* Fill preferred codec configuration, returns size of filled buffer or 0 on failure */ - uint8_t (*fill_preferred_configuration)(const pa_sample_spec *default_sample_spec, const uint8_t *capabilities_buffer, uint8_t capabilities_size, uint8_t config_buffer[MAX_A2DP_CAPS_SIZE]); + uint8_t (*fill_preferred_configuration)(const pa_sample_spec *spec, const uint8_t *capabilities_buffer, uint8_t capabilities_size, uint8_t config_buffer[MAX_A2DP_CAPS_SIZE]); /* Bluetooth codec */ pa_bt_codec bt_codec; diff --git a/src/modules/bluetooth/a2dp-codec-aptx-gst.c b/src/modules/bluetooth/a2dp-codec-aptx-gst.c index d0573e3b2..322ea7e4a 100644 --- a/src/modules/bluetooth/a2dp-codec-aptx-gst.c +++ b/src/modules/bluetooth/a2dp-codec-aptx-gst.c @@ -89,7 +89,7 @@ static bool can_accept_capabilities_hd(const uint8_t *capabilities_buffer, uint8 return can_accept_capabilities_common(&capabilities->aptx, APTX_HD_VENDOR_ID, APTX_HD_CODEC_ID); } -static const char *choose_remote_endpoint(const pa_hashmap *capabilities_hashmap, const pa_sample_spec *default_sample_spec, bool for_encoding) { +static const char *choose_remote_endpoint(const pa_hashmap *capabilities_hashmap, const pa_sample_spec *spec, bool for_encoding) { const pa_a2dp_codec_capabilities *a2dp_capabilities; const char *key; void *state; @@ -103,7 +103,7 @@ static const char *choose_remote_endpoint(const pa_hashmap *capabilities_hashmap return NULL; } -static const char *choose_remote_endpoint_hd(const pa_hashmap *capabilities_hashmap, const pa_sample_spec *default_sample_spec, bool for_encoding) { +static const char *choose_remote_endpoint_hd(const pa_hashmap *capabilities_hashmap, const pa_sample_spec *spec, bool for_encoding) { const pa_a2dp_codec_capabilities *a2dp_capabilities; const char *key; void *state; @@ -182,7 +182,7 @@ static bool is_configuration_valid_hd(const uint8_t *config_buffer, uint8_t conf return is_configuration_valid_common(&config->aptx, APTX_HD_VENDOR_ID, APTX_HD_CODEC_ID); } -static int fill_preferred_configuration_common(const pa_sample_spec *default_sample_spec, const a2dp_aptx_t *capabilities, a2dp_aptx_t *config, uint32_t vendor_id, uint16_t codec_id) { +static int fill_preferred_configuration_common(const pa_sample_spec *spec, const a2dp_aptx_t *capabilities, a2dp_aptx_t *config, uint32_t vendor_id, uint16_t codec_id) { int i; static const struct { @@ -211,7 +211,7 @@ static int fill_preferred_configuration_common(const pa_sample_spec *default_sam /* Find the lowest freq that is at least as high as the requested sampling rate */ for (i = 0; (unsigned) i < PA_ELEMENTSOF(freq_table); i++) { - if (freq_table[i].rate >= default_sample_spec->rate && (capabilities->frequency & freq_table[i].cap)) { + if (freq_table[i].rate >= spec->rate && (capabilities->frequency & freq_table[i].cap)) { config->frequency = freq_table[i].cap; break; } @@ -234,7 +234,7 @@ static int fill_preferred_configuration_common(const pa_sample_spec *default_sam return 0; } -static uint8_t fill_preferred_configuration(const pa_sample_spec *default_sample_spec, const uint8_t *capabilities_buffer, uint8_t capabilities_size, uint8_t config_buffer[MAX_A2DP_CAPS_SIZE]) { +static uint8_t fill_preferred_configuration(const pa_sample_spec *spec, const uint8_t *capabilities_buffer, uint8_t capabilities_size, uint8_t config_buffer[MAX_A2DP_CAPS_SIZE]) { a2dp_aptx_t *config = (a2dp_aptx_t *) config_buffer; const a2dp_aptx_t *capabilities = (const a2dp_aptx_t *) capabilities_buffer; @@ -245,13 +245,13 @@ static uint8_t fill_preferred_configuration(const pa_sample_spec *default_sample pa_zero(*config); - if (fill_preferred_configuration_common(default_sample_spec, capabilities, config, APTX_VENDOR_ID, APTX_CODEC_ID) < 0) + if (fill_preferred_configuration_common(spec, capabilities, config, APTX_VENDOR_ID, APTX_CODEC_ID) < 0) return 0; return sizeof(*config); } -static uint8_t fill_preferred_configuration_hd(const pa_sample_spec *default_sample_spec, const uint8_t *capabilities_buffer, uint8_t capabilities_size, uint8_t config_buffer[MAX_A2DP_CAPS_SIZE]) { +static uint8_t fill_preferred_configuration_hd(const pa_sample_spec *spec, const uint8_t *capabilities_buffer, uint8_t capabilities_size, uint8_t config_buffer[MAX_A2DP_CAPS_SIZE]) { a2dp_aptx_hd_t *config = (a2dp_aptx_hd_t *) config_buffer; const a2dp_aptx_hd_t *capabilities = (const a2dp_aptx_hd_t *) capabilities_buffer; @@ -262,7 +262,7 @@ static uint8_t fill_preferred_configuration_hd(const pa_sample_spec *default_sam pa_zero(*config); - if (fill_preferred_configuration_common(default_sample_spec, &capabilities->aptx, &config->aptx, APTX_HD_VENDOR_ID, APTX_HD_CODEC_ID) < 0) + if (fill_preferred_configuration_common(spec, &capabilities->aptx, &config->aptx, APTX_HD_VENDOR_ID, APTX_HD_CODEC_ID) < 0) return 0; return sizeof(*config); diff --git a/src/modules/bluetooth/a2dp-codec-ldac-gst.c b/src/modules/bluetooth/a2dp-codec-ldac-gst.c index dfefbf186..63807af57 100644 --- a/src/modules/bluetooth/a2dp-codec-ldac-gst.c +++ b/src/modules/bluetooth/a2dp-codec-ldac-gst.c @@ -72,7 +72,7 @@ static bool can_accept_capabilities(const uint8_t *capabilities_buffer, uint8_t return can_accept_capabilities_common(capabilities, LDAC_VENDOR_ID, LDAC_CODEC_ID); } -static const char *choose_remote_endpoint(const pa_hashmap *capabilities_hashmap, const pa_sample_spec *default_sample_spec, bool for_encoding) { +static const char *choose_remote_endpoint(const pa_hashmap *capabilities_hashmap, const pa_sample_spec *spec, bool for_encoding) { const pa_a2dp_codec_capabilities *a2dp_capabilities; const char *key; void *state; @@ -128,7 +128,7 @@ static bool is_configuration_valid(const uint8_t *config_buffer, uint8_t config_ return true; } -static int fill_preferred_configuration_common(const pa_sample_spec *default_sample_spec, const a2dp_ldac_t *capabilities, a2dp_ldac_t *config, uint32_t vendor_id, uint16_t codec_id) { +static int fill_preferred_configuration_common(const pa_sample_spec *spec, const a2dp_ldac_t *capabilities, a2dp_ldac_t *config, uint32_t vendor_id, uint16_t codec_id) { int i; static const struct { @@ -157,7 +157,7 @@ static int fill_preferred_configuration_common(const pa_sample_spec *default_sam /* Find the lowest freq that is at least as high as the requested sampling rate */ for (i = 0; (unsigned) i < PA_ELEMENTSOF(freq_table); i++) { - if (freq_table[i].rate >= default_sample_spec->rate && (capabilities->frequency & freq_table[i].cap)) { + if (freq_table[i].rate >= spec->rate && (capabilities->frequency & freq_table[i].cap)) { config->frequency = freq_table[i].cap; break; } @@ -180,7 +180,7 @@ static int fill_preferred_configuration_common(const pa_sample_spec *default_sam return 0; } -static uint8_t fill_preferred_configuration(const pa_sample_spec *default_sample_spec, const uint8_t *capabilities_buffer, uint8_t capabilities_size, uint8_t config_buffer[MAX_A2DP_CAPS_SIZE]) { +static uint8_t fill_preferred_configuration(const pa_sample_spec *spec, const uint8_t *capabilities_buffer, uint8_t capabilities_size, uint8_t config_buffer[MAX_A2DP_CAPS_SIZE]) { a2dp_ldac_t *config = (a2dp_ldac_t *) config_buffer; const a2dp_ldac_t *capabilities = (const a2dp_ldac_t *) capabilities_buffer; @@ -191,7 +191,7 @@ static uint8_t fill_preferred_configuration(const pa_sample_spec *default_sample pa_zero(*config); - if (fill_preferred_configuration_common(default_sample_spec, capabilities, config, LDAC_VENDOR_ID, LDAC_CODEC_ID) < 0) + if (fill_preferred_configuration_common(spec, capabilities, config, LDAC_VENDOR_ID, LDAC_CODEC_ID) < 0) return 0; return sizeof(*config); diff --git a/src/modules/bluetooth/a2dp-codec-sbc.c b/src/modules/bluetooth/a2dp-codec-sbc.c index 5476697f8..f89bac435 100644 --- a/src/modules/bluetooth/a2dp-codec-sbc.c +++ b/src/modules/bluetooth/a2dp-codec-sbc.c @@ -134,7 +134,7 @@ static bool can_accept_capabilities_faststream(const uint8_t *capabilities_buffe return true; } -static const char *choose_remote_endpoint(const pa_hashmap *capabilities_hashmap, const pa_sample_spec *default_sample_spec, bool for_encoding) { +static const char *choose_remote_endpoint(const pa_hashmap *capabilities_hashmap, const pa_sample_spec *spec, bool for_encoding) { const pa_a2dp_codec_capabilities *a2dp_capabilities; const char *key; void *state; @@ -495,7 +495,7 @@ static uint8_t default_bitpool(uint8_t freq, uint8_t mode) { pa_assert_not_reached(); } -static uint8_t fill_preferred_configuration(const pa_sample_spec *default_sample_spec, const uint8_t *capabilities_buffer, uint8_t capabilities_size, uint8_t config_buffer[MAX_A2DP_CAPS_SIZE]) { +static uint8_t fill_preferred_configuration(const pa_sample_spec *spec, const uint8_t *capabilities_buffer, uint8_t capabilities_size, uint8_t config_buffer[MAX_A2DP_CAPS_SIZE]) { a2dp_sbc_t *config = (a2dp_sbc_t *) config_buffer; const a2dp_sbc_t *capabilities = (const a2dp_sbc_t *) capabilities_buffer; int i; @@ -519,7 +519,7 @@ static uint8_t fill_preferred_configuration(const pa_sample_spec *default_sample /* Find the lowest freq that is at least as high as the requested sampling rate */ for (i = 0; (unsigned) i < PA_ELEMENTSOF(freq_table); i++) - if (freq_table[i].rate >= default_sample_spec->rate && (capabilities->frequency & freq_table[i].cap)) { + if (freq_table[i].rate >= spec->rate && (capabilities->frequency & freq_table[i].cap)) { config->frequency = freq_table[i].cap; break; } @@ -540,7 +540,7 @@ static uint8_t fill_preferred_configuration(const pa_sample_spec *default_sample pa_assert((unsigned) i < PA_ELEMENTSOF(freq_table)); - if (default_sample_spec->channels <= 1) { + if (spec->channels <= 1) { if (capabilities->channel_mode & SBC_CHANNEL_MODE_MONO) config->channel_mode = SBC_CHANNEL_MODE_MONO; else if (capabilities->channel_mode & SBC_CHANNEL_MODE_JOINT_STEREO) From f70a1672c66c9341de8df0354e4fc90861991100 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Sun, 24 Jan 2021 15:40:01 +0100 Subject: [PATCH 3/8] bluetooth: set u->transport null when it has been freed Prevent a pointer to freed memory from lingering around. --- src/modules/bluetooth/module-bluez5-device.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c index ab1bc098e..55f0c31e6 100644 --- a/src/modules/bluetooth/module-bluez5-device.c +++ b/src/modules/bluetooth/module-bluez5-device.c @@ -2439,6 +2439,9 @@ static pa_hook_result_t transport_state_changed_cb(pa_bluetooth_discovery *y, pa if (t->device == u->device) handle_transport_state_change(u, t); + if (t == u->transport && t->state <= PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED) + u->transport = NULL; + return PA_HOOK_OK; } From 856d2b8af80b5b8fb215e08e6b90bcd0c95e8c86 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Sun, 24 Jan 2021 16:53:28 +0100 Subject: [PATCH 4/8] bluetooth: Switch codecs without tearing down sink/source/thread The main source of routing audio to another sink on codec switch is the PA sink disappearing, together with its IO thread and other primitives. This is annoying for users but also wasteful; all we need to do for a codec switch is make the sink temporarily unavailable, release the transport, perform the usual `SetConfiguration` on another `SEP` and `Acquire` the resulting stream that is made available, to finally reconfigure the sink with the new sampling rate and format and make the sink available again. --- src/modules/bluetooth/module-bluez5-device.c | 119 ++++++++++++------- 1 file changed, 73 insertions(+), 46 deletions(-) diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c index 55f0c31e6..6ebb9a98c 100644 --- a/src/modules/bluetooth/module-bluez5-device.c +++ b/src/modules/bluetooth/module-bluez5-device.c @@ -1086,7 +1086,6 @@ static int sink_process_msg(pa_msgobject *o, int code, void *data, int64_t offse struct userdata *u = PA_SINK(o)->userdata; pa_assert(u->sink == PA_SINK(o)); - pa_assert(u->transport); switch (code) { @@ -1116,6 +1115,8 @@ static int sink_process_msg(pa_msgobject *o, int code, void *data, int64_t offse } case PA_SINK_MESSAGE_SETUP_STREAM: + pa_assert(u->transport); + /* Skip stream setup if stream_fd has been invalidated. This can occur if the stream has already been set up and then immediately received POLLHUP. If the stream has @@ -1809,6 +1810,37 @@ static int start_thread(struct userdata *u) { return 0; } +static void release_codec(struct userdata *u) { + if (u->bt_codec) { + if (u->encoder_info) { + u->bt_codec->deinit(u->encoder_info); + u->encoder_info = NULL; + } + + if (u->decoder_info) { + u->bt_codec->deinit(u->decoder_info); + u->decoder_info = NULL; + } + + u->bt_codec = NULL; + } + + if (u->encoder_buffer) { + pa_xfree(u->encoder_buffer); + u->encoder_buffer = NULL; + } + + u->encoder_buffer_size = 0; + u->encoder_buffer_used = 0; + + if (u->decoder_buffer) { + pa_xfree(u->decoder_buffer); + u->decoder_buffer = NULL; + } + + u->decoder_buffer_size = 0; +} + /* Run from main thread */ static void stop_thread(struct userdata *u) { pa_assert(u); @@ -1873,34 +1905,7 @@ static void stop_thread(struct userdata *u) { u->read_smoother = NULL; } - if (u->bt_codec) { - if (u->encoder_info) { - u->bt_codec->deinit(u->encoder_info); - u->encoder_info = NULL; - } - - if (u->decoder_info) { - u->bt_codec->deinit(u->decoder_info); - u->decoder_info = NULL; - } - - u->bt_codec = NULL; - } - - if (u->encoder_buffer) { - pa_xfree(u->encoder_buffer); - u->encoder_buffer = NULL; - } - - u->encoder_buffer_size = 0; - u->encoder_buffer_used = 0; - - if (u->decoder_buffer) { - pa_xfree(u->decoder_buffer); - u->decoder_buffer = NULL; - } - - u->decoder_buffer_size = 0; + release_codec(u); } /* Run from main thread */ @@ -2326,11 +2331,13 @@ static void handle_transport_state_change(struct userdata *u, struct pa_bluetoot oldavail = cp->available; /* - * If codec switching is in progress, transport state change should not - * make profile unavailable. + * If codec switching is in progress, do not acquire the transport until our + * "codec switch complete" callback is called, which calls transport_config + * in addition to just acquiring it (through setup_transport). That means it + * also has to apply any eventual format changes back to the pa_sink/source. */ - if (!t->device->codec_switching_in_progress) - pa_card_profile_set_available(cp, transport_state_to_availability(t->state)); + if (t->device->codec_switching_in_progress) + return; /* Update port availability */ pa_assert_se(port = pa_hashmap_get(u->card->ports, u->output_port_name)); @@ -2433,7 +2440,7 @@ static pa_hook_result_t transport_state_changed_cb(pa_bluetooth_discovery *y, pa pa_assert(t); pa_assert(u); - if (t == u->transport && t->state <= PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED) + if (t == u->transport && t->state <= PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED && !t->device->codec_switching_in_progress) pa_assert_se(pa_card_set_profile(u->card, pa_hashmap_get(u->card->profiles, "off"), false) >= 0); if (t->device == u->device) @@ -2506,25 +2513,36 @@ static char* make_message_handler_path(const char *name) { return pa_sprintf_malloc("/card/%s/bluez", name); } -static void switch_codec_cb_handler(bool success, pa_bluetooth_profile_t profile, void *userdata) -{ +static void switch_codec_cb_handler(bool success, pa_bluetooth_profile_t profile, void *userdata) { struct userdata *u = (struct userdata *) userdata; + bool is_a2dp_sink; + int r; if (!success) goto off; u->profile = profile; + is_a2dp_sink = u->profile == PA_BLUETOOTH_PROFILE_A2DP_SINK; - if (init_profile(u) < 0) { - pa_log_info("Failed to initialise profile after codec switching"); - goto off; - } + // SUSPEND callbacks release the transport and free the codec, but do not reinitialize it yet. + // Acquire the transport (could have been done in the callbacks when leaving SUSPEND) but + // _also_ reinitialize the codec with transport_config here: - if (u->sink || u->source) - if (start_thread(u) < 0) { - pa_log_info("Failed to start thread after codec switching"); - goto off; - } + r = setup_transport(u); + pa_assert(!r); + // if (r == -EINPROGRESS) + // return 0; + // else if (r < 0) + // return -1; + + // TODO: If we were to call this, that'd end in our ->reconfigure handler. + // pa_sink_reconfigure() + // That's where we need to (eventually) update the current codec and finally commit the rate change + + if (is_a2dp_sink) + pa_sink_suspend(u->sink, false, PA_SUSPEND_INTERNAL); + else + pa_source_suspend(u->source, false, PA_SUSPEND_INTERNAL); pa_log_info("Codec successfully switched to %s with profile: %s", u->bt_codec->name, pa_bluetooth_profile_to_string(u->profile)); @@ -2532,6 +2550,7 @@ static void switch_codec_cb_handler(bool success, pa_bluetooth_profile_t profile return; off: + stop_thread(u); pa_assert_se(pa_card_set_profile(u->card, pa_hashmap_get(u->card->profiles, "off"), false) >= 0); } @@ -2695,7 +2714,14 @@ static int bluez5_device_message_handler(const char *object_path, const char *me */ profile = u->profile; - stop_thread(u); + // Suspending releases the transport + if (is_a2dp_sink) + pa_sink_suspend(u->sink, true, PA_SUSPEND_INTERNAL); + else + pa_source_suspend(u->source, true, PA_SUSPEND_INTERNAL); + + // TODO: Can we do this if it's synchronous? + release_codec(u); if (!pa_bluetooth_device_switch_codec(u->device, profile, capabilities_hashmap, endpoint_conf, switch_codec_cb_handler, userdata) && !u->device->codec_switching_in_progress) @@ -2723,6 +2749,7 @@ static int bluez5_device_message_handler(const char *object_path, const char *me return -PA_ERR_NOTIMPLEMENTED; profile_off: + stop_thread(u); pa_assert_se(pa_card_set_profile(u->card, pa_hashmap_get(u->card->profiles, "off"), false) >= 0); return -PA_ERR_IO; From 7fb7ed521e22fd4322ed87a17e374186411da2a9 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Sun, 24 Jan 2021 19:20:21 +0100 Subject: [PATCH 5/8] bluetooth: Use pa_sink/source_reconfigure to update resamplers This unfortunately requires us to manually (un)cork inputs/outputs so that _update_resampler is called on them. It is anyway fine to cork these as their sink/source is temporarily idle to perform a codec switch. --- src/modules/bluetooth/module-bluez5-device.c | 72 ++++++++++++++++++-- 1 file changed, 67 insertions(+), 5 deletions(-) diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c index 6ebb9a98c..9f6f66439 100644 --- a/src/modules/bluetooth/module-bluez5-device.c +++ b/src/modules/bluetooth/module-bluez5-device.c @@ -929,6 +929,24 @@ static int source_set_state_in_io_thread_cb(pa_source *s, pa_source_state_t new_ return 0; } +/* Run from main thread */ +static void source_reconfigure_cb(pa_source *s, pa_sample_spec *spec, bool passthrough) { + struct userdata *u; + + pa_assert(s); + pa_assert_se(u = s->userdata); + + if (!pa_sample_spec_equal(spec, &u->decoder_sample_spec)) { + pa_log_warn("Reconfiguration not yet supported"); + return; + } + + pa_source_set_sample_format(u->source, spec->format); + pa_source_set_sample_rate(u->source, spec->rate); + // Does not allow reconfiguring channels yet. + pa_assert(u->source->sample_spec.channels == spec->channels); +} + /* Run from main thread */ static void source_set_volume_cb(pa_source *s) { pa_volume_t volume; @@ -1075,6 +1093,7 @@ static int add_source(struct userdata *u) { u->source->userdata = u; u->source->parent.process_msg = source_process_msg; u->source->set_state_in_io_thread = source_set_state_in_io_thread_cb; + u->source->reconfigure = source_reconfigure_cb; source_setup_volume_callback(u->source); @@ -1178,6 +1197,24 @@ static int sink_set_state_in_io_thread_cb(pa_sink *s, pa_sink_state_t new_state, return 0; } +/* Run from main thread */ +static void sink_reconfigure_cb(pa_sink *s, pa_sample_spec *spec, bool passthrough) { + struct userdata *u; + + pa_assert(s); + pa_assert_se(u = s->userdata); + + if (!pa_sample_spec_equal(spec, &u->encoder_sample_spec)) { + pa_log_warn("Reconfiguration not yet supported"); + return; + } + + pa_sink_set_sample_format(u->sink, spec->format); + pa_sink_set_sample_rate(u->sink, spec->rate); + // Does not allow reconfiguring channels yet. + pa_assert(u->sink->sample_spec.channels == spec->channels); +} + /* Run from main thread */ static void sink_set_volume_cb(pa_sink *s) { pa_volume_t volume; @@ -1322,6 +1359,7 @@ static int add_sink(struct userdata *u) { u->sink->userdata = u; u->sink->parent.process_msg = sink_process_msg; u->sink->set_state_in_io_thread = sink_set_state_in_io_thread_cb; + u->sink->reconfigure = sink_reconfigure_cb; sink_setup_volume_callback(u->sink); @@ -2515,6 +2553,7 @@ static char* make_message_handler_path(const char *name) { static void switch_codec_cb_handler(bool success, pa_bluetooth_profile_t profile, void *userdata) { struct userdata *u = (struct userdata *) userdata; + pa_sample_spec *spec; bool is_a2dp_sink; int r; @@ -2535,14 +2574,37 @@ static void switch_codec_cb_handler(bool success, pa_bluetooth_profile_t profile // else if (r < 0) // return -1; - // TODO: If we were to call this, that'd end in our ->reconfigure handler. - // pa_sink_reconfigure() - // That's where we need to (eventually) update the current codec and finally commit the rate change + spec = is_a2dp_sink ? &u->encoder_sample_spec : &u->decoder_sample_spec; + + // Finish off by reconfiguring + + // TODO HACK: Also cork/uncork sources so that the resampler is updated + + if (is_a2dp_sink) { + pa_sink_input *i; + uint32_t idx; + PA_IDXSET_FOREACH(i, u->sink->inputs, idx) { + pa_sink_input_cork(i, true); + } + pa_sink_reconfigure(u->sink, spec, true); + PA_IDXSET_FOREACH(i, u->sink->inputs, idx) { + pa_sink_input_cork(i, false); + } - if (is_a2dp_sink) pa_sink_suspend(u->sink, false, PA_SUSPEND_INTERNAL); - else + } else { + pa_source_output *i; + uint32_t idx; + PA_IDXSET_FOREACH(i, u->source->outputs, idx) { + pa_source_output_cork(i, true); + } + pa_source_reconfigure(u->source, spec, true); + PA_IDXSET_FOREACH(i, u->source->outputs, idx) { + pa_source_output_cork(i, false); + } + pa_source_suspend(u->source, false, PA_SUSPEND_INTERNAL); + } pa_log_info("Codec successfully switched to %s with profile: %s", u->bt_codec->name, pa_bluetooth_profile_to_string(u->profile)); From 54ddc5425467180008beeb9323a034a6f0c04545 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Sun, 24 Jan 2021 23:35:41 +0100 Subject: [PATCH 6/8] fixup! bluetooth: Switch codecs without tearing down sink/source/thread --- src/modules/bluetooth/module-bluez5-device.c | 42 +++++++++++++++----- 1 file changed, 32 insertions(+), 10 deletions(-) diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c index 9f6f66439..102704e6e 100644 --- a/src/modules/bluetooth/module-bluez5-device.c +++ b/src/modules/bluetooth/module-bluez5-device.c @@ -893,6 +893,13 @@ static int source_set_state_in_io_thread_cb(pa_source *s, pa_source_state_t new_ if (!PA_SOURCE_IS_OPENED(s->thread_info.state)) break; + /* Ignore the transition if the source is suspended for internal reasons + * such as external source-output changes resulting in pa_source_reconfigure. + * Logic in our handler decides whether to release the transport or not. + */ + if (new_suspend_cause & PA_SUSPEND_INTERNAL) + break; + /* Stop the device if the sink is suspended as well */ if (!u->sink || u->sink->state == PA_SINK_SUSPENDED) transport_release(u); @@ -910,6 +917,9 @@ static int source_set_state_in_io_thread_cb(pa_source *s, pa_source_state_t new_ if (s->thread_info.state != PA_SOURCE_SUSPENDED) break; + if (s->suspend_cause & PA_SUSPEND_INTERNAL) + break; + /* Resume the device if the sink was suspended as well */ if (!u->sink || !PA_SINK_IS_OPENED(u->sink->thread_info.state)) if (!setup_transport_and_stream(u)) @@ -1167,6 +1177,13 @@ static int sink_set_state_in_io_thread_cb(pa_sink *s, pa_sink_state_t new_state, if (!PA_SINK_IS_OPENED(s->thread_info.state)) break; + /* Ignore the transition if the sink is suspended for internal reasons + * such as external sink-input changes resulting in pa_sink_reconfigure. + * Logic in our handler decides whether to release the transport or not. + */ + if (new_suspend_cause & PA_SUSPEND_INTERNAL) + break; + /* Stop the device if the source is suspended as well */ if (!u->source || u->source->state == PA_SOURCE_SUSPENDED) /* We deliberately ignore whether stopping @@ -1181,6 +1198,9 @@ static int sink_set_state_in_io_thread_cb(pa_sink *s, pa_sink_state_t new_state, if (s->thread_info.state != PA_SINK_SUSPENDED) break; + if (s->suspend_cause & PA_SUSPEND_INTERNAL) + break; + /* Resume the device if the source was suspended as well */ if (!u->source || !PA_SOURCE_IS_OPENED(u->source->thread_info.state)) if (!setup_transport_and_stream(u)) @@ -2563,10 +2583,7 @@ static void switch_codec_cb_handler(bool success, pa_bluetooth_profile_t profile u->profile = profile; is_a2dp_sink = u->profile == PA_BLUETOOTH_PROFILE_A2DP_SINK; - // SUSPEND callbacks release the transport and free the codec, but do not reinitialize it yet. - // Acquire the transport (could have been done in the callbacks when leaving SUSPEND) but - // _also_ reinitialize the codec with transport_config here: - + // Reacquire the transport and configure the codec r = setup_transport(u); pa_assert(!r); // if (r == -EINPROGRESS) @@ -2578,11 +2595,10 @@ static void switch_codec_cb_handler(bool success, pa_bluetooth_profile_t profile // Finish off by reconfiguring - // TODO HACK: Also cork/uncork sources so that the resampler is updated - if (is_a2dp_sink) { pa_sink_input *i; uint32_t idx; + PA_IDXSET_FOREACH(i, u->sink->inputs, idx) { pa_sink_input_cork(i, true); } @@ -2591,10 +2607,13 @@ static void switch_codec_cb_handler(bool success, pa_bluetooth_profile_t profile pa_sink_input_cork(i, false); } - pa_sink_suspend(u->sink, false, PA_SUSPEND_INTERNAL); + pa_asyncmsgq_send(u->sink->asyncmsgq, PA_MSGOBJECT(u->sink), PA_SINK_MESSAGE_SETUP_STREAM, NULL, 0, NULL); + + pa_sink_suspend(u->sink, false, PA_SUSPEND_UNAVAILABLE); } else { pa_source_output *i; uint32_t idx; + PA_IDXSET_FOREACH(i, u->source->outputs, idx) { pa_source_output_cork(i, true); } @@ -2603,7 +2622,9 @@ static void switch_codec_cb_handler(bool success, pa_bluetooth_profile_t profile pa_source_output_cork(i, false); } - pa_source_suspend(u->source, false, PA_SUSPEND_INTERNAL); + pa_asyncmsgq_send(u->source->asyncmsgq, PA_MSGOBJECT(u->source), PA_SOURCE_MESSAGE_SETUP_STREAM, NULL, 0, NULL); + + pa_source_suspend(u->source, false, PA_SUSPEND_UNAVAILABLE); } pa_log_info("Codec successfully switched to %s with profile: %s", @@ -2777,10 +2798,11 @@ static int bluez5_device_message_handler(const char *object_path, const char *me profile = u->profile; // Suspending releases the transport + if (is_a2dp_sink) - pa_sink_suspend(u->sink, true, PA_SUSPEND_INTERNAL); + pa_sink_suspend(u->sink, true, PA_SUSPEND_UNAVAILABLE); else - pa_source_suspend(u->source, true, PA_SUSPEND_INTERNAL); + pa_source_suspend(u->source, true, PA_SUSPEND_UNAVAILABLE); // TODO: Can we do this if it's synchronous? release_codec(u); From 616017ca86edcba6f8a29c17a953932ef538fe2b Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Sat, 30 Jan 2021 17:37:41 +0100 Subject: [PATCH 7/8] fixup: bluetooth: Use avoid_resampling instead of passthrough to force callback --- src/modules/bluetooth/module-bluez5-device.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c index 102704e6e..3db2c5876 100644 --- a/src/modules/bluetooth/module-bluez5-device.c +++ b/src/modules/bluetooth/module-bluez5-device.c @@ -1060,6 +1060,7 @@ static int add_source(struct userdata *u) { if (u->bt_codec) pa_proplist_sets(data.proplist, PA_PROP_BLUETOOTH_CODEC, u->bt_codec->name); pa_source_new_data_set_sample_spec(&data, &u->decoder_sample_spec); + pa_source_new_data_set_avoid_resampling(&data, true); if (u->profile == PA_BLUETOOTH_PROFILE_HSP_HS || u->profile == PA_BLUETOOTH_PROFILE_HFP_HF) pa_proplist_sets(data.proplist, PA_PROP_DEVICE_INTENDED_ROLES, "phone"); @@ -1340,6 +1341,7 @@ static int add_sink(struct userdata *u) { if (u->bt_codec) pa_proplist_sets(data.proplist, PA_PROP_BLUETOOTH_CODEC, u->bt_codec->name); pa_sink_new_data_set_sample_spec(&data, &u->encoder_sample_spec); + pa_sink_new_data_set_avoid_resampling(&data, true); if (u->profile == PA_BLUETOOTH_PROFILE_HSP_HS || u->profile == PA_BLUETOOTH_PROFILE_HFP_HF) pa_proplist_sets(data.proplist, PA_PROP_DEVICE_INTENDED_ROLES, "phone"); @@ -2602,7 +2604,7 @@ static void switch_codec_cb_handler(bool success, pa_bluetooth_profile_t profile PA_IDXSET_FOREACH(i, u->sink->inputs, idx) { pa_sink_input_cork(i, true); } - pa_sink_reconfigure(u->sink, spec, true); + pa_sink_reconfigure(u->sink, spec, false); PA_IDXSET_FOREACH(i, u->sink->inputs, idx) { pa_sink_input_cork(i, false); } @@ -2617,7 +2619,7 @@ static void switch_codec_cb_handler(bool success, pa_bluetooth_profile_t profile PA_IDXSET_FOREACH(i, u->source->outputs, idx) { pa_source_output_cork(i, true); } - pa_source_reconfigure(u->source, spec, true); + pa_source_reconfigure(u->source, spec, false); PA_IDXSET_FOREACH(i, u->source->outputs, idx) { pa_source_output_cork(i, false); } From c22ff90a18e04a67e8c26d9400fd71c561c3a378 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Sat, 30 Jan 2021 17:52:41 +0100 Subject: [PATCH 8/8] HELPNEEDED: Suspending the whole monitor works --- src/modules/bluetooth/module-bluez5-device.c | 39 +++++++++++++------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c index 3db2c5876..7071859c7 100644 --- a/src/modules/bluetooth/module-bluez5-device.c +++ b/src/modules/bluetooth/module-bluez5-device.c @@ -2578,6 +2578,9 @@ static void switch_codec_cb_handler(bool success, pa_bluetooth_profile_t profile pa_sample_spec *spec; bool is_a2dp_sink; int r; + pa_sink_input *i; + pa_source_output *o; + uint32_t idx; if (!success) goto off; @@ -2598,31 +2601,41 @@ static void switch_codec_cb_handler(bool success, pa_bluetooth_profile_t profile // Finish off by reconfiguring if (is_a2dp_sink) { - pa_sink_input *i; - uint32_t idx; - - PA_IDXSET_FOREACH(i, u->sink->inputs, idx) { + PA_IDXSET_FOREACH(i, u->sink->inputs, idx) pa_sink_input_cork(i, true); + if (u->sink->monitor_source) { + pa_source_suspend(u->sink->monitor_source, true, PA_SUSPEND_INTERNAL); + PA_IDXSET_FOREACH(o, u->sink->monitor_source->outputs, idx) + pa_source_output_cork(o, true); } pa_sink_reconfigure(u->sink, spec, false); - PA_IDXSET_FOREACH(i, u->sink->inputs, idx) { - pa_sink_input_cork(i, false); + if (u->sink->monitor_source) { + PA_IDXSET_FOREACH(o, u->sink->monitor_source->outputs, idx) + pa_source_output_cork(o, false); + pa_source_suspend(u->sink->monitor_source, false, PA_SUSPEND_INTERNAL); } + PA_IDXSET_FOREACH(i, u->sink->inputs, idx) + pa_sink_input_cork(i, false); pa_asyncmsgq_send(u->sink->asyncmsgq, PA_MSGOBJECT(u->sink), PA_SINK_MESSAGE_SETUP_STREAM, NULL, 0, NULL); pa_sink_suspend(u->sink, false, PA_SUSPEND_UNAVAILABLE); } else { - pa_source_output *i; - uint32_t idx; - - PA_IDXSET_FOREACH(i, u->source->outputs, idx) { - pa_source_output_cork(i, true); + PA_IDXSET_FOREACH(o, u->source->outputs, idx) + pa_source_output_cork(o, true); + if (u->source->monitor_of) { + pa_sink_suspend(u->source->monitor_of, true, PA_SUSPEND_INTERNAL); + PA_IDXSET_FOREACH(i, u->source->monitor_of->inputs, idx) + pa_sink_input_cork(i, true); } pa_source_reconfigure(u->source, spec, false); - PA_IDXSET_FOREACH(i, u->source->outputs, idx) { - pa_source_output_cork(i, false); + if (u->source->monitor_of) { + PA_IDXSET_FOREACH(i, u->source->monitor_of->inputs, idx) + pa_sink_input_cork(i, false); + pa_sink_suspend(u->source->monitor_of, false, PA_SUSPEND_INTERNAL); } + PA_IDXSET_FOREACH(o, u->source->outputs, idx) + pa_source_output_cork(o, false); pa_asyncmsgq_send(u->source->asyncmsgq, PA_MSGOBJECT(u->source), PA_SOURCE_MESSAGE_SETUP_STREAM, NULL, 0, NULL);