[PATCH v3 3/3] mf: Partially implement the topology loader.

[Nikolay Sivov]

On 4/2/20 12:11 AM, Sergio Gómez Del Real wrote:
> +static void topology_loader_add_branch(struct topology *topology, IMFTopologyNode *first, IMFTopologyNode *last)
> +{
> +    IMFTopology *full_topo = &topology->IMFTopology_iface;
> +    IMFTopologyNode *in, *out;
> +    DWORD index;
> +
> +    in = first;
> +    IMFTopology_AddNode(full_topo, in);
> +    while (SUCCEEDED(IMFTopologyNode_GetOutput(in, 0, &out, &index)))
> +    {
> +        IMFTopology_AddNode(full_topo, out);
> +        in = out;
> +    }
> +}
Second node argument is not used, is that intentional? It also leaks on 
every iteration. I'm surprised it works, that you can connect nodes 
before adding them,
and then have a situation when one is added and has a connection to 
another that isn't yet.

It also assumes that all nodes are 1-1 for i/o count, which is 
reasonable, but we'll need a fixme for unsupported cases. Obviously not 
in this helper.

> +static HRESULT topology_loader_resolve_branch(IMFTopologyNode *src, IMFMediaType *mediatype, IMFTopologyNode *sink, MF_CONNECT_METHOD method)
> +{
> +    IMFStreamSink *streamsink;
> +    IMFMediaTypeHandler *mth;
> +    HRESULT hr;
> +
> +    IMFTopologyNode_GetObject(sink, (IUnknown **)&streamsink);
> +    IMFStreamSink_GetMediaTypeHandler(streamsink, &mth);
> +    if (method == MF_CONNECT_DIRECT)
> +    {
> +        if (FAILED(hr = IMFMediaTypeHandler_SetCurrentMediaType(mth, mediatype)))
> +            return hr;
> +        hr = IMFTopologyNode_ConnectOutput(src, 0, sink, 0);
> +        return hr;
> +    }
> +    else
Is it checked anywhere that types are actually compatible? Also, this 
function only checks for DIRECT mode, what's a point of iterating over 
all methods when it's called?

> +        /* for getting converters later on */
> +        if (IsEqualGUID(&major_type, &MFMediaType_Audio))
> +            mft_category = MFT_CATEGORY_AUDIO_EFFECT;
> +        else if (IsEqualGUID(&major_type, &MFMediaType_Video))
> +            mft_category = MFT_CATEGORY_VIDEO_EFFECT;
You have similar condition right above that one. I think you can ignore 
that for now and assume we only need decoders.

> +        for (i = 0; i < num_activates_decs; i++)
> +        {
> +            IMFTransform *decoder;
> +
> +            IMFActivate_ActivateObject(activates_decs[i], &IID_IMFTransform, (void **)&decoder);
This definitely needs some error handling.

> +                    /* succeeded with source -> decoder -> sink */
> +                    if (SUCCEEDED(IMFMediaTypeHandler_SetCurrentMediaType(mth, decoder_mtype)))
> +                    {
> +                        MFCreateTopologyNode(MF_TOPOLOGY_TRANSFORM_NODE, &node_dec);
> +                        IMFTopologyNode_SetObject(node_dec, (IUnknown *)decoder);
> +                        IMFTopologyNode_ConnectOutput(src, 0, node_dec, 0);
> +                        IMFTopologyNode_ConnectOutput(node_dec, 0, sink, 0);
> +
> +                        IMFActivate_ShutdownObject(activates_convs[i]);
> +                        return S_OK;
> +                    }
You need to set decoder output type as well, and shutting it down looks, 
even if it works. Is it possible loader stashes activator in some attribute?
Or maybe it sets IMFActivate as object itself, and pre-activates it. Or 
maybe loader does not use it at all and keeps using CLSIDs.
Conceptually after loader did its job it should still be possible to 
shut down.

topology_loader_resolve_branch() has a lot of leaks, for example you 
should free returned IMFActivate arrays.

> +    MFCreateTopologyNode(MF_TOPOLOGY_SOURCESTREAM_NODE, &clone_src);
> +    MFCreateTopologyNode(MF_TOPOLOGY_OUTPUT_NODE, &clone_sink);
> +    IMFTopologyNode_CloneFrom(clone_src, &src->IMFTopologyNode_iface);
> +    IMFTopologyNode_CloneFrom(clone_sink, &sink->IMFTopologyNode_iface);
> +
> +    if (FAILED(IMFTopologyNode_GetUINT32(clone_sink, &MF_TOPONODE_STREAMID, &streamid)))
> +        IMFTopologyNode_SetUINT32(clone_sink, &MF_TOPONODE_STREAMID, 0);
I would prefer to clone whole topology first, and do all necessary fixups.

> +    if (enum_src_types)
> +    {
> +        hr = IMFAttributes_GetUINT32(attrs_src, &MF_TOPONODE_CONNECT_METHOD, &method);
> +        if (hr == S_OK && method != MF_CONNECT_RESOLVE_INDEPENDENT_OUTPUTTYPES)
> +        {
> +            for (method = MF_CONNECT_DIRECT; method < MF_CONNECT_RESOLVE_INDEPENDENT_OUTPUTTYPES; method++)
> +                for (i = 0; i < num_media_types; i++)
> +                    if (SUCCEEDED(topology_loader_resolve_branch(clone_src, src_mediatypes[i], clone_sink, method)))
> +                    {
> +                        topology_loader_add_branch(*full_topology, clone_src, clone_sink);
> +                        heap_free(src_mediatypes);
> +                        return S_OK;
> +                    }
> +        }
> +        else
> +        {
> +            for (i = 0; i < num_media_types; i++)
> +                for (method = MF_CONNECT_DIRECT; method < MF_CONNECT_RESOLVE_INDEPENDENT_OUTPUTTYPES; method++)
> +                    if (SUCCEEDED(topology_loader_resolve_branch(clone_src, src_mediatypes[i], clone_sink, method)))
> +                    {
> +                        topology_loader_add_branch(*full_topology, clone_src, clone_sink);
> +                        heap_free(src_mediatypes);
> +                        return S_OK;
> +                    }
> +        }
> +    }
> +    else
> +    {
> +        if (SUCCEEDED(topology_loader_resolve_branch(clone_src, src_mediatypes[0], clone_sink, MF_CONNECT_DIRECT)))
> +        {
> +            topology_loader_add_branch(*full_topology, clone_src, clone_sink);
> +            heap_free(src_mediatypes);
> +            return S_OK;
> +        }
> +    }
'enum_src_types' should only control if all supported output types are 
considered, and not just current, so why false branch is using DIRECT?
I'd expect you still might want a decoder.

I would suggest to split this patch as much as possible. It's hard to 
see the whole picture, but so far I have a feeling that assuming that you
need to resolve from source to sink all the time is not sustainable. For 
example if partial topology already has decoders for some branches that 
won't apply.
More general approach of resolving from input to output might be better, 
for example taking sink media type, comparing to upstream peer, 
regardless of node types.