Bug#843173: libfaad-dev: Implicit SBR detection via AudioSpecificConfig fails when char is signed
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 control: forwarded -1 https://sourceforge.net/p/faac/bugs/207/ control: tags -1 + patch -BEGIN PGP SIGNATURE- iQIcBAEBCAAGBQJYH0/0AAoJEMvqjpcMzVnf2tcQAKUTqkG1Wgv8eOjkMwgf6LI5 v4Iq93y2das6g/3VrXbmaba3x2I1ZnIONP6HBa/+9qpXkqkw4qvPpCNZ9F8XBcUf lS7BQUfINosIX0Z1WAuaKUtxT+TP8h1RlAiPyYjY/wD8QRl0zbycDqfzLnX1iWbB ee0Bf9GMPQI2RlR/1g4qJhSSG6aUcBwgejq1xRiJlB0EFj97XAAAXNmwp2Hirfy6 xDXpzMJDNuk14f/YU4ZYjMRrqh6ts4xI+rBgjDvoeX8e1qDDoCp44fAv6MbTyp4x bCEGvGkaY/dz2MCYDTZZpBgA0eNlT2sd69hZZCZyKYIJREBXwMhXd4YJPOlmHuj9 ghs3pJgkDW6gOZ7WCD9lvSfZI3Tc9/OcAKgN38CPMxBP7WEDaE2yC6oBh5HRL25L 59xeYX82RrzCGoUBPFO41so3UB33UClSdTP6TicCXpW6Vqc8ZVZFtCYFgE+jt/jC CgRx4TQqAdsjokIw7i2ni3Q9eUDy23viWSIz+BU45YVf5yU6B/LPfQ2mTOy+MJih tRbitqYao8FgVKqHRRIZngIXRrBEPF+TTqykwLeGTpahzKngRcyLLTJ3nJ/m1Yw1 38dBWYos97C3C3eUv1SF5tPl3a2C9FJpSdwm9b9FKJ/iqEbQv/8GhVs7+itN5OWh td8T0q0hVhv7suGqmN5G =yzLX -END PGP SIGNATURE-
Bug#843173: libfaad-dev: Implicit SBR detection via AudioSpecificConfig fails when char is signed
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 Hi there, Am Samstag, den 05.11.2016, 23:47 +0100 schrieb Stefan Pöschel: > To be honest: I have no knowledge in terms of ABI and which changes > affect it. But I have a strong feeling that both of my proposals > break it :-/ probably. As a rule of thumb, if a change in a library requires depending binaries to get rebuilt in order for the change to take effect, then it's an ABI break. Sure, I want to avoid that. > The first occasion also works without the cast as far as I can see. Strictly speaking, setting an unsigned variable to a negative value should be undefined behaviour, but I am pretty sure that the compiler does an implicit cast and simply sets the variable value to 0xff. > The second cast should IMO be annoted with a respective comment, to > describe the issue. Probably. I'll forward this issue upstream and see what he has to say about it. Thanks! - Fabian -BEGIN PGP SIGNATURE- iQIcBAEBCAAGBQJYH01bAAoJEMvqjpcMzVnf6x0QAKsIKRTvCkvrFcSkA9QEG0GW r+JpIBZR765BsXBsP8lJ3JSHLa3QeMhU7J/vaEF9K4amGjfCpkJjn6V7t8uH6ksL MH+BVczvVWNP31PQ09tHavulTmbBN1GDeqWlqaaspciyPFlEI61kWpjA11YoZrYV ssDYUbdCU4vOtN7ADjgBhw3UI/IfyquW3jVSv00sTIU5omw6n+fiXJN7IE6di3vR CJbf4E4j3QtIQUpTa4R2sPFyJ/vuwB9Vbom/w3GkmAWer/8hiNdBvAPeoBYUyPp6 PCRpbcC5fXUaDfpJb+eqxEk6COTk3qiDLF2vtu8wukPhQJuDE0Wz1VikmddnmPJl 8XJPZUJDsb1AzwZz9M8uTEAfNLzPiCKS1MxHrFXacp5EQIzA3APG31mmrPE759gn S0uVnjgJI/7xtsh6TRuI3OE2Z5XajhvyMaK+4MAlVSNuS3KOm29QDbAgbXsWT4FO qwFbOuxwAgmvCuNDGH/z/nd7DBKMEaW8PzrP4z9u08NGMoFj20BcO/FufmsXIsgM qaybbKqaINjbSwb2+MLbsirxTYcjkOxYnRJwxMNvSC4YIaDAHtN7zpcj/LMvWG11 22tHCQl169FV9G9rYB3HrPR0Pd7WbtmExXr9b56XPhsUQpckpqwb7bXZY421b0N+ ajOMxDsr2uEOxUtk+bSN =E/9U -END PGP SIGNATURE-
Bug#843173: libfaad-dev: Implicit SBR detection via AudioSpecificConfig fails when char is signed
Hi Fabian, Am 05.11.2016 um 20:34 schrieb Fabian Greffrath: > Did you mean to write "fails when char is unsigned" in the subject > line? This was indeed a typo - sorry! > Am Freitag, den 04.11.2016, 16:18 +0100 schrieb Stefan Pöschel: >> One way to fix this is to make the struct variable sbr_present_flag >> signed. >> Another way is to enforce signed char by compiling FAAD2 with >> -fsigned-char. > > I see that the fix is rather trivial, but wouldn't it introduce an ABI > (not API) break? To be honest: I have no knowledge in terms of ABI and which changes affect it. But I have a strong feeling that both of my proposals break it :-/ Am 05.11.2016 um 21:13 schrieb Fabian Greffrath: > will it help if we explicitely cast the (-1) to char in the two > occasions where it is used for sbr_present_flag? This seems to be an elegant way to me. The first occasion also works without the cast as far as I can see. Or do you think adding the cast there as well helps to clarify the issue? The second cast should IMO be annoted with a respective comment, to describe the issue. BTW: This problem of comparing to -1 also affects the API user: The mentioned internal function AudioSpecificConfigFromBitfile is exposed to the public API by the function NeAACDecAudioSpecificConfig (not mentioned in the documentation but in the include header). Furthermore the mentioned field sbr_present_flag of the mp4AudioSpecificConfig struct could return the problematic value -1 to the user (in case the _presence_ of SBR is not signalled; SBR itself may nonetheless be present or not). So the user has to compare to "(char) -1" as well for a proper result on all systems. This behaviour is left unchanged by your proposed fix and IMO should be left unchanged to not break the API (though the comparison the user has to make of course remains to be more elaborated). Regards, Stefan P.S. An interesting blog post about the three possible types of "char": http://blog.cdleary.com/2012/11/arm-chars-are-unsigned-by-default/
Bug#843173: libfaad-dev: Implicit SBR detection via AudioSpecificConfig fails when char is signed
Hi again,
will it help if we explicitely cast the (-1) to char in the two
occasions where it is used for sbr_present_flag?
- Fabian
diff --git a/libfaad/mp4.c b/libfaad/mp4.c
index 72b2af6..14e607a 100644
--- a/libfaad/mp4.c
+++ b/libfaad/mp4.c
@@ -174,7 +174,7 @@ int8_t AudioSpecificConfigFromBitfile(bitfile *ld,
#endif
#ifdef SBR_DEC
-mp4ASC->sbr_present_flag = -1;
+mp4ASC->sbr_present_flag = (char)-1;
if (mp4ASC->objectTypeIndex == 5)
{
uint8_t tmp;
@@ -276,7 +276,7 @@ int8_t AudioSpecificConfigFromBitfile(bitfile *ld,
/* no SBR signalled, this could mean either implicit signalling or no SBR in this file */
/* MPEG specification states: assume SBR on files with samplerate <= 24000 Hz */
-if (mp4ASC->sbr_present_flag == -1)
+if (mp4ASC->sbr_present_flag == (char)-1)
{
if (mp4ASC->samplingFrequency <= 24000)
{
signature.asc
Description: This is a digitally signed message part
Bug#843173: libfaad-dev: Implicit SBR detection via AudioSpecificConfig fails when char is signed
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 Hi Stefan, thanks for the bug report! Did you mean to write "fails when char is unsigned" in the subject line? Am Freitag, den 04.11.2016, 16:18 +0100 schrieb Stefan Pöschel: > One way to fix this is to make the struct variable sbr_present_flag > signed. > Another way is to enforce signed char by compiling FAAD2 with > -fsigned-char. I see that the fix is rather trivial, but wouldn't it introduce an ABI (not API) break? - Fabian -BEGIN PGP SIGNATURE- iQIcBAEBCAAGBQJYHjQrAAoJEMvqjpcMzVnfgkoQAOMWHhbkhl5QJ4c+O3EEmtBQ Doyz8HezqGLWud0PmHf9YOO7EK+gQusqN/3M/T/Qpe0/xbRE35ZiDO1kxNmiZk0O OPdrihKkSF5LWbYqJE3QgZ9gRcFtWx0OB4YeVkjoCTi+bhEPDwPrzxLQtAo2sqtB Sf39DsK6++zQfQNAzyxRZyVw6oKg9B9XuDblFcNUD16MgOCxlVlTswVR2iCD0+Pd 299geikSfG292LrFGiZD8PBE9S961mt4QXDAtcEdiznKrI6oZRpyShOPWWQPyqqm x6y/MlatAUeBEXfngRibdxyIT23edZUPAhyfvuhD0blgB5cYYYHZ/vKnXZjfEurA lEpor/KZA9QueKt31nLN5cpFPOGN5DtQX9Sy7AG9Cthcqyky9BWK2FnEEd8YFsYL NgJa4kbFEYbOl1UnBbSarQz8g69tXcfk2gshzJvU/+SaQKNEsyBrLoWNen9gkYEd c+3z0TPBAXVNw/MNA8LASapRPU6SLT7liqnb+RkdNB8Qnj+HLkwtWQyq67o+O9de UvU2jXkwCK4tTSjM++fMRKJFSAV7aCFQR4KeGrvfPDz9E89rkzPwPeI6uUnGSaND hYnYBGAPLZ37eDYA2uUkSf0ShGJn/OGOcyIyGji9XJyeWBKfewBG3KGO/JpNsSHB Aw1oAfQjVWnAoWuGfSal =wauB -END PGP SIGNATURE-
Bug#843173: libfaad-dev: Implicit SBR detection via AudioSpecificConfig fails when char is signed
Package: libfaad-dev
Version: 2.7-8
Severity: normal
I'm not sure if this an issue which affects Debian itself. As it affects FAAD2
users and the development seems (the original homepage is orphaned) to be
continued within a Debian Git repo, I post it here.
FAAD2 provides the ability to init the decoder with the help of an
AudioSpecificConfig
bit sequence (see also ISO/IEC 14496-3) via NeAACDecInit2. That function also
returns the "output" sample rate of the AAC stream.
When the AAC stream has the SBR extension (therefore: the core sample rate is
the half of the output sample rate), this can be signalled implicitely. In this
case the AudioSpecificConfig signals e.g. 24 kHz. FAAD2 treats streams with
sample rates of 24 kHz (or lower) automatically as having the SBR extension
(see the end of AudioSpecificConfigFromBitfile in /libfaad/mp4.c) and the
double of the core sample rate is returned as output sample rate.
Unfortunately, this does not work on systems where the char data type is
unsigned (e.g. Raspbian Jessie). In this case, the variable
mp4ASC->sbr_present_flag in the mentioned function
AudioSpecificConfigFromBitfile does not get initialized to -1. Therefore the
sample rate returned by NeAACDecInit2 is not doubled and still refers to the
core sample rate instead of the output sample rate.
The problem affects the installed version (see below) as well as the latest Git
version from git://anonscm.debian.org/pkg-multimedia/faad2.git
One way to fix this is to make the struct variable sbr_present_flag signed.
Another way is to enforce signed char by compiling FAAD2 with -fsigned-char.
I attach a short example in C which reproduces the problem. It prints out the
expected vs. the returned sample rate.
-- System Information:
Distributor ID: Raspbian
Description:Raspbian GNU/Linux 8.0 (jessie)
Release:8.0
Codename: jessie
Architecture: armv7l
Kernel: Linux 4.4.21-v7+ (SMP w/4 CPU cores)
Locale: LANG=en_GB.UTF-8, LC_CTYPE=en_GB.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash
Init: systemd (via /run/systemd/system)
Versions of packages libfaad-dev depends on:
ii libfaad2 2.7-8
libfaad-dev recommends no packages.
libfaad-dev suggests no packages.
-- no debconf information
// compile with:
// gcc -o faad2_test faad2_test.c -lfaad
#include
#include
#include
int main() {
NeAACDecHandle handle = NeAACDecOpen();
if(!handle) {
fprintf(stderr, "Error while NeAACDecOpen\n");
return 1;
}
/* AudioSpecificConfig structure (see also ISO/IEC 14496-3)
*
* 00010 = AudioObjectType 2 (AAC LC)
* = (core) sample rate index
* = (core) channel config
* 100 = GASpecificConfig with 960 transform (used for DAB+; does not matter here)
*
* SBR extension: implicit signaling used - libfaad2 automatically assumes SBR on sample rates <= 24 kHz
*/
int core_sr_index = 0b0110; // 24 kHz
int core_ch_config = 0b0010; // L/R
uint8_t asc[2];
asc[0] = 0b00010 << 3 | core_sr_index >> 1;
asc[1] = (core_sr_index & 0x01) << 7 | core_ch_config << 3 | 0b100;
// init decoder
unsigned long output_sr;
unsigned char output_ch;
long int init_result = NeAACDecInit2(handle, asc, sizeof(asc), _sr, _ch);
if(init_result != 0) {
fprintf(stderr, "Error while NeAACDecInit2\n");
NeAACDecClose(handle);
return 1;
}
printf("The output sample rate is: %ld (expected: 48000)\n", output_sr);
NeAACDecClose(handle);
return 0;
}
