[pulseaudio-discuss] [PATCH v13 05/10] bluetooth: Add A2DP aptX and aptX HD codecs support

[Georg Chini]

On 02.12.19 15:47, Tanu Kaskinen wrote:
> On Mon, 2019-12-02 at 13:00 +0100, Pali Rohár wrote:
>> On Sunday 01 December 2019 12:24:07 Georg Chini wrote:
>>> On 30.11.19 23:39, Pali Rohár wrote:
>>>> On Saturday 30 November 2019 22:43:47 Georg Chini wrote:
>>>>> On 06.10.19 19:58, Pali Rohár wrote:
>>>>>> This patch provides support for aptX and aptX HD codecs in bluetooth A2DP
>>>>>> profile. It uses open source LGPLv2.1+ licensed libopenaptx library which
>>>>>> can be found at https://github.com/pali/libopenaptx.
>>>>>>
>>>>>> aptX for s24 stereo samples provides fixed 6:1 compression ratio and
>>>>>> bitrate 352.8 kbit/s, aptX HD provides fixed 4:1 compression ratio and
>>>>>> bitrate 529.2 kbit/s.
>>>>>>
>>>>>> According to soundexpert research, aptX codec used in bluetooth A2DP is no
>>>>>> better than SBC High Quality settings. And you cannot hear difference
>>>>>> between aptX and SBC High Quality, aptX is just a copper-less overpriced
>>>>>> audio cable.
>>>>>>
>>>>>> aptX HD is high-bitrate version of aptX. It has clearly noticeable increase
>>>>>> in sound quality (not dramatic though taking into account the increase in
>>>>>> bitrate).
>>>>>>
>>>>>> http://soundexpert.org/news/-/blogs/audio-quality-of-bluetooth-aptx
>>>>> One general remark: I would consider passing the a2dp_codec as argument
>>>>> to the API functions. This way you can avoid having to create one function
>>>>> per codec variant. Instead you can use the vendor ID/codec ID or just the
>>>>> codec name as a key to a "case" or "if" instruction to distinguish between
>>>>> different codec variants. I think especially in the SBC case the code would
>>>>> be better readable. In this patch you could avoid duplicating functions
>>>>> if you can read the vendor and codec ID from the a2dp_codec instead of
>>>>> hard coding them.
>>>> I do not think that passing codec structure into every function is a big
>>>> win. In your suggestion, instead of N functions (one for each codec) you
>>>> would have just one function with N if-then-else-else-else... branches,
>>>> one branch for each codec. Currently common parts for all codecs is
>>>> already in sub-function with suffix _common (so code is not duplicated).
>>> I do not think it would produce less code, but the code would be
>>> easier to read.
>>> Now you have to check which codec variant uses which function,
>>> then you have to check this function for the arguments of the
>>> common function and then you can look what the common function
>>> does.
>>> With passing the codec that could be avoided: All (or at least most)
>>> codec variants would use the same function
>> That is not fully truth. SBC XQ variant needs to check that remote side
>> supports configuration for SBC XQ. SBC LQ needs to check that remote
>> side supports configuration for SBC LQ.
>>
>> If variants would be same, then one function for checking would be
>> enough. But they are variants of just because codecs are not same.
>>
>>> and you could directly
>>> see what happens for each variant in this function. Effectively, multiple
>>> functions would be replaced by if or case instructions as you already
>>> said, but for me that would be the purpose of the change.
>>>
>>> Maybe we should ask Tanu for an opinion? I can live with the current
>>> approach but would prefer passing the codec.
>> Both approaches are (if implemented) correctly should provide equivalent
>> set of features. So there is no functionality lost when choosing either
>> my our your implementation variant.
>>
>> So maybe Tanu (or somebody else) could give us some comments? If my
>> currently implemented variant of code API is OK or if Georg's variant
>> should be used and therefore changing existing code and code in my
>> patches to it...
> I'm fine with either. I vote for keeping the current API, since that
> means less work.
>
OK, so let's keep it.