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 _______________________________________________ pulseaudio-discuss mailing list pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
