It would help the flow of discussion if you and/or your mailer did proper interleaved replies instead of top-posting.
On Wed, Feb 07, 2018 at 06:39:39PM +0000, Mironov, Mikhail wrote: > Not yet, I mean we will fix them, in libav or in AMF. Libav is not the place to fix missing #includes in AMF headers. > I quickly searched but did not find any formal standard on include guards. I > did see some recommendations in forums though. I suggest searching the C99 standard instead of random forums: ISO/IEC 9899:1999 (E) 7.1.3 Reserved identifiers — All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use. > To get some statistics: I checked few header files in Windows SDK, OpenCL > SDK, OpenGL, Intel Media SDK - I see all kind of styles including __FILE_H__. > For example: > https://github.com/Intel-Media-SDK/MediaSDK/blob/master/api/include/mfxvideo.h Yes, bad practices abound. People look at kernel headers or libc headers and wrongly believe that that is the example to follow. Then others look at other projects for inspiration and cargo cult the error around. Please be a good example and fix it instead of perpetuating this malpractice. It's as easy as find . -name \*.h | xargs sed -i -e 's/ __AMF\(.*\)__/ AMF_\1/' from the root of the AMF source tree or the include dir (requires GNU sed). At least one of the examples you cite above is from Intel, you could fix that one as well :) Feel free to also run find . -name \*.h | xargs sed -i -e 's/AMF_Ambisonic2SRendererHW__h/AMF_Ambisonic2SRendererHW_h/' to fix a silly typo. > > -----Original Message----- > > From: libav-devel [mailto:[email protected]] On Behalf Of Diego > > Biurrun > > Sent: February 7, 2018 12:37 PM > > To: libav development <[email protected]> > > Subject: Re: [libav-devel] Add HW H.264 and HEVC encoding for AMD GPUs > > based on AMF SDK > > > > On Wed, Feb 07, 2018 at 04:27:30PM +0000, Mironov, Mikhail wrote: > > > Warnings should be fixed. More comments inline. > > > > Fixed as in fixed if I pull in fresh headers from Git? > > > > > > On Sun, Feb 04, 2018 at 07:04:39PM +0000, Mironov, Mikhail wrote: > > > > > I have a developer who will cover AMF integration on regular basis > > > > (together with me). He starts tomorrow and will be up to date soon. > > > > > I will ask him to cleanup the things you mentioned if you will not > > > > > submit the > > > > changes first. > > > > > He is on BCC for now. > > > > > > > > It seems a bit more cleanup is needed on your side as well: > > > > > > > > /home/libav/libs/include/AMF/core/Platform.h:431:16: error: implicit > > > > declaration of function ‘memcmp’ > > > > [-Werror=implicit-function-declaration] > > > > > > > > Indeed that header is missing a string.h #include and things break > > > > on one of my systems. > > > > > > > > Also, there are a bunch of warnings: > > > > > > > > ~/src/build/amf $ make libavcodec/amfenc.o > > > > CC libavcodec/amfenc.o > > > > /home/diego/src/libav.vanilla/libavcodec/amfenc.c: In function > > > > ‘amf_init_context’: > > > > /home/diego/src/libav.vanilla/libavcodec/amfenc.c:159:51: warning: > > > > passing argument 2 of ‘ctx->trace->pVtbl->RegisterWriter’ from > > > > incompatible pointer type [-Wincompatible-pointer-types] > > > > ctx->trace->pVtbl->RegisterWriter(ctx->trace, ctx->writer_id, > > > > (AMFTraceWriter *)&ctx->tracer, 1); > > > > ^~~ > > > > /home/diego/src/libav.vanilla/libavcodec/amfenc.c:159:51: note: > > > > expected ‘const wchar_t * {aka const int *}’ but argument is of type > > > > ‘char > > *’ > > > > /home/diego/src/libav.vanilla/libavcodec/amfenc.c:160:51: warning: > > > > passing argument 2 of ‘ctx->trace->pVtbl->SetWriterLevel’ from > > > > incompatible pointer type [-Wincompatible-pointer-types] > > > > ctx->trace->pVtbl->SetWriterLevel(ctx->trace, ctx->writer_id, > > > > AMF_TRACE_TRACE); > > > > ^~~ > > > > /home/diego/src/libav.vanilla/libavcodec/amfenc.c:160:51: note: > > > > expected ‘const wchar_t * {aka const int *}’ but argument is of type > > > > ‘char > > *’ > > > > /home/diego/src/libav.vanilla/libavcodec/amfenc.c: In function > > > > ‘ff_amf_encode_close’: > > > > /home/diego/src/libav.vanilla/libavcodec/amfenc.c:265:57: warning: > > > > passing argument 2 of ‘ctx->trace->pVtbl->UnregisterWriter’ from > > > > incompatible pointer type [-Wincompatible-pointer-types] > > > > ctx->trace->pVtbl->UnregisterWriter(ctx->trace, > > > > ctx->writer_id); > > > > ^~~ > > > > /home/diego/src/libav.vanilla/libavcodec/amfenc.c:265:57: note: > > > > expected ‘const wchar_t * {aka const int *}’ but argument is of type > > > > ‘char > > *’ > > > > > > > > One could just cast them away, but probably it's better to ajust the > > > > types. > > > > > > > > > > > > Also, I noticed two issues in your headers (at a glance): > > > > > > > > 1) You recreate parts of inttypes.h, i.e. > > > > > > > > #define AMFPRId64 "lld" > > > > > > > > and similar - why, oh, why? What ass-backwards systems are you trying > > to > > > > support there? > > > > > > I don’t remember right now which system required the custom macro but it > > was real issue five years ago. > > > This is AMF SDK issue, not libav integration, we should clean this up > > though. > > > > 5 years ago on Linux or with some of those horrible, now-obsolete MSVC > > versions? The latter I believe right away, but finding Linux systems that > > were > > not C99 compliant was difficult even in 2012... > > > > > > 2) You are invading system namespace: > > > > > > > > #ifndef __AMFPlatform_h__ > > > > > > > > Identifiers starting with __ (or _ and an uppercase letter) are > > > > reserved > > > > for the system. Should be easy enough to replace these with > > > > > > This is a decision made for AMF SDK which is external for libav > > > integration. > > Which standard or recommendation are you refer to? > > > > The C standard reserves those identifiers for the system, i.e. the kernel > > or the > > C library. > > > > Diego > > _______________________________________________ > > libav-devel mailing list > > [email protected] > > https://lists.libav.org/mailman/listinfo/libav-devel > _______________________________________________ > libav-devel mailing list > [email protected] > https://lists.libav.org/mailman/listinfo/libav-devel _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
