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.