Re: [FFmpeg-devel] Type mismatch in ADPCM

[Michael Niedermayer]

On Sat, Mar 24, 2018 at 11:10:41AM +0100, Carlo Bramini wrote:
> Hello,
> 
> > > > > However, what about the patch attached for fixing the declaration
> > > > > of ff_adpcm_afc_coeffs[2][16]?
> > > > 
> > > > This would revert 10542491, a relatively recent change: Maybe Paul,
> > > > the author, wants to comment.
> > > > 
> > > > Do you think the code gets more readable?
> > > 
> > > Excuse me... I was just wondering what to do
> > 
> > You could answer above question.
> 
> I'm sorry, I was thinking that you did the question to Mr. Paul, since you 
> were requesting a comment from him.

> For me, I would say that this change makes the code more correct rather than 
> more readable, which should be a more important reason, but that's just my 
> opinion.

then please submit a proper git patch with that.
people may have missed this change due to the more exciting extern C++ stuff


[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

"I am not trying to be anyone's saviour, I'm trying to think about the
 future and not be sad" - Elon Musk



signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] Type mismatch in ADPCM

[Carlo Bramini]

Hello,

> > > > However, what about the patch attached for fixing the declaration
> > > > of ff_adpcm_afc_coeffs[2][16]?
> > > 
> > > This would revert 10542491, a relatively recent change: Maybe Paul,
> > > the author, wants to comment.
> > > 
> > > Do you think the code gets more readable?
> > 
> > Excuse me... I was just wondering what to do
> 
> You could answer above question.

I'm sorry, I was thinking that you did the question to Mr. Paul, since you were 
requesting a comment from him.
For me, I would say that this change makes the code more correct rather than 
more readable, which should be a more important reason, but that's just my 
opinion.

Sincerely.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] Type mismatch in ADPCM

[Carl Eugen Hoyos]

2018-03-23 12:35 GMT+01:00, Carlo Bramini :
> Hello everyone,
>
>> Il 11 marzo 2018 alle 11.42 Carl Eugen Hoyos  ha
>> scritto:
>>
>> 2018-03-11 11:27 GMT+01:00 Carlo Bramini :
>>
>> > Hello,
>> > I see. I expected that adding that could be considered out of the coding
>> > guidelines.
>>
>> You misunderstand:
>> The issue is not (afaict) the coding guidelines but the fact that the
>> change would mean we claim a compatibility that we cannot guarantee.
>>
>> > However, what about the patch attached for fixing the declaration
>> > of ff_adpcm_afc_coeffs[2][16]?
>>
>> This would revert 10542491, a relatively recent change: Maybe Paul,
>> the author, wants to comment.

>> Do you think the code gets more readable?
>>
>
> Excuse me... I was just wondering what to do

You could answer above question.

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] Type mismatch in ADPCM

[Carlo Bramini]

Hello everyone,

> Il 11 marzo 2018 alle 11.42 Carl Eugen Hoyos  ha scritto:
> 
> 2018-03-11 11:27 GMT+01:00 Carlo Bramini :
> 
> > Hello,
> > I see. I expected that adding that could be considered out of the coding 
> > guidelines.
> 
> You misunderstand:
> The issue is not (afaict) the coding guidelines but the fact that the
> change would mean we claim a compatibility that we cannot guarantee.
> 
> > However, what about the patch attached for fixing the declaration
> > of ff_adpcm_afc_coeffs[2][16]?
> 
> This would revert 10542491, a relatively recent change: Maybe Paul,
> the author, wants to comment. Do you think the code gets more
> readable?
> 

Excuse me... I was just wondering what to do, because I had not received any 
more replies about that patch. Thank you very much for your time.

Sincerely.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] Type mismatch in ADPCM

[Carl Eugen Hoyos]

2018-03-11 11:27 GMT+01:00 Carlo Bramini :
> Hello,
> I see. I expected that adding that could be considered out of the coding 
> guidelines.

