Re: [PATCH v2 3/5] winegstreamer: Add H264 encoded format support in wg_transform.

[Zebediah Figura]

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