GNOME Bugzilla – Bug 751105
baseparse: drain on stream-start
Last modified: 2018-11-03 12:28:05 UTC
When handling stream-start events, baseparse should drain its current data and then reset itself as a new stream is about to start, no data from previous stream should be reused.
Created attachment 305478 [details] [review] baseparse: drain and reset on stream-start When a stream-start event is received it indicates a new stream is starting and elements should reset their status to start processing new data. This patch makes baseparse drain the current data and then reset itself when handling this event.
Created attachment 305479 [details] [review] h264parse: reset parser state After reset h264parse shouldn't rely on having received previous SPS or PPS
Created attachment 305480 [details] [review] h264parse: handle stream-start event Reset internal state when stream-start is received
The bug is about baseparse but I decided to attach those 2 extra patches here for h264parse as it was the use case that had me modifying baseparse. I've been trying to use h264parse to play 2 streams sequentially and I found that it was using data from a previous stream to parse the new one leading to broken decoding and bad frames on screen. Using the stream-start to signal the beginning of a completely new stream seems to make sense.
Haven't looked at the patches yet, but I think it makes sense conceptually and seems like the right thing to do.
Review of attachment 305478 [details] [review]: Shouldn't it do it only if the stream-id changed? The same event could have been re-sent, or some other property than the stream-id could have changed.
If the group-id changes, it should also drain. So maybe check both, yes.
I don't know if the fact that it's the same stream semantically has any meaning with regard to whether any of the technical parameters extracted from it are still valid. Imagine you have a long stream and are now going back to the start and feeding data again from the beginning. Do we know cases where stream-start events are gratuitiously re-sent? Or some property changes?
Also note that videodecoder and others are also draining unconditionally on stream-start events already.
Nicolas also told me I was an idiot, forget what I said about stream-start events being re-sent.
Comment on attachment 305480 [details] [review] h264parse: handle stream-start event Shouldn't the draining make this happen already? Or some GstBaseParse::reset()?
(In reply to Sebastian Dröge (slomo) from comment #11) > Comment on attachment 305480 [details] [review] [review] > h264parse: handle stream-start event > > Shouldn't the draining make this happen already? Or some > GstBaseParse::reset()? baseparse class doesn't have a reset(), we could add that and call it from the baseparse reset() function, then we wouldn't have to deal with the stream-start directly in h264parse. Both ways are ok with me, I went with this one to avoid adding extra API as it could be done from the event handler anyway.
A ::reset() has the problem to decide when to call it, like in the other base classes. And what makes it different from a ::flush() :) But I think your patch shows that something there is needed, it's probably not the only parser (definitely not: h265parse).
So, should we go with reset or flush? I think reset is more appropriate as it is usually what we call when we do a READY-PAUSED transition, meaning *all* data is cleared. But then what is different in a flush? Is some data preserved? Flush events, for example, don't remove the caps from pads, so we assume the caps are to remain the same.
Well, flush() would be something that we call on flush-stop. You probably don't want to drop the SPS/PPS on flush-stop, e.g. after a seek you might not get a new one :)
Created attachment 306031 [details] [review] baseparse: add reset virtual function Used to instruct subclasses to reset their state and be ready to start processing a new stream. It is optional and was added to handle when a new STREAM_START event is received before a new stream starts, making sure the subclass is ready to start processing again without residues from a previous stream.
Created attachment 306032 [details] [review] h264parse: implement reset() method Allow h264parse to reset internal state when a new stream is about to start.
will do the same to h265parse once those patches are accepted. And also check some other parsers to see if they should also need this reset() handling.
Comment on attachment 306031 [details] [review] baseparse: add reset virtual function Should it also be called after drain? Or after start() or before stop() or something like that? Or when completely new caps arrive? Or ...? :)
Reset is called from the reset() function that already was present at base parse. So it is now being called at: init stream-start after stop Lots of elements call a reset() to initialize their variables but it feels weird to have it at a constructor. Drain() serves a different purpose and not necessarily means it needs to reset, might just be a discont handling. new caps also might not necessarily mean a reset, at least not a hard one as this is supposed to mean. For start() and stop() it might make sense, but the subclass can also call directly their implementation if needed.
Ok, maybe make that more clear in the docs but apart from that just go ahead :)
What's the status here?
Thiago?
Ping?
Hi, this was part of a bigger change to allow playing media sequentially without having to put the pipeline to NULL and back. Specially on small devices, resetting hardware elements was costly. This was done to mitigate this delay when we knew the streams shared the same formats over time. This use case is not a direct need for me anymore but it might still be interesting for more people. If you agree, let me know and I'll test those patches again and get them merged.
Didn't review the patches, but it makes sense to me to drain on stream-start. gst_base_parse_drain() also has just been made public if it helps with anything.
Yes, we should get this change in. The STREAM_START part makes sense in general and is the same that is done elsewhere.
We should definitely try to get these patches in.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer/issues/116.