[PATCH v2 0/2] MR2899: winegstreamer: Only warn on wg_transform input buffer push errors.
Some GStreamer plugins such as openh264 return spurious errors when running the tests. They pass the tests successfull if we simply ignore them.
It does not make much difference with returning the error, as they are not supposed to happen anyway, and most of the time the MFT clients don't expect or handle errors.
Split from https://gitlab.winehq.org/wine/wine/-/merge_requests/2893 as this is also orthogonal and only fixes the tests when running them with openh264, which is not the case on either the testbot or Gitlab.
-- v2: winegstreamer: Set the default H264 caps profile to "baseline". winegstreamer: Only warn on wg_transform input buffer push errors.
From: Rémi Bernon rbernon@codeweavers.com
Some GStreamer plugins such as openh264 return spurious errors when running the tests. They pass the tests successfull if we simply ignore them.
It does not make much difference with returning the error, as they are not supposed to happen anyway, and most of the time the MFT clients don't expect or handle errors. --- dlls/winegstreamer/wg_transform.c | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-)
diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index f75d1b4b6df..559e8f976d2 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -740,30 +740,25 @@ static NTSTATUS read_transform_output_data(GstBuffer *buffer, GstCaps *caps, gsi
static bool get_transform_output(struct wg_transform *transform, struct wg_sample *sample) { - GstFlowReturn ret = GST_FLOW_OK; GstBuffer *input_buffer; + GstFlowReturn ret;
/* Provide the sample for transform_request_sample to pick it up */ InterlockedIncrement(&sample->refcount); InterlockedExchangePointer((void **)&transform->output_wg_sample, sample);
- while (!(transform->output_sample = gst_atomic_queue_pop(transform->output_queue))) + while (!(transform->output_sample = gst_atomic_queue_pop(transform->output_queue)) + && (input_buffer = gst_atomic_queue_pop(transform->input_queue))) { - if (!(input_buffer = gst_atomic_queue_pop(transform->input_queue))) - break; - if ((ret = gst_pad_push(transform->my_src, input_buffer))) - { - GST_ERROR("Failed to push transform input, error %d", ret); - break; - } + GST_WARNING("Failed to push transform input, error %d", ret); }
/* Remove the sample so transform_request_sample cannot use it */ if (InterlockedExchangePointer((void **)&transform->output_wg_sample, NULL)) InterlockedDecrement(&sample->refcount);
- return ret == GST_FLOW_OK; + return !!transform->output_sample; }
NTSTATUS wg_transform_read_data(void *args) @@ -778,12 +773,6 @@ NTSTATUS wg_transform_read_data(void *args) NTSTATUS status;
if (!transform->output_sample && !get_transform_output(transform, sample)) - { - wg_allocator_release_sample(transform->allocator, sample, false); - return STATUS_UNSUCCESSFUL; - } - - if (!transform->output_sample) { sample->size = 0; params->result = MF_E_TRANSFORM_NEED_MORE_INPUT;
From: Rémi Bernon rbernon@codeweavers.com
Some elements such as openh264dec require it to be present. --- dlls/winegstreamer/wg_format.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/dlls/winegstreamer/wg_format.c b/dlls/winegstreamer/wg_format.c index da0cd052804..a579a1101d1 100644 --- a/dlls/winegstreamer/wg_format.c +++ b/dlls/winegstreamer/wg_format.c @@ -571,11 +571,10 @@ static GstCaps *wg_format_to_caps_video_h264(const struct wg_format *format) GST_FIXME("H264 profile attribute %u not implemented.", format->u.video_h264.profile); /* fallthrough */ case eAVEncH264VProfile_unknown: - profile = NULL; + profile = "baseline"; break; } - if (profile) - gst_caps_set_simple(caps, "profile", G_TYPE_STRING, profile, NULL); + gst_caps_set_simple(caps, "profile", G_TYPE_STRING, profile, NULL);
switch (format->u.video_h264.level) {
v2: Add a change to always set h264 caps "profile", which is what openh264 complains about. I'm keeping the initial change as I don't think it helps anything to return the errors here, and this shows that it can still work if we ignore them.
I don't think we want patch 1/2. Returning an error from the chain function means that the data wasn't actually consumed. If an element consumes the data and returns an error anyway, that's a bug in that element. Given that 2/2 fixes the interaction in a way that seems correct, I don't think we want to work around it in a worse way.
I don't think that's true. Some elements return "error" statuses in some very specific case, while at the same time consuming the input. That's the case for instance with qtdemux, which returns GST_FLOW_EOS when the last part of a file is pushed, while it still demuxes all the sample it could from it.
There is no point returning the error to the client. It doesn't expect it, and doesn't handle it anyway. From its point of view, the input has been accepted, and thus consumed by the ProcessInput call, and everything is valid.
This is also not silently swallowing the error as it still prints a warning (which arguably could be raised to error level, but I'm not even sure it's worth it).
Okay, this strikes me as poor design on GStreamer's part, or at least poor documentation.
It's perhaps worth mentioning that pretty much all GStreamer code treats chain errors as at least a reason to stop pushing data and propagate the error. This commit does the opposite of that, which doesn't seem great. I can see an argument for not propagating errors to the PE side, at least, but we should probably leave the break in there.
This merge request was approved by Zebediah Figura.
-
Rémi Bernon -
Rémi Bernon (＠rbernon)
-
Zebediah Figura (＠zfigura)