Re: [PATCH v7 04/13] bluetooth: Modular API for A2DP codecs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thursday 04 April 2019 18:19:32 Tanu Kaskinen wrote:
> On Sat, 2019-02-23 at 10:45 +0100, Pali Rohár wrote:
> > +typedef struct pa_a2dp_codec {
> > +    /* Unique name of the codec, lowercase and without whitespaces, used for constructing identifier, D-Bus paths, ... */
> > +    const char *codec_name;
> > +    /* Human readable codec description */
> > +    const char *codec_description;
> > +
> > +    /* A2DP codec id */
> > +    pa_a2dp_codec_id codec_id;
> 
> I agree with Luiz that it would be good to drop "codec_" from the names
> of these variables.

Ok, I will remove them.

> > +
> > +    /* True if codec is bi-directional and supports backchannel */
> > +    bool support_backchannel;
> > +
> > +    /* Returns true if codec accepts capabilities, for_encoding is true when capabilities are used for encoding */
> > +    bool (*accept_capabilities)(const uint8_t *capabilities_buffer, uint8_t capabilities_size, bool for_encoding);
> > +    /* Choose preferred capabilities from hash map (const char * -> const pa_a2dp_codec_capabilities *) and returns corresponding key, for encoder is true when capabilities hash map is used for encoding */
> 
> Typo: "for encoder" should be "for_encoder".

Ok.

> Also, comments should be wrapped at 80 characters, that makes them
> easier to read.

Ok.

> > +    const char *(*choose_capabilities)(const pa_hashmap *capabilities_hashmap, const pa_sample_spec *default_sample_spec, bool for_encoding);
> 
> Returning a string key rather than a pa_a2dp_codec_capabilities looked
> weird, and the comment didn't explain what the hashmap keys should look
> like. I looked up how choose_capabilities() is used, and it looks like
> the returned key is an endpoint path. I think choose_remote_endpoint
> would be a better name for this callback, and the documentation should
> explain that the keys are remote endpoint paths.

Yes, it returns name of remote endpoint based on capabilities_hashmap
(capability --> endpoint).

choose_remote_endpoint seems like a better name, I will change it and
also update documentation.

> It seems that returning NULL is allowed when none of the endpoints can
> be accepted. It would be good to document that.

Yes.

> > +    /* Fill codec capabilities, returns size of filled buffer */
> > +    uint8_t (*fill_capabilities)(uint8_t capabilities_buffer[254]);
> 
> I think we should use a define for the maximum capabilities buffer
> size.

254 is limit by A2DP protocol itself. I can define macro for it.

> > +    /* Validate codec configuration, returns true on success */
> > +    bool (*validate_configuration)(const uint8_t *config_buffer, uint8_t config_size);
> 
> I commented earlier:
> 
>     The bool return value is used for reporting failures, but according
>     to our coding style failures should be reported using a negative
>     int.
> 
> You replied:
> 
>     It is really needed to use integer return value for true/false?
> 
> I never answered that question (sorry). When the result is about
> success/failure, then an int should be used. If the result is a yes/no
> answer, then bool is correct. If the function name was
> is_configuration_valid, then you could use bool here.

Because this codec API is slightly complicated, due to A2DP, I would
rather use C types which describe returned type unambiguously. So for
boolean value is C bool better. So if name "is_configuration_valid" is
acceptable with return type "bool" I will rename this function.

> > +    /* Fill preferred codec configuration, returns size of filled buffer */
> > +    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[254]);
> 
> It should be documented that the callback should return 0 on failure.

Ok.

> > +
> > +    /* Initialize codec, returns codec info data and set sample_spec, for_encoding is true when codec_info is used for encoding, for_backchannel is true when codec_info is used for backchannel */
> > +    void *(*init_codec)(bool for_encoding, bool for_backchannel, const uint8_t *config_buffer, uint8_t config_size, pa_sample_spec *sample_spec);
> 
> In order to be more consistent with other abstraction structs, I'd like
> to add a void pointer variable called "userdata" to pa_a2dp_codec.

This is not possible. Instances of pa_a2dp_codec struct are statically
defined directly in implementation of codec and are constant. So you
cannot put there some dynamic data, like user pointers. See e.g.
pa_a2dp_codec_aptx_hd how it is used.

