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

[Tanu Kaskinen]

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.

-- 
Tanu

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