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@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...