[pulseaudio-discuss] [PATCH 1/4] A2DP api with multi-codec support

[Pali Rohár]

On Thursday 13 December 2018 19:43:36 EHfive wrote:
> diff --git a/src/modules/bluetooth/a2dp/a2dp-api.h b/src/modules/bluetooth/a2dp/a2dp-api.h
> new file mode 100644
> index 000000000..b357555d0
> --- /dev/null
> +++ b/src/modules/bluetooth/a2dp/a2dp-api.h
> @@ -0,0 +1,170 @@
> +#ifndef fooa2dpcodecapifoo
> +#define fooa2dpcodecapifoo
> +
> +#ifdef HAVE_CONFIG_H
> +
> +#include <config.h>
> +
> +#endif
> +
> +#include <pulse/sample.h>
> +#include <pulse/proplist.h>
> +#include <pulsecore/hashmap.h>
> +
> +#include "a2dp-codecs.h"
> +#include "rtp.h"
> +
> +typedef struct pa_a2dp_codec pa_a2dp_codec_t;
> +typedef struct pa_a2dp_config pa_a2dp_config_t;
> +
> +extern const pa_a2dp_codec_t pa_a2dp_sbc;
> +
> +
> +/* Run from <pa_a2dp_sink_t>.encode */
> +
> +typedef void (*pa_a2dp_source_read_cb_t)(const void **read_buf, size_t read_buf_size, void *data);
> +
> +typedef void (*pa_a2dp_source_read_buf_free_cb_t)(const void **read_buf, void *data);
> +
> +
> +// Larger index stands for higher priority by default
> +typedef enum pa_a2dp_codec_index {
> +    PA_A2DP_SINK_MIN,
> +    PA_A2DP_SINK_SBC,
> +    PA_A2DP_SINK_MAX,
> +    PA_A2DP_SOURCE_MIN = PA_A2DP_SINK_MAX,
> +    PA_A2DP_SOURCE_SBC,
> +    PA_A2DP_SOURCE_MAX,
> +    PA_A2DP_CODEC_INDEX_UNAVAILABLE
> +} pa_a2dp_codec_index_t;

This priority enum seems to be strange. One enum mixes both sink and
sources priorities? And why sink max has same value as source min?

> +typedef struct pa_a2dp_sink {
> +    int priority;
> +
> +    /* Load decoder if it's not loaded; Return true if it's loaded */
> +    bool (*decoder_load)();

It is really needed? Cannot be decoder library dynamically linked into
pulseaudio bluez module, and therefore by definition always loaded?

Support for dlopening decoders at runtime just cause problems. Some
users would have installed needed libraries and it work without
problems. Some not and it will not work... And all users have same
pulseaudio version and compiled with same flags..

> +    /* Memory management is pa_a2dp_sink's work */
> +    bool (*init)(void **codec_data);
> +
> +    /* Optional. Update user configurations
> +     * Note: not transport 'configuration' or 'capabilities' */
> +    int (*update_user_config)(pa_proplist *user_config, void **codec_data);
> +
> +    void (*config_transport)(pa_sample_spec default_sample_spec, const void *configuration, size_t configuration_size,
> +                             pa_sample_spec *sample_spec, void **codec_data);
> +
> +    void (*get_block_size)(size_t read_link_mtu, size_t *read_block_size, void **codec_data);
> +
> +    void (*setup_stream)(void **codec_data);
> +
> +    size_t
> +    (*decode)(const void *read_buf, size_t read_buf_size, void *write_buf, size_t write_buf_size, size_t *decoded,
> +              uint32_t *timestamp, void **codec_data);
> +
> +    void (*free)(void **codec_data);
> +} pa_a2dp_sink_t;
> +
> +
> +typedef struct pa_a2dp_source {
> +    int priority;
> +
> +    /* Load encoder if it's not loaded; Return true if it's loaded */
> +    bool (*encoder_load)();
> +
> +    /* Memory management is pa_a2dp_source's work */
> +    bool (*init)(pa_a2dp_source_read_cb_t read_cb, pa_a2dp_source_read_buf_free_cb_t free_cb, void **codec_data);
> +
> +    /* Optional. Update user configurations
> +     * Note: not transport 'configuration' or 'capabilities' */
> +    int (*update_user_config)(pa_proplist *user_config, void **codec_data);
> +
> +    void (*config_transport)(pa_sample_spec default_sample_spec, const void *configuration, size_t configuration_size,
> +                             pa_sample_spec *sample_spec, void **codec_data);
> +
> +    void (*get_block_size)(size_t write_link_mtu, size_t *write_block_size, void **codec_data);
> +
> +    void (*setup_stream)(void **codec_data);
> +
> +    /* The implement should pass read_cb_data to pa_a2dp_source_read_cb, pa_a2dp_source_read_buf_free_cb */
> +    size_t (*encode)(uint32_t timestamp, void *write_buf, size_t write_buf_size, size_t *encoded,
> +                     void *read_cb_data, void **codec_data);
> +
> +    /* Optional */
> +    void (*set_tx_length)(size_t len, void **codec_data);
> +
> +    /* Optional */
> +    void (*decrease_quality)(void **codec_data);
> +
> +    void (*free)(void **codec_data);
> +} pa_a2dp_source_t;
> +
> +
> +struct pa_a2dp_codec {
> +    const char *name;
> +    uint8_t codec;
> +    const a2dp_vendor_codec_t *vendor_codec;
> +    pa_a2dp_sink_t *a2dp_sink;
> +    pa_a2dp_source_t *a2dp_source;
> +
> +    /* Memory management is pa_a2dp_codec's work */
> +    size_t (*get_capabilities)(void **capabilities);
> +
> +    void (*free_capabilities)(void **capabilities);
> +
> +    size_t (*select_configuration)(const pa_sample_spec default_sample_spec, const uint8_t *supported_capabilities,
> +                                   const size_t capabilities_size, void **configuration);
> +
> +    void (*free_configuration)(void **configuration);

You can simplify memory management if caller (instead of callee) would
be responsible for passing pointer to allocated memory (e.g. also from
stack) with buffer size. Then API would not need any "free" functions.

> +    /* Return true if configuration valid */
> +    bool (*set_configuration)(const uint8_t *selected_configuration, const size_t configuration_size);

If this function validates configuration, then "validate_configuration"
is better name.

> +
> +};

-- 
Pali Rohár
pali.rohar at gmail.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/pulseaudio-discuss/attachments/20181215/8c2f43cf/attachment-0001.sig>