[PATCH 1/2] winegstreamer: Implement pausing the media source.

[Nikolay Sivov]

On 6/10/21 10:36 PM, Giovanni Mascellani wrote:
> Hi,
>
> Il 10/06/21 18:59, Nikolay Sivov ha scritto:
>> On 6/10/21 7:25 PM, Zebediah Figura (she/her) wrote:
>>> I agree with Derek's comment here. Assuming that buffering samples
>>> like this is the right thing to do, I also wonder:
>>>
>>> * should we buffer samples, or requests?
>>>
>>> * should the samples/requests be stored in a flat array instead of a
>>> linked list? Based on usage patterns it seems that'd be easier and
>>> more efficient.
>>>
>> Agreed, list vs array is not really important one. Why do we need to
>> buffer samples, is that to cover for discrepancy for already issued
>> requests before Pause() was called? And what should really happen there,
>> if we queue async command on RequestSample() (which is already
>> questionable) any number of times, and then call Pause(), or Stop(), we
>> should have a way to drop pending requests. And that's not possible with
>> MFPutWorkItem().
>>
>> With this patch any Start() when paused results in draining all the
>> samples to the receiver, and that's not obviously correct, unless that
>> was somehow tested. I imagine if you seek to a new position in either
>> direction, you're not really interested in old samples. If session
>> currently does not handle this correctly (by resetting expected samples
>> counters, etc), we should figure out how to test it separately.
>
> We need to somehow remember how many times RequestSample was called
> during pause, because when we start again we have to emit a
> corresponding number of samples. Windows does that.
>
> As for whether we need to buffer requests or samples, I will check
> tomorrow, as I already discussed replying to other reviews.
>
>>> -   
>>> unix_funcs->wg_parser_stream_seek(source->streams[0]->wg_stream, 1.0,
>>> -            position->hVal.QuadPart, 0,
>>> AM_SEEKING_AbsolutePositioning, AM_SEEKING_NoPositioning);
>>> +    if (position->vt != VT_EMPTY)
>>> +       
>>> unix_funcs->wg_parser_stream_seek(source->streams[0]->wg_stream, 1.0,
>>> +                position->hVal.QuadPart, 0,
>>> AM_SEEKING_AbsolutePositioning, AM_SEEKING_NoPositioning);
>>>       unix_funcs->wg_parser_end_flush(source->wg_parser);
>> Is this really relevant to Pause() ?
>
> No, you're right. I will move it to another patch.
>
>>> @@ -318,6 +345,8 @@ static void start_pipeline(struct media_source
>>> *source, struct source_async_comm
>>>                
>>> IMFMediaEventQueue_QueueEventParamVar(stream->event_queue,
>>>                   seek_message ? MEStreamSeeked : MEStreamStarted,
>>> &GUID_NULL, S_OK, position);
>>> +
>>> +            flush_pending_queue(stream, TRUE);
>>>           }
>>>       }
>>>   
>> This will send old samples before MESourceStarted/MESourceSeeked, which
>> is questionable.
>
> As I asked to Derek, is it relevant to compare which event comes first
> from two different queues? I see no indication that two different
> queues should be somehow synchronized. Even if queued MESourceStarted
> before, the application could receive it later.

You can easily check that. Default event queue implementation uses
standard system queue, which is single-threaded. Since you queue events
always from the same thread, I think they might get in order. My main
concern is to get session working correctly in this case, when source
state has not fully transitioned, but samples are coming already.

>
> Thanks, Giovanni.
>