On Mon, 17 Apr 2017 18:14:45 -0700 Aaron Levinson <alevi...@aracnet.com> wrote:
> On 4/17/2017 8:28 AM, wm4 wrote: > > On Mon, 17 Apr 2017 12:06:59 -0300 > > James Almer <jamr...@gmail.com> wrote: > > > >> On 4/17/2017 5:39 AM, Clément Bœsch wrote: > >>> On Sun, Apr 16, 2017 at 05:20:02PM -0700, Aaron Levinson wrote: > >>>> From 9e6a9e2b8d58f17c661a3f455e03c95587ec7b18 Mon Sep 17 00:00:00 2001 > >>>> From: Aaron Levinson <alevi...@aracnet.com> > >>>> Date: Sun, 16 Apr 2017 17:13:31 -0700 > >>>> Subject: [PATCH] libavutil/thread.h: Fixed g++ build error when > >>>> ASSERT_LEVEL is greater than 1 > >>>> > >>>> Purpose: libavutil/thread.h: Fixed g++ build error when ASSERT_LEVEL > >>>> is greater than 1. This is only relevant when thread.h is included by > >>>> C++ files. In this case, the relevant code is only defined if > >>>> HAVE_PTHREADS is defined as 1. Use configure --assert-level=2 to do > >>>> so. > >>>> > >>>> Note: Issue discovered as a result of Coverity build failure. Cause > >>>> of build failure pinpointed by Hendrik Leppkes. > >>>> > >>>> Comments: > >>>> > >>>> -- libavutil/thread.h: Altered ASSERT_PTHREAD_NORET definition such > >>>> that it uses av_make_error_string instead of av_err2str(). > >>>> av_err2str() uses a "parenthesized type followed by an initializer > >>>> list", which is apparently not valid C++. This issue started > >>>> occurring because thread.h is now included by the DeckLink C++ > >>>> files. The alteration does the equivalent of what av_err2str() > >>>> does, but instead declares the character buffer as a local > >>>> variable. > >>>> --- > >>>> libavutil/thread.h | 5 ++++- > >>>> 1 file changed, 4 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/libavutil/thread.h b/libavutil/thread.h > >>>> index 6e57447..f108e20 100644 > >>>> --- a/libavutil/thread.h > >>>> +++ b/libavutil/thread.h > >>>> @@ -36,8 +36,11 @@ > >>>> #define ASSERT_PTHREAD_NORET(func, ...) do { > >>>> \ > >>>> int ret = func(__VA_ARGS__); > >>>> \ > >>>> if (ret) { > >>>> \ > >>>> + char errbuf[AV_ERROR_MAX_STRING_SIZE] = ""; > >>>> \ > >>>> av_log(NULL, AV_LOG_FATAL, AV_STRINGIFY(func) > >>>> \ > >>>> - " failed with error: %s\n", av_err2str(AVERROR(ret))); > >>>> \ > >>>> + " failed with error: %s\n", > >>>> \ > >>>> + av_make_error_string(errbuf, AV_ERROR_MAX_STRING_SIZE, > >>>> \ > >>>> + AVERROR(ret))); > >>>> \ > >>>> abort(); > >>>> \ > >>>> } > >>>> \ > >>>> } while (0) > >>> > >>> I don't like limiting ourselves in the common C code of the project > >>> because C++ is a bad and limited language. Can't you solve this by bumping > >>> the minimal requirement of C++ version? > >> > >> We're already using C++11 when available because of atomics on mediacodec. > >> Also, just tried and it seems to fail even with C++14, so it just doesn't > >> work with C++. > >> > >> We could instead just make these strict assert wrappers work only on C > >> code by for example checking for defined(__cplusplus). > > > > Better solution: move all the code to a .c file. > > I've spent some time considering what would be involved in moving the > relevant code into a .c file. thread.h is a header file that needs to > be included to provide implementations of various pthread_ APIs on > Windows and OS/2 without needing to link to pthread on those OS's. If > pthread is available, it just includes pthread.h. So, it sort of acts > like a portability layer. Providing it in the form of a .h file acts as > a convenience. If the implementation were moved into a .c file, that > tends to imply that it will reside in one of the ffmpeg libraries, > likely libavutil. And that also implies that functions with the name > pthread_create, etc, would be exported by libavutil, which is a bad > idea. Instead, the right way to go is to provide a true threading > portability layer with exported functions that start with, say, > av_thread_. But, that's a decent project, and while I'm willing to > undertake it, I would like to see some support for this endeavor first. Good point, I forgot about that headache. There are various tricks to get around it, but since we actually need to have visible symbols to use it from C++, it all becomes a step harder. > However, there also seems to be some resistance to supporting C++ in > ffmpeg. The DeckLink C++ files were contributed to ffmpeg in February > 2014, over three years ago. While there is certainly no issue with > using C-specific functionality in .c files, there is certainly an issue > with doing so in header files that are intended to be used by any aspect > of the project, whether in .c or .cpp files. thread.h is an example of > a header file that should be suitable for use in either .c or .cpp > files. The patch that I submitted accomplishes exactly that. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel