diff mbox

[FFmpeg-devel] avdevice/decklink: Remove pthread dependency

Message ID 1487804123-1539-1-git-send-email-zeranoe@gmail.com
State New
Headers show

Commit Message

Kyle Schwarz Feb. 22, 2017, 10:55 p.m. UTC 
DeckLink depends on pthread, and is silently disabled if pthread is missing.

Also fixes w32pthreads to support C++.
---
 compat/w32pthreads.h            | 31 +++++++++++++++++++------------
 configure                       |  4 ++--
 libavdevice/decklink_common.cpp |  3 ---
 libavdevice/decklink_common.h   |  4 +++-
 libavdevice/decklink_dec.cpp    |  3 ---
 libavdevice/decklink_enc.cpp    | 14 ++++++--------
 6 files changed, 30 insertions(+), 29 deletions(-)
 mode change 100644 => 100755 compat/w32pthreads.h
 mode change 100644 => 100755 libavdevice/decklink_common.cpp
 mode change 100644 => 100755 libavdevice/decklink_common.h
 mode change 100644 => 100755 libavdevice/decklink_dec.cpp
 mode change 100644 => 100755 libavdevice/decklink_enc.cpp

Comments

Ricardo Constantino Feb. 22, 2017, 11:26 p.m. UTC | #1 
On 22 February 2017 at 22:55, Kyle Schwarz <zeranoe@gmail.com> wrote:

> DeckLink depends on pthread, and is silently disabled if pthread is
> missing.
>
> Also fixes w32pthreads to support C++.
> ---
>  compat/w32pthreads.h            | 31 +++++++++++++++++++------------
>  configure                       |  4 ++--
>  libavdevice/decklink_common.cpp |  3 ---
>  libavdevice/decklink_common.h   |  4 +++-
>  libavdevice/decklink_dec.cpp    |  3 ---
>  libavdevice/decklink_enc.cpp    | 14 ++++++--------
>  6 files changed, 30 insertions(+), 29 deletions(-)
>  mode change 100644 => 100755 compat/w32pthreads.h
>  mode change 100644 => 100755 libavdevice/decklink_common.cpp
>  mode change 100644 => 100755 libavdevice/decklink_common.h
>  mode change 100644 => 100755 libavdevice/decklink_dec.cpp
>  mode change 100644 => 100755 libavdevice/decklink_enc.cpp
>
>
These modes are probably unintended?
Kyle Schwarz Feb. 22, 2017, 11:35 p.m. UTC | #2 
Indeed, an unintended change. 644 is preferred.

On Wed, Feb 22, 2017 at 6:26 PM, Ricardo Constantino <wiiaboo@gmail.com>
wrote:

> On 22 February 2017 at 22:55, Kyle Schwarz <zeranoe@gmail.com> wrote:
>
> > DeckLink depends on pthread, and is silently disabled if pthread is
> > missing.
> >
> > Also fixes w32pthreads to support C++.
> > ---
> >  compat/w32pthreads.h            | 31 +++++++++++++++++++------------
> >  configure                       |  4 ++--
> >  libavdevice/decklink_common.cpp |  3 ---
> >  libavdevice/decklink_common.h   |  4 +++-
> >  libavdevice/decklink_dec.cpp    |  3 ---
> >  libavdevice/decklink_enc.cpp    | 14 ++++++--------
> >  6 files changed, 30 insertions(+), 29 deletions(-)
> >  mode change 100644 => 100755 compat/w32pthreads.h
> >  mode change 100644 => 100755 libavdevice/decklink_common.cpp
> >  mode change 100644 => 100755 libavdevice/decklink_common.h
> >  mode change 100644 => 100755 libavdevice/decklink_dec.cpp
> >  mode change 100644 => 100755 libavdevice/decklink_enc.cpp
> >
> >
> These modes are probably unintended?
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Carl Eugen Hoyos Feb. 22, 2017, 11:37 p.m. UTC | #3 
2017-02-22 23:55 GMT+01:00 Kyle Schwarz <zeranoe@gmail.com>:
> DeckLink depends on pthread, and is silently disabled if pthread is missing.

> Also fixes w32pthreads to support C++.

Please make this a separate patch.

Thank you, Carl Eugen
Marton Balint Feb. 22, 2017, 11:55 p.m. UTC | #4 
On Wed, 22 Feb 2017, Kyle Schwarz wrote:

> DeckLink depends on pthread, and is silently disabled if pthread is missing.
>
> Also fixes w32pthreads to support C++.

