| Message ID | 1994689946.824459.1520690576097@mail.libero.it |
|---|---|
| State | New |
| Headers | show |
On Sat, 10 Mar 2018 15:02:56 +0100 (CET)
Carlo Bramini <carlo.bramix@libero.it> 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
On Sat, 10 Mar 2018 15:02:56 +0100 (CET)
Carlo Bramini <carlo.bramix@libero.it> 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.
2018-03-10 15:02 GMT+01:00 Carlo Bramini <carlo.bramix@libero.it>: > 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
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 <ceffmpeg@gmail.com> ha scritto: > > 2018-03-10 15:02 GMT+01:00 Carlo Bramini <carlo.bramix@libero.it>: > > > 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
2018-03-11 11:27 GMT+01:00 Carlo Bramini <carlo.bramix@libero.it>: > 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
Hello everyone, > Il 11 marzo 2018 alle 11.42 Carl Eugen Hoyos <ceffmpeg@gmail.com> ha scritto: > > 2018-03-11 11:27 GMT+01:00 Carlo Bramini <carlo.bramix@libero.it>: > > > 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.
2018-03-23 12:35 GMT+01:00, Carlo Bramini <carlo.bramix@libero.it>: > Hello everyone, > >> Il 11 marzo 2018 alle 11.42 Carl Eugen Hoyos <ceffmpeg@gmail.com> ha >> scritto: >> >> 2018-03-11 11:27 GMT+01:00 Carlo Bramini <carlo.bramix@libero.it>: >> >> > 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
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.
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 [...]
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 <stdint.h> +#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] = {
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.