GNOME Bugzilla – Bug 722791
basesrc: not respecting seqnum of eos from send_event
Last modified: 2014-01-24 16:49:15 UTC
Here is the program https://github.com/hizukiayaka/rtsp_gst_record The rtsp server is from https://github.com/revmischa/rtsp-server the src is made by a libav 0.8 program The program will record a video and then change it the another file after a period of time. I have done it before if the EOS is not invoked, but the output mkv file won't have the correct time information. Then I modify the source code and met the problem. But thiagoss said “indeed there seems to be a bug with the eos forwarding” Here the log of GST_DEBUG=GST_EVENT:9 ./server > gst.log 2>&1 can be found on the github.
Created attachment 267011 [details] [review] basesrc: preserve seqnum of eos events sent by the user Store the eos event seqnum and use it when creating the new eos event to be pushed downstream Useful if the application wants to check if the EOS message was generated from its own pushed EOS or from another source (stream really finished). Also adds a test for this
Review of attachment 267011 [details] [review]: The seqnum should be the one of the segment, and as such of the seek that happened before. Check the design docs about seqnums, I think this approach is wrong according to the docs. ::: libs/gst/base/gstbasesrc.c @@ +231,3 @@ + /* seqnum of the EOS if pending (atomic) */ + guint pending_eos; 0 is a valid seqnum
(In reply to comment #2) > Review of attachment 267011 [details] [review]: > > The seqnum should be the one of the segment, and as such of the seek that > happened before. Check the design docs about seqnums, I think this approach is > wrong according to the docs. Should the seqnum for the EOS always be the same of the segment even when there was no seek? If so, this should be NOTABUG, right? > > ::: libs/gst/base/gstbasesrc.c > @@ +231,3 @@ > > + /* seqnum of the EOS if pending (atomic) */ > + guint pending_eos; > > 0 is a valid seqnum true, got confused with g_atomic_int_add
Created attachment 267094 [details] [review] docs: design: add part-seqnums Hopefully clarifies how seqnums should be used and copied from events to events/messages when those are handled.
Created attachment 267095 [details] [review] basesrc: preserve seqnum of eos events sent by the user Store the eos event seqnum and use it when creating the new eos event to be pushed downstream. To know if the eos was caused by the eos events received on send_event, a 'forced_eos' flag is used to use the correct seqnum on the event pushed downstream. Useful if the application wants to check if the EOS message was generated from its own pushed EOS or from another source (stream really finished). Also adds a test for this
Review of attachment 267094 [details] [review]: Looks good, you might want to have Wim read it too
Review of attachment 267095 [details] [review]: Generally looks good ::: libs/gst/base/gstbasesrc.c @@ +194,3 @@ + if (bsrc->priv->pending_eos) { \ + gst_event_unref (bsrc->priv->pending_eos); \ + bsrc->priv->pending_eos = NULL; \ Just use gst_event_replace() maybe? Makes this macro two lines instead of many @@ +1799,3 @@ + if (src->priv->pending_eos) + gst_event_unref (src->priv->pending_eos); + src->priv->pending_eos = event; Shouldn't we set forced_eos here? Also what's the point of using atomic operations if you lock anyway?
(In reply to comment #7) > Review of attachment 267095 [details] [review]: > > Generally looks good > > ::: libs/gst/base/gstbasesrc.c > @@ +194,3 @@ > + if (bsrc->priv->pending_eos) { \ > + gst_event_unref (bsrc->priv->pending_eos); \ > + bsrc->priv->pending_eos = NULL; \ > > Just use gst_event_replace() maybe? Makes this macro two lines instead of many Updated. > > @@ +1799,3 @@ > + if (src->priv->pending_eos) > + gst_event_unref (src->priv->pending_eos); > + src->priv->pending_eos = event; > > Shouldn't we set forced_eos here? Also what's the point of using atomic > operations if you lock anyway? forced_eos is only set from the _loop/_getrange functions to identify when it was stopped because of the user sent EOS or if it was for another reason (end of segment, end of file...) About the atomic operations, I just followed the same strategy used for src->priv->have_events and pending_events. I think the idea is to avoid locking every time at the loop and use the atomic int to only lock if required.
Created attachment 267126 [details] [review] basesrc: preserve seqnum of eos events sent by the user Updated patch
Review of attachment 267126 [details] [review]: Looks good
commit fdfc6dc98385816fa3a3765841f40e6c73af58d3 Author: Thiago Santos <ts.santos@sisa.samsung.com> Date: Thu Jan 23 15:52:51 2014 -0300 basesrc: preserve seqnum of eos events sent by the user Store the eos event seqnum and use it when creating the new eos event to be pushed downstream. To know if the eos was caused by the eos events received on send_event, a 'forced_eos' flag is used to use the correct seqnum on the event pushed downstream. Useful if the application wants to check if the EOS message was generated from its own pushed EOS or from another source (stream really finished). Also adds a test for this https://bugzilla.gnome.org/show_bug.cgi?id=722791 commit 5880c9c4c38911bced21854383a5b836d2875f71 Author: Thiago Santos <ts.santos@sisa.samsung.com> Date: Thu Jan 23 15:34:27 2014 -0300 docs: design: add part-seqnums Hopefully clarifies how seqnums should be used and copied from events to events/messages when those are handled. https://bugzilla.gnome.org/show_bug.cgi?id=722791
Comment on attachment 267126 [details] [review] basesrc: preserve seqnum of eos events sent by the user Thanks for the reviews!
commit 78d13b6642968fc1d98f505a3e6d79e3777796da Author: Thiago Santos <ts.santos@sisa.samsung.com> Date: Fri Jan 24 13:20:49 2014 -0300 basesrc: do not forget to clear the forced_eos flag otherwise it will always use the seqnum of the event sent by the application a follow up fix for this bug.