From: "Zebediah Figura (she/her)" Subject: Re: [PATCH v2 3/4] winegstreamer: Only seek if it was requested by the caller. Message-Id: <7901bf90-6ece-1969-90be-136710b88841@codeweavers.com> Date: Tue, 15 Jun 2021 10:33:29 -0500 In-Reply-To: References: <20210611105356.311376-1-gmascellani@codeweavers.com> <20210611105356.311376-3-gmascellani@codeweavers.com> On 6/11/21 4:10 PM, Zebediah Figura (she/her) wrote: > On 6/11/21 5:53 AM, Giovanni Mascellani wrote: >> Signed-off-by: Giovanni Mascellani >> --- >> dlls/winegstreamer/media_source.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c >> index 3e1e3001dc7..9188a350305 100644 >> --- a/dlls/winegstreamer/media_source.c >> +++ b/dlls/winegstreamer/media_source.c >> @@ -324,8 +324,9 @@ static void start_pipeline(struct media_source *source, struct source_async_comm >> >> source->state = SOURCE_RUNNING; >> >> - 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_I8) >> + 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); >> } >> >> > > I don't think this works reliably as-is. The API is a little awkward and > doesn't communicate this clearly (sorry), but if GStreamer gives us a > sample between wg_parser_stream_enable() and wg_parser_end_flush(), > we'll return GST_FLOW_FLUSHING, which will generally cause the upstream > element to stop sending data. The seek event is necessary to tell it to > start sending data again. > > This situation is probably more than a little undesirable. We probably > need to modify the interface and backend to prevent any samples from > getting lost (and, ideally, to avoid seeking the GStreamer backend if we > don't need to). Urgh, I confused myself, this isn't actually how it works. wg_parser_begin_flush() actually only exists to unblock a thread stuck in wg_parser_stream_get_event(). Unfortunately I named two different variables "flushing" :-( So I think the patch should be fine as-is. > What happens if you change which streams are selected without seeking? > Currently the WG backend discards any events/buffers on disabled > streams, which means that in general they won't be synchronized and we > may end up throwing away an entire stream. My guess is that this is > fine, because as far as I can tell mfplat doesn't actually make any > synchronization guarantees even among selected streams, but I'd like to > make sure... I am still curious about this part, though.