[PATCH v2 0/9] MR5055: winegstreamer: Support input video frame padding and cropping.
On the client side, frame aperture may be included or omitted. This can be used to include or drop the frame padding, and the VideoProcessor MFT supports it. On GStreamer side, we can only use the unpadded frame size in the caps, because this is how decoders work, and padding needs to be added as a property of the input/output video buffers. Then, frame size needs to be adjusted to be consistent between input and output, and any difference considered as extra padding to be added on one side or the other. -- v2: winegstreamer: Respect video format padding for input buffers too. winegstreamer: Exclude padding from wg_format video frame size. winegstreamer: Use video info stride in buffer meta rather than videoflip. winegstreamer: Use a new wg_video_buffer_pool class to add buffer meta. winegstreamer: Update the output_format when stream format changes. winegstreamer: Pass optional GstVideoInfo to read_transform_output_data. winegstreamer: Split read_transform_output_data in two helpers. winegstreamer: Introduce a new sample_needs_copy_buffer helper. winegstreamer: Introduce a new sample_set_flags_from_buffer helper. https://gitlab.winehq.org/wine/wine/-/merge_requests/5055
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/wg_transform.c | 47 +++++++++++++++++-------------- 1 file changed, 26 insertions(+), 21 deletions(-) diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 516b28e82e2..51706a6e6a6 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -730,6 +730,31 @@ static NTSTATUS copy_buffer(GstBuffer *buffer, GstCaps *caps, struct wg_sample * return STATUS_SUCCESS; } +static void sample_set_flags_from_buffer(struct wg_sample *sample, GstBuffer *buffer, gsize total_size) +{ + if (GST_BUFFER_PTS_IS_VALID(buffer)) + { + sample->flags |= WG_SAMPLE_FLAG_HAS_PTS; + sample->pts = GST_BUFFER_PTS(buffer) / 100; + } + if (GST_BUFFER_DURATION_IS_VALID(buffer)) + { + GstClockTime duration = GST_BUFFER_DURATION(buffer) / 100; + + duration = (duration * sample->size) / total_size; + GST_BUFFER_DURATION(buffer) -= duration * 100; + if (GST_BUFFER_PTS_IS_VALID(buffer)) + GST_BUFFER_PTS(buffer) += duration * 100; + + sample->flags |= WG_SAMPLE_FLAG_HAS_DURATION; + sample->duration = duration; + } + if (!GST_BUFFER_FLAG_IS_SET(buffer, GST_BUFFER_FLAG_DELTA_UNIT)) + sample->flags |= WG_SAMPLE_FLAG_SYNC_POINT; + if (GST_BUFFER_FLAG_IS_SET(buffer, GST_BUFFER_FLAG_DISCONT)) + sample->flags |= WG_SAMPLE_FLAG_DISCONTINUITY; +} + static NTSTATUS read_transform_output_data(GstBuffer *buffer, GstCaps *caps, gsize plane_align, struct wg_sample *sample) { @@ -762,27 +787,7 @@ static NTSTATUS read_transform_output_data(GstBuffer *buffer, GstCaps *caps, gsi return status; } - if (GST_BUFFER_PTS_IS_VALID(buffer)) - { - sample->flags |= WG_SAMPLE_FLAG_HAS_PTS; - sample->pts = GST_BUFFER_PTS(buffer) / 100; - } - if (GST_BUFFER_DURATION_IS_VALID(buffer)) - { - GstClockTime duration = GST_BUFFER_DURATION(buffer) / 100; - - duration = (duration * sample->size) / total_size; - GST_BUFFER_DURATION(buffer) -= duration * 100; - if (GST_BUFFER_PTS_IS_VALID(buffer)) - GST_BUFFER_PTS(buffer) += duration * 100; - - sample->flags |= WG_SAMPLE_FLAG_HAS_DURATION; - sample->duration = duration; - } - if (!GST_BUFFER_FLAG_IS_SET(buffer, GST_BUFFER_FLAG_DELTA_UNIT)) - sample->flags |= WG_SAMPLE_FLAG_SYNC_POINT; - if (GST_BUFFER_FLAG_IS_SET(buffer, GST_BUFFER_FLAG_DISCONT)) - sample->flags |= WG_SAMPLE_FLAG_DISCONTINUITY; + sample_set_flags_from_buffer(sample, buffer, total_size); if (needs_copy) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5055
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/wg_transform.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 51706a6e6a6..4152856ec5a 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -755,13 +755,10 @@ static void sample_set_flags_from_buffer(struct wg_sample *sample, GstBuffer *bu sample->flags |= WG_SAMPLE_FLAG_DISCONTINUITY; } -static NTSTATUS read_transform_output_data(GstBuffer *buffer, GstCaps *caps, gsize plane_align, - struct wg_sample *sample) +static bool sample_needs_copy_buffer(struct wg_sample *sample, GstBuffer *buffer, gsize *total_size) { - gsize total_size; - bool needs_copy; - NTSTATUS status; GstMapInfo info; + bool needs_copy; if (!gst_buffer_map(buffer, &info, GST_MAP_READ)) { @@ -770,10 +767,20 @@ static NTSTATUS read_transform_output_data(GstBuffer *buffer, GstCaps *caps, gsi return STATUS_UNSUCCESSFUL; } needs_copy = info.data != wg_sample_data(sample); - total_size = sample->size = info.size; + *total_size = sample->size = info.size; gst_buffer_unmap(buffer, &info); - if (!needs_copy) + return needs_copy; +} + +static NTSTATUS read_transform_output_data(GstBuffer *buffer, GstCaps *caps, gsize plane_align, + struct wg_sample *sample) +{ + gsize total_size; + NTSTATUS status; + bool needs_copy; + + if (!(needs_copy = sample_needs_copy_buffer(sample, buffer, &total_size))) status = STATUS_SUCCESS; else if (stream_type_from_caps(caps) == GST_STREAM_TYPE_VIDEO) status = copy_video_buffer(buffer, caps, plane_align, sample, &total_size); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5055
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/wg_transform.c | 52 ++++++++++++++++++++++++------- 1 file changed, 40 insertions(+), 12 deletions(-) diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 4152856ec5a..fc7dd3363ca 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -704,7 +704,7 @@ static NTSTATUS copy_video_buffer(GstBuffer *buffer, GstCaps *caps, gsize plane_ return status; } -static NTSTATUS copy_buffer(GstBuffer *buffer, GstCaps *caps, struct wg_sample *sample, +static NTSTATUS copy_buffer(GstBuffer *buffer, struct wg_sample *sample, gsize *total_size) { GstMapInfo info; @@ -773,8 +773,8 @@ static bool sample_needs_copy_buffer(struct wg_sample *sample, GstBuffer *buffer return needs_copy; } -static NTSTATUS read_transform_output_data(GstBuffer *buffer, GstCaps *caps, gsize plane_align, - struct wg_sample *sample) +static NTSTATUS sample_read_video_buffer(struct wg_sample *sample, GstBuffer *buffer, + GstCaps *caps, gsize plane_align) { gsize total_size; NTSTATUS status; @@ -782,10 +782,8 @@ static NTSTATUS read_transform_output_data(GstBuffer *buffer, GstCaps *caps, gsi if (!(needs_copy = sample_needs_copy_buffer(sample, buffer, &total_size))) status = STATUS_SUCCESS; - else if (stream_type_from_caps(caps) == GST_STREAM_TYPE_VIDEO) - status = copy_video_buffer(buffer, caps, plane_align, sample, &total_size); else - status = copy_buffer(buffer, caps, sample, &total_size); + status = copy_video_buffer(buffer, caps, plane_align, sample, &total_size); if (status) { @@ -797,12 +795,37 @@ static NTSTATUS read_transform_output_data(GstBuffer *buffer, GstCaps *caps, gsi sample_set_flags_from_buffer(sample, buffer, total_size); if (needs_copy) + GST_WARNING("Copied %u bytes, sample %p, flags %#x", sample->size, sample, sample->flags); + else if (sample->flags & WG_SAMPLE_FLAG_INCOMPLETE) + GST_ERROR("Partial read %u bytes, sample %p, flags %#x", sample->size, sample, sample->flags); + else + GST_INFO("Read %u bytes, sample %p, flags %#x", sample->size, sample, sample->flags); + + return STATUS_SUCCESS; +} + +static NTSTATUS sample_read_other_buffer(struct wg_sample *sample, GstBuffer *buffer) +{ + gsize total_size; + NTSTATUS status; + bool needs_copy; + + if (!(needs_copy = sample_needs_copy_buffer(sample, buffer, &total_size))) + status = STATUS_SUCCESS; + else + status = copy_buffer(buffer, sample, &total_size); + + if (status) { - if (stream_type_from_caps(caps) == GST_STREAM_TYPE_VIDEO) - GST_WARNING("Copied %u bytes, sample %p, flags %#x", sample->size, sample, sample->flags); - else - GST_INFO("Copied %u bytes, sample %p, flags %#x", sample->size, sample, sample->flags); + GST_ERROR("Failed to copy buffer %"GST_PTR_FORMAT, buffer); + sample->size = 0; + return status; } + + sample_set_flags_from_buffer(sample, buffer, total_size); + + if (needs_copy) + GST_INFO("Copied %u bytes, sample %p, flags %#x", sample->size, sample, sample->flags); else if (sample->flags & WG_SAMPLE_FLAG_INCOMPLETE) GST_ERROR("Partial read %u bytes, sample %p, flags %#x", sample->size, sample, sample->flags); else @@ -893,8 +916,13 @@ NTSTATUS wg_transform_read_data(void *args) return STATUS_SUCCESS; } - if ((status = read_transform_output_data(output_buffer, output_caps, - transform->attrs.output_plane_align, sample))) + if (stream_type_from_caps(output_caps) == GST_STREAM_TYPE_VIDEO) + status = sample_read_video_buffer(sample, output_buffer, output_caps, + transform->attrs.output_plane_align); + else + status = sample_read_other_buffer(sample, output_buffer); + + if (status) { wg_allocator_release_sample(transform->allocator, sample, false); return status; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5055
From: Rémi Bernon <rbernon(a)codeweavers.com> Instead of computing them in copy_video_buffer. --- dlls/winegstreamer/wg_transform.c | 59 ++++++++++++++----------------- 1 file changed, 27 insertions(+), 32 deletions(-) diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index fc7dd3363ca..08ba442366a 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -650,25 +650,14 @@ NTSTATUS wg_transform_push_data(void *args) return STATUS_SUCCESS; } -static NTSTATUS copy_video_buffer(GstBuffer *buffer, GstCaps *caps, gsize plane_align, - struct wg_sample *sample, gsize *total_size) +static NTSTATUS copy_video_buffer(GstBuffer *buffer, const GstVideoInfo *src_video_info, + const GstVideoInfo *dst_video_info, struct wg_sample *sample, gsize *total_size) { NTSTATUS status = STATUS_UNSUCCESSFUL; GstVideoFrame src_frame, dst_frame; - GstVideoInfo src_info, dst_info; - GstVideoAlignment align; GstBuffer *dst_buffer; - if (!gst_video_info_from_caps(&src_info, caps)) - { - GST_ERROR("Failed to get video info from caps."); - return STATUS_UNSUCCESSFUL; - } - - dst_info = src_info; - align_video_info_planes(plane_align, &dst_info, &align); - - if (sample->max_size < dst_info.size) + if (sample->max_size < dst_video_info->size) { GST_ERROR("Output buffer is too small."); return STATUS_BUFFER_TOO_SMALL; @@ -680,14 +669,14 @@ static NTSTATUS copy_video_buffer(GstBuffer *buffer, GstCaps *caps, gsize plane_ GST_ERROR("Failed to wrap wg_sample into GstBuffer"); return STATUS_UNSUCCESSFUL; } - gst_buffer_set_size(dst_buffer, dst_info.size); - *total_size = sample->size = dst_info.size; + gst_buffer_set_size(dst_buffer, dst_video_info->size); + *total_size = sample->size = dst_video_info->size; - if (!gst_video_frame_map(&src_frame, &src_info, buffer, GST_MAP_READ)) + if (!gst_video_frame_map(&src_frame, src_video_info, buffer, GST_MAP_READ)) GST_ERROR("Failed to map source frame."); else { - if (!gst_video_frame_map(&dst_frame, &dst_info, dst_buffer, GST_MAP_WRITE)) + if (!gst_video_frame_map(&dst_frame, dst_video_info, dst_buffer, GST_MAP_WRITE)) GST_ERROR("Failed to map destination frame."); else { @@ -704,8 +693,7 @@ static NTSTATUS copy_video_buffer(GstBuffer *buffer, GstCaps *caps, gsize plane_ return status; } -static NTSTATUS copy_buffer(GstBuffer *buffer, struct wg_sample *sample, - gsize *total_size) +static NTSTATUS copy_buffer(GstBuffer *buffer, struct wg_sample *sample, gsize *total_size) { GstMapInfo info; @@ -774,7 +762,7 @@ static bool sample_needs_copy_buffer(struct wg_sample *sample, GstBuffer *buffer } static NTSTATUS sample_read_video_buffer(struct wg_sample *sample, GstBuffer *buffer, - GstCaps *caps, gsize plane_align) + const GstVideoInfo *src_video_info, const GstVideoInfo *dst_video_info) { gsize total_size; NTSTATUS status; @@ -783,7 +771,7 @@ static NTSTATUS sample_read_video_buffer(struct wg_sample *sample, GstBuffer *bu if (!(needs_copy = sample_needs_copy_buffer(sample, buffer, &total_size))) status = STATUS_SUCCESS; else - status = copy_video_buffer(buffer, caps, plane_align, sample, &total_size); + status = copy_video_buffer(buffer, src_video_info, dst_video_info, sample, &total_size); if (status) { @@ -858,8 +846,10 @@ NTSTATUS wg_transform_read_data(void *args) { struct wg_transform_read_data_params *params = args; struct wg_transform *transform = get_transform(params->transform); + GstVideoInfo src_video_info, dst_video_info; struct wg_sample *sample = params->sample; struct wg_format *format = params->format; + GstVideoAlignment align = {0}; GstBuffer *output_buffer; GstCaps *output_caps; bool discard_data; @@ -877,6 +867,18 @@ NTSTATUS wg_transform_read_data(void *args) output_buffer = gst_sample_get_buffer(transform->output_sample); output_caps = gst_sample_get_caps(transform->output_sample); + if (stream_type_from_caps(output_caps) == GST_STREAM_TYPE_VIDEO) + { + gsize plane_align = transform->attrs.output_plane_align; + + if (!gst_video_info_from_caps(&src_video_info, output_caps)) + GST_ERROR("Failed to get video info from %"GST_PTR_FORMAT, output_caps); + dst_video_info = src_video_info; + + /* set the desired output buffer alignment on the dest video info */ + align_video_info_planes(plane_align, &dst_video_info, &align); + } + if (GST_MINI_OBJECT_FLAG_IS_SET(transform->output_sample, GST_SAMPLE_FLAG_WG_CAPS_CHANGED)) { GST_MINI_OBJECT_FLAG_UNSET(transform->output_sample, GST_SAMPLE_FLAG_WG_CAPS_CHANGED); @@ -885,17 +887,10 @@ NTSTATUS wg_transform_read_data(void *args) if (format) { - gsize plane_align = transform->attrs.output_plane_align; - GstVideoAlignment align; - GstVideoInfo info; - wg_format_from_caps(format, output_caps); - if (format->major_type == WG_MAJOR_TYPE_VIDEO - && gst_video_info_from_caps(&info, output_caps)) + if (format->major_type == WG_MAJOR_TYPE_VIDEO) { - align_video_info_planes(plane_align, &info, &align); - GST_INFO("Returning video alignment left %u, top %u, right %u, bottom %u.", align.padding_left, align.padding_top, align.padding_right, align.padding_bottom); @@ -917,8 +912,8 @@ NTSTATUS wg_transform_read_data(void *args) } if (stream_type_from_caps(output_caps) == GST_STREAM_TYPE_VIDEO) - status = sample_read_video_buffer(sample, output_buffer, output_caps, - transform->attrs.output_plane_align); + status = sample_read_video_buffer(sample, output_buffer, + &src_video_info, &dst_video_info); else status = sample_read_other_buffer(sample, output_buffer); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5055
From: Rémi Bernon <rbernon(a)codeweavers.com> This makes sure transform->output_format always reflect the internal stream format, regardless of whether the client calls set_output_format or not, as they aren't actually required to. --- dlls/winegstreamer/wg_transform.c | 48 +++++++++++++++++-------------- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 08ba442366a..06be9d048d5 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -38,6 +38,7 @@ #include "mferror.h" #include "unix_private.h" +#include "wine/debug.h" #define GST_SAMPLE_FLAG_WG_CAPS_CHANGED (GST_MINI_OBJECT_FLAG_LAST << 0) @@ -543,16 +544,16 @@ NTSTATUS wg_transform_set_output_format(void *args) { struct wg_transform_set_output_format_params *params = args; struct wg_transform *transform = get_transform(params->transform); - const struct wg_format *format = params->format; + struct wg_format output_format = *params->format; GstSample *sample; GstCaps *caps; - if (!(caps = transform_format_to_caps(transform, format))) + if (!(caps = transform_format_to_caps(transform, &output_format))) { - GST_ERROR("Failed to convert format %p to caps.", format); + GST_ERROR("Failed to convert format to caps."); return STATUS_UNSUCCESSFUL; } - transform->output_format = *format; + transform->output_format = output_format; GST_INFO("transform %p output caps %"GST_PTR_FORMAT, transform, caps); @@ -574,7 +575,7 @@ NTSTATUS wg_transform_set_output_format(void *args) if (transform->video_flip) { const char *value; - if (transform->input_is_flipped != wg_format_video_is_flipped(format)) + if (transform->input_is_flipped != wg_format_video_is_flipped(&output_format)) value = "vertical-flip"; else value = "none"; @@ -881,30 +882,33 @@ NTSTATUS wg_transform_read_data(void *args) if (GST_MINI_OBJECT_FLAG_IS_SET(transform->output_sample, GST_SAMPLE_FLAG_WG_CAPS_CHANGED)) { + struct wg_format output_format; + GST_MINI_OBJECT_FLAG_UNSET(transform->output_sample, GST_SAMPLE_FLAG_WG_CAPS_CHANGED); GST_INFO("transform %p output caps %"GST_PTR_FORMAT, transform, output_caps); - if (format) + wg_format_from_caps(&output_format, output_caps); + if (output_format.major_type == WG_MAJOR_TYPE_VIDEO) { - wg_format_from_caps(format, output_caps); - - if (format->major_type == WG_MAJOR_TYPE_VIDEO) - { - GST_INFO("Returning video alignment left %u, top %u, right %u, bottom %u.", align.padding_left, - align.padding_top, align.padding_right, align.padding_bottom); - - format->u.video.padding.left = align.padding_left; - format->u.video.width += format->u.video.padding.left; - format->u.video.padding.right = align.padding_right; - format->u.video.width += format->u.video.padding.right; - format->u.video.padding.top = align.padding_top; - format->u.video.height += format->u.video.padding.top; - format->u.video.padding.bottom = align.padding_bottom; - format->u.video.height += format->u.video.padding.bottom; - } + output_format.u.video.padding.left = align.padding_left; + output_format.u.video.width += output_format.u.video.padding.left; + output_format.u.video.padding.right = align.padding_right; + output_format.u.video.width += output_format.u.video.padding.right; + output_format.u.video.padding.top = align.padding_top; + output_format.u.video.height += output_format.u.video.padding.top; + output_format.u.video.padding.bottom = align.padding_bottom; + output_format.u.video.height += output_format.u.video.padding.bottom; + GST_INFO("new video padding rect %s", wine_dbgstr_rect(&output_format.u.video.padding)); + + if (transform->output_format.u.video.height < 0) + output_format.u.video.height *= -1; } + if (format) + *format = output_format; + transform->output_format = output_format; + params->result = MF_E_TRANSFORM_STREAM_CHANGE; GST_INFO("Format changed detected, returning no output"); wg_allocator_release_sample(transform->allocator, sample, false); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5055
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/wg_transform.c | 75 +++++++++++++++++++++---------- 1 file changed, 51 insertions(+), 24 deletions(-) diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 06be9d048d5..10d4b205ef0 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -83,6 +83,51 @@ static void align_video_info_planes(gsize plane_align, GstVideoInfo *info, GstVi gst_video_info_align(info, align); } +typedef struct +{ + GstVideoBufferPool parent; +} WgVideoBufferPool; + +typedef struct +{ + GstVideoBufferPoolClass parent_class; +} WgVideoBufferPoolClass; + +G_DEFINE_TYPE(WgVideoBufferPool, wg_video_buffer_pool, GST_TYPE_VIDEO_BUFFER_POOL); + +static void wg_video_buffer_pool_init(WgVideoBufferPool * pool) {} +static void wg_video_buffer_pool_class_init(WgVideoBufferPoolClass *klass) {} + +static WgVideoBufferPool *wg_video_buffer_pool_create(GstCaps *caps, gsize plane_align, + GstAllocator *allocator, GstVideoInfo *info, GstVideoAlignment *align) +{ + WgVideoBufferPool *pool; + GstStructure *config; + + if (!(pool = g_object_new(wg_video_buffer_pool_get_type(), NULL))) + return NULL; + + gst_video_info_from_caps(info, caps); + align_video_info_planes(plane_align, info, align); + + if (!(config = gst_buffer_pool_get_config(GST_BUFFER_POOL(pool)))) + GST_ERROR("Failed to get %"GST_PTR_FORMAT" config.", pool); + else + { + gst_buffer_pool_config_add_option(config, GST_BUFFER_POOL_OPTION_VIDEO_META); + gst_buffer_pool_config_add_option(config, GST_BUFFER_POOL_OPTION_VIDEO_ALIGNMENT); + gst_buffer_pool_config_set_video_alignment(config, align); + + gst_buffer_pool_config_set_params(config, caps, info->size, 0, 0); + gst_buffer_pool_config_set_allocator(config, allocator, NULL); + if (!gst_buffer_pool_set_config(GST_BUFFER_POOL(pool), config)) + GST_ERROR("Failed to set %"GST_PTR_FORMAT" config.", pool); + } + + GST_INFO("Created %"GST_PTR_FORMAT, pool); + return pool; +} + static GstFlowReturn transform_sink_chain_cb(GstPad *pad, GstObject *parent, GstBuffer *buffer) { struct wg_transform *transform = gst_pad_get_element_private(pad); @@ -129,11 +174,10 @@ static gboolean transform_src_query_cb(GstPad *pad, GstObject *parent, GstQuery static gboolean transform_sink_query_allocation(struct wg_transform *transform, GstQuery *query) { - gsize plane_align = transform->attrs.output_plane_align; - GstStructure *config, *params; + WgVideoBufferPool *pool; GstVideoAlignment align; + GstStructure *params; gboolean needs_pool; - GstBufferPool *pool; GstVideoInfo info; GstCaps *caps; @@ -143,12 +187,10 @@ static gboolean transform_sink_query_allocation(struct wg_transform *transform, if (stream_type_from_caps(caps) != GST_STREAM_TYPE_VIDEO || !needs_pool) return false; - if (!gst_video_info_from_caps(&info, caps) - || !(pool = gst_video_buffer_pool_new())) + if (!(pool = wg_video_buffer_pool_create(caps, transform->attrs.output_plane_align, + transform->allocator, &info, &align))) return false; - align_video_info_planes(plane_align, &info, &align); - if ((params = gst_structure_new("video-meta", "padding-top", G_TYPE_UINT, align.padding_top, "padding-bottom", G_TYPE_UINT, align.padding_bottom, @@ -160,26 +202,11 @@ static gboolean transform_sink_query_allocation(struct wg_transform *transform, gst_structure_free(params); } - if (!(config = gst_buffer_pool_get_config(pool))) - GST_ERROR("Failed to get %"GST_PTR_FORMAT" config.", pool); - else - { - gst_buffer_pool_config_add_option(config, GST_BUFFER_POOL_OPTION_VIDEO_META); - gst_buffer_pool_config_add_option(config, GST_BUFFER_POOL_OPTION_VIDEO_ALIGNMENT); - gst_buffer_pool_config_set_video_alignment(config, &align); - - gst_buffer_pool_config_set_params(config, caps, - info.size, 0, 0); - gst_buffer_pool_config_set_allocator(config, transform->allocator, NULL); - if (!gst_buffer_pool_set_config(pool, config)) - GST_ERROR("Failed to set %"GST_PTR_FORMAT" config.", pool); - } - /* Prevent pool reconfiguration, we don't want another alignment. */ - if (!gst_buffer_pool_set_active(pool, true)) + if (!gst_buffer_pool_set_active(GST_BUFFER_POOL(pool), true)) GST_ERROR("%"GST_PTR_FORMAT" failed to activate.", pool); - gst_query_add_allocation_pool(query, pool, info.size, 0, 0); + gst_query_add_allocation_pool(query, GST_BUFFER_POOL(pool), info.size, 0, 0); gst_query_add_allocation_param(query, transform->allocator, NULL); GST_INFO("Proposing %"GST_PTR_FORMAT", buffer size %#zx, %"GST_PTR_FORMAT", for %"GST_PTR_FORMAT, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5055
From: Rémi Bernon <rbernon(a)codeweavers.com> This removes the need for an extra videoconvert, and any ambiguity that comes with it. Also reduce our dependency on the GStreamer elements to behave as we assume they do, and reduce the chances to see unnecessary copies happening which can be harmful with large buffers. Some GStreamer versions for instance have a bogus videoflip that causes a buffer copy to happen when it is supposed to be pass through. --- dlls/winegstreamer/wg_transform.c | 142 ++++++++++++++++++++---------- 1 file changed, 94 insertions(+), 48 deletions(-) diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 10d4b205ef0..6882e16bf1c 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -54,14 +54,13 @@ struct wg_transform GstAtomicQueue *input_queue; - bool input_is_flipped; - GstElement *video_flip; - + struct wg_format input_format; struct wg_format output_format; GstAtomicQueue *output_queue; GstSample *output_sample; bool output_caps_changed; GstCaps *output_caps; + GstCaps *input_caps; }; static struct wg_transform *get_transform(wg_transform_t trans) @@ -69,7 +68,8 @@ static struct wg_transform *get_transform(wg_transform_t trans) return (struct wg_transform *)(ULONG_PTR)trans; } -static void align_video_info_planes(gsize plane_align, GstVideoInfo *info, GstVideoAlignment *align) +static void align_video_info_planes(struct wg_format *format, gsize plane_align, + GstVideoInfo *info, GstVideoAlignment *align) { gst_video_alignment_reset(align); @@ -81,11 +81,21 @@ static void align_video_info_planes(gsize plane_align, GstVideoInfo *info, GstVi align->stride_align[3] = plane_align; gst_video_info_align(info, align); + + if (format->u.video.height < 0) + { + for (guint i = 0; i < ARRAY_SIZE(info->offset); ++i) + { + info->offset[i] += (info->height - 1) * info->stride[i]; + info->stride[i] = -info->stride[i]; + } + } } typedef struct { GstVideoBufferPool parent; + GstVideoInfo info; } WgVideoBufferPool; typedef struct @@ -95,11 +105,51 @@ typedef struct G_DEFINE_TYPE(WgVideoBufferPool, wg_video_buffer_pool, GST_TYPE_VIDEO_BUFFER_POOL); +static void buffer_add_video_meta(GstBuffer *buffer, GstVideoInfo *info) +{ + GstVideoMeta *meta; + + if (!(meta = gst_buffer_get_video_meta(buffer))) + meta = gst_buffer_add_video_meta(buffer, GST_VIDEO_FRAME_FLAG_NONE, + info->finfo->format, info->width, info->height); + + if (!meta) + GST_ERROR("Failed to add video meta to buffer %"GST_PTR_FORMAT, buffer); + else + { + memcpy(meta->offset, info->offset, sizeof(info->offset)); + memcpy(meta->stride, info->stride, sizeof(info->stride)); + } +} + +static GstFlowReturn wg_video_buffer_pool_alloc_buffer(GstBufferPool *gst_pool, GstBuffer **buffer, + GstBufferPoolAcquireParams *params) +{ + GstBufferPoolClass *parent_class = GST_BUFFER_POOL_CLASS(wg_video_buffer_pool_parent_class); + WgVideoBufferPool *pool = (WgVideoBufferPool *)gst_pool; + GstFlowReturn ret; + + GST_LOG("%"GST_PTR_FORMAT", buffer %p, params %p", pool, buffer, params); + + if (!(ret = parent_class->alloc_buffer(gst_pool, buffer, params))) + { + buffer_add_video_meta(*buffer, &pool->info); + GST_INFO("%"GST_PTR_FORMAT" allocated buffer %"GST_PTR_FORMAT, pool, *buffer); + } + + return ret; +} + static void wg_video_buffer_pool_init(WgVideoBufferPool * pool) {} -static void wg_video_buffer_pool_class_init(WgVideoBufferPoolClass *klass) {} + +static void wg_video_buffer_pool_class_init(WgVideoBufferPoolClass *klass) +{ + GstBufferPoolClass *pool_class = GST_BUFFER_POOL_CLASS(klass); + pool_class->alloc_buffer = wg_video_buffer_pool_alloc_buffer; +} static WgVideoBufferPool *wg_video_buffer_pool_create(GstCaps *caps, gsize plane_align, - GstAllocator *allocator, GstVideoInfo *info, GstVideoAlignment *align) + GstAllocator *allocator, struct wg_format *format, GstVideoAlignment *align) { WgVideoBufferPool *pool; GstStructure *config; @@ -107,8 +157,10 @@ static WgVideoBufferPool *wg_video_buffer_pool_create(GstCaps *caps, gsize plane if (!(pool = g_object_new(wg_video_buffer_pool_get_type(), NULL))) return NULL; - gst_video_info_from_caps(info, caps); - align_video_info_planes(plane_align, info, align); + if (!gst_video_info_from_caps(&pool->info, caps)) + GST_ERROR("Failed to create video info from caps %" GST_PTR_FORMAT, caps); + else + align_video_info_planes(format, plane_align, &pool->info, align); if (!(config = gst_buffer_pool_get_config(GST_BUFFER_POOL(pool)))) GST_ERROR("Failed to get %"GST_PTR_FORMAT" config.", pool); @@ -118,7 +170,7 @@ static WgVideoBufferPool *wg_video_buffer_pool_create(GstCaps *caps, gsize plane gst_buffer_pool_config_add_option(config, GST_BUFFER_POOL_OPTION_VIDEO_ALIGNMENT); gst_buffer_pool_config_set_video_alignment(config, align); - gst_buffer_pool_config_set_params(config, caps, info->size, 0, 0); + gst_buffer_pool_config_set_params(config, caps, pool->info.size, 0, 0); gst_buffer_pool_config_set_allocator(config, allocator, NULL); if (!gst_buffer_pool_set_config(GST_BUFFER_POOL(pool), config)) GST_ERROR("Failed to set %"GST_PTR_FORMAT" config.", pool); @@ -178,7 +230,6 @@ static gboolean transform_sink_query_allocation(struct wg_transform *transform, GstVideoAlignment align; GstStructure *params; gboolean needs_pool; - GstVideoInfo info; GstCaps *caps; GST_LOG("transform %p, %"GST_PTR_FORMAT, transform, query); @@ -188,7 +239,7 @@ static gboolean transform_sink_query_allocation(struct wg_transform *transform, return false; if (!(pool = wg_video_buffer_pool_create(caps, transform->attrs.output_plane_align, - transform->allocator, &info, &align))) + transform->allocator, &transform->output_format, &align))) return false; if ((params = gst_structure_new("video-meta", @@ -206,11 +257,11 @@ static gboolean transform_sink_query_allocation(struct wg_transform *transform, if (!gst_buffer_pool_set_active(GST_BUFFER_POOL(pool), true)) GST_ERROR("%"GST_PTR_FORMAT" failed to activate.", pool); - gst_query_add_allocation_pool(query, GST_BUFFER_POOL(pool), info.size, 0, 0); + gst_query_add_allocation_pool(query, GST_BUFFER_POOL(pool), pool->info.size, 0, 0); gst_query_add_allocation_param(query, transform->allocator, NULL); GST_INFO("Proposing %"GST_PTR_FORMAT", buffer size %#zx, %"GST_PTR_FORMAT", for %"GST_PTR_FORMAT, - pool, info.size, transform->allocator, query); + pool, pool->info.size, transform->allocator, query); g_object_unref(pool); return true; @@ -349,27 +400,23 @@ NTSTATUS wg_transform_destroy(void *args) g_object_unref(transform->my_src); gst_query_unref(transform->drain_query); gst_caps_unref(transform->output_caps); + gst_caps_unref(transform->input_caps); gst_atomic_queue_unref(transform->output_queue); free(transform); return STATUS_SUCCESS; } -static bool wg_format_video_is_flipped(const struct wg_format *format) -{ - return format->major_type == WG_MAJOR_TYPE_VIDEO && (format->u.video.height < 0); -} - NTSTATUS wg_transform_create(void *args) { struct wg_transform_create_params *params = args; 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; + GstCaps *raw_caps = NULL; const gchar *media_type; GstEvent *event; @@ -386,18 +433,19 @@ NTSTATUS wg_transform_create(void *args) if (!(transform->allocator = wg_allocator_create())) goto out; transform->attrs = *params->attrs; + transform->input_format = input_format; transform->output_format = output_format; - if (!(src_caps = transform_format_to_caps(transform, &input_format))) + if (!(transform->input_caps = transform_format_to_caps(transform, &input_format))) goto out; - if (!(template = gst_pad_template_new("src", GST_PAD_SRC, GST_PAD_ALWAYS, src_caps))) + if (!(template = gst_pad_template_new("src", GST_PAD_SRC, GST_PAD_ALWAYS, transform->input_caps))) goto out; transform->my_src = gst_pad_new_from_template(template, "src"); g_object_unref(template); if (!transform->my_src) goto out; - GST_INFO("transform %p input caps %"GST_PTR_FORMAT, transform, src_caps); + GST_INFO("transform %p input caps %"GST_PTR_FORMAT, transform, transform->input_caps); gst_pad_set_element_private(transform->my_src, transform); gst_pad_set_query_function(transform->my_src, transform_src_query_cb); @@ -436,7 +484,7 @@ NTSTATUS wg_transform_create(void *args) case WG_MAJOR_TYPE_VIDEO_INDEO: case WG_MAJOR_TYPE_VIDEO_WMV: case WG_MAJOR_TYPE_VIDEO_MPEG1: - if (!(element = find_element(GST_ELEMENT_FACTORY_TYPE_DECODER, src_caps, raw_caps)) + if (!(element = find_element(GST_ELEMENT_FACTORY_TYPE_DECODER, transform->input_caps, raw_caps)) || !append_element(transform->container, element, &first, &last)) { gst_caps_unref(raw_caps); @@ -476,15 +524,6 @@ NTSTATUS wg_transform_create(void *args) break; case WG_MAJOR_TYPE_VIDEO: - if (!(element = create_element("videoconvert", "base")) - || !append_element(transform->container, element, &first, &last)) - goto out; - if (!(transform->video_flip = create_element("videoflip", "base")) - || !append_element(transform->container, transform->video_flip, &first, &last)) - goto out; - transform->input_is_flipped = wg_format_video_is_flipped(&input_format); - if (transform->input_is_flipped != wg_format_video_is_flipped(&output_format)) - gst_util_set_object_arg(G_OBJECT(transform->video_flip), "method", "vertical-flip"); if (!(element = create_element("videoconvert", "base")) || !append_element(transform->container, element, &first, &last)) goto out; @@ -521,7 +560,7 @@ NTSTATUS wg_transform_create(void *args) if (!(event = gst_event_new_stream_start("stream")) || !push_event(transform->my_src, event)) goto out; - if (!(event = gst_event_new_caps(src_caps)) + if (!(event = gst_event_new_caps(transform->input_caps)) || !push_event(transform->my_src, event)) goto out; @@ -534,8 +573,6 @@ NTSTATUS wg_transform_create(void *args) || !push_event(transform->my_src, event)) goto out; - gst_caps_unref(src_caps); - GST_INFO("Created winegstreamer transform %p.", transform); params->transform = (wg_transform_t)(ULONG_PTR)transform; return STATUS_SUCCESS; @@ -547,8 +584,8 @@ out: gst_caps_unref(transform->output_caps); if (transform->my_src) gst_object_unref(transform->my_src); - if (src_caps) - gst_caps_unref(src_caps); + if (transform->input_caps) + gst_caps_unref(transform->input_caps); if (transform->allocator) wg_allocator_destroy(transform->allocator); if (transform->drain_query) @@ -599,15 +636,6 @@ NTSTATUS wg_transform_set_output_format(void *args) gst_caps_unref(transform->output_caps); transform->output_caps = caps; - if (transform->video_flip) - { - const char *value; - if (transform->input_is_flipped != wg_format_video_is_flipped(&output_format)) - value = "vertical-flip"; - else - value = "none"; - gst_util_set_object_arg(G_OBJECT(transform->video_flip), "method", value); - } if (!push_event(transform->my_sink, gst_event_new_reconfigure())) { GST_ERROR("Failed to reconfigure transform %p.", transform); @@ -641,6 +669,7 @@ NTSTATUS wg_transform_push_data(void *args) struct wg_transform_push_data_params *params = args; struct wg_transform *transform = get_transform(params->transform); struct wg_sample *sample = params->sample; + GstVideoInfo video_info; GstBuffer *buffer; guint length; @@ -664,6 +693,14 @@ NTSTATUS wg_transform_push_data(void *args) GST_INFO("Wrapped %u/%u bytes from sample %p to %"GST_PTR_FORMAT, sample->size, sample->max_size, sample, buffer); } + if (stream_type_from_caps(transform->input_caps) == GST_STREAM_TYPE_VIDEO + && gst_video_info_from_caps(&video_info, transform->input_caps)) + { + GstVideoAlignment align; + align_video_info_planes(&transform->input_format, 0, &video_info, &align); + buffer_add_video_meta(buffer, &video_info); + } + if (sample->flags & WG_SAMPLE_FLAG_HAS_PTS) GST_BUFFER_PTS(buffer) = sample->pts * 100; if (sample->flags & WG_SAMPLE_FLAG_HAS_DURATION) @@ -898,13 +935,22 @@ NTSTATUS wg_transform_read_data(void *args) if (stream_type_from_caps(output_caps) == GST_STREAM_TYPE_VIDEO) { gsize plane_align = transform->attrs.output_plane_align; + GstVideoMeta *meta; if (!gst_video_info_from_caps(&src_video_info, output_caps)) GST_ERROR("Failed to get video info from %"GST_PTR_FORMAT, output_caps); dst_video_info = src_video_info; - /* set the desired output buffer alignment on the dest video info */ - align_video_info_planes(plane_align, &dst_video_info, &align); + /* set the desired output buffer alignment and stride on the dest video info */ + align_video_info_planes(&transform->output_format, plane_align, + &dst_video_info, &align); + + /* copy the actual output buffer alignment and stride to the src video info */ + if ((meta = gst_buffer_get_video_meta(output_buffer))) + { + memcpy(src_video_info.offset, meta->offset, sizeof(meta->offset)); + memcpy(src_video_info.stride, meta->stride, sizeof(meta->stride)); + } } if (GST_MINI_OBJECT_FLAG_IS_SET(transform->output_sample, GST_SAMPLE_FLAG_WG_CAPS_CHANGED)) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5055
From: Rémi Bernon <rbernon(a)codeweavers.com> Because it was inconsistently done before we need to adjust the frame size temporarily on the wg_transform side. This fixes one test but breaks a different one. Next change will fix it properly. --- dlls/mf/tests/transform.c | 20 ++++++++++-- dlls/winegstreamer/mfplat.c | 33 ++++++++++--------- dlls/winegstreamer/wg_transform.c | 53 +++++++++++++++++++++++++++---- 3 files changed, 81 insertions(+), 25 deletions(-) diff --git a/dlls/mf/tests/transform.c b/dlls/mf/tests/transform.c index 468239b0fd4..a13d9cb79c1 100644 --- a/dlls/mf/tests/transform.c +++ b/dlls/mf/tests/transform.c @@ -7394,6 +7394,19 @@ static void test_video_processor(void) .sample_time = 0, .sample_duration = 10000000, .buffer_count = 1, .buffers = &rgb32_buffer_desc, }; + const struct buffer_desc rgb32_buffer_desc_todo = + { + .length = actual_width * actual_height * 4, + .compare = compare_rgb32, .dump = dump_rgb32, .rect = {.top = 12, .right = 82, .bottom = 96}, + .todo_length = TRUE, + }; + const struct sample_desc rgb32_sample_desc_todo = + { + .attributes = output_sample_attributes, + .sample_time = 0, .sample_duration = 10000000, + .buffer_count = 1, .buffers = &rgb32_buffer_desc_todo, + .todo_length = TRUE, + }; const struct buffer_desc rgb555_buffer_desc = { @@ -7480,7 +7493,7 @@ static void test_video_processor(void) }, { .input_type_desc = rgb32_with_aperture, .output_type_desc = rgb32_with_aperture, - .output_sample_desc = &rgb32_sample_desc, .result_bitmap = L"rgb32frame.bmp", + .output_sample_desc = &rgb32_sample_desc_todo, .result_bitmap = L"rgb32frame.bmp", .broken = TRUE /* old Windows version incorrectly rescale */ }, { @@ -7915,6 +7928,7 @@ static void test_video_processor(void) ok(ref == 1, "Release returned %ld\n", ref); ret = check_mf_sample_collection(output_samples, test->output_sample_desc, test->result_bitmap); + todo_wine_if(i == 10) ok(ret <= test->delta || broken(test->broken), "got %lu%% diff\n", ret); IMFCollection_Release(output_samples); @@ -7947,8 +7961,8 @@ static void test_video_processor(void) check_mft_set_output_type(transform, rgb32_no_aperture, S_OK); check_mft_get_output_current_type(transform, rgb32_no_aperture); - check_mft_set_input_type_(__LINE__, transform, nv12_with_aperture, TRUE); - check_mft_get_input_current_type_(__LINE__, transform, nv12_with_aperture, TRUE, FALSE); + check_mft_set_input_type(transform, nv12_with_aperture); + check_mft_get_input_current_type(transform, nv12_with_aperture); /* output type is the same as before */ check_mft_get_output_current_type(transform, rgb32_no_aperture); diff --git a/dlls/winegstreamer/mfplat.c b/dlls/winegstreamer/mfplat.c index e6d9fb9fd2c..c27a2646937 100644 --- a/dlls/winegstreamer/mfplat.c +++ b/dlls/winegstreamer/mfplat.c @@ -519,6 +519,22 @@ static IMFMediaType *mf_media_type_from_wg_format_video(const struct wg_format * if (FAILED(MFCreateMediaType(&type))) return NULL; + if (format->u.video.padding.left || format->u.video.padding.right + || format->u.video.padding.top || format->u.video.padding.bottom) + { + MFVideoArea aperture = + { + .OffsetX = {.value = format->u.video.padding.left}, + .OffsetY = {.value = format->u.video.padding.top}, + .Area.cx = width, .Area.cy = height, + }; + width += format->u.video.padding.left + format->u.video.padding.right; + height += format->u.video.padding.top + format->u.video.padding.bottom; + + IMFMediaType_SetBlob(type, &MF_MT_MINIMUM_DISPLAY_APERTURE, + (BYTE *)&aperture, sizeof(aperture)); + } + IMFMediaType_SetGUID(type, &MF_MT_MAJOR_TYPE, &MFMediaType_Video); IMFMediaType_SetGUID(type, &MF_MT_SUBTYPE, video_formats[i].subtype); IMFMediaType_SetUINT64(type, &MF_MT_FRAME_SIZE, make_uint64(width, height)); @@ -532,21 +548,6 @@ static IMFMediaType *mf_media_type_from_wg_format_video(const struct wg_format * stride = -stride; IMFMediaType_SetUINT32(type, &MF_MT_DEFAULT_STRIDE, stride); - if (format->u.video.padding.left || format->u.video.padding.right - || format->u.video.padding.top || format->u.video.padding.bottom) - { - MFVideoArea aperture = - { - .OffsetX = {.value = format->u.video.padding.left}, - .OffsetY = {.value = format->u.video.padding.top}, - .Area.cx = width - format->u.video.padding.right - format->u.video.padding.left, - .Area.cy = height - format->u.video.padding.bottom - format->u.video.padding.top, - }; - - IMFMediaType_SetBlob(type, &MF_MT_MINIMUM_DISPLAY_APERTURE, - (BYTE *)&aperture, sizeof(aperture)); - } - return type; } } @@ -706,6 +707,8 @@ static void mf_media_type_to_wg_format_video(IMFMediaType *type, const GUID *sub format->u.video.padding.top = aperture.OffsetY.value; format->u.video.padding.right = format->u.video.width - aperture.Area.cx - aperture.OffsetX.value; format->u.video.padding.bottom = format->u.video.height - aperture.Area.cy - aperture.OffsetY.value; + format->u.video.width -= format->u.video.padding.left + format->u.video.padding.right; + format->u.video.height -= format->u.video.padding.top + format->u.video.padding.bottom; } if (SUCCEEDED(IMFMediaType_GetUINT64(type, &MF_MT_FRAME_RATE, &frame_rate)) && (UINT32)frame_rate) diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 6882e16bf1c..65b95229096 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -92,6 +92,48 @@ static void align_video_info_planes(struct wg_format *format, gsize plane_align, } } +static void update_format_width_padding(struct wg_format *max, struct wg_format *min) +{ + max->u.video.padding.right += max->u.video.width - min->u.video.width; + max->u.video.width = min->u.video.width; +} + +static void update_format_height_padding(struct wg_format *max, struct wg_format *min) +{ + max->u.video.padding.bottom += abs(max->u.video.height) - abs(min->u.video.height); + max->u.video.height = (max->u.video.height < 0 ? -1 : 1) * abs(min->u.video.height); +} + +static void update_format_padding(struct wg_format *input_format, struct wg_format *output_format) +{ + if (input_format->major_type == WG_MAJOR_TYPE_VIDEO) + { + input_format->u.video.width += input_format->u.video.padding.left + input_format->u.video.padding.right; + input_format->u.video.height += input_format->u.video.padding.top + input_format->u.video.padding.bottom; + } + + if (output_format->major_type == WG_MAJOR_TYPE_VIDEO) + { + output_format->u.video.width += output_format->u.video.padding.left + output_format->u.video.padding.right; + output_format->u.video.height += output_format->u.video.padding.top + output_format->u.video.padding.bottom; + } + + if (input_format->major_type != output_format->major_type) + return; + if (input_format->major_type != WG_MAJOR_TYPE_VIDEO) + return; + + if (input_format->u.video.width > output_format->u.video.width) + update_format_width_padding(input_format, output_format); + else + update_format_width_padding(output_format, input_format); + + if (abs(input_format->u.video.height) > abs(output_format->u.video.height)) + update_format_height_padding(input_format, output_format); + else + update_format_height_padding(output_format, input_format); +} + typedef struct { GstVideoBufferPool parent; @@ -433,6 +475,8 @@ NTSTATUS wg_transform_create(void *args) if (!(transform->allocator = wg_allocator_create())) goto out; transform->attrs = *params->attrs; + + update_format_padding(&input_format, &output_format); transform->input_format = input_format; transform->output_format = output_format; @@ -964,18 +1008,13 @@ NTSTATUS wg_transform_read_data(void *args) wg_format_from_caps(&output_format, output_caps); if (output_format.major_type == WG_MAJOR_TYPE_VIDEO) { + if (transform->output_format.u.video.height < 0) + output_format.u.video.height *= -1; output_format.u.video.padding.left = align.padding_left; - output_format.u.video.width += output_format.u.video.padding.left; output_format.u.video.padding.right = align.padding_right; - output_format.u.video.width += output_format.u.video.padding.right; output_format.u.video.padding.top = align.padding_top; - output_format.u.video.height += output_format.u.video.padding.top; output_format.u.video.padding.bottom = align.padding_bottom; - output_format.u.video.height += output_format.u.video.padding.bottom; GST_INFO("new video padding rect %s", wine_dbgstr_rect(&output_format.u.video.padding)); - - if (transform->output_format.u.video.height < 0) - output_format.u.video.height *= -1; } if (format) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5055
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/mf/tests/transform.c | 16 +--------------- dlls/winegstreamer/wg_transform.c | 32 +++++++++++++++++++------------ 2 files changed, 21 insertions(+), 27 deletions(-) diff --git a/dlls/mf/tests/transform.c b/dlls/mf/tests/transform.c index a13d9cb79c1..0129f8a112b 100644 --- a/dlls/mf/tests/transform.c +++ b/dlls/mf/tests/transform.c @@ -7394,19 +7394,6 @@ static void test_video_processor(void) .sample_time = 0, .sample_duration = 10000000, .buffer_count = 1, .buffers = &rgb32_buffer_desc, }; - const struct buffer_desc rgb32_buffer_desc_todo = - { - .length = actual_width * actual_height * 4, - .compare = compare_rgb32, .dump = dump_rgb32, .rect = {.top = 12, .right = 82, .bottom = 96}, - .todo_length = TRUE, - }; - const struct sample_desc rgb32_sample_desc_todo = - { - .attributes = output_sample_attributes, - .sample_time = 0, .sample_duration = 10000000, - .buffer_count = 1, .buffers = &rgb32_buffer_desc_todo, - .todo_length = TRUE, - }; const struct buffer_desc rgb555_buffer_desc = { @@ -7493,7 +7480,7 @@ static void test_video_processor(void) }, { .input_type_desc = rgb32_with_aperture, .output_type_desc = rgb32_with_aperture, - .output_sample_desc = &rgb32_sample_desc_todo, .result_bitmap = L"rgb32frame.bmp", + .output_sample_desc = &rgb32_sample_desc, .result_bitmap = L"rgb32frame.bmp", .broken = TRUE /* old Windows version incorrectly rescale */ }, { @@ -7928,7 +7915,6 @@ static void test_video_processor(void) ok(ref == 1, "Release returned %ld\n", ref); ret = check_mf_sample_collection(output_samples, test->output_sample_desc, test->result_bitmap); - todo_wine_if(i == 10) ok(ret <= test->delta || broken(test->broken), "got %lu%% diff\n", ret); IMFCollection_Release(output_samples); diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 65b95229096..47ddd0ff81f 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -75,6 +75,18 @@ static void align_video_info_planes(struct wg_format *format, gsize plane_align, align->padding_right = ((plane_align + 1) - (info->width & plane_align)) & plane_align; align->padding_bottom = ((plane_align + 1) - (info->height & plane_align)) & plane_align; + align->padding_right = max(align->padding_right, format->u.video.padding.right); + align->padding_bottom = max(align->padding_bottom, format->u.video.padding.bottom); + align->padding_top = format->u.video.padding.top; + align->padding_left = format->u.video.padding.left; + + if (format->u.video.height < 0) + { + gsize top = align->padding_top; + align->padding_top = align->padding_bottom; + align->padding_bottom = top; + } + align->stride_align[0] = plane_align; align->stride_align[1] = plane_align; align->stride_align[2] = plane_align; @@ -106,18 +118,6 @@ static void update_format_height_padding(struct wg_format *max, struct wg_format static void update_format_padding(struct wg_format *input_format, struct wg_format *output_format) { - if (input_format->major_type == WG_MAJOR_TYPE_VIDEO) - { - input_format->u.video.width += input_format->u.video.padding.left + input_format->u.video.padding.right; - input_format->u.video.height += input_format->u.video.padding.top + input_format->u.video.padding.bottom; - } - - if (output_format->major_type == WG_MAJOR_TYPE_VIDEO) - { - output_format->u.video.width += output_format->u.video.padding.left + output_format->u.video.padding.right; - output_format->u.video.height += output_format->u.video.padding.top + output_format->u.video.padding.bottom; - } - if (input_format->major_type != output_format->major_type) return; if (input_format->major_type != WG_MAJOR_TYPE_VIDEO) @@ -476,6 +476,7 @@ NTSTATUS wg_transform_create(void *args) goto out; transform->attrs = *params->attrs; + /* GStreamer cannot include the buffer padding in the frame sizes but MF does, make sure the formats have the same */ update_format_padding(&input_format, &output_format); transform->input_format = input_format; transform->output_format = output_format; @@ -656,6 +657,13 @@ NTSTATUS wg_transform_set_output_format(void *args) GstSample *sample; GstCaps *caps; + if (output_format.major_type == WG_MAJOR_TYPE_VIDEO) + { + /* GStreamer cannot include the buffer padding in the frame sizes but MF does, make sure the formats have the same */ + update_format_width_padding(&output_format, &transform->output_format); + update_format_height_padding(&output_format, &transform->output_format); + } + if (!(caps = transform_format_to_caps(transform, &output_format))) { GST_ERROR("Failed to convert format to caps."); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5055
v2: Split read_transform_output_data to avoid ptr variables, add more description to the commits, split last change to change wg_format semantic separately. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5055#note_61439
This removes the need for an extra videoconvert, and any ambiguity that comes with it.
Ambiguity? What does that mean?
Also reduce our dependency on the GStreamer elements to behave as we assume they do, and reduce the chances to see unnecessary copies happening which can be harmful with large buffers. Some GStreamer versions for instance have a bogus videoflip that causes a buffer copy to happen when it is supposed to be pass through.
If videoflip is copying when it should be passthrough then that should be fixed upstream. I don't want to introduce a bunch of extra code just to work around a GStreamer bug, especially if it's already fixed upstream (which is not clear from this commit message).
+ update_format_padding(&input_format, &output_format);
I still am failing to understand this at all, sorry. Why is the format that's set on the transform not the format we store? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5055#note_61508
Ambiguity? What does that mean?
It means that we have no control over what `videoconvert | videoflip | videoconvert` will actually do, and we can only hope that it will do something sensible and, for instance, not decide to color convert unnecessarily or fail to pass our pool through when it could.
If videoflip is copying when it should be passthrough then that should be fixed upstream. I don't want to introduce a bunch of extra code just to work around a GStreamer bug, especially if it's already fixed upstream (which is not clear from this commit message).
Sure, but we cannot fix older GStreamer versions, so lets also fix it on our side the right way, which is to provide the correct stride information on our buffers, and reduce the complexity of our pipelines, which will help reducing the risk and ease the debugging.
I still am failing to understand this at all, sorry. Why is the format that's set on the transform not the format we store?
Like the test shows, the input and output media types don't have to match their frame size exactly or can include or omit frame padding freely. This ends up with buffers being passed through with padding, with padding added or cropped accordingly. However, GStreamer is unable to convey the buffer padding information in its caps and it is a buffer property only. When it matches caps, both input and output frame sizes have to match exactly. If the client called the video processor SetInputType with a frame size of 96x96 and an aperture to 82x84, and SetOutputType with a frame size of 96x96 but without any aperture, we will try to create input/output formats with two different frame size, which would fail the caps negociation. We need to consider in this case that the client wanted a 82x84 output with 14x12 padding. Other combinations need to be handled as well, so this takes the smaller frame size on input and output and consider any extra to be included in buffer padding. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5055#note_61514
Ambiguity? What does that mean?
It means that we have no control over what `videoconvert | videoflip | videoconvert` will actually do, and we can only hope that it will do something sensible and, for instance, not decide to color convert unnecessarily or fail to pass our pool through when it could.
That seems unnecessarily defeatist...? The behaviour of those elements is documented. If they're failing to respect parts of the GStreamer API, that's a bug in the element and it should be fixed there. If they're doing something less performantly than they should, that's also something that should be fixed in the element.
If videoflip is copying when it should be passthrough then that should be fixed upstream. I don't want to introduce a bunch of extra code just to work around a GStreamer bug, especially if it's already fixed upstream (which is not clear from this commit message).
Sure, but we cannot fix older GStreamer versions, so lets also fix it on our side the right way, which is to provide the correct stride information on our buffers, and reduce the complexity of our pipelines, which will help reducing the risk and ease the debugging.
Why is this any more the "right" way than using videoflip? I also don't understand the bit about complexity. From a somewhat abstract level, code gets more complex and harder to work with when you add multiple *different* interacting components. Having more of the *same* component—in this case, adding more beads to a string of postprocessing elements—doesn't make anything harder to work with.
I still am failing to understand this at all, sorry. Why is the format that's set on the transform not the format we store?
Like the test shows, the input and output media types don't have to match their frame size exactly or can include or omit frame padding freely. This ends up with buffers being passed through with padding, with padding added or cropped accordingly.
However, GStreamer is unable to convey the buffer padding information in its caps and it is a buffer property only. When it matches caps, both input and output frame sizes have to match exactly.
That part all makes sense, and it seems sensible to me that Media Foundation would let you arbitrarily add or remove padding from a frame.
If the client called the video processor SetInputType with a frame size of 96x96 and an aperture to 82x84, and SetOutputType with a frame size of 96x96 but without any aperture, we will try to create input/output formats with two different frame size, which would fail the caps negociation.
We need to consider in this case that the client wanted a 82x84 output with 14x12 padding. Other combinations need to be handled as well, so this takes the smaller frame size on input and output and consider any extra to be included in buffer padding.
This is the surprising part—viz. that the actual content size can fail to match. It's not what the code was doing before, but it's not described in the patch subject or a comment either. And it seems like it should be unrelated to the main purpose of *either* patch 8 (stop including padding in the width/height) or patch 9 (which still seems like it should just be a matter of putting meta on the input buffers, and I'm confused why we're not doing that.) And I must be missing something, because I don't see this situation tested in test_video_processor()? I only see tests where both the input and output have the same aperture. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5055#note_61519
That seems unnecessarily defeatist...? The behaviour of those elements is documented. If they're failing to respect parts of the GStreamer API, that's a bug in the element and it should be fixed there. If they're doing something less performantly than they should, that's also something that should be fixed in the element.
This has nothing to do with documentation, and the behavior of components when combined together isn't documented anyway. They can behave as they will, and all depends on the caps negotiation that happens between them and depends on their capabilities. The videoflip elements has specific video format requirements, and can very well end up causing suboptimal negotiation in the pipeline.
Why is this any more the "right" way than using videoflip?
We are not doing any kind of frame flipping, but instead we are implementing buffers with negative strides. Providing stride information to GStreamer is the right way to tell about buffer stride. Using a videoflip element is an equivalent but convoluted way to do it.
I also don't understand the bit about complexity. From a somewhat abstract level, code gets more complex and harder to work with when you add multiple *different* interacting components. Having more of the *same* component—in this case, adding more beads to a string of postprocessing elements—doesn't make anything harder to work with.
Of course it does, it increases the number of possible failures. Doesn't matter of the components are the same, the more you add the more complex it gets. And the worst part is that it's not components we have the source directly available, it's GStreamer components which are most often pre-built from the system distribution. Take debugging of the current video processor pipeline for instance, you have three elements when we could have only one. The two videoconvert and videoflip are talking to each other back and forth to negotiate their caps. Decyphering the GStreamer trace to understand what is actually going on, and figure out in the end that somewhere in the middle of all these verbose traces, videoflip has decided to drop the pool it was provided by downstream element to use its own, *is* way more complicated than it could be.
And I must be missing something, because I don't see this situation tested in test_video_processor()? I only see tests where both the input and output have the same aperture.
There's one test which is fixed with this MR (well now that I've split the last patch, there's another one which is broken then fixed), and it is about using an aperture on input and no aperture on output. It fails before this MR, it passes after. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5055#note_61520
participants (2)
-
Rémi Bernon -
Zebediah Figura (＠zfigura)