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

[Huang-Huang Bao]

On 12/16/18 5:48 AM, Pali Rohár wrote:
> 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?

Because there are some functions in a2dp-codec needs it, one enum let
functions more concise.

"PA_A2DP_SOURCE_MIN = PA_A2DP_SINK_MAX" is no sense.

>> +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?

If <pa_a2dp_codec_index>'s encoder/decoder not loaded, the load function
should return false, then "pa_a2dp_init()" would not add that codec
index to indices lists.

In this patch, all codecs libraries dynamically linked so the
"decoder_load()" always return true;

Except ffmpeg, which require a motion for loading aptX(HD) encoder/decoder.

It just support dynamic loading .

> 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.

Well, the api just "hide" all details.

>> +    /* 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.
Yes. I simply use bluez apis names before.
>> +
>> +};

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/pulseaudio-discuss/attachments/20181216/2d61b028/attachment.sig>