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

[wm4]

On Mon, 17 Apr 2017 18:14:45 -0700
Aaron Levinson <alevinsn at aracnet.com> wrote:

> 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.

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.