GNOME Bugzilla – Bug 797162
baseparse: Segment events are not necessarily a discontinuity
Last modified: 2018-11-03 12:48:23 UTC
For a long time (since 0.10.12) baseparse has drained/discared any incomplete packets on a NEW_SEGMENT (now SEGMENT) event and triggered a discont on the next buffer, but in 1.x the semantics of segments changed and segment events don't necessarily imply the data stream is discontinuous now. Baseparse should instead only rely on a DISCONT flag on the next buffer to trigger that behaviour.
Created attachment 373686 [details] [review] baseparse: Don't cause discont on a segment event. For a long time (since 0.10.12) baseparse has drained/discarded any incomplete packets on a NEW_SEGMENT (now SEGMENT) event and triggered a discont on the next buffer, but in 1.x the semantics of segments changed and segment events don't necessarily imply the data stream is discontinuous now. Baseparse should instead only rely on a DISCONT flag on the next buffer to trigger that behaviour.
Doesn't baseparse uses an adapter ? GstAdapter does not have what's needed to properly track time when buffers from multiple segments are in the adapter. Do we need to drain in order to place the new segment at output at the right place. The discont flag though is not needed.
(In reply to Nicolas Dufresne (ndufresne) from comment #2) > Doesn't baseparse uses an adapter ? GstAdapter does not have what's needed > to properly track time when buffers from multiple segments are in the > adapter. Do we need to drain in order to place the new segment at output at > the right place. That indeed needs some checking, but then also needs some detection to see if the segment actually changes anything with regard to sync so that this is needed at all. > The discont flag though is not needed. What do you mean? It's what is signalling that this buffer starts "elsewhere" (independent from time information though, there's another independent flag for that).
(In reply to Sebastian Dröge (slomo) from comment #3) > (In reply to Nicolas Dufresne (ndufresne) from comment #2) > > Doesn't baseparse uses an adapter ? GstAdapter does not have what's needed > > to properly track time when buffers from multiple segments are in the > > adapter. Do we need to drain in order to place the new segment at output at > > the right place. The main problem is that I can't think of the use case for the existing behaviour, and there's no test case for it. I suspect this code should have been rewritten when segments were. > That indeed needs some checking, but then also needs some detection to see > if the segment actually changes anything with regard to sync so that this is > needed at all. If someone can define what baseparse and adapter should do exactly, then we could write some unit tests and implement it. > > The discont flag though is not needed. > > What do you mean? It's what is signalling that this buffer starts > "elsewhere" (independent from time information though, there's another > independent flag for that). He was agreeing with me that the output from base parse should have a discont flag only if the input did. baseparse shouldn't add it just because it got a segment event.
I simply mean that it's not needed if the segment is contiguous. If the segment is not, we can probably assume the following buffer will carry this flags, instead of adding checks all over the place.
-- 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/314.