(I sent my review both to you and to the list, but you replied only
privately - probably not on purpose. I'll send this mail only to the
list.)
On Thu, 2018-09-13 at 10:54 +0200, Pali Rohár wrote:
> On Tuesday 04 September 2018 11:44:10 Tanu Kaskinen wrote:
> > Using libopenaptx directly
> > doesn't prevent us from switching to GStreamer later if someone writes
> > the code.
>
> Yes, that is truth.
>
> > Another point that was raised that the codec choice shouldn't be bound
> > to the card profile. I tend to agree with that. There are situations
> > where module-bluetooth-policy or the user only wants to switch from HSP
> > to A2DP and not care about the codec. You asked how the codec should be
> > negotiated or switched by the user if it's not bound to the profile.
> > Regarding negotiation, we can add a hook that module-bluetooth-policy
> > can use to make the codec decision (if module-bluetooth-policy isn't
> > loaded, SBC seems like a sensible default).
> >
> > Is there a need for the user to be able to choose the codec? Shouldn't
> > we always automatically pick the highest-quality one?
>
> What is "highest-quality" codec? How you would compare e.g. our SBC
> implementation which can dynamically change bitpool and therefore
> quality?
>
> Also how you can compare... has SBC higher quality as MPEG1? Or has aptX
> HD higher quality as MPEG/AAC?
Fair point.
> Until now everybody said that aptX "is better" then SBC. But today it
> does not look like it is truth.
>
> Also if you have MP3 files on disk, then the best quality which you can
> achieve is to passthru it without any reencoding. In other cases AAC can
> be better.
Passthrough is a separate thing. If someone implements passthrough
support for bluetooth and a client requests it, we should always switch
to the requested codec regardless of other user preferences.
> So answer to this question depends on lot of things and either user
> itself or user's application would better it.
>
> > I'm not against
> > adding the functionality, it would be useful for testing if nothing
> > else. It just doesn't seem very important.
> >
> > We could have a "preferred-a2dp-codec" option in bluetooth.conf (that
> > file doesn't exist, but can be added). A per-card option would be
> > possible too, as would be having both a global option that could be
> > overridden with a per-card option.
>
> Preferred codec is not helpful. The only mandatory codec is SBC, all
> other are optional. And basically every manufactor supports different
> set of codecs. So at least some kind of list or partially ordered set of
> codec is needed.
Sure, a priority list can better capture the user preferences, but
surely a "preferred codec" option is better than nothing. A priority
list of codecs isn't an obvious ultimate solution either, since some
codecs have adjustable parameters that can affect the codec preference
ranking. For example, someone's preference might be:
SBC (high bitrate)
aptX
SBC (medium bitrate)
It's not clear to me how fine-grained control is optimal. A solution
that allows specifying a priority list with an arbitrary number of
codec parameters would obviously be sufficient, but it's unlikely we
want that complex system.
Real use cases are needed. I can't provide any, since I don't use
bluetooth (and if I did, I probably wouldn't bother with the codec
settings at all - SBC and aptX seem both good enough for me). Now that
you found out that aptX isn't that great after all, do you have
personal use for aptX either?
> > For runtime configuration, we can add a command to set the codec
> > preference. This should be done with the message API that Georg Chini
> > has been working on (not yet finished). There's then need for saving
> > the choice too. We can either introduce a new database for bluetooth
> > stuff, or we can add some API to module-card-restore so that other
> > modules (module-bluetooth-discover in this case) can save arbitrary
> > parameters for cards.
> >
> > I recall there was some lack of relevant APIs in bluetoothd for
> > choosing the codec from PulseAudio. Can you remind me, what are the
> > limitations, and how did you plan to deal with them?
>
> Currently when bluetooth headset initialize connection, it is up to
> headset which codec would choose. I did some testing and my headset
> choose codec (sbc vs aptx) just randomly.
>
> When bluez initialize connection, then it prefer codecs in order in
> which pulseaudio did dbus endpoint registration. Order is global and
> hardcoded in source code.
>
> Once connection is established there is no way to change codec. bluez
> does not provide API for it.
>
> So everything which you wrote above would apply only when pulseaudio
> starts a2dp connection, not headset.
>
> And I think it is very bad if we cannot achieve stable codec selection.
So what do you propose? If your point was that my suggestions result in
unstable codec selection, I don't see how your original proposal of
having a separate card profile for each codec (or potentially each set
of codec parameters) makes the situation better.
> > Comments on the code below. This review was done over several days,
> > sorry for any inconsistencies or strange repetition (I noticed I
> > explain some things multiple times).
>
> Currently I do not have much time to work on this. So just few comments
> below.
>
> > > @@ -888,10 +889,21 @@ finish:
> > > static void register_endpoint(pa_bluetooth_discovery *y, const char *path, const char *endpoint, const char *uuid) {
> > > DBusMessage *m;
> > > DBusMessageIter i, d;
> > > - uint8_t codec = 0;
> > > + uint8_t capabilities[1024];
> > > + size_t capabilities_size;
> > > + uint8_t codec_id;
> > > + const pa_a2dp_codec_t *codec;
> > > +
> > > + codec = pa_a2dp_endpoint_to_a2dp_codec(endpoint);
> >
> > I think it would be better to change the function parameters so that
> > instead of an endpoint path the function would take a codec id.
>
> What do you mean by codec id? Do you mean A2DP codec id? If yes, then it
> would not work, as every non-sbc and non-mpeg codec has codec is 0xFF.
> Codec itself is then determined by passed capabilities.
I thought we could use the id from the A2DP spec, but apparently not. I
propose having our own separate codec enumeration then that provides
the necessary ids.
> > There
> > would be a function called get_a2dp_codec() that would take the codec
> > id as a parameter and return the codec struct. The endpoint path could
> > be added to the codec struct.
> >
> > Shuffling things around like this wouldn't have huge benefits, I just
> > find looking up the codec based on the id neater than based on the
> > endpoint path.
> >
> > I hope you don't have a problem with that suggestion, but if you do and
> > you want to keep using pa_a2dp_endpoint_to_a2dp_codec(), then the
> > function should be made static (it's not used outside bluez5-utils.c)
> > and the pa_ prefix should be dropped, since functions that aren't
> > exported through headers should not use the pa_ prefix according to our
> > coding style.
> > > + sbc_info->min_bitpool = config->min_bitpool;
> > > + sbc_info->max_bitpool = config->max_bitpool;
> > > +
> > > + /* Set minimum bitpool for source to get the maximum possible block_size */
> > > + sbc_info->sbc.bitpool = is_a2dp_sink ? sbc_info->max_bitpool : sbc_info->min_bitpool;
> >
> > Do you understand the logic here?
>
> I have not looked deeply at this code. So I'm not fully sure. I just
> moved existing code into new file.
Ok, so this remains a mystery to all of us.
--
Tanu
https://www.patreon.com/tanuk
https://liberapay.com/tanuk
