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

[Zebediah Figura]

On 3/21/22 14:55, Rémi Bernon wrote:
>>> +
>>> +    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.

Okay, fair enough.

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

I'm sorry, I can't read. I mixed up which enumeration this is, and 
thought it was the other one...