You should split this into two patches, one the w32pthreads C++ support, 
other the decklink patch.

Also, I checked the decklink part, and I am a bit confused. I don't see 
how you ensure that ctx->frames_buffer number of frames are scheduled. 
Also, are you sure you use the condition/mutexes properly? Your code does 
not even contains a pthread_mutex_lock call, that seems odd.

What I would expect is to protect the scheduled frame counter with a 
mutex, and do a cond_signal in the scheduled frame completed callback. Or 
do you have something else in mind?

Regards,
Marton
Kyle Schwarz Feb. 23, 2017, 12:13 a.m. UTC | #5 
On Wed, Feb 22, 2017 at 6:55 PM, Marton Balint <cus@passwd.hu> wrote:
>
> On Wed, 22 Feb 2017, Kyle Schwarz wrote:
>
>> DeckLink depends on pthread, and is silently disabled if pthread is
>> missing.
>>
>> Also fixes w32pthreads to support C++.
>
>
> You should split this into two patches, one the w32pthreads C++ support,
> other the decklink patch.

Would it be best to resubmit as two new threads for both, or simply
split and reply to this thread?

> Also, I checked the decklink part, and I am a bit confused. I don't see how
> you ensure that ctx->frames_buffer number of frames are scheduled. Also, are
> you sure you use the condition/mutexes properly? Your code does not even
> contains a pthread_mutex_lock call, that seems odd.
>
> What I would expect is to protect the scheduled frame counter with a mutex,
> and do a cond_signal in the scheduled frame completed callback. Or do you
> have something else in mind?

I'm not terribly familiar with mutex and pthread, so you're probably right.

I'll look into your suggestions and come up with a revision.

Thanks,
Kyle
wm4 Feb. 23, 2017, 5:45 a.m. UTC | #6 
On Wed, 22 Feb 2017 14:55:23 -0800
Kyle Schwarz <zeranoe@gmail.com> wrote:

> DeckLink depends on pthread, and is silently disabled if pthread is missing.
> 
> Also fixes w32pthreads to support C++.
> ---


>  
> -        sem_post(&ctx->semaphore);
> +        pthread_mutex_unlock(&ctx->mutex);
>  
>          return S_OK;

