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.