GNOME Bugzilla – Bug 756628
audioclock: Add run time type check safety
Last modified: 2016-11-01 17:55:07 UTC
I've noted that some methods of GstAudioClock class take GstClock pointer as an argument, but inside this methods GstClock object is casted to GstAudioClock. I'd change type of argument, but it breaks the API - I'm not sure what are the rules about changing the API in GStreamer project. So, for now I'm providing a patch, which checks if passed object is of GstAudioClock type. In the future, I could also create a patch which changes type of the argument, but as I said, I don't know whether I can change the API or not.
Created attachment 313360 [details] [review] proposed solution
Review of attachment 313360 [details] [review]: All these method are static method, in fact, they are the implementation of virtual methods from the base class. Adding run-time type check is nice, but very unlikely in this regard, since the method itself is found through the virtual table of the type attach to the instance. hence cannot really be confused. Specifically, I would avoid adding extra check in time sensitive methods like _get_time() and get_internal_time(). The GOjbect typing system uses many global lock which can quickly create contention.
(In reply to Nicolas Dufresne (stormer) from comment #2) > Review of attachment 313360 [details] [review] [review]: > > All these method are static method, in fact, they are the implementation of > virtual methods from the base class. Are you sure? AFAIK, only gst_audio_clock_get_internal_time is static method, and cannot be used somewhere else, but the others seem to be public for everyone, am I wrong?
All but gst_audio_clock_get_internal_time() are non-static, and for them this check makes sense IMHO.
Created attachment 313817 [details] [review] proposed solution v2 OK, I've improved my patch by removing check in static method gst_audio_clock_get_internal_time(). Btw. could somebody explain me, why do you prefer argument of type GstClock * instead of GstAudioClock *? User probably doesn't know method's implementation of some methods, so if he sees that he can pass GstClock *, he passes GstClock *, which doesn't have to be of type GstAudioClock*. I think, this: gst_audio_clock_get_time (GST_AUDIO_CLOCK (some_clock)) // user knows, that he has to pass GstAudioClock* is better than this: gst_audio_clock_get_time (some_clock) In first case, we have compile-time check of type, in second - runtime. I think, the first one is a way better. gst_audio_clock_get_internal_time() is used as virtual method, so it's obvious, that must have parameter of type GstClock. But the other methods? Please, don't treat my comment as offense. I'm pretty sure, there is a reason of using GstClock instead of GstAudioClock. But I don't see this reason, and want to find it out :)
It's not offensive at all, it's a legitimate question. I agree with you that the first argument of any gst_audio_clock_*() function should be a GstAudioClock * pointer. I don't know why it's a GstClock * pointer here. Maybe for convenience because the function gets assigned to some vfunc elsewhere that takes a GstClock. We should check the rest of the GStreamer code to see how it's used. Perhaps we should fix up the type of the first argument even. I think that'd be ok, it would be both API and ABI compatible in all cases, the 'API breakage' that would be is more of an inconvenience really, I don't think it would affect anything in practice unless they compile with -Werror perhaps.
IMHO (apart from ABI/API compatibility reasons) this should be GstAudioClock instead of GstClock.
Created attachment 325274 [details] [review] proposed solution v3 I've changed method declarations, so they require GstAudioClock* as a first argument. I haven't changed return type of gst_audio_clock_new method, because it probably requires more work. If you agree with me, that return type also should be changed, I could provide another patch for that.
Comment on attachment 325274 [details] [review] proposed solution v3 Good for 2.0 IMHO, thanks :) The return type of gst_audio_clock_new() seems ok as is, this is not really different from e.g. gst_bin_new() returning a GstElement* or various GTK widget constructors returning just a GtkWidget*.
I think we can make that change in 1.x, it doesn't break ABI or API, just causes a compiler warning at worst. And it's not very widely-used API anyway.
Since I don't have write-access to the repository, please push my patch (whenever you want).
Tim, let's get this in then?
commit 89e711663f48809cc7c68b3f3ff831c9e1695a9e Author: Marcin Kolny <marcin.kolny@gmail.com> Date: Thu Oct 15 12:52:27 2015 +0200 audioclock: use GstAudioClock* as first argument in GstAudioClock methods All the GstAudioClock method declarations required object of GstClock type as a first argument, but in fact, required GstAudioClock object (runtime check in function body). Instead of checking type in run-time, we can change functions declaration, to accept only GstAudioClock methods. Then, runtime check is not necessary anymore, since always GstAudioClock object is passed to a function. https://bugzilla.gnome.org/show_bug.cgi?id=756628