Re: [PATCH 08/11] winegstreamer: Genericize gstreamer callback system.

[Zebediah Figura]

On 2/4/20 11:17 AM, Derek Lesho wrote:
> Signed-off-by: Derek Lesho
>
> dlls/winegstreamer/gst_cbs.c  | 97 ++++++++++++++++++++++++++++++-----
>  dlls/winegstreamer/gst_cbs.h  | 27 +++++-----
>  dlls/winegstreamer/gstdemux.c | 75 +++++----------------------
>  3 files changed, 107 insertions(+), 92 deletions(-)

It might be nice to split this up into two patches, one to change the
names of functions, and one to move declarations into gst_cbs.c.

If not, I'd at least change "genericize" to something more properly
English, say, "Make the callback system more generic".
> diff --git a/dlls/winegstreamer/gst_cbs.c b/dlls/winegstreamer/gst_cbs.c
> index e7a4b41d55..50735a0344 100644
> --- a/dlls/winegstreamer/gst_cbs.c
> +++ b/dlls/winegstreamer/gst_cbs.c
> @@ -20,10 +20,79 @@
>
> #include <gst/gst.h>
>
> +#include "objbase.h"
> -
>
> #include "wine/list.h"
> +#include "wine/debug.h"
>
> #include "gst_cbs.h"
>
> +WINE_DEFAULT_DEBUG_CHANNEL(gstreamer);
> -
>
> +static pthread_key_t wine_gst_key;
> -
>
> +void mark_wine_thread(void)
> +{
> - /* set it to non-NULL to indicate that this is a Wine thread */
> - pthread_setspecific(wine_gst_key, &wine_gst_key);
>
> +}
> -
>
> +BOOL is_wine_thread(void)
> +{
> - return pthread_getspecific(wine_gst_key) != NULL;
>
> +}
> -
>
> +pthread_mutex_t cb_list_lock = PTHREAD_MUTEX_INITIALIZER;
> +pthread_cond_t cb_list_cond = PTHREAD_COND_INITIALIZER;
> +struct list cb_list = LIST_INIT(cb_list);
> -
>
> +void CALLBACK perform_cb(TP_CALLBACK_INSTANCE *instance, void *user)
> +{
> - struct cb_data *cbdata = user;
> -
> - if (cbdata->type < GSTDEMUX_MAX)
> -
>    forward_cb_gstdemux(cbdata);

I guess I'd prefer "perform_cb_gstdemux()", for consistency.

Also, while I don't object to renaming the callbacks to match GStreamer
names (and would probably prefer it), if you're going to have the
individual callback functions be specific to e.g. quartz vs mfplat vs
wmvcore (which is probably the best way to go), I don't particularly see
the point in renaming them to be "more generic".
> - else
> -
>    ERR("invalid cbdata struct\n");

I don't know that this is worth adding; I think the code is simple
enough that we don't need these kinds of sanity checks.
> -
> - pthread_mutex_lock(&cbdata->lock);
> - cbdata->finished = 1;
> - pthread_cond_broadcast(&cbdata->cond);
> - pthread_mutex_unlock(&cbdata->lock);
>
> +}
> -
>
> +static DWORD WINAPI dispatch_thread(void *user)
> +{
> - struct cb_data *cbdata;
> -
> - CoInitializeEx(NULL, COINIT_MULTITHREADED);
> -
> - pthread_mutex_lock(&cb_list_lock);
> -
> - while(1){
> -
>    pthread_cond_wait(&cb_list_cond, &cb_list_lock);
> -
> -
>    while(!list_empty(&cb_list)){
> -
>        cbdata = LIST_ENTRY(list_head(&cb_list), struct cb_data, entry);
> -
>        list_remove(&cbdata->entry);
> -
> -
>        TrySubmitThreadpoolCallback(&perform_cb, cbdata, NULL);
> -
>    }
> - }
> -
> - pthread_mutex_unlock(&cb_list_lock);
> -
> - CoUninitialize();
> -
> - return 0;
>
> +}
> -
>
> +void start_dispatch_thread(void)
> +{
> - pthread_key_create(&wine_gst_key, NULL);
> - CloseHandle(CreateThread(NULL, 0, &dispatch_thread, NULL, 0, NULL));
>
> +}
> -
>
> /* gstreamer calls our callbacks from threads that Wine did not create. Some
> - callbacks execute code which requires Wine to have created the thread
> - (critical sections, debug logging, dshow client code). Since gstreamer can't
>
> @@ -31,7 +100,7 @@
> - callbacks in code which avoids the Wine thread requirement, and then
> - dispatch those callbacks on a thread that is known to be created by Wine.
> -
> -
> - This file must not contain any code that depends on the Wine TEB!
> -
> - This thread must not run any code that depends on the Wine TEB!
> */
>
> static void call_cb(struct cb_data *cbdata)
> @@ -89,9 +158,9 @@ void existing_new_pad_wrapper(GstElement *bin, GstPad *pad, gpointer user)
>  {
>      struct cb_data cbdata = { EXISTING_NEW_PAD };
> - cbdata.u.existing_new_pad_data.bin = bin;
> - cbdata.u.existing_new_pad_data.pad = pad;
> - cbdata.u.existing_new_pad_data.user = user;
> -
>
> cbdata.u.pad_added_data.element = bin;
> -
>
> cbdata.u.pad_added_data.pad = pad;
> -
>
> cbdata.u.pad_added_data.user = user;
>
> call_cb(&cbdata);
>
> }
> @@ -127,7 +196,7 @@ void no_more_pads_wrapper(GstElement *decodebin, gpointer user)
>  {
>      struct cb_data cbdata = { NO_MORE_PADS };
> - cbdata.u.no_more_pads_data.decodebin = decodebin;
> -
>
> cbdata.u.no_more_pads_data.element = decodebin;
> cbdata.u.no_more_pads_data.user = user;
>
> call_cb(&cbdata);

While you're at it you could change "decodebin" to "element"; it's not
even true that it's always a decodebin for quartz anymore.
> @@ -138,15 +207,15 @@ GstFlowReturn request_buffer_src_wrapper(GstPad *pad, GstObject *parent, guint64
>  {
>      struct cb_data cbdata = { REQUEST_BUFFER_SRC };
> - cbdata.u.request_buffer_src_data.pad = pad;
> - cbdata.u.request_buffer_src_data.parent = parent;
> - cbdata.u.request_buffer_src_data.ofs = ofs;
> - cbdata.u.request_buffer_src_data.len = len;
> - cbdata.u.request_buffer_src_data.buf = buf;
> -
>
> cbdata.u.getrange_data.pad = pad;
> -
>
> cbdata.u.getrange_data.parent = parent;
> -
>
> cbdata.u.getrange_data.ofs = ofs;
> -
>
> cbdata.u.getrange_data.len = len;
> -
>
> cbdata.u.getrange_data.buf = buf;
>
> call_cb(&cbdata);
> - return cbdata.u.request_buffer_src_data.ret;
> - return cbdata.u.getrange_data.ret;
>
> }
>
> gboolean event_src_wrapper(GstPad *pad, GstObject *parent, GstEvent *event)
> @@ -192,9 +261,9 @@ void removed_decoded_pad_wrapper(GstElement *bin, GstPad *pad, gpointer user)
>  {
>      struct cb_data cbdata = { REMOVED_DECODED_PAD };
> - cbdata.u.removed_decoded_pad_data.bin = bin;
> - cbdata.u.removed_decoded_pad_data.pad = pad;
> - cbdata.u.removed_decoded_pad_data.user = user;
> -
>
> cbdata.u.pad_removed_data.element = bin;
> -
>
> cbdata.u.pad_removed_data.pad = pad;
> -
>
> cbdata.u.pad_removed_data.user = user;
>
> call_cb(&cbdata);
>
> }
> diff --git a/dlls/winegstreamer/gst_cbs.h b/dlls/winegstreamer/gst_cbs.h
> index 46f8add57c..3b391a7650 100644
> --- a/dlls/winegstreamer/gst_cbs.h
> +++ b/dlls/winegstreamer/gst_cbs.h
> @@ -43,7 +43,8 @@ enum CB_TYPE {
>      AUTOPLUG_BLACKLIST,
>      UNKNOWN_TYPE,
>      RELEASE_SAMPLE,
> - QUERY_SINK
> - QUERY_SINK,
> -
>    GSTDEMUX_MAX

Spacing
> };
>
> struct cb_data {
> @@ -55,11 +56,11 @@ struct cb_data {
>              gpointer user;
>              GstBusSyncReply ret;
>          } watch_bus_data;
> -
>    struct existing_new_pad_data {
> -
>        GstElement *bin;
> -
>    struct pad_added_data {
> -
>        GstElement *element;
>        GstPad *pad;
>        gpointer user;
> -
>    } existing_new_pad_data;
> -
>    } pad_added_data;
>    struct query_function_data {
>        GstPad *pad;
>        GstObject *parent;
>
> @@ -74,17 +75,17 @@ struct cb_data {
>              gboolean ret;
>          } activate_mode_data;
>          struct no_more_pads_data {
> -
>        GstElement *decodebin;
> -
>        GstElement *element;
>        gpointer user;
>    } no_more_pads_data;
> -
>    struct request_buffer_src_data {
> -
>    struct getrange_data {
>        GstPad *pad;
>        GstObject *parent;
>        guint64 ofs;
>        guint len;
>        GstBuffer **buf;
>        GstFlowReturn ret;
> -
>    } request_buffer_src_data;
> -
>    } getrange_data;
>    struct event_src_data {
>        GstPad *pad;
>        GstObject *parent;
>
> @@ -103,11 +104,11 @@ struct cb_data {
>              GstBuffer *buf;
>              GstFlowReturn ret;
>          } got_data_sink_data;
> -
>    struct removed_decoded_pad_data {
> -
>        GstElement *bin;
> -
>    struct pad_removed_data {
> -
>        GstElement *element;
>        GstPad *pad;
>        gpointer user;
> -
>    } removed_decoded_pad_data;
> -
>    } pad_removed_data;
>    struct autoplug_blacklist_data {
>        GstElement *bin;
>        GstPad *pad;
>
> @@ -139,12 +140,8 @@ struct cb_data {
>      struct list entry;
>  };
>
> -extern pthread_mutex_t cb_list_lock DECLSPEC_HIDDEN;
> -extern pthread_cond_t cb_list_cond DECLSPEC_HIDDEN;
> -extern struct list cb_list DECLSPEC_HIDDEN;
> -void CALLBACK perform_cb(TP_CALLBACK_INSTANCE *instance, void *user) DECLSPEC_HIDDEN;
> -BOOL is_wine_thread(void) DECLSPEC_HIDDEN;
>  void mark_wine_thread(void) DECLSPEC_HIDDEN;
> +void forward_cb_gstdemux(struct cb_data *data) DECLSPEC_HIDDEN;
>
> GstBusSyncReply watch_bus_wrapper(GstBus *bus, GstMessage *msg, gpointer user) DECLSPEC_HIDDEN;
>  void existing_new_pad_wrapper(GstElement *bin, GstPad *pad, gpointer user) DECLSPEC_HIDDEN;
> diff --git a/dlls/winegstreamer/gstdemux.c b/dlls/winegstreamer/gstdemux.c
> index 0b5fee3027..4d8f6705fb 100644
> --- a/dlls/winegstreamer/gstdemux.c
> +++ b/dlls/winegstreamer/gstdemux.c
> @@ -48,8 +48,6 @@ WINE_DEFAULT_DEBUG_CHANNEL(gstreamer);
>
> static const GUID MEDIASUBTYPE_CVID = {mmioFOURCC('c','v','i','d'), 0x0000, 0x0010, {0x80, 0x00, 0x00, 0xaa, 0x00, 0x38, 0x9b, 0x71}};
>
> -static pthread_key_t wine_gst_key;
> -
>
> struct gstdemux
>  {
>      struct strmbase_filter filter;
> @@ -109,17 +107,6 @@ static HRESULT WINAPI GST_ChangeCurrent(IMediaSeeking *iface);
>  static HRESULT WINAPI GST_ChangeStop(IMediaSeeking *iface);
>  static HRESULT WINAPI GST_ChangeRate(IMediaSeeking *iface);
>
> -void mark_wine_thread(void)
> -{
> - /* set it to non-NULL to indicate that this is a Wine thread */
> - pthread_setspecific(wine_gst_key, &wine_gst_key);
>
> -}
> -
>
> -BOOL is_wine_thread(void)
> -{
> - return pthread_getspecific(wine_gst_key) != NULL;
>
> -}
> -
>
> static gboolean amt_from_gst_caps_audio_raw(const GstCaps *caps, AM_MEDIA_TYPE *amt)
>  {
>      WAVEFORMATEXTENSIBLE *wfe;
> @@ -2248,14 +2235,8 @@ static HRESULT GST_RemoveOutputPins(struct gstdemux *This)
>      return S_OK;
>  }
>
> -pthread_mutex_t cb_list_lock = PTHREAD_MUTEX_INITIALIZER;
> -pthread_cond_t cb_list_cond = PTHREAD_COND_INITIALIZER;
> -struct list cb_list = LIST_INIT(cb_list);
> -
>
> -void CALLBACK perform_cb(TP_CALLBACK_INSTANCE *instance, void *user)
> +void forward_cb_gstdemux(struct cb_data *cbdata)
>  {
> - struct cb_data *cbdata = user;
> - switch(cbdata->type)
>    {
>    case WATCH_BUS:
>
> @@ -2266,8 +2247,8 @@ void CALLBACK perform_cb(TP_CALLBACK_INSTANCE *instance, void *user)
>          }
>      case EXISTING_NEW_PAD:
>          {
> -
>        struct existing_new_pad_data *data = &cbdata->u.existing_new_pad_data;
> -
>        existing_new_pad(data->bin, data->pad, data->user);
> -
>        struct pad_added_data *data = &cbdata->u.pad_added_data;
> -
>        existing_new_pad(data->element, data->pad, data->user);
>        break;
>    }
> case QUERY_FUNCTION:
>
> @@ -2285,13 +2266,13 @@ void CALLBACK perform_cb(TP_CALLBACK_INSTANCE *instance, void *user)
>      case NO_MORE_PADS:
>          {
>              struct no_more_pads_data *data = &cbdata->u.no_more_pads_data;
> -
>        no_more_pads(data->decodebin, data->user);
> -
>        no_more_pads(data->element, data->user);
>        break;
>    }
> case REQUEST_BUFFER_SRC:
>        {
> -
>        struct request_buffer_src_data *data = &cbdata->u.request_buffer_src_data;
> -
>        cbdata->u.request_buffer_src_data.ret = request_buffer_src(data->pad, data->parent,
> -
>        struct getrange_data *data = &cbdata->u.getrange_data;
> -
>        cbdata->u.getrange_data.ret = request_buffer_src(data->pad, data->parent,
>                data->ofs, data->len, data->buf);
>        break;
>    }
>
> @@ -2315,8 +2296,8 @@ void CALLBACK perform_cb(TP_CALLBACK_INSTANCE *instance, void *user)
>          }
>      case REMOVED_DECODED_PAD:
>          {
> -
>        struct removed_decoded_pad_data *data = &cbdata->u.removed_decoded_pad_data;
> -
>        removed_decoded_pad(data->bin, data->pad, data->user);
> -
>        struct pad_removed_data *data = &cbdata->u.pad_removed_data;
> -
>        removed_decoded_pad(data->element, data->pad, data->user);
>        break;
>    }
> case AUTOPLUG_BLACKLIST:
>
> @@ -2345,44 +2326,12 @@ void CALLBACK perform_cb(TP_CALLBACK_INSTANCE *instance, void *user)
>                      data->query);
>              break;
>          }
> - }
> -
> - pthread_mutex_lock(&cbdata->lock);
> - cbdata->finished = 1;
> - pthread_cond_broadcast(&cbdata->cond);
> - pthread_mutex_unlock(&cbdata->lock);
>
> -}
> -
>
> -static DWORD WINAPI dispatch_thread(void *user)
> -{
> - struct cb_data *cbdata;
> -
> - CoInitializeEx(NULL, COINIT_MULTITHREADED);
> -
> - pthread_mutex_lock(&cb_list_lock);
> -
> - while(1){
> -
>    pthread_cond_wait(&cb_list_cond, &cb_list_lock);
> -
> -
>    while(!list_empty(&cb_list)){
> -
>        cbdata = LIST_ENTRY(list_head(&cb_list), struct cb_data, entry);
> -
>        list_remove(&cbdata->entry);
> -
> -
>        TrySubmitThreadpoolCallback(&perform_cb, cbdata, NULL);
> - default:
> -
>    {
> -
>        ERR("Wrong callback forwarder called\n");
> -
>        return;

Perhaps "assert(0)" even.
>      }
>  }
> -
> - pthread_mutex_unlock(&cb_list_lock);
> -
> - CoUninitialize();
> -
> - return 0;
>
> -}
> -
>
> -void start_dispatch_thread(void)
> -{
> - pthread_key_create(&wine_gst_key, NULL);
> - CloseHandle(CreateThread(NULL, 0, &dispatch_thread, NULL, 0, NULL));
>
> }
>
> static BOOL compare_media_types(const AM_MEDIA_TYPE *a, const AM_MEDIA_TYPE *b)