Created attachment 295202 [details] [review]
Proposed patch fixing the problem.

Comment on attachment 295202 [details] [review]
Proposed patch fixing the problem.

Thanks for the patch.

While the patch is ok, the problem is that we break API and ABI here because of the changed function parameters.

We would need to keep support for the old functions and add new ones that use microseconds instead of a GTimeVal.

Created attachment 295672 [details] [review]
Proposed patch fixing the problem.

Review of attachment 295672 [details] [review]:

::: gst/rtsp-server/rtsp-session.c
@@ +595,2 @@
  * @session: a #GstRTSPSession
+ * @now: (transfer none): the current monotonic time

This is still an API change. Whatever passed the system time before will now end up in undefined behaviour

As discussed on IRC:

<wtay_> slomo, maybe it's better to add a method to set the clock used and leave the API as it is (maybe with extra convenience method to give time as gint64)

So, you still want wall-clock time support with the built-in possibility for the reported bug?

But, you want me to add support for monotonic time to remove the possibility for the reported bug!

And the proposed solution is:
1. add a method to set the clock used
2. keep the api for wall-clock time
3. add the convenience methods that are introduced in the latest patch for monotonic time

/KII

There would be a method to choose between monotonic and realtime clock (see the enum in GstClock, you can reuse that). And then the method that exists already takes whatever is selected as clock, and additionally there will be some convenience method to work directly with a gint64 instead of GTimeVal.

The default clock-mode would be real-time to make the solution backwards compatible.

Created attachment 296199 [details] [review]
Proposed patch fixing the problem.

Review of attachment 296199 [details] [review]:

Generally looks good, thanks for updating the patch :)

::: gst/rtsp-server/rtsp-session.c
@@ +665,3 @@
  */
 gint
+gst_rtsp_session_next_timeout_usec (GstRTSPSession * session, gint64 * now)

This should probably be a plain gint64 and not a gint64*

@@ +741,3 @@
+ */
+gboolean
+gst_rtsp_session_is_expired_usec (GstRTSPSession * session, gint64 * now)

This probably shouldn't be a gint64* but a plain gint64

Created attachment 296268 [details] [review]
Proposed patch fixing the problem.

gint64 * changed to gint64!!

Review of attachment 296268 [details] [review]:

I don't like that every use of the API now has to check which specific clock is used, like rtsp-session-pool.c. And we still have to default to the realtime clock, so by default every session created by the server still has this bug.

Not sure what to do about this. Wim?

::: gst/rtsp-server/rtsp-session.c
@@ +125,3 @@
 