You misunderstand:
The issue is not (afaict) the coding guidelines but the fact that the
change would mean we claim a compatibility that we cannot guarantee.

> However, what about the patch attached for fixing the declaration
> of ff_adpcm_afc_coeffs[2][16]?

This would revert 10542491, a relatively recent change: Maybe Paul,
the author, wants to comment. Do you think the code gets more
readable?

Please do not top-post here, Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] Type mismatch in ADPCM

[Carlo Bramini]

Hello,
I see. I expected that adding that could be considered out of the coding 
guidelines.
However, what about the patch attached for fixing the declaration of 
ff_adpcm_afc_coeffs[2][16]?
Although it works because the binary values are the same, this is wrong, 
independently by the fact that you are compiling for plain C or C++.

Sincerely.


> Il 10 marzo 2018 alle 20.38 Carl Eugen Hoyos  ha scritto:
> 
> 2018-03-10 15:02 GMT+01:00 Carlo Bramini :
> 
> > I noticed this thing because I compiled those sources with a more robust 
> > syntax check, by using C++ rather that plain C. At pratical level, nothing 
> > changed, except for the .h files that required to use the extern "C" 
> > keyword. I was wondering if you could evaulate the chance to add this 
> > feature to the include files.
> > It could be done directly by using some #ifdef/#endif, or perhaps by doing 
> > something like this, somewhere in a common file:
> > 
> > #ifdef __cplusplus
> > 
> > #define FFMPEG_EXTERN_C_BEGIN extern "C" {
> > 
> > #define FFMPEG_EXTERN_C_END }
> > 
> > #else
> > 
> > #define FFMPEG_EXTERN_C_BEGIN
> > 
> > #define FFMPEG_EXTERN_C_END
> > 
> > #endif
> 
> In addition to what was said:
> We cannot commit above because we would not test it
> and we cannot guarantee that it will always work: We
> provide C headers, if you want to use them in a C++
> project, it is your responsibility to make sure it works,
> not ours.
> 
> Carl Eugen
> 
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] Type mismatch in ADPCM

[Carl Eugen Hoyos]

2018-03-10 15:02 GMT+01:00 Carlo Bramini :

> I noticed this thing because I compiled those sources with a more robust 
> syntax check, by using C++ rather that plain C. At pratical level, nothing 
> changed, except for the .h files that required to use the extern "C" keyword. 
> I was wondering if you could evaulate the chance to add this feature to the 
> include files.
> It could be done directly by using some #ifdef/#endif, or perhaps by doing 
> something like this, somewhere in a common file:
>
> #ifdef __cplusplus
> #define FFMPEG_EXTERN_C_BEGIN extern "C" {
> #define FFMPEG_EXTERN_C_END   }
> #else
> #define FFMPEG_EXTERN_C_BEGIN
> #define FFMPEG_EXTERN_C_END
> #endif

In addition to what was said:
We cannot commit above because we would not test it
and we cannot guarantee that it will always work: We
provide C headers, if you want to use them in a C++
project, it is your responsibility to make sure it works,
not ours.

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] Type mismatch in ADPCM

[wm4]

On Sat, 10 Mar 2018 15:02:56 +0100 (CET)
Carlo Bramini  wrote:

> and just add FFMPEG_EXTERN_C_BEGIN and FFMPEG_EXTERN_C_END to the top and to 
> the bottom of each include file. You could take it as a suggestion, allowing 
> compilation with both GCC and G++ may show you some defects you cannot see 
> normally and give room to some improvements.

FFmpeg can't be built with C++ because it uses some C99 only features.
Besides, having to add all that casting to void*->structtype* would be
annoying.

Ignore my previous mail, shitty mail client maps + to "send mail" or
something.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] Type mismatch in ADPCM

[wm4]

On Sat, 10 Mar 2018 15:02:56 +0100 (CET)
Carlo Bramini  wrote:

> and just add FFMPEG_EXTERN_C_BEGIN and FFMPEG_EXTERN_C_END to the top and to 
> the bottom of each include file. You could take it as a suggestion, allowing 
> compilation with both GCC and G++ may show you some defects you cannot see 
> normally and give room to some improvements.

