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@codeweavers.com ---
(This need to come first, before 231660-231665 as the spurious todo_wine is caused by the H264 format being synchronously rejected when libav plugins are used)
In this patch I assumed that sink callbacks are always called from the same thread, and then do not require any locking around sink_caps accesses.
The output samples will each hold their own ref on the caps, eventually different ones as the stream format changes, and wg_transform_read_data will only read the caps from the sample it unqueued.
dlls/winegstreamer/unixlib.h | 1 + dlls/winegstreamer/wg_transform.c | 89 ++++++++++++++++++++++++------- 2 files changed, 71 insertions(+), 19 deletions(-)
diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index f4e2ea4966b..7a31020fb87 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -122,6 +122,7 @@ struct wg_sample UINT32 max_size; UINT32 size; BYTE *data; + struct wg_format *format; };
struct wg_parser_buffer diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 0327b92ce8e..ddf86de13ae 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -51,40 +51,73 @@ struct wg_transform GstBufferList *input; guint input_max_length; GstAtomicQueue *output_queue; - GstBuffer *output_buffer; + GstSample *output_sample; + GstCaps *sink_caps; };
static GstFlowReturn transform_sink_chain_cb(GstPad *pad, GstObject *parent, GstBuffer *buffer) { struct wg_transform *transform = gst_pad_get_element_private(pad); + GstSample *sample;
GST_LOG("transform %p, buffer %p.", transform, buffer);
- gst_atomic_queue_push(transform->output_queue, buffer); + if ((sample = gst_sample_new(buffer, transform->sink_caps, NULL, NULL))) + gst_atomic_queue_push(transform->output_queue, sample);
- return GST_FLOW_OK; + gst_buffer_unref(buffer); + + return sample ? GST_FLOW_OK : GST_FLOW_ERROR; +} + +static gboolean transform_sink_event_cb(GstPad *pad, GstObject *parent, GstEvent *event) +{ + struct wg_transform *transform = gst_pad_get_element_private(pad); + + GST_LOG("transform %p, type "%s".", transform, GST_EVENT_TYPE_NAME(event)); + + switch (event->type) + { + case GST_EVENT_CAPS: + { + GstCaps *caps; + + gst_event_parse_caps(event, &caps); + + gst_caps_unref(transform->sink_caps); + transform->sink_caps = gst_caps_ref(caps); + break; + } + default: + GST_WARNING("Ignoring "%s" event.", GST_EVENT_TYPE_NAME(event)); + break; + } + + gst_event_unref(event); + return TRUE; }
NTSTATUS wg_transform_destroy(void *args) { struct wg_transform *transform = args; - GstBuffer *buffer; + GstSample *sample;
if (transform->input) gst_buffer_list_unref(transform->input);
gst_element_set_state(transform->container, GST_STATE_NULL);
- if (transform->output_buffer) - gst_buffer_unref(transform->output_buffer); - while ((buffer = gst_atomic_queue_pop(transform->output_queue))) - gst_buffer_unref(buffer); + if (transform->output_sample) + gst_sample_unref(transform->output_sample); + while ((sample = gst_atomic_queue_pop(transform->output_queue))) + gst_sample_unref(sample);
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); + gst_caps_unref(transform->sink_caps); gst_atomic_queue_unref(transform->output_queue); free(transform);
@@ -162,10 +195,10 @@ static bool transform_append_element(struct wg_transform *transform, GstElement NTSTATUS wg_transform_create(void *args) { struct wg_transform_create_params *params = args; - GstCaps *raw_caps = NULL, *src_caps = NULL, *sink_caps = NULL; struct wg_format output_format = *params->output_format; struct wg_format input_format = *params->input_format; GstElement *first = NULL, *last = NULL, *element; + GstCaps *raw_caps = NULL, *src_caps = NULL; NTSTATUS status = STATUS_UNSUCCESSFUL; GstPadTemplate *template = NULL; struct wg_transform *transform; @@ -194,9 +227,9 @@ NTSTATUS wg_transform_create(void *args) if (!transform->my_src) goto out;
- if (!(sink_caps = wg_format_to_caps(&output_format))) + if (!(transform->sink_caps = wg_format_to_caps(&output_format))) goto out; - if (!(template = gst_pad_template_new("sink", GST_PAD_SINK, GST_PAD_ALWAYS, sink_caps))) + if (!(template = gst_pad_template_new("sink", GST_PAD_SINK, GST_PAD_ALWAYS, transform->sink_caps))) goto out; transform->my_sink = gst_pad_new_from_template(template, "sink"); g_object_unref(template); @@ -204,13 +237,14 @@ NTSTATUS wg_transform_create(void *args) goto out;
gst_pad_set_element_private(transform->my_sink, transform); + gst_pad_set_event_function(transform->my_sink, transform_sink_event_cb); gst_pad_set_chain_function(transform->my_sink, transform_sink_chain_cb);
/* Since we append conversion elements, we don't want to filter decoders * based on the actual output caps now. Matching decoders with the * raw output media type should be enough. */ - media_type = gst_structure_get_name(gst_caps_get_structure(sink_caps, 0)); + media_type = gst_structure_get_name(gst_caps_get_structure(transform->sink_caps, 0)); if (!(raw_caps = gst_caps_new_empty_simple(media_type))) goto out;
@@ -264,6 +298,11 @@ NTSTATUS wg_transform_create(void *args) break;
case WG_MAJOR_TYPE_VIDEO: + if (!(element = create_element("videoconvert", "base")) + || !transform_append_element(transform, element, &first, &last)) + goto out; + /* Let GStreamer choose a default number of threads. */ + gst_util_set_object_arg(G_OBJECT(element), "n-threads", "0"); break;
case WG_MAJOR_TYPE_H264: @@ -306,7 +345,6 @@ NTSTATUS wg_transform_create(void *args) || !gst_pad_push_event(transform->my_src, event)) goto out;
- gst_caps_unref(sink_caps); gst_caps_unref(src_caps);
GST_INFO("Created winegstreamer transform %p.", transform); @@ -320,8 +358,8 @@ out: gst_object_unref(transform->their_src); if (transform->my_sink) gst_object_unref(transform->my_sink); - if (sink_caps) - gst_caps_unref(sink_caps); + if (transform->sink_caps) + gst_caps_unref(transform->sink_caps); if (transform->my_src) gst_object_unref(transform->my_src); if (src_caps) @@ -401,8 +439,10 @@ NTSTATUS wg_transform_read_data(void *args) struct wg_transform *transform = params->transform; struct wg_sample *sample = params->sample; GstBufferList *input = transform->input; + struct wg_format format; GstFlowReturn ret; NTSTATUS status; + GstCaps *caps;
if (!gst_buffer_list_length(transform->input)) GST_DEBUG("Not input buffer queued"); @@ -417,7 +457,7 @@ NTSTATUS wg_transform_read_data(void *args) return STATUS_UNSUCCESSFUL; }
- if (!transform->output_buffer && !(transform->output_buffer = gst_atomic_queue_pop(transform->output_queue))) + if (!transform->output_sample && !(transform->output_sample = gst_atomic_queue_pop(transform->output_queue))) { sample->size = 0; params->result = MF_E_TRANSFORM_NEED_MORE_INPUT; @@ -425,7 +465,18 @@ NTSTATUS wg_transform_read_data(void *args) return STATUS_SUCCESS; }
- if ((status = read_transform_output_data(transform->output_buffer, sample))) + if (sample->format && (caps = gst_sample_get_caps(transform->output_sample))) + { + wg_format_from_caps(&format, caps); + if (!wg_format_compare(&format, sample->format)) + { + *sample->format = format; + params->result = MF_E_TRANSFORM_STREAM_CHANGE; + return STATUS_SUCCESS; + } + } + + if ((status = read_transform_output_data(gst_sample_get_buffer(transform->output_sample), sample))) { sample->size = 0; return status; @@ -433,8 +484,8 @@ NTSTATUS wg_transform_read_data(void *args)
if (!(sample->flags & WG_SAMPLE_FLAG_INCOMPLETE)) { - gst_buffer_unref(transform->output_buffer); - transform->output_buffer = NULL; + gst_sample_unref(transform->output_sample); + transform->output_sample = NULL; }
params->result = S_OK;
To support formats with no forced width/height in the H264 transform.
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@codeweavers.com --- dlls/winegstreamer/wg_format.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/dlls/winegstreamer/wg_format.c b/dlls/winegstreamer/wg_format.c index b4455a6e6d0..1c3c8e94f5c 100644 --- a/dlls/winegstreamer/wg_format.c +++ b/dlls/winegstreamer/wg_format.c @@ -390,6 +390,10 @@ static GstCaps *wg_format_to_caps_video(const struct wg_format *format) { gst_structure_remove_fields(gst_caps_get_structure(caps, i), "framerate", "pixel-aspect-ratio", "colorimetry", "chroma-site", NULL); + if (!format->u.video.width) + gst_structure_remove_fields(gst_caps_get_structure(caps, i), "width", NULL); + if (!format->u.video.height) + gst_structure_remove_fields(gst_caps_get_structure(caps, i), "height", NULL); } } return caps;
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@codeweavers.com --- dlls/mf/tests/mf.c | 7 +--- dlls/winegstreamer/h264_decoder.c | 59 ++++++++++++++++++++++++++++--- 2 files changed, 55 insertions(+), 11 deletions(-)
diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c index 31a84e47bc1..e7d8de649ef 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -6868,19 +6868,15 @@ static void test_h264_decoder(void) ok(i == 2, "got %lu iterations\n", i); todo_wine ok(h264_encoded_data_len == 48194, "got h264_encoded_data_len %lu\n", h264_encoded_data_len); - todo_wine ok(hr == MF_E_TRANSFORM_STREAM_CHANGE, "ProcessOutput returned %#lx\n", hr); ok(output.dwStreamID == 0, "got dwStreamID %lu\n", output.dwStreamID); ok(!!output.pSample, "got pSample %p\n", output.pSample); - todo_wine ok(output.dwStatus == MFT_OUTPUT_DATA_BUFFER_FORMAT_CHANGE, "got dwStatus %#lx\n", output.dwStatus); ok(!output.pEvents, "got pEvents %p\n", output.pEvents); - todo_wine ok(status == MFT_PROCESS_OUTPUT_STATUS_NEW_STREAMS, "got status %#lx\n", status); - if (status == MFT_PROCESS_OUTPUT_STATUS_NEW_STREAMS) - check_sample(output.pSample, NULL, 0, NULL); + check_sample(output.pSample, NULL, 0, NULL); ret = IMFSample_Release(output.pSample); ok(ret == 0, "Release returned %lu\n", ret);
@@ -6922,7 +6918,6 @@ static void test_h264_decoder(void) memset(&output, 0, sizeof(output)); output.pSample = create_sample(NULL, actual_width * actual_height * 2); hr = IMFTransform_ProcessOutput(transform, 0, 1, &output, &status); - todo_wine ok(hr == S_OK, "ProcessOutput returned %#lx\n", hr); ok(output.dwStreamID == 0, "got dwStreamID %lu\n", output.dwStreamID); ok(!!output.pSample, "got pSample %p\n", output.pSample); diff --git a/dlls/winegstreamer/h264_decoder.c b/dlls/winegstreamer/h264_decoder.c index 8bfa15529db..2fbbefd1e2d 100644 --- a/dlls/winegstreamer/h264_decoder.c +++ b/dlls/winegstreamer/h264_decoder.c @@ -50,6 +50,7 @@ struct h264_decoder IMFMediaType *output_type;
struct wg_transform *wg_transform; + struct wg_format output_format; };
static struct h264_decoder *impl_from_IMFTransform(IMFTransform *iface) @@ -60,7 +61,6 @@ static struct h264_decoder *impl_from_IMFTransform(IMFTransform *iface) static HRESULT try_create_wg_transform(struct h264_decoder *decoder) { struct wg_format input_format; - struct wg_format output_format;
if (decoder->wg_transform) wg_transform_destroy(decoder->wg_transform); @@ -70,11 +70,18 @@ static HRESULT try_create_wg_transform(struct h264_decoder *decoder) if (input_format.major_type == WG_MAJOR_TYPE_UNKNOWN) return MF_E_INVALIDMEDIATYPE;
- mf_media_type_to_wg_format(decoder->output_type, &output_format); - if (output_format.major_type == WG_MAJOR_TYPE_UNKNOWN) + mf_media_type_to_wg_format(decoder->output_type, &decoder->output_format); + if (decoder->output_format.major_type == WG_MAJOR_TYPE_UNKNOWN) return MF_E_INVALIDMEDIATYPE;
- if (!(decoder->wg_transform = wg_transform_create(&input_format, &output_format))) + /* Don't force any output frame size, H264 streams already have the + * metadata for it and will generate a MF_E_TRANSFORM_STREAM_CHANGE + * result later. + */ + decoder->output_format.u.video.width = 0; + decoder->output_format.u.video.height = 0; + + if (!(decoder->wg_transform = wg_transform_create(&input_format, &decoder->output_format))) return E_FAIL;
return S_OK; @@ -369,7 +376,7 @@ static HRESULT WINAPI transform_GetOutputAvailableType(IMFTransform *iface, DWOR if (FAILED(hr = IMFMediaType_SetGUID(media_type, &MF_MT_SUBTYPE, output_type))) goto done;
- hr = fill_output_media_type(media_type, NULL); + hr = fill_output_media_type(media_type, decoder->output_type);
done: if (SUCCEEDED(hr)) @@ -418,6 +425,7 @@ static HRESULT WINAPI transform_SetOutputType(IMFTransform *iface, DWORD id, IMF { struct h264_decoder *decoder = impl_from_IMFTransform(iface); GUID major, subtype; + BOOL identical; HRESULT hr; ULONG i;
@@ -440,7 +448,14 @@ static HRESULT WINAPI transform_SetOutputType(IMFTransform *iface, DWORD id, IMF return MF_E_INVALIDMEDIATYPE;
if (decoder->output_type) + { + fill_output_media_type(decoder->output_type, NULL); + if (SUCCEEDED(hr = IMFMediaType_Compare(decoder->output_type, (IMFAttributes *)type, + MF_ATTRIBUTES_MATCH_THEIR_ITEMS, &identical)) && identical) + return S_OK; IMFMediaType_Release(decoder->output_type); + } + IMFMediaType_AddRef((decoder->output_type = type));
if (FAILED(hr = try_create_wg_transform(decoder))) @@ -490,7 +505,23 @@ static HRESULT WINAPI transform_ProcessEvent(IMFTransform *iface, DWORD id, IMFM
static HRESULT WINAPI transform_ProcessMessage(IMFTransform *iface, MFT_MESSAGE_TYPE message, ULONG_PTR param) { + struct h264_decoder *decoder = impl_from_IMFTransform(iface); + FIXME("iface %p, message %#x, param %Ix stub!\n", iface, message, param); + + switch (message) + { + case MFT_MESSAGE_NOTIFY_BEGIN_STREAMING: + /* Call of Duty Black Ops 3 expects to receive an MF_E_TRANSFORM_STREAM_CHANGE result again + * after it has called ProcessMessage with BEGIN_STREAMING. We could destroy and re-create + * the wg_transform instead but resetting the output format should be enough. + */ + memset(&decoder->output_format, 0, sizeof(decoder->output_format)); + break; + default: + break; + } + return S_OK; }
@@ -524,6 +555,8 @@ static HRESULT WINAPI transform_ProcessOutput(IMFTransform *iface, DWORD flags, struct h264_decoder *decoder = impl_from_IMFTransform(iface); MFT_OUTPUT_STREAM_INFO info; struct wg_sample *wg_sample; + IMFMediaType *media_type; + UINT64 frame_rate; HRESULT hr;
TRACE("iface %p, flags %#lx, count %lu, samples %p, status %p.\n", iface, flags, count, samples, status); @@ -537,6 +570,9 @@ static HRESULT WINAPI transform_ProcessOutput(IMFTransform *iface, DWORD flags, if (!decoder->wg_transform) return MF_E_TRANSFORM_TYPE_NOT_SET;
+ if (FAILED(IMFMediaType_GetUINT64(decoder->output_type, &MF_MT_FRAME_RATE, &frame_rate))) + frame_rate = (UINT64)30000 << 32 | 1001; + *status = 0; samples[0].dwStatus = 0; if (!samples[0].pSample) return E_INVALIDARG; @@ -544,11 +580,24 @@ static HRESULT WINAPI transform_ProcessOutput(IMFTransform *iface, DWORD flags, if (FAILED(hr = mf_create_wg_sample(samples[0].pSample, &wg_sample))) return hr;
+ wg_sample->format = &decoder->output_format; if (wg_sample->max_size < info.cbSize) hr = MF_E_BUFFERTOOSMALL; else hr = wg_transform_read_data(decoder->wg_transform, wg_sample);
+ if (hr == MF_E_TRANSFORM_STREAM_CHANGE) + { + media_type = mf_media_type_from_wg_format(&decoder->output_format); + IMFMediaType_SetUINT64(media_type, &MF_MT_FRAME_RATE, frame_rate); + + IMFMediaType_Release(decoder->output_type); + decoder->output_type = media_type; + + samples[0].dwStatus |= MFT_OUTPUT_DATA_BUFFER_FORMAT_CHANGE; + *status |= MFT_OUTPUT_DATA_BUFFER_FORMAT_CHANGE; + } + mf_destroy_wg_sample(wg_sample); return hr; }
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=111958
Your paranoid android.
=== build (build log) ===
error: patch failed: dlls/winegstreamer/wg_transform.c:51 error: patch failed: dlls/mf/tests/mf.c:6868 error: patch failed: dlls/winegstreamer/h264_decoder.c:490 Task: Patch failed to apply
On 4/5/22 07:26, Rémi Bernon wrote:
@@ -70,11 +70,18 @@ static HRESULT try_create_wg_transform(struct h264_decoder *decoder) if (input_format.major_type == WG_MAJOR_TYPE_UNKNOWN) return MF_E_INVALIDMEDIATYPE;
- mf_media_type_to_wg_format(decoder->output_type, &output_format);
- if (output_format.major_type == WG_MAJOR_TYPE_UNKNOWN)
- mf_media_type_to_wg_format(decoder->output_type, &decoder->output_format);
- if (decoder->output_format.major_type == WG_MAJOR_TYPE_UNKNOWN) return MF_E_INVALIDMEDIATYPE;
- if (!(decoder->wg_transform = wg_transform_create(&input_format, &output_format)))
- /* Don't force any output frame size, H264 streams already have the
* metadata for it and will generate a MF_E_TRANSFORM_STREAM_CHANGE* result later.*/- decoder->output_format.u.video.width = 0;
- decoder->output_format.u.video.height = 0;
This seems like it's orthogonal to dynamic format change.
It also leads me to wonder: do we need the output_type field at all if we're going to have decoder->output_format?
if (!(decoder->wg_transform = wg_transform_create(&input_format, &decoder->output_format))) return E_FAIL;
return S_OK;
@@ -369,7 +376,7 @@ static HRESULT WINAPI transform_GetOutputAvailableType(IMFTransform *iface, DWOR if (FAILED(hr = IMFMediaType_SetGUID(media_type, &MF_MT_SUBTYPE, output_type))) goto done;
- hr = fill_output_media_type(media_type, NULL);
hr = fill_output_media_type(media_type, decoder->output_type);
done: if (SUCCEEDED(hr))
This also seems like it's orthogonal to dynamic format change. (Is it possible to write tests for this?)
@@ -418,6 +425,7 @@ static HRESULT WINAPI transform_SetOutputType(IMFTransform *iface, DWORD id, IMF { struct h264_decoder *decoder = impl_from_IMFTransform(iface); GUID major, subtype;
- BOOL identical; HRESULT hr; ULONG i;
@@ -440,7 +448,14 @@ static HRESULT WINAPI transform_SetOutputType(IMFTransform *iface, DWORD id, IMF return MF_E_INVALIDMEDIATYPE;
if (decoder->output_type)
- {
fill_output_media_type(decoder->output_type, NULL);if (SUCCEEDED(hr = IMFMediaType_Compare(decoder->output_type, (IMFAttributes *)type,MF_ATTRIBUTES_MATCH_THEIR_ITEMS, &identical)) && identical)return S_OK;
So we require the user to specify meaningless 29.97 frame rate (and so on), and reject different real values?
This is awfully surprising behaviour; do we have tests for it?
IMFMediaType_Release(decoder->output_type);
}
IMFMediaType_AddRef((decoder->output_type = type)); if (FAILED(hr = try_create_wg_transform(decoder)))@@ -490,7 +505,23 @@ static HRESULT WINAPI transform_ProcessEvent(IMFTransform *iface, DWORD id, IMFM
static HRESULT WINAPI transform_ProcessMessage(IMFTransform *iface, MFT_MESSAGE_TYPE message, ULONG_PTR param) {
- struct h264_decoder *decoder = impl_from_IMFTransform(iface);
FIXME("iface %p, message %#x, param %Ix stub!\n", iface, message, param);- switch (message)
- {
- case MFT_MESSAGE_NOTIFY_BEGIN_STREAMING:
/* Call of Duty Black Ops 3 expects to receive an MF_E_TRANSFORM_STREAM_CHANGE result again* after it has called ProcessMessage with BEGIN_STREAMING. We could destroy and re-create* the wg_transform instead but resetting the output format should be enough.*/memset(&decoder->output_format, 0, sizeof(decoder->output_format));break;- default:
break;- }
return S_OK;@@ -524,6 +555,8 @@ static HRESULT WINAPI transform_ProcessOutput(IMFTransform *iface, DWORD flags, struct h264_decoder *decoder = impl_from_IMFTransform(iface); MFT_OUTPUT_STREAM_INFO info; struct wg_sample *wg_sample;
IMFMediaType *media_type;
UINT64 frame_rate; HRESULT hr;
TRACE("iface %p, flags %#lx, count %lu, samples %p, status %p.\n", iface, flags, count, samples, status);
@@ -537,6 +570,9 @@ static HRESULT WINAPI transform_ProcessOutput(IMFTransform *iface, DWORD flags, if (!decoder->wg_transform) return MF_E_TRANSFORM_TYPE_NOT_SET;
- if (FAILED(IMFMediaType_GetUINT64(decoder->output_type, &MF_MT_FRAME_RATE, &frame_rate)))
frame_rate = (UINT64)30000 << 32 | 1001;*status = 0; samples[0].dwStatus = 0; if (!samples[0].pSample) return E_INVALIDARG;@@ -544,11 +580,24 @@ static HRESULT WINAPI transform_ProcessOutput(IMFTransform *iface, DWORD flags, if (FAILED(hr = mf_create_wg_sample(samples[0].pSample, &wg_sample))) return hr;
wg_sample->format = &decoder->output_format; if (wg_sample->max_size < info.cbSize) hr = MF_E_BUFFERTOOSMALL; else hr = wg_transform_read_data(decoder->wg_transform, wg_sample);
if (hr == MF_E_TRANSFORM_STREAM_CHANGE)
{
media_type = mf_media_type_from_wg_format(&decoder->output_format);IMFMediaType_SetUINT64(media_type, &MF_MT_FRAME_RATE, frame_rate);
So if the user never requests a specific frame rate, then we always force it to 29.97? And we ignore dynamic frame rate changes?
This is also surprising; do we have tests for these?
IMFMediaType_Release(decoder->output_type);decoder->output_type = media_type;samples[0].dwStatus |= MFT_OUTPUT_DATA_BUFFER_FORMAT_CHANGE;*status |= MFT_OUTPUT_DATA_BUFFER_FORMAT_CHANGE;- }
mf_destroy_wg_sample(wg_sample); return hr;
On 4/6/22 00:11, Zebediah Figura wrote:
On 4/5/22 07:26, Rémi Bernon wrote:
@@ -70,11 +70,18 @@ static HRESULT try_create_wg_transform(struct h264_decoder *decoder) if (input_format.major_type == WG_MAJOR_TYPE_UNKNOWN) return MF_E_INVALIDMEDIATYPE; - mf_media_type_to_wg_format(decoder->output_type, &output_format); - if (output_format.major_type == WG_MAJOR_TYPE_UNKNOWN) + mf_media_type_to_wg_format(decoder->output_type, &decoder->output_format); + if (decoder->output_format.major_type == WG_MAJOR_TYPE_UNKNOWN) return MF_E_INVALIDMEDIATYPE; - if (!(decoder->wg_transform = wg_transform_create(&input_format, &output_format))) + /* Don't force any output frame size, H264 streams already have the + * metadata for it and will generate a MF_E_TRANSFORM_STREAM_CHANGE + * result later. + */ + decoder->output_format.u.video.width = 0; + decoder->output_format.u.video.height = 0;
This seems like it's orthogonal to dynamic format change.
It also leads me to wonder: do we need the output_type field at all if we're going to have decoder->output_format?
Maybe not completely useful, but I think it makes the MF API implementation easier. It will also later hold information which is not present in the wg_format struct and doesn't need to be, such as video frame aperture for proper NV12 plane alignment and frame crop.
+ if (!(decoder->wg_transform = wg_transform_create(&input_format, &decoder->output_format))) return E_FAIL; return S_OK; @@ -369,7 +376,7 @@ static HRESULT WINAPI transform_GetOutputAvailableType(IMFTransform *iface, DWOR if (FAILED(hr = IMFMediaType_SetGUID(media_type, &MF_MT_SUBTYPE, output_type))) goto done; - hr = fill_output_media_type(media_type, NULL); + hr = fill_output_media_type(media_type, decoder->output_type); done: if (SUCCEEDED(hr))
This also seems like it's orthogonal to dynamic format change. (Is it possible to write tests for this?)
I don't think it's completely orthogonal, and there are tests already, checking the new output types after a format change.
I can add more tests to see what is enumerated after an output type has been selected but TBH I don't think it really matters, none of the games I saw using this transform seem to really care about what is enumerated there.
@@ -418,6 +425,7 @@ static HRESULT WINAPI transform_SetOutputType(IMFTransform *iface, DWORD id, IMF { struct h264_decoder *decoder = impl_from_IMFTransform(iface); GUID major, subtype; + BOOL identical; HRESULT hr; ULONG i; @@ -440,7 +448,14 @@ static HRESULT WINAPI transform_SetOutputType(IMFTransform *iface, DWORD id, IMF return MF_E_INVALIDMEDIATYPE; if (decoder->output_type) + { + fill_output_media_type(decoder->output_type, NULL); + if (SUCCEEDED(hr = IMFMediaType_Compare(decoder->output_type, (IMFAttributes *)type, + MF_ATTRIBUTES_MATCH_THEIR_ITEMS, &identical)) && identical) + return S_OK;
So we require the user to specify meaningless 29.97 frame rate (and so on), and reject different real values?
This is awfully surprising behaviour; do we have tests for it?
This is just checking that the new output type is the same as the one we just had from the stream format change, and that the transform doesn't need to be re-created.
AFAIU transform clients are supposed to enumerate types on stream format change and set the output type again. Most games simply call GetOutputAvailableType, pick the first one and pass it directly to SetOutputType without even looking at it. Though they keep a lot of expectations about that type, and most notably that it's NV12 with the same plane alignment as native.
We don't have that much level of detail in the tests. I can probably add more, though I'm not sure it's really interesting to implement that level of checking (as in requiring clients to enumerate types again, etc).
IMFMediaType_Release(decoder->output_type); + }
IMFMediaType_AddRef((decoder->output_type = type)); if (FAILED(hr = try_create_wg_transform(decoder))) @@ -490,7 +505,23 @@ static HRESULT WINAPI transform_ProcessEvent(IMFTransform *iface, DWORD id, IMFM static HRESULT WINAPI transform_ProcessMessage(IMFTransform *iface, MFT_MESSAGE_TYPE message, ULONG_PTR param) { + struct h264_decoder *decoder = impl_from_IMFTransform(iface);
FIXME("iface %p, message %#x, param %Ix stub!\n", iface, message, param);
+ switch (message) + { + case MFT_MESSAGE_NOTIFY_BEGIN_STREAMING: + /* Call of Duty Black Ops 3 expects to receive an MF_E_TRANSFORM_STREAM_CHANGE result again + * after it has called ProcessMessage with BEGIN_STREAMING. We could destroy and re-create + * the wg_transform instead but resetting the output format should be enough. + */ + memset(&decoder->output_format, 0, sizeof(decoder->output_format)); + break; + default: + break; + }
return S_OK; } @@ -524,6 +555,8 @@ static HRESULT WINAPI transform_ProcessOutput(IMFTransform *iface, DWORD flags, struct h264_decoder *decoder = impl_from_IMFTransform(iface); MFT_OUTPUT_STREAM_INFO info; struct wg_sample *wg_sample; + IMFMediaType *media_type; + UINT64 frame_rate; HRESULT hr; TRACE("iface %p, flags %#lx, count %lu, samples %p, status %p.\n", iface, flags, count, samples, status); @@ -537,6 +570,9 @@ static HRESULT WINAPI transform_ProcessOutput(IMFTransform *iface, DWORD flags, if (!decoder->wg_transform) return MF_E_TRANSFORM_TYPE_NOT_SET; + if (FAILED(IMFMediaType_GetUINT64(decoder->output_type, &MF_MT_FRAME_RATE, &frame_rate))) + frame_rate = (UINT64)30000 << 32 | 1001;
*status = 0; samples[0].dwStatus = 0; if (!samples[0].pSample) return E_INVALIDARG; @@ -544,11 +580,24 @@ static HRESULT WINAPI transform_ProcessOutput(IMFTransform *iface, DWORD flags, if (FAILED(hr = mf_create_wg_sample(samples[0].pSample, &wg_sample))) return hr; + wg_sample->format = &decoder->output_format; if (wg_sample->max_size < info.cbSize) hr = MF_E_BUFFERTOOSMALL; else hr = wg_transform_read_data(decoder->wg_transform, wg_sample); + if (hr == MF_E_TRANSFORM_STREAM_CHANGE) + { + media_type = mf_media_type_from_wg_format(&decoder->output_format); + IMFMediaType_SetUINT64(media_type, &MF_MT_FRAME_RATE, frame_rate);
So if the user never requests a specific frame rate, then we always force it to 29.97? And we ignore dynamic frame rate changes?
This is also surprising; do we have tests for these?
We have tests that this is the default frame rate, yes.
Then I think the reason why this keeps and restore the frame rate is because GStreamer output caps did not convey the frame rate, and we then had a 1:1 frame rate instead.
On 4/5/22 18:05, Rémi Bernon wrote:
On 4/6/22 00:11, Zebediah Figura wrote:
On 4/5/22 07:26, Rémi Bernon wrote:
@@ -70,11 +70,18 @@ static HRESULT try_create_wg_transform(struct h264_decoder *decoder) if (input_format.major_type == WG_MAJOR_TYPE_UNKNOWN) return MF_E_INVALIDMEDIATYPE; - mf_media_type_to_wg_format(decoder->output_type, &output_format); - if (output_format.major_type == WG_MAJOR_TYPE_UNKNOWN) + mf_media_type_to_wg_format(decoder->output_type, &decoder->output_format); + if (decoder->output_format.major_type == WG_MAJOR_TYPE_UNKNOWN) return MF_E_INVALIDMEDIATYPE; - if (!(decoder->wg_transform = wg_transform_create(&input_format, &output_format))) + /* Don't force any output frame size, H264 streams already have the + * metadata for it and will generate a MF_E_TRANSFORM_STREAM_CHANGE + * result later. + */ + decoder->output_format.u.video.width = 0; + decoder->output_format.u.video.height = 0;
This seems like it's orthogonal to dynamic format change.
It also leads me to wonder: do we need the output_type field at all if we're going to have decoder->output_format?
Maybe not completely useful, but I think it makes the MF API implementation easier. It will also later hold information which is not present in the wg_format struct and doesn't need to be, such as video frame aperture for proper NV12 plane alignment and frame crop.
Okay, it wasn't clear to me as-is since it was only used in one place (and since IMFMediaType is not the easiest thing to work with). But extra members, plus 231873, are probably good arguments for keeping it around.
+ if (!(decoder->wg_transform = wg_transform_create(&input_format, &decoder->output_format))) return E_FAIL; return S_OK; @@ -369,7 +376,7 @@ static HRESULT WINAPI transform_GetOutputAvailableType(IMFTransform *iface, DWOR if (FAILED(hr = IMFMediaType_SetGUID(media_type, &MF_MT_SUBTYPE, output_type))) goto done; - hr = fill_output_media_type(media_type, NULL); + hr = fill_output_media_type(media_type, decoder->output_type); done: if (SUCCEEDED(hr))
This also seems like it's orthogonal to dynamic format change. (Is it possible to write tests for this?)
I don't think it's completely orthogonal, and there are tests already, checking the new output types after a format change.
Right, I missed those the first time...
I think I see that this isn't orthogonal to dynamic format change, but now I'm left wondering why those tests aren't fixed by this patch. We should be reporting the real width and height now, right?
I can add more tests to see what is enumerated after an output type has been selected but TBH I don't think it really matters, none of the games I saw using this transform seem to really care about what is enumerated there.
@@ -418,6 +425,7 @@ static HRESULT WINAPI transform_SetOutputType(IMFTransform *iface, DWORD id, IMF { struct h264_decoder *decoder = impl_from_IMFTransform(iface); GUID major, subtype; + BOOL identical; HRESULT hr; ULONG i; @@ -440,7 +448,14 @@ static HRESULT WINAPI transform_SetOutputType(IMFTransform *iface, DWORD id, IMF return MF_E_INVALIDMEDIATYPE; if (decoder->output_type) + { + fill_output_media_type(decoder->output_type, NULL); + if (SUCCEEDED(hr = IMFMediaType_Compare(decoder->output_type, (IMFAttributes *)type, + MF_ATTRIBUTES_MATCH_THEIR_ITEMS, &identical)) && identical) + return S_OK;
So we require the user to specify meaningless 29.97 frame rate (and so on), and reject different real values?
This is awfully surprising behaviour; do we have tests for it?
This is just checking that the new output type is the same as the one we just had from the stream format change, and that the transform doesn't need to be re-created.
Right, I once again can't read ._.
I guess the structure here is kind of weird, but it's not immediately clear to me how to improve it.
AFAIU transform clients are supposed to enumerate types on stream format change and set the output type again. Most games simply call GetOutputAvailableType, pick the first one and pass it directly to SetOutputType without even looking at it. Though they keep a lot of expectations about that type, and most notably that it's NV12 with the same plane alignment as native.
We don't have that much level of detail in the tests. I can probably add more, though I'm not sure it's really interesting to implement that level of checking (as in requiring clients to enumerate types again, etc).
IMFMediaType_Release(decoder->output_type); + }
IMFMediaType_AddRef((decoder->output_type = type)); if (FAILED(hr = try_create_wg_transform(decoder))) @@ -490,7 +505,23 @@ static HRESULT WINAPI transform_ProcessEvent(IMFTransform *iface, DWORD id, IMFM static HRESULT WINAPI transform_ProcessMessage(IMFTransform *iface, MFT_MESSAGE_TYPE message, ULONG_PTR param) { + struct h264_decoder *decoder = impl_from_IMFTransform(iface);
FIXME("iface %p, message %#x, param %Ix stub!\n", iface, message, param);
+ switch (message) + { + case MFT_MESSAGE_NOTIFY_BEGIN_STREAMING: + /* Call of Duty Black Ops 3 expects to receive an MF_E_TRANSFORM_STREAM_CHANGE result again + * after it has called ProcessMessage with BEGIN_STREAMING. We could destroy and re-create + * the wg_transform instead but resetting the output format should be enough. + */ + memset(&decoder->output_format, 0, sizeof(decoder->output_format)); + break; + default: + break; + }
return S_OK; } @@ -524,6 +555,8 @@ static HRESULT WINAPI transform_ProcessOutput(IMFTransform *iface, DWORD flags, struct h264_decoder *decoder = impl_from_IMFTransform(iface); MFT_OUTPUT_STREAM_INFO info; struct wg_sample *wg_sample; + IMFMediaType *media_type; + UINT64 frame_rate; HRESULT hr; TRACE("iface %p, flags %#lx, count %lu, samples %p, status %p.\n", iface, flags, count, samples, status); @@ -537,6 +570,9 @@ static HRESULT WINAPI transform_ProcessOutput(IMFTransform *iface, DWORD flags, if (!decoder->wg_transform) return MF_E_TRANSFORM_TYPE_NOT_SET; + if (FAILED(IMFMediaType_GetUINT64(decoder->output_type, &MF_MT_FRAME_RATE, &frame_rate))) + frame_rate = (UINT64)30000 << 32 | 1001;
*status = 0; samples[0].dwStatus = 0; if (!samples[0].pSample) return E_INVALIDARG; @@ -544,11 +580,24 @@ static HRESULT WINAPI transform_ProcessOutput(IMFTransform *iface, DWORD flags, if (FAILED(hr = mf_create_wg_sample(samples[0].pSample, &wg_sample))) return hr; + wg_sample->format = &decoder->output_format; if (wg_sample->max_size < info.cbSize) hr = MF_E_BUFFERTOOSMALL; else hr = wg_transform_read_data(decoder->wg_transform, wg_sample); + if (hr == MF_E_TRANSFORM_STREAM_CHANGE) + { + media_type = mf_media_type_from_wg_format(&decoder->output_format); + IMFMediaType_SetUINT64(media_type, &MF_MT_FRAME_RATE, frame_rate);
So if the user never requests a specific frame rate, then we always force it to 29.97? And we ignore dynamic frame rate changes?
This is also surprising; do we have tests for these?
We have tests that this is the default frame rate, yes.
Right, but this code will end up forcing the frame rate to 29.97 even if the H.264 data yields something different, or am I misreading again?
On 4/6/22 22:13, Zebediah Figura wrote:
+ if (!(decoder->wg_transform = wg_transform_create(&input_format, &decoder->output_format))) return E_FAIL; return S_OK; @@ -369,7 +376,7 @@ static HRESULT WINAPI transform_GetOutputAvailableType(IMFTransform *iface, DWOR if (FAILED(hr = IMFMediaType_SetGUID(media_type, &MF_MT_SUBTYPE, output_type))) goto done; - hr = fill_output_media_type(media_type, NULL); + hr = fill_output_media_type(media_type, decoder->output_type); done: if (SUCCEEDED(hr))
This also seems like it's orthogonal to dynamic format change. (Is it possible to write tests for this?)
I don't think it's completely orthogonal, and there are tests already, checking the new output types after a format change.
Right, I missed those the first time...
I think I see that this isn't orthogonal to dynamic format change, but now I'm left wondering why those tests aren't fixed by this patch. We should be reporting the real width and height now, right?
Because the image size that native reports includes NV12 plane alignments that we still have wrong.
frame_rate);
So if the user never requests a specific frame rate, then we always force it to 29.97? And we ignore dynamic frame rate changes?
This is also surprising; do we have tests for these?
We have tests that this is the default frame rate, yes.
Right, but this code will end up forcing the frame rate to 29.97 even if the H.264 data yields something different, or am I misreading again?
Yes, it's wrong. It worked for all the games I know are using the transform, because most don't even care about the frame rate.
I'll have a better look at why GStreamer doesn't get us the correct frame rate.
I any case, I wrote more tests about the format changes, and if we want to have things done right from the start, and although the target games don't need it, I'll have to write things differently, using a colorconvert transform on the Win32 side (still implemented using GStreamer videoconvert), and passing buffers back and forth from the unix side.
The stream format can be changed at will, even after buffers have been decoded and queued, and without even pushing any more input. I don't think we can do that easily with the current pipeline as simple as it is, as the decoder and the videoconvert elements are plugged together.
I also don't want to introduce complexity from an async pipeline as all my previous attempts already failed to satisfy the games expectations.
On 4/6/22 15:29, Rémi Bernon wrote:
On 4/6/22 22:13, Zebediah Figura wrote:
+ if (!(decoder->wg_transform = wg_transform_create(&input_format, &decoder->output_format))) return E_FAIL; return S_OK; @@ -369,7 +376,7 @@ static HRESULT WINAPI transform_GetOutputAvailableType(IMFTransform *iface, DWOR if (FAILED(hr = IMFMediaType_SetGUID(media_type, &MF_MT_SUBTYPE, output_type))) goto done; - hr = fill_output_media_type(media_type, NULL); + hr = fill_output_media_type(media_type, decoder->output_type); done: if (SUCCEEDED(hr))
This also seems like it's orthogonal to dynamic format change. (Is it possible to write tests for this?)
I don't think it's completely orthogonal, and there are tests already, checking the new output types after a format change.
Right, I missed those the first time...
I think I see that this isn't orthogonal to dynamic format change, but now I'm left wondering why those tests aren't fixed by this patch. We should be reporting the real width and height now, right?
Because the image size that native reports includes NV12 plane alignments that we still have wrong.
Ugh, okay...
frame_rate);
So if the user never requests a specific frame rate, then we always force it to 29.97? And we ignore dynamic frame rate changes?
This is also surprising; do we have tests for these?
We have tests that this is the default frame rate, yes.
Right, but this code will end up forcing the frame rate to 29.97 even if the H.264 data yields something different, or am I misreading again?
Yes, it's wrong. It worked for all the games I know are using the transform, because most don't even care about the frame rate.
I'll have a better look at why GStreamer doesn't get us the correct frame rate.
I any case, I wrote more tests about the format changes, and if we want to have things done right from the start, and although the target games don't need it, I'll have to write things differently, using a colorconvert transform on the Win32 side (still implemented using GStreamer videoconvert), and passing buffers back and forth from the unix side.
The stream format can be changed at will, even after buffers have been decoded and queued, and without even pushing any more input. I don't think we can do that easily with the current pipeline as simple as it is, as the decoder and the videoconvert elements are plugged together.
I also don't want to introduce complexity from an async pipeline as all my previous attempts already failed to satisfy the games expectations.
I'm not necessarily advocating that we get everything exactly right, although having the tests still seems valuable. Rather I was just trying to understand, or fix, some code that looked really surprising :-)
On 4/6/22 22:33, Zebediah Figura wrote:
On 4/6/22 15:29, Rémi Bernon wrote:
On 4/6/22 22:13, Zebediah Figura wrote:
+ if (!(decoder->wg_transform = wg_transform_create(&input_format, &decoder->output_format))) return E_FAIL; return S_OK; @@ -369,7 +376,7 @@ static HRESULT WINAPI transform_GetOutputAvailableType(IMFTransform *iface, DWOR if (FAILED(hr = IMFMediaType_SetGUID(media_type, &MF_MT_SUBTYPE, output_type))) goto done; - hr = fill_output_media_type(media_type, NULL); + hr = fill_output_media_type(media_type, decoder->output_type); done: if (SUCCEEDED(hr))
This also seems like it's orthogonal to dynamic format change. (Is it possible to write tests for this?)
I don't think it's completely orthogonal, and there are tests already, checking the new output types after a format change.
Right, I missed those the first time...
I think I see that this isn't orthogonal to dynamic format change, but now I'm left wondering why those tests aren't fixed by this patch. We should be reporting the real width and height now, right?
Because the image size that native reports includes NV12 plane alignments that we still have wrong.
Ugh, okay...
frame_rate);
So if the user never requests a specific frame rate, then we always force it to 29.97? And we ignore dynamic frame rate changes?
This is also surprising; do we have tests for these?
We have tests that this is the default frame rate, yes.
Right, but this code will end up forcing the frame rate to 29.97 even if the H.264 data yields something different, or am I misreading again?
Yes, it's wrong. It worked for all the games I know are using the transform, because most don't even care about the frame rate.
I'll have a better look at why GStreamer doesn't get us the correct frame rate.
I any case, I wrote more tests about the format changes, and if we want to have things done right from the start, and although the target games don't need it, I'll have to write things differently, using a colorconvert transform on the Win32 side (still implemented using GStreamer videoconvert), and passing buffers back and forth from the unix side.
The stream format can be changed at will, even after buffers have been decoded and queued, and without even pushing any more input. I don't think we can do that easily with the current pipeline as simple as it is, as the decoder and the videoconvert elements are plugged together.
I also don't want to introduce complexity from an async pipeline as all my previous attempts already failed to satisfy the games expectations.
I'm not necessarily advocating that we get everything exactly right, although having the tests still seems valuable. Rather I was just trying to understand, or fix, some code that looked really surprising :-)
Well I think it's still best, although I'm not really excited about the time it'll still require.
I intended to try upstreaming ResamplerDMO and ColorConvertDMO anyway, as they are currently bit rotting in Proton and would clearly benefit from the wg_transform simplicity.
On 4/5/22 07:26, Rémi Bernon wrote:
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@codeweavers.com
(This need to come first, before 231660-231665 as the spurious todo_wine is caused by the H264 format being synchronously rejected when libav plugins are used)
In this patch I assumed that sink callbacks are always called from the same thread, and then do not require any locking around sink_caps accesses.
That's not guaranteed by the API. I'd rather just be explicitly thread-safe.
@@ -264,6 +298,11 @@ NTSTATUS wg_transform_create(void *args) break;
case WG_MAJOR_TYPE_VIDEO:
if (!(element = create_element("videoconvert", "base"))|| !transform_append_element(transform, element, &first, &last))goto out;
This is one of those things that probably should have been split anyway, but as before, I'm left wondering why we need this, and in particular why it's related to the rest of this patch. Why would H.264 dynamic format changes imply that the video format changes, and why would we need to convert it?
/* Let GStreamer choose a default number of threads. */gst_util_set_object_arg(G_OBJECT(element), "n-threads", "0");
I'd be curious to learn whether this has a measurable positive impact, especially with different video sizes.
@@ -425,7 +465,18 @@ NTSTATUS wg_transform_read_data(void *args) return STATUS_SUCCESS; }
- if ((status = read_transform_output_data(transform->output_buffer, sample)))
- if (sample->format && (caps = gst_sample_get_caps(transform->output_sample)))
This bothers me. Patch ordering problems aside, the way this reads implies that the winegstreamer API consumer is free to ignore sample format changes, which doesn't seem right.
- {
wg_format_from_caps(&format, caps);if (!wg_format_compare(&format, sample->format)){*sample->format = format;params->result = MF_E_TRANSFORM_STREAM_CHANGE;return STATUS_SUCCESS;}- }
- if ((status = read_transform_output_data(gst_sample_get_buffer(transform->output_sample), sample))) { sample->size = 0; return status;
On 4/6/22 00:11, Zebediah Figura wrote:
On 4/5/22 07:26, Rémi Bernon wrote:
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@codeweavers.com
(This need to come first, before 231660-231665 as the spurious todo_wine is caused by the H264 format being synchronously rejected when libav plugins are used)
In this patch I assumed that sink callbacks are always called from the same thread, and then do not require any locking around sink_caps accesses.
That's not guaranteed by the API. I'd rather just be explicitly thread-safe.
@@ -264,6 +298,11 @@ NTSTATUS wg_transform_create(void *args) break; case WG_MAJOR_TYPE_VIDEO: + if (!(element = create_element("videoconvert", "base")) + || !transform_append_element(transform, element, &first, &last)) + goto out;
This is one of those things that probably should have been split anyway, but as before, I'm left wondering why we need this, and in particular why it's related to the rest of this patch. Why would H.264 dynamic format changes imply that the video format changes, and why would we need to convert it?
Because what actually may change is the frame size, not the image format that is output, though maybe a transform user could change it, games don't expect it to change and are confused if it does.
But then yes, probably it could and should be added beforehand, as otherwise caps negotiation is failing.
+ /* Let GStreamer choose a default number of threads. */ + gst_util_set_object_arg(G_OBJECT(element), "n-threads", "0");
I'd be curious to learn whether this has a measurable positive impact, especially with different video sizes.
I believe I saw that it did especially on large videos, though I don't have any numbers to back that claim.
@@ -425,7 +465,18 @@ NTSTATUS wg_transform_read_data(void *args) return STATUS_SUCCESS; } - if ((status = read_transform_output_data(transform->output_buffer, sample))) + if (sample->format && (caps = gst_sample_get_caps(transform->output_sample)))
This bothers me. Patch ordering problems aside, the way this reads implies that the winegstreamer API consumer is free to ignore sample format changes, which doesn't seem right.
Format change is really only used by H264. I initially had it non-optional, but it means every transform needs to keep a wg_format too, as well as passing it around. None of the other transforms I've tested (4-5 so far) will generate stream change events.
If the client uses a fixed format, it will be translated to a fixed caps and I believe no stream change can happen.
Only the H264 client, which clears its width / height before creating the wg_transform, is expecting format (but actually only frame size) to change.
On 4/5/22 17:39, Rémi Bernon wrote:
On 4/6/22 00:11, Zebediah Figura wrote:
On 4/5/22 07:26, Rémi Bernon wrote:
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@codeweavers.com
(This need to come first, before 231660-231665 as the spurious todo_wine is caused by the H264 format being synchronously rejected when libav plugins are used)
In this patch I assumed that sink callbacks are always called from the same thread, and then do not require any locking around sink_caps accesses.
That's not guaranteed by the API. I'd rather just be explicitly thread-safe.
@@ -264,6 +298,11 @@ NTSTATUS wg_transform_create(void *args) break; case WG_MAJOR_TYPE_VIDEO: + if (!(element = create_element("videoconvert", "base")) + || !transform_append_element(transform, element, &first, &last)) + goto out;
This is one of those things that probably should have been split anyway, but as before, I'm left wondering why we need this, and in particular why it's related to the rest of this patch. Why would H.264 dynamic format changes imply that the video format changes, and why would we need to convert it?
Because what actually may change is the frame size, not the image format that is output, though maybe a transform user could change it, games don't expect it to change and are confused if it does.
But videoconvert doesn't affect frame size, only colour space. Is videoconvert relevant to this patch?
But then yes, probably it could and should be added beforehand, as otherwise caps negotiation is failing.
+ /* Let GStreamer choose a default number of threads. */ + gst_util_set_object_arg(G_OBJECT(element), "n-threads", "0");
I'd be curious to learn whether this has a measurable positive impact, especially with different video sizes.
I believe I saw that it did especially on large videos, though I don't have any numbers to back that claim.
@@ -425,7 +465,18 @@ NTSTATUS wg_transform_read_data(void *args) return STATUS_SUCCESS; } - if ((status = read_transform_output_data(transform->output_buffer, sample))) + if (sample->format && (caps = gst_sample_get_caps(transform->output_sample)))
This bothers me. Patch ordering problems aside, the way this reads implies that the winegstreamer API consumer is free to ignore sample format changes, which doesn't seem right.
Actually, I just reread this, and no, it doesn't really imply that. So forget what I said...
-
Marvin -
Rémi Bernon -
Zebediah Figura