[PATCH 1/5] winegstreamer: Activate source pad in push mode if it isn't activated in pull mode.
Since our source pad is not part of any element, gstreamer won't end up activating it directly through the state transition. Instead, if the downstream element doesn't activate the source pad into pull mode during the transition to the READY state, we activate our pad in push mode. Signed-off-by: Derek Lesho <dlesho(a)codeweavers.com> --- dlls/winegstreamer/wg_parser.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c index b8dd75d5fff..67f65264a8d 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -58,7 +58,7 @@ struct wg_parser pthread_mutex_t mutex; pthread_cond_t init_cond; - bool no_more_pads, has_duration, error; + bool no_more_pads, has_duration, error, pull_mode; pthread_cond_t read_cond, read_done_cond; struct @@ -1331,9 +1331,12 @@ static gboolean src_activate_mode_cb(GstPad *pad, GstObject *parent, GstPadMode GST_DEBUG("%s source pad for parser %p in %s mode.", activate ? "Activating" : "Deactivating", parser, gst_pad_mode_get_name(mode)); + parser->pull_mode = false; + switch (mode) { case GST_PAD_MODE_PULL: + parser->pull_mode = activate; return TRUE; case GST_PAD_MODE_PUSH: return activate_push(pad, activate); @@ -1573,6 +1576,8 @@ static void CDECL wg_parser_disconnect(struct wg_parser *parser) pthread_mutex_unlock(&parser->mutex); gst_element_set_state(parser->container, GST_STATE_NULL); + if (!parser->pull_mode) + gst_pad_set_active(parser->my_src, 0); gst_pad_unlink(parser->my_src, parser->their_sink); gst_object_unref(parser->my_src); gst_object_unref(parser->their_sink); @@ -1628,6 +1633,8 @@ static BOOL decodebin_parser_init_gst(struct wg_parser *parser) } gst_element_set_state(parser->container, GST_STATE_PAUSED); + if (!parser->pull_mode) + gst_pad_set_active(parser->my_src, 1); ret = gst_element_get_state(parser->container, NULL, NULL, -1); if (ret == GST_STATE_CHANGE_FAILURE) { @@ -1679,6 +1686,8 @@ static BOOL avi_parser_init_gst(struct wg_parser *parser) } gst_element_set_state(parser->container, GST_STATE_PAUSED); + if (!parser->pull_mode) + gst_pad_set_active(parser->my_src, 1); ret = gst_element_get_state(parser->container, NULL, NULL, -1); if (ret == GST_STATE_CHANGE_FAILURE) { @@ -1733,6 +1742,8 @@ static BOOL mpeg_audio_parser_init_gst(struct wg_parser *parser) gst_pad_set_active(stream->my_sink, 1); gst_element_set_state(parser->container, GST_STATE_PAUSED); + if (!parser->pull_mode) + gst_pad_set_active(parser->my_src, 1); ret = gst_element_get_state(parser->container, NULL, NULL, -1); if (ret == GST_STATE_CHANGE_FAILURE) { @@ -1787,6 +1798,8 @@ static BOOL wave_parser_init_gst(struct wg_parser *parser) gst_pad_set_active(stream->my_sink, 1); gst_element_set_state(parser->container, GST_STATE_PAUSED); + if (!parser->pull_mode) + gst_pad_set_active(parser->my_src, 1); ret = gst_element_get_state(parser->container, NULL, NULL, -1); if (ret == GST_STATE_CHANGE_FAILURE) { -- 2.30.1
Signed-off-by: Derek Lesho <dlesho(a)codeweavers.com> --- dlls/winegstreamer/wg_parser.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c index 67f65264a8d..0dc0d30f64c 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -1252,6 +1252,7 @@ static void *push_data(void *arg) { struct wg_parser *parser = arg; GstBuffer *buffer; + GstSegment *segment; guint max_size; GST_DEBUG("Starting push thread."); @@ -1264,6 +1265,12 @@ static void *push_data(void *arg) max_size = parser->stop_offset ? parser->stop_offset : parser->file_size; + gst_pad_push_event(parser->my_src, gst_event_new_stream_start("wg_stream")); + + segment = gst_segment_new(); + gst_segment_init(segment, GST_FORMAT_BYTES); + gst_pad_push_event(parser->my_src, gst_event_new_segment(segment)); + for (;;) { ULONG size; @@ -1398,6 +1405,7 @@ static gboolean src_perform_seek(struct wg_parser *parser, GstEvent *event) GstEvent *flush_event; GstSeekFlags flags; gint64 cur, stop; + GstSegment *seg; guint32 seqnum; gdouble rate; @@ -1431,7 +1439,12 @@ static gboolean src_perform_seek(struct wg_parser *parser, GstEvent *event) gst_event_set_seqnum(flush_event, seqnum); gst_pad_push_event(parser->my_src, flush_event); if (thread) + { gst_pad_set_active(parser->my_src, 1); + seg = gst_segment_new(); + gst_segment_init(seg, GST_FORMAT_BYTES); + gst_pad_push_event(parser->my_src, gst_event_new_segment(seg)); + } } return TRUE; -- 2.30.1
On 3/10/21 1:33 PM, Derek Lesho wrote:
Signed-off-by: Derek Lesho <dlesho(a)codeweavers.com> --- dlls/winegstreamer/wg_parser.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c index 67f65264a8d..0dc0d30f64c 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -1252,6 +1252,7 @@ static void *push_data(void *arg) { struct wg_parser *parser = arg; GstBuffer *buffer; + GstSegment *segment; guint max_size;
GST_DEBUG("Starting push thread."); @@ -1264,6 +1265,12 @@ static void *push_data(void *arg)
max_size = parser->stop_offset ? parser->stop_offset : parser->file_size;
+ gst_pad_push_event(parser->my_src, gst_event_new_stream_start("wg_stream")); + + segment = gst_segment_new(); + gst_segment_init(segment, GST_FORMAT_BYTES); + gst_pad_push_event(parser->my_src, gst_event_new_segment(segment)); +
This (and the below) leaks "segment". You probably want to allocate it on stack instead (despite the naming, it's not a proper object). Should we initialize the segment stop time (i.e. file size)?
for (;;) { ULONG size; @@ -1398,6 +1405,7 @@ static gboolean src_perform_seek(struct wg_parser *parser, GstEvent *event) GstEvent *flush_event; GstSeekFlags flags; gint64 cur, stop; + GstSegment *seg; guint32 seqnum; gdouble rate;
@@ -1431,7 +1439,12 @@ static gboolean src_perform_seek(struct wg_parser *parser, GstEvent *event) gst_event_set_seqnum(flush_event, seqnum); gst_pad_push_event(parser->my_src, flush_event); if (thread) + { gst_pad_set_active(parser->my_src, 1); + seg = gst_segment_new(); + gst_segment_init(seg, GST_FORMAT_BYTES); + gst_pad_push_event(parser->my_src, gst_event_new_segment(seg)); + } }
return TRUE;
On 3/11/21 3:27 PM, Zebediah Figura (she/her) wrote:
On 3/10/21 1:33 PM, Derek Lesho wrote:
Signed-off-by: Derek Lesho <dlesho(a)codeweavers.com> --- dlls/winegstreamer/wg_parser.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c index 67f65264a8d..0dc0d30f64c 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -1252,6 +1252,7 @@ static void *push_data(void *arg) { struct wg_parser *parser = arg; GstBuffer *buffer; + GstSegment *segment; guint max_size;
GST_DEBUG("Starting push thread."); @@ -1264,6 +1265,12 @@ static void *push_data(void *arg)
max_size = parser->stop_offset ? parser->stop_offset : parser->file_size;
+ gst_pad_push_event(parser->my_src, gst_event_new_stream_start("wg_stream")); + + segment = gst_segment_new(); + gst_segment_init(segment, GST_FORMAT_BYTES); + gst_pad_push_event(parser->my_src, gst_event_new_segment(segment)); + This (and the below) leaks "segment". You probably want to allocate it on stack instead (despite the naming, it's not a proper object). 👌
Should we initialize the segment stop time (i.e. file size)? Yeah, that wouldn't hurt.
for (;;) { ULONG size; @@ -1398,6 +1405,7 @@ static gboolean src_perform_seek(struct wg_parser *parser, GstEvent *event) GstEvent *flush_event; GstSeekFlags flags; gint64 cur, stop; + GstSegment *seg; guint32 seqnum; gdouble rate;
@@ -1431,7 +1439,12 @@ static gboolean src_perform_seek(struct wg_parser *parser, GstEvent *event) gst_event_set_seqnum(flush_event, seqnum); gst_pad_push_event(parser->my_src, flush_event); if (thread) + { gst_pad_set_active(parser->my_src, 1); + seg = gst_segment_new(); + gst_segment_init(seg, GST_FORMAT_BYTES); + gst_pad_push_event(parser->my_src, gst_event_new_segment(seg)); + } }
return TRUE;
Signed-off-by: Derek Lesho <dlesho(a)codeweavers.com> --- dlls/winegstreamer/Makefile.in | 1 + dlls/winegstreamer/decode_transform.c | 301 +++++++++++++++++++ dlls/winegstreamer/gst_private.h | 2 + dlls/winegstreamer/mfplat.c | 1 + dlls/winegstreamer/winegstreamer_classes.idl | 6 + include/mfidl.idl | 1 + 6 files changed, 312 insertions(+) create mode 100644 dlls/winegstreamer/decode_transform.c diff --git a/dlls/winegstreamer/Makefile.in b/dlls/winegstreamer/Makefile.in index 4d5dece64b3..7459cccf7e4 100644 --- a/dlls/winegstreamer/Makefile.in +++ b/dlls/winegstreamer/Makefile.in @@ -8,6 +8,7 @@ EXTRADLLFLAGS = -mno-cygwin C_SRCS = \ audioconvert.c \ + decode_transform.c \ main.c \ media_source.c \ mfplat.c \ diff --git a/dlls/winegstreamer/decode_transform.c b/dlls/winegstreamer/decode_transform.c new file mode 100644 index 00000000000..f5d4763bde4 --- /dev/null +++ b/dlls/winegstreamer/decode_transform.c @@ -0,0 +1,301 @@ +/* GStreamer Decoder Transform + * + * Copyright 2021 Derek Lesho + * + * 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 + */ + +#include "gst_private.h" + +#include "mfapi.h" +#include "mferror.h" +#include "mfobjects.h" +#include "mftransform.h" + +#include "wine/debug.h" +#include "wine/heap.h" + +WINE_DEFAULT_DEBUG_CHANNEL(mfplat); + +struct mf_decoder +{ + IMFTransform IMFTransform_iface; + LONG refcount; +}; + +static struct mf_decoder *impl_mf_decoder_from_IMFTransform(IMFTransform *iface) +{ + return CONTAINING_RECORD(iface, struct mf_decoder, IMFTransform_iface); +} + +static HRESULT WINAPI mf_decoder_QueryInterface (IMFTransform *iface, REFIID riid, void **out) +{ + TRACE("%p, %s, %p.\n", iface, debugstr_guid(riid), out); + + if (IsEqualIID(riid, &IID_IMFTransform) || + IsEqualIID(riid, &IID_IUnknown)) + { + *out = iface; + IMFTransform_AddRef(iface); + return S_OK; + } + + WARN("Unsupported %s.\n", debugstr_guid(riid)); + *out = NULL; + return E_NOINTERFACE; +} + +static ULONG WINAPI mf_decoder_AddRef(IMFTransform *iface) +{ + struct mf_decoder *decoder = impl_mf_decoder_from_IMFTransform(iface); + ULONG refcount = InterlockedIncrement(&decoder->refcount); + + TRACE("%p, refcount %u.\n", iface, refcount); + + return refcount; +} + +static ULONG WINAPI mf_decoder_Release(IMFTransform *iface) +{ + struct mf_decoder *decoder = impl_mf_decoder_from_IMFTransform(iface); + ULONG refcount = InterlockedDecrement(&decoder->refcount); + + TRACE("%p, refcount %u.\n", iface, refcount); + + if (!refcount) + { + heap_free(decoder); + } + + return refcount; +} + +static HRESULT WINAPI mf_decoder_GetStreamLimits(IMFTransform *iface, DWORD *input_minimum, DWORD *input_maximum, + DWORD *output_minimum, DWORD *output_maximum) +{ + TRACE("%p, %p, %p, %p, %p.\n", iface, input_minimum, input_maximum, output_minimum, output_maximum); + + *input_minimum = *input_maximum = *output_minimum = *output_maximum = 1; + + return S_OK; +} + +static HRESULT WINAPI mf_decoder_GetStreamCount(IMFTransform *iface, DWORD *inputs, DWORD *outputs) +{ + TRACE("%p %p %p.\n", iface, inputs, outputs); + + *inputs = *outputs = 1; + + return S_OK; +} + +static HRESULT WINAPI mf_decoder_GetStreamIDs(IMFTransform *iface, DWORD input_size, DWORD *inputs, + DWORD output_size, DWORD *outputs) +{ + TRACE("%p %u %p %u %p.\n", iface, input_size, inputs, output_size, outputs); + + return E_NOTIMPL; +} + +static HRESULT WINAPI mf_decoder_GetInputStreamInfo(IMFTransform *iface, DWORD id, MFT_INPUT_STREAM_INFO *info) +{ + FIXME("%p %u %p.\n", iface, id, info); + + return E_NOTIMPL; +} + +static HRESULT WINAPI mf_decoder_GetOutputStreamInfo(IMFTransform *iface, DWORD id, MFT_OUTPUT_STREAM_INFO *info) +{ + FIXME("%p %u %p.\n", iface, id, info); + + return E_NOTIMPL; +} + +static HRESULT WINAPI mf_decoder_GetAttributes(IMFTransform *iface, IMFAttributes **attributes) +{ + FIXME("%p, %p.\n", iface, attributes); + + return E_NOTIMPL; +} + +static HRESULT WINAPI mf_decoder_GetInputStreamAttributes(IMFTransform *iface, DWORD id, + IMFAttributes **attributes) +{ + FIXME("%p, %u, %p.\n", iface, id, attributes); + + return E_NOTIMPL; +} + +static HRESULT WINAPI mf_decoder_GetOutputStreamAttributes(IMFTransform *iface, DWORD id, + IMFAttributes **attributes) +{ + FIXME("%p, %u, %p.\n", iface, id, attributes); + + return E_NOTIMPL; +} + +static HRESULT WINAPI mf_decoder_DeleteInputStream(IMFTransform *iface, DWORD id) +{ + TRACE("%p, %u.\n", iface, id); + + return E_NOTIMPL; +} + +static HRESULT WINAPI mf_decoder_AddInputStreams(IMFTransform *iface, DWORD streams, DWORD *ids) +{ + TRACE("%p, %u, %p.\n", iface, streams, ids); + + return E_NOTIMPL; +} + +static HRESULT WINAPI mf_decoder_GetInputAvailableType(IMFTransform *iface, DWORD id, DWORD index, + IMFMediaType **type) +{ + FIXME("%p, %u, %u, %p.\n", iface, id, index, type); + + return E_NOTIMPL; +} + +static HRESULT WINAPI mf_decoder_GetOutputAvailableType(IMFTransform *iface, DWORD id, DWORD index, + IMFMediaType **type) +{ + FIXME("%p, %u, %u, %p.\n", iface, id, index, type); + + return E_NOTIMPL; +} + +static HRESULT WINAPI mf_decoder_SetInputType(IMFTransform *iface, DWORD id, IMFMediaType *type, DWORD flags) +{ + FIXME("%p, %u, %p, %#x.\n", iface, id, type, flags); + + return E_NOTIMPL; +} + +static HRESULT WINAPI mf_decoder_SetOutputType(IMFTransform *iface, DWORD id, IMFMediaType *type, DWORD flags) +{ + FIXME("%p, %u, %p, %#x.\n", iface, id, type, flags); + + return E_NOTIMPL; +} + +static HRESULT WINAPI mf_decoder_GetInputCurrentType(IMFTransform *iface, DWORD id, IMFMediaType **type) +{ + FIXME("%p, %u, %p.\n", iface, id, type); + + return E_NOTIMPL; +} + +static HRESULT WINAPI mf_decoder_GetOutputCurrentType(IMFTransform *iface, DWORD id, IMFMediaType **type) +{ + FIXME("%p, %u, %p.\n", iface, id, type); + + return E_NOTIMPL; +} + +static HRESULT WINAPI mf_decoder_GetInputStatus(IMFTransform *iface, DWORD id, DWORD *flags) +{ + FIXME("%p, %u, %p\n", iface, id, flags); + + return E_NOTIMPL; +} + +static HRESULT WINAPI mf_decoder_GetOutputStatus(IMFTransform *iface, DWORD *flags) +{ + FIXME("%p, %p.\n", iface, flags); + + return E_NOTIMPL; +} + +static HRESULT WINAPI mf_decoder_SetOutputBounds(IMFTransform *iface, LONGLONG lower, LONGLONG upper) +{ + FIXME("%p, %s, %s.\n", iface, wine_dbgstr_longlong(lower), wine_dbgstr_longlong(upper)); + + return E_NOTIMPL; +} + +static HRESULT WINAPI mf_decoder_ProcessEvent(IMFTransform *iface, DWORD id, IMFMediaEvent *event) +{ + FIXME("%p, %u, %p.\n", iface, id, event); + + return E_NOTIMPL; +} + +static HRESULT WINAPI mf_decoder_ProcessMessage(IMFTransform *iface, MFT_MESSAGE_TYPE message, ULONG_PTR param) +{ + FIXME("%p, %u %lu.\n", iface, message, param); + + return E_NOTIMPL; +} + +static HRESULT WINAPI mf_decoder_ProcessInput(IMFTransform *iface, DWORD id, IMFSample *sample, DWORD flags) +{ + FIXME("%p, %u, %p, %#x.\n", iface, id, sample, flags); + + return E_NOTIMPL; +} + +static HRESULT WINAPI mf_decoder_ProcessOutput(IMFTransform *iface, DWORD flags, DWORD count, + MFT_OUTPUT_DATA_BUFFER *samples, DWORD *status) +{ + FIXME("%p, %#x, %u, %p, %p.\n", iface, flags, count, samples, status); + + return E_NOTIMPL; +} + +static const IMFTransformVtbl mf_decoder_vtbl = +{ + mf_decoder_QueryInterface, + mf_decoder_AddRef, + mf_decoder_Release, + mf_decoder_GetStreamLimits, + mf_decoder_GetStreamCount, + mf_decoder_GetStreamIDs, + mf_decoder_GetInputStreamInfo, + mf_decoder_GetOutputStreamInfo, + mf_decoder_GetAttributes, + mf_decoder_GetInputStreamAttributes, + mf_decoder_GetOutputStreamAttributes, + mf_decoder_DeleteInputStream, + mf_decoder_AddInputStreams, + mf_decoder_GetInputAvailableType, + mf_decoder_GetOutputAvailableType, + mf_decoder_SetInputType, + mf_decoder_SetOutputType, + mf_decoder_GetInputCurrentType, + mf_decoder_GetOutputCurrentType, + mf_decoder_GetInputStatus, + mf_decoder_GetOutputStatus, + mf_decoder_SetOutputBounds, + mf_decoder_ProcessEvent, + mf_decoder_ProcessMessage, + mf_decoder_ProcessInput, + mf_decoder_ProcessOutput, +}; + +HRESULT decode_transform_create(REFIID riid, void **obj) +{ + struct mf_decoder *object; + + TRACE("%s, %p.\n", debugstr_guid(riid), obj); + + if (!(object = heap_alloc_zero(sizeof(*object)))) + return E_OUTOFMEMORY; + + object->IMFTransform_iface.lpVtbl = &mf_decoder_vtbl; + object->refcount = 1; + + *obj = &object->IMFTransform_iface; + return S_OK; +} diff --git a/dlls/winegstreamer/gst_private.h b/dlls/winegstreamer/gst_private.h index 3f09d7cf4f5..c97c874fd68 100644 --- a/dlls/winegstreamer/gst_private.h +++ b/dlls/winegstreamer/gst_private.h @@ -216,4 +216,6 @@ HRESULT winegstreamer_stream_handler_create(REFIID riid, void **obj) DECLSPEC_HI HRESULT audio_converter_create(REFIID riid, void **ret) DECLSPEC_HIDDEN; +HRESULT decode_transform_create(REFIID riid, void **obj) DECLSPEC_HIDDEN; + #endif /* __GST_PRIVATE_INCLUDED__ */ diff --git a/dlls/winegstreamer/mfplat.c b/dlls/winegstreamer/mfplat.c index 602fa631a43..cb862375276 100644 --- a/dlls/winegstreamer/mfplat.c +++ b/dlls/winegstreamer/mfplat.c @@ -412,6 +412,7 @@ class_objects[] = { &CLSID_VideoProcessorMFT, &video_processor_create }, { &CLSID_GStreamerByteStreamHandler, &winegstreamer_stream_handler_create }, { &CLSID_WINEAudioConverter, &audio_converter_create }, + { &CLSID_CMSH264DecoderMFT, &decode_transform_create }, }; HRESULT mfplat_get_class_object(REFCLSID rclsid, REFIID riid, void **obj) diff --git a/dlls/winegstreamer/winegstreamer_classes.idl b/dlls/winegstreamer/winegstreamer_classes.idl index 072ec90eea4..064a6872c79 100644 --- a/dlls/winegstreamer/winegstreamer_classes.idl +++ b/dlls/winegstreamer/winegstreamer_classes.idl @@ -67,3 +67,9 @@ coclass GStreamerByteStreamHandler {} uuid(6a170414-aad9-4693-b806-3a0c47c570d6) ] coclass WINEAudioConverter { } + +[ + threading(both), + uuid(62ce7e72-4c71-4d20-b15d-452831a87d9d) +] +coclass CMSH264DecoderMFT { } diff --git a/include/mfidl.idl b/include/mfidl.idl index 46c715a3752..63b2ab84c92 100644 --- a/include/mfidl.idl +++ b/include/mfidl.idl @@ -1412,3 +1412,4 @@ cpp_quote("EXTERN_GUID(MF_ACTIVATE_CUSTOM_VIDEO_PRESENTER_ACTIVATE, 0xba491365, cpp_quote("EXTERN_GUID(MF_ACTIVATE_CUSTOM_VIDEO_PRESENTER_FLAGS, 0xba491366, 0xbe50, 0x451e, 0x95, 0xab, 0x6d, 0x4a, 0xcc, 0xc7, 0xda, 0xd8);") cpp_quote("EXTERN_GUID(CLSID_VideoProcessorMFT, 0x88753b26, 0x5b24, 0x49bd, 0xb2, 0xe7, 0xc, 0x44, 0x5c, 0x78, 0xc9, 0x82);") +cpp_quote("EXTERN_GUID(CLSID_CMSH264DecoderMFT, 0x62ce7e72, 0x4c71, 0x4d20, 0xb1, 0x5d, 0x45, 0x28, 0x31, 0xa8, 0x7d, 0x9d);") -- 2.30.1
On 3/10/21 1:33 PM, Derek Lesho wrote:
Signed-off-by: Derek Lesho <dlesho(a)codeweavers.com> --- dlls/winegstreamer/Makefile.in | 1 + dlls/winegstreamer/decode_transform.c | 301 +++++++++++++++++++ dlls/winegstreamer/gst_private.h | 2 + dlls/winegstreamer/mfplat.c | 1 + dlls/winegstreamer/winegstreamer_classes.idl | 6 + include/mfidl.idl | 1 + 6 files changed, 312 insertions(+) create mode 100644 dlls/winegstreamer/decode_transform.c
This patch is just mfplat code, so I'm not particularly qualified to review it. It might make sense to prepend "mf" or "mfplat" to "decode_transform", though, maybe shortening it to "mfplat_decoder". We may want to introduce a DirectShow filter (see e.g. bug 34744), which can also be said to be a decoder transform.
On 3/11/21 3:33 PM, Zebediah Figura (she/her) wrote:
On 3/10/21 1:33 PM, Derek Lesho wrote:
Signed-off-by: Derek Lesho <dlesho(a)codeweavers.com> --- dlls/winegstreamer/Makefile.in | 1 + dlls/winegstreamer/decode_transform.c | 301 +++++++++++++++++++ dlls/winegstreamer/gst_private.h | 2 + dlls/winegstreamer/mfplat.c | 1 + dlls/winegstreamer/winegstreamer_classes.idl | 6 + include/mfidl.idl | 1 + 6 files changed, 312 insertions(+) create mode 100644 dlls/winegstreamer/decode_transform.c
This patch is just mfplat code, so I'm not particularly qualified to review it.
It might make sense to prepend "mf" or "mfplat" to "decode_transform", though, maybe shortening it to "mfplat_decoder". We may want to introduce a DirectShow filter (see e.g. bug 34744), which can also be said to be a decoder transform. Hmm, from what I've read on the MSDN many transforms share the same code, exposing themselves as both a directshow filter and a MFT. Would we want to replicate this? For what it's worth, I don't plan on looking into this any time soon, so maybe for now mfplat_decoder is better and it could be renamed to something more appropriate if/when somebody took this up.
On 3/11/21 2:39 PM, Derek Lesho wrote:
On 3/11/21 3:33 PM, Zebediah Figura (she/her) wrote:
On 3/10/21 1:33 PM, Derek Lesho wrote:
Signed-off-by: Derek Lesho <dlesho(a)codeweavers.com> --- dlls/winegstreamer/Makefile.in | 1 + dlls/winegstreamer/decode_transform.c | 301 +++++++++++++++++++ dlls/winegstreamer/gst_private.h | 2 + dlls/winegstreamer/mfplat.c | 1 + dlls/winegstreamer/winegstreamer_classes.idl | 6 + include/mfidl.idl | 1 + 6 files changed, 312 insertions(+) create mode 100644 dlls/winegstreamer/decode_transform.c
This patch is just mfplat code, so I'm not particularly qualified to review it.
It might make sense to prepend "mf" or "mfplat" to "decode_transform", though, maybe shortening it to "mfplat_decoder". We may want to introduce a DirectShow filter (see e.g. bug 34744), which can also be said to be a decoder transform. Hmm, from what I've read on the MSDN many transforms share the same code, exposing themselves as both a directshow filter and a MFT. Would we want to replicate this? For what it's worth, I don't plan on looking into this any time soon, so maybe for now mfplat_decoder is better and it could be renamed to something more appropriate if/when somebody took this up.
MSDN is kind of fudging the truth, what these transforms are actually exposing is not a native DirectShow filter but rather a DirectX Media Object (DMO), which can be wrapped by DirectShow. Still, you have a good point, we may likely want to use a combined object here.
Signed-off-by: Derek Lesho <dlesho(a)codeweavers.com> --- dlls/winegstreamer/decode_transform.c | 60 +++++++++++++++++++++++++-- dlls/winegstreamer/gst_private.h | 6 ++- dlls/winegstreamer/mfplat.c | 7 +++- 3 files changed, 67 insertions(+), 6 deletions(-) diff --git a/dlls/winegstreamer/decode_transform.c b/dlls/winegstreamer/decode_transform.c index f5d4763bde4..55a0c1c6c9b 100644 --- a/dlls/winegstreamer/decode_transform.c +++ b/dlls/winegstreamer/decode_transform.c @@ -29,10 +29,33 @@ WINE_DEFAULT_DEBUG_CHANNEL(mfplat); +const GUID *h264_input_types[] = {&MFVideoFormat_H264}; +/* NV12 comes first https://docs.microsoft.com/en-us/windows/win32/medfound/mft-decoder-expose-o... . thanks to @vitorhnn */ +const GUID *h264_output_types[] = {&MFVideoFormat_NV12, &MFVideoFormat_I420, &MFVideoFormat_IYUV, &MFVideoFormat_YUY2, &MFVideoFormat_YV12}; + +static struct decoder_desc +{ + const GUID *major_type; + const GUID **input_types; + unsigned int input_types_count; + const GUID **output_types; + unsigned int output_types_count; +} decoder_descs[] = +{ + { /* DECODER_TYPE_H264 */ + &MFMediaType_Video, + h264_input_types, + ARRAY_SIZE(h264_input_types), + h264_output_types, + ARRAY_SIZE(h264_output_types), + }, +}; + struct mf_decoder { IMFTransform IMFTransform_iface; LONG refcount; + enum decoder_type type; }; static struct mf_decoder *impl_mf_decoder_from_IMFTransform(IMFTransform *iface) @@ -163,9 +186,36 @@ static HRESULT WINAPI mf_decoder_AddInputStreams(IMFTransform *iface, DWORD stre static HRESULT WINAPI mf_decoder_GetInputAvailableType(IMFTransform *iface, DWORD id, DWORD index, IMFMediaType **type) { - FIXME("%p, %u, %u, %p.\n", iface, id, index, type); + struct mf_decoder *decoder = impl_mf_decoder_from_IMFTransform(iface); + IMFMediaType *input_type; + HRESULT hr; - return E_NOTIMPL; + TRACE("%p, %u, %u, %p\n", decoder, id, index, type); + + if (id != 0) + return MF_E_INVALIDSTREAMNUMBER; + + if (index >= decoder_descs[decoder->type].input_types_count) + return MF_E_NO_MORE_TYPES; + + if (FAILED(hr = MFCreateMediaType(&input_type))) + return hr; + + if (FAILED(hr = IMFMediaType_SetGUID(input_type, &MF_MT_MAJOR_TYPE, decoder_descs[decoder->type].major_type))) + { + IMFMediaType_Release(input_type); + return hr; + } + + if (FAILED(hr = IMFMediaType_SetGUID(input_type, &MF_MT_SUBTYPE, decoder_descs[decoder->type].input_types[index]))) + { + IMFMediaType_Release(input_type); + return hr; + } + + *type = input_type; + + return S_OK; } static HRESULT WINAPI mf_decoder_GetOutputAvailableType(IMFTransform *iface, DWORD id, DWORD index, @@ -284,11 +334,11 @@ static const IMFTransformVtbl mf_decoder_vtbl = mf_decoder_ProcessOutput, }; -HRESULT decode_transform_create(REFIID riid, void **obj) +HRESULT decode_transform_create(REFIID riid, void **obj, enum decoder_type type) { struct mf_decoder *object; - TRACE("%s, %p.\n", debugstr_guid(riid), obj); + TRACE("%s, %p %u.\n", debugstr_guid(riid), obj, type); if (!(object = heap_alloc_zero(sizeof(*object)))) return E_OUTOFMEMORY; @@ -296,6 +346,8 @@ HRESULT decode_transform_create(REFIID riid, void **obj) object->IMFTransform_iface.lpVtbl = &mf_decoder_vtbl; object->refcount = 1; + object->type = type; + *obj = &object->IMFTransform_iface; return S_OK; } diff --git a/dlls/winegstreamer/gst_private.h b/dlls/winegstreamer/gst_private.h index c97c874fd68..54aba7a09ae 100644 --- a/dlls/winegstreamer/gst_private.h +++ b/dlls/winegstreamer/gst_private.h @@ -216,6 +216,10 @@ HRESULT winegstreamer_stream_handler_create(REFIID riid, void **obj) DECLSPEC_HI HRESULT audio_converter_create(REFIID riid, void **ret) DECLSPEC_HIDDEN; -HRESULT decode_transform_create(REFIID riid, void **obj) DECLSPEC_HIDDEN; +enum decoder_type +{ + DECODER_TYPE_H264, +}; +HRESULT decode_transform_create(REFIID riid, void **obj, enum decoder_type) DECLSPEC_HIDDEN; #endif /* __GST_PRIVATE_INCLUDED__ */ diff --git a/dlls/winegstreamer/mfplat.c b/dlls/winegstreamer/mfplat.c index cb862375276..65c55e4835d 100644 --- a/dlls/winegstreamer/mfplat.c +++ b/dlls/winegstreamer/mfplat.c @@ -402,6 +402,11 @@ static const GUID CLSID_GStreamerByteStreamHandler = {0x317df618, 0x5e5a, 0x468a static const GUID CLSID_WINEAudioConverter = {0x6a170414,0xaad9,0x4693,{0xb8,0x06,0x3a,0x0c,0x47,0xc5,0x70,0xd6}}; +static HRESULT h264_decoder_create(REFIID riid, void **ret) +{ + return decode_transform_create(riid, ret, DECODER_TYPE_H264); +} + static const struct class_object { const GUID *clsid; @@ -412,7 +417,7 @@ class_objects[] = { &CLSID_VideoProcessorMFT, &video_processor_create }, { &CLSID_GStreamerByteStreamHandler, &winegstreamer_stream_handler_create }, { &CLSID_WINEAudioConverter, &audio_converter_create }, - { &CLSID_CMSH264DecoderMFT, &decode_transform_create }, + { &CLSID_CMSH264DecoderMFT, &h264_decoder_create }, }; HRESULT mfplat_get_class_object(REFCLSID rclsid, REFIID riid, void **obj) -- 2.30.1
On 3/10/21 1:33 PM, Derek Lesho wrote:
Signed-off-by: Derek Lesho <dlesho(a)codeweavers.com> --- dlls/winegstreamer/decode_transform.c | 60 +++++++++++++++++++++++++-- dlls/winegstreamer/gst_private.h | 6 ++- dlls/winegstreamer/mfplat.c | 7 +++- 3 files changed, 67 insertions(+), 6 deletions(-)
Applications are creating this specific object, right? Can they be mentioned in the patch subject or in a commment? Also, should we add tests, at least corresponding to what the applications are doing?
diff --git a/dlls/winegstreamer/decode_transform.c b/dlls/winegstreamer/decode_transform.c index f5d4763bde4..55a0c1c6c9b 100644 --- a/dlls/winegstreamer/decode_transform.c +++ b/dlls/winegstreamer/decode_transform.c @@ -29,10 +29,33 @@
WINE_DEFAULT_DEBUG_CHANNEL(mfplat);
+const GUID *h264_input_types[] = {&MFVideoFormat_H264}; +/* NV12 comes first https://docs.microsoft.com/en-us/windows/win32/medfound/mft-decoder-expose-o... . thanks to @vitorhnn */ +const GUID *h264_output_types[] = {&MFVideoFormat_NV12, &MFVideoFormat_I420, &MFVideoFormat_IYUV, &MFVideoFormat_YUY2, &MFVideoFormat_YV12};
"static const GUID *const h264_input_types[]"
+ +static struct decoder_desc
"static const", but...
+{ + const GUID *major_type; + const GUID **input_types; + unsigned int input_types_count; + const GUID **output_types; + unsigned int output_types_count; +} decoder_descs[] = +{ + { /* DECODER_TYPE_H264 */ + &MFMediaType_Video, + h264_input_types, + ARRAY_SIZE(h264_input_types), + h264_output_types, + ARRAY_SIZE(h264_output_types), + }, +}; + struct mf_decoder { IMFTransform IMFTransform_iface; LONG refcount; + enum decoder_type type; };
...you might find it cleaner to pass and store a "const struct decoder_desc *", which would get rid of the need to keep the "decoder_descs" array in the correct order.
static struct mf_decoder *impl_mf_decoder_from_IMFTransform(IMFTransform *iface) @@ -163,9 +186,36 @@ static HRESULT WINAPI mf_decoder_AddInputStreams(IMFTransform *iface, DWORD stre static HRESULT WINAPI mf_decoder_GetInputAvailableType(IMFTransform *iface, DWORD id, DWORD index, IMFMediaType **type) { - FIXME("%p, %u, %u, %p.\n", iface, id, index, type); + struct mf_decoder *decoder = impl_mf_decoder_from_IMFTransform(iface); + IMFMediaType *input_type; + HRESULT hr;
- return E_NOTIMPL; + TRACE("%p, %u, %u, %p\n", decoder, id, index, type); + + if (id != 0) + return MF_E_INVALIDSTREAMNUMBER; + + if (index >= decoder_descs[decoder->type].input_types_count) + return MF_E_NO_MORE_TYPES; + + if (FAILED(hr = MFCreateMediaType(&input_type))) + return hr; + + if (FAILED(hr = IMFMediaType_SetGUID(input_type, &MF_MT_MAJOR_TYPE, decoder_descs[decoder->type].major_type))) + { + IMFMediaType_Release(input_type); + return hr; + } + + if (FAILED(hr = IMFMediaType_SetGUID(input_type, &MF_MT_SUBTYPE, decoder_descs[decoder->type].input_types[index]))) + { + IMFMediaType_Release(input_type); + return hr; + } + + *type = input_type; + + return S_OK; }
static HRESULT WINAPI mf_decoder_GetOutputAvailableType(IMFTransform *iface, DWORD id, DWORD index, @@ -284,11 +334,11 @@ static const IMFTransformVtbl mf_decoder_vtbl = mf_decoder_ProcessOutput, };
-HRESULT decode_transform_create(REFIID riid, void **obj) +HRESULT decode_transform_create(REFIID riid, void **obj, enum decoder_type type) { struct mf_decoder *object;
- TRACE("%s, %p.\n", debugstr_guid(riid), obj); + TRACE("%s, %p %u.\n", debugstr_guid(riid), obj, type);
if (!(object = heap_alloc_zero(sizeof(*object)))) return E_OUTOFMEMORY; @@ -296,6 +346,8 @@ HRESULT decode_transform_create(REFIID riid, void **obj) object->IMFTransform_iface.lpVtbl = &mf_decoder_vtbl; object->refcount = 1;
+ object->type = type; + *obj = &object->IMFTransform_iface; return S_OK; } diff --git a/dlls/winegstreamer/gst_private.h b/dlls/winegstreamer/gst_private.h index c97c874fd68..54aba7a09ae 100644 --- a/dlls/winegstreamer/gst_private.h +++ b/dlls/winegstreamer/gst_private.h @@ -216,6 +216,10 @@ HRESULT winegstreamer_stream_handler_create(REFIID riid, void **obj) DECLSPEC_HI
HRESULT audio_converter_create(REFIID riid, void **ret) DECLSPEC_HIDDEN;
-HRESULT decode_transform_create(REFIID riid, void **obj) DECLSPEC_HIDDEN; +enum decoder_type +{ + DECODER_TYPE_H264, +}; +HRESULT decode_transform_create(REFIID riid, void **obj, enum decoder_type) DECLSPEC_HIDDEN;
#endif /* __GST_PRIVATE_INCLUDED__ */ diff --git a/dlls/winegstreamer/mfplat.c b/dlls/winegstreamer/mfplat.c index cb862375276..65c55e4835d 100644 --- a/dlls/winegstreamer/mfplat.c +++ b/dlls/winegstreamer/mfplat.c @@ -402,6 +402,11 @@ static const GUID CLSID_GStreamerByteStreamHandler = {0x317df618, 0x5e5a, 0x468a
static const GUID CLSID_WINEAudioConverter = {0x6a170414,0xaad9,0x4693,{0xb8,0x06,0x3a,0x0c,0x47,0xc5,0x70,0xd6}};
+static HRESULT h264_decoder_create(REFIID riid, void **ret) +{ + return decode_transform_create(riid, ret, DECODER_TYPE_H264); +} + static const struct class_object { const GUID *clsid; @@ -412,7 +417,7 @@ class_objects[] = { &CLSID_VideoProcessorMFT, &video_processor_create }, { &CLSID_GStreamerByteStreamHandler, &winegstreamer_stream_handler_create }, { &CLSID_WINEAudioConverter, &audio_converter_create }, - { &CLSID_CMSH264DecoderMFT, &decode_transform_create }, + { &CLSID_CMSH264DecoderMFT, &h264_decoder_create }, };
HRESULT mfplat_get_class_object(REFCLSID rclsid, REFIID riid, void **obj)
On 3/11/21 3:39 PM, Zebediah Figura (she/her) wrote:
On 3/10/21 1:33 PM, Derek Lesho wrote:
Signed-off-by: Derek Lesho <dlesho(a)codeweavers.com> --- dlls/winegstreamer/decode_transform.c | 60 +++++++++++++++++++++++++-- dlls/winegstreamer/gst_private.h | 6 ++- dlls/winegstreamer/mfplat.c | 7 +++- 3 files changed, 67 insertions(+), 6 deletions(-)
Applications are creating this specific object, right? Can they be mentioned in the patch subject or in a commment? Yep, sounds good.
Also, should we add tests, at least corresponding to what the applications are doing? Yeah, we could, although what they're doing is not very noteworthy, they just set the input and output types and start pushing their stream through ProcessInput, then wait for decoded samples on ProcessOutput. We're pretty much a thunk between them and the decoder.
diff --git a/dlls/winegstreamer/decode_transform.c b/dlls/winegstreamer/decode_transform.c index f5d4763bde4..55a0c1c6c9b 100644 --- a/dlls/winegstreamer/decode_transform.c +++ b/dlls/winegstreamer/decode_transform.c @@ -29,10 +29,33 @@
WINE_DEFAULT_DEBUG_CHANNEL(mfplat);
+const GUID *h264_input_types[] = {&MFVideoFormat_H264}; +/* NV12 comes first https://docs.microsoft.com/en-us/windows/win32/medfound/mft-decoder-expose-o... . thanks to @vitorhnn */ +const GUID *h264_output_types[] = {&MFVideoFormat_NV12, &MFVideoFormat_I420, &MFVideoFormat_IYUV, &MFVideoFormat_YUY2, &MFVideoFormat_YV12}; "static const GUID *const h264_input_types[]"
+ +static struct decoder_desc "static const", but...
+{ + const GUID *major_type; + const GUID **input_types; + unsigned int input_types_count; + const GUID **output_types; + unsigned int output_types_count; +} decoder_descs[] = +{ + { /* DECODER_TYPE_H264 */ + &MFMediaType_Video, + h264_input_types, + ARRAY_SIZE(h264_input_types), + h264_output_types, + ARRAY_SIZE(h264_output_types), + }, +}; + struct mf_decoder { IMFTransform IMFTransform_iface; LONG refcount; + enum decoder_type type; };
...you might find it cleaner to pass and store a "const struct decoder_desc *", which would get rid of the need to keep the "decoder_descs" array in the correct order. 👌
static struct mf_decoder *impl_mf_decoder_from_IMFTransform(IMFTransform *iface) @@ -163,9 +186,36 @@ static HRESULT WINAPI mf_decoder_AddInputStreams(IMFTransform *iface, DWORD stre static HRESULT WINAPI mf_decoder_GetInputAvailableType(IMFTransform *iface, DWORD id, DWORD index, IMFMediaType **type) { - FIXME("%p, %u, %u, %p.\n", iface, id, index, type); + struct mf_decoder *decoder = impl_mf_decoder_from_IMFTransform(iface); + IMFMediaType *input_type; + HRESULT hr;
- return E_NOTIMPL; + TRACE("%p, %u, %u, %p\n", decoder, id, index, type); + + if (id != 0) + return MF_E_INVALIDSTREAMNUMBER; + + if (index >= decoder_descs[decoder->type].input_types_count) + return MF_E_NO_MORE_TYPES; + + if (FAILED(hr = MFCreateMediaType(&input_type))) + return hr; + + if (FAILED(hr = IMFMediaType_SetGUID(input_type, &MF_MT_MAJOR_TYPE, decoder_descs[decoder->type].major_type))) + { + IMFMediaType_Release(input_type); + return hr; + } + + if (FAILED(hr = IMFMediaType_SetGUID(input_type, &MF_MT_SUBTYPE, decoder_descs[decoder->type].input_types[index]))) + { + IMFMediaType_Release(input_type); + return hr; + } + + *type = input_type; + + return S_OK; }
static HRESULT WINAPI mf_decoder_GetOutputAvailableType(IMFTransform *iface, DWORD id, DWORD index, @@ -284,11 +334,11 @@ static const IMFTransformVtbl mf_decoder_vtbl = mf_decoder_ProcessOutput, };
-HRESULT decode_transform_create(REFIID riid, void **obj) +HRESULT decode_transform_create(REFIID riid, void **obj, enum decoder_type type) { struct mf_decoder *object;
- TRACE("%s, %p.\n", debugstr_guid(riid), obj); + TRACE("%s, %p %u.\n", debugstr_guid(riid), obj, type);
if (!(object = heap_alloc_zero(sizeof(*object)))) return E_OUTOFMEMORY; @@ -296,6 +346,8 @@ HRESULT decode_transform_create(REFIID riid, void **obj) object->IMFTransform_iface.lpVtbl = &mf_decoder_vtbl; object->refcount = 1;
+ object->type = type; + *obj = &object->IMFTransform_iface; return S_OK; } diff --git a/dlls/winegstreamer/gst_private.h b/dlls/winegstreamer/gst_private.h index c97c874fd68..54aba7a09ae 100644 --- a/dlls/winegstreamer/gst_private.h +++ b/dlls/winegstreamer/gst_private.h @@ -216,6 +216,10 @@ HRESULT winegstreamer_stream_handler_create(REFIID riid, void **obj) DECLSPEC_HI
HRESULT audio_converter_create(REFIID riid, void **ret) DECLSPEC_HIDDEN;
-HRESULT decode_transform_create(REFIID riid, void **obj) DECLSPEC_HIDDEN; +enum decoder_type +{ + DECODER_TYPE_H264, +}; +HRESULT decode_transform_create(REFIID riid, void **obj, enum decoder_type) DECLSPEC_HIDDEN;
#endif /* __GST_PRIVATE_INCLUDED__ */ diff --git a/dlls/winegstreamer/mfplat.c b/dlls/winegstreamer/mfplat.c index cb862375276..65c55e4835d 100644 --- a/dlls/winegstreamer/mfplat.c +++ b/dlls/winegstreamer/mfplat.c @@ -402,6 +402,11 @@ static const GUID CLSID_GStreamerByteStreamHandler = {0x317df618, 0x5e5a, 0x468a
static const GUID CLSID_WINEAudioConverter = {0x6a170414,0xaad9,0x4693,{0xb8,0x06,0x3a,0x0c,0x47,0xc5,0x70,0xd6}};
+static HRESULT h264_decoder_create(REFIID riid, void **ret) +{ + return decode_transform_create(riid, ret, DECODER_TYPE_H264); +} + static const struct class_object { const GUID *clsid; @@ -412,7 +417,7 @@ class_objects[] = { &CLSID_VideoProcessorMFT, &video_processor_create }, { &CLSID_GStreamerByteStreamHandler, &winegstreamer_stream_handler_create }, { &CLSID_WINEAudioConverter, &audio_converter_create }, - { &CLSID_CMSH264DecoderMFT, &decode_transform_create }, + { &CLSID_CMSH264DecoderMFT, &h264_decoder_create }, };
HRESULT mfplat_get_class_object(REFCLSID rclsid, REFIID riid, void **obj)
On 3/11/21 2:51 PM, Derek Lesho wrote:
On 3/11/21 3:39 PM, Zebediah Figura (she/her) wrote:
On 3/10/21 1:33 PM, Derek Lesho wrote:
Signed-off-by: Derek Lesho <dlesho(a)codeweavers.com> --- dlls/winegstreamer/decode_transform.c | 60 +++++++++++++++++++++++++-- dlls/winegstreamer/gst_private.h | 6 ++- dlls/winegstreamer/mfplat.c | 7 +++- 3 files changed, 67 insertions(+), 6 deletions(-)
Applications are creating this specific object, right? Can they be mentioned in the patch subject or in a commment? Yep, sounds good.
Also, should we add tests, at least corresponding to what the applications are doing? Yeah, we could, although what they're doing is not very noteworthy, they just set the input and output types and start pushing their stream through ProcessInput, then wait for decoded samples on ProcessOutput. We're pretty much a thunk between them and the decoder.
Sure, but even so, I think it's a good idea to have tests, to make sure that the whole stack works, and probably also to prove our media type handling code is correct to some degree.
diff --git a/dlls/winegstreamer/decode_transform.c b/dlls/winegstreamer/decode_transform.c index f5d4763bde4..55a0c1c6c9b 100644 --- a/dlls/winegstreamer/decode_transform.c +++ b/dlls/winegstreamer/decode_transform.c @@ -29,10 +29,33 @@ WINE_DEFAULT_DEBUG_CHANNEL(mfplat); +const GUID *h264_input_types[] = {&MFVideoFormat_H264}; +/* NV12 comes first https://docs.microsoft.com/en-us/windows/win32/medfound/mft-decoder-expose-o... . thanks to @vitorhnn */ +const GUID *h264_output_types[] = {&MFVideoFormat_NV12, &MFVideoFormat_I420, &MFVideoFormat_IYUV, &MFVideoFormat_YUY2, &MFVideoFormat_YV12}; "static const GUID *const h264_input_types[]"
+ +static struct decoder_desc "static const", but...
+{ + const GUID *major_type; + const GUID **input_types; + unsigned int input_types_count; + const GUID **output_types; + unsigned int output_types_count; +} decoder_descs[] = +{ + { /* DECODER_TYPE_H264 */ + &MFMediaType_Video, + h264_input_types, + ARRAY_SIZE(h264_input_types), + h264_output_types, + ARRAY_SIZE(h264_output_types), + }, +}; + struct mf_decoder { IMFTransform IMFTransform_iface; LONG refcount; + enum decoder_type type; }; ...you might find it cleaner to pass and store a "const struct decoder_desc *", which would get rid of the need to keep the "decoder_descs" array in the correct order.
👌
static struct mf_decoder *impl_mf_decoder_from_IMFTransform(IMFTransform *iface) @@ -163,9 +186,36 @@ static HRESULT WINAPI mf_decoder_AddInputStreams(IMFTransform *iface, DWORD stre static HRESULT WINAPI mf_decoder_GetInputAvailableType(IMFTransform *iface, DWORD id, DWORD index, IMFMediaType **type) { - FIXME("%p, %u, %u, %p.\n", iface, id, index, type); + struct mf_decoder *decoder = impl_mf_decoder_from_IMFTransform(iface); + IMFMediaType *input_type; + HRESULT hr; - return E_NOTIMPL; + TRACE("%p, %u, %u, %p\n", decoder, id, index, type); + + if (id != 0) + return MF_E_INVALIDSTREAMNUMBER; + + if (index >= decoder_descs[decoder->type].input_types_count) + return MF_E_NO_MORE_TYPES; + + if (FAILED(hr = MFCreateMediaType(&input_type))) + return hr; + + if (FAILED(hr = IMFMediaType_SetGUID(input_type, &MF_MT_MAJOR_TYPE, decoder_descs[decoder->type].major_type))) + { + IMFMediaType_Release(input_type); + return hr; + } + + if (FAILED(hr = IMFMediaType_SetGUID(input_type, &MF_MT_SUBTYPE, decoder_descs[decoder->type].input_types[index]))) + { + IMFMediaType_Release(input_type); + return hr; + } + + *type = input_type; + + return S_OK; } static HRESULT WINAPI mf_decoder_GetOutputAvailableType(IMFTransform *iface, DWORD id, DWORD index, @@ -284,11 +334,11 @@ static const IMFTransformVtbl mf_decoder_vtbl = mf_decoder_ProcessOutput, }; -HRESULT decode_transform_create(REFIID riid, void **obj) +HRESULT decode_transform_create(REFIID riid, void **obj, enum decoder_type type) { struct mf_decoder *object; - TRACE("%s, %p.\n", debugstr_guid(riid), obj); + TRACE("%s, %p %u.\n", debugstr_guid(riid), obj, type); if (!(object = heap_alloc_zero(sizeof(*object)))) return E_OUTOFMEMORY; @@ -296,6 +346,8 @@ HRESULT decode_transform_create(REFIID riid, void **obj) object->IMFTransform_iface.lpVtbl = &mf_decoder_vtbl; object->refcount = 1; + object->type = type; + *obj = &object->IMFTransform_iface; return S_OK; } diff --git a/dlls/winegstreamer/gst_private.h b/dlls/winegstreamer/gst_private.h index c97c874fd68..54aba7a09ae 100644 --- a/dlls/winegstreamer/gst_private.h +++ b/dlls/winegstreamer/gst_private.h @@ -216,6 +216,10 @@ HRESULT winegstreamer_stream_handler_create(REFIID riid, void **obj) DECLSPEC_HI HRESULT audio_converter_create(REFIID riid, void **ret) DECLSPEC_HIDDEN; -HRESULT decode_transform_create(REFIID riid, void **obj) DECLSPEC_HIDDEN; +enum decoder_type +{ + DECODER_TYPE_H264, +}; +HRESULT decode_transform_create(REFIID riid, void **obj, enum decoder_type) DECLSPEC_HIDDEN; #endif /* __GST_PRIVATE_INCLUDED__ */ diff --git a/dlls/winegstreamer/mfplat.c b/dlls/winegstreamer/mfplat.c index cb862375276..65c55e4835d 100644 --- a/dlls/winegstreamer/mfplat.c +++ b/dlls/winegstreamer/mfplat.c @@ -402,6 +402,11 @@ static const GUID CLSID_GStreamerByteStreamHandler = {0x317df618, 0x5e5a, 0x468a static const GUID CLSID_WINEAudioConverter = {0x6a170414,0xaad9,0x4693,{0xb8,0x06,0x3a,0x0c,0x47,0xc5,0x70,0xd6}}; +static HRESULT h264_decoder_create(REFIID riid, void **ret) +{ + return decode_transform_create(riid, ret, DECODER_TYPE_H264); +} + static const struct class_object { const GUID *clsid; @@ -412,7 +417,7 @@ class_objects[] = { &CLSID_VideoProcessorMFT, &video_processor_create }, { &CLSID_GStreamerByteStreamHandler, &winegstreamer_stream_handler_create }, { &CLSID_WINEAudioConverter, &audio_converter_create }, - { &CLSID_CMSH264DecoderMFT, &decode_transform_create }, + { &CLSID_CMSH264DecoderMFT, &h264_decoder_create }, }; HRESULT mfplat_get_class_object(REFCLSID rclsid, REFIID riid, void **obj)
Signed-off-by: Derek Lesho <dlesho(a)codeweavers.com> --- dlls/winegstreamer/decode_transform.c | 31 +++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/dlls/winegstreamer/decode_transform.c b/dlls/winegstreamer/decode_transform.c index 55a0c1c6c9b..3c71fddd67c 100644 --- a/dlls/winegstreamer/decode_transform.c +++ b/dlls/winegstreamer/decode_transform.c @@ -221,9 +221,36 @@ static HRESULT WINAPI mf_decoder_GetInputAvailableType(IMFTransform *iface, DWOR static HRESULT WINAPI mf_decoder_GetOutputAvailableType(IMFTransform *iface, DWORD id, DWORD index, IMFMediaType **type) { - FIXME("%p, %u, %u, %p.\n", iface, id, index, type); + struct mf_decoder *decoder = impl_mf_decoder_from_IMFTransform(iface); + IMFMediaType *output_type; + HRESULT hr; - return E_NOTIMPL; + TRACE("%p, %u, %u, %p\n", decoder, id, index, type); + + if (id != 0) + return MF_E_INVALIDSTREAMNUMBER; + + if (index >= decoder_descs[decoder->type].output_types_count) + return MF_E_NO_MORE_TYPES; + + if (FAILED(hr = MFCreateMediaType(&output_type))) + return hr; + + if (FAILED(hr = IMFMediaType_SetGUID(output_type, &MF_MT_MAJOR_TYPE, decoder_descs[decoder->type].major_type))) + { + IMFMediaType_Release(output_type); + return hr; + } + + if (FAILED(hr = IMFMediaType_SetGUID(output_type, &MF_MT_SUBTYPE, decoder_descs[decoder->type].output_types[index]))) + { + IMFMediaType_Release(output_type); + return hr; + } + + *type = output_type; + + return S_OK; } static HRESULT WINAPI mf_decoder_SetInputType(IMFTransform *iface, DWORD id, IMFMediaType *type, DWORD flags) -- 2.30.1
On 3/10/21 1:33 PM, Derek Lesho wrote:
Since our source pad is not part of any element, gstreamer won't end up activating it directly through the state transition. Instead, if the downstream element doesn't activate the source pad into pull mode during the transition to the READY state, we activate our pad in push mode.
Signed-off-by: Derek Lesho <dlesho(a)codeweavers.com> --- dlls/winegstreamer/wg_parser.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c index b8dd75d5fff..67f65264a8d 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -58,7 +58,7 @@ struct wg_parser pthread_mutex_t mutex;
pthread_cond_t init_cond; - bool no_more_pads, has_duration, error; + bool no_more_pads, has_duration, error, pull_mode;
pthread_cond_t read_cond, read_done_cond; struct @@ -1331,9 +1331,12 @@ static gboolean src_activate_mode_cb(GstPad *pad, GstObject *parent, GstPadMode GST_DEBUG("%s source pad for parser %p in %s mode.", activate ? "Activating" : "Deactivating", parser, gst_pad_mode_get_name(mode));
+ parser->pull_mode = false; +
I think this statement is redundant—the field will already be initialized to zero, and I don't think we should be activated in pull-mode directly after being activated in push-mode.
switch (mode) { case GST_PAD_MODE_PULL: + parser->pull_mode = activate; return TRUE; case GST_PAD_MODE_PUSH: return activate_push(pad, activate); @@ -1573,6 +1576,8 @@ static void CDECL wg_parser_disconnect(struct wg_parser *parser) pthread_mutex_unlock(&parser->mutex);
gst_element_set_state(parser->container, GST_STATE_NULL); + if (!parser->pull_mode) + gst_pad_set_active(parser->my_src, 0); gst_pad_unlink(parser->my_src, parser->their_sink); gst_object_unref(parser->my_src); gst_object_unref(parser->their_sink); @@ -1628,6 +1633,8 @@ static BOOL decodebin_parser_init_gst(struct wg_parser *parser) }
gst_element_set_state(parser->container, GST_STATE_PAUSED); + if (!parser->pull_mode) + gst_pad_set_active(parser->my_src, 1); ret = gst_element_get_state(parser->container, NULL, NULL, -1); if (ret == GST_STATE_CHANGE_FAILURE) {
I think this and the following should happen after the gst_element_get_state() call, right? We shouldn't spawn a thread if state change fails. In theory that might risk a deadlock—i.e. GStreamer doesn't make guarantees that NULL->READY->PAUSED can happen without any buffers being pushed—but I don't know if it happens in practice. The really correct solution to that deadlock is to create our own element (or, actually, use appsrc—now that winegstreamer is split into PE and Unix parts, that option is starting to look more attractive, especially if we want to make winegstreamer compatible with WoW64 emulation.)
@@ -1679,6 +1686,8 @@ static BOOL avi_parser_init_gst(struct wg_parser *parser) }
gst_element_set_state(parser->container, GST_STATE_PAUSED); + if (!parser->pull_mode) + gst_pad_set_active(parser->my_src, 1); ret = gst_element_get_state(parser->container, NULL, NULL, -1); if (ret == GST_STATE_CHANGE_FAILURE) { @@ -1733,6 +1742,8 @@ static BOOL mpeg_audio_parser_init_gst(struct wg_parser *parser)
gst_pad_set_active(stream->my_sink, 1); gst_element_set_state(parser->container, GST_STATE_PAUSED); + if (!parser->pull_mode) + gst_pad_set_active(parser->my_src, 1); ret = gst_element_get_state(parser->container, NULL, NULL, -1); if (ret == GST_STATE_CHANGE_FAILURE) { @@ -1787,6 +1798,8 @@ static BOOL wave_parser_init_gst(struct wg_parser *parser)
gst_pad_set_active(stream->my_sink, 1); gst_element_set_state(parser->container, GST_STATE_PAUSED); + if (!parser->pull_mode) + gst_pad_set_active(parser->my_src, 1); ret = gst_element_get_state(parser->container, NULL, NULL, -1); if (ret == GST_STATE_CHANGE_FAILURE) {
On 3/11/21 3:15 PM, Zebediah Figura (she/her) wrote:
On 3/10/21 1:33 PM, Derek Lesho wrote:
Since our source pad is not part of any element, gstreamer won't end up activating it directly through the state transition. Instead, if the downstream element doesn't activate the source pad into pull mode during the transition to the READY state, we activate our pad in push mode.
Signed-off-by: Derek Lesho <dlesho(a)codeweavers.com> --- dlls/winegstreamer/wg_parser.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c index b8dd75d5fff..67f65264a8d 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -58,7 +58,7 @@ struct wg_parser pthread_mutex_t mutex;
pthread_cond_t init_cond; - bool no_more_pads, has_duration, error; + bool no_more_pads, has_duration, error, pull_mode;
pthread_cond_t read_cond, read_done_cond; struct @@ -1331,9 +1331,12 @@ static gboolean src_activate_mode_cb(GstPad *pad, GstObject *parent, GstPadMode GST_DEBUG("%s source pad for parser %p in %s mode.", activate ? "Activating" : "Deactivating", parser, gst_pad_mode_get_name(mode));
+ parser->pull_mode = false; + I think this statement is redundant—the field will already be initialized to zero, and I don't think we should be activated in pull-mode directly after being activated in push-mode. Makes sense.
switch (mode) { case GST_PAD_MODE_PULL: + parser->pull_mode = activate; return TRUE; case GST_PAD_MODE_PUSH: return activate_push(pad, activate); @@ -1573,6 +1576,8 @@ static void CDECL wg_parser_disconnect(struct wg_parser *parser) pthread_mutex_unlock(&parser->mutex);
gst_element_set_state(parser->container, GST_STATE_NULL); + if (!parser->pull_mode) + gst_pad_set_active(parser->my_src, 0); gst_pad_unlink(parser->my_src, parser->their_sink); gst_object_unref(parser->my_src); gst_object_unref(parser->their_sink); @@ -1628,6 +1633,8 @@ static BOOL decodebin_parser_init_gst(struct wg_parser *parser) }
gst_element_set_state(parser->container, GST_STATE_PAUSED); + if (!parser->pull_mode) + gst_pad_set_active(parser->my_src, 1); ret = gst_element_get_state(parser->container, NULL, NULL, -1); if (ret == GST_STATE_CHANGE_FAILURE) {
I think this and the following should happen after the gst_element_get_state() call, right? We shouldn't spawn a thread if state change fails.
In theory that might risk a deadlock—i.e. GStreamer doesn't make guarantees that NULL->READY->PAUSED can happen without any buffers being pushed—but I don't know if it happens in practice.
Yeah, it does deadlock and I think Gstreamer specifies that the PAUSED state indicates that pipeline is prerolled. https://gstreamer.freedesktop.org/documentation/plugin-development/basics/st... "GST_STATE_PAUSED is ... At this point the pipeline is 'prerolled' and ready to render data immediately." For what it's worth, it isn't guaranteed (although in practice it seems to be the case) that even the transition from NULL to READY is completely synchronous. The most correct code would involve first transitioning to READY, checking the result, activating the pad, then continuing to transition to PAUSED.
The really correct solution to that deadlock is to create our own element (or, actually, use appsrc—now that winegstreamer is split into PE and Unix parts Yeah, I still believe GstAppSrc is the cleaner solution, as the interface is quite simple and well documented. But functionally, the manual pad mode should work just as well, it's up to you. , that option is starting to look more attractive, especially if we want to make winegstreamer compatible with WoW64 emulation.) I'm not sure I see how this is relevant to the WoW64 emulation, if you don't mind, could you explain this?
@@ -1679,6 +1686,8 @@ static BOOL avi_parser_init_gst(struct wg_parser *parser) }
gst_element_set_state(parser->container, GST_STATE_PAUSED); + if (!parser->pull_mode) + gst_pad_set_active(parser->my_src, 1); ret = gst_element_get_state(parser->container, NULL, NULL, -1); if (ret == GST_STATE_CHANGE_FAILURE) { @@ -1733,6 +1742,8 @@ static BOOL mpeg_audio_parser_init_gst(struct wg_parser *parser)
gst_pad_set_active(stream->my_sink, 1); gst_element_set_state(parser->container, GST_STATE_PAUSED); + if (!parser->pull_mode) + gst_pad_set_active(parser->my_src, 1); ret = gst_element_get_state(parser->container, NULL, NULL, -1); if (ret == GST_STATE_CHANGE_FAILURE) { @@ -1787,6 +1798,8 @@ static BOOL wave_parser_init_gst(struct wg_parser *parser)
gst_pad_set_active(stream->my_sink, 1); gst_element_set_state(parser->container, GST_STATE_PAUSED); + if (!parser->pull_mode) + gst_pad_set_active(parser->my_src, 1); ret = gst_element_get_state(parser->container, NULL, NULL, -1); if (ret == GST_STATE_CHANGE_FAILURE) {
On 3/11/21 2:29 PM, Derek Lesho wrote:
On 3/11/21 3:15 PM, Zebediah Figura (she/her) wrote:
On 3/10/21 1:33 PM, Derek Lesho wrote:
Since our source pad is not part of any element, gstreamer won't end up activating it directly through the state transition. Instead, if the downstream element doesn't activate the source pad into pull mode during the transition to the READY state, we activate our pad in push mode.
Signed-off-by: Derek Lesho <dlesho(a)codeweavers.com> --- dlls/winegstreamer/wg_parser.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c index b8dd75d5fff..67f65264a8d 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -58,7 +58,7 @@ struct wg_parser pthread_mutex_t mutex; pthread_cond_t init_cond; - bool no_more_pads, has_duration, error; + bool no_more_pads, has_duration, error, pull_mode; pthread_cond_t read_cond, read_done_cond; struct @@ -1331,9 +1331,12 @@ static gboolean src_activate_mode_cb(GstPad *pad, GstObject *parent, GstPadMode GST_DEBUG("%s source pad for parser %p in %s mode.", activate ? "Activating" : "Deactivating", parser, gst_pad_mode_get_name(mode)); + parser->pull_mode = false; + I think this statement is redundant—the field will already be initialized to zero, and I don't think we should be activated in pull-mode directly after being activated in push-mode. Makes sense.
switch (mode) { case GST_PAD_MODE_PULL: + parser->pull_mode = activate; return TRUE; case GST_PAD_MODE_PUSH: return activate_push(pad, activate); @@ -1573,6 +1576,8 @@ static void CDECL wg_parser_disconnect(struct wg_parser *parser) pthread_mutex_unlock(&parser->mutex); gst_element_set_state(parser->container, GST_STATE_NULL); + if (!parser->pull_mode) + gst_pad_set_active(parser->my_src, 0); gst_pad_unlink(parser->my_src, parser->their_sink); gst_object_unref(parser->my_src); gst_object_unref(parser->their_sink); @@ -1628,6 +1633,8 @@ static BOOL decodebin_parser_init_gst(struct wg_parser *parser) } gst_element_set_state(parser->container, GST_STATE_PAUSED); + if (!parser->pull_mode) + gst_pad_set_active(parser->my_src, 1); ret = gst_element_get_state(parser->container, NULL, NULL, -1); if (ret == GST_STATE_CHANGE_FAILURE) { I think this and the following should happen after the gst_element_get_state() call, right? We shouldn't spawn a thread if state change fails.
In theory that might risk a deadlock—i.e. GStreamer doesn't make guarantees that NULL->READY->PAUSED can happen without any buffers being pushed—but I don't know if it happens in practice.
Yeah, it does deadlock and I think Gstreamer specifies that the PAUSED state indicates that pipeline is prerolled.
https://gstreamer.freedesktop.org/documentation/plugin-development/basics/st...
"GST_STATE_PAUSED is ... At this point the pipeline is 'prerolled' and ready to render data immediately."
That's assuming that the sink prerolls—which most sinks do, but we don't.
For what it's worth, it isn't guaranteed (although in practice it seems to be the case) that even the transition from NULL to READY is completely synchronous. The most correct code would involve first transitioning to READY, checking the result, activating the pad, then continuing to transition to PAUSED.
Unfortunately it's not quite that simple. Pads are activated on READY->PAUSED, not on NULL->READY, and upstream pads have to be activated *after* downstream ones.
The really correct solution to that deadlock is to create our own element (or, actually, use appsrc—now that winegstreamer is split into PE and Unix parts Yeah, I still believe GstAppSrc is the cleaner solution, as the interface is quite simple and well documented. But functionally, the manual pad mode should work just as well, it's up to you.
I don't remember what discussion we had about appsrc in the first place (if we had any?), but I think my argument was that it wasn't really a *better* fit than using getrange callbacks directly, i.e. it wasn't saving us much work (at best it would have saved us the need to separately handle push mode, but it was better to simply not implement push mode in the media source). But that's less true now, given the way that read requests are handled at the Unix library boundary, and the combined backend interface for DirectShow and Media Foundation, and I think appsrc may be a better fit now. The "need-data"/"push-data" model looks like a better fit, that might allow us to avoid the current awkward callback marshalling, and we'd be able to get rid of push-mode handling completely.
, that option is starting to look more attractive, especially if we want to make winegstreamer compatible with WoW64 emulation.) I'm not sure I see how this is relevant to the WoW64 emulation, if you don't mind, could you explain this?
One reason that appsrc is undesirable (which I probably didn't mention earlier) is that it always takes buffers allocated by the "app", as it were, rather than by the element calling gst_pad_get_range(). That's fine if the downstream element didn't allocate its own buffer, but if it did (and oggdemux is one major example of an element that does) appsrc will do an extra blit. That should be avoided if possible. I'm not particularly concerned about latency—I don't think it should be a bottleneck, as long as the machine has enough cores—but I am more concerned about CPU usage, and cases where we might be CPU limited and *don't* have enough cores. Yes, it's probably premature optimization, but I'm a lot warier about removing an optimization already present than I am about introducing a new one. Currently we fill read buffers allocated by the Unix side—either already allocated by a GStreamer element, or allocated by us using gst_buffer_new_and_alloc(). That's not compatible with WoW64 emulation (aka 32-on-64-bit support), as it requires the 32-bit PE side to write into a 64-bit pointer. Instead we would need to write into a buffer allocated from the 32-bit side, and pass that to the Unix side. But this also means that we have to do a blit. [Note that we can in theory avoid the blit by wrapping the caller-allocated buffer in a custom GstBuffer class and passing that down to the Unix side. We still have to do a blit if reading into a downstream-allocated GstBuffer, but we can at least avoid it otherwise.]
@@ -1679,6 +1686,8 @@ static BOOL avi_parser_init_gst(struct wg_parser *parser) } gst_element_set_state(parser->container, GST_STATE_PAUSED); + if (!parser->pull_mode) + gst_pad_set_active(parser->my_src, 1); ret = gst_element_get_state(parser->container, NULL, NULL, -1); if (ret == GST_STATE_CHANGE_FAILURE) { @@ -1733,6 +1742,8 @@ static BOOL mpeg_audio_parser_init_gst(struct wg_parser *parser) gst_pad_set_active(stream->my_sink, 1); gst_element_set_state(parser->container, GST_STATE_PAUSED); + if (!parser->pull_mode) + gst_pad_set_active(parser->my_src, 1); ret = gst_element_get_state(parser->container, NULL, NULL, -1); if (ret == GST_STATE_CHANGE_FAILURE) { @@ -1787,6 +1798,8 @@ static BOOL wave_parser_init_gst(struct wg_parser *parser) gst_pad_set_active(stream->my_sink, 1); gst_element_set_state(parser->container, GST_STATE_PAUSED); + if (!parser->pull_mode) + gst_pad_set_active(parser->my_src, 1); ret = gst_element_get_state(parser->container, NULL, NULL, -1); if (ret == GST_STATE_CHANGE_FAILURE) {
On 3/11/21 4:06 PM, Zebediah Figura (she/her) wrote:
The really correct solution to that deadlock is to create our own element (or, actually, use appsrc—now that winegstreamer is split into PE and Unix parts Yeah, I still believe GstAppSrc is the cleaner solution, as the interface is quite simple and well documented. But functionally, the manual pad mode should work just as well, it's up to you. I don't remember what discussion we had about appsrc in the first place (if we had any?), but I think my argument was that it wasn't really a *better* fit than using getrange callbacks directly, i.e. it wasn't saving us much work (at best it would have saved us the need to separately handle push mode, but it was better to simply not implement push mode in the media source).
But that's less true now, given the way that read requests are handled at the Unix library boundary, and the combined backend interface for DirectShow and Media Foundation, and I think appsrc may be a better fit now. The "need-data"/"push-data" model looks like a better fit, that might allow us to avoid the current awkward callback marshalling, and we'd be able to get rid of push-mode handling completely.
, that option is starting to look more attractive, especially if we want to make winegstreamer compatible with WoW64 emulation.) I'm not sure I see how this is relevant to the WoW64 emulation, if you don't mind, could you explain this? One reason that appsrc is undesirable (which I probably didn't mention earlier) is that it always takes buffers allocated by the "app", as it were, rather than by the element calling gst_pad_get_range(). That's fine if the downstream element didn't allocate its own buffer, but if it did (and oggdemux is one major example of an element that does) appsrc will do an extra blit.
That should be avoided if possible. I'm not particularly concerned about latency—I don't think it should be a bottleneck, as long as the machine has enough cores—but I am more concerned about CPU usage, and cases where we might be CPU limited and *don't* have enough cores. Yes, it's probably premature optimization, but I'm a lot warier about removing an optimization already present than I am about introducing a new one.
Currently we fill read buffers allocated by the Unix side—either already allocated by a GStreamer element, or allocated by us using gst_buffer_new_and_alloc(). That's not compatible with WoW64 emulation (aka 32-on-64-bit support), as it requires the 32-bit PE side to write into a 64-bit pointer. Instead we would need to write into a buffer allocated from the 32-bit side, and pass that to the Unix side. But this also means that we have to do a blit.
[Note that we can in theory avoid the blit by wrapping the caller-allocated buffer in a custom GstBuffer class and passing that down to the Unix side. We still have to do a blit if reading into a downstream-allocated GstBuffer, but we can at least avoid it otherwise.]
Thanks for the comprehensive explanation; so for it's worth, the code I intended to submit as part of getting the decoder MFT to work contains some questionable code to generally preserve the current API where the PE side gets the buffer from the Unix side, with the pushing thread increasing the size of its buffer allocations to match the size of the buffer being pushed from the PE side. It also contained a fairly large amount of code handling flushes and EOS, which is most likely more error-prone than the GstAppSrc equivalent. If we intend to move to GstAppSrc, then there's no point in me trying to upstream this questionable code whose only reason for existing is to preserve a model which can't last anyway. Would you like me to take a shot at moving wg_parser to GstAppSrc?
On 3/11/21 3:33 PM, Derek Lesho wrote:
On 3/11/21 4:06 PM, Zebediah Figura (she/her) wrote:
The really correct solution to that deadlock is to create our own element (or, actually, use appsrc—now that winegstreamer is split into PE and Unix parts Yeah, I still believe GstAppSrc is the cleaner solution, as the interface is quite simple and well documented. But functionally, the manual pad mode should work just as well, it's up to you. I don't remember what discussion we had about appsrc in the first place (if we had any?), but I think my argument was that it wasn't really a *better* fit than using getrange callbacks directly, i.e. it wasn't saving us much work (at best it would have saved us the need to separately handle push mode, but it was better to simply not implement push mode in the media source).
But that's less true now, given the way that read requests are handled at the Unix library boundary, and the combined backend interface for DirectShow and Media Foundation, and I think appsrc may be a better fit now. The "need-data"/"push-data" model looks like a better fit, that might allow us to avoid the current awkward callback marshalling, and we'd be able to get rid of push-mode handling completely.
, that option is starting to look more attractive, especially if we want to make winegstreamer compatible with WoW64 emulation.) I'm not sure I see how this is relevant to the WoW64 emulation, if you don't mind, could you explain this? One reason that appsrc is undesirable (which I probably didn't mention earlier) is that it always takes buffers allocated by the "app", as it were, rather than by the element calling gst_pad_get_range(). That's fine if the downstream element didn't allocate its own buffer, but if it did (and oggdemux is one major example of an element that does) appsrc will do an extra blit.
That should be avoided if possible. I'm not particularly concerned about latency—I don't think it should be a bottleneck, as long as the machine has enough cores—but I am more concerned about CPU usage, and cases where we might be CPU limited and *don't* have enough cores. Yes, it's probably premature optimization, but I'm a lot warier about removing an optimization already present than I am about introducing a new one.
Currently we fill read buffers allocated by the Unix side—either already allocated by a GStreamer element, or allocated by us using gst_buffer_new_and_alloc(). That's not compatible with WoW64 emulation (aka 32-on-64-bit support), as it requires the 32-bit PE side to write into a 64-bit pointer. Instead we would need to write into a buffer allocated from the 32-bit side, and pass that to the Unix side. But this also means that we have to do a blit.
[Note that we can in theory avoid the blit by wrapping the caller-allocated buffer in a custom GstBuffer class and passing that down to the Unix side. We still have to do a blit if reading into a downstream-allocated GstBuffer, but we can at least avoid it otherwise.]
Thanks for the comprehensive explanation; so for it's worth, the code I intended to submit as part of getting the decoder MFT to work contains some questionable code to generally preserve the current API where the PE side gets the buffer from the Unix side, with the pushing thread increasing the size of its buffer allocations to match the size of the buffer being pushed from the PE side. It also contained a fairly large amount of code handling flushes and EOS, which is most likely more error-prone than the GstAppSrc equivalent.
If we intend to move to GstAppSrc, then there's no point in me trying to upstream this questionable code whose only reason for existing is to preserve a model which can't last anyway. Would you like me to take a shot at moving wg_parser to GstAppSrc?
I think that new wg_* objects should definitely use appsrc. I'm mostly inclined to say that the parser should as well; I'm still a little concerned about potential performance regression (and I'm not sure to what degree WoW64 emulation will end up actually being implemented in upstream Wine) but I'm leaning towards it being a good idea. That shouldn't actually block work on the decoder, but it may help give a more coherent picture of the unixlib interface. In any case I would definitely appreciate your work on that conversion. I think we should probably leave off the aforementioned buffer wrapping optimization, at least for an initial pass, and then evaluate the actual cost of blitting. It may turn out to be negligible.
participants (2)
-
Derek Lesho -
Zebediah Figura (she/her)