FFmpeg can't be built with C
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] Type mismatch in ADPCM

[Carlo Bramini]

Hello,
while working with ADPCM source code, I noticed that there is a type mismatch 
in ff_adpcm_afc_coeffs[2][16]. Inside libavcodec/adpcm_data.c it is declared as 
uint16_t:

https://github.com/FFmpeg/FFmpeg/blob/d168e78effd170377ec57f67bca05c9f0de91bca/libavcodec/adpcm_data.c#L109

but into libavcodec/adpcm_data.h it is declared as int16_t:

https://github.com/FFmpeg/FFmpeg/blob/d168e78effd170377ec57f67bca05c9f0de91bca/libavcodec/adpcm_data.h#L43

Those values are used as signed integer elsewhere.
So, I would like to suggest to fix this declaration with attached patch.

I noticed this thing because I compiled those sources with a more robust syntax 
check, by using C++ rather that plain C. At pratical level, nothing changed, 
except for the .h files that required to use the extern "C" keyword. I was 
wondering if you could evaulate the chance to add this feature to the include 
files.
It could be done directly by using some #ifdef/#endif, or perhaps by doing 
something like this, somewhere in a common file:

#ifdef __cplusplus
#define FFMPEG_EXTERN_C_BEGIN extern "C" {
#define FFMPEG_EXTERN_C_END   }
#else
#define FFMPEG_EXTERN_C_BEGIN
#define FFMPEG_EXTERN_C_END
#endif

and just add FFMPEG_EXTERN_C_BEGIN and FFMPEG_EXTERN_C_END to the top and to 
the bottom of each include file. You could take it as a suggestion, allowing 
compilation with both GCC and G++ may show you some defects you cannot see 
normally and give room to some improvements.

Sincerely.diff --git a/libavcodec/adpcm.c b/libavcodec/adpcm.c
index cd3bbd33c2..2753bd852d 100644
--- a/libavcodec/adpcm.c
+++ b/libavcodec/adpcm.c
@@ -1254,7 +1254,7 @@ static int adpcm_decode_frame(AVCodecContext *avctx, void 
*data,
 int coeff1, coeff2;
 int shift;
 unsigned int channel;
-uint16_t *samplesC;
+int16_t *samplesC;
 int count = 0;
 int offsets[6];
 
diff --git a/libavcodec/adpcm_data.c b/libavcodec/adpcm_data.c
index 4cce0a5857..e7ca5bc697 100644
--- a/libavcodec/adpcm_data.c
+++ b/libavcodec/adpcm_data.c
@@ -25,6 +25,8 @@
 
 #include 
 
+#include "adpcm_data.h"
+
 /* ff_adpcm_step_table[] and ff_adpcm_index_table[] are from the ADPCM
reference source */
 static const int8_t adpcm_index_table2[4] = {
@@ -106,9 +108,9 @@ const int8_t ff_adpcm_yamaha_difflookup[] = {
 -1, -3, -5, -7, -9, -11, -13, -15
 };
 
-const uint16_t ff_adpcm_afc_coeffs[2][16] = {
-{ 0, 2048, 0, 1024, 4096, 3584, 3072, 4608, 4200, 4800, 5120, 2048, 1024, 
64512, 64512, 63488 },
-{ 0, 0, 2048, 1024, 63488, 64000, 64512, 62976, 63288, 63236, 62464, 
63488, 64512, 1024, 0, 0 }
+const int16_t ff_adpcm_afc_coeffs[2][16] = {
+{ 0, 2048, 0, 1024, 4096, 3584, 3072, 4608, 4200, 4800, 5120, 2048, 1024, 
-1024, -1024, -2048, },
+{ 0, 0, 2048, 1024, -2048, -1536, -1024, -2560, -2248, -2300, -3072, 
-2048, -1024, 1024, 0, 0, }
 };
 
 const int16_t ff_adpcm_mtaf_stepsize[32][16] = {
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel