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

[James Almer]

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.