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

[Rémi Bernon]

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>