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
