[PATCH v5 0/5] MR302: winegstreamer: Some wg_transform H264 fixes for Mortal Kombat 11 and Yakuza 4.
-- v5: winegstreamer: Use an atomic queue for wg_transform input buffers. winegstreamer: Release requested samples if they are too small. winegstreamer: Introduce a new wg_allocator_set_next_sample helper. winegstreamer: Use the GstObject lock instead of a new allocator mutex. mf/tests: Add todo_wine for newer FFmpeg versions. https://gitlab.winehq.org/wine/wine/-/merge_requests/302
From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/mf/tests/mf.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c index 2f3ee3151da..5f26b1a6238 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -5911,7 +5911,7 @@ static void check_sample_pcm16_(int line, IMFSample *sample, const BYTE *expect_ if (expect - value + 512 > 1024) break; } - todo_wine_if(todo) + todo_wine_if(todo && i < length / 2) ok_(__FILE__, line)(i == length, "unexpected buffer data\n"); if (output_file) WriteFile(output_file, buffer, length, &length, NULL); @@ -6544,6 +6544,9 @@ static void test_wma_decoder(void) hr = IMFTransform_ProcessOutput(transform, 0, 1, &output, &status); winetest_pop_context(); + + /* some FFmpeg version request more input to complete decoding */ + if (hr == MF_E_TRANSFORM_NEED_MORE_INPUT && i == 2) break; } todo_wine ok(wmadec_data_len == 0, "missing %#lx bytes\n", wmadec_data_len); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/302
From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/wg_allocator.c | 37 ++++++++++++++----------------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/dlls/winegstreamer/wg_allocator.c b/dlls/winegstreamer/wg_allocator.c index c31751ce83f..16e961a57d4 100644 --- a/dlls/winegstreamer/wg_allocator.c +++ b/dlls/winegstreamer/wg_allocator.c @@ -57,8 +57,7 @@ typedef struct wg_allocator_request_sample_cb request_sample; void *request_sample_context; - pthread_mutex_t mutex; - pthread_cond_t release_cond; + GCond release_cond; struct list memory_list; } WgAllocator; @@ -79,7 +78,7 @@ static gpointer wg_allocator_map(GstMemory *gst_memory, GstMapInfo *info, gsize GST_LOG("memory %p, info %p, maxsize %#zx", memory, info, maxsize); - pthread_mutex_lock(&allocator->mutex); + GST_OBJECT_LOCK(allocator); if (!memory->sample) info->data = memory->unix_map_info.data; @@ -91,7 +90,7 @@ static gpointer wg_allocator_map(GstMemory *gst_memory, GstMapInfo *info, gsize if (info->flags & GST_MAP_WRITE) memory->written = max(memory->written, maxsize); - pthread_mutex_unlock(&allocator->mutex); + GST_OBJECT_UNLOCK(allocator); GST_INFO("Mapped memory %p to %p", memory, info->data); return info->data; @@ -107,15 +106,15 @@ static void wg_allocator_unmap(GstMemory *gst_memory, GstMapInfo *info) GST_LOG("memory %p, info %p", memory, info); - pthread_mutex_lock(&allocator->mutex); + GST_OBJECT_LOCK(allocator); if (memory->sample && info->data == memory->sample->data) { InterlockedDecrement(&memory->sample->refcount); - pthread_cond_signal(&allocator->release_cond); + g_cond_signal(&allocator->release_cond); } - pthread_mutex_unlock(&allocator->mutex); + GST_OBJECT_UNLOCK(allocator); } static void wg_allocator_init(WgAllocator *allocator) @@ -129,8 +128,7 @@ static void wg_allocator_init(WgAllocator *allocator) GST_OBJECT_FLAG_SET(allocator, GST_ALLOCATOR_FLAG_CUSTOM_ALLOC); - pthread_mutex_init(&allocator->mutex, NULL); - pthread_cond_init(&allocator->release_cond, NULL); + g_cond_init(&allocator->release_cond); list_init(&allocator->memory_list); } @@ -140,8 +138,7 @@ static void wg_allocator_finalize(GObject *object) GST_LOG("allocator %p", allocator); - pthread_cond_destroy(&allocator->release_cond); - pthread_mutex_destroy(&allocator->mutex); + g_cond_clear(&allocator->release_cond); G_OBJECT_CLASS(wg_allocator_parent_class)->finalize(object); } @@ -160,12 +157,12 @@ static GstMemory *wg_allocator_alloc(GstAllocator *gst_allocator, gsize size, memory->unix_memory = gst_allocator_alloc(NULL, size, params); gst_memory_map(memory->unix_memory, &memory->unix_map_info, GST_MAP_WRITE); - pthread_mutex_lock(&allocator->mutex); + GST_OBJECT_LOCK(allocator); memory->sample = allocator->request_sample(size, allocator->request_sample_context); list_add_tail(&allocator->memory_list, &memory->entry); - pthread_mutex_unlock(&allocator->mutex); + GST_OBJECT_UNLOCK(allocator); GST_INFO("Allocated memory %p, sample %p, unix_memory %p, data %p", memory, memory->sample, memory->unix_memory, memory->unix_map_info.data); @@ -179,7 +176,7 @@ static void wg_allocator_free(GstAllocator *gst_allocator, GstMemory *gst_memory GST_LOG("allocator %p, memory %p", allocator, memory); - pthread_mutex_lock(&allocator->mutex); + GST_OBJECT_LOCK(allocator); if (memory->sample) InterlockedDecrement(&memory->sample->refcount); @@ -187,7 +184,7 @@ static void wg_allocator_free(GstAllocator *gst_allocator, GstMemory *gst_memory list_remove(&memory->entry); - pthread_mutex_unlock(&allocator->mutex); + GST_OBJECT_UNLOCK(allocator); gst_memory_unmap(memory->unix_memory, &memory->unix_map_info); gst_memory_unref(memory->unix_memory); @@ -228,7 +225,7 @@ static void release_memory_sample(WgAllocator *allocator, WgMemory *memory, bool while (sample->refcount > 1) { GST_WARNING("Waiting for sample %p to be unmapped", sample); - pthread_cond_wait(&allocator->release_cond, &allocator->mutex); + g_cond_wait(&allocator->release_cond, GST_OBJECT_GET_LOCK(allocator)); } InterlockedDecrement(&sample->refcount); @@ -249,10 +246,10 @@ void wg_allocator_destroy(GstAllocator *gst_allocator) GST_LOG("allocator %p", allocator); - pthread_mutex_lock(&allocator->mutex); + GST_OBJECT_LOCK(allocator); LIST_FOR_EACH_ENTRY(memory, &allocator->memory_list, WgMemory, entry) release_memory_sample(allocator, memory, true); - pthread_mutex_unlock(&allocator->mutex); + GST_OBJECT_UNLOCK(allocator); g_object_unref(allocator); @@ -278,10 +275,10 @@ void wg_allocator_release_sample(GstAllocator *gst_allocator, struct wg_sample * GST_LOG("allocator %p, sample %p, discard_data %u", allocator, sample, discard_data); - pthread_mutex_lock(&allocator->mutex); + GST_OBJECT_LOCK(allocator); if ((memory = find_sample_memory(allocator, sample))) release_memory_sample(allocator, memory, discard_data); else if (sample->refcount) GST_ERROR("Couldn't find memory for sample %p", sample); - pthread_mutex_unlock(&allocator->mutex); + GST_OBJECT_UNLOCK(allocator); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/302
From: Rémi Bernon <rbernon(a)codeweavers.com> Using the allocator lock and replacing the transform_request_sample callback with a default wg_allocator callback. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/unix_private.h | 2 ++ dlls/winegstreamer/wg_allocator.c | 42 +++++++++++++++++++++++++++++-- dlls/winegstreamer/wg_transform.c | 24 +++--------------- 3 files changed, 46 insertions(+), 22 deletions(-) diff --git a/dlls/winegstreamer/unix_private.h b/dlls/winegstreamer/unix_private.h index e9f472986ae..2bfdc6f9f5b 100644 --- a/dlls/winegstreamer/unix_private.h +++ b/dlls/winegstreamer/unix_private.h @@ -44,5 +44,7 @@ extern GstAllocator *wg_allocator_create(wg_allocator_request_sample_cb request_ extern void wg_allocator_destroy(GstAllocator *allocator) DECLSPEC_HIDDEN; extern void wg_allocator_release_sample(GstAllocator *allocator, struct wg_sample *sample, bool discard_data) DECLSPEC_HIDDEN; +extern void wg_allocator_set_next_sample(GstAllocator *allocator, + struct wg_sample *sample) DECLSPEC_HIDDEN; #endif /* __WINE_WINEGSTREAMER_UNIX_PRIVATE_H */ diff --git a/dlls/winegstreamer/wg_allocator.c b/dlls/winegstreamer/wg_allocator.c index 16e961a57d4..53ea5d08c8e 100644 --- a/dlls/winegstreamer/wg_allocator.c +++ b/dlls/winegstreamer/wg_allocator.c @@ -54,6 +54,7 @@ typedef struct { GstAllocator parent; + struct wg_sample *next_sample; wg_allocator_request_sample_cb request_sample; void *request_sample_context; @@ -68,6 +69,23 @@ typedef struct G_DEFINE_TYPE(WgAllocator, wg_allocator, GST_TYPE_ALLOCATOR); +static struct wg_sample *default_request_sample(gsize size, void *context) +{ + WgAllocator *allocator = context; + struct wg_sample *sample; + + GST_LOG("size %#zx, context %p", size, context); + + if (!(sample = allocator->next_sample)) + return NULL; + allocator->next_sample = NULL; + + if (sample->max_size < size) + return NULL; + + return sample; +} + static gpointer wg_allocator_map(GstMemory *gst_memory, GstMapInfo *info, gsize maxsize) { WgAllocator *allocator = (WgAllocator *)gst_memory->allocator; @@ -210,8 +228,14 @@ GstAllocator *wg_allocator_create(wg_allocator_request_sample_cb request_sample, if (!(allocator = g_object_new(wg_allocator_get_type(), NULL))) return NULL; - allocator->request_sample = request_sample; - allocator->request_sample_context = request_sample_context; + if ((allocator->request_sample = request_sample)) + allocator->request_sample_context = request_sample_context; + else + { + allocator->request_sample = default_request_sample; + allocator->request_sample_context = allocator; + } + return GST_ALLOCATOR(allocator); } @@ -282,3 +306,17 @@ void wg_allocator_release_sample(GstAllocator *gst_allocator, struct wg_sample * GST_ERROR("Couldn't find memory for sample %p", sample); GST_OBJECT_UNLOCK(allocator); } + +void wg_allocator_set_next_sample(GstAllocator *gst_allocator, struct wg_sample *sample) +{ + WgAllocator *allocator = (WgAllocator *)gst_allocator; + + GST_LOG("allocator %p, sample %p", allocator, sample); + + GST_OBJECT_LOCK(allocator); + if (allocator->next_sample) + InterlockedDecrement(&allocator->next_sample->refcount); + if ((allocator->next_sample = sample)) + InterlockedIncrement(&allocator->next_sample->refcount); + GST_OBJECT_UNLOCK(allocator); +} diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index e05432f6ac7..557e085ea31 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -53,8 +53,8 @@ struct wg_transform GstSegment segment; GstBufferList *input; guint input_max_length; + guint output_plane_align; - struct wg_sample *output_wg_sample; GstAtomicQueue *output_queue; GstSample *output_sample; bool output_caps_changed; @@ -307,20 +307,6 @@ static bool transform_append_element(struct wg_transform *transform, GstElement return success; } -static struct wg_sample *transform_request_sample(gsize size, void *context) -{ - struct wg_transform *transform = context; - struct wg_sample *sample; - - GST_LOG("size %#zx, context %p", size, transform); - - sample = InterlockedExchangePointer((void **)&transform->output_wg_sample, NULL); - if (!sample || sample->max_size < size) - return NULL; - - return sample; -} - NTSTATUS wg_transform_create(void *args) { struct wg_transform_create_params *params = args; @@ -345,7 +331,7 @@ NTSTATUS wg_transform_create(void *args) goto out; if (!(transform->output_queue = gst_atomic_queue_new(8))) goto out; - if (!(transform->allocator = wg_allocator_create(transform_request_sample, transform))) + if (!(transform->allocator = wg_allocator_create(NULL, NULL))) goto out; transform->input_max_length = 1; transform->output_plane_align = 0; @@ -724,8 +710,7 @@ NTSTATUS wg_transform_read_data(void *args) NTSTATUS status; /* Provide the sample for transform_request_sample to pick it up */ - InterlockedIncrement(&sample->refcount); - InterlockedExchangePointer((void **)&transform->output_wg_sample, sample); + wg_allocator_set_next_sample(transform->allocator, sample); if (!gst_buffer_list_length(transform->input)) GST_DEBUG("Not input buffer queued"); @@ -741,8 +726,7 @@ NTSTATUS wg_transform_read_data(void *args) } /* Remove the sample so transform_request_sample cannot use it */ - if (InterlockedExchangePointer((void **)&transform->output_wg_sample, NULL)) - InterlockedDecrement(&sample->refcount); + wg_allocator_set_next_sample(transform->allocator, NULL); if (ret) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/302
On 6/30/22 07:33, Rémi Bernon wrote:
From: Rémi Bernon <rbernon(a)codeweavers.com>
Using the allocator lock and replacing the transform_request_sample callback with a default wg_allocator callback.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/unix_private.h | 2 ++ dlls/winegstreamer/wg_allocator.c | 42 +++++++++++++++++++++++++++++-- dlls/winegstreamer/wg_transform.c | 24 +++--------------- 3 files changed, 46 insertions(+), 22 deletions(-)
This is making things even more complicated; now we have two entirely different code paths to set samples. I understand that adding more locks increases the mental burden, but I think this is worse—sure, we're "reusing" a lock that was going to be created anyway, but it's still effectively introducing another lock, plus now this requires us to understand when GStreamer is going to implicitly take that lock. Not to mention that this is similar to what I was trying to propose, enough that I'd like to revisit it. I really don't want to argue about the way forward—there's been too much of that already—but I'm still finding it hard to justify the complexity of this whole callback infrastructure, when we could more simply set up a fixed-size pool beforehand. Yes, we might run out of space in the pool and need to allocate more sysmem buffers, but it should be easy enough to find a pool size large enough that that doesn't happen in practice. [Ideally it would take an application leaking an entire stream of a muxed file—which has been known to happen—but in that case, it strikes me as better to eat up the Unix address space instead of the win32 address space, so the limited size is actually a good thing.] Or, failing that (or in addition to it?), maybe we could just not use the same wg_allocator object for both transforms and parsers. Yes, it's more gobject boilerplate, which is annoying, but wg_allocator is a complex beast and I really don't want to make it worse. And when it comes to output samples, the allocator and the transform really do seem to have entirely different considerations.
From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/wg_allocator.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/dlls/winegstreamer/wg_allocator.c b/dlls/winegstreamer/wg_allocator.c index 53ea5d08c8e..46343db8aae 100644 --- a/dlls/winegstreamer/wg_allocator.c +++ b/dlls/winegstreamer/wg_allocator.c @@ -80,9 +80,6 @@ static struct wg_sample *default_request_sample(gsize size, void *context) return NULL; allocator->next_sample = NULL; - if (sample->max_size < size) - return NULL; - return sample; } @@ -165,6 +162,7 @@ static GstMemory *wg_allocator_alloc(GstAllocator *gst_allocator, gsize size, GstAllocationParams *params) { WgAllocator *allocator = (WgAllocator *)gst_allocator; + struct wg_sample *sample; WgMemory *memory; GST_LOG("allocator %p, size %#zx, params %p", allocator, size, params); @@ -177,7 +175,12 @@ static GstMemory *wg_allocator_alloc(GstAllocator *gst_allocator, gsize size, GST_OBJECT_LOCK(allocator); - memory->sample = allocator->request_sample(size, allocator->request_sample_context); + sample = allocator->request_sample(size, allocator->request_sample_context); + if (sample->max_size < size) + InterlockedDecrement(&sample->refcount); + else + memory->sample = sample; + list_add_tail(&allocator->memory_list, &memory->entry); GST_OBJECT_UNLOCK(allocator); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/302
From: Rémi Bernon <rbernon(a)codeweavers.com> And push them one by one until an output buffer is generated, to avoid generating multiple output buffers without a backing wg_sample. This makes zero-copy more efficient for games which queue multiple input buffers before checking output, such as Yakuza 4. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=45988 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=47084 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=49715 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52183 Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/wg_transform.c | 56 +++++++++++++++---------------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 557e085ea31..3bdd5cbb481 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -51,8 +51,9 @@ struct wg_transform GstPad *my_src, *my_sink; GstPad *their_sink, *their_src; GstSegment segment; - GstBufferList *input; + guint input_max_length; + GstAtomicQueue *input_queue; guint output_plane_align; GstAtomicQueue *output_queue; @@ -215,9 +216,11 @@ NTSTATUS wg_transform_destroy(void *args) { struct wg_transform *transform = args; GstSample *sample; + GstBuffer *buffer; - if (transform->input) - gst_buffer_list_unref(transform->input); + while ((buffer = gst_atomic_queue_pop(transform->input_queue))) + gst_buffer_unref(buffer); + gst_atomic_queue_unref(transform->input_queue); gst_element_set_state(transform->container, GST_STATE_NULL); @@ -327,7 +330,7 @@ NTSTATUS wg_transform_create(void *args) return STATUS_NO_MEMORY; if (!(transform->container = gst_bin_new("wg_transform"))) goto out; - if (!(transform->input = gst_buffer_list_new())) + if (!(transform->input_queue = gst_atomic_queue_new(8))) goto out; if (!(transform->output_queue = gst_atomic_queue_new(8))) goto out; @@ -494,8 +497,8 @@ out: wg_allocator_destroy(transform->allocator); if (transform->output_queue) gst_atomic_queue_unref(transform->output_queue); - if (transform->input) - gst_buffer_list_unref(transform->input); + if (transform->input_queue) + gst_atomic_queue_unref(transform->input_queue); if (transform->container) { gst_element_set_state(transform->container, GST_STATE_NULL); @@ -521,7 +524,7 @@ NTSTATUS wg_transform_push_data(void *args) GstBuffer *buffer; guint length; - length = gst_buffer_list_length(transform->input); + length = gst_atomic_queue_length(transform->input_queue); if (length >= transform->input_max_length) { GST_INFO("Refusing %u bytes, %u buffers already queued", sample->size, length); @@ -547,7 +550,7 @@ NTSTATUS wg_transform_push_data(void *args) GST_BUFFER_DURATION(buffer) = sample->duration * 100; if (!(sample->flags & WG_SAMPLE_FLAG_SYNC_POINT)) GST_BUFFER_FLAG_SET(buffer, GST_BUFFER_FLAG_DELTA_UNIT); - gst_buffer_list_insert(transform->input, -1, buffer); + gst_atomic_queue_push(transform->input_queue, buffer); params->result = S_OK; return STATUS_SUCCESS; @@ -702,9 +705,7 @@ NTSTATUS wg_transform_read_data(void *args) struct wg_transform *transform = params->transform; struct wg_sample *sample = params->sample; struct wg_format *format = params->format; - GstFlowReturn ret = GST_FLOW_OK; GstBuffer *output_buffer; - GstBufferList *input; GstCaps *output_caps; bool discard_data; NTSTATUS status; @@ -712,30 +713,29 @@ NTSTATUS wg_transform_read_data(void *args) /* Provide the sample for transform_request_sample to pick it up */ wg_allocator_set_next_sample(transform->allocator, sample); - if (!gst_buffer_list_length(transform->input)) - GST_DEBUG("Not input buffer queued"); - else if ((input = gst_buffer_list_new())) - { - ret = gst_pad_push_list(transform->my_src, transform->input); - transform->input = input; - } - else + while (!transform->output_sample) { - GST_ERROR("Failed to allocate new input queue"); - ret = GST_FLOW_ERROR; + GstFlowReturn ret = GST_FLOW_OK; + GstBuffer *input_buffer; + + if ((input_buffer = gst_atomic_queue_pop(transform->input_queue)) + && (ret = gst_pad_push(transform->my_src, input_buffer))) + { + GST_ERROR("Failed to push transform input, error %d", ret); + wg_allocator_set_next_sample(transform->allocator, NULL); + wg_allocator_release_sample(transform->allocator, sample, false); + return STATUS_UNSUCCESSFUL; + } + + transform->output_sample = gst_atomic_queue_pop(transform->output_queue); + if (!input_buffer) + break; } /* Remove the sample so transform_request_sample cannot use it */ wg_allocator_set_next_sample(transform->allocator, NULL); - if (ret) - { - GST_ERROR("Failed to push transform input, error %d", ret); - wg_allocator_release_sample(transform->allocator, sample, false); - return STATUS_UNSUCCESSFUL; - } - - if (!transform->output_sample && !(transform->output_sample = gst_atomic_queue_pop(transform->output_queue))) + if (!transform->output_sample) { sample->size = 0; params->result = MF_E_TRANSFORM_NEED_MORE_INPUT; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/302
v4: Refactor wg_transform allocator requests and input buffer atomic queue, drop `wg_transform_set_format` patches for now. v5: Remove a leftover output_wg_sample from wg_transform struct. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/302#note_3036
On Mon Jul 4 21:34:22 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list: ``` On 6/30/22 07:33, Rémi Bernon wrote:
From: Rémi Bernon <rbernon(a)codeweavers.com>
Using the allocator lock and replacing the transform_request_sample callback with a default wg_allocator callback.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/unix_private.h | 2 ++ dlls/winegstreamer/wg_allocator.c | 42 +++++++++++++++++++++++++++++-- dlls/winegstreamer/wg_transform.c | 24 +++--------------- 3 files changed, 46 insertions(+), 22 deletions(-)
This is making things even more complicated; now we have two entirely different code paths to set samples. I understand that adding more locks increases the mental burden, but I think this is worse—sure, we're "reusing" a lock that was going to be created anyway, but it's still effectively introducing another lock, plus now this requires us to understand when GStreamer is going to implicitly take that lock. Not to mention that this is similar to what I was trying to propose, enough that I'd like to revisit it. I really don't want to argue about the way forward—there's been too much of that already—but I'm still finding it hard to justify the complexity of this whole callback infrastructure, when we could more simply set up a fixed-size pool beforehand. Yes, we might run out of space in the pool and need to allocate more sysmem buffers, but it should be easy enough to find a pool size large enough that that doesn't happen in practice. [Ideally it would take an application leaking an entire stream of a muxed file—which has been known to happen—but in that case, it strikes me as better to eat up the Unix address space instead of the win32 address space, so the limited size is actually a good thing.] Or, failing that (or in addition to it?), maybe we could just not use the same wg_allocator object for both transforms and parsers. Yes, it's more gobject boilerplate, which is annoying, but wg_allocator is a complex beast and I really don't want to make it worse. And when it comes to output samples, the allocator and the transform really do seem to have entirely different considerations. ``` I don't think a fixed pool can work. We cannot expect the output buffers passed downstream to the application to ever be released, or, even if they probably will, I don't think we can easily and safely put them in back in the pool when they are.
1) We'd need a custom buffer implementation for each API. 2) Some API provide allocation callback which conflicts with 1) needs. 3) The allocation callbacks may block. 4) The buffers may be released after the pipeline has been shutdown, we'd need to keep it alive until all the buffers are released, which is likely going to cause problems. Another way would be to keep a list of all the allocated buffers, monitoring it for released buffers, but that doesn't sound great either. Especially with the allocation callbacks there's no way of telling whether the pipeline is the only owner. I think the `wg_allocator` callback is overall simpler, even though there's considerations to have too, especially with the blocking API callbacks, which need to be solved without blocking the reading thread or other streams allocations. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/302#note_3305
On 7/5/22 01:04, Rémi Bernon (@rbernon) wrote:
On Mon Jul 4 21:34:22 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list: ``` On 6/30/22 07:33, Rémi Bernon wrote:
From: Rémi Bernon <rbernon(a)codeweavers.com>
Using the allocator lock and replacing the transform_request_sample callback with a default wg_allocator callback.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/unix_private.h | 2 ++ dlls/winegstreamer/wg_allocator.c | 42 +++++++++++++++++++++++++++++-- dlls/winegstreamer/wg_transform.c | 24 +++--------------- 3 files changed, 46 insertions(+), 22 deletions(-)
This is making things even more complicated; now we have two entirely different code paths to set samples. I understand that adding more locks increases the mental burden, but I think this is worse—sure, we're "reusing" a lock that was going to be created anyway, but it's still effectively introducing another lock, plus now this requires us to understand when GStreamer is going to implicitly take that lock. Not to mention that this is similar to what I was trying to propose, enough that I'd like to revisit it. I really don't want to argue about the way forward—there's been too much of that already—but I'm still finding it hard to justify the complexity of this whole callback infrastructure, when we could more simply set up a fixed-size pool beforehand. Yes, we might run out of space in the pool and need to allocate more sysmem buffers, but it should be easy enough to find a pool size large enough that that doesn't happen in practice. [Ideally it would take an application leaking an entire stream of a muxed file—which has been known to happen—but in that case, it strikes me as better to eat up the Unix address space instead of the win32 address space, so the limited size is actually a good thing.] Or, failing that (or in addition to it?), maybe we could just not use the same wg_allocator object for both transforms and parsers. Yes, it's more gobject boilerplate, which is annoying, but wg_allocator is a complex beast and I really don't want to make it worse. And when it comes to output samples, the allocator and the transform really do seem to have entirely different considerations. ``` I don't think a fixed pool can work. We cannot expect the output buffers passed downstream to the application to ever be released, or, even if they probably will, I don't think we can easily and safely put them in back in the pool when they are.
Wait, we can't? I assume you mean for Media Foundation specifically, in particular because DirectShow does have pooling built into the API (and with a maximum buffer count as well). As far as I understand neither Media Foundation nor the ASF reader explicitly pool, but I'd still be surprised if a well-behaved application would never release samples. That would result in a significant memory leak.
1) We'd need a custom buffer implementation for each API.
Wait, why?
2) Some API provide allocation callback which conflicts with 1) needs.
3) The allocation callbacks may block.
They may block waiting for what?
4) The buffers may be released after the pipeline has been shutdown, we'd need to keep it alive until all the buffers are released, which is likely going to cause problems.
Why? We shouldn't need to provide anything to the Unix side except for the buffer pointer and size. I would expect that the PE buffers should outlive the unix device under normal operation.
Another way would be to keep a list of all the allocated buffers, monitoring it for released buffers, but that doesn't sound great either. Especially with the allocation callbacks there's no way of telling whether the pipeline is the only owner.
I think the `wg_allocator` callback is overall simpler, even though there's considerations to have too, especially with the blocking API callbacks, which need to be solved without blocking the reading thread or other streams allocations.
I can grudgingly accept the allocator callback, although I'm not convinced yet it's the best solution, but if it's necessary, I'd rather be consistent about using it, and I'd rather keep the sample in the wg_transform object (and ideally introduce a new lock to protect it) than what 2/5 and 3/5 do.
participants (2)
-
Rémi Bernon -
Zebediah Figura