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... > > > > > > > diff --git a/src/Makefile.am b/src/Makefile.am > > > > index b84c778cc..e317b7972 100644 > > > > --- a/src/Makefile.am > > > > +++ b/src/Makefile.am > > > > @@ -2164,6 +2164,12 @@ libbluez5_util_la_SOURCES += modules/bluetooth/a2dp-codec-sbc.c > > > > libbluez5_util_la_LIBADD += $(SBC_LIBS) > > > > libbluez5_util_la_CFLAGS += $(SBC_CFLAGS) > > > > +if HAVE_OPENAPTX > > > > +libbluez5_util_la_SOURCES += modules/bluetooth/a2dp-codec-aptx.c > > > > +libbluez5_util_la_CPPFLAGS += $(OPENAPTX_CPPFLAGS) > > > > +libbluez5_util_la_LDFLAGS += $(OPENAPTX_LDFLAGS) > > > > +endif > > > > + > > > > module_bluez5_discover_la_SOURCES = modules/bluetooth/module-bluez5-discover.c > > > > module_bluez5_discover_la_LDFLAGS = $(MODULE_LDFLAGS) > > > > module_bluez5_discover_la_LIBADD = $(MODULE_LIBADD) $(DBUS_LIBS) libbluez5-util.la > > > The part for the meson build system is missing. > > Sorry, I do not understand meson build system deeply enough to write > > needed rule for it (including searching & linking flags). > > > > Can somebody help with this part? > > I guess Arun can help here. > > > > > > > > diff --git a/src/modules/bluetooth/a2dp-codec-aptx.c b/src/modules/bluetooth/a2dp-codec-aptx.c > > > > new file mode 100644 > > > > index 000000000..2bd9e7652 > > > > --- /dev/null > > > > +++ b/src/modules/bluetooth/a2dp-codec-aptx.c > > ... > > > > + pa_log_error("Not suitable sample rate"); > > > > + return false; > > > > + } > > > > + } > > > > + > > > > + return true; > > > > +} > > > > + > > > > +static uint8_t fill_preferred_configuration(const pa_sample_spec *default_sample_spec, const uint8_t *capabilities_buffer, uint8_t capabilities_size, uint8_t config_buffer[MAX_A2DP_CAPS_SIZE]) { > > > Return type should be size_t and function should return (size_t) -1 on > > > error. > > No, it cannot. API expects that zero value is returned on error. > > Then the API should be changed to expect a negative value on error because > this is > the convention in PA (unless there is a very good reason not to do so). > > > > > > Also maximal size of A2DP cap buffer is 255 bytes, so it does not make > > sense to use bigger types. It is A2DP specific. > > I think because the function returns a size, size_t is the correct type for > the > return value, even if the maximum is 255, but that's not something I have > a strong opinion about. > > > > > > > > + a2dp_aptx_t *config = (a2dp_aptx_t *) config_buffer; > > > > + const a2dp_aptx_t *capabilities = (const a2dp_aptx_t *) capabilities_buffer; > > > > + > > > > + if (capabilities_size != sizeof(*capabilities)) { > > > > + pa_log_error("Invalid size of capabilities buffer"); > > > > + return 0; > > > > + } > > > > + > > > > + pa_zero(*config); > > > > + > > > > + if (!fill_preferred_configuration_common(default_sample_spec, capabilities, config, APTX_VENDOR_ID, APTX_CODEC_ID)) > > > > + return 0; > > > > + > > > > + return sizeof(*config); > > > > +} > > > > + > > > > +static uint8_t fill_preferred_configuration_hd(const pa_sample_spec *default_sample_spec, const uint8_t *capabilities_buffer, uint8_t capabilities_size, uint8_t config_buffer[MAX_A2DP_CAPS_SIZE]) { > > > Same here. > > > > > > > > > > + a2dp_aptx_hd_t *config = (a2dp_aptx_hd_t *) config_buffer; > > > > + const a2dp_aptx_hd_t *capabilities = (const a2dp_aptx_hd_t *) capabilities_buffer; > > ... > > > > > > + > > > > + PA_ONCE_BEGIN { > > > > +#if OPENAPTX_MAJOR == 0 && OPENAPTX_MINOR == 0 && OPENAPTX_PATCH == 0 > > > > + /* libopenaptx version 0.0.0 does not export version global variables */ > > > > + const int aptx_major = OPENAPTX_MAJOR; > > > > + const int aptx_minor = OPENAPTX_MINOR; > > > > + const int aptx_patch = OPENAPTX_PATCH; > > > > +#endif > > > This does not make sense because it is evaluated at compile time. So if > > > compiled on a system with version 0.0.0, it will never show anything else > > > and > > That is truth. But we know that this could happen. And I think that for > > debugging purposes it make sense to print version. > > > > > if compiled on a system with a higher version it will crash when run > > > on a system with 0.0.0. So I would skip displaying the version. > > Now question is if such thing is even supported. Compile program with > > new version of library and then run it with older. I guess that more > > libraries would crash in this case... > > If it works or not with an old library is just a question of the API. > If the API did not change, it should work. And in any case we > should not add code where we know it crashes or does the wrong > thing under certain conditions, so this should be removed. If you > need to know which version of the aptx library you are using, there > are surely other ways to find out. Major version of API is mean to be backward compatible. So new features / functions / symbols may appear in new version. I guess this is what other libraries are doing too. So compiling application with new version of library and then using old version does not have to work. But compiling with old version and using with new (with same major version) should work fine. > > > > > > > > + pa_log_debug("Using aptX codec implementation: libopenaptx %d.%d.%d from https://github.com/pali/libopenaptx";, aptx_major, aptx_minor, aptx_patch); > > > > + } PA_ONCE_END; > > > > + > > > > + return aptx_c; > > > > +} > > ... > > > > > > +static size_t encode_buffer(void *codec_info, uint32_t timestamp, const uint8_t *input_buffer, size_t input_size, uint8_t *output_buffer, size_t output_size, size_t *processed) { > > > > + struct aptx_context *aptx_c = (struct aptx_context *) codec_info; > > > > + size_t written; > > > > + > > > > + *processed = aptx_encode(aptx_c, input_buffer, input_size, output_buffer, output_size, &written); > > > > + if (PA_UNLIKELY(*processed == 0 || *processed != input_size)) > > > > + pa_log_error("aptX encoding error"); > > > Should that only produce a log message or does it indicate a severe error? > > It is error. And because *processed is set to different value as > > input_size, caller would handle this error (stop encoding and drop A2DP > > connection). > > I don't like that the caller has to check this. Can the check be done inside > the > function and the function return -1 on error? It can be changed... But I have not did it as it was used in this way also before. -- Pali Rohár pali.rohar@xxxxxxxxx _______________________________________________ pulseaudio-discuss mailing list pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
