On Monday 17 September 2018 15:02:18 Tanu Kaskinen wrote:
> (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.)
Sorry for that. I was not my intension.
> 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.
The best thing is allowing user to choose codec itself (in some GUI
under section additional settings, or similar) and ideally remember it
for every device separately.
> 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?
This is interesting question. For more then month I'm using aptX codec
and I have not hear difference or something which would disturb me.
So I probably really do not need aptX at all.
> > > 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.
Wait until bluez adds new API for choosing codec and use this new API in
pulseaudio for codec selection. This is really something which we need
for proper and deterministic (multi)codec support.
> > > 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.
>
--
Pali Rohár
pali.rohar at gmail.com
