[PATCH 08/16] winegstreamer: Implement media source on gstreamer.

[Nikolay Sivov]

On 3/26/20 5:13 PM, Derek Lesho wrote:
>
> On 3/26/20 12:23 AM, Nikolay Sivov wrote:
>> On 3/26/20 3:12 AM, Derek Lesho wrote:
>>> +static ULONG WINAPI media_stream_Release(IMFMediaStream *iface)
>>> +{
>>> +    struct media_stream *This = impl_from_IMFMediaStream(iface);
>>> +
>>> +    ULONG ref = InterlockedDecrement(&This->ref);
>>> +
>>> +    TRACE("(%p) ref=%u\n", This, ref);
>>> +
>>> +    if (!ref)
>>> +    {
>>> +        if (This->state != STREAM_SHUTDOWN)
>>> +            ERR("incomplete cleanup\n");
>>> +        heap_free(This);
>>> +    }
>>> +
>>> +    return ref;
>>> +}
>> Error message here is redundant I think. The point of shutdown is to 
>> break circular references,
>> where source holds streams references and every stream hold source 
>> reference. So in theory
>> you shouldn't be able to release unless you shut it down.
>>
>>> +static HRESULT WINAPI media_stream_GetEvent(IMFMediaStream *iface, 
>>> DWORD flags, IMFMediaEvent **event)
>>> +{
>>> +    struct media_stream *This = impl_from_IMFMediaStream(iface);
>>> +
>>> +    TRACE("(%p)->(%#x, %p)\n", This, flags, event);
>>> +
>>> +    if (This->state == STREAM_SHUTDOWN)
>>> +        return MF_E_SHUTDOWN;
>>> +
>>> +    return IMFMediaEventQueue_GetEvent(This->event_queue, flags, 
>>> event);
>>> +}
>> Same as with the source, you only need to shutdown the queue and it 
>> will return MF_E_SHUTDOWN for you.
>>
>>> +static HRESULT WINAPI 
>>> media_stream_GetStreamDescriptor(IMFMediaStream* iface, 
>>> IMFStreamDescriptor **descriptor)
>>> +{
>>> +    struct media_stream *This = impl_from_IMFMediaStream(iface);
>>> +
>>> +    TRACE("(%p)->(%p)\n", This, descriptor);
>>> +
>>> +    if (This->state == STREAM_SHUTDOWN)
>>> +        return MF_E_SHUTDOWN;
>>> +
>>> +    IMFStreamDescriptor_AddRef(This->descriptor);
>>> +    *descriptor = This->descriptor;
>>> +
>>> +    return S_OK;
>>> +}
>> It's not obvious that it's correct but could be problematic to test 
>> properly, because it could differ between implementations.
> What do you mean?  What might not be correct?
Failing to return descriptor.
>>
>>> +    req = heap_alloc(sizeof(*req));
>>> +    if (token)
>>> +        IUnknown_AddRef(token);
>>> +    req->token = token;
>>> +    list_add_tail(&This->sample_requests, &req->entry);
>> This should be protected, there is no guarantee client is using the 
>> same thread to make requests, or serializes them.
>>
>>> @@ -118,28 +684,102 @@ static HRESULT WINAPI 
>>> media_source_GetCharacteristics(IMFMediaSource *iface, DWO
>>>   {
>>>       struct media_source *This = impl_from_IMFMediaSource(iface);
>>>   -    FIXME("(%p)->(%p): stub\n", This, characteristics);
>>> +    TRACE("(%p)->(%p)\n", This, characteristics);
>>>   -    return E_NOTIMPL;
>>> +    if (This->state == SOURCE_SHUTDOWN)
>>> +        return MF_E_SHUTDOWN;
>>> +
>>> +    *characteristics = 0;
>>> +
>>> +    return S_OK;
>>>   }
>> Returning 0 flags will prevent seeking. It should be derived from 
>> bytestream caps + gstreamer object constraints if any. There is a 
>> number of flags,
>> but CAN_PAUSE/CAN_SEEK are probably most important ones.
> Right now I haven't implemented pausing/seeking, shouldn't we wait 
> until we support it before adding the characteristics?
Sure, I don't know why I assumed seeking was supposed to work.
>>
>>>   static HRESULT WINAPI 
>>> media_source_CreatePresentationDescriptor(IMFMediaSource *iface, 
>>> IMFPresentationDescriptor **descriptor)
>>>   {
>>>       struct media_source *This = impl_from_IMFMediaSource(iface);
>>>   -    FIXME("(%p)->(%p): stub\n", This, descriptor);
>>> +    TRACE("(%p)->(%p)\n", This, descriptor);
>>>   -    return E_NOTIMPL;
>>> +    if (This->state == SOURCE_SHUTDOWN)
>>> +        return MF_E_SHUTDOWN;
>>> +
>>> +    if (!(This->pres_desc))
>>> +    {
>>> +        return MF_E_NOT_INITIALIZED;
>>> +    }
>>> +
>>> +    IMFPresentationDescriptor_Clone(This->pres_desc, descriptor);
>>> +
>>> +    return S_OK;
>>>   }
>> Is NOT_INITIALIZED path reachable? I think it's generally assumed 
>> that once you got a source you can get its descriptor, even if stream 
>> configuration
>> can change dynamically later.