[pulseaudio-discuss] [PATCH v7 04/13] bluetooth: Modular API for A2DP codecs

[Tanu Kaskinen]

On Fri, 2019-04-05 at 12:36 +0200, Pali Rohár wrote:
> On Thursday 04 April 2019 18:19:32 Tanu Kaskinen wrote:
> > On Sat, 2019-02-23 at 10:45 +0100, Pali Rohár wrote:
> > > +
> > > +    /* 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.

I was going to say that we could just drop the const declaration, but
since it's possible to use two bluetooth devices simultaneously, the
fact that there's only one instance of each codec prevents adding any
state to the structs.

I think it would be better to allocate the codec structs dynamically so
that the codecs could store their own state rather than requiring
module-bluez5-device.c to do that, but I don't want to block the patch
series on this issue, so I guess we'll keep the code as it is.

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

a2dp_codec_count is only used in the initialization part of the for
loop, so it's not used every iteration.

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

Ah, so sbc_info.bitpool is the default bitpool value which is not
changed when the bitpool is reduced. Could the variable be renamed to
default_bitpool or initial_bitpool?

-- 
Tanu

https://www.patreon.com/tanuk
https://liberapay.com/tanuk