Re: [PATCH 1/2] winegstreamer: Add helper for GstCaps <-> IMFMediaType conversion.

[Zebediah Figura]

On 3/25/20 11:57 PM, Derek Lesho wrote:
> On 3/24/20 3:22 PM, Zebediah Figura wrote:
> > General comments:
> >
> > It's not great to introduce code that's not used anywhere, it's
> > essentially dead until then.
> >
> > This could, I think, be split up into much smaller pieces in any case:
> > you're introducing two different functions here, and each function
> > introduces support for several different formats.
> >
> > On 3/24/20 2:39 PM, Derek Lesho wrote:
> > > Signed-off-by: Derek Lesho
> > >
> > > dlls/winegstreamer/gst_private.h | 4 +
> > > dlls/winegstreamer/mfplat.c | 533 ++++++++++++++++++++++++++++++-
> > > include/codecapi.h | 38 +++
> > > 3 files changed, 574 insertions(+), 1 deletion(-)
> > > create mode 100644 include/codecapi.h
> > >
> > > diff --git a/dlls/winegstreamer/gst_private.h 
> > > b/dlls/winegstreamer/gst_private.h
> > > index e6fb841fc8..a6c3fd3784 100644
> > > --- a/dlls/winegstreamer/gst_private.h
> > > +++ b/dlls/winegstreamer/gst_private.h
> > > @@ -36,6 +36,7 @@
> > > #include "winuser.h"
> > > #include "dshow.h"
> > > #include "strmif.h"
> > > +#include "mfobjects.h"
> > > #include "wine/heap.h"
> > > #include "wine/strmbase.h"
> > > @@ -54,4 +55,7 @@ void start_dispatch_thread(void) DECLSPEC_HIDDEN;
> > > extern HRESULT mfplat_get_class_object(REFCLSID rclsid, REFIID riid, 
> > > void **obj) DECLSPEC_HIDDEN;
> > > +IMFMediaType* media_type_from_caps(GstCaps *caps);
> > > +GstCaps *caps_from_media_type(IMFMediaType *type);
> > > -
> >
> > Using the generic name "media_type", in a module that serves multiple
> > media APIs, is not great.
>
> Would you prefer mf_media_type?

That's probably better, yes.
> > Also, why is this in the public header?
>
> Would it be better to split this into a mfplat_private.h header?

I mean, why do you need to use it from anything other than mfplat.c?

This is another reason why it doesn't make a lot of sense to submit dead
code.
> > Also, style nitpick: please try to be consistent about your asterisk
> > placement (ideally using "type *var" style.)
>
> Ack.
> > > #endif /* __GST_PRIVATE_INCLUDED__ */
> >
> > ...
> > > @@ -433,3 +438,529 @@ HRESULT mfplat_get_class_object(REFCLSID 
> > > rclsid, REFIID riid, void **obj)
> > > return CLASS_E_CLASSNOTAVAILABLE;
> > > }
> > > -
> > >
> > > +struct aac_user_data
> > > +{
> > > - WORD payload_type;
> > > - WORD profile_level_indication;
> > > - WORD struct_type;
> > > - WORD reserved;
> > > - /*BYTE audio_specific_config;*/
> >
> > What's this field doing here?
>
> We store the audio_config_config after these fields, and I wanted to 
> express that here, it's not important though.

It's not necessarily a problem to specify that arbitrary data comes
after the struct, but that comment is not particularly clear.
> > > +};
> > > -
> > >
> > > +/* IMPORTANT: caps will be modified to represent the exact type 
> > > needed for the format */
> >
> > Why?
>
> Because in the case of a demuxer, the caps of the stream we receive 
> might not map 1:1 with the representation in media foundation. Because 
> of this, in the media source, if any adjustments are needed, we feed the 
> buffers through a parser to correct it.
>
> See: 
> https://github.com/Guy1524/wine/commit/7ab88be3882ab95f3fc17dab374184e06f018...

This seems like a very confusing way to do that. At least I'd relegate
it to a separate function. I wouldn't expect a conversion function to
modify its argument, and it moreover makes it essentially unusable
anywhere else.

That said, these modifications are specific to the format, and along
those lines it may make more sense to append specific elements rather
than to make specific changes to the caps and try to find an element
that can accommodate those. This will also help if you ever need to
append multiple such elements. Thus you can e.g. append an audioconvert
element unconditionally, and if no conversion is necessary it'll just
pass through.

Looking at the modifications you do make—

* you force h264 into annex B format, which is the job of h264parse;

