[PATCH v3 0/3] MR3737: winegstreamer: Implement wg_muxer.
This patch set is part of !3303 -- v3: winegstreamer: Create wg_muxer for media sink. winegstreamer: Implement seeking query for wg_muxer sink pad. winegstreamer: Introduce new wg_muxer struct. https://gitlab.winehq.org/wine/wine/-/merge_requests/3737
From: Ziqing Hui <zhui(a)codeweavers.com> --- dlls/winegstreamer/Makefile.in | 1 + dlls/winegstreamer/gst_private.h | 3 + dlls/winegstreamer/main.c | 26 ++++++ dlls/winegstreamer/unix_private.h | 5 ++ dlls/winegstreamer/unixlib.c | 22 +++-- dlls/winegstreamer/unixlib.h | 10 +++ dlls/winegstreamer/wg_muxer.c | 139 ++++++++++++++++++++++++++++++ dlls/winegstreamer/wg_parser.c | 25 +++++- 8 files changed, 222 insertions(+), 9 deletions(-) create mode 100644 dlls/winegstreamer/wg_muxer.c diff --git a/dlls/winegstreamer/Makefile.in b/dlls/winegstreamer/Makefile.in index 78bdd6c0ef1..f963dcea7f0 100644 --- a/dlls/winegstreamer/Makefile.in +++ b/dlls/winegstreamer/Makefile.in @@ -22,6 +22,7 @@ C_SRCS = \ video_processor.c \ wg_allocator.c \ wg_format.c \ + wg_muxer.c \ wg_parser.c \ wg_sample.c \ wg_transform.c \ diff --git a/dlls/winegstreamer/gst_private.h b/dlls/winegstreamer/gst_private.h index ed867f741d9..4c397b5f385 100644 --- a/dlls/winegstreamer/gst_private.h +++ b/dlls/winegstreamer/gst_private.h @@ -110,6 +110,9 @@ bool wg_transform_get_status(wg_transform_t transform, bool *accepts_input); HRESULT wg_transform_drain(wg_transform_t transform); HRESULT wg_transform_flush(wg_transform_t transform); +HRESULT wg_muxer_create(const char *format, wg_muxer_t *muxer); +void wg_muxer_destroy(wg_muxer_t muxer); + unsigned int wg_format_get_max_size(const struct wg_format *format); HRESULT avi_splitter_create(IUnknown *outer, IUnknown **out); diff --git a/dlls/winegstreamer/main.c b/dlls/winegstreamer/main.c index c78d73537f7..f45d061200a 100644 --- a/dlls/winegstreamer/main.c +++ b/dlls/winegstreamer/main.c @@ -456,6 +456,32 @@ HRESULT wg_transform_flush(wg_transform_t transform) return S_OK; } +HRESULT wg_muxer_create(const char *format, wg_muxer_t *muxer) +{ + struct wg_muxer_create_params params = + { + .format = format, + }; + NTSTATUS status; + + TRACE("format %p, muxer %p.\n", format, muxer); + + if (SUCCEEDED(status = WINE_UNIX_CALL(unix_wg_muxer_create, ¶ms))) + { + *muxer = params.muxer; + TRACE("Created wg_muxer %#I64x.\n", params.muxer); + } + + return status; +} + +void wg_muxer_destroy(wg_muxer_t muxer) +{ + TRACE("muxer %#I64x.\n", muxer); + + WINE_UNIX_CALL(unix_wg_muxer_destroy, &muxer); +} + #define ALIGN(n, alignment) (((n) + (alignment) - 1) & ~((alignment) - 1)) unsigned int wg_format_get_stride(const struct wg_format *format) diff --git a/dlls/winegstreamer/unix_private.h b/dlls/winegstreamer/unix_private.h index 8bef7b2b2bd..305d69c12a8 100644 --- a/dlls/winegstreamer/unix_private.h +++ b/dlls/winegstreamer/unix_private.h @@ -58,6 +58,11 @@ extern NTSTATUS wg_transform_get_status(void *args) DECLSPEC_HIDDEN; extern NTSTATUS wg_transform_drain(void *args) DECLSPEC_HIDDEN; extern NTSTATUS wg_transform_flush(void *args) DECLSPEC_HIDDEN; +/* wg_muxer.c */ + +extern NTSTATUS wg_muxer_create(void *args) DECLSPEC_HIDDEN; +extern NTSTATUS wg_muxer_destroy(void *args) DECLSPEC_HIDDEN; + /* wg_allocator.c */ static inline BYTE *wg_sample_data(struct wg_sample *sample) diff --git a/dlls/winegstreamer/unixlib.c b/dlls/winegstreamer/unixlib.c index 7e5ef34c4d7..513ece95a90 100644 --- a/dlls/winegstreamer/unixlib.c +++ b/dlls/winegstreamer/unixlib.c @@ -87,15 +87,21 @@ GstElement *find_element(GstElementFactoryListType type, GstCaps *src_caps, GstC if (!(transforms = gst_element_factory_list_get_elements(type, GST_RANK_MARGINAL))) goto done; - tmp = gst_element_factory_list_filter(transforms, src_caps, GST_PAD_SINK, FALSE); - gst_plugin_feature_list_free(transforms); - if (!(transforms = tmp)) - goto done; + if (src_caps) + { + tmp = gst_element_factory_list_filter(transforms, src_caps, GST_PAD_SINK, FALSE); + gst_plugin_feature_list_free(transforms); + if (!(transforms = tmp)) + goto done; + } - tmp = gst_element_factory_list_filter(transforms, sink_caps, GST_PAD_SRC, FALSE); - gst_plugin_feature_list_free(transforms); - if (!(transforms = tmp)) - goto done; + if (sink_caps) + { + tmp = gst_element_factory_list_filter(transforms, sink_caps, GST_PAD_SRC, FALSE); + gst_plugin_feature_list_free(transforms); + if (!(transforms = tmp)) + goto done; + } transforms = g_list_sort(transforms, gst_plugin_feature_rank_compare_func); for (tmp = transforms; tmp != NULL && element == NULL; tmp = tmp->next) diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index 15e0605fdde..a5015ef9b22 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -209,6 +209,7 @@ enum wg_parser_type typedef UINT64 wg_parser_t; typedef UINT64 wg_parser_stream_t; typedef UINT64 wg_transform_t; +typedef UINT64 wg_muxer_t; struct wg_parser_create_params { @@ -365,6 +366,12 @@ struct wg_transform_get_status_params UINT32 accepts_input; }; +struct wg_muxer_create_params +{ + wg_muxer_t muxer; + const char *format; +}; + enum unix_funcs { unix_wg_init_gstreamer, @@ -404,6 +411,9 @@ enum unix_funcs unix_wg_transform_get_status, unix_wg_transform_drain, unix_wg_transform_flush, + + unix_wg_muxer_create, + unix_wg_muxer_destroy, }; #endif /* __WINE_WINEGSTREAMER_UNIXLIB_H */ diff --git a/dlls/winegstreamer/wg_muxer.c b/dlls/winegstreamer/wg_muxer.c new file mode 100644 index 00000000000..56abdc821e3 --- /dev/null +++ b/dlls/winegstreamer/wg_muxer.c @@ -0,0 +1,139 @@ +/* + * GStreamer muxer backend + * + * Copyright 2023 Ziqing Hui for CodeWeavers + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#if 0 +#pragma makedep unix +#endif + +#include "ntstatus.h" +#define WIN32_NO_STATUS +#include "winternl.h" + +#include "unix_private.h" + +struct wg_muxer +{ + GstElement *container, *muxer; + + GstPad *my_sink; + GstAtomicQueue *output_queue; + + pthread_mutex_t mutex; + pthread_cond_t cond; +}; + +static struct wg_muxer *get_muxer(wg_muxer_t muxer) +{ + return (struct wg_muxer *)(ULONG_PTR)muxer; +} + +NTSTATUS wg_muxer_create(void *args) +{ + struct wg_muxer_create_params *params = args; + GstElement *first = NULL, *last = NULL; + GstPadTemplate *template = NULL; + GstCaps *sink_caps = NULL; + NTSTATUS status = E_FAIL; + struct wg_muxer *muxer; + + /* Create wg_muxer object. */ + if (!(muxer = calloc(1, sizeof(*muxer)))) + return E_OUTOFMEMORY; + pthread_mutex_init(&muxer->mutex, NULL); + pthread_cond_init(&muxer->cond, NULL); + if (!(muxer->container = gst_bin_new("wg_muxer"))) + goto out; + if (!(muxer->output_queue = gst_atomic_queue_new(8))) + goto out; + + /* Create sink pad. */ + if (!(sink_caps = gst_caps_from_string(params->format))) + { + GST_ERROR("Failed to get caps from format string: \"%s\".", params->format); + goto out; + } + if (!(template = gst_pad_template_new("sink", GST_PAD_SINK, GST_PAD_ALWAYS, sink_caps))) + goto out; + muxer->my_sink = gst_pad_new_from_template(template, "wg_muxer_sink"); + if (!muxer->my_sink) + goto out; + gst_pad_set_element_private(muxer->my_sink, muxer); + + /* Create gstreamer muxer element. */ + if (!(muxer->muxer = find_element(GST_ELEMENT_FACTORY_TYPE_MUXER | GST_ELEMENT_FACTORY_TYPE_FORMATTER, + NULL, sink_caps))) + goto out; + if (!append_element(muxer->container, muxer->muxer, &first, &last)) + goto out; + + /* Link muxer to sink pad. */ + if (!link_element_to_sink(muxer->muxer, muxer->my_sink)) + goto out; + if (!gst_pad_set_active(muxer->my_sink, 1)) + goto out; + + /* Set to pause state. */ + gst_element_set_state(muxer->container, GST_STATE_PAUSED); + if (!gst_element_get_state(muxer->container, NULL, NULL, -1)) + goto out; + + gst_object_unref(template); + gst_caps_unref(sink_caps); + + GST_INFO("Created winegstreamer muxer %p.", muxer); + params->muxer = (wg_transform_t)(ULONG_PTR)muxer; + + return S_OK; + +out: + if (muxer->my_sink) + gst_object_unref(muxer->my_sink); + if (template) + gst_object_unref(template); + if (sink_caps) + gst_caps_unref(sink_caps); + if (muxer->output_queue) + gst_atomic_queue_unref(muxer->output_queue); + if (muxer->container) + { + gst_element_set_state(muxer->container, GST_STATE_NULL); + gst_object_unref(muxer->container); + } + pthread_cond_destroy(&muxer->cond); + pthread_mutex_destroy(&muxer->mutex); + free(muxer); + + return status; +} + +NTSTATUS wg_muxer_destroy(void *args) +{ + struct wg_muxer *muxer = get_muxer(*(wg_muxer_t *)args); + + pthread_cond_destroy(&muxer->cond); + pthread_mutex_destroy(&muxer->mutex); + gst_atomic_queue_unref(muxer->output_queue); + gst_object_unref(muxer->my_sink); + gst_element_set_state(muxer->container, GST_STATE_NULL); + gst_object_unref(muxer->container); + free(muxer); + + return S_OK; +} diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c index 5556b52829c..016691d448d 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -1945,6 +1945,9 @@ const unixlib_entry_t __wine_unix_call_funcs[] = X(wg_transform_get_status), X(wg_transform_drain), X(wg_transform_flush), + + X(wg_muxer_create), + X(wg_muxer_destroy), }; #ifdef _WIN64 @@ -2052,7 +2055,6 @@ static NTSTATUS wow64_wg_parser_stream_copy_buffer(void *args) return wg_parser_stream_copy_buffer(¶ms); } - static NTSTATUS wow64_wg_parser_stream_get_tag(void *args) { struct @@ -2152,6 +2154,24 @@ NTSTATUS wow64_wg_transform_read_data(void *args) return ret; } +NTSTATUS wow64_wg_muxer_create(void *args) +{ + struct + { + wg_muxer_t muxer; + PTR32 format; + } *params32 = args; + struct wg_muxer_create_params params = + { + .format = ULongToPtr(params32->format), + }; + NTSTATUS ret; + + ret = wg_muxer_create(¶ms); + params32->muxer = params.muxer; + return ret; +} + const unixlib_entry_t __wine_unix_call_wow64_funcs[] = { #define X64(name) [unix_ ## name] = wow64_ ## name @@ -2192,6 +2212,9 @@ const unixlib_entry_t __wine_unix_call_wow64_funcs[] = X(wg_transform_get_status), X(wg_transform_drain), X(wg_transform_flush), + + X64(wg_muxer_create), + X(wg_muxer_destroy), }; #endif /* _WIN64 */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3737
From: Ziqing Hui <zhui(a)codeweavers.com> This makes muxer element know that it can perform seeking. --- dlls/winegstreamer/wg_muxer.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/dlls/winegstreamer/wg_muxer.c b/dlls/winegstreamer/wg_muxer.c index 56abdc821e3..5f819fbafa9 100644 --- a/dlls/winegstreamer/wg_muxer.c +++ b/dlls/winegstreamer/wg_muxer.c @@ -44,6 +44,24 @@ static struct wg_muxer *get_muxer(wg_muxer_t muxer) return (struct wg_muxer *)(ULONG_PTR)muxer; } +static gboolean muxer_sink_query_cb(GstPad *pad, GstObject *parent, GstQuery *query) +{ + struct wg_muxer *muxer = gst_pad_get_element_private(pad); + + GST_DEBUG("pad %p, parent %p, query %p, muxer %p, type \"%s\".", + pad, parent, query, muxer, gst_query_type_get_name(query->type)); + + switch (query->type) + { + case GST_QUERY_SEEKING: + gst_query_set_seeking(query, GST_FORMAT_BYTES, true, 0, -1); + return true; + default: + GST_WARNING("Ignoring \"%s\" query.", gst_query_type_get_name(query->type)); + return gst_pad_query_default(pad, parent, query); + } +} + NTSTATUS wg_muxer_create(void *args) { struct wg_muxer_create_params *params = args; @@ -75,6 +93,7 @@ NTSTATUS wg_muxer_create(void *args) if (!muxer->my_sink) goto out; gst_pad_set_element_private(muxer->my_sink, muxer); + gst_pad_set_query_function(muxer->my_sink, muxer_sink_query_cb); /* Create gstreamer muxer element. */ if (!(muxer->muxer = find_element(GST_ELEMENT_FACTORY_TYPE_MUXER | GST_ELEMENT_FACTORY_TYPE_FORMATTER, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3737
From: Ziqing Hui <zhui(a)codeweavers.com> --- dlls/winegstreamer/gst_private.h | 3 +- dlls/winegstreamer/main.c | 7 ++-- dlls/winegstreamer/media_sink.c | 65 +++++++++++++++++++++++++------- 3 files changed, 58 insertions(+), 17 deletions(-) diff --git a/dlls/winegstreamer/gst_private.h b/dlls/winegstreamer/gst_private.h index 4c397b5f385..8cfadd10bfc 100644 --- a/dlls/winegstreamer/gst_private.h +++ b/dlls/winegstreamer/gst_private.h @@ -125,7 +125,8 @@ HRESULT wma_decoder_create(IUnknown *outer, IUnknown **out); HRESULT wmv_decoder_create(IUnknown *outer, IUnknown **out); HRESULT resampler_create(IUnknown *outer, IUnknown **out); HRESULT color_convert_create(IUnknown *outer, IUnknown **out); -HRESULT sink_class_factory_create(IUnknown *outer, IUnknown **out); +HRESULT mp3_sink_class_factory_create(IUnknown *outer, IUnknown **out); +HRESULT mpeg4_sink_class_factory_create(IUnknown *outer, IUnknown **out); bool amt_from_wg_format(AM_MEDIA_TYPE *mt, const struct wg_format *format, bool wm); bool amt_to_wg_format(const AM_MEDIA_TYPE *mt, struct wg_format *format); diff --git a/dlls/winegstreamer/main.c b/dlls/winegstreamer/main.c index f45d061200a..853907e1825 100644 --- a/dlls/winegstreamer/main.c +++ b/dlls/winegstreamer/main.c @@ -638,7 +638,8 @@ static struct class_factory wma_decoder_cf = {{&class_factory_vtbl}, wma_decoder static struct class_factory wmv_decoder_cf = {{&class_factory_vtbl}, wmv_decoder_create}; static struct class_factory resampler_cf = {{&class_factory_vtbl}, resampler_create}; static struct class_factory color_convert_cf = {{&class_factory_vtbl}, color_convert_create}; -static struct class_factory sink_class_factory_cf = {{&class_factory_vtbl}, sink_class_factory_create}; +static struct class_factory mp3_sink_class_factory_cf = {{&class_factory_vtbl}, mp3_sink_class_factory_create}; +static struct class_factory mpeg4_sink_class_factory_cf = {{&class_factory_vtbl}, mpeg4_sink_class_factory_create}; HRESULT WINAPI DllGetClassObject(REFCLSID clsid, REFIID iid, void **out) { @@ -674,9 +675,9 @@ HRESULT WINAPI DllGetClassObject(REFCLSID clsid, REFIID iid, void **out) else if (IsEqualGUID(clsid, &CLSID_CColorConvertDMO)) factory = &color_convert_cf; else if (IsEqualGUID(clsid, &CLSID_MFMP3SinkClassFactory)) - factory = &sink_class_factory_cf; + factory = &mp3_sink_class_factory_cf; else if (IsEqualGUID(clsid, &CLSID_MFMPEG4SinkClassFactory)) - factory = &sink_class_factory_cf; + factory = &mpeg4_sink_class_factory_cf; else { FIXME("%s not implemented, returning CLASS_E_CLASSNOTAVAILABLE.\n", debugstr_guid(clsid)); diff --git a/dlls/winegstreamer/media_sink.c b/dlls/winegstreamer/media_sink.c index fcea67eb8f1..6bd9fdcfb7d 100644 --- a/dlls/winegstreamer/media_sink.c +++ b/dlls/winegstreamer/media_sink.c @@ -77,6 +77,8 @@ struct media_sink IMFMediaEventQueue *event_queue; struct list stream_sinks; + + wg_muxer_t muxer; }; static struct stream_sink *impl_from_IMFStreamSink(IMFStreamSink *iface) @@ -567,6 +569,7 @@ static ULONG WINAPI media_sink_Release(IMFFinalizableMediaSink *iface) IMFByteStream_Release(media_sink->bytestream); media_sink->cs.DebugInfo->Spare[0] = 0; DeleteCriticalSection(&media_sink->cs); + wg_muxer_destroy(media_sink->muxer); free(media_sink); } @@ -1022,7 +1025,7 @@ static const IMFAsyncCallbackVtbl media_sink_callback_vtbl = media_sink_callback_Invoke, }; -static HRESULT media_sink_create(IMFByteStream *bytestream, struct media_sink **out) +static HRESULT media_sink_create(IMFByteStream *bytestream, const char *format, struct media_sink **out) { struct media_sink *media_sink; HRESULT hr; @@ -1035,11 +1038,10 @@ static HRESULT media_sink_create(IMFByteStream *bytestream, struct media_sink ** if (!(media_sink = calloc(1, sizeof(*media_sink)))) return E_OUTOFMEMORY; + if (FAILED(hr = wg_muxer_create(format, &media_sink->muxer))) + goto fail; if (FAILED(hr = MFCreateEventQueue(&media_sink->event_queue))) - { - free(media_sink); - return hr; - } + goto fail; media_sink->IMFFinalizableMediaSink_iface.lpVtbl = &media_sink_vtbl; media_sink->IMFMediaEventGenerator_iface.lpVtbl = &media_sink_event_vtbl; @@ -1056,6 +1058,12 @@ static HRESULT media_sink_create(IMFByteStream *bytestream, struct media_sink ** TRACE("Created media sink %p.\n", media_sink); return S_OK; + +fail: + if (media_sink->muxer) + wg_muxer_destroy(media_sink->muxer); + free(media_sink); + return hr; } static HRESULT WINAPI sink_class_factory_QueryInterface(IMFSinkClassFactory *iface, REFIID riid, void **out) @@ -1082,8 +1090,8 @@ static ULONG WINAPI sink_class_factory_Release(IMFSinkClassFactory *iface) return 1; } -static HRESULT WINAPI sink_class_factory_CreateMediaSink(IMFSinkClassFactory *iface, IMFByteStream *bytestream, - IMFMediaType *video_type, IMFMediaType *audio_type, IMFMediaSink **out) +static HRESULT WINAPI sink_class_factory_create_media_sink(IMFSinkClassFactory *iface, IMFByteStream *bytestream, + const char *format, IMFMediaType *video_type, IMFMediaType *audio_type, IMFMediaSink **out) { IMFFinalizableMediaSink *media_sink_iface; struct media_sink *media_sink; @@ -1092,7 +1100,7 @@ static HRESULT WINAPI sink_class_factory_CreateMediaSink(IMFSinkClassFactory *if TRACE("iface %p, bytestream %p, video_type %p, audio_type %p, out %p.\n", iface, bytestream, video_type, audio_type, out); - if (FAILED(hr = media_sink_create(bytestream, &media_sink))) + if (FAILED(hr = media_sink_create(bytestream, format, &media_sink))) return hr; media_sink_iface = &media_sink->IMFFinalizableMediaSink_iface; @@ -1119,18 +1127,49 @@ static HRESULT WINAPI sink_class_factory_CreateMediaSink(IMFSinkClassFactory *if return S_OK; } -static const IMFSinkClassFactoryVtbl sink_class_factory_vtbl = +static HRESULT WINAPI mp3_sink_class_factory_CreateMediaSink(IMFSinkClassFactory *iface, IMFByteStream *bytestream, + IMFMediaType *video_type, IMFMediaType *audio_type, IMFMediaSink **out) +{ + const char *format = "application/x-id3"; + + return sink_class_factory_create_media_sink(iface, bytestream, format, video_type, audio_type, out); +} + +static HRESULT WINAPI mpeg4_sink_class_factory_CreateMediaSink(IMFSinkClassFactory *iface, IMFByteStream *bytestream, + IMFMediaType *video_type, IMFMediaType *audio_type, IMFMediaSink **out) +{ + const char *format = "video/quicktime, variant=iso"; + + return sink_class_factory_create_media_sink(iface, bytestream, format, video_type, audio_type, out); +} + +static const IMFSinkClassFactoryVtbl mp3_sink_class_factory_vtbl = +{ + sink_class_factory_QueryInterface, + sink_class_factory_AddRef, + sink_class_factory_Release, + mp3_sink_class_factory_CreateMediaSink, +}; + +static const IMFSinkClassFactoryVtbl mpeg4_sink_class_factory_vtbl = { sink_class_factory_QueryInterface, sink_class_factory_AddRef, sink_class_factory_Release, - sink_class_factory_CreateMediaSink, + mpeg4_sink_class_factory_CreateMediaSink, }; -static IMFSinkClassFactory sink_class_factory = { &sink_class_factory_vtbl }; +static IMFSinkClassFactory mp3_sink_class_factory = { &mp3_sink_class_factory_vtbl }; +static IMFSinkClassFactory mpeg4_sink_class_factory = { &mpeg4_sink_class_factory_vtbl }; + +HRESULT mp3_sink_class_factory_create(IUnknown *outer, IUnknown **out) +{ + *out = (IUnknown *)&mp3_sink_class_factory; + return S_OK; +} -HRESULT sink_class_factory_create(IUnknown *outer, IUnknown **out) +HRESULT mpeg4_sink_class_factory_create(IUnknown *outer, IUnknown **out) { - *out = (IUnknown *)&sink_class_factory; + *out = (IUnknown *)&mpeg4_sink_class_factory; return S_OK; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3737
On Fri Sep 8 05:59:16 2023 +0000, Rémi Bernon wrote:
What about simply one wg_muxer_get_data where you would just read some requested number of bytes, or less, from the output buffer, popping additional buffers from the output queue if needed and if available? I don't think there's any reason to express individual buffers to the PE side, as it's all written to a stream anyway. Sounds good to me.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3737#note_44680
Rémi Bernon (@rbernon) commented about dlls/winegstreamer/wg_muxer.c:
+ +#include "ntstatus.h" +#define WIN32_NO_STATUS +#include "winternl.h" + +#include "unix_private.h" + +struct wg_muxer +{ + GstElement *container, *muxer; + + GstPad *my_sink; + GstAtomicQueue *output_queue; + + pthread_mutex_t mutex; + pthread_cond_t cond; Sorry for noticing this now but I think you should really avoid the condition variable (and perhaps not even need the mutex).
Condition variables are broken and cause random crashes on process termination (in my opinion we should completely get rid of them in winegstreamer). Things will be simpler if the muxer works in a synchronous way, producing buffers only when samples are pushed. Also, maybe you can delay adding the output queue until you actually use it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3737#note_44683
On Fri Sep 8 10:27:13 2023 +0000, Rémi Bernon wrote:
Sorry for noticing this now but I think you should really avoid the condition variable (and perhaps not even need the mutex). The pthread condition variables are broken and cause random crashes on process termination (in my opinion we should completely get rid of them in winegstreamer). Things will be simpler if the muxer works in a synchronous way, producing buffers only when samples are pushed. Also, maybe you can delay adding the output queue until you actually use it. The problem is that: the gstreamer muxer element itself is using GstTask(https://gstreamer.freedesktop.org/documentation/gstreamer/gsttask.html) at its src pad. GstTask will launch a new thread. In order to synchronize the GstTask thread and our executing main thread, pthread is needed.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3737#note_44684
On Fri Sep 8 10:38:38 2023 +0000, Ziqing Hui wrote:
The problem is that: the gstreamer muxer element itself is using GstTask(https://gstreamer.freedesktop.org/documentation/gstreamer/gsttask.html) at its src pad. GstTask will launch a new thread. In order to synchronize the GstTask thread and our executing main thread, pthread is needed. 
I've dumped a pipeline image of wg_muxer, we can see that there is a "[T]" mark at the src pad of the muxer element. From the left-bottom coner of the image, we know that "[T]" stands for a started GstTask. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3737#note_44685
On Fri Sep 8 10:42:19 2023 +0000, Ziqing Hui wrote:
 I've dumped a pipeline image of wg_muxer, we can see that there is a "[T]" mark at the src pad of the muxer element. From the left-bottom coner of the image, we know that "[T]" stands for a started GstTask. The condition variable is used here: https://gitlab.winehq.org/wine/wine/-/merge_requests/3303/diffs?commit_id=5f...
After we done muxing, we call finalize() in our PE side, then PE side call the corresponding finalize() function in wg_muxer backend. Our backend finalize() look like this: ``` sink_event_cb() /* sink cb of muxer my_sink. */ { [...] switch (event->type) { case GST_EVENT_EOS: pthread_mutex_lock(&muxer->mutex); muxer->eos = true; pthread_mutex_unlock(&muxer->mutex); pthread_cond_signal(&muxer->cond); break; [...] } [...] } NTSTATUS wg_muxer_finalize(void *args) { struct wg_muxer *muxer = get_muxer(*(wg_muxer_t *)args); struct wg_muxer_stream *stream; /* Notify each stream of EOS. */ LIST_FOR_EACH_ENTRY(stream, &muxer->streams, struct wg_muxer_stream, entry) { if (!push_event(stream->my_src, gst_event_new_segment_done(GST_FORMAT_BYTES, -1)) || !push_event(stream->my_src, gst_event_new_eos())) return E_FAIL; } /* Wait for muxer EOS. */ pthread_mutex_lock(&muxer->mutex); while (!muxer->eos) pthread_cond_wait(&muxer->cond, &muxer->mutex); pthread_mutex_unlock(&muxer->mutex); return S_OK; } ``` The sink_event_cb() will be called in GstTask thread of mp4muxer element, which is not the same thread with wg_muxer_finalize(). If we get rid of condition variable, how do we wait for the EOS, use a simple while() loop? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3737#note_44687
On Fri Sep 8 10:55:31 2023 +0000, Ziqing Hui wrote:
The condition variable is used here: https://gitlab.winehq.org/wine/wine/-/merge_requests/3303/diffs?commit_id=5f... After we done muxing, we call finalize() in our PE side, then PE side call the corresponding finalize() function in wg_muxer backend. Our backend finalize() look like this: ``` sink_event_cb() /* sink cb of muxer my_sink. */ { [...] switch (event->type) { case GST_EVENT_EOS: pthread_mutex_lock(&muxer->mutex); muxer->eos = true; pthread_mutex_unlock(&muxer->mutex); pthread_cond_signal(&muxer->cond); break; [...] } [...] } NTSTATUS wg_muxer_finalize(void *args) { struct wg_muxer *muxer = get_muxer(*(wg_muxer_t *)args); struct wg_muxer_stream *stream; /* Notify each stream of EOS. */ LIST_FOR_EACH_ENTRY(stream, &muxer->streams, struct wg_muxer_stream, entry) { if (!push_event(stream->my_src, gst_event_new_segment_done(GST_FORMAT_BYTES, -1)) || !push_event(stream->my_src, gst_event_new_eos())) return E_FAIL; } /* Wait for muxer EOS. */ pthread_mutex_lock(&muxer->mutex); while (!muxer->eos) pthread_cond_wait(&muxer->cond, &muxer->mutex); pthread_mutex_unlock(&muxer->mutex); return S_OK; } ``` The sink_event_cb() will be called in GstTask thread of mp4muxer element, which is not the same thread with wg_muxer_finalize(). If we get rid of condition variable, how do we wait for the EOS, use a simple while() loop? I see, this is very unfortunate IMO... Looks like it's coming from the GstAggregator class.
I still think we should avoid the condition variable, but I don't really have any good suggestion to workaround that. A busy loop might be better, assuming it will quickly EOS, but it's a bit ugly. Alternatively I had thought about implementing a GTask pool with Win32 threads, which would also solve some problems we have with logging, but I don't know if it's a good idea. It would let us use Win32 sync primitives for notifications instead though, so less ugly but a lot more code so I'm not sure it's worth it. Then, as we already have condition variables elsewhere, I can probably live with it in this case, if you think it's better. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3737#note_44690
On Fri Sep 8 08:42:42 2023 +0000, Ziqing Hui wrote:
Sounds good to me. Yeah, I think it'd be better to avoid zero-copy until we have some evidence it's going to make a difference.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3737#note_44724
On Fri Sep 8 11:51:16 2023 +0000, Rémi Bernon wrote:
I see, this is very unfortunate IMO... Looks like it's coming from the GstAggregator class. I still think we should avoid the condition variable, but I don't really have any good suggestion to workaround that. A busy loop might be better, assuming it will quickly EOS, but it's a bit ugly. Alternatively I had thought about implementing a GTask pool with Win32 threads, which would also solve some problems we have with logging, but I don't know if it's a good idea. It would let us use Win32 sync primitives for notifications instead though, so less ugly but a lot more code so I'm not sure it's worth it. Then, as we already have condition variables elsewhere, I can probably live with it in this case, if you think it's better. Hi Rémi, I've asked you this before in 3606, but in what way are condition variables broken? Can you please explain?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3737#note_44725
On Fri Sep 8 16:26:44 2023 +0000, Zebediah Figura wrote:
Hi Rémi, I've asked you this before in 3606, but in what way are condition variables broken? Can you please explain? See the details in https://bugs.winehq.org/show_bug.cgi?id=52213 and https://gitlab.winehq.org/wine/wine/-/merge_requests/1088, also https://gitlab.winehq.org/wine/wine/-/merge_requests/1503#note_17119
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3737#note_44728
See the details in https://bugs.winehq.org/show_bug.cgi?id=52213 and https://gitlab.winehq.org/wine/wine/-/merge_requests/1088, also https://gitlab.winehq.org/wine/wine/-/merge_requests/1503#note_17119
As far as I understand, what's happening in bug 52213 is that threads that are killed while in glibc die noisily, by triggering a Wine segmentation fault. But this doesn't inhere to condition variables, it only happens after threads are killed for one reason or another, and it doesn't actually have any adverse effects besides printing some extra messages to the console (I don't even think it triggers a winedbg window?) In that case it doesn't seem to me that condition variables are actually broken, or really that anything is. The extra messages do seem undesirable and misleading, but they're a consequence of killing threads, which I think should be avoidable in most cases anyway. Or is there some other aspect of the problem that I'm misunderstanding? Do condition variables actually misbehave in some meaningful way? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3737#note_44734
On Fri Sep 8 17:30:15 2023 +0000, Zebediah Figura wrote:
See the details in https://bugs.winehq.org/show_bug.cgi?id=52213 and https://gitlab.winehq.org/wine/wine/-/merge_requests/1088, also https://gitlab.winehq.org/wine/wine/-/merge_requests/1503#note_17119 As far as I understand, what's happening in bug 52213 is that threads that are killed while in glibc die noisily, by triggering a Wine segmentation fault. But this doesn't inhere to condition variables, it only happens after threads are killed for one reason or another, and it doesn't actually have any adverse effects besides printing some extra messages to the console (I don't even think it triggers a winedbg window?) In that case it doesn't seem to me that condition variables are actually broken, or really that anything is. The extra messages do seem undesirable and misleading, but they're a consequence of killing threads, which I think should be avoidable in most cases anyway. Or is there some other aspect of the problem that I'm misunderstanding? Do condition variables actually misbehave in some meaningful way? It happens every time a process terminates while one of its thread is waiting on a condition variable.
The result is completely unpredictable and it may "just" crash indeed, but I've also seen it trigger silent infinite loops if the invalid stack frames are bad enough, or the segv_handler can handle the crash and restore the thread to another corrupted frame, again and again. This can and does end up with stuck processes. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3737#note_44738
The result is completely unpredictable and it may "just" crash indeed, but I've also seen it trigger silent infinite loops if the invalid stack frames are bad enough, or the segv_handler can handle the crash and restore the thread to another corrupted frame, again and again. This can and does end up with stuck processes.
Wait, why aren't we sending processes SIGKILL in that case? If we need to kill the process we should be sending SIGKILL anyway; there's no guarantee it'll respond to SIGQUIT in the first place. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3737#note_44740
participants (4)
-
Rémi Bernon -
Zebediah Figura (＠zfigura)
-
Ziqing Hui
-
Ziqing Hui (＠zhui)