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

[Derek Lesho]

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?
>
>> +    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?
>
>>   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.