* you force all raw audio into 32-bit float. Does native mfplat really
never output integer PCM?
> > > +IMFMediaType* media_type_from_caps(GstCaps *caps)
> > > +{
> > > - IMFMediaType *media_type;
> > > - GstStructure *info;
> > > - const char *media_type_name;
> > > - gchar *human_readable;
> > > -
> > > - if (FAILED(MFCreateMediaType(&media_type)))
> > > - {
> > > - return NULL;
> > > - }
> > > -
> > > - info = gst_caps_get_structure(caps, 0);
> > > - media_type_name = gst_structure_get_name(info);
> > > -
> > > - human_readable = gst_caps_to_string(caps);
> > > - TRACE("caps = %s\n", human_readable);
> > > - g_free(human_readable);
> >
> > Probably would be best to guard this with TRACE_ON, so that we don't
> > bother allocating anything otherwise.
> >
> > Also, you'll want to use debugstr_a(), especially since caps can overrun
> > the static buffer in ntdll.
>
> Ack.
> > > -
> > > - if (!(strncmp(media_type_name, "video", 5)))
> >
> > Style nitpick, superfluous parentheses.
> >
> > I think Nikolay already mentioned this, but it's probably not a bad idea
> > to just match against the whole "video/x-h264" etc. sequence.
>
> Ack.
> > > - {
> > > - const char *video_format = media_type_name + 6;
> > > - gint width, height, framerate_num, framerate_den;
> > > -
> > > - IMFMediaType_SetGUID(media_type, &MF_MT_MAJOR_TYPE,
> > >
> > > &MFMediaType_Video);
> > > -
> > > - if (gst_structure_get_int(info, "width", &width) &&
> > >
> > > gst_structure_get_int(info, "height", &height))
> > > - {
> > > - IMFMediaType_SetUINT64(media_type, &MF_MT_FRAME_SIZE,
> > >
> > > ((UINT64)width << 32) | height);
> > > - }
> > > - if (gst_structure_get_fraction(info, "framerate", &framerate_num,
> > >
> > > &framerate_den))
> > > - {
> > > - IMFMediaType_SetUINT64(media_type, &MF_MT_FRAME_RATE,
> > >
> > > ((UINT64)framerate_num << 32) | framerate_den);
> > > - }
> > > -
> > > - if (!(strcmp(video_format, "x-h264")))
> > > - {
> > > - const char *profile, *level;
> > > -
> > > - IMFMediaType_SetGUID(media_type, &MF_MT_SUBTYPE, &MFVideoFormat_H264);
> > > - IMFMediaType_SetUINT32(media_type, &MF_MT_COMPRESSED, TRUE);
> > > -
> > > - if ((profile = gst_structure_get_string(info, "profile")))
> > > - {
> > > - if (!(strcmp(profile, "main")))
> > > - IMFMediaType_SetUINT32(media_type, &MF_MT_MPEG2_PROFILE,
> > >
> > > eAVEncH264VProfile_Main);
> > > - else if (!(strcmp(profile, "high")))
> > > - IMFMediaType_SetUINT32(media_type, &MF_MT_MPEG2_PROFILE,
> > >
> > > eAVEncH264VProfile_High);
> > > - else if (!(strcmp(profile, "high-4:4:4")))
> > > - IMFMediaType_SetUINT32(media_type, &MF_MT_MPEG2_PROFILE,
> > >
> > > eAVEncH264VProfile_444);
> > > - else
> > > - ERR("Unrecognized profile %s\n", profile);
> >
> > This ERR (and many below) should probably be a FIXME instead, methinks.
>
> Ack.
> > > - }
> > > - if ((level = gst_structure_get_string(info, "level")))
> > > - {
> > > - if (!(strcmp(level, "1")))
> > > - IMFMediaType_SetUINT32(media_type, &MF_MT_MPEG2_LEVEL,
> > >
> > > eAVEncH264VLevel1);
> > > - else if (!(strcmp(level, "1.1")))
> > > - IMFMediaType_SetUINT32(media_type, &MF_MT_MPEG2_LEVEL,
> > >
> > > eAVEncH264VLevel1_1);
> > > - else if (!(strcmp(level, "1.2")))
> > > - IMFMediaType_SetUINT32(media_type, &MF_MT_MPEG2_LEVEL,
> > >
> > > eAVEncH264VLevel1_2);
> > > - else if (!(strcmp(level, "1.3")))
> > > - IMFMediaType_SetUINT32(media_type, &MF_MT_MPEG2_LEVEL,
> > >
> > > eAVEncH264VLevel1_3);
> > > - else if (!(strcmp(level, "2")))
> > > - IMFMediaType_SetUINT32(media_type, &MF_MT_MPEG2_LEVEL,
> > >
> > > eAVEncH264VLevel2);
> > > - else if (!(strcmp(level, "2.1")))
> > > - IMFMediaType_SetUINT32(media_type, &MF_MT_MPEG2_LEVEL,
> > >
> > > eAVEncH264VLevel2_1);
> > > - else if (!(strcmp(level, "2.2")))
> > > - IMFMediaType_SetUINT32(media_type, &MF_MT_MPEG2_LEVEL,
> > >
> > > eAVEncH264VLevel2_2);
> > > - else if (!(strcmp(level, "3")))
> > > - IMFMediaType_SetUINT32(media_type, &MF_MT_MPEG2_LEVEL,
> > >
> > > eAVEncH264VLevel3);
> > > - else if (!(strcmp(level, "3.1")))
> > > - IMFMediaType_SetUINT32(media_type, &MF_MT_MPEG2_LEVEL,
> > >
> > > eAVEncH264VLevel3_1);
> > > - else if (!(strcmp(level, "3.2")))
> > > - IMFMediaType_SetUINT32(media_type, &MF_MT_MPEG2_LEVEL,
> > >
> > > eAVEncH264VLevel3_2);
> > > - else if (!(strcmp(level, "4")))
> > > - IMFMediaType_SetUINT32(media_type, &MF_MT_MPEG2_LEVEL,
> > >
> > > eAVEncH264VLevel4);
> > > - else if (!(strcmp(level, "4.1")))
> > > - IMFMediaType_SetUINT32(media_type, &MF_MT_MPEG2_LEVEL,
> > >
> > > eAVEncH264VLevel4_1);
> > > - else if (!(strcmp(level, "4.2")))
> > > - IMFMediaType_SetUINT32(media_type, &MF_MT_MPEG2_LEVEL,
> > >
> > > eAVEncH264VLevel4_2);
> > > - else if (!(strcmp(level, "5")))
> > > - IMFMediaType_SetUINT32(media_type, &MF_MT_MPEG2_LEVEL,
> > >
> > > eAVEncH264VLevel5);
> > > - else if (!(strcmp(level, "5.1")))
> > > - IMFMediaType_SetUINT32(media_type, &MF_MT_MPEG2_LEVEL,
> > >
> > > eAVEncH264VLevel5_1);
> > > - else if (!(strcmp(level, "5.2")))
> > > - IMFMediaType_SetUINT32(media_type, &MF_MT_MPEG2_LEVEL,
> > >
> > > eAVEncH264VLevel5_2);
> > > - else
> > > - ERR("Unrecognized level %s\n", level);
> > > - }
> >
> > Could we maybe make this a table instead?
>
> Sure.
> > > - gst_caps_set_simple(caps, "stream-format", G_TYPE_STRING,
> > >
> > > "byte-stream", NULL);
> > > - gst_caps_set_simple(caps, "alignment", G_TYPE_STRING, "au", NULL);
> > > - for (unsigned int i = 0; i < gst_caps_get_size(caps); i++)
> > > - {
> > > - GstStructure *structure = gst_caps_get_structure (caps, i);
> > > - gst_structure_remove_field(structure, "codec_data");
> > > - }
> > > - }
> > > - else if (!(strcmp(video_format, "x-wmv")))
> > > - {
> > > - gint wmv_version;
> > > - const char *format;
> > > - const GValue *codec_data;
> > > -
> > > - if (gst_structure_get_int(info, "wmvversion", &wmv_version))
> > > - {
> > > - switch (wmv_version)
> > > - {
> > > - case 1:
> > > - IMFMediaType_SetGUID(media_type, &MF_MT_SUBTYPE, &MFVideoFormat_WMV1);
> > > - break;
> > > - case 2:
> > > - IMFMediaType_SetGUID(media_type, &MF_MT_SUBTYPE, &MFVideoFormat_WMV2);
> > > - break;
> > > - case 3:
> > > - IMFMediaType_SetGUID(media_type, &MF_MT_SUBTYPE, &MFVideoFormat_WMV3);
> > > - break;
> > > - default:
> > > - ERR("Unrecognized wmvversion %d\n", wmv_version);
> > > - }
> > > - }
> > > -
> > > - if ((format = gst_structure_get_string(info, "format")))
> > > - {
> > > - if (!(strcmp(format, "WVC1")))
> > > - IMFMediaType_SetGUID(media_type, &MF_MT_SUBTYPE, &MFVideoFormat_WVC1);
> >
> > What if it's not? I think that deserves at least a FIXME.
> >
> > (Style nitpick, extra parentheses.)
>
> Ack.
> > > - }
> > > -
> > > - if ((codec_data = gst_structure_get_value(info, "codec_data")))
> > > - {
> > > - GstBuffer *codec_data_buffer = gst_value_get_buffer(codec_data);
> > > - if (codec_data_buffer)
> > > - {
> > > - gsize codec_data_size = gst_buffer_get_size(codec_data_buffer);
> > > - gpointer codec_data_raw = heap_alloc(codec_data_size);
> > > - gst_buffer_extract(codec_data_buffer, 0, codec_data_raw,
> > >
> > > codec_data_size);
> > > - IMFMediaType_SetBlob(media_type, &MF_MT_USER_DATA, codec_data_raw,
> > >
> > > codec_data_size);
> > > - }
> > > - }
> > > - }
> > > - else if (!(strcmp(video_format, "mpeg")))
> > > - {
> > > - IMFMediaType_SetGUID(media_type, &MF_MT_SUBTYPE, &MFVideoFormat_M4S2);
> > > - IMFMediaType_SetUINT32(media_type, &MF_MT_COMPRESSED, TRUE);
> >
> > There are other video/mpeg formats.
>
> TBH, the only reason I've included this is for the tests to work, I'll 
> look into how to differentiate the mpeg types tomorrow.
> > > - }
> > > - else if (!(strcmp(video_format, "x-raw")))
> > > - {
> > > - const char *fourcc = gst_structure_get_string(info, "stream-format");
> > > - IMFMediaType_SetUINT32(media_type, &MF_MT_COMPRESSED, FALSE);
> > > - if (fourcc && (strlen(fourcc) == 4))
> > > - {
> > > - GUID fourcc_subtype = MFVideoFormat_Base;
> > > - fourcc_subtype.Data1 = MAKEFOURCC(
> > > - toupper(fourcc[0]), toupper(fourcc[1]), toupper(fourcc[2]),
> > >
> > > toupper(fourcc[3]));
> > > - IMFMediaType_SetGUID(media_type, &MF_MT_SUBTYPE, &fourcc_subtype);
> > > - }
> > > - else
> > > - ERR("uncompressed video has no stream-format\n");
> >
> > I've never seen a FOURCC stored in the "stream-format" tag; where are
> > you getting this from?
>
> You're right, I think I'm supposed to use "format" here, but this is 
> dead code rn so I that's why I didn't see any problems.
> > > - }
> > > - else
> > > - ERR("Unrecognized video format %s\n", video_format);
> > > - }
> > > - else if (!(strncmp(media_type_name, "audio", 5)))
> > > - {
> > > - const char *audio_format = media_type_name + 6;
> > > -
> > > - IMFMediaType_SetGUID(media_type, &MF_MT_MAJOR_TYPE,
> > >
> > > &MFMediaType_Audio);
> > > - if (!(strcmp(audio_format, "mpeg")))
> > > - {
> > > - int mpeg_version = -1;
> > > -
> > > - IMFMediaType_SetUINT32(media_type, &MF_MT_COMPRESSED, TRUE);
> > > -
> > > - if (!(gst_structure_get_int(info, "mpegversion", &mpeg_version)))
> > > - ERR("Failed to get mpegversion\n");
> > > - switch (mpeg_version)
> > > - {
> > > - case 1:
> > > - {
> > > - IMFMediaType_SetGUID(media_type, &MF_MT_SUBTYPE, &MFAudioFormat_MPEG);
> > > - break;
> > > - }
> >
> > What about MFAudioFormat_MP3?
>
> I'm actually not sure what to use here, I should probably remove it for now.
> > > - case 2:
> > > - case 4:
> > > - {
> > > - const char *format, *profile, *level;
> > > - DWORD profile_level_indication = 0;
> > > - const GValue *codec_data;
> > > - DWORD asc_size = 0;
> > > - struct aac_user_data *user_data = NULL;
> > > -
> > > - IMFMediaType_SetGUID(media_type, &MF_MT_SUBTYPE, &MFAudioFormat_AAC);
> > > -
> > > - codec_data = gst_structure_get_value(info, "codec_data");
> > > - if (codec_data)
> > > - {
> > > - GstBuffer *codec_data_buffer = gst_value_get_buffer(codec_data);
> > > - if (codec_data_buffer)
> > > - {
> > > - if ((asc_size = gst_buffer_get_size(codec_data_buffer)) >= 2)
> > > - {
> > > - user_data = heap_alloc_zero(sizeof(*user_data)+asc_size);
> > > - gst_buffer_extract(codec_data_buffer, 0, (gpointer)(user_data + 1),
> > >
> > > asc_size);
> > > - }
> > > - else
> > > - ERR("Unexpected buffer size\n");
> > > - }
> > > - else
> > > - ERR("codec_data not a buffer\n");
> > > - }
> > > - else
> > > - ERR("codec_data not found\n");
> > > - if (!user_data)
> > > - user_data = heap_alloc_zero(sizeof(*user_data));
> > > -
> > > - {
> > > - int rate;
> > > - if (gst_structure_get_int(info, "rate", &rate))
> > > - IMFMediaType_SetUINT32(media_type, &MF_MT_AUDIO_SAMPLES_PER_SECOND,
> > >
> > > rate);
> > > - }
> > > - {
> > > - int channels;
> > > - if (gst_structure_get_int(info, "channels", &channels))
> > > - IMFMediaType_SetUINT32(media_type, &MF_MT_AUDIO_NUM_CHANNELS,
> > >
> > > channels);
> > > - }
> >
> > Did you mean to add these blocks?
>
> Yeah, it's so I can declare the variables closer to where they are used.

I'll admit I don't get the obsession with C99 variable declarations, but
this just seems janky.
> > > -
> > > - if ((format = gst_structure_get_string(info, "stream-format")))
> > > - {
> > > - DWORD payload_type = -1;
> > > - if (!(strcmp(format, "raw")))
> > > - payload_type = 0;
> > > - else if (!(strcmp(format, "adts")))
> > > - payload_type = 1;
> > > - else
> > > - ERR("Unrecognized stream-format\n");
> > > - if (payload_type != -1)
> > > - {
> > > - IMFMediaType_SetUINT32(media_type, &MF_MT_AAC_PAYLOAD_TYPE,
> > >
> > > payload_type);
> > > - user_data->payload_type = payload_type;
> > > - }
> > > - }
> > > - else
> > > - {
> > > - ERR("Stream format not present\n");
> > > - }
> > > -
> > > - profile = gst_structure_get_string(info, "profile");
> > > - level = gst_structure_get_string(info, "level");
> > > - /* Data from
> > >
> > > https://docs.microsoft.com/en-us/windows/win32/medfound/aac-encoder#output-t... 
> > > */
> >
> > I'm not sure I'd link to Microsoft documentation; it's not very stable.
>
> Would a link to an archive.is backup of it be better?

Probably.
> > > - if (profile && level)
> > > - {
> > > - if (!(strcmp(profile, "lc")) && !(strcmp(level, "2")))
> > > - profile_level_indication = 0x29;
> > > - else if (!(strcmp(profile, "lc")) && !(strcmp(level, "4")))
> > > - profile_level_indication = 0x2A;
> > > - else if (!(strcmp(profile, "lc")) && !(strcmp(level, "5")))
> > > - profile_level_indication = 0x2B;
> > > - else
> > > - ERR("Unhandled profile/level combo\n");
> > > - }
> > > - else
> > > - ERR("Profile or level not present\n");
> > > -
> > > - if (profile_level_indication)
> > > - {
> > > - IMFMediaType_SetUINT32(media_type,
> > >
> > > &MF_MT_AAC_AUDIO_PROFILE_LEVEL_INDICATION, profile_level_indication);
> > > - user_data->profile_level_indication = profile_level_indication;
> > > - }
> > > -
> > > - IMFMediaType_SetBlob(media_type, &MF_MT_USER_DATA, (BYTE
> > >
> > > *)user_data, sizeof(user_data) + asc_size);
> > > - heap_free(user_data);
> > > - break;
> > > - }
> > > - default:
> > > - ERR("Unhandled mpegversion %d\n", mpeg_version);
> > > - }
> > > - }
> > > - else if (!(strcmp(audio_format, "x-raw")))
> > > - {
> > > - IMFMediaType_SetGUID(media_type, &MF_MT_SUBTYPE, &MFAudioFormat_Float);
> > > -
> > > - gst_caps_set_simple(caps, "format", G_TYPE_STRING, "F32LE", NULL);
> >
> > There are other audio formats.
>
> Ah, you mean PCM?  I'll add a case for that tomorrow.

f32le is PCM, but I mean integer PCM and other depths than 32-bit.

Presumably there should also be channel and sample rate data here.
> > > - }
> > > - else
> > > - ERR("Unrecognized audio format %s\n", audio_format);
> > > - }
> > > - else
> > > - {
> > > - goto fail;
> >
> > I'm generally of the opinion that one line of cleanup doesn't merit a
> > "goto".
>
> Okay I'll change that then.
> > > - }
> > > -
> > > - return media_type;
> > > - fail:
> > > - IMFMediaType_Release(media_type);
> > > - return NULL;
> > >
> > > +}
> > > -
> > >
> > > +static const char *fourcc_str(DWORD fourcc)
> > > +{
> > > - if (!fourcc) return NULL;
> > > - return wine_dbg_sprintf ("%c%c%c%c",
> > > - (char)(fourcc), (char)(fourcc >> 8),
> > > - (char)(fourcc >> 16), (char)(fourcc >> 24));
> > >
> > > +}
> >
> > I don't think you want to use Wine's debugging utilities for non-debug 
> > code.
>
> Ack.
> > > -
> > >
> > > +GstCaps *caps_from_media_type(IMFMediaType *type)
> > > +{
> > > - GUID major_type;
> > > - GUID subtype;
> > > - GUID base_masked_subtype;
> > > - GstCaps *output = NULL;
> > > -
> > > - if (FAILED(IMFMediaType_GetMajorType(type, &major_type)))
> > > - return NULL;
> > > - if (FAILED(IMFMediaType_GetGUID(type, &MF_MT_SUBTYPE, &subtype)))
> > > - return NULL;
> > > - base_masked_subtype = subtype;
> > > - base_masked_subtype.Data1 = 0;
> > > -
> > > - if (IsEqualGUID(&major_type, &MFMediaType_Video))
> > > - {
> > > - UINT64 frame_rate = 0, frame_size = 0;
> > > - DWORD *framerate_num = ((DWORD*)&frame_rate) + 1;
> > > - DWORD *framerate_den = ((DWORD*)&frame_rate);
> > > - DWORD *width = ((DWORD*)&frame_size) + 1;
> > > - DWORD *height = ((DWORD*)&frame_size);
> >
> > It seems simpler to me to do e.g.
> >
> > DWORD width = frame_size;
> > DWORD height = frame_size >> 32;
>
> I'm not getting the width and height here, I'm declaring pointers to 
> them which are set later on.

Right, I mean actually set the variables after retrieving frame_size; in
full something like

DWORD width, height;
/* ... */
IMFMediaType_GetUINT64(type, &MF_MT_FRAME_SIZE, &frame_size);
width = frame_size;
height = frame_size >> 32;
> > > -
> > > - if (IsEqualGUID(&subtype, &MFVideoFormat_H264))
> > > - {
> > > - enum eAVEncH264VProfile h264_profile;
> > > - enum eAVEncH264VLevel h264_level;
> > > - output = gst_caps_new_empty_simple("video/x-h264");
> > > - gst_caps_set_simple(output, "stream-format", G_TYPE_STRING,
> > >
> > > "byte-stream", NULL);
> > > - gst_caps_set_simple(output, "alignment", G_TYPE_STRING, "au", NULL);
> > > -
> > > - if (SUCCEEDED(IMFMediaType_GetUINT32(type, &MF_MT_MPEG2_PROFILE,
> > >
> > > &h264_profile)))
> > > - {
> > > - const char *profile = NULL;
> > > - switch (h264_profile)
> > > - {
> > > - case eAVEncH264VProfile_Main: profile = "main"; break;
> > > - case eAVEncH264VProfile_High: profile = "high"; break;
> > > - case eAVEncH264VProfile_444: profile = "high-4:4:4"; break;
> > > - default: ERR("Unknown profile %u\n", h264_profile);
> > > - }
> > > - if (profile)
> > > - gst_caps_set_simple(output, "profile", G_TYPE_STRING, profile, NULL);
> > > - }
> > > - if (SUCCEEDED(IMFMediaType_GetUINT32(type, &MF_MT_MPEG2_LEVEL,
> > >
> > > &h264_level)))
> > > - {
> > > - const char *level = NULL;
> > > - switch (h264_level)
> > > - {
> > > - case eAVEncH264VLevel1: level = "1"; break;
> > > - case eAVEncH264VLevel1_1: level = "1.1"; break;
> > > - case eAVEncH264VLevel1_2: level = "1.2"; break;
> > > - case eAVEncH264VLevel1_3: level = "1.3"; break;
> > > - case eAVEncH264VLevel2: level = "2"; break;
> > > - case eAVEncH264VLevel2_1: level = "2.1"; break;
> > > - case eAVEncH264VLevel2_2: level = "2.2"; break;
> > > - case eAVEncH264VLevel3: level = "3"; break;
> > > - case eAVEncH264VLevel3_1: level = "3.1"; break;
> > > - case eAVEncH264VLevel3_2: level = "3.2"; break;
> > > - case eAVEncH264VLevel4: level = "4"; break;
> > > - case eAVEncH264VLevel4_1: level = "4.1"; break;
> > > - case eAVEncH264VLevel4_2: level = "4.2"; break;
> > > - case eAVEncH264VLevel5: level = "5"; break;
> > > - case eAVEncH264VLevel5_1: level = "5.1"; break;
> > > - case eAVEncH264VLevel5_2: level = "5.2"; break;
> > > - default: ERR("Unknown level %u\n", h264_level);
> > > - }
> > > - if (level)
> > > - gst_caps_set_simple(output, "level", G_TYPE_STRING, level, NULL);
> > > - }
> > > - }
> > > - else if (IsEqualGUID(&subtype, &MFVideoFormat_WVC1))
> > > - {
> > > - BYTE *user_data;
> > > - DWORD user_data_size;
> > > - output = gst_caps_new_empty_simple("video/x-wmv");
> > > - gst_caps_set_simple(output, "format", G_TYPE_STRING, "WVC1", NULL);
> > > -
> > > - gst_caps_set_simple(output, "wmvversion", G_TYPE_INT, 3, NULL);
> > > -
> > > - if (SUCCEEDED(IMFMediaType_GetAllocatedBlob(type, &MF_MT_USER_DATA,
> > >
> > > &user_data, &user_data_size)))
> > > - {
> > > - GstBuffer *codec_data_buffer = gst_buffer_new_allocate(NULL,
> > >
> > > user_data_size, NULL);
> > > - gst_buffer_fill(codec_data_buffer, 0, user_data, user_data_size);
> > > - gst_caps_set_simple(output, "codec_data", GST_TYPE_BUFFER,
> > >
> > > codec_data_buffer, NULL);
> > > - gst_buffer_unref(codec_data_buffer);
> > > - CoTaskMemFree(user_data);
> > > - }
> > > - }
> > > - else if (IsEqualGUID(&base_masked_subtype, &MFVideoFormat_Base))
> > > - {
> > > - output = gst_caps_new_empty_simple("video/x-raw");
> > > - gst_caps_set_simple(output, "format", G_TYPE_STRING,
> > >
> > > fourcc_str(subtype.Data1), NULL);
> >
> > What about RGB formats?
>
> Ah, I didn't think about those, looks like we'll have to use a table of 
> known conversions instead.

Well, to some degree, though you can also make use of
gst_video_format_from_fourcc(). See also amt_to_gst_caps_video() in
gstdemux.c.
> > > - }
> > > - else {
> > > - ERR("Unrecognized subtype %s\n", debugstr_guid(&subtype));
> > > - return NULL;
> > > - }
> > > -
> > > - IMFMediaType_GetUINT64(type, &MF_MT_FRAME_RATE, &frame_rate);
> > > - IMFMediaType_GetUINT64(type, &MF_MT_FRAME_SIZE, &frame_size);
> > > -
> > > - if (frame_rate)
> > > - gst_caps_set_simple(output, "framerate", GST_TYPE_FRACTION,
> > >
> > > *framerate_num, *framerate_den, NULL);
> > > - if (frame_size)
> > > - {
> > > - gst_caps_set_simple(output, "width", G_TYPE_INT, *width, NULL);
> > > - gst_caps_set_simple(output, "height", G_TYPE_INT, *height, NULL);
> > > - }
> > > - return output;
> > > - }
> > > - else if (IsEqualGUID(&major_type, &MFMediaType_Audio))
> > > - {
> > > - DWORD rate, channels;
> > > -
> > > - if (IsEqualGUID(&subtype, &MFAudioFormat_AAC))
> > > - {
> > > - DWORD payload_type, indication;
> > > - struct aac_user_data *user_data;
> > > - UINT32 user_data_size;
> > > - output = gst_caps_new_empty_simple("audio/mpeg");
> > > -
> > > - /* TODO */
> > > - gst_caps_set_simple(output, "framed", G_TYPE_BOOLEAN, TRUE, NULL);
> > > - gst_caps_set_simple(output, "mpegversion", G_TYPE_INT, 4, NULL);
> >
> > What's TODO here?
>
> MFAudioFormat_AAC could also mean mpegversion=2, and I don't know what 
> the "framed" attribute is for.

A TODO message should probably mention what exactly is to be done.

In general it's good practice to understand what your code is doing
before you submit it, but regardless, "framed" means there is exactly
one frame per buffer. Is that guaranteed by the MF source? (It's not
obvious to me that it is...)
> > > -
> > > - if (SUCCEEDED(IMFMediaType_GetUINT32(type, &MF_MT_AAC_PAYLOAD_TYPE,
> > >
> > > &payload_type)))
> > > - {
> > > - switch (payload_type)
> > > - {
> > > - case 0:
> > > - gst_caps_set_simple(output, "stream-format", G_TYPE_STRING, "raw",
> > >
> > > NULL);
> > > - break;
> > > - case 1:
> > > - gst_caps_set_simple(output, "stream-format", G_TYPE_STRING, "adts",
> > >
> > > NULL);
> > > - break;
> > > - default:
> > > - gst_caps_set_simple(output, "stream-format", G_TYPE_STRING, "raw",
> > >
> > > NULL);
> >
> > Seems to me that 2 and 3 should be mapped to "adif" and "loas",
> > respectively.
>
> Ack.
> > > - }
> > > - }
> > > - else
> > > - gst_caps_set_simple(output, "stream-format", G_TYPE_STRING, "raw",
> > >
> > > NULL);
> > > -
> > > - if (SUCCEEDED(IMFMediaType_GetUINT32(type,
> > >
> > > &MF_MT_AAC_AUDIO_PROFILE_LEVEL_INDICATION, &indication)))
> > > - {
> > > - switch (indication)
> > > - {
> > > - case 0x29:
> > > - {
> > > - gst_caps_set_simple(output, "profile", G_TYPE_STRING, "lc", NULL);
> > > - gst_caps_set_simple(output, "level", G_TYPE_STRING, "2", NULL);
> > > - break;
> > > - }
> > > - case 0x2A:
> > > - {
> > > - gst_caps_set_simple(output, "profile", G_TYPE_STRING, "lc", NULL);
> > > - gst_caps_set_simple(output, "level", G_TYPE_STRING, "4", NULL);
> > > - break;
> > > - }
> > > - case 0x2B:
> > > - {
> > > - gst_caps_set_simple(output, "profile", G_TYPE_STRING, "lc", NULL);
> > > - gst_caps_set_simple(output, "level", G_TYPE_STRING, "5", NULL);
> > > - break;
> > > - }
> > > - default:
> > > - ERR("Unrecognized profile-level-indication %u\n", indication);
> > > - }
> >
> > I think you could significantly deduplicate this switch.
>
> Ack.
> > > - }
> > > -
> > > - if (SUCCEEDED(IMFMediaType_GetAllocatedBlob(type, &MF_MT_USER_DATA,
> > >
> > > (BYTE **) &user_data, &user_data_size)))
> > > - {
> > > - if (user_data_size > sizeof(sizeof(*user_data)))
> > > - {
> > > - GstBuffer *audio_specific_config = gst_buffer_new_allocate(NULL,
> > >
> > > user_data_size - sizeof(*user_data), NULL);
> > > - gst_buffer_fill(audio_specific_config, 0, user_data + 1,
> > >
> > > user_data_size - sizeof(*user_data));
> > > -
> > > - gst_caps_set_simple(output, "codec_data", GST_TYPE_BUFFER,
> > >
> > > audio_specific_config, NULL);
> > > - gst_buffer_unref(audio_specific_config);
> > > - }
> > > - CoTaskMemFree(user_data);
> > > - }
> > > - }
> > > - else if (IsEqualGUID(&subtype, &MFAudioFormat_Float))
> > > - {
> > > - output = gst_caps_new_empty_simple("audio/x-raw");
> > > -
> > > - gst_caps_set_simple(output, "format", G_TYPE_STRING, "F32LE", NULL);
> > > - }
> > > - else
> > > - {
> > > - ERR("Unrecognized subtype %s\n", debugstr_guid(&subtype));
> > > - if (output)
> > > - gst_caps_unref(output);
> > > - return NULL;
> > > - }
> > > - if (SUCCEEDED(IMFMediaType_GetUINT32(type,
> > >
> > > &MF_MT_AUDIO_SAMPLES_PER_SECOND, &rate)))
> > > - {
> > > - gst_caps_set_simple(output, "rate", G_TYPE_INT, rate, NULL);
> > > - }
> > > - if (SUCCEEDED(IMFMediaType_GetUINT32(type,
> > >
> > > &MF_MT_AUDIO_NUM_CHANNELS, &channels)))
> > > - {
> > > - gst_caps_set_simple(output, "channels", G_TYPE_INT, channels, NULL);
> > > - }
> > > -
> > > - return output;
> > > - }
> > > -
> > > - ERR("Unrecognized major type %s\n", debugstr_guid(&major_type));
> > > - return NULL;
> > >
> > > +}
> > > diff --git a/include/codecapi.h b/include/codecapi.h
> > > new file mode 100644
> > > index 0000000000..2690b523d7
> > > --- /dev/null
> > > +++ b/include/codecapi.h
> > > @@ -0,0 +1,38 @@
> > > +#ifndef __CODECAPI_H
> > > +#define __CODECAPI_H
> > > -
> > >
> > > +enum eAVEncH264VProfile
> > > +{
> > > - eAVEncH264VProfile_unknown = 0,
> > > - eAVEncH264VProfile_Simple = 66,
> > > - eAVEncH264VProfile_Base = 66,
> > > - eAVEncH264VProfile_Main = 77,
> > > - eAVEncH264VProfile_High = 100,
> > > - eAVEncH264VProfile_422 = 122,
> > > - eAVEncH264VProfile_High10 = 110,
> > > - eAVEncH264VProfile_444 = 244,
> > > - eAVEncH264VProfile_Extended = 88,
> > >
> > > +};
> > > -
> > >
> > > +enum eAVEncH264VLevel
> > > +{
> > > - eAVEncH264VLevel1 = 10,
> > > - eAVEncH264VLevel1_b = 11,
> > > - eAVEncH264VLevel1_1 = 11,
> > > - eAVEncH264VLevel1_2 = 12,
> > > - eAVEncH264VLevel1_3 = 13,
> > > - eAVEncH264VLevel2 = 20,
> > > - eAVEncH264VLevel2_1 = 21,
> > > - eAVEncH264VLevel2_2 = 22,
> > > - eAVEncH264VLevel3 = 30,
> > > - eAVEncH264VLevel3_1 = 31,
> > > - eAVEncH264VLevel3_2 = 32,
> > > - eAVEncH264VLevel4 = 40,
> > > - eAVEncH264VLevel4_1 = 41,
> > > - eAVEncH264VLevel4_2 = 42,
> > > - eAVEncH264VLevel5 = 50,
> > > - eAVEncH264VLevel5_1 = 51,
> > > - eAVEncH264VLevel5_2 = 52
> > >
> > > +};
> > > -
> > >
> > > +#endif
> > > \ No newline at end of file