GNOME Bugzilla – Bug 654900
gdppay: Sending a duplicated newsegment event for every tag event
Last modified: 2014-11-26 15:50:58 UTC
Currently gdppay resets the streamheader for every tag event because the tag events are part of the streamheader. This results in a new "streamheader packet" to be sent downstream, which also includes the current segment as newsegment event. On the depayloading side these additional newsegment events are depayloaded as usual and are causing confusion downstream because they always open a new segment. Simple way to reproduce this is: gst-launch-0.10 -v videotestsrc num-buffers=100 ! xvidenc ! mpeg4videoparse ! gdppay ! gdpdepay ! fakesink Just look at all the newsegment events that appear in fakesink. From the debug logs these happen because of the tag events sent by mpeg4videoparse.
The same happens for every caps changes. As of now, ffdec_mpeg4 tends to change its output caps a lot (see [1]). The change of caps trigger the same issue as with tag events: another newsegment event is sent in the payload. [1] https://bugzilla.gnome.org/show_bug.cgi?id=670142
Created attachment 207784 [details] [review] gdppay: only send streamheader buffers that changed Here is a proposed patch to solve the issue. The problem as I see it is that _reset_streamheader() indiscriminately (re)sends all the buffers that make the streamheader (newsegment and tag events + caps), whereas it should only send the ones that have changed if the streamheader has been sent previously. I think this patch would be nicer if we could add information in GstGDPPay itself on whether the various buffers (caps_buf, new_segment_buf and tag_buf) have already been sent, but I guess we don't want to break ABI (and there is no reserved space in the structure), that's why I went for the solution with an added argument to _reset_streamheader().
If we want to change the behaviour of gdppay this drastically, the elements that use streamheaders need to be adapted somehow. Right now multifdsink doesn't know anything about the contents of those buffers, so how is it supposed to "know" that it already sent a newsegment to one of its clients, or that it is supposed to send an old newsegment from a previous streamheader ? I think it makes a lot more sense to have gdpdepay do this logic, and ignore newsegment events it's already seen.
@thomas: do you have a suggested simple way to test interaction with multifdsink?
Created attachment 211823 [details] [review] gdpdepay: Avoid sending twice the same newsegment event Here is a first attempt at fixing this on the side of gdpdepay. This patch seems to work, but has a few suspected drawbacks: - I suspect that the following case is possible (at least theoretically, and we break it): if upstream sends a newsegment event that is identical to the last one it sent, but actually wants them to be accumulated - not sure it's safe to add a field to GstGDPDepay (ABI breakage) - We probably need to reset ->last_newsegment_event in some places (like when flushing or when going to READY) Anyway, I still need to understand better how the streamheader thing works and if/why the previous patch actually breaks that.
I've played a bit with both patches with tcpserversink and with flumotion (both live and ondemand), and things seem to behave in the same way as before the patch. From what I understand, I don't think the first version, where the modification is done in gdppay, is dangerous for multifdsink since multifdsink only seems to care about the streamheader and whether it changes, and the code that handles the streamheader is not modified by the patch. The patch only removes the duplicate sending (as buffers sent along the pipeline) of some buffers (the ones encoding newsegments/tags/caps if they didn't change). So I believe that multifdsink would only have to send the buffers in its streamheader for a new connection, later if upstream changes segment, gdppay with that patch would both update the streamheader and send a buffer with the gdp-encoded newsegment event, as it does without the patch. @thomas if you still think that multifdsink would need some adaptation, then there is something I did not understand, and I would like to have it explained to me :) Also, the method employed in the patch in gdpdepay has plenty of drawbacks (listed in my previous comment), and I can't really think of another way to do that. @thomas if you have a set-up that makes it easy to test various segment cases with flumotion, it would be cool to try these patches on it.
Any news on this?
Let's close this, I think this code has been rewritten quite a bit in gdp because of the sticky events. If it's still a problem in 1.x, please re-open.