[PATCH v1 1/3] mfreadwrite: Find a decoder in IMFSourceReader::SetCurrentMediaType.

[Nikolay Sivov]

On 3/17/20 7:48 PM, Derek Lesho wrote:
>               LIST_FOR_EACH_ENTRY_SAFE(ptr, next, &stream->samples, struct sample, entry)
> @@ -755,6 +758,15 @@ static HRESULT WINAPI src_reader_SetCurrentMediaType(IMFSourceReader *iface, DWO
>           IMFMediaType *type)
>   {
>       struct source_reader *reader = impl_from_IMFSourceReader(iface);
> +    DWORD equal_flags;
> +    GUID major_type, target_subtype, decoder_category;
> +    IMFStreamDescriptor *stream_descriptor = NULL;
> +    BOOL selected;
> +    IMFMediaTypeHandler *stream_type_handler = NULL;
> +    IMFMediaType *in_stream_type = NULL, *potential_out_stream_type = NULL;
> +    CLSID decoder_id;
> +    BOOL decoder_found = FALSE;
> +    IMFTransform *decoder = NULL;
>       HRESULT hr;
>   
>       TRACE("%p, %#x, %p, %p.\n", iface, index, reserved, type);
> @@ -774,14 +786,186 @@ static HRESULT WINAPI src_reader_SetCurrentMediaType(IMFSourceReader *iface, DWO
>       if (index >= reader->stream_count)
>           return MF_E_INVALIDSTREAMNUMBER;
>   
> -    /* FIXME: validate passed type and current presentation state. */
> -
Second part of this comment still applies.
>       EnterCriticalSection(&reader->cs);
>   
> -    hr = IMFMediaType_CopyAllItems(type, (IMFAttributes *)reader->streams[index].current);
> +    if (FAILED(hr = IMFMediaType_IsEqual(type, reader->streams[index].current, &equal_flags)))
> +        goto done;
>   
> -    LeaveCriticalSection(&reader->cs);
> +    if (!(equal_flags & MF_MEDIATYPE_EQUAL_MAJOR_TYPES))
> +    {
> +        /* Not gonna happen */
> +        hr = MF_E_INVALIDMEDIATYPE;
> +        goto done;
> +    }
Does it mean usually not going to happen or can't happen?
> +
> +    if (equal_flags & MF_MEDIATYPE_EQUAL_FORMAT_TYPES)
> +    {
> +        /* Similar enough not to need to refind a decoder */
> +        hr = IMFMediaType_CopyAllItems(type, (IMFAttributes *)reader->streams[index].current);
> +        goto done;
> +    }
This should be possible to test if new, yet equal, type attributes 
replace existing attributes. It's not obviously correct.

Regarding skipping decoder decoder lookup, while true, what should 
happen if you already have a decoder?
E.g. should you be able to change current type after you set it once. If 
yes, old decoder should be shut down/released first,
and such switch should be likely rejected if you're currently streaming.
> +
> +    if (FAILED(hr = IMFMediaType_GetMajorType(type, &major_type)))
> +        goto done;
> +
> +    if (FAILED(hr = IMFMediaType_GetGUID(type, &MF_MT_SUBTYPE, &target_subtype)))
> +        goto done;
> +
> +    if (FAILED(hr = IMFPresentationDescriptor_GetStreamDescriptorByIndex(reader->descriptor, index, &selected, &stream_descriptor)))
> +        goto done;
> +
> +    if (FAILED(hr = IMFStreamDescriptor_GetMediaTypeHandler(stream_descriptor, &stream_type_handler)))
> +        goto done;
> +
> +    /* first, check if it's a native type */
> +    for (unsigned int i = 0; hr != MF_E_NO_MORE_TYPES; i++)
> +    {
> +        if (FAILED(hr = IMFSourceReader_GetNativeMediaType(iface, index, i, &in_stream_type)))
> +            continue;
> +
> +        if (FAILED(hr = IMFMediaType_IsEqual(type, in_stream_type, &equal_flags)))
> +            goto done;
> +
> +        if (equal_flags & MF_MEDIATYPE_EQUAL_FORMAT_TYPES)
> +        {
> +            if (FAILED(hr = IMFMediaTypeHandler_SetCurrentMediaType(stream_type_handler, in_stream_type)))
> +                goto done;
> +            hr = IMFMediaType_CopyAllItems(in_stream_type, (IMFAttributes *)reader->streams[index].current);
> +            goto done;
> +        }
> +
> +        IMFMediaType_Release(in_stream_type);
> +        in_stream_type = NULL;
> +    }
Calls preceding this loop essentially duplicate what 
GetNativeMediaType() would do. Could it use IsMediaTypeSupported() instead?

Is it correct to discard input type attributes and use full native type?
> +
> +    /* TODO: should we check if the source type is compressed? */
> +
> +    if (IsEqualGUID(&major_type, &MFMediaType_Video))
> +    {
> +        decoder_category = MFT_CATEGORY_VIDEO_DECODER;
> +    }
> +    else if (IsEqualGUID(&major_type, &MFMediaType_Audio))
> +    {
> +        decoder_category = MFT_CATEGORY_AUDIO_DECODER;
> +    }
> +    else
> +    {
> +        hr = MF_E_TOPO_CODEC_NOT_FOUND;
> +        goto done;
> +    }
Yes, we probably should check that, for example in case of color 
conversion. But as understand current plan for decoders was to advertise
more types and get paired with videoconvert, so maybe for now it's not 
really necessary.
> +
> +    hr = S_OK;
> +    for (unsigned int i = 0; hr != MF_E_NO_MORE_TYPES; i++)
> +    {
> +        GUID source_subtype;
> +        MFT_REGISTER_TYPE_INFO in_type, out_type;
> +        CLSID *decoder_ids;
> +        ULONG decoder_count;
> +
> +        if (FAILED(hr = IMFSourceReader_GetNativeMediaType(iface, index, i, &in_stream_type)))
> +            continue;
> +
> +        if (FAILED(hr = IMFMediaType_GetGUID(in_stream_type, &MF_MT_SUBTYPE, &source_subtype)))
> +            goto done;
> +
> +        in_type.guidMajorType = major_type;
> +        in_type.guidSubtype = source_subtype;
> +
> +        out_type.guidMajorType = major_type;
> +        out_type.guidSubtype = target_subtype;
>   
> +        if (FAILED(hr = MFTEnum(decoder_category, 0, &in_type, &out_type, NULL, &decoder_ids, &decoder_count)) || decoder_count == 0)
> +        {
> +            if (!(decoder_found))
> +            {
> +                /* see if there are other decoders for this stream */
> +                if (SUCCEEDED(MFTEnum(decoder_category, 0, &in_type, NULL, NULL, &decoder_ids, &decoder_count)) && decoder_count > 0)
> +                {
> +                    decoder_found = TRUE;
> +                    CoTaskMemFree(decoder_ids);
> +                }
> +            }
> +
> +            IMFMediaType_Release(in_stream_type);
> +            in_stream_type = NULL;
> +            continue;
> +        }
Is it really useful to have a second call here? It probably means "any" 
output type, which could work in some interactive scenario. Does it really
offer anything available in this case on Windows and what happens to 
current type?
> +
> +        if (decoder_count > 1)
> +            FIXME("Just using the first decoder.\n");
There is a way to sort in MFTEnum*, and it's normal to get a number of 
candidates, so I don't know what this fixme will tell us. Do you mean to
iterate over them all in case advertised types don't actually work?
> +        decoder_id = decoder_ids[0];
> +        CoTaskMemFree(decoder_ids);
> +
> +        if (FAILED(hr = IMFMediaTypeHandler_SetCurrentMediaType(stream_type_handler, in_stream_type)))
> +            goto done;
> +
> +        break;
> +    }
> +
> +    if (!in_stream_type)
> +    {
> +        ERR("Failed to find decoder matching request, source has decoder: %u.\n", decoder_found);
> +        hr = decoder_found ? MF_E_INVALIDREQUEST : MF_E_TOPO_CODEC_NOT_FOUND;
> +        goto done;
> +    }
> +
> +    /* we have the decoder id at this point */
> +    if (FAILED(hr = CoCreateInstance(&decoder_id, NULL, CLSCTX_INPROC_SERVER, &IID_IMFTransform, (void**)&decoder)))
> +        goto done;
> +
> +    {
> +        DWORD in_min, in_max, out_min, out_max;
> +
> +        if (FAILED(hr = IMFTransform_GetStreamLimits(decoder, &in_min, &in_max, &out_min, &out_max)))
> +            goto done;
> +        if (in_min != 1 || in_max != 1 || out_min != 1 || out_max != 1)
> +        {
> +            hr = ERROR_INTERNAL_ERROR;
> +            goto done;
> +        }
> +    }
GetStreamCount() seems easier. Why ERROR_INTERNAL_ERROR? It's not a 
valid failure HRESULT.

Would it help to use IMFActivate instances instead? For example that 
would be the only way to access local transforms, but I don't know if 
reader is using them.
> +
> +    if (FAILED(hr = IMFTransform_SetInputType(decoder, 0, in_stream_type, 0)))
> +        goto done;
> +
> +    /* find the relevant output type */
> +    for (unsigned int i = 0;;i++)
> +    {
> +        if (FAILED(hr = IMFTransform_GetOutputAvailableType(decoder, 0, i, &potential_out_stream_type)))
> +            goto done;
> +
> +        if (FAILED(hr = IMFMediaType_IsEqual(type, potential_out_stream_type, &equal_flags)))
> +            goto done;
> +
> +        if (equal_flags & MF_MEDIATYPE_EQUAL_FORMAT_TYPES)
> +        {
> +            if (FAILED(hr = IMFTransform_SetOutputType(decoder, 0, potential_out_stream_type, 0)))
> +                goto done;
> +
> +            if (FAILED(hr = IMFMediaType_CopyAllItems(potential_out_stream_type, (IMFAttributes *)reader->streams[index].current)))
> +                goto done;
> +
> +            break;
> +        }
> +        IMFMediaType_Release(potential_out_stream_type);
> +        potential_out_stream_type = NULL;
> +    }
This should handle a case when output type should be set first.
> +
> +    reader->streams[index].decoder = decoder;
> +    decoder = NULL;
> +
> +    done:
> +    LeaveCriticalSection(&reader->cs);
> +    if (stream_descriptor)
> +        IMFStreamDescriptor_Release(stream_descriptor);
> +    if (stream_type_handler)
> +        IMFMediaTypeHandler_Release(stream_type_handler);
> +    if (in_stream_type)
> +        IMFMediaType_Release(in_stream_type);
> +    if (decoder)
> +        IMFTransform_Release(decoder);
> +    if (potential_out_stream_type)
> +        IMFMediaType_Release(potential_out_stream_type);
>       return hr;
>   }
>