[PATCH v1 1/3] mfreadwrite: Find a decoder in IMFSourceReader::SetCurrentMediaType.
Derek Lesho
dlesho at codeweavers.com
Wed Mar 18 09:53:10 CDT 2020
On 2020-03-18 08:23, Nikolay Sivov wrote:
> 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.
Ack.
>> 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?
It's just a bad comment I forgot to remove.
>> +
>> + 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.
Ack.
>
> 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.
Ah, I didn't think about that, and looking at the return values for the
method, it looks like you need to flush the source reader before you can
change the type.
>> +
>> + 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?
Ack.
>> +
>> + /* 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.
My intention is the advertise whatever the windows variant of the
decoder advertises, and so far I haven't seen an application set the
output type to something the decoder doesn't support. If that ever were
the case, the correct solution would be to use VideoProcessorMFT.
>> +
>> + 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?
The reason we need the second call is so that we return the correct
error code. `MF_E_INVALIDREQUEST` when the type can't be output, and
`MF_E_TOPO_CODEC_NOT_FOUND` when a decoder isn't found for a native
media 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?
Nope, this is just me putting too many logs in.
>> + 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.
Because it auto-filled when I searched INTERNAL_ERROR, whoops. To be
honest, I'm not sure why I added this check in the first place, it
really shouldn't be necessary.
>
> 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.
I'm also unsure whether the source reader can access local transforms,
but it doesn't seem critical to me. Do you think this could instead be
added in a future patch?
>> +
>> + 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.
Is this ever the case with a decoder?
>> +
>> + 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;
>> }
>
>
More information about the wine-devel
mailing list