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