[PATCH v2 3/5] winegstreamer: Add H264 encoded format support in wg_transform.
Rémi Bernon
rbernon at codeweavers.com
Mon Mar 21 14:55:44 CDT 2022
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.
--
Rémi Bernon <rbernon at codeweavers.com>
More information about the wine-devel
mailing list