[FFmpeg-devel] [PATCH] avdevice/decklink: Removed pthread dependency

[wm4]

On Sun, 16 Apr 2017 15:23:15 -0300
James Almer <jamrial at gmail.com> wrote:

> On 4/16/2017 2:17 PM, Aaron Levinson wrote:
> > On 4/15/2017 9:32 PM, James Almer wrote:  
> >> On 4/15/2017 7:41 AM, Marton Balint wrote:  
> >>>
> >>> On Thu, 13 Apr 2017, Aaron Levinson wrote:
> >>>  
> >>>> On 4/13/2017 3:40 PM, Marton Balint wrote:  
> >>>>>
> >>>>> On Thu, 13 Apr 2017, Aaron Levinson wrote:
> >>>>>  
> >>>>>> On 4/13/2017 2:12 AM, Hendrik Leppkes wrote:  
> >>>>>>> On Thu, Apr 13, 2017 at 10:36 AM, Aaron Levinson <alevinsn at aracnet.com>  
> >>>>>> wrote:  
> >>>>>>> Give it some time for the other changes to be reviewed by the people
> >>>>>>> that actually know decklink itself, you can include that in any new
> >>>>>>> versions of the patch then, no need to send one for that right now.
> >>>>>>>
> >>>>>>> - Hendrik  
> >>>>>>
> >>>>>> Actually, the coding changes made to the decklink source files in this
> >>>>>> patch have pretty much nothing to do with decklink itself, and anyone
> >>>>>> with familiarity with semaphores and pthread could review those changes.
> >>>>>>  In fact, all I really did was use already existing approaches for
> >>>>>> replacing a semaphore with the combination of a condition variable,
> >>>>>> mutex, and counter, and there are plenty of examples of how to do this
> >>>>>> on the Web.
> >>>>>>  
> >>>>>
> >>>>> Yeah, the changes look fine, please send an updated patch, I will apply
> >>>>> it after some final testing.
> >>>>>
> >>>>> Thanks,
> >>>>> Marton  
> >>>>
> >>>> Here's a new version of the patch with the pthreads dependency replaced with threads.
> >>>>  
> >>>
> >>> Thanks, applied.  
> >>
> >> Wouldn't it be simpler to add posix semaphore emulation to w32threads
> >> and os2threads?
> >> The former should be trivial, and probably even without the need to
> >> use mutexes or conditional variables given there's CreateSemaphore
> >> and ReleaseSemaphore for this purpose. Not sure about the latter.  
> > 
> > I addressed this in one of the commit log messages:  
> 
> Yes. I made the mistake of reading the commit message after
> reading the code and writing a reply, sorry about that :P
> 
> > 
> > -------------------------------------------------------------------------
> > 
> > -- libavdevice/decklink_enc.cpp:
> > a) Eliminated include of pthread.h and semaphore.h.
> > b) Replaced use of semaphore with the equivalent using a combination
> >    of a mutex, condition variable, and a counter
> >    (frames_buffer_available_spots).  In theory, libavutil/thread.h and
> >    the associated code could have been modified instead to add
> >    cross-platform implementations of the sem_ functions, but an
> >    inspection of the ffmpeg source base indicates that there are only
> >    two cases in which semaphores are used (including this one that was
> >    replaced), so it was deemed to not be worth the effort.
> > 
> > --------------------------------------------------------------------------
> > 
> > I considered this approach, but the thought of having to do this in os2threads.h pretty much dissuaded me.  I had thought that OS/2 was pretty much dead, but it appears that I was wrong.  There is a yearly conference, and a new version of OS/2 will soon be released.
> > 
> > However, the two places in ffmpeg that the sem_ functions were/are used are likely not relevant to OS/2 anyway:
> > 
> > -- DeckLink:  Blackmagic only provides drivers/kernel modules for Windows, Linux, and OS/X
> > -- avdevice/jack.c:  This pertains to www.jackaudio.org, and at least based on what I can see at https://github.com/jackaudio/jack2, there doesn't appear to be any build configuration for OS/2.  jack support also apparently needs sem_timedwait().
> > 
> > As such, someone could likely get away with just implementing the sem_ functions in w32threads.h.
> > 
> > Aaron  
> 
> We have a developer that keeps os2threads.h up to date, so he
> will probably add sem_t compat wrappers to it, if not now when
> a module that works on OS/2 needs it.
> 
> IMO, if you can and want then add win32 wrappers to w32threads
> right now. The pthread_once wrapper was added for one use case
> then as soon as it became available it started to see more use
> elsewhere in the tree. The same might happen to Semaphores.

Beware that semaphores don't work on OSX.