> @@ -248,7 +246,7 @@ static int decklink_write_video_packet(AVFormatContext *avctx, AVPacket *pkt)
>      }
>  
>      /* Always keep at most one second of frames buffered. */
> -    sem_wait(&ctx->semaphore);
> +    pthread_cond_wait(&ctx->cond, &ctx->mutex);
>  
>      /* Schedule frame for playback. */
>      hr = ctx->dlo->ScheduleVideoFrame((struct IDeckLinkVideoFrame *) frame,

That doesn't really look convincing. These operations do different
things. Moreover, calling pthread_mutex_unlock without a preceding
pthread_mutex_lock is invalid. Calling pthread_cond_wait without
locking the mutex is an error.
diff mbox

Patch

diff --git a/compat/w32pthreads.h b/compat/w32pthreads.h
old mode 100644
new mode 100755
index 4ac2a99..45c565a
--- a/compat/w32pthreads.h
+++ b/compat/w32pthreads.h
@@ -77,7 +77,7 @@  typedef struct pthread_cond_t {
 
 static av_unused unsigned __stdcall attribute_align_arg win32thread_worker(void *arg)
 {
-    pthread_t *h = arg;
+    pthread_t *h = (pthread_t*)arg;
     h->ret = h->func(h->arg);
     return 0;
 }
@@ -270,7 +270,7 @@  static av_unused int pthread_cond_init(pthread_cond_t *cond, const void *unused_
     }
 
     /* non native condition variables */
-    win32_cond = av_mallocz(sizeof(win32_cond_t));
+    win32_cond = (win32_cond_t*)av_mallocz(sizeof(win32_cond_t));
     if (!win32_cond)
         return ENOMEM;
     cond->Ptr = win32_cond;
@@ -288,7 +288,7 @@  static av_unused int pthread_cond_init(pthread_cond_t *cond, const void *unused_
 
 static av_unused int pthread_cond_destroy(pthread_cond_t *cond)
 {
-    win32_cond_t *win32_cond = cond->Ptr;
+    win32_cond_t *win32_cond = (win32_cond_t*)cond->Ptr;
     /* native condition variables do not destroy */
     if (cond_init)
         return 0;
@@ -305,7 +305,7 @@  static av_unused int pthread_cond_destroy(pthread_cond_t *cond)
 
 static av_unused int pthread_cond_broadcast(pthread_cond_t *cond)
 {
-    win32_cond_t *win32_cond = cond->Ptr;
+    win32_cond_t *win32_cond = (win32_cond_t*)cond->Ptr;
     int have_waiter;
 
     if (cond_broadcast) {
@@ -337,7 +337,7 @@  static av_unused int pthread_cond_broadcast(pthread_cond_t *cond)
 
 static av_unused int pthread_cond_wait(pthread_cond_t *cond, pthread_mutex_t *mutex)
 {
-    win32_cond_t *win32_cond = cond->Ptr;
+    win32_cond_t *win32_cond = (win32_cond_t*)cond->Ptr;
     int last_waiter;
     if (cond_wait) {
         cond_wait(cond, mutex, INFINITE);
@@ -369,7 +369,7 @@  static av_unused int pthread_cond_wait(pthread_cond_t *cond, pthread_mutex_t *mu
 
 static av_unused int pthread_cond_signal(pthread_cond_t *cond)
 {
-    win32_cond_t *win32_cond = cond->Ptr;
+    win32_cond_t *win32_cond = (win32_cond_t*)cond->Ptr;
     int have_waiter;
     if (cond_signal) {
         cond_signal(cond);
@@ -397,20 +397,27 @@  static av_unused int pthread_cond_signal(pthread_cond_t *cond)
 static av_unused void w32thread_init(void)
 {
 #if _WIN32_WINNT < 0x0600
+    typedef void (*init_cond_var)(pthread_cond_t*);
+    typedef void (*wake_all_cond_var)(pthread_cond_t*);
+    typedef void (*wake_cond_var)(pthread_cond_t*);
+    typedef BOOL (*sleep_cond_var)(pthread_cond_t*, pthread_mutex_t*, DWORD);
+    typedef BOOL (*init_once_begin_init)(pthread_once_t*, DWORD, BOOL*, void**);
+    typedef BOOL (*init_once_complete)(pthread_once_t*, DWORD, void*);
+
     HANDLE kernel_dll = GetModuleHandle(TEXT("kernel32.dll"));
     /* if one is available, then they should all be available */
     cond_init      =
-        (void*)GetProcAddress(kernel_dll, "InitializeConditionVariable");
+        (init_cond_var)GetProcAddress((HMODULE)kernel_dll, "InitializeConditionVariable");
     cond_broadcast =
-        (void*)GetProcAddress(kernel_dll, "WakeAllConditionVariable");
+        (wake_all_cond_var)GetProcAddress((HMODULE)kernel_dll, "WakeAllConditionVariable");
     cond_signal    =
-        (void*)GetProcAddress(kernel_dll, "WakeConditionVariable");
+        (wake_cond_var)GetProcAddress((HMODULE)kernel_dll, "WakeConditionVariable");
     cond_wait      =
-        (void*)GetProcAddress(kernel_dll, "SleepConditionVariableCS");
+        (sleep_cond_var)GetProcAddress((HMODULE)kernel_dll, "SleepConditionVariableCS");
     initonce_begin =
-        (void*)GetProcAddress(kernel_dll, "InitOnceBeginInitialize");
+        (init_once_begin_init)GetProcAddress((HMODULE)kernel_dll, "InitOnceBeginInitialize");
     initonce_complete =
-        (void*)GetProcAddress(kernel_dll, "InitOnceComplete");
+        (init_once_complete)GetProcAddress((HMODULE)kernel_dll, "InitOnceComplete");
 #endif
 
 }
diff --git a/configure b/configure
index 6431313..ab9e005 100755
--- a/configure
+++ b/configure
@@ -2977,9 +2977,9 @@  avfoundation_indev_extralibs="-framework CoreVideo -framework Foundation -framew
 avfoundation_indev_select="avfoundation"
 bktr_indev_deps_any="dev_bktr_ioctl_bt848_h machine_ioctl_bt848_h dev_video_bktr_ioctl_bt848_h dev_ic_bt8xx_h"
 caca_outdev_deps="libcaca"
-decklink_indev_deps="decklink pthreads"
+decklink_indev_deps="decklink"
 decklink_indev_extralibs="-lstdc++"
-decklink_outdev_deps="decklink pthreads"
+decklink_outdev_deps="decklink"
 decklink_outdev_extralibs="-lstdc++"
 dshow_indev_deps="IBaseFilter"
 dshow_indev_extralibs="-lpsapi -lole32 -lstrmiids -luuid -loleaut32 -lshlwapi"
diff --git a/libavdevice/decklink_common.cpp b/libavdevice/decklink_common.cpp
old mode 100644
new mode 100755
index a3bc58d..7552059
--- a/libavdevice/decklink_common.cpp
+++ b/libavdevice/decklink_common.cpp
@@ -26,9 +26,6 @@ 
 #include <DeckLinkAPIDispatch.cpp>
 #endif
 
-#include <pthread.h>
-#include <semaphore.h>
-
 extern "C" {
 #include "libavformat/avformat.h"
 #include "libavformat/internal.h"
diff --git a/libavdevice/decklink_common.h b/libavdevice/decklink_common.h
old mode 100644
new mode 100755
index bfa2b08..4644cb4
--- a/libavdevice/decklink_common.h
+++ b/libavdevice/decklink_common.h
@@ -24,6 +24,7 @@ 
 
 #include <DeckLinkAPIVersion.h>
 
+#include "libavutil/thread.h"
 #include "decklink_common_c.h"
 
 class decklink_output_callback;
@@ -89,7 +90,8 @@  struct decklink_ctx {
     int frames_preroll;
     int frames_buffer;
 
-    sem_t semaphore;
+    pthread_mutex_t mutex;
+    pthread_cond_t cond;
 
     int channels;
 };
diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp
old mode 100644
new mode 100755
index 7df841b..151dabb
--- a/libavdevice/decklink_dec.cpp
+++ b/libavdevice/decklink_dec.cpp
@@ -21,9 +21,6 @@ 
 
 #include <DeckLinkAPI.h>
 
-#include <pthread.h>
-#include <semaphore.h>
-
 extern "C" {
 #include "config.h"
 #include "libavformat/avformat.h"
diff --git a/libavdevice/decklink_enc.cpp b/libavdevice/decklink_enc.cpp
old mode 100644
new mode 100755
index ad00224..5c3166f
--- a/libavdevice/decklink_enc.cpp
+++ b/libavdevice/decklink_enc.cpp
@@ -21,9 +21,6 @@ 
 
 #include <DeckLinkAPI.h>
 
-#include <pthread.h>
-#include <semaphore.h>
-
 extern "C" {
 #include "libavformat/avformat.h"
 #include "libavformat/internal.h"
@@ -78,7 +75,7 @@  public:
 
         av_frame_free(&avframe);
 
-        sem_post(&ctx->semaphore);
+        pthread_mutex_unlock(&ctx->mutex);
 
         return S_OK;
     }
@@ -120,7 +117,6 @@  static int decklink_setup_video(AVFormatContext *avctx, AVStream *st)
     ctx->output_callback = new decklink_output_callback();
     ctx->dlo->SetScheduledFrameCompletionCallback(ctx->output_callback);
 
-    /* Start video semaphore. */
     ctx->frames_preroll = c->time_base.den * ctx->preroll;
     if (c->time_base.den > 1000)
         ctx->frames_preroll /= 1000;
@@ -128,7 +124,8 @@  static int decklink_setup_video(AVFormatContext *avctx, AVStream *st)
     /* Buffer twice as many frames as the preroll. */
     ctx->frames_buffer = ctx->frames_preroll * 2;
     ctx->frames_buffer = FFMIN(ctx->frames_buffer, 60);
-    sem_init(&ctx->semaphore, 0, ctx->frames_buffer);
+    pthread_mutex_init(&ctx->mutex, NULL);
+    pthread_cond_init(&ctx->cond, NULL);
 
     /* The device expects the framerate to be fixed. */
     avpriv_set_pts_info(st, 64, c->time_base.num, c->time_base.den);
@@ -198,7 +195,8 @@  av_cold int ff_decklink_write_trailer(AVFormatContext *avctx)
     if (ctx->output_callback)
         delete ctx->output_callback;
 
-    sem_destroy(&ctx->semaphore);
+    pthread_mutex_destroy(&ctx->mutex);
+    pthread_cond_destroy(&ctx->cond);
 
     av_freep(&cctx->ctx);
 
@@ -248,7 +246,7 @@  static int decklink_write_video_packet(AVFormatContext *avctx, AVPacket *pkt)
     }
 
     /* Always keep at most one second of frames buffered. */
-    sem_wait(&ctx->semaphore);
+    pthread_cond_wait(&ctx->cond, &ctx->mutex);
 
     /* Schedule frame for playback. */
     hr = ctx->dlo->ScheduleVideoFrame((struct IDeckLinkVideoFrame *) frame,