[PATCH 0/5] MR5971: winegstreamer/video_encoder: More of encoder implementation.
From: Ziqing Hui <zhui(a)codeweavers.com> --- dlls/mf/tests/transform.c | 53 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 51 insertions(+), 2 deletions(-) diff --git a/dlls/mf/tests/transform.c b/dlls/mf/tests/transform.c index deec2c7dd92..16b8416b4ae 100644 --- a/dlls/mf/tests/transform.c +++ b/dlls/mf/tests/transform.c @@ -310,6 +310,30 @@ void check_attributes_(const char *file, int line, IMFAttributes *attributes, } } +static void set_attribute_desc(struct attribute_desc *desc, const struct attribute_desc *to_set, int count) +{ + static const struct attribute_desc empty = {0}; + int i, j; + + for (i = 0; i < count; ++i) + { + for (j = 0; desc[j].key; ++j) + { + if (IsEqualGUID(desc[j].key, to_set[i].key)) + { + desc[j] = to_set[i]; + break; + } + } + + if (!desc[j].key) + { + desc[j] = to_set[i]; + desc[j + 1] = empty; + } + } +} + struct transform_info { const WCHAR *name; @@ -3886,6 +3910,14 @@ static void test_h264_encoder(void) ATTR_UINT32(test_attr_guid, 0), {0}, }; + static const struct attribute_desc to_set_attributes[] = + { + ATTR_RATIO(MF_MT_FRAME_SIZE, actual_width * 2, actual_height * 2), + ATTR_RATIO(MF_MT_FRAME_RATE, 10, 1), + ATTR_UINT32(MF_MT_INTERLACE_MODE, MFVideoInterlace_MixedInterlaceOrProgressive), + ATTR_UINT32(MF_MT_VIDEO_NOMINAL_RANGE, MFNominalRange_Normal), + ATTR_RATIO(MF_MT_PIXEL_ASPECT_RATIO, 2, 1), + }; const struct attribute_desc expect_input_type_desc[] = { ATTR_GUID(MF_MT_MAJOR_TYPE, MFMediaType_Video), @@ -3907,9 +3939,10 @@ static void test_h264_encoder(void) ATTR_UINT32(test_attr_guid, 0), {0}, }; - static const MFT_OUTPUT_STREAM_INFO expect_output_info = {.cbSize = 0x8000}; + static const MFT_OUTPUT_STREAM_INFO expect_output_info[] = {{.cbSize = 0x8000}, {.cbSize = 0x10e00}}; MFT_REGISTER_TYPE_INFO output_type = {MFMediaType_Video, MFVideoFormat_H264}; MFT_REGISTER_TYPE_INFO input_type = {MFMediaType_Video, MFVideoFormat_NV12}; + media_type_desc type_desc; IMFMediaType *media_type; IMFTransform *transform; HRESULT hr; @@ -3970,7 +4003,7 @@ static void test_h264_encoder(void) check_mft_set_output_type_required(transform, output_type_desc); check_mft_set_output_type(transform, output_type_desc, S_OK); check_mft_get_output_current_type(transform, expect_output_type_desc); - check_mft_get_output_stream_info(transform, S_OK, &expect_output_info); + check_mft_get_output_stream_info(transform, S_OK, &expect_output_info[0]); /* Input types can now be enumerated. */ i = -1; @@ -3993,6 +4026,22 @@ static void test_h264_encoder(void) check_mft_get_input_current_type(transform, expect_input_type_desc); check_mft_get_input_stream_info(transform, S_OK, NULL); + /* Input type attributes should match output type attributes. */ + for (i = 0; i < ARRAY_SIZE(to_set_attributes); ++i) + { + winetest_push_context("attr %lu", i); + memcpy(type_desc, input_type_desc, sizeof(input_type_desc)); + set_attribute_desc(type_desc, &to_set_attributes[i], 1); + check_mft_set_input_type(transform, type_desc, MF_E_INVALIDMEDIATYPE); + winetest_pop_context(); + } + + /* Output info cbSize will change if we change output type attributes. */ + memcpy(type_desc, output_type_desc, sizeof(output_type_desc)); + set_attribute_desc(type_desc, to_set_attributes, ARRAY_SIZE(to_set_attributes)); + check_mft_set_output_type(transform, type_desc, S_OK); + check_mft_get_output_stream_info(transform, S_OK, &expect_output_info[1]); + ret = IMFTransform_Release(transform); ok(ret == 0, "Release returned %lu\n", ret); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5971
From: Ziqing Hui <zhui(a)codeweavers.com> --- dlls/winegstreamer/video_encoder.c | 74 ++++++++++++++++-------------- 1 file changed, 40 insertions(+), 34 deletions(-) diff --git a/dlls/winegstreamer/video_encoder.c b/dlls/winegstreamer/video_encoder.c index cfdd5be9d50..c880cd062ce 100644 --- a/dlls/winegstreamer/video_encoder.c +++ b/dlls/winegstreamer/video_encoder.c @@ -54,6 +54,45 @@ static inline struct video_encoder *impl_from_IMFTransform(IMFTransform *iface) return CONTAINING_RECORD(iface, struct video_encoder, IMFTransform_iface); } +static HRESULT create_input_type(struct video_encoder *encoder, const GUID *subtype, IMFMediaType **out) +{ + IMFVideoMediaType *input_type; + UINT64 ratio; + UINT32 value; + HRESULT hr; + + if (FAILED(hr = MFCreateVideoMediaTypeFromSubtype(subtype, &input_type))) + return hr; + + if (FAILED(hr = IMFMediaType_GetUINT64(encoder->output_type, &MF_MT_FRAME_SIZE, &ratio)) + || FAILED(hr = IMFVideoMediaType_SetUINT64(input_type, &MF_MT_FRAME_SIZE, ratio))) + goto done; + + if (FAILED(hr = IMFMediaType_GetUINT64(encoder->output_type, &MF_MT_FRAME_RATE, &ratio)) + || FAILED(hr = IMFVideoMediaType_SetUINT64(input_type, &MF_MT_FRAME_RATE, ratio))) + goto done; + + if (FAILED(hr = IMFMediaType_GetUINT32(encoder->output_type, &MF_MT_INTERLACE_MODE, &value)) + || FAILED(hr = IMFVideoMediaType_SetUINT32(input_type, &MF_MT_INTERLACE_MODE, value))) + goto done; + + if (FAILED(IMFMediaType_GetUINT32(encoder->output_type, &MF_MT_VIDEO_NOMINAL_RANGE, &value))) + value = MFNominalRange_Wide; + if (FAILED(hr = IMFVideoMediaType_SetUINT32(input_type, &MF_MT_VIDEO_NOMINAL_RANGE, value))) + goto done; + + if (FAILED(IMFMediaType_GetUINT64(encoder->output_type, &MF_MT_PIXEL_ASPECT_RATIO, &ratio))) + ratio = (UINT64)1 << 32 | 1; + if (FAILED(hr = IMFVideoMediaType_SetUINT64(input_type, &MF_MT_PIXEL_ASPECT_RATIO, ratio))) + goto done; + + IMFMediaType_AddRef((*out = (IMFMediaType *)input_type)); + +done: + IMFVideoMediaType_Release(input_type); + return hr; +} + static HRESULT WINAPI transform_QueryInterface(IMFTransform *iface, REFIID iid, void **out) { struct video_encoder *encoder = impl_from_IMFTransform(iface); @@ -180,10 +219,6 @@ static HRESULT WINAPI transform_GetInputAvailableType(IMFTransform *iface, DWORD IMFMediaType **type) { struct video_encoder *encoder = impl_from_IMFTransform(iface); - IMFVideoMediaType *input_type; - UINT64 ratio; - UINT32 value; - HRESULT hr; TRACE("iface %p, id %#lx, index %#lx, type %p.\n", iface, id, index, type); @@ -194,36 +229,7 @@ static HRESULT WINAPI transform_GetInputAvailableType(IMFTransform *iface, DWORD if (index >= encoder->input_type_count) return MF_E_NO_MORE_TYPES; - if (!(hr = MFCreateVideoMediaTypeFromSubtype(encoder->input_types[index], &input_type))) - return hr; - - if (FAILED(hr = IMFMediaType_GetUINT64(encoder->output_type, &MF_MT_FRAME_SIZE, &ratio)) - || FAILED(hr = IMFVideoMediaType_SetUINT64(input_type, &MF_MT_FRAME_SIZE, ratio))) - goto done; - - if (FAILED(hr = IMFMediaType_GetUINT64(encoder->output_type, &MF_MT_FRAME_RATE, &ratio)) - || FAILED(hr = IMFVideoMediaType_SetUINT64(input_type, &MF_MT_FRAME_RATE, ratio))) - goto done; - - if (FAILED(hr = IMFMediaType_GetUINT32(encoder->output_type, &MF_MT_INTERLACE_MODE, &value)) - || FAILED(hr = IMFVideoMediaType_SetUINT32(input_type, &MF_MT_INTERLACE_MODE, value))) - goto done; - - if (FAILED(IMFMediaType_GetUINT32(encoder->output_type, &MF_MT_VIDEO_NOMINAL_RANGE, &value))) - value = MFNominalRange_Wide; - if (FAILED(hr = IMFVideoMediaType_SetUINT32(input_type, &MF_MT_VIDEO_NOMINAL_RANGE, value))) - goto done; - - if (FAILED(IMFMediaType_GetUINT64(encoder->output_type, &MF_MT_PIXEL_ASPECT_RATIO, &ratio))) - ratio = (UINT64)1 << 32 | 1; - if (FAILED(hr = IMFVideoMediaType_SetUINT64(input_type, &MF_MT_PIXEL_ASPECT_RATIO, ratio))) - goto done; - - IMFMediaType_AddRef((*type = (IMFMediaType *)input_type)); - -done: - IMFVideoMediaType_Release(input_type); - return hr; + return create_input_type(encoder, encoder->input_types[index], type); } static HRESULT WINAPI transform_GetOutputAvailableType(IMFTransform *iface, DWORD id, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5971
From: Ziqing Hui <zhui(a)codeweavers.com> --- dlls/winegstreamer/video_encoder.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/dlls/winegstreamer/video_encoder.c b/dlls/winegstreamer/video_encoder.c index c880cd062ce..7fd0e3c9da0 100644 --- a/dlls/winegstreamer/video_encoder.c +++ b/dlls/winegstreamer/video_encoder.c @@ -248,8 +248,10 @@ static HRESULT WINAPI transform_GetOutputAvailableType(IMFTransform *iface, DWOR static HRESULT WINAPI transform_SetInputType(IMFTransform *iface, DWORD id, IMFMediaType *type, DWORD flags) { struct video_encoder *encoder = impl_from_IMFTransform(iface); + IMFMediaType *good_input_type; GUID major, subtype; - UINT64 ratio; + BOOL result; + HRESULT hr; ULONG i; TRACE("iface %p, id %#lx, type %p, flags %#lx.\n", iface, id, type, flags); @@ -281,8 +283,12 @@ static HRESULT WINAPI transform_SetInputType(IMFTransform *iface, DWORD id, IMFM if (i == encoder->input_type_count) return MF_E_INVALIDMEDIATYPE; - if (FAILED(IMFMediaType_GetUINT64(type, &MF_MT_FRAME_SIZE, &ratio)) - || FAILED(IMFMediaType_GetUINT64(type, &MF_MT_FRAME_RATE, &ratio))) + if (FAILED(hr = create_input_type(encoder, &subtype, &good_input_type))) + return hr; + if (FAILED(hr = IMFMediaType_Compare(good_input_type, (IMFAttributes *)type, + MF_ATTRIBUTES_MATCH_OUR_ITEMS, &result))) + return hr; + if (!result) return MF_E_INVALIDMEDIATYPE; if (flags & MFT_SET_TYPE_TEST_ONLY) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5971
From: Ziqing Hui <zhui(a)codeweavers.com> --- dlls/winegstreamer/video_encoder.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/dlls/winegstreamer/video_encoder.c b/dlls/winegstreamer/video_encoder.c index 7fd0e3c9da0..7f693a55031 100644 --- a/dlls/winegstreamer/video_encoder.c +++ b/dlls/winegstreamer/video_encoder.c @@ -44,6 +44,7 @@ struct video_encoder UINT output_type_count; IMFMediaType *input_type; + MFT_INPUT_STREAM_INFO input_info; IMFMediaType *output_type; IMFAttributes *attributes; @@ -168,8 +169,12 @@ static HRESULT WINAPI transform_GetStreamIDs(IMFTransform *iface, DWORD input_si static HRESULT WINAPI transform_GetInputStreamInfo(IMFTransform *iface, DWORD id, MFT_INPUT_STREAM_INFO *info) { - FIXME("iface %p, id %#lx, info %p.\n", iface, id, info); - return E_NOTIMPL; + struct video_encoder *encoder = impl_from_IMFTransform(iface); + + TRACE("iface %p, id %#lx, info %p.\n", iface, id, info); + + *info = encoder->input_info; + return S_OK; } static HRESULT WINAPI transform_GetOutputStreamInfo(IMFTransform *iface, DWORD id, MFT_OUTPUT_STREAM_INFO *info) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5971
From: Ziqing Hui <zhui(a)codeweavers.com> --- dlls/winegstreamer/video_encoder.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/dlls/winegstreamer/video_encoder.c b/dlls/winegstreamer/video_encoder.c index 7f693a55031..40f116e21e3 100644 --- a/dlls/winegstreamer/video_encoder.c +++ b/dlls/winegstreamer/video_encoder.c @@ -46,6 +46,7 @@ struct video_encoder IMFMediaType *input_type; MFT_INPUT_STREAM_INFO input_info; IMFMediaType *output_type; + MFT_OUTPUT_STREAM_INFO output_info; IMFAttributes *attributes; }; @@ -94,6 +95,20 @@ done: return hr; } +static HRESULT update_output_info(struct video_encoder *encoder) +{ + UINT64 frame_size; + HRESULT hr; + + if (FAILED(hr = IMFMediaType_GetUINT64(encoder->output_type, &MF_MT_FRAME_SIZE, &frame_size))) + return hr; + + /* FIXME: It's hard to calculate the encoded output size, + * use a uncompressed size here and hope it will work. */ + return MFCalculateImageSize(&MFVideoFormat_NV12, frame_size >> 32, frame_size, + (UINT32 *)&encoder->output_info.cbSize); +} + static HRESULT WINAPI transform_QueryInterface(IMFTransform *iface, REFIID iid, void **out) { struct video_encoder *encoder = impl_from_IMFTransform(iface); @@ -179,8 +194,12 @@ static HRESULT WINAPI transform_GetInputStreamInfo(IMFTransform *iface, DWORD id static HRESULT WINAPI transform_GetOutputStreamInfo(IMFTransform *iface, DWORD id, MFT_OUTPUT_STREAM_INFO *info) { - FIXME("iface %p, id %#lx, info %p.\n", iface, id, info); - return E_NOTIMPL; + struct video_encoder *encoder = impl_from_IMFTransform(iface); + + TRACE("iface %p, id %#lx, info %p.\n", iface, id, info); + + *info = encoder->output_info; + return S_OK; } static HRESULT WINAPI transform_GetAttributes(IMFTransform *iface, IMFAttributes **attributes) @@ -327,6 +346,7 @@ static HRESULT WINAPI transform_SetOutputType(IMFTransform *iface, DWORD id, IMF { IMFMediaType_Release(encoder->output_type); encoder->output_type = NULL; + memset(&encoder->output_info, 0, sizeof(encoder->output_info)); } return S_OK; } @@ -361,7 +381,7 @@ static HRESULT WINAPI transform_SetOutputType(IMFTransform *iface, DWORD id, IMF /* FIXME: Add MF_MT_MPEG_SEQUENCE_HEADER attribute. */ - return S_OK; + return update_output_info(encoder); } static HRESULT WINAPI transform_GetInputCurrentType(IMFTransform *iface, DWORD id, IMFMediaType **type) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5971
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=146742 Your paranoid android. === debian11 (build log) === 0120:err:winediag:h264_encoder_create GStreamer doesn't support H.264 encoding, please install appropriate plugins 0120:err:winediag:h264_encoder_create GStreamer doesn't support H.264 encoding, please install appropriate plugins 0120:err:winediag:h264_encoder_create GStreamer doesn't support H.264 encoding, please install appropriate plugins 0120:err:winediag:h264_encoder_create GStreamer doesn't support H.264 encoding, please install appropriate plugins 0120:err:winediag:h264_encoder_create GStreamer doesn't support H.264 encoding, please install appropriate plugins 0120:err:winediag:h264_encoder_create GStreamer doesn't support H.264 encoding, please install appropriate plugins 0120:err:winediag:h264_encoder_create GStreamer doesn't support H.264 encoding, please install appropriate plugins === debian11b (build log) === 0120:err:winediag:h264_encoder_create GStreamer doesn't support H.264 encoding, please install appropriate plugins
Rémi Bernon (@rbernon) commented about dlls/mf/tests/transform.c:
check_mft_get_input_current_type(transform, expect_input_type_desc); check_mft_get_input_stream_info(transform, S_OK, NULL);
+ /* Input type attributes should match output type attributes. */ + for (i = 0; i < ARRAY_SIZE(to_set_attributes); ++i) + { + winetest_push_context("attr %lu", i); + memcpy(type_desc, input_type_desc, sizeof(input_type_desc)); + set_attribute_desc(type_desc, &to_set_attributes[i], 1); + check_mft_set_input_type(transform, type_desc, MF_E_INVALIDMEDIATYPE); + winetest_pop_context(); + }
Fwiw we don't *have* to use these helpers for every check, I think they are useful to set common check patterns, but in this case it seems specific to this transform and something like that would be simpler and save the need for a new helper: ``` hr = MFCreateMediaType(&media_type); ok(hr == S_OK, "MFCreateMediaType returned %#lx.\n", hr); /* Input type attributes should match output type attributes. */ for (i = 0; i < ARRAY_SIZE(to_set_attributes); ++i) { winetest_push_context("attr %lu", i); init_media_type(media_type, input_type_desc, -1); hr = IMFMediaType_SetItem(media_type, to_set_attributes[i].key, &to_set_attributes[i].item); ok(hr == S_OK, "got %#lx.\n", hr); hr = IMFTransform_SetInputType(transform, 0, media_type, MFT_SET_TYPE_TEST_ONLY); ok(hr == MF_E_INVALIDMEDIATYPE, "got %#lx.\n", hr); winetest_pop_context(); } IMFMediaType_Release(media_type); ``` I would even consider dropping the loop and making individual checks, to avoid having to look for the actual values that are checked, up to you. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5971#note_74880
Rémi Bernon (@rbernon) commented about dlls/winegstreamer/video_encoder.c:
return hr; }
+static HRESULT update_output_info(struct video_encoder *encoder) +{ + UINT64 frame_size; + HRESULT hr; + + if (FAILED(hr = IMFMediaType_GetUINT64(encoder->output_type, &MF_MT_FRAME_SIZE, &frame_size))) + return hr; + + /* FIXME: It's hard to calculate the encoded output size, + * use a uncompressed size here and hope it will work. */ + return MFCalculateImageSize(&MFVideoFormat_NV12, frame_size >> 32, frame_size, + (UINT32 *)&encoder->output_info.cbSize);
This doesn't look right and we could return a hardcoded value just as well for now. I'm not sure where this information comes from but if it really matters maybe we need a way to get it from the actual encoder? The same question goes for `MF_MT_MPEG_SEQUENCE_HEADER`, is it possible to get it from GStreamer? Or does it simply not matter? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5971#note_74879
Rémi Bernon (@rbernon) commented about dlls/mf/tests/transform.c:
+ /* Input type attributes should match output type attributes. */ + for (i = 0; i < ARRAY_SIZE(to_set_attributes); ++i) + { + winetest_push_context("attr %lu", i); + memcpy(type_desc, input_type_desc, sizeof(input_type_desc)); + set_attribute_desc(type_desc, &to_set_attributes[i], 1); + check_mft_set_input_type(transform, type_desc, MF_E_INVALIDMEDIATYPE); + winetest_pop_context(); + } + + /* Output info cbSize will change if we change output type attributes. */ + memcpy(type_desc, output_type_desc, sizeof(output_type_desc)); + set_attribute_desc(type_desc, to_set_attributes, ARRAY_SIZE(to_set_attributes)); + check_mft_set_output_type(transform, type_desc, S_OK); + check_mft_get_output_stream_info(transform, S_OK, &expect_output_info[1]);
Similar comment here, and it may also be good to test only one attribute change at a time, to see which one have an effect on the output size. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5971#note_74881
Rémi Bernon (@rbernon) commented about dlls/winegstreamer/video_encoder.c:
for (i = 0; i < encoder->input_type_count; ++i) if (IsEqualGUID(&subtype, encoder->input_types[i])) break; if (i == encoder->input_type_count) return MF_E_INVALIDMEDIATYPE;
- if (FAILED(IMFMediaType_GetUINT64(type, &MF_MT_FRAME_SIZE, &ratio)) - || FAILED(IMFMediaType_GetUINT64(type, &MF_MT_FRAME_RATE, &ratio))) + if (FAILED(hr = create_input_type(encoder, &subtype, &good_input_type))) + return hr; + if (FAILED(hr = IMFMediaType_Compare(good_input_type, (IMFAttributes *)type, + MF_ATTRIBUTES_MATCH_OUR_ITEMS, &result))) + return hr; + if (!result) return MF_E_INVALIDMEDIATYPE;
You're leaking good_input_type here. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5971#note_74882
This doesn't look right and we could return a hardcoded value just as well for now.
I'm not sure where this information comes from but if it really matters maybe we need a way to get it from the actual encoder?
The cbSize of output info is usually used for determining the output buffer size. So I think using a size that is large enough to hold the encoded sample will make things work. So I just use a uncompressed size here, which must bigger than the encoded h264 size.
The same question goes for `MF_MT_MPEG_SEQUENCE_HEADER`, is it possible to get it from GStreamer? Or does it simply not matter?
I'm not pretty sure for that. I'm planning to put it aside for now, and doing researh after we are starting to implement the actuall encoding code. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5971#note_74966
participants (4)
-
Marvin -
Rémi Bernon -
Ziqing Hui
-
Ziqing Hui (＠zhui)