[v12,02/13] bluetooth: Change A2DP codec API of reset() method to indicate failure

Submitted by Pali Rohár on July 5, 2019, 1:02 p.m.

Details

Message ID 20190705130226.5496-3-pali.rohar@gmail.com
State Superseded
Headers show
Series "New API for Bluetooth A2DP codecs" ( rev: 95 94 93 92 91 90 89 88 87 86 85 84 ) in PulseAudio

Not browsing as part of any series.

Commit Message

Pali Rohár July 5, 2019, 1:02 p.m.
SBC codec reset() method may fail, so propagate this failure to caller.
---
 src/modules/bluetooth/a2dp-codec-api.h       |  4 ++--
 src/modules/bluetooth/a2dp-codec-sbc.c       |  5 +++--
 src/modules/bluetooth/module-bluez5-device.c | 18 ++++++++++++------
 3 files changed, 17 insertions(+), 10 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/modules/bluetooth/a2dp-codec-api.h b/src/modules/bluetooth/a2dp-codec-api.h
index 881cc659b..bc4844596 100644
--- a/src/modules/bluetooth/a2dp-codec-api.h
+++ b/src/modules/bluetooth/a2dp-codec-api.h
@@ -69,8 +69,8 @@  typedef struct pa_a2dp_codec {
     void *(*init)(bool for_encoding, bool for_backchannel, const uint8_t *config_buffer, uint8_t config_size, pa_sample_spec *sample_spec);
     /* Deinitialize and release codec info data in codec_info */
     void (*deinit)(void *codec_info);
-    /* Reset internal state of codec info data in codec_info */
-    void (*reset)(void *codec_info);
+    /* Reset internal state of codec info data in codec_info, returns non-zero on failure */
+    int (*reset)(void *codec_info);
 
     /* Get read block size for codec, it is minimal size of buffer
      * needed to decode read_link_mtu bytes of encoded data */
diff --git a/src/modules/bluetooth/a2dp-codec-sbc.c b/src/modules/bluetooth/a2dp-codec-sbc.c
index e2db91b63..f57c7b01a 100644
--- a/src/modules/bluetooth/a2dp-codec-sbc.c
+++ b/src/modules/bluetooth/a2dp-codec-sbc.c
@@ -466,20 +466,21 @@  static void set_bitpool(struct sbc_info *sbc_info, uint8_t bitpool) {
     pa_log_debug("Bitpool has changed to %u", sbc_info->sbc.bitpool);
 }
 
-static void reset(void *codec_info) {
+static int reset(void *codec_info) {
     struct sbc_info *sbc_info = (struct sbc_info *) codec_info;
     int ret;
 
     ret = sbc_reinit(&sbc_info->sbc, 0);
     if (ret != 0) {
         pa_log_error("SBC reinitialization failed: %d", ret);
-        return;
+        return -1;
     }
 
     /* sbc_reinit() sets also default parameters, so reset them back */
     set_params(sbc_info);
 
     sbc_info->seq_num = 0;
+    return 0;
 }
 
 static size_t get_block_size(void *codec_info, size_t link_mtu) {
diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
index 140ddb8fb..0b63cd44a 100644
--- a/src/modules/bluetooth/module-bluez5-device.c
+++ b/src/modules/bluetooth/module-bluez5-device.c
@@ -762,22 +762,24 @@  static void transport_config_mtu(struct userdata *u) {
 }
 
 /* Run from I/O thread */
-static void setup_stream(struct userdata *u) {
+static bool setup_stream(struct userdata *u) {
     struct pollfd *pollfd;
     int one;
 
     /* return if stream is already set up */
     if (u->stream_setup_done)
-        return;
+        return true;
 
     pa_log_info("Transport %s resuming", u->transport->path);
 
     if (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SINK) {
         pa_assert(u->a2dp_codec);
-        u->a2dp_codec->reset(u->encoder_info);
+        if (u->a2dp_codec->reset(u->encoder_info) != 0)
+            return false;
     } else if (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SOURCE) {
         pa_assert(u->a2dp_codec);
-        u->a2dp_codec->reset(u->decoder_info);
+        if (u->a2dp_codec->reset(u->decoder_info) != 0)
+            return false;
     }
 
     transport_config_mtu(u);
@@ -802,6 +804,8 @@  static void setup_stream(struct userdata *u) {
 
     if (u->source)
         u->read_smoother = pa_smoother_new(PA_USEC_PER_SEC, 2*PA_USEC_PER_SEC, true, true, 10, pa_rtclock_now(), true);
+
+    return true;
 }
 
 /* Called from I/O thread, returns true if the transport was acquired or
@@ -813,8 +817,10 @@  static bool setup_transport_and_stream(struct userdata *u) {
     if (transport_error < 0) {
         if (transport_error != -EAGAIN)
             return false;
-    } else
-        setup_stream(u);
+    } else {
+        if (!setup_stream(u))
+            return false;
+    }
     return true;
 }
 

Comments

On Fri, 2019-07-05 at 15:02 +0200, Pali Rohár wrote:
> SBC codec reset() method may fail, so propagate this failure to caller.
> ---
>  src/modules/bluetooth/a2dp-codec-api.h       |  4 ++--
>  src/modules/bluetooth/a2dp-codec-sbc.c       |  5 +++--
>  src/modules/bluetooth/module-bluez5-device.c | 18 ++++++++++++------
>  3 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/src/modules/bluetooth/a2dp-codec-api.h b/src/modules/bluetooth/a2dp-codec-api.h
> index 881cc659b..bc4844596 100644
> --- a/src/modules/bluetooth/a2dp-codec-api.h
> +++ b/src/modules/bluetooth/a2dp-codec-api.h
> @@ -69,8 +69,8 @@ typedef struct pa_a2dp_codec {
>      void *(*init)(bool for_encoding, bool for_backchannel, const uint8_t *config_buffer, uint8_t config_size, pa_sample_spec *sample_spec);
>      /* Deinitialize and release codec info data in codec_info */
>      void (*deinit)(void *codec_info);
> -    /* Reset internal state of codec info data in codec_info */
> -    void (*reset)(void *codec_info);
> +    /* Reset internal state of codec info data in codec_info, returns non-zero on failure */
> +    int (*reset)(void *codec_info);
>  
>      /* Get read block size for codec, it is minimal size of buffer
>       * needed to decode read_link_mtu bytes of encoded data */
> diff --git a/src/modules/bluetooth/a2dp-codec-sbc.c b/src/modules/bluetooth/a2dp-codec-sbc.c
> index e2db91b63..f57c7b01a 100644
> --- a/src/modules/bluetooth/a2dp-codec-sbc.c
> +++ b/src/modules/bluetooth/a2dp-codec-sbc.c
> @@ -466,20 +466,21 @@ static void set_bitpool(struct sbc_info *sbc_info, uint8_t bitpool) {
>      pa_log_debug("Bitpool has changed to %u", sbc_info->sbc.bitpool);
>  }
>  
> -static void reset(void *codec_info) {
> +static int reset(void *codec_info) {
>      struct sbc_info *sbc_info = (struct sbc_info *) codec_info;
>      int ret;
>  
>      ret = sbc_reinit(&sbc_info->sbc, 0);
>      if (ret != 0) {
>          pa_log_error("SBC reinitialization failed: %d", ret);
> -        return;
> +        return -1;
>      }
>  
>      /* sbc_reinit() sets also default parameters, so reset them back */
>      set_params(sbc_info);
>  
>      sbc_info->seq_num = 0;
> +    return 0;
>  }
>  
>  static size_t get_block_size(void *codec_info, size_t link_mtu) {
> diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
> index 140ddb8fb..0b63cd44a 100644
> --- a/src/modules/bluetooth/module-bluez5-device.c
> +++ b/src/modules/bluetooth/module-bluez5-device.c
> @@ -762,22 +762,24 @@ static void transport_config_mtu(struct userdata *u) {
>  }
>  
>  /* Run from I/O thread */
> -static void setup_stream(struct userdata *u) {
> +static bool setup_stream(struct userdata *u) {

Failure should be indicated using a negative int rather than a boolean
value. I see a bool return value for setup_transport_and_stream() has
previously somehow passed review, which is probably why you opted for
bool here, but could you change this anyway?
On Fri, 2019-07-05 at 15:02 +0200, Pali Rohár wrote:
>  /* Run from I/O thread */
> -static void setup_stream(struct userdata *u) {
> +static bool setup_stream(struct userdata *u) {
>      struct pollfd *pollfd;
>      int one;
>  
>      /* return if stream is already set up */
>      if (u->stream_setup_done)
> -        return;
> +        return true;
>  
>      pa_log_info("Transport %s resuming", u->transport->path);
>  
>      if (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SINK) {
>          pa_assert(u->a2dp_codec);
> -        u->a2dp_codec->reset(u->encoder_info);
> +        if (u->a2dp_codec->reset(u->encoder_info) != 0)
> +            return false;
>      } else if (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SOURCE) {
>          pa_assert(u->a2dp_codec);
> -        u->a2dp_codec->reset(u->decoder_info);
> +        if (u->a2dp_codec->reset(u->decoder_info) != 0)

"< 0" is the usual convention rather than "!= 0".

(Sorry for this extra mail, I originally wrote this in the first review
mail, but it seems that before sending it, I trimmed too much from the
bottom of the email and accidentally deleted this bit.)