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

[Pali Rohár]

On Friday 05 April 2019 17:14:33 Tanu Kaskinen wrote:
> 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.

Ok, let it currently as is and later somebody else can do refactors (if
are really needed).

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

You are right, in this case count is used only for initialization and
iteration is done in reverse order. So I can do that replacement as you
wrote.

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

There is minimal bitpool value, maximal bitpool value, current bitpool
value and initial bitpool. Initial is either minimal or maximal value
which depends on either device is sink or source. Current value is
stored in sbc structure (exported by libsbc library) which cannot be
changed. I will try to change names where it is possible.

-- 
Pali Rohár
pali.rohar at gmail.com