[PATCH 1/5] winegstreamer: Link the container pads and activate ours.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51931 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52391 Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- This series introduces a new wg_sample structure, which is used to pass media buffers from PE to unix and back, as well as their metadata. Initially the buffers are copied to GStreamer buffer, but the last patch of the series introduces a zero-copy mechanism, which is used for input buffers only for now. Testing shows that IMFTransform are usually keeping a reference on their input samples, and this makes it easier to keep the IMFSample alive as long as the unix side has a reference on them, reaping them on specific occasion to retrieve released samples and release their PE-side objects. For output buffers, zero-copy is also possible, using the same structure and mapping mechanism, but is a little trickier as transforms aren't apparently keeping a reference on the output samples they are passed across calls to ProcessOutput. They can also optionally allocate and provide samples themselves to the caller, instead of depending on their samples, but it's not how native transforms are working, and to keep the behavior and compatibility close we cannot safely rely on it. The idea, which will be proposed later, and for the H264 transform (as video buffers may be large, it makes most sense there), will be to decouple GStreamer decoding using a queue, and a buffer pool which will wait on ProcessOutput calls to provide output buffers to the pipeline. dlls/winegstreamer/wg_transform.c | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index d316071cf60..7160fd087b6 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -46,6 +46,7 @@ struct wg_transform { GstElement *container; GstPad *my_src, *my_sink; + GstPad *their_sink, *their_src; }; static GstFlowReturn transform_sink_chain_cb(GstPad *pad, GstObject *parent, GstBuffer *buffer) @@ -64,6 +65,10 @@ NTSTATUS wg_transform_destroy(void *args) struct wg_transform *transform = args; gst_element_set_state(transform->container, GST_STATE_NULL); + gst_pad_unlink(transform->their_src, transform->my_sink); + gst_pad_unlink(transform->my_src, transform->their_sink); + g_object_unref(transform->their_sink); + g_object_unref(transform->their_src); g_object_unref(transform->container); g_object_unref(transform->my_sink); g_object_unref(transform->my_src); @@ -237,9 +242,22 @@ NTSTATUS wg_transform_create(void *args) goto out_free_sink_pad; } + if (!(transform->their_sink = gst_element_get_static_pad(first, "sink"))) + goto out_free_sink_pad; + if (!(transform->their_src = gst_element_get_static_pad(last, "src"))) + goto out_free_their_sink; + if (gst_pad_link(transform->my_src, transform->their_sink) < 0) + goto out_free_their_src; + if (gst_pad_link(transform->their_src, transform->my_sink) < 0) + goto out_unlink_src_pad; + if (!gst_pad_set_active(transform->my_sink, 1)) + goto out_unlink_sink_pad; + if (!gst_pad_set_active(transform->my_src, 1)) + goto out_unlink_sink_pad; + gst_element_set_state(transform->container, GST_STATE_PAUSED); if (!gst_element_get_state(transform->container, NULL, NULL, -1)) - goto out_free_sink_pad; + goto out_unlink_sink_pad; gst_caps_unref(sink_caps); gst_caps_unref(src_caps); @@ -248,6 +266,14 @@ NTSTATUS wg_transform_create(void *args) params->transform = transform; return STATUS_SUCCESS; +out_unlink_sink_pad: + gst_pad_unlink(transform->their_src, transform->my_sink); +out_unlink_src_pad: + gst_pad_unlink(transform->my_src, transform->their_sink); +out_free_their_src: + g_object_unref(transform->their_src); +out_free_their_sink: + g_object_unref(transform->their_sink); out_free_sink_pad: gst_object_unref(transform->my_sink); out_free_sink_caps: -- 2.34.1
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51931 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52391 Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/wg_transform.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 7160fd087b6..a4a4d886865 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -47,6 +47,7 @@ struct wg_transform GstElement *container; GstPad *my_src, *my_sink; GstPad *their_sink, *their_src; + GstSegment segment; }; static GstFlowReturn transform_sink_chain_cb(GstPad *pad, GstObject *parent, GstBuffer *buffer) @@ -156,6 +157,7 @@ NTSTATUS wg_transform_create(void *args) GstPadTemplate *template = NULL; struct wg_transform *transform; const gchar *media_type; + GstEvent *event; if (!init_gstreamer()) return STATUS_UNSUCCESSFUL; @@ -259,6 +261,23 @@ NTSTATUS wg_transform_create(void *args) if (!gst_element_get_state(transform->container, NULL, NULL, -1)) goto out_unlink_sink_pad; + if (!(event = gst_event_new_stream_start("stream")) + || !gst_pad_push_event(transform->my_src, event)) + goto out_unlink_sink_pad; + if (!(event = gst_event_new_caps(src_caps)) + || !gst_pad_push_event(transform->my_src, event)) + goto out_unlink_sink_pad; + + /* We need to use GST_FORMAT_TIME here because it's the only format + * some elements such avdec_wmav2 correctly support. + */ + gst_segment_init(&transform->segment, GST_FORMAT_TIME); + transform->segment.start = 0; + transform->segment.stop = -1; + if (!(event = gst_event_new_segment(&transform->segment)) + || !gst_pad_push_event(transform->my_src, event)) + goto out_unlink_sink_pad; + gst_caps_unref(sink_caps); gst_caps_unref(src_caps); @@ -283,6 +302,7 @@ out_free_src_pad: out_free_src_caps: gst_caps_unref(src_caps); out_free_container: + gst_element_set_state(transform->container, GST_STATE_NULL); gst_object_unref(transform->container); out_free_transform: free(transform); -- 2.34.1
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=108993 Your paranoid android. === debian11 (build log) === error: patch failed: dlls/winegstreamer/wg_transform.c:46 error: patch failed: dlls/winegstreamer/wg_transform.c:47 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/winegstreamer/wg_transform.c:46 error: patch failed: dlls/winegstreamer/wg_transform.c:47 Task: Patch failed to apply
Simply keeping a reference on the input sample for now, wrapped in a new wg_sample struct. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51931 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52391 Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/mf/tests/mf.c | 6 ---- dlls/winegstreamer/gst_private.h | 3 ++ dlls/winegstreamer/mfplat.c | 59 ++++++++++++++++++++++++++++++++ dlls/winegstreamer/unixlib.h | 8 +++++ dlls/winegstreamer/wma_decoder.c | 37 ++++++++++++++++++-- 5 files changed, 105 insertions(+), 8 deletions(-) diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c index 60d95529f97..be02a0909e8 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -5996,25 +5996,20 @@ static void test_wma_decoder(void) sample = create_sample(wma_encoded_data, wma_block_size / 2); hr = IMFTransform_ProcessInput(transform, 0, sample, 0); - todo_wine ok(hr == S_OK, "ProcessInput returned %#x\n", hr); ret = IMFSample_Release(sample); ok(ret == 0, "Release returned %u\n", ret); sample = create_sample(wma_encoded_data + wma_block_size, wma_block_size - wma_block_size / 2); hr = IMFTransform_ProcessInput(transform, 0, sample, 0); - todo_wine ok(hr == S_OK, "ProcessInput returned %#x\n", hr); ret = IMFSample_Release(sample); ok(ret == 0, "Release returned %u\n", ret); sample = create_sample(wma_encoded_data, wma_block_size); hr = IMFTransform_ProcessInput(transform, 0, sample, 0); - todo_wine ok(hr == S_OK, "ProcessInput returned %#x\n", hr); hr = IMFTransform_ProcessInput(transform, 0, sample, 0); - todo_wine ok(hr == MF_E_NOTACCEPTING, "ProcessInput returned %#x\n", hr); ret = IMFSample_Release(sample); - todo_wine ok(ret == 1, "Release returned %u\n", ret); /* As output_info.dwFlags doesn't have MFT_OUTPUT_STREAM_CAN_PROVIDE_SAMPLES @@ -6037,7 +6032,6 @@ static void test_wma_decoder(void) sample = create_sample(wma_encoded_data, wma_block_size); hr = IMFTransform_ProcessInput(transform, 0, sample, 0); - todo_wine ok(hr == MF_E_NOTACCEPTING, "ProcessInput returned %#x\n", hr); ret = IMFSample_Release(sample); ok(ret == 0, "Release returned %u\n", ret); diff --git a/dlls/winegstreamer/gst_private.h b/dlls/winegstreamer/gst_private.h index a63daaf04b9..7ad3434fd1d 100644 --- a/dlls/winegstreamer/gst_private.h +++ b/dlls/winegstreamer/gst_private.h @@ -119,6 +119,9 @@ extern HRESULT mfplat_DllRegisterServer(void); IMFMediaType *mf_media_type_from_wg_format(const struct wg_format *format); void mf_media_type_to_wg_format(IMFMediaType *type, struct wg_format *format); +HRESULT mf_create_wg_sample(IMFSample *sample, struct wg_sample **out); +void mf_destroy_wg_sample(struct wg_sample *wg_sample); + HRESULT winegstreamer_stream_handler_create(REFIID riid, void **obj); HRESULT audio_converter_create(REFIID riid, void **ret); diff --git a/dlls/winegstreamer/mfplat.c b/dlls/winegstreamer/mfplat.c index 9b3fc429d32..2ba1490f20b 100644 --- a/dlls/winegstreamer/mfplat.c +++ b/dlls/winegstreamer/mfplat.c @@ -827,3 +827,62 @@ void mf_media_type_to_wg_format(IMFMediaType *type, struct wg_format *format) else FIXME("Unrecognized major type %s.\n", debugstr_guid(&major_type)); } + +HRESULT mf_create_wg_sample(IMFSample *sample, struct wg_sample **out) +{ + DWORD current_length, max_length, count; + IMFMediaBuffer *media_buffer = NULL; + struct wg_sample *entry; + BYTE *buffer; + HRESULT hr; + + if (FAILED(hr = IMFSample_GetBufferCount(sample, &count)) || count != 1) + { + FIXME("Only samples with 1 buffer are supported!\n"); + return hr ? hr : E_FAIL; + } + + if (!(entry = calloc(1, sizeof(*entry)))) + return E_OUTOFMEMORY; + + if (FAILED(hr = IMFSample_GetBufferByIndex(sample, 0, &media_buffer))) + goto failed; + if (FAILED(hr = IMFMediaBuffer_Lock(media_buffer, &buffer, &max_length, ¤t_length))) + goto failed; + + IMFSample_AddRef((entry->private = sample)); + entry->data = buffer; + entry->size = current_length; + entry->max_size = max_length; + + TRACE("Created sample entry %p.\n", entry); + *out = entry; + return S_OK; + +failed: + if (media_buffer) + IMFMediaBuffer_Release(media_buffer); + free(entry); + return hr; +} + +void mf_destroy_wg_sample(struct wg_sample *sample) +{ + IMFMediaBuffer *media_buffer; + HRESULT hr; + + if (FAILED(hr = IMFSample_GetBufferByIndex(sample->private, 0, &media_buffer))) + WARN("Failed to get first buffer, sample %p\n", sample->private); + else + { + /* release lock and ref taken in mf_create_wg_sample */ + IMFMediaBuffer_Unlock(media_buffer); + IMFMediaBuffer_SetCurrentLength(media_buffer, sample->size); + IMFMediaBuffer_Release(media_buffer); + + IMFMediaBuffer_Release(media_buffer); + } + + IMFSample_Release(sample->private); + free(sample); +} diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index 4adbb694766..59b15df7d08 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -103,6 +103,14 @@ struct wg_format } u; }; +struct wg_sample +{ + UINT32 max_size; + UINT32 size; + BYTE *data; + void *private; +}; + enum wg_parser_event_type { WG_PARSER_EVENT_NONE = 0, diff --git a/dlls/winegstreamer/wma_decoder.c b/dlls/winegstreamer/wma_decoder.c index 6c198706944..f84faad669d 100644 --- a/dlls/winegstreamer/wma_decoder.c +++ b/dlls/winegstreamer/wma_decoder.c @@ -55,6 +55,7 @@ struct wma_decoder IMFMediaType *output_type; struct wg_transform *wg_transform; + struct wg_sample *input_sample; }; static inline struct wma_decoder *impl_from_IUnknown(IUnknown *iface) @@ -134,6 +135,8 @@ static ULONG WINAPI unknown_Release(IUnknown *iface) IMFMediaType_Release(decoder->input_type); if (decoder->output_type) IMFMediaType_Release(decoder->output_type); + if (decoder->input_sample) + mf_destroy_wg_sample(decoder->input_sample); free(decoder); } @@ -520,8 +523,38 @@ static HRESULT WINAPI transform_ProcessMessage(IMFTransform *iface, MFT_MESSAGE_ static HRESULT WINAPI transform_ProcessInput(IMFTransform *iface, DWORD id, IMFSample *sample, DWORD flags) { - FIXME("iface %p, id %lu, sample %p, flags %#lx stub!\n", iface, id, sample, flags); - return E_NOTIMPL; + struct wma_decoder *decoder = impl_from_IMFTransform(iface); + IMFMediaBuffer *media_buffer; + struct wg_sample *wg_sample; + MFT_INPUT_STREAM_INFO info; + HRESULT hr; + + TRACE("iface %p, id %lu, sample %p, flags %#lx.\n", iface, id, sample, flags); + + if (FAILED(hr = IMFTransform_GetInputStreamInfo(iface, 0, &info))) + return hr; + + if (!decoder->wg_transform) + return MF_E_TRANSFORM_TYPE_NOT_SET; + + if (decoder->input_sample) + return MF_E_NOTACCEPTING; + + if (FAILED(hr = IMFSample_ConvertToContiguousBuffer(sample, &media_buffer))) + return hr; + IMFMediaBuffer_Release(media_buffer); + + if (FAILED(hr = mf_create_wg_sample(sample, &wg_sample))) + return hr; + + if (!(wg_sample->size = (wg_sample->size / info.cbSize) * info.cbSize)) + { + mf_destroy_wg_sample(wg_sample); + return S_OK; + } + + decoder->input_sample = wg_sample; + return S_OK; } static HRESULT WINAPI transform_ProcessOutput(IMFTransform *iface, DWORD flags, DWORD count, -- 2.34.1
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=108994 Your paranoid android. === build (build log) === error: patch failed: dlls/winegstreamer/wg_transform.c:46 error: patch failed: dlls/winegstreamer/wg_transform.c:47 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/winegstreamer/wg_transform.c:46 error: patch failed: dlls/winegstreamer/wg_transform.c:47 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/winegstreamer/wg_transform.c:46 error: patch failed: dlls/winegstreamer/wg_transform.c:47 Task: Patch failed to apply
Much of this patch doesn't make sense without 4/5. Could the two patches be squashed together? On 2/22/22 16:48, Rémi Bernon wrote:
+ if (FAILED(hr = IMFSample_GetBufferCount(sample, &count)) || count != 1) + { + FIXME("Only samples with 1 buffer are supported!\n"); + return hr ? hr : E_FAIL; + }
This made me do a double take. Since these conditions don't really share their bodies, perhaps they should be separated?
+void mf_destroy_wg_sample(struct wg_sample *sample) +{ + IMFMediaBuffer *media_buffer; + HRESULT hr; + + if (FAILED(hr = IMFSample_GetBufferByIndex(sample->private, 0, &media_buffer))) + WARN("Failed to get first buffer, sample %p\n", sample->private); + else + { + /* release lock and ref taken in mf_create_wg_sample */ + IMFMediaBuffer_Unlock(media_buffer); + IMFMediaBuffer_SetCurrentLength(media_buffer, sample->size); + IMFMediaBuffer_Release(media_buffer);
Why are we holding a reference to the media buffer? (And if we need to, should we be storing that instead of the IMFSample?)
+ + IMFMediaBuffer_Release(media_buffer); + } + + IMFSample_Release(sample->private); + free(sample); +} diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index 4adbb694766..59b15df7d08 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -103,6 +103,14 @@ struct wg_format } u; };
+struct wg_sample +{ + UINT32 max_size; + UINT32 size; + BYTE *data; + void *private; +};
"max_size" isn't relevant until patch 5/5. "private" also doesn't particularly seem like it belongs here. In particular, if you're going to wrap the wg_sample in another structure in patch 5/5, I'd rather see the same approach taken for frontend-specific data.
+ if (FAILED(hr = IMFSample_ConvertToContiguousBuffer(sample, &media_buffer))) + return hr; + IMFMediaBuffer_Release(media_buffer);
This seems pointless (and hence, at least, doesn't belong in this patch). Or does ConvertToContiguousBuffer() also modify the original sample?
+ if (!(wg_sample->size = (wg_sample->size / info.cbSize) * info.cbSize)) + { + mf_destroy_wg_sample(wg_sample); + return S_OK; + }
I can't tell if this is trying to align to info.cbSize or (as the documentation specifies) treat it as a minimum, but either way I think it's broken.
+ + decoder->input_sample = wg_sample;
Do we have to store this for API compatibility? If so it would seem helpful to specify that in a comment. It looks wrong as-is, since we don't need the data after we copy it. (And depending on how hard we need to preserve API compatibility in this area, I'm concerned that patch 5/5 may be making assumptions about how GStreamer uses buffers that aren't actually guaranteed.)
On 2/23/22 03:09, Zebediah Figura (she/her) wrote:
Much of this patch doesn't make sense without 4/5. Could the two patches be squashed together?
On 2/22/22 16:48, Rémi Bernon wrote:
+ if (FAILED(hr = IMFSample_GetBufferCount(sample, &count)) || count != 1) + { + FIXME("Only samples with 1 buffer are supported!\n"); + return hr ? hr : E_FAIL; + }
This made me do a double take. Since these conditions don't really share their bodies, perhaps they should be separated?
Sure, I wanted to keep it short as it's not supposed to happen anyway.
+void mf_destroy_wg_sample(struct wg_sample *sample) +{ + IMFMediaBuffer *media_buffer; + HRESULT hr; + + if (FAILED(hr = IMFSample_GetBufferByIndex(sample->private, 0, &media_buffer))) + WARN("Failed to get first buffer, sample %p\n", sample->private); + else + { + /* release lock and ref taken in mf_create_wg_sample */ + IMFMediaBuffer_Unlock(media_buffer); + IMFMediaBuffer_SetCurrentLength(media_buffer, sample->size); + IMFMediaBuffer_Release(media_buffer);
Why are we holding a reference to the media buffer? (And if we need to, should we be storing that instead of the IMFSample?)
I think we have to keep a reference on the buffer, because Lock doesn't, and because we keep the buffer data pointer around. It's probably unnecessary, and if someone modifies the sample while we're holding it, removing or adding buffers, it'll all probably crash but it felt more correct to keep a buffer reference. We'll need the pointer to the IMFSample later to be able to update its properties, such as timestamp and durations. As I used a single private pointer I had to chose which one to store, but I later added a wrapper struct so maybe I can put the pointers there instead.
+ + IMFMediaBuffer_Release(media_buffer); + } + + IMFSample_Release(sample->private); + free(sample); +} diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index 4adbb694766..59b15df7d08 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -103,6 +103,14 @@ struct wg_format } u; };
+struct wg_sample +{ + UINT32 max_size; + UINT32 size; + BYTE *data; + void *private; +};
"max_size" isn't relevant until patch 5/5.
Sure.
"private" also doesn't particularly seem like it belongs here. In particular, if you're going to wrap the wg_sample in another structure in patch 5/5, I'd rather see the same approach taken for frontend-specific data.
I used it on the unix side when re-using the struct for internal sample passing, but I guess I can also wrap it there, it'll probably make things cleaner.
+ if (FAILED(hr = IMFSample_ConvertToContiguousBuffer(sample, &media_buffer))) + return hr; + IMFMediaBuffer_Release(media_buffer);
This seems pointless (and hence, at least, doesn't belong in this patch). Or does ConvertToContiguousBuffer() also modify the original sample?
It does, and it makes sure there's a single buffer in the sample, as wg_sample_create only support that case.
+ if (!(wg_sample->size = (wg_sample->size / info.cbSize) * info.cbSize)) + { + mf_destroy_wg_sample(wg_sample); + return S_OK; + }
I can't tell if this is trying to align to info.cbSize or (as the documentation specifies) treat it as a minimum, but either way I think it's broken.
Testing shows that pushing buffers that aren't multiple of the input sample size truncates the data to the nearest sample size, and if the sample size ends up being 0, just returns S_OK and do nothing. This is checked by some of the mf tests.
+ + decoder->input_sample = wg_sample;
Do we have to store this for API compatibility? If so it would seem helpful to specify that in a comment. It looks wrong as-is, since we don't need the data after we copy it.
We do, for the tests to pass at least.
(And depending on how hard we need to preserve API compatibility in this area, I'm concerned that patch 5/5 may be making assumptions about how GStreamer uses buffers that aren't actually guaranteed.)
I'm not sure what your concerns are exactly? -- Rémi Bernon <rbernon(a)codeweavers.com>
On 2/23/22 02:32, Rémi Bernon wrote:
Why are we holding a reference to the media buffer? (And if we need to, should we be storing that instead of the IMFSample?)
I think we have to keep a reference on the buffer, because Lock doesn't, and because we keep the buffer data pointer around.
It's probably unnecessary, and if someone modifies the sample while we're holding it, removing or adding buffers, it'll all probably crash but it felt more correct to keep a buffer reference.
I guess so. It does look like the sample object is supposed to keep a reference to its own buffers, but I guess someone could manhandle the buffers while we're using them. On the other hand, if we care about that, we're currently keeping a reference to one buffer but releasing a reference to what could be another, which doesn't look right by any definition.
We'll need the pointer to the IMFSample later to be able to update its properties, such as timestamp and durations.
Okay, makes sense.
As I used a single private pointer I had to chose which one to store, but I later added a wrapper struct so maybe I can put the pointers there instead.
+ + IMFMediaBuffer_Release(media_buffer); + } + + IMFSample_Release(sample->private); + free(sample); +} diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index 4adbb694766..59b15df7d08 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -103,6 +103,14 @@ struct wg_format } u; }; +struct wg_sample +{ + UINT32 max_size; + UINT32 size; + BYTE *data; + void *private; +};
"max_size" isn't relevant until patch 5/5.
Sure.
"private" also doesn't particularly seem like it belongs here. In particular, if you're going to wrap the wg_sample in another structure in patch 5/5, I'd rather see the same approach taken for frontend-specific data.
I used it on the unix side when re-using the struct for internal sample passing, but I guess I can also wrap it there, it'll probably make things cleaner.
+ if (FAILED(hr = IMFSample_ConvertToContiguousBuffer(sample, &media_buffer))) + return hr; + IMFMediaBuffer_Release(media_buffer);
This seems pointless (and hence, at least, doesn't belong in this patch). Or does ConvertToContiguousBuffer() also modify the original sample?
It does, and it makes sure there's a single buffer in the sample, as wg_sample_create only support that case.
Ah, okay, I guess on reflection it would have to. FWIW, it seems it's legal to call that function with a NULL argument.
+ if (!(wg_sample->size = (wg_sample->size / info.cbSize) * info.cbSize)) + { + mf_destroy_wg_sample(wg_sample); + return S_OK; + }
I can't tell if this is trying to align to info.cbSize or (as the documentation specifies) treat it as a minimum, but either way I think it's broken.
Testing shows that pushing buffers that aren't multiple of the input sample size truncates the data to the nearest sample size, and if the sample size ends up being 0, just returns S_OK and do nothing.
This is checked by some of the mf tests.
Ah, okay. I was about to ask you to add a comment for that, but I see you've already changed the code in v2 :-)
+ + decoder->input_sample = wg_sample;
Do we have to store this for API compatibility? If so it would seem helpful to specify that in a comment. It looks wrong as-is, since we don't need the data after we copy it.
We do, for the tests to pass at least.
Right, but it's not clear to me how important it is that we have those tests, or that they pass.
(And depending on how hard we need to preserve API compatibility in this area, I'm concerned that patch 5/5 may be making assumptions about how GStreamer uses buffers that aren't actually guaranteed.)
I'm not sure what your concerns are exactly?
Well, GStreamer isn't actually guaranteeing that we'll get one sample output synchronously per sample input, or that the input sample will be released synchronously, or even that it'll be reused in-place. If applications are depending on any of these, that'll be hard to follow, and harder if we let GStreamer dictate when the input IMFSample is released. I'm guessing it's not really that important anyway (and if GStreamer does behave differently, there's not much we can do in general...)
And use it in WMA decoder ProcessInput to push the input samples. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51931 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52391 Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/gst_private.h | 1 + dlls/winegstreamer/main.c | 11 +++++++++++ dlls/winegstreamer/unix_private.h | 1 + dlls/winegstreamer/unixlib.h | 9 +++++++++ dlls/winegstreamer/wg_parser.c | 2 ++ dlls/winegstreamer/wg_transform.c | 22 +++++++++++++++++++++- dlls/winegstreamer/wma_decoder.c | 6 ++++++ 7 files changed, 51 insertions(+), 1 deletion(-) diff --git a/dlls/winegstreamer/gst_private.h b/dlls/winegstreamer/gst_private.h index 7ad3434fd1d..f0774d53613 100644 --- a/dlls/winegstreamer/gst_private.h +++ b/dlls/winegstreamer/gst_private.h @@ -99,6 +99,7 @@ void wg_parser_stream_seek(struct wg_parser_stream *stream, double rate, struct wg_transform *wg_transform_create(const struct wg_format *input_format, const struct wg_format *output_format); void wg_transform_destroy(struct wg_transform *transform); +HRESULT wg_transform_push_data(struct wg_transform *transform, struct wg_sample *sample); unsigned int wg_format_get_max_size(const struct wg_format *format); diff --git a/dlls/winegstreamer/main.c b/dlls/winegstreamer/main.c index f85e9995525..a3a59e62b98 100644 --- a/dlls/winegstreamer/main.c +++ b/dlls/winegstreamer/main.c @@ -273,6 +273,17 @@ void wg_transform_destroy(struct wg_transform *transform) __wine_unix_call(unix_handle, unix_wg_transform_destroy, transform); } +HRESULT wg_transform_push_data(struct wg_transform *transform, struct wg_sample *sample) +{ + struct wg_transform_push_data_params params = + { + .transform = transform, + .sample = sample, + }; + + return __wine_unix_call(unix_handle, unix_wg_transform_push_data, ¶ms); +} + BOOL WINAPI DllMain(HINSTANCE instance, DWORD reason, void *reserved) { if (reason == DLL_PROCESS_ATTACH) diff --git a/dlls/winegstreamer/unix_private.h b/dlls/winegstreamer/unix_private.h index d3f32484ee6..4f197787baa 100644 --- a/dlls/winegstreamer/unix_private.h +++ b/dlls/winegstreamer/unix_private.h @@ -34,5 +34,6 @@ extern GstCaps *wg_format_to_caps(const struct wg_format *format) DECLSPEC_HIDDE extern NTSTATUS wg_transform_create(void *args) DECLSPEC_HIDDEN; extern NTSTATUS wg_transform_destroy(void *args) DECLSPEC_HIDDEN; +extern NTSTATUS wg_transform_push_data(void *args) DECLSPEC_HIDDEN; #endif /* __WINE_WINEGSTREAMER_UNIX_PRIVATE_H */ diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index 59b15df7d08..0162ed8f550 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -29,6 +29,7 @@ #include "mmreg.h" #include "wine/unixlib.h" +#include "wine/list.h" struct wg_format { @@ -244,6 +245,12 @@ struct wg_transform_create_params const struct wg_format *output_format; }; +struct wg_transform_push_data_params +{ + struct wg_transform *transform; + struct wg_sample *sample; +}; + enum unix_funcs { unix_wg_parser_create, @@ -275,6 +282,8 @@ enum unix_funcs unix_wg_transform_create, unix_wg_transform_destroy, + + unix_wg_transform_push_data, }; #endif /* __WINE_WINEGSTREAMER_UNIXLIB_H */ diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c index d9bbc60964e..5fa49c0219b 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -1660,4 +1660,6 @@ const unixlib_entry_t __wine_unix_call_funcs[] = X(wg_transform_create), X(wg_transform_destroy), + + X(wg_transform_push_data), }; diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index a4a4d886865..7a26e0e4774 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -35,7 +35,7 @@ #include "ntstatus.h" #define WIN32_NO_STATUS #include "winternl.h" -#include "dshow.h" +#include "mferror.h" #include "unix_private.h" @@ -309,3 +309,23 @@ out_free_transform: GST_ERROR("Failed to create winegstreamer transform."); return status; } + +NTSTATUS wg_transform_push_data(void *args) +{ + struct wg_transform_push_data_params *params = args; + struct wg_transform *transform = params->transform; + struct wg_sample *sample = params->sample; + GstFlowReturn ret; + GstBuffer *buffer; + + buffer = gst_buffer_new_and_alloc(sample->size); + if (!buffer) + return STATUS_NO_MEMORY; + + gst_buffer_fill(buffer, 0, sample->data, sample->size); + if ((ret = gst_pad_push(transform->my_src, buffer))) + return MF_E_NOTACCEPTING; + + GST_DEBUG("Pushed %u bytes", sample->size); + return STATUS_SUCCESS; +} diff --git a/dlls/winegstreamer/wma_decoder.c b/dlls/winegstreamer/wma_decoder.c index f84faad669d..55b827ed3cd 100644 --- a/dlls/winegstreamer/wma_decoder.c +++ b/dlls/winegstreamer/wma_decoder.c @@ -553,6 +553,12 @@ static HRESULT WINAPI transform_ProcessInput(IMFTransform *iface, DWORD id, IMFS return S_OK; } + if (FAILED(hr = wg_transform_push_data(decoder->wg_transform, wg_sample))) + { + mf_destroy_wg_sample(wg_sample); + return hr; + } + decoder->input_sample = wg_sample; return S_OK; } -- 2.34.1
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=108995 Your paranoid android. === debian11 (build log) === error: patch failed: dlls/winegstreamer/wg_transform.c:46 error: patch failed: dlls/winegstreamer/wg_transform.c:47 error: patch failed: dlls/winegstreamer/wg_transform.c:309 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/winegstreamer/wg_transform.c:46 error: patch failed: dlls/winegstreamer/wg_transform.c:47 error: patch failed: dlls/winegstreamer/wg_transform.c:309 Task: Patch failed to apply
On 2/22/22 16:48, Rémi Bernon wrote:
+NTSTATUS wg_transform_push_data(void *args) +{ + struct wg_transform_push_data_params *params = args; + struct wg_transform *transform = params->transform; + struct wg_sample *sample = params->sample; + GstFlowReturn ret; + GstBuffer *buffer; + + buffer = gst_buffer_new_and_alloc(sample->size); + if (!buffer) + return STATUS_NO_MEMORY; + + gst_buffer_fill(buffer, 0, sample->data, sample->size); + if ((ret = gst_pad_push(transform->my_src, buffer))) + return MF_E_NOTACCEPTING;
Does MF_E_NOTACCEPTING correspond to any failure GstFlowReturn, though? As far as I can tell GStreamer doesn't even have an equivalent; it just blocks in the chain function if applicable. (Side note: I'd really prefer that we don't mix two different error code namespaces. Note that we're already returning STATUS_NO_MEMORY here when the PE side expects an HRESULT.)
On 2/23/22 03:09, Zebediah Figura (she/her) wrote:
On 2/22/22 16:48, Rémi Bernon wrote:
+NTSTATUS wg_transform_push_data(void *args) +{ + struct wg_transform_push_data_params *params = args; + struct wg_transform *transform = params->transform; + struct wg_sample *sample = params->sample; + GstFlowReturn ret; + GstBuffer *buffer; + + buffer = gst_buffer_new_and_alloc(sample->size); + if (!buffer) + return STATUS_NO_MEMORY; + + gst_buffer_fill(buffer, 0, sample->data, sample->size); + if ((ret = gst_pad_push(transform->my_src, buffer))) + return MF_E_NOTACCEPTING;
Does MF_E_NOTACCEPTING correspond to any failure GstFlowReturn, though? As far as I can tell GStreamer doesn't even have an equivalent; it just blocks in the chain function if applicable.
Yeah we could/should probably return an error instead here.
(Side note: I'd really prefer that we don't mix two different error code namespaces. Note that we're already returning STATUS_NO_MEMORY here when the PE side expects an HRESULT.)
I agree, I'm not sure how to do that though. Perhaps an output result parameter then, I really don't like returning non-NTSTATUS values when the return type is NTSTATUS. -- Rémi Bernon <rbernon(a)codeweavers.com>
This wraps the wg_sample struct in a PE-side struct to add a list entry without exposing it to the unix-side. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51931 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52391 Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/gst_private.h | 3 ++- dlls/winegstreamer/mfplat.c | 36 ++++++++++++++++++++++++------- dlls/winegstreamer/unixlib.h | 2 ++ dlls/winegstreamer/wg_transform.c | 15 +++++++++++-- dlls/winegstreamer/wma_decoder.c | 13 +++++------ 5 files changed, 52 insertions(+), 17 deletions(-) diff --git a/dlls/winegstreamer/gst_private.h b/dlls/winegstreamer/gst_private.h index f0774d53613..e794b2879de 100644 --- a/dlls/winegstreamer/gst_private.h +++ b/dlls/winegstreamer/gst_private.h @@ -120,8 +120,9 @@ extern HRESULT mfplat_DllRegisterServer(void); IMFMediaType *mf_media_type_from_wg_format(const struct wg_format *format); void mf_media_type_to_wg_format(IMFMediaType *type, struct wg_format *format); -HRESULT mf_create_wg_sample(IMFSample *sample, struct wg_sample **out); +HRESULT mf_create_wg_sample(IMFSample *sample, struct list *sample_list, struct wg_sample **out); void mf_destroy_wg_sample(struct wg_sample *wg_sample); +void mf_reap_wg_samples(struct list *sample_list, bool force); HRESULT winegstreamer_stream_handler_create(REFIID riid, void **obj); diff --git a/dlls/winegstreamer/mfplat.c b/dlls/winegstreamer/mfplat.c index 2ba1490f20b..9fbf35378a9 100644 --- a/dlls/winegstreamer/mfplat.c +++ b/dlls/winegstreamer/mfplat.c @@ -828,11 +828,17 @@ void mf_media_type_to_wg_format(IMFMediaType *type, struct wg_format *format) FIXME("Unrecognized major type %s.\n", debugstr_guid(&major_type)); } -HRESULT mf_create_wg_sample(IMFSample *sample, struct wg_sample **out) +struct wg_sample_entry +{ + struct list entry; + struct wg_sample sample; +}; + +HRESULT mf_create_wg_sample(IMFSample *sample, struct list *sample_list, struct wg_sample **out) { DWORD current_length, max_length, count; IMFMediaBuffer *media_buffer = NULL; - struct wg_sample *entry; + struct wg_sample_entry *entry; BYTE *buffer; HRESULT hr; @@ -850,13 +856,14 @@ HRESULT mf_create_wg_sample(IMFSample *sample, struct wg_sample **out) if (FAILED(hr = IMFMediaBuffer_Lock(media_buffer, &buffer, &max_length, ¤t_length))) goto failed; - IMFSample_AddRef((entry->private = sample)); - entry->data = buffer; - entry->size = current_length; - entry->max_size = max_length; + IMFSample_AddRef((entry->sample.private = sample)); + entry->sample.data = buffer; + entry->sample.size = current_length; + entry->sample.max_size = max_length; + list_add_tail(sample_list, &entry->entry); TRACE("Created sample entry %p.\n", entry); - *out = entry; + *out = &entry->sample; return S_OK; failed: @@ -868,9 +875,13 @@ failed: void mf_destroy_wg_sample(struct wg_sample *sample) { + struct wg_sample_entry *entry = CONTAINING_RECORD(sample, struct wg_sample_entry, sample); IMFMediaBuffer *media_buffer; HRESULT hr; + if (sample->refcount) + ERR("Sample %p is still in use, trouble ahead!\n", sample->private); + if (FAILED(hr = IMFSample_GetBufferByIndex(sample->private, 0, &media_buffer))) WARN("Failed to get first buffer, sample %p\n", sample->private); else @@ -884,5 +895,14 @@ void mf_destroy_wg_sample(struct wg_sample *sample) } IMFSample_Release(sample->private); - free(sample); + list_remove(&entry->entry); + free(entry); +} + +void mf_reap_wg_samples(struct list *sample_list, bool force) +{ + struct wg_sample_entry *entry, *next; + LIST_FOR_EACH_ENTRY_SAFE(entry, next, sample_list, struct wg_sample_entry, entry) + if (!entry->sample.refcount || force) + mf_destroy_wg_sample(&entry->sample); } diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index 0162ed8f550..5666149a6a1 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -106,6 +106,8 @@ struct wg_format struct wg_sample { + volatile LONG refcount; /* unix refcount */ + const LONG __pad; UINT32 max_size; UINT32 size; BYTE *data; diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 7a26e0e4774..787157fa1a0 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -310,6 +310,13 @@ out_free_transform: return status; } +static void wg_sample_free_notify(void *arg) +{ + struct wg_sample *sample = arg; + GST_DEBUG("Releasing wg_sample %p", sample); + InterlockedDecrement(&sample->refcount); +} + NTSTATUS wg_transform_push_data(void *args) { struct wg_transform_push_data_params *params = args; @@ -318,11 +325,15 @@ NTSTATUS wg_transform_push_data(void *args) GstFlowReturn ret; GstBuffer *buffer; - buffer = gst_buffer_new_and_alloc(sample->size); + InterlockedIncrement(&sample->refcount); + buffer = gst_buffer_new_wrapped_full(GST_MEMORY_FLAG_READONLY, sample->data, sample->max_size, + 0, sample->size, sample, wg_sample_free_notify); if (!buffer) + { + InterlockedDecrement(&sample->refcount); return STATUS_NO_MEMORY; + } - gst_buffer_fill(buffer, 0, sample->data, sample->size); if ((ret = gst_pad_push(transform->my_src, buffer))) return MF_E_NOTACCEPTING; diff --git a/dlls/winegstreamer/wma_decoder.c b/dlls/winegstreamer/wma_decoder.c index 55b827ed3cd..03cd6ad54c0 100644 --- a/dlls/winegstreamer/wma_decoder.c +++ b/dlls/winegstreamer/wma_decoder.c @@ -55,7 +55,7 @@ struct wma_decoder IMFMediaType *output_type; struct wg_transform *wg_transform; - struct wg_sample *input_sample; + struct list input_samples; }; static inline struct wma_decoder *impl_from_IUnknown(IUnknown *iface) @@ -135,8 +135,7 @@ static ULONG WINAPI unknown_Release(IUnknown *iface) IMFMediaType_Release(decoder->input_type); if (decoder->output_type) IMFMediaType_Release(decoder->output_type); - if (decoder->input_sample) - mf_destroy_wg_sample(decoder->input_sample); + mf_reap_wg_samples(&decoder->input_samples, true); free(decoder); } @@ -537,14 +536,16 @@ static HRESULT WINAPI transform_ProcessInput(IMFTransform *iface, DWORD id, IMFS if (!decoder->wg_transform) return MF_E_TRANSFORM_TYPE_NOT_SET; - if (decoder->input_sample) + if (!list_empty(&decoder->input_samples)) return MF_E_NOTACCEPTING; if (FAILED(hr = IMFSample_ConvertToContiguousBuffer(sample, &media_buffer))) return hr; IMFMediaBuffer_Release(media_buffer); - if (FAILED(hr = mf_create_wg_sample(sample, &wg_sample))) + mf_reap_wg_samples(&decoder->input_samples, false); + + if (FAILED(hr = mf_create_wg_sample(sample, &decoder->input_samples, &wg_sample))) return hr; if (!(wg_sample->size = (wg_sample->size / info.cbSize) * info.cbSize)) @@ -559,7 +560,6 @@ static HRESULT WINAPI transform_ProcessInput(IMFTransform *iface, DWORD id, IMFS return hr; } - decoder->input_sample = wg_sample; return S_OK; } @@ -847,6 +847,7 @@ HRESULT wma_decoder_create(IUnknown *outer, IUnknown **out) decoder->IPropertyBag_iface.lpVtbl = &property_bag_vtbl; decoder->refcount = 1; decoder->outer = outer ? outer : &decoder->IUnknown_inner; + list_init(&decoder->input_samples); *out = &decoder->IUnknown_inner; TRACE("Created decoder %p\n", *out); -- 2.34.1
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=108996 Your paranoid android. === debian11 (build log) === error: patch failed: dlls/winegstreamer/wg_transform.c:46 error: patch failed: dlls/winegstreamer/wg_transform.c:47 error: patch failed: dlls/winegstreamer/wg_transform.c:309 error: patch failed: dlls/winegstreamer/wg_transform.c:310 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/winegstreamer/wg_transform.c:46 error: patch failed: dlls/winegstreamer/wg_transform.c:47 error: patch failed: dlls/winegstreamer/wg_transform.c:309 error: patch failed: dlls/winegstreamer/wg_transform.c:310 Task: Patch failed to apply
On 2/22/22 16:48, Rémi Bernon wrote:
This wraps the wg_sample struct in a PE-side struct to add a list entry without exposing it to the unix-side.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51931 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52391 Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/gst_private.h | 3 ++- dlls/winegstreamer/mfplat.c | 36 ++++++++++++++++++++++++------- dlls/winegstreamer/unixlib.h | 2 ++ dlls/winegstreamer/wg_transform.c | 15 +++++++++++-- dlls/winegstreamer/wma_decoder.c | 13 +++++------ 5 files changed, 52 insertions(+), 17 deletions(-)
This patch is kind of hard to review (or test) without ProcessOutput() implemented. Is there a chance that you can send that first?
+void mf_reap_wg_samples(struct list *sample_list, bool force) +{ + struct wg_sample_entry *entry, *next; + LIST_FOR_EACH_ENTRY_SAFE(entry, next, sample_list, struct wg_sample_entry, entry) + if (!entry->sample.refcount || force) + mf_destroy_wg_sample(&entry->sample); }
"force" seems awkward here. Since it should never happen anyway, it strikes me as better just to print an ERR here [and probably here rather than in mf_destroy_wg_sample()] and leak it.
diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index 0162ed8f550..5666149a6a1 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -106,6 +106,8 @@ struct wg_format
struct wg_sample { + volatile LONG refcount; /* unix refcount */
Does this need to be a refcount? We only ever incref once in this patch; this could just as easily be a "free" member instead. It doesn't make much of a difference, but when I see "refcount" I find myself thinking about why it's a refcount...
+ const LONG __pad; UINT32 max_size; UINT32 size; BYTE *data;
Also: do the rest of these members really need to be a part of struct wg_sample per se? It seems they could just as easily be passed to wg_transform_push_data() and then forgotten about. Unless I'm missing something, the only thing that really needs to be shared is the refcount (or free flag). Obviously it's convenient to put the other members here, but I think it makes the code less clear.
@@ -318,11 +325,15 @@ NTSTATUS wg_transform_push_data(void *args) GstFlowReturn ret; GstBuffer *buffer;
- buffer = gst_buffer_new_and_alloc(sample->size); + InterlockedIncrement(&sample->refcount); + buffer = gst_buffer_new_wrapped_full(GST_MEMORY_FLAG_READONLY, sample->data, sample->max_size, + 0, sample->size, sample, wg_sample_free_notify); if (!buffer) + { + InterlockedDecrement(&sample->refcount); return STATUS_NO_MEMORY; + }
We don't need to incref before calling gst_buffer_new_wrapped_full(), do we? We haven't passed the buffer to anything yet.
@@ -537,14 +536,16 @@ static HRESULT WINAPI transform_ProcessInput(IMFTransform *iface, DWORD id, IMFS if (!decoder->wg_transform) return MF_E_TRANSFORM_TYPE_NOT_SET;
- if (decoder->input_sample) + if (!list_empty(&decoder->input_samples)) return MF_E_NOTACCEPTING;
Why store samples in a list, if we're still only going to limit it to one at a time?
On 2/23/22 03:09, Zebediah Figura (she/her) wrote:
On 2/22/22 16:48, Rémi Bernon wrote:
This wraps the wg_sample struct in a PE-side struct to add a list entry without exposing it to the unix-side.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51931 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52391 Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/gst_private.h | 3 ++- dlls/winegstreamer/mfplat.c | 36 ++++++++++++++++++++++++------- dlls/winegstreamer/unixlib.h | 2 ++ dlls/winegstreamer/wg_transform.c | 15 +++++++++++-- dlls/winegstreamer/wma_decoder.c | 13 +++++------ 5 files changed, 52 insertions(+), 17 deletions(-)
This patch is kind of hard to review (or test) without ProcessOutput() implemented. Is there a chance that you can send that first?
Well ProcessOutput would actually do nothing unless there's buffer pushed first, so it'd be kind of dead code but probably, yes.
+void mf_reap_wg_samples(struct list *sample_list, bool force) +{ + struct wg_sample_entry *entry, *next; + LIST_FOR_EACH_ENTRY_SAFE(entry, next, sample_list, struct wg_sample_entry, entry) + if (!entry->sample.refcount || force) + mf_destroy_wg_sample(&entry->sample); }
"force" seems awkward here. Since it should never happen anyway, it strikes me as better just to print an ERR here [and probably here rather than in mf_destroy_wg_sample()] and leak it.
wg_sample_destroy could also separately and incorrectly be called with a non zero refcount, so I think the ERR is better to have it in it. Otherwise yeah leaking the sample might be better.
diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index 0162ed8f550..5666149a6a1 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -106,6 +106,8 @@ struct wg_format
struct wg_sample { + volatile LONG refcount; /* unix refcount */
Does this need to be a refcount? We only ever incref once in this patch; this could just as easily be a "free" member instead.
It doesn't make much of a difference, but when I see "refcount" I find myself thinking about why it's a refcount...
Yes, the same sample may be pushed multiple times. Not with WMA, because it doesn't accept input as soon as a has been provided, but other transform may and do.
+ const LONG __pad; UINT32 max_size; UINT32 size; BYTE *data;
Also: do the rest of these members really need to be a part of struct wg_sample per se? It seems they could just as easily be passed to wg_transform_push_data() and then forgotten about. Unless I'm missing something, the only thing that really needs to be shared is the refcount (or free flag).
Obviously it's convenient to put the other members here, but I think it makes the code less clear.
I think it makes the code clearer as it saves us tons of free parameters passed around. We need at least refcount and size to be kept so that we can update the size of the sample buffer when it's released (used for output). Later we will also need a flags member, wg_format, timestamp and duration. All are properties of the sample / buffer we've wrapped and imho makes sense to keep here. Also later, in ProcessOutput I re-use the struct to provide the unix output buffer from the sink_chain_cb to read_data function. It could be another struct but as it shares most of the properties here this struct fits very well.
@@ -318,11 +325,15 @@ NTSTATUS wg_transform_push_data(void *args) GstFlowReturn ret; GstBuffer *buffer;
- buffer = gst_buffer_new_and_alloc(sample->size); + InterlockedIncrement(&sample->refcount); + buffer = gst_buffer_new_wrapped_full(GST_MEMORY_FLAG_READONLY, sample->data, sample->max_size, + 0, sample->size, sample, wg_sample_free_notify); if (!buffer) + { + InterlockedDecrement(&sample->refcount); return STATUS_NO_MEMORY; + }
We don't need to incref before calling gst_buffer_new_wrapped_full(), do we? We haven't passed the buffer to anything yet.
Sure, it seemed more correct to increment refount before any use, as gst_buffer_new_wrapped_full could do anything, and decrement after it failed to wrap it.
@@ -537,14 +536,16 @@ static HRESULT WINAPI transform_ProcessInput(IMFTransform *iface, DWORD id, IMFS if (!decoder->wg_transform) return MF_E_TRANSFORM_TYPE_NOT_SET;
- if (decoder->input_sample) + if (!list_empty(&decoder->input_samples)) return MF_E_NOTACCEPTING;
Why store samples in a list, if we're still only going to limit it to one at a time?
Right now we do, and with WMA it'll probably always be the case, but it's not a requirement and using a list makes the queueing mechanism reusable in other transforms which do not share the same pattern. Later the check will also be moved to the unix side and return MF_E_NOTACCEPTING if there's output sample read, which may not correspond to a single input sample being queued. -- Rémi Bernon <rbernon(a)codeweavers.com>
On 2/23/22 02:22, Rémi Bernon wrote:
On 2/23/22 03:09, Zebediah Figura (she/her) wrote:
On 2/22/22 16:48, Rémi Bernon wrote:
This wraps the wg_sample struct in a PE-side struct to add a list entry without exposing it to the unix-side.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51931 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52391 Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/gst_private.h | 3 ++- dlls/winegstreamer/mfplat.c | 36 ++++++++++++++++++++++++------- dlls/winegstreamer/unixlib.h | 2 ++ dlls/winegstreamer/wg_transform.c | 15 +++++++++++-- dlls/winegstreamer/wma_decoder.c | 13 +++++------ 5 files changed, 52 insertions(+), 17 deletions(-)
This patch is kind of hard to review (or test) without ProcessOutput() implemented. Is there a chance that you can send that first?
Well ProcessOutput would actually do nothing unless there's buffer pushed first, so it'd be kind of dead code but probably, yes.
I meant just implementing sample processing first without the zero-copy optimization, as you seem to have done in v2.
+void mf_reap_wg_samples(struct list *sample_list, bool force) +{ + struct wg_sample_entry *entry, *next; + LIST_FOR_EACH_ENTRY_SAFE(entry, next, sample_list, struct wg_sample_entry, entry) + if (!entry->sample.refcount || force) + mf_destroy_wg_sample(&entry->sample); }
"force" seems awkward here. Since it should never happen anyway, it strikes me as better just to print an ERR here [and probably here rather than in mf_destroy_wg_sample()] and leak it.
wg_sample_destroy could also separately and incorrectly be called with a non zero refcount, so I think the ERR is better to have it in it. Otherwise yeah leaking the sample might be better.
Sure, on reflection I think that makes sense.
diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index 0162ed8f550..5666149a6a1 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -106,6 +106,8 @@ struct wg_format struct wg_sample { + volatile LONG refcount; /* unix refcount */
Does this need to be a refcount? We only ever incref once in this patch; this could just as easily be a "free" member instead.
It doesn't make much of a difference, but when I see "refcount" I find myself thinking about why it's a refcount...
Yes, the same sample may be pushed multiple times. Not with WMA, because it doesn't accept input as soon as a has been provided, but other transform may and do.
Okay, makes sense.
+ const LONG __pad; UINT32 max_size; UINT32 size; BYTE *data;
Also: do the rest of these members really need to be a part of struct wg_sample per se? It seems they could just as easily be passed to wg_transform_push_data() and then forgotten about. Unless I'm missing something, the only thing that really needs to be shared is the refcount (or free flag).
Obviously it's convenient to put the other members here, but I think it makes the code less clear.
I think it makes the code clearer as it saves us tons of free parameters passed around.
We need at least refcount and size to be kept so that we can update the size of the sample buffer when it's released (used for output). Later we will also need a flags member, wg_format, timestamp and duration.
All are properties of the sample / buffer we've wrapped and imho makes sense to keep here.
Also later, in ProcessOutput I re-use the struct to provide the unix output buffer from the sink_chain_cb to read_data function.
It could be another struct but as it shares most of the properties here this struct fits very well.
I'll admit I'm not thrilled about it. I can see the logic behind making it a single coherent object; what I think is made less clear this way is the synchronization model (for those fields in particular). Still, I can probably live with it...
@@ -318,11 +325,15 @@ NTSTATUS wg_transform_push_data(void *args) GstFlowReturn ret; GstBuffer *buffer; - buffer = gst_buffer_new_and_alloc(sample->size); + InterlockedIncrement(&sample->refcount); + buffer = gst_buffer_new_wrapped_full(GST_MEMORY_FLAG_READONLY, sample->data, sample->max_size, + 0, sample->size, sample, wg_sample_free_notify); if (!buffer) + { + InterlockedDecrement(&sample->refcount); return STATUS_NO_MEMORY; + }
We don't need to incref before calling gst_buffer_new_wrapped_full(), do we? We haven't passed the buffer to anything yet.
Sure, it seemed more correct to increment refount before any use, as gst_buffer_new_wrapped_full could do anything, and decrement after it failed to wrap it.
I guess...
@@ -537,14 +536,16 @@ static HRESULT WINAPI transform_ProcessInput(IMFTransform *iface, DWORD id, IMFS if (!decoder->wg_transform) return MF_E_TRANSFORM_TYPE_NOT_SET; - if (decoder->input_sample) + if (!list_empty(&decoder->input_samples)) return MF_E_NOTACCEPTING;
Why store samples in a list, if we're still only going to limit it to one at a time?
Right now we do, and with WMA it'll probably always be the case, but it's not a requirement and using a list makes the queueing mechanism reusable in other transforms which do not share the same pattern.
Later the check will also be moved to the unix side and return MF_E_NOTACCEPTING if there's output sample read, which may not correspond to a single input sample being queued.
Okay, I guess I can live with that...
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=108992 Your paranoid android. === debian11 (build log) === error: patch failed: dlls/winegstreamer/wg_transform.c:46 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/winegstreamer/wg_transform.c:46 Task: Patch failed to apply
participants (3)
-
Marvin -
Rémi Bernon -
Zebediah Figura (she/her)