GNOME Bugzilla – Bug 731769
onvif: add element implementing the ONVIF RTP extension
Last modified: 2016-03-29 04:50:36 UTC
Created attachment 278581 [details] [review] add onviftimestamp_apply element The following patch introduces a new element 'onviftimestamp_apply' implementing the ONVIF timestamp RTP extension as defined in http://www.onvif.org/specs/stream/ONVIF-Streaming-Spec-v241.pdf (pg 25). Note that the fixes from bug #730563 are needed in order to use this extension properly.
Review of attachment 278581 [details] [review]: This generally looks good, but it introduces a one frame latency without ever declaring it. Which is quite annoying to do because it's in RTP land and we have no idea of the buffer rate. ::: gst/onvif/gstonvif.c @@ +31,3 @@ +{ + if (!gst_element_register (plugin, "onviftimestamp_apply", GST_RANK_NONE, + GST_TYPE_ONVIF_TIMESTAMP_APPLY)) I don't like the name too much, any reason not to name it "rtponviftimestamp" for example? That would say it's about RTP, and you can use "timestamp" as a verb" ::: gst/onvif/gstonviftimestamp-apply.c @@ +212,3 @@ + gst_event_copy_segment (event, &self->segment); + } else if (GST_EVENT_TYPE (event) == GST_EVENT_FLUSH_START) { + gst_segment_init (&self->segment, GST_FORMAT_UNDEFINED); This should probably happen on flush stop (as in this case, the stream lock is held). This function may also be a good case for a switch/case.
(In reply to comment #1) > Review of attachment 278581 [details] [review]: > > This generally looks good, but it introduces a one frame latency without ever > declaring it. Which is quite annoying to do because it's in RTP land and we > have no idea of the buffer rate. That's why we implemented the 'set-e-bit' property, defaulting to FALSE. While it's not set no latency is introduced. > ::: gst/onvif/gstonvif.c > @@ +31,3 @@ > +{ > + if (!gst_element_register (plugin, "onviftimestamp_apply", GST_RANK_NONE, > + GST_TYPE_ONVIF_TIMESTAMP_APPLY)) > > I don't like the name too much, any reason not to name it "rtponviftimestamp" > for example? That would say it's about RTP, and you can use "timestamp" as a > verb" We have some plan to have the reverse, client side, element which would be called "onviftimestamp_extract". Suggestions for better names welcome. > ::: gst/onvif/gstonviftimestamp-apply.c > @@ +212,3 @@ > + gst_event_copy_segment (event, &self->segment); > + } else if (GST_EVENT_TYPE (event) == GST_EVENT_FLUSH_START) { > + gst_segment_init (&self->segment, GST_FORMAT_UNDEFINED); > > This should probably happen on flush stop (as in this case, the stream lock is > held). You mean that gst_segment_init() should be called for both events? FLUSH_START and FLUSH_STOP? > This function may also be a good case for a switch/case. done.
Created attachment 279115 [details] [review] add onviftimestamp_apply element
(In reply to comment #2) > (In reply to comment #1) > > ::: gst/onvif/gstonviftimestamp-apply.c > > @@ +212,3 @@ > > + gst_event_copy_segment (event, &self->segment); > > + } else if (GST_EVENT_TYPE (event) == GST_EVENT_FLUSH_START) { > > + gst_segment_init (&self->segment, GST_FORMAT_UNDEFINED); > > > > This should probably happen on flush stop (as in this case, the stream lock is > > held). > > You mean that gst_segment_init() should be called for both events? FLUSH_START > and FLUSH_STOP? I think only on flush_stop, as the stream lock is not held during flush-start, so I think it could race with the segment event.
Created attachment 279314 [details] [review] add onviftimestamp_apply element
(In reply to comment #4) > (In reply to comment #2) > > (In reply to comment #1) > > > ::: gst/onvif/gstonviftimestamp-apply.c > > > @@ +212,3 @@ > > > + gst_event_copy_segment (event, &self->segment); > > > + } else if (GST_EVENT_TYPE (event) == GST_EVENT_FLUSH_START) { > > > + gst_segment_init (&self->segment, GST_FORMAT_UNDEFINED); > > > > > > This should probably happen on flush stop (as in this case, the stream lock is > > > held). > > > > You mean that gst_segment_init() should be called for both events? FLUSH_START > > and FLUSH_STOP? > > I think only on flush_stop, as the stream lock is not held during flush-start, > so I think it could race with the segment event. Ok; I changed that.
Created attachment 289473 [details] [review] add onviftimestamp_apply element
Created attachment 289474 [details] [review] add rtponviftimestamp element
Created attachment 289475 [details] [review] add rtponvifextract element
Review of attachment 289474 [details] [review]: ::: gst/onvif/gstrtponviftimestamp.c @@ +161,3 @@ + g_param_spec_uint64 ("ntp-offset", "NTP offset", + "Offset between the pipeline running time and the absolute UTC time, " + "in seconds since 1900", Instead of having the app set this, if, on the paused->playing, we could just do a gettimeofday()+gst_clock_get_time(), and that as a base time for the translation to NTP time? @@ +302,3 @@ + + /* NTP timestamp */ + if (GST_BUFFER_DTS_IS_VALID (buf)) { Shouldn't it use PTS as a first attempt ?
(In reply to comment #10) > Review of attachment 289474 [details] [review]: > > ::: gst/onvif/gstrtponviftimestamp.c > @@ +161,3 @@ > + g_param_spec_uint64 ("ntp-offset", "NTP offset", > + "Offset between the pipeline running time and the absolute UTC time, > " > + "in seconds since 1900", > > Instead of having the app set this, if, on the paused->playing, we could just > do a gettimeofday()+gst_clock_get_time(), and that as a base time for the > translation to NTP time? Basically the same thing as what is in the get_current_times() here: http://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/gst/rtpmanager/gstrtpsession.c#n855
Created attachment 290889 [details] [review] rtponviftimestamp: Automatically discover the ntp-offset by default. This only works if no packets are pushed in before the pipeline goes to playing. So it helps to have a live source. The DTS is indeed the right choice as the spec says the NTP should advance always. But it seems a bit buggy as the Onvif spec also says that the NTP should correspond to the frame, which I would assume means the PTS.
The current element creates a NTP time that matches the NTP time used in rtcp only if "use-pipeline-clock" property is enabled on the rtpbin. Otherwise, if the pipeline clock and system clocks have different speeds, they will slowly drift apart. As the NTP timestamps in the ONVIF RTP header extension is based on the pipeline clock (the buffer timestamps). That said, unless the thing runs for a very lonig time, it's probably not a huge problem. I guess we could re-do the matching for every buffer (or for every N buffers) to avoid this problem if it becomes an issue.
Merged: commit 8cddfe64774eb0f47e6026f96b56e97fb40c39bc Author: Olivier Crête <olivier.crete@collabora.com> Date: Mon Nov 17 19:26:53 2014 -0500 rtponviftimestamp: Automatically discover the ntp-offset by default. This only works if no packets are pushed in before the pipeline goes to playing. So it helps to have a live source. https://bugzilla.gnome.org/show_bug.cgi?id=731769 commit 189005184c2f398509500ad69a2ef74708cfa576 Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk> Date: Thu Jun 5 15:06:33 2014 +0200 add rtponvifextract element https://bugzilla.gnome.org/show_bug.cgi?id=731769 commit b424b72df12965dea8db6c7ad8bb9c96d73f015f Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk> Date: Mon Apr 28 11:07:17 2014 +0200 add rtponviftimestamp element https://bugzilla.gnome.org/show_bug.cgi?id=731769