GNOME Bugzilla – Bug 770221
rtponviftimestamp: Implement property "Immediate"
Last modified: 2016-09-30 07:04:15 UTC
If the property "immediate" is set then is the D flag set in the first rtp buffer to indicate discontinuity. This property is used when the "Immediate" header is included in the rtsp play command.
Created attachment 333868 [details] [review] patch for implementing Immediate property
Comment on attachment 333868 [details] [review] patch for implementing Immediate property What's the meaning of the Immediate header? Wouldn't it be more consistent to always set the D bit after a FLUSH_STOP and on the very first buffer? After all, those buffers are always going to be a discontinuity.
Immediate Header is specified in Onvif Streaming Specification, Chapter 6.9: Go To Time As stated in [RFC 2326] section 10.5, a PLAY command received when replay is already in progress will not take effect until the existing play operation has completed. This specification adds a new RTSP header, “Immediate”, which overrides this behaviour for the PLAY command that it is used with: PLAY rtsp://192.168.0.1/path/to/recording RTSP/1.0 CSeq: 123 Session: 12345678 Require: onvif-replay Range: clock=20090615T114900.440Z- Rate-Control: no Immediate: yes If the server receives a PLAY command with the Immediate header set to “yes”, it will immediately start playing from the new location, cancelling any existing PLAY command. The first packet sent from the new location shall have the D (discontinuity) bit set in its RTP extension header. This is a special use of the D-flag, normally shall only the D-bit be set to indicate a discontinuity in the stream.
However after a *flushing* seek, there is a discontinuity in the stream. Basically you should always receive a buffer with the DISCONT flag set in all cases where you would want to set the D flag, including at the beginning of a stream or after a seek. You always get what this Immediate header would want. I'm not sure why the D flag should not be set on the first packet in general though. What's the difference between "starting playing from a new location, cancelling any existing PLAY command" and "normal PLAY command"?
The definition of D-bit according to the specification: D: 1 bit. Indicates that this access unit follows a discontinuity in transmission. It is primarily used during reverse replay; the first packet of each GOP has the D bit set since it does not chronologically follow the previous packet in the data stream (see section 6.5). So, primarily it is used for reversed replay where each GOP is not considered to follow previous packet since it is played reversed. And then is the special case in Chapter 6.9 defined with the Immediate Header which is supposed to Cancel all previous stacked Play and set the D-bit, why D-bit needs to be set in this case is not explained and it is not very clear since it doesn't seems to be a discontinuity in this case. I and Linus reads the specification as that D-bit shall be used to indicate a discontinuity and for us is not these use cases discontinuity: - Initial Play - Seek However, the use case Play, Pause (for example 1 minute), Play (with no range header) contains a discontinuity but that use case is not either working. This is not detected (currently) by the certification tool but we have reported that problem in bugzilla #770133 with a proposed solution as well. As you say, a flushing seek is always used and the discont flag is set, but that flag is not used (or checked by the certification tool) but we do not know if anyone is really using the D-bit flag (except for the certification tool). Do you really think that Initial Play and a new Play triggered by a seek operation is a discontinued stream where the D-bit flag shall be set?
Thanks for the clarification. (In reply to Joakim Johansson from comment #5) > > Do you really think that Initial Play and a new Play triggered by a seek > operation is a discontinued stream where the D-bit flag shall be set? *I* think so, yes. It is not a continuation of a previous stream, but something new starts now or there was a "gap" (seek). In both cases you would have to reinit decoders, etc. If that's what the ONVIF spec means, I don't know. I would recommend bringing this up to the ONVIF people and request clarification about this first before we patch in any things that we might have to remove again later. It seems underdefined and weird as is. The certification tool also seems to not properly check that if I understand you correctly. So there's clearly some clarification needed, and fixup of the certification tool probably.
I will write a new patch that only sets the d-bit flag at FLUSH STOP and test that with the compliant tool. However, I noticed that the value is overwritten with the discont flag in the GST_EVENT_CUSTOM_DOWNSTREAM event. And tracing back in the code gives that this discont flag is set in: /libs/gst-plugins-recordingdemuxbin/gst-plugins-recordingdemuxbin/src/gstrecordingdemuxbin.c The discont flag is set to FALSE in these methods when calling gst_recording_demux_bin_create_ntp_offset_event: - gst_recording_demux_bin_perform_seek - gst_recording_demux_bin_setup_source And is set to TRUE in (if a gap is detected): - gst_recording_demux_bin_change_file_unlocked This means that it is not true that we receive a DISCONT flag in all cases where we would like to set the D-bit but as far as I understand can we set the D-bit at Flush Stop and override the discont flag if d-bit is set when the event is received. I will test that, but do you think that this is a solution that you will take?
That would be a bug in your recordingbin plugin though
True, upstream is not owner of gst-plugins-recordingdemuxbin. I will change that code to set DISCONT flag for setup and seek as well. That change probably mean that D-bit doesn't needs to be set to TRUE at FLUSH STOP but considering this discussion, shouldn't we change the default value from FALSE to TRUE? - GST_STATE_CHANGE_READY_TO_PAUSED - GST_EVENT_FLUSH_STOP
Created attachment 335427 [details] [review] Change of default value for D_bit This patch changes the default value of D_bit from FALSE to TRUE in order to always have the D_bit set in the first RTP package. (Initial request, Seek) It is then upto recordingdemuxbin to report discontinuity in the stream with the discont flag. Since the discont flag is also used for setting e_bit, then is there a need for protection to avoid setting e_bit for the first buffer if discont flag is set for Initial request or Seek, this is handled by checking if there is any buffer cached before setting the e_bit.
commit 2837ca997fda8ecc1fee3d1fb1318585e3ad6c38 Author: Joakim Johansson <joakimj@axis.com> Date: Tue Sep 13 11:18:27 2016 +0200 rtponviftimestamp: Change default value of D-bit The default value of D-bit is changed to TRUE so discontinuity is set for initial request and seek request as well. Only set the e_bit flag for the CUSTOM_DOWNSTREAM event if a cached buffer exists. https://bugzilla.gnome.org/show_bug.cgi?id=770221
This commit makes the unit test fail: tpm@xps:~/gst/master/gst-plugins-bad/tests/check$ make elements/rtponviftimestamp.check-norepeat Running suite(s): onviftimestamp 70%: Checks: 10, Failures: 3, Errors: 0 elements/rtponviftimestamp.c:211:F:general:test_apply_clean_point:0: 'memcmp (info_buf.data, info_expected.data, info_buf.size)' (32) is not equal to '0' (0) elements/rtponviftimestamp.c:211:F:general:test_apply_no_e_bit:0: 'memcmp (info_buf.data, info_expected.data, info_buf.size)' (32) is not equal to '0' (0) elements/rtponviftimestamp.c:211:F:general:test_apply_e_bit:0: 'memcmp (info_buf.data, info_expected.data, info_buf.size)' (32) is not equal to '0' (0)
Created attachment 335994 [details] [review] Updated Unit tests I am sorry, there was a late change to also set D-bit to true when GST_STATE_CHANGE_READY_TO_PAUSED that I didn't think would affect the unit testing. But, actually, all the unit tests where using GST_STATE_CHANGE_READY_TO_PAUSED and the test that I had done when setting D-bit to true for GST_EVENT_FLUSH_STOP was not tested by the unit tests. I have run all tests successfully again with updated unit tests where D-bit is always set for first buffer.
commit 25cb3afb58f8bc9c1f7134666f80d8173999a992 Author: Joakim Johansson <joakimj@axis.com> Date: Wed Sep 21 15:09:26 2016 +0200 rtponviftimestamp: Update unit tests after changing default value of D-bit The D bit shall always be set to true for the first buffer. https://bugzilla.gnome.org/show_bug.cgi?id=770221