> init_codec() would set that variable instead of returning the data. All
> callbacks would receive a pa_a2dp_codec pointer as a parameter instead
> of a void pointer.
> 
> > +    /* Deinitialize and release codec info data in codec_info */
> > +    void (*finish_codec)(void *codec_info);
> > +    /* Reset internal state of codec info data in codec_info */
> > +    void (*reset_codec)(void *codec_info);
> > +
> > +    /* Get read block size for codec */
> > +    size_t (*get_read_block_size)(void *codec_info, size_t read_link_mtu);
> > +    /* Get write block size for codec */
> > +    size_t (*get_write_block_size)(void *codec_info, size_t write_link_mtu);
> > +
> > +    /* Reduce encoder bitrate for codec, returns new write block size or zero if not changed, called when socket is not accepting encoded data fast enough */
> > +    size_t (*reduce_encoder_bitrate)(void *codec_info, size_t write_link_mtu);
> > +
> > +    /* Encode input_buffer of input_size to output_buffer of output_size, returns size of filled ouput_buffer and set processed to size of processed input_buffer */
> > +    size_t (*encode_buffer)(void *codec_info, uint32_t timestamp, const uint8_t *input_buffer, size_t input_size, uint8_t *output_buffer, size_t output_size, size_t *processed);
> > +    /* Decode input_buffer of input_size to output_buffer of output_size, returns size of filled ouput_buffer and set processed to size of processed input_buffer */
> > +    size_t (*decode_buffer)(void *codec_info, const uint8_t *input_buffer, size_t input_size, uint8_t *output_buffer, size_t output_size, size_t *processed);
> > +} pa_a2dp_codec;
> > +
> > +#endif

> > diff --git a/src/modules/bluetooth/a2dp-codec-sbc.c b/src/modules/bluetooth/a2dp-codec-sbc.c
> > new file mode 100644
> > index 000000000..3db827db3
> > --- /dev/null
> > +++ b/src/modules/bluetooth/a2dp-codec-sbc.c
> > +    if (!(capabilities->subbands & (SBC_SUBBANDS_4 | SBC_SUBBANDS_8)))
> > +        return false;
> > +
> > +    if (!(capabilities->block_length & (SBC_BLOCK_LENGTH_4 | SBC_BLOCK_LENGTH_8 | SBC_BLOCK_LENGTH_12 | SBC_BLOCK_LENGTH_16)))
> > +        return false;
> > +
> > +    return true;
> > +}
> > +
> > +static const char *choose_capabilities(const pa_hashmap *capabilities_hashmap, bool for_encoding) {
> 
> The default_sample_spec argument is missing.

Ou, that is bug. I will fix it.

> > +    config->min_bitpool = (uint8_t) PA_MAX(SBC_MIN_BITPOOL, capabilities->min_bitpool);
> > +    config->max_bitpool = (uint8_t) PA_MIN(default_bitpool(config->frequency, config->channel_mode), capabilities->max_bitpool);
> > +
> > +    if (config->min_bitpool > config->max_bitpool)
> > +        return 0;
> 
> Logging an error message would be helpful here.

This is just code movement. And your comment is fixed in patch 05/13.

> > +
> > +    return sizeof(*config);
> > +}

> > diff --git a/src/modules/bluetooth/a2dp-codec-util.c b/src/modules/bluetooth/a2dp-codec-util.c
> > new file mode 100644
> > index 000000000..27128d8ae
> > --- /dev/null
> > +++ b/src/modules/bluetooth/a2dp-codec-util.c
> > +const pa_a2dp_codec *pa_bluetooth_a2dp_codec(const char *name) {
> 
> Some bikeshedding: I'd prefer pa_bluetooth_get_a2dp_codec as the name.

Ok.

> > diff --git a/src/modules/bluetooth/bluez5-util.c b/src/modules/bluetooth/bluez5-util.c
> > index 485a57515..e4450cfe3 100644
> > --- a/src/modules/bluetooth/bluez5-util.c
> > +++ b/src/modules/bluetooth/bluez5-util.c
> > @@ -950,6 +943,7 @@ static void parse_interfaces_and_properties(pa_bluetooth_discovery *y, DBusMessa
> >  
> >          if (pa_streq(interface, BLUEZ_ADAPTER_INTERFACE)) {
> >              pa_bluetooth_adapter *a;
> > +            unsigned a2dp_codec_i, a2dp_codec_count;
> 
> The a2dp_codec_count variable doesn't have much value, since it's used
> only once and can be replaced with the pa_bluetooth_a2dp_codec_count()
> call.

This is prevention to be called this external function N times. It is
used as condition in for() loop and if I replace it on that place like
you wrote it would be called every iteration of loop. Function is cannot
be inlined as it is defined in .c file.

> > +            /* Order is important. bluez prefers endpoints registered earlier.
> > +             * And codec with higher number has higher priority. So iterate in reverse order. */
> > +            a2dp_codec_count = pa_bluetooth_a2dp_codec_count();
> > +            for (a2dp_codec_i = a2dp_codec_count; a2dp_codec_i > 0; a2dp_codec_i--) {
> > +                const pa_a2dp_codec *a2dp_codec = pa_bluetooth_a2dp_codec_iter(a2dp_codec_i-1);
> > +                char *endpoint;
> > +
> > +                endpoint = pa_sprintf_malloc("%s/%s", A2DP_SINK_ENDPOINT, a2dp_codec->codec_name);
> > +                register_endpoint(y, a2dp_codec, path, endpoint, PA_BLUETOOTH_UUID_A2DP_SINK);
> > +                pa_xfree(endpoint);
> > +
> > +                endpoint = pa_sprintf_malloc("%s/%s", A2DP_SOURCE_ENDPOINT, a2dp_codec->codec_name);
> > +                register_endpoint(y, a2dp_codec, path, endpoint, PA_BLUETOOTH_UUID_A2DP_SOURCE);
> > +                pa_xfree(endpoint);
> > +            }

> > diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
> > index 3e72ac3c1..531bc25a5 100644
> > --- a/src/modules/bluetooth/module-bluez5-device.c
> > +++ b/src/modules/bluetooth/module-bluez5-device.c
> > @@ -413,115 +409,41 @@ static int sco_process_push(struct userdata *u) {
> >  }
> >  
> >  /* Run from IO thread */
> > -static void a2dp_prepare_buffer(struct userdata *u) {
> > +static void a2dp_prepare_encoder_buffer(struct userdata *u) {
> >      size_t min_buffer_size = PA_MAX(u->read_link_mtu, u->write_link_mtu);
> 
> Since you added separate "prepare buffer" functions for encoding and
> decoding, it seems that the PA_MAX() usage is unnecessary, and you
> should just use u->write_link_mtu here (and u->read_link_mtu in
> prepare_decoder_buffer).

Good catch!

> > @@ -620,50 +569,18 @@ static int a2dp_process_push(struct userdata *u) {
> >              tstamp = pa_rtclock_now();
> >          }
> >  
> > -        p = (uint8_t*) sbc_info->buffer + sizeof(*header) + sizeof(*payload);
> > -        to_decode = l - sizeof(*header) - sizeof(*payload);
> > -
> > -        d = pa_memblock_acquire(memchunk.memblock);
> > -        to_write = memchunk.length = pa_memblock_get_length(memchunk.memblock);
> > -
> > -        while (PA_LIKELY(to_decode > 0)) {
> > -            size_t written;
> > -            ssize_t decoded;
> > +        ptr = pa_memblock_acquire(memchunk.memblock);
> > +        memchunk.length = pa_memblock_get_length(memchunk.memblock);
> >  
> > -            decoded = sbc_decode(&sbc_info->sbc,
> > -                                 p, to_decode,
> > -                                 d, to_write,
> > -                                 &written);
> > -
> > -            if (PA_UNLIKELY(decoded <= 0)) {
> > -                pa_log_error("SBC decoding error (%li)", (long) decoded);
> > -                pa_memblock_release(memchunk.memblock);
> > -                pa_memblock_unref(memchunk.memblock);
> > -                return 0;
> > -            }
> > -
> > -            total_written += written;
> > -
> > -            /* Reset frame length, it can be changed due to bitpool change */
> > -            sbc_info->frame_length = sbc_get_frame_length(&sbc_info->sbc);
> > -
> > -            pa_assert_fp((size_t) decoded <= to_decode);
> > -            pa_assert_fp((size_t) decoded == sbc_info->frame_length);
> > -
> > -            pa_assert_fp((size_t) written == sbc_info->codesize);
> > -
> > -            p = (const uint8_t*) p + decoded;
> > -            to_decode -= decoded;
> > -
> > -            d = (uint8_t*) d + written;
> > -            to_write -= written;
> > -        }
> > +        total_written = u->a2dp_codec->decode_buffer(u->decoder_info, u->decoder_buffer, l, ptr, memchunk.length, &processed);
> > +        if (total_written == 0)
> > +            return 0;
> >  
> >          u->read_index += (uint64_t) total_written;
> > -        pa_smoother_put(u->read_smoother, tstamp, pa_bytes_to_usec(u->read_index, &u->sample_spec));
> > +        pa_smoother_put(u->read_smoother, tstamp, pa_bytes_to_usec(u->read_index, &u->decoder_sample_spec));
> >          pa_smoother_resume(u->read_smoother, tstamp, true);
> >  
> > -        memchunk.length -= to_write;
> > +        memchunk.length -= l - processed;
> 
> This doesn't make sense to me, but maybe I'm missing something, since
> you've probably tested that this works fine... memchunk.length should
> be the length of uncompressed audio that was decoded, but
> memchunk.length is initially set to read_block_size, i.e. the maximum
> possible amount of compressed data that can be read in one go, and then
> "l - processed" is subtracted, and "l - processed" is the amount of
> compressed audio that was read but for some reason not processed. So
> memchunk.length is set to the maximum amount of compressed data minus
> received but unprocessed compressed data. That doesn't seem to have
> anything to do with the length of uncompressed audio that was decoded.

to_write was passed as argument to sbc_decode function which describes
length of output buffer for decoded data.

Until there are some data to decode, to_write is decremented by length
of written decoded data. So to_write hold length of buffer need for
decoded data every step.

So you are right that (l-processed) is *not* same as to_write. And this
is wrong.

> Why is memchunk.length not simply set to the return value of
> decode_buffer()?

I looked at it again and seems that you are right. Return value of
decode_buffer() gives us length of data written into output buffer which
is really what subtraction by to_write from memchunk.length returns.

I will change it.

> >  
> >          pa_memblock_release(memchunk.memblock);
> >  

> > @@ -865,28 +734,22 @@ static void transport_config_mtu(struct userdata *u) {
> >              u->write_block_size = pa_frame_align(u->write_block_size, &u->sink->sample_spec);
> >          }
> >      } else {
> > -        u->read_block_size =
> > -            (u->read_link_mtu - sizeof(struct rtp_header) - sizeof(struct rtp_payload))
> > -            / u->sbc_info.frame_length * u->sbc_info.codesize;
> > -
> > -        u->write_block_size =
> > -            (u->write_link_mtu - sizeof(struct rtp_header) - sizeof(struct rtp_payload))
> > -            / u->sbc_info.frame_length * u->sbc_info.codesize;
> > +        pa_assert(u->a2dp_codec);
> > +        if (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SINK) {
> > +            u->write_block_size = u->a2dp_codec->get_write_block_size(u->encoder_info, u->write_link_mtu);
> > +        } else {
> > +            u->read_block_size = u->a2dp_codec->get_read_block_size(u->encoder_info, u->read_link_mtu);
> 
> Should be decoder_info instead of encoder_info.

Yes! This is bug.

> > @@ -911,11 +782,6 @@ static void setup_stream(struct userdata *u) {
> >  
> >      pa_log_debug("Stream properly set up, we're ready to roll!");
> >  
> > -    if (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SINK) {
> > -        a2dp_set_bitpool(u, u->sbc_info.max_bitpool);
> 
> It looks like resetting the bitpool isn't done anywhere. Should it be
> done in the reset_codec() callback?

reset_codec() for SBC calls set_params() and it resets sbc.bitpool to
default value. So reset_codec() calls resets bitpool.

> > -        update_buffer_size(u);
> > -    }
> > -
> >      u->rtpoll_item = pa_rtpoll_item_new(u->rtpoll, PA_RTPOLL_NEVER, 1);
> >      pollfd = pa_rtpoll_item_get_pollfd(u->rtpoll_item, NULL);
> >      pollfd->fd = u->stream_fd;

> > @@ -2497,11 +2301,18 @@ void pa__done(pa_module *m) {
> >      if (u->transport_microphone_gain_changed_slot)
> >          pa_hook_slot_free(u->transport_microphone_gain_changed_slot);
> >  
> > -    if (u->sbc_info.buffer)
> > -        pa_xfree(u->sbc_info.buffer);
> > +    if (u->encoder_buffer)
> > +        pa_xfree(u->encoder_buffer);
> >  
> > -    if (u->sbc_info.sbc_initialized)
> > -        sbc_finish(&u->sbc_info.sbc);
> > +    if (u->decoder_buffer)
> > +        pa_xfree(u->decoder_buffer);
> > +
> > +    if (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SINK || u->profile == PA_BLUETOOTH_PROFILE_A2DP_SOURCE) {
> > +        if (u->encoder_info)
> > +            u->a2dp_codec->finish_codec(u->encoder_info);
> > +        if (u->decoder_info)
> > +            u->a2dp_codec->finish_codec(u->decoder_info);
> 
> stop_thread() ensures that encoder_info and decoder_info are NULL, so
> this code is unnecessary. You can add assertions if you like.

Ok.

-- 
Pali Rohár
pali.rohar@xxxxxxxxx
_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss




[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux