[FFmpeg-devel] libavutil/thread.h: Fixed g++ build error when ASSERT_LEVEL is greater than 1

[Aaron Levinson]

On 4/17/2017 8:28 AM, wm4 wrote:
> On Mon, 17 Apr 2017 12:06:59 -0300
> James Almer <jamrial at 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 <alevinsn at 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.

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.

Aaron Levinson