nu774 / fdkaac

command line encoder frontend for libfdk-aac
Other
260 stars 58 forks source link

process 32-bit input if possible (i.e. respect aac:INT_PCM type) #41

Closed DmitryYudin closed 4 years ago

nu774 commented 4 years ago

Hi, thanks for your work. Respecting INT_PCM type defined in the library seems a good thing.

However, when INT_PCM can be 32-bit? It's declared in libSYS/machine_type.h like this:

typedef signed short SHORT;
#define SHORT_BTIS 16
typedef SHORT INT_PCM;

It seems to me that INT_PCM is assumed to be always 16 bit. (And it seems no way is provided for configuring the size of INT_PCM)

DmitryYudin commented 4 years ago

Hi,

There are two items to change: INT_PCM and SAMPLE_BITS macro. You may find that the rest of stuff (WAV BITS, SAMPLE MAX) is not used. Actually, it would be much better if implementing this as the following within AAC:

#define SAMPLE_BITS 16 // 32
#if SAMPLE_BITS == 16
typedef SHORT INT_PCM;
#else
typedef INT INT_PCM;
#endif

Check it out)

nu774 commented 4 years ago

OK, so INT_PCM is defined as 16bit signed integer, but this patch allows you to modify the official header of libfdk-aac to change the size of INT_PCM type, right? It seems to be a very specific needs...

Anyways, I don't think separated pcm_sint32_converter to be necessary. Can you make pcm_sint16_converter to support both cases? (by ifdef of SAMPLE_BITS, or by runtime checking sizeof INT_PCM)

DmitryYudin commented 4 years ago

... but this patch allows you to modify the official header of libfdk-aac to change the size of INT_PCM type, right?

Not "allows to modify ...", but keep frontend aligned with the FDK-library. The INP_PCM and SAMPLE_BITS are API level macros. This is by design. They can be changed by FDK user to feed samples with more than 16-bit accuracy to encoder (i.e. extend dinamic range) or get more than 16-bit from the decoder. ((There are few more macro inside FDK to control coefficients precision.))

... both cases?

ok

DmitryYudin commented 4 years ago

Done. Up to you to rename 'pcm_sint16_xxx' to 'pcm_sint_xxx'

nu774 commented 4 years ago

Merged. Thanks.