On 3/21/22 18:55, Zebediah Figura wrote:
On 3/21/22 03:42, Rémi Bernon wrote:
+static void mf_media_type_to_wg_format_h264(IMFMediaType *type, struct wg_format *format) +{ + UINT64 frame_rate, frame_size; + UINT32 profile, level;
+ format->major_type = WG_MAJOR_TYPE_H264; + format->u.h264.width = 0; + format->u.h264.height = 0; + format->u.h264.fps_n = 1; + format->u.h264.fps_d = 1;
Something of a nitpick, but it looks wrong to partially zero-initialize here. If we're relying on the struct to be zeroed I'd rather be consistent about it.
Sure.
+ if (SUCCEEDED(IMFMediaType_GetUINT64(type, &MF_MT_FRAME_SIZE, &frame_size))) + { + format->u.h264.width = (UINT32)(frame_size >> 32); + format->u.h264.height = (UINT32)frame_size; + }
+ if (SUCCEEDED(IMFMediaType_GetUINT64(type, &MF_MT_FRAME_RATE, &frame_rate)) && (UINT32)frame_rate) + { + format->u.h264.fps_n = (UINT32)(frame_rate >> 32); + format->u.h264.fps_d = (UINT32)frame_rate; + }
These casts are redundant.
Yes, but in the case of a later type change for the struct members, this will go very wrong and unnoticed.
I think it's best cast there (at least the lower bits) to make sure we extract the right values from the ratio, regardless to where we store them.
+ if (SUCCEEDED(IMFMediaType_GetUINT32(type, &MF_MT_MPEG2_PROFILE, &profile))) + format->u.h264.profile = profile;
+ if (SUCCEEDED(IMFMediaType_GetUINT32(type, &MF_MT_MPEG2_LEVEL, &level))) + format->u.h264.level = level; +}
...
+static GstCaps *wg_format_to_caps_h264(const struct wg_format *format) +{ + const char *profile, *level; + GstCaps *caps;
+ caps = gst_caps_new_empty_simple("video/x-h264"); + gst_caps_set_simple(caps, "stream-format", G_TYPE_STRING, "byte-stream", NULL); + gst_caps_set_simple(caps, "alignment", G_TYPE_STRING, "au", NULL);
+ if (format->u.h264.width) + gst_caps_set_simple(caps, "width", G_TYPE_INT, format->u.h264.width, NULL); + if (format->u.h264.height) + gst_caps_set_simple(caps, "height", G_TYPE_INT, format->u.h264.height, NULL); + if (format->u.h264.fps_n || format->u.h264.fps_d) + gst_caps_set_simple(caps, "framerate", GST_TYPE_FRACTION, format->u.h264.fps_n, format->u.h264.fps_d, NULL);
+ switch (format->u.h264.profile) + { + case /* eAVEncH264VProfile_Main */ 77: profile = "main"; break; + case /* eAVEncH264VProfile_High */ 100: profile = "high"; break; + case /* eAVEncH264VProfile_444 */ 244: profile = "high-4:4:4"; break; + default: + GST_ERROR("Unrecognized H.264 profile attribute %u.", format->u.h264.profile); + /* fallthrough */ + case 0: profile = NULL; + } + if (profile) + gst_caps_set_simple(caps, "profile", G_TYPE_STRING, profile, NULL);
+ switch (format->u.h264.level) + { + case /* eAVEncH264VLevel1 */ 10: level = "1"; break; + case /* eAVEncH264VLevel1_1 */ 11: level = "1.1"; break; + case /* eAVEncH264VLevel1_2 */ 12: level = "1.2"; break; + case /* eAVEncH264VLevel1_3 */ 13: level = "1.3"; break; + case /* eAVEncH264VLevel2 */ 20: level = "2"; break; + case /* eAVEncH264VLevel2_1 */ 21: level = "2.1"; break; + case /* eAVEncH264VLevel2_2 */ 22: level = "2.2"; break; + case /* eAVEncH264VLevel3 */ 30: level = "3"; break; + case /* eAVEncH264VLevel3_1 */ 31: level = "3.1"; break; + case /* eAVEncH264VLevel3_2 */ 32: level = "3.2"; break; + case /* eAVEncH264VLevel4 */ 40: level = "4"; break; + case /* eAVEncH264VLevel4_1 */ 41: level = "4.1"; break; + case /* eAVEncH264VLevel4_2 */ 42: level = "4.2"; break; + case /* eAVEncH264VLevel5 */ 50: level = "5"; break; + case /* eAVEncH264VLevel5_1 */ 51: level = "5.1"; break; + case /* eAVEncH264VLevel5_2 */ 52: level = "5.2"; break; + default: + GST_ERROR("Unrecognized H.264 level attribute %u.", format->u.h264.level); + /* fallthrough */ + case 0: level = NULL; + } + if (level) + gst_caps_set_simple(caps, "level", G_TYPE_STRING, level, NULL);
+ return caps; +}
Any reason not to use these constants?
@@ -251,6 +252,9 @@ NTSTATUS wg_transform_create(void *args) break; case WG_MAJOR_TYPE_VIDEO: + break;
+ case WG_MAJOR_TYPE_H264: case WG_MAJOR_TYPE_WMA: case WG_MAJOR_TYPE_UNKNOWN: GST_FIXME("Format %u not implemented!", output_format.major_type);
This hunk doesn't seem like it belongs in this patch...
I think it does, the patch adds support for H264 formats, and creates a wg_transform in the H264 decoder to use it, the transform has a decoded video output format, which would cause an error if the hunk was not there.