Expose encoding API defined by Qualcomm library #5
Conversation
|
@pali, I've made only offline verification. Please, verify this code with some real hardware capable of aptX, aptX HD decoding, or perform offline verification by yourself. |
|
Hello @arkq! I have looked at your changes in this pull request and I do not like them. The main issue is that Qualcomm is insane and unsuitable for either low-latency or fast operations. In most cases audio applications are using one linear stereo buffer (either in S16LE/BE, S24LE/BE, S32LE/BE or S24 with padding to S32) and not divided into separate channels. Also this API expects that caller correctly allocates needed memory for library structures. This is something which is not part of my libopenaptx API and I do not want to provide it. And the last part is that I dislike mixing camel case, underlines and kebab case naming conventions in public API part of library. For me it looks like "poisoning" public API of my new libopenaptx library which I tried to design in better way. If public API has bugs it means that users would use it incorrectly and this is source of problems. Plus there are more problems with this patch. Logical errors in current implementation:
Semantic errors in API:
Also changes from |
|
These two problems (unused member/variable - unfinished implementation) and usage of static storage are basically no-go for inclusion into libopenaptx. If your purpose is to use libopenaptx on Android phones, I would rather suggest to provide separate wrapper library which exports that Android Qualcomm API and calls libopenaptx functions. Android is already slow and latency orientated, so nobody would complain. But personally I do not see any benefit for adding this Qualcomm API into libopenaptx library. I see just problems which it can introduce it. And also I'm not very happy that I would have to maintain another set of public API for in libopenaptx library. Therefore external wrapper library for Andorid which I would not have to maintain would be better solution. But let me know your opinions about it. What do you think... |
|
I don't want to rudely intrude in your discussion, but sourcing aptX-HD via bluealsa with this patch for libopenaptx works great! you're both onto something nice here. cheers, |
The case is abut API, not Qualcomm library. If your library will support Qualcomm API, then "insane and unsuitable" library might be replaced by the proper one. And about the channel dividing thingy, it's only a matter of API, not latency or fast or slow operations. Other encoders could also get input from separate buffers, because later they need to separate channels (in most cases) anyway. In fact, for aptX, is might be faster when input is already separated. If you configure your hardware to operate on separate channels (not interleaved), then you can run QMF directly on these buffers - no unnecessary data copying.
Then you can operate in an environment where there is no heap (but of course that's not the case for e.g. Linux, anyway, Qualcomm API allows such operation mode).
Agree. Someone was on acid apparently, when inventing these names :D
OK, my bad. Can be removed, it's not needed with ffmpeg aptX encoder anyway.
This function is not thread safe, indeed. I might add an explicit comment in the header file, just like here.
The
Could you point me to "dealing with algorithmic complexity and internal codec delays" before
You are using
I totally agree, maintaining something is a burden. But I think, that there no much to maintain in the case of this extra public API, unless you intent to write this encoder by yourself from scratches. But maybe I'm wrong... :) Anyway, if you feel that libopenaptx is not the place for Qualcomm API, please close this PR. |
Apparently this is part of that (insane) Qualcomm API. With swap parameter you specifty if you want to swap endianity of PCM samples. Therefore if swap is true then on x86 system with Qualcomm API it expects S16BE/S32BE samples, not S16LE/S32LE. swap is just misleading name in API, I would call it either endianity (with enum) or bigendian (bool).
Sorry for confusion, I mean that Qualcomm API is insane. I'm not saying/reviewing anything about Qualcomm implementation.
I really do not want to support such thing. Moreover libopenaptx is not written neither in floating-point and neither optimized for DSP. Is somebody willing to use libopenaptx on system without heap? If there are such users, I would like to talk with them to find some solution. But if there is nobody I really do not want to care about it.
This is something which I really do not like. I have there in libopenaptx thread-safe API suitable for multithreaded application and there is a request to add a new function with into public API which is not thread-safe. I see this as a real problem as new developers may choose to start using that Qualcomm API with libopenaptx library and can introduce bugs, just because they chosen bad API provided by libopenaptx library.
I see... Do not know if this is easily fixable... I will think about iy.
Ou, sorry. I exchanged encode and decode procedure. Encoding is OK, so ignore this comment. Qualcomm API just do not provide anything like
Should not
My plan was to rewrite encoder to floating-point (or rather adds second floating-point implementation) for performance reasons, but I had not find time to do it.
If whole pull request is for Android, I'm thinking if it would not be better to write just small wrapper specially prepared for Android system, for how it uses Qualcomm API. It does not need thread-safe support and IIRC it also does not use multiple instances of encoder, so this could be possible to implement as wrapper around libopenaptx. And therefore it does not have to be as part of libopenaptx library. I will think more about it. Also in Qualcomm API aptxbtenc_encodestereo() function takes int16_t, not int32_t. I guess that due to Linux X86 and ARM ABI function arguments are passed in registers and because registers are 32bit, you have not spotted any problem. |
|
What about following wrapper for Android? I have not tested it, but idea is simple... #include <stdio.h>
#include <openaptx.h>
struct ctx_state {
unsigned char swapendian;
unsigned char ctx_i;
};
struct ctx_state glob_ctx_state;
static struct aptx_context *ctx[10];
static unsigned char ctx_next;
static char build[60];
__attribute__((constructor)) static void init(void) {
snprintf(build, sizeof(build), "libopenaptx-%d.%d.%d", aptx_major, aptx_minor, aptx_patch);
}
__attribute__((destructor)) static void fini(void) {
unsigned char i;
for (i = 0; i < sizeof(ctx)/sizeof(ctx[0]); i++) {
if (ctx[i])
aptx_finish(ctx[i]);
}
}
const char *aptxbtenc_version(void) {
return build + sizeof("libopenaptx-")-1;
}
const char *aptxbtenc_build(void) {
return build;
}
int aptxbtenc_init(void *state, short swapendian) {
struct ctx_state *ctx_state = state;
if (!ctx_state)
return 1;
ctx_state->swapendian = !!swapendian;
ctx_state->ctx_i = ctx_next;
if (!ctx[ctx_state->ctx_i])
ctx[ctx_state->ctx_i] = aptx_init(0);
else
aptx_reset(ctx[ctx_state->ctx_i]);
ctx_next = (ctx_next+1) % (sizeof(ctx)/sizeof(ctx[0]));
return ctx[ctx_state->ctx_i] ? 0 : 1;
}
int SizeofAptxbtenc(void) {
return sizeof(struct ctx_state);
}
void *NewAptxEnc(short swapendian) {
if (aptxbtenc_init(&glob_ctx_state, swapendian) != 0)
return NULL;
return &glob_ctx_state;
}
int aptxbtenc_encodestereo(void *state, int *pcmL, int *pcmR, unsigned short *buffer) {
struct ctx_state *ctx_state = state;
#define PCM(val) ((val >> 8) & 0xFF), ((val >> 16) & 0xFF), ((val >> 24) & 0xFF)
const unsigned char input[24] = { PCM(pcmL[0]), PCM(pcmR[0]), PCM(pcmL[1]), PCM(pcmR[1]), PCM(pcmL[2]), PCM(pcmR[2]), PCM(pcmL[3]), PCM(pcmR[3]) };
unsigned char output[4];
size_t written;
if (aptx_encode(ctx[ctx_state->ctx_i], input, sizeof(input), output, sizeof(output), &written) != sizeof(input) || written != sizeof(output))
return 1;
if (ctx_state->swapendian) {
buffer[0] = ((unsigned short)output[1] << 8) | output[0];
buffer[1] = ((unsigned short)output[3] << 8) | output[2];
} else {
buffer[0] = ((unsigned short)output[0] << 8) | output[1];
buffer[1] = ((unsigned short)output[2] << 8) | output[3];
}
return 0;
} |
No, that's not the swap/endianess parameter function.
OK, we can drop
Hmm... maybe indeed it will work that way. I was compiling
Nope. Qualcomm API takes pointers to 32-bit arrays (for left and right channel) with s16le samples, and puts output to uint16 (imho, that's why endianess param was added) buffer. |
You are right. I have checked it and samples for non-HD variant are S16 but stored in int32 array. Another insanity in Qualcomm API.
I will recheck it. Seems that API is even more insane as I thought... |
I checked it and I was wrong. Still this parameter specify endianity, but not endianty of input PCM samples (they are expected to be in host native endianity), but rather endianity of output |
Qualcomm aptX library is designed to be as fast as possible. I've got no numbers before me, but it beats ffmpeg implementation :) The bottle neck is QMF, which should be implemented with SIMD - that's why Qualcomm API takes arrays of int32 even for s16le PCM.
Yes, this parameter is for output buffer, and with ffmpeg code is not required. Like I've said before, it can be omitted. |
That is why -O3 is in Makefile and why it should be built with make. New version of gcc can do some optimization via SSE instructions and it radically speed up library. -mavx2 is another great improvement, but that depends on Haswell processors. I documented both -O3 and -mavx2 in README file. You are right that vector/SIMD instructions are really needed, but performance either of QMF or dithering/convolution can be improved even more by rewriting those 24bit integer operations to floating point. That is why it was my plan.
It cannot be omitted if caller supply non-zero value, which indicates that caller wants to have output in swapped endianity. |
Exposing the same encoding API as Qualcomm apt-X library provides will allow to use libopenaptx library as a drop-in replacement for aptX and aptX-HD libraries available of various Android-based smartphones.
|
I'm going to close this pull request. Adding another API to libopenaptx, which even is worse than current API, does not make much sense. For external applications which depends on different API I proposed to use wrapper which converts that "different API" to current libopenaptx API. I really do not want to maintain in libopenaptx another set of API functions which just increase for me maintenance cost without any value. |
Exposing the same encoding API as Qualcomm apt-X library provides will
allow to use libopenaptx library as a drop-in replacement for aptX and
aptX-HD libraries available of various Android-based smartphones.