On 9/29/21 17:30, Zebediah Figura (she/her) wrote:
On 9/29/21 02:59, Derek Lesho wrote:
On 9/29/21 00:18, Zebediah Figura (she/her) wrote:
On 9/28/21 03:08, Derek Lesho wrote:
On 9/28/21 07:23, Zebediah Figura (she/her) wrote:
On 9/24/21 14:23, Derek Lesho wrote:
On 9/24/21 12:38, Zebediah Figura wrote: > On 9/24/21 3:05 AM, Derek Lesho wrote: >> On 9/21/21 13:02, Zebediah Figura wrote: >> >>> >>> >>> 1. Rely on the fact that seek-data is sent during a seek, and >>> use it >>> to effectively invalidate any buffers sent before the seek. >>> Unlike the >>> aforementioned problem with validation, this will actually >>> work: at >>> the time that seek-data is sent appsrc should be flushing, so any >>> buffers we send before it will be discarded by appsrc, and any >>> buffers >>> we send afterward can be discarded by us if they aren't valid. >>> This is >>> very fragile, though. There's really no reason for appsrc to send >>> seek-data for random-access streams in the first place, and >>> this kind >>> of synchronization is easy to get wrong if I haven't already. >> >> This sounds like the best way forward in my opinion, how is it >> fragile? > > It's fragile in general because we're making a lot of > assumptions, that aren't documented, about when and from which > thread gstappsrc will send these signals, and what > synchronization guarantees it applies when doing so.
My perspective is that if we can get it working, documenting the two required quirks to the GStreamer project, eventually the problem will be be cleared up and/or fixed, and we can remove the quirks then.
The problem is that we're still going to have to keep those workarounds for a long time. Not to mention that clearly filing a bug is not enough to get the attention of the GStreamer developers.
As the maintainer of this code, I don't think I feel very comfortable relying this much on the internals of appsrc, whether as workarounds or not.
> >> It seems that if we just ensure that a pushed buffer takes into >> account >> the latest seek/need data pair, not much can go wrong. >> > > That's not enough by itself. We can, generally speaking, send a > buffer after a seek-data/need-data is triggered but before it is > actually sent. Before what is sent?
Before the seek-data or need-data signal is sent. There is a window there, however short.
I'm not sure exactly what the difference between triggering a signal and sending a signal is, but assuming you mean that if seek-data doesn't acquire the mutex, it would be possible to push-buffer to not see the seek, then send the buffer after seek-data, then yeah, we do need to put seek-data and push-buffer in the same mutex to make sure that push-buffer is responding to the latest seek/need data callbacks.
> This happens in practice with flushes. > > In order to get around that, you need the hack I mentioned: > assume that a flush will be accompanied by a seek-data signal. > This is especially fragile because there's no reason for appsrc > to even be sending this signal in the first place (it only > really makes sense in "seekable" mode), and they could easily > decide to not do that.
I'm not sure I understand, whenever the next buffer app source wants is not consecutively after the last, seek-data is required (and sent). What does "assume the flush will be accompanied by a seek-data signal" mean? The app source client doesn't have any conception of a flush, just seek-data and need-data, and to fix this problem all we need to do is add a quite rational check that push_data is responding to the latest seek/need data pair.
Let me try to explain more clearly. At least one race, the one I've been trying to describe, looks like this:
read thread main thread
push seek event retrieve data validate offset send flush-start emit seek-data send flush-stop push-buffer
At which point appsrc will get the wrong buffer.
Solution 1 has us rely on the seek-data signal to effectively wait for the read thread (most likely using the parser mutex) and, in a sense, put a barrier between reads occuring before and after the flush.
I mean, yeah, but from the perspective of the interface, all we are doing is making sure that buffers that are responding to requests from before a seek-data don't get sent, which I really don't think should be too controversial. That the seek happens to be caused by a flush shouldn't matter, we're putting a barrier between buffers sent for a different offset, and the current offset we've gotten from seek-data. Honestly, I'm not even sure this is a GStreamer bug, should they really have to handle a case where the client responds with an incorrect buffer after a seek?
I don't think this is obvious at all. It's certainly not called out in the documentation.
The documentation does say "After receiving the seek-data signal, the application should push-buffers from the new position."
There's quite a difference between that and "make sure that any old buffers are flushed out,
Old buffers don't need to be flushed out, app source does that.
and don't send any new buffers from the old position".
"don't send any new buffers from the old position" logically has the same meaning as "should push-buffers from the new position".
And, moreover, it's a lot of work, and it doesn't even look like a remotely idiomatic solution.
As I've already said, making sure we don't push buffers to an old offset after receiving a seek seems quite idiomatic to me.
Not to mention that, as I've said, appsrc really has no reason to send seek-data at all here.
It sends it here to ensure that pre-flush buffers are discarded. After it calls seek-data, it clears the queue, because of the aforementioned expectation, namely that the app will respond to seek-data, this works.
Well, no, I don't think it does. I think it sends seek-data here for "seekable" mode, because that actually makes sense, and ends up also sending it for "random-access" mode unnecessarily.
Sending it for "random-access" mode does makes sense, because downstream wants all future(post flush) buffers to be from a new offset, so it needs to tell the app to stop sending buffers from the old location, and start from the new location, then discard the queued buffer/s for the old location if needed. As mentioned above this, according to the documentation, when a seek-data is sent, buffers sent afterwards must be responsive to the new offset.
I'm pretty sure that you're correct that putting the whole of wg_parser_push_data inside a mutex, and the seek-data callback inside the same mutex, is actually sufficient to fix this race. But that stops being true if appsrc stops sending seek-data here.
What reason would appsrc have to stop sending seek-data here, it has to as far as I can see.
Fundamentally the API contract here is that you don't send old buffers after a flush stops. DirectShow and GStreamer are both built on that idea. I believe that Media Foundation does away with it, which is one of the good things about Media Foundation. appsrc doesn't expose flushes to us, and it doesn't seem to do a good enough job of taking care of them itself.
I think that requiring us not to send old buffers after seek-data and sending seek-data when they flush, then discarding the rest of the queue is a pretty good way of taking care of flushes.
Trying to infer when they happen is too fragile for my taste.
We don't need to infer when they happen, all we need to do is follow the guideline not to send buffers to a previous offset after seek-data.