+  g_object_class_install_property (gobject_class, PROP_CLOCKTYPE,
+      g_param_spec_enum ("clocktype", "clocktype",

clock-type as property name, "Clock Type" for the second parameter

@@ +145,3 @@
   g_mutex_init (&priv->last_access_lock);
   priv->timeout = DEFAULT_TIMEOUT;
+

The clock-type should be initialized here

To make the discussion permanent:

09:34 < sebras> slomo: https://bugzilla.gnome.org/show_bug.cgi?id=743346 would you mind comitting this?
09:35 < sebras> slomo: we haven't seen any input from you for a long time and are anxious to get it on
                master.
09:43 < slomo> sebras: patch looks good, but what does it help really? all sessions created by the
               server will still use the realtime clock by default
09:51 < slomo> sebras: it's all ugly
09:51 < sebras> slomo: ?
09:51 < sebras> slomo: I agree that the patch as it stands now does not set the clocktype.
09:52 < sebras> slomo: we aim to set the clocktype property of GstRTSPSession from inside our derived 
                GstRTSPServer I believe (I'm in the process of checking this).
09:52 < sebras> slomo: are you saying that you want a similar fix for gst-rtsp-server as default?
09:54 < slomo> no, we can't change that default unfortunately
09:54 < slomo> see my comment :)
09:54 < slomo> wtay: https://bugzilla.gnome.org/show_bug.cgi?id=743346 your opinion please :)
09:56 < sebras> slomo: ok, but if we can't change the default then that means that what this patch
                allows you to do is to set the timeout clocktype to monotonic instead of the default 
                realtime clocktype for your own derived GstRTSPServer in anticipation that GStreamer 
                2.0 is released where the default clocktype is changed realtime->monotonic.
09:57 < sebras> slomo: does that make sense? if so then the patch ougth to be appealing right now,
                no..?
09:57 < sebras> slomo: it is backwards compatible but you can enable the correct clocktype and get 
                decent timeouts if you so desire.
09:57 < sebras> slomo: this is the intent at least.
09:59 < slomo> yes
09:59 < slomo> i just think it's still ugly that by default everything is broken :)
09:59 < sebras> slomo: yeah I agree, but that's probably wtay's mistake back in 2012 or something. at 
                least this patch sets us on the right track.
10:00 < slomo> and also that every user of that API now has to check the clock type
10:00 < slomo> it would be nicer if that could be isolated to rtsp-session.c
10:00 < wtay> why even check for realtime?
10:01 < wtay> the new api works with monotonic time and that's it
10:01 < wtay> and the old api with realtime
10:03 < slomo> wtay: how would that look like API wise?
10:03 < sebras> I'm not sure I get it now.
10:03 < sebras> rtsp-session is asked when the next timeout will occur, right?
10:04 < sebras> so it must know what clock to use to determine when that is.
10:04 < sebras> if rtsp-session is supposed to know this own its own, then you must have some way to 
                tell rtsp-session this.
10:05 < wtay> if you use old api, it's the system clock
10:07 < slomo> what would the session pool do? what would happen if you mix old and new API? e.g.
               session pool using old, application using new
10:08 < sebras> slomo: exactly, it only calls gst_rtsp_session_next_timeout() currently.
10:08 < sebras> slomo: after the patch it may call old or new api.
10:11 < sebras> slomo: wtay: I guess the underlaying problem is that it is the caller of
                gst_rtsp_session_next_timeout() that provides the "now" timestamp, it is not collected 
                by rtsp-session on its own.
10:11 < wtay> in _touch() you save both current time and monotonic time, in _next_timeout() you 
              calculate time against system clock, in _next_timeout_usec() you calculate against 
              monotonic clock
10:11 < sebras> wtay: how would rtsp-session-pool know which api to call?
10:12 < wtay> it can call both as long as it gives either a system time or monotonic time
10:12 < wtay> but I would change it to call the new method
10:13 < sebras> wtay: what is the "it" in "change it"?
10:13 < wtay> rtsp-session-pool
10:13 < wtay> make it use the new api so that it uses the monotonic clock
10:13 < sebras> wtay: ok, but then it wouldn't be backwards compatible, right..?
10:14  * sebras is sure he's missing something vital...
10:14 < wtay> you think people expect weird timeouts when the clock is changed?
10:14 < sebras> wtay: well, that is the problem we are attempting to fix.
10:14 < wtay> I think it's a bug
10:14 < sebras> wtay: it is!
10:15 < wtay> so fix it!
10:15 < sebras> wtay: we tried. :)
10:15 < wtay> if you fix the bug, of course the old broken behaviour is no more
10:15 < sebras> wtay: ok, now you and slomo appear to disagree about whether to keep the old 
                behaviour. why is that?
10:16 < slomo> because we're not the same person ;) ok, so what about this then?
10:17 < sebras> slomo: yeah I know, but ideally you guys would agree. it makes it much easier. :)
10:17 < slomo> the GTimeVal API continues with realtime, the new API uses monotonic time. all calls to 
               the old API cause a g_warning() to be printed
10:17 < slomo> rtsp-session-pool uses the new API
10:17 < slomo> and otherwise like wtay just said
10:17 < slomo> and no clock selection API anymore :) which btw was wtay's suggestions some weeks ago ;)
10:18 < sebras> slomo: oh, I though that suggestion came from you. my bad.
10:18 < wtay> I think it's ok to change behaviour here in this case, I can't see how the previous
              behaviour was desirable
10:23 < slomo> sebras: can you update the bug with this new information then?
10:24 < sebras> slomo: I can, but I'm not 100% sure I grasp the full issue yet. we're still discussing
                at our end and trying to understand what you and wtay actually meant. :)
10:26 < slomo> sebras: next_timeout_usec() uses monotonic clock, the GTimeVal variant uses the system
               clock. you store both clock values internally
10:26 < slomo> sebras: and the GTimeVal API should be wrapped in #ifndef GST_DISABLE_DEPRECATED
               block... and probably also print a g_warning() to tell people that this is a bad idea
10:33 < sebras> slomo: and rtsp-session-pool should be using monotonic clock and calling
                next_timeout_usec()?
10:35 < slomo> yes

Created attachment 297097 [details] [review]
Proposed patch fixing the problem.

Hopefully all the agreed fixes in the previous commit are implemented!!

(In reply to Kent Inge Ingesson from comment #14)
> Created attachment 297097 [details] [review] [review]
> Proposed patch fixing the problem.
> 
> Hopefully all the agreed fixes in the previous commit are implemented!!

NOT commit ..... comment of course!!

Thanks for updating the patch :) Looks all good now

commit d2f1997c4b38b9aa0c85a9c4dffdc336cddd747d
Author: Kent-Inge Ingesson <kenti@axis.com>
Date:   Thu Feb 19 10:43:16 2015 +0200

    rtsp-session: Use monotonic time for RTSP session timeout
    
    Changed RTSP session timeout handling to monotonic time
    and deprecating the API for current system time.
    
    This fixes timeouts when the system time changes.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=743346