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