GNOME Bugzilla – Bug 758062
rtsp-client: emit new rtsp request signals in the beginning of each request
Last modified: 2016-11-01 18:26:05 UTC
It would be good to have additional signals/virtual methods for each client RTSP request similar to the ones already present but emitted in the beginning of each request. This way the application may validate the requests, parse custom headers, configure the media/stream and also generate error status code in case of error or bad request
Created attachment 315411 [details] [review] client: emit signal in the beginning of rtsp requests The attached patch adds client request signals to the SETUP and PLAY request, signals are emitted as soon as the client has obtained the media/the stream. Such signals should possibly be added to all RTSP requests but I would first like to know your opinion about the way they are implemented. Or should they be virtual methods instead?
For consistency we should do that for all message types. Also, why are you doing the complicated g_signal_emitv() stuff with the GValues? The varargs g_signal_emit() does the same and is a bit less verbose :)
The reason is the return value, g_signal_emit() sets it to a default value when no signal handler is attached unlike the v version which does not touch the return value in such cases.
Created attachment 315421 [details] [review] client: emit signal in the beginning of each rtsp request Here comes an updated patch, but again, what do you think about the emitv() version of the function? Shall i change to g_signal_emit()? But how about the return value then if there is no signal handler installed? :)
Comment on attachment 315421 [details] [review] client: emit signal in the beginning of each rtsp request Don't like the naming so much, but don't have a better suggestion either right now. Isn't there an enum GType for the RTSP status returns? If yes, that should probably be used instead of G_TYPE_UINT. If not, perhaps it's time to add it, unless something prevents that?
Created attachment 315665 [details] [review] client: emit signal in the beginning of each rtsp request Now the patch is using enum GType for the RTSP status returns. I initially thought that these enum GType's were only there for win32 since 'git grep g_enum_register_static' never showed 'gst-libs/gst/rtsp/gstrtsp-enumtypes.c', no idea why :)
> I initially thought that these enum GType's were only there for win32 since > 'git grep g_enum_register_static' never showed > 'gst-libs/gst/rtsp/gstrtsp-enumtypes.c', no idea why :) Because gst-libs/gst/rtsp/gstrtsp-enumtypes.c is not checked into git but generated at build time using glib-mkenums.
Created attachment 315745 [details] [review] client: emit signal in the beginning of each rtsp request Patch has been updated, now even function declarations for the signal use correct return type, rtsp-client.c now also includes gst/rtsp/gstrtsp-enumtypes.h
Created attachment 334065 [details] [review] client: emit signal in the beginning of each rtsp request Here is a new version. Now there are default handlers for all the pre-signals, which sub classes can be override. Added an accumulator function for the signals as well. Also moved the virtual pre-signal methods towards the end of the class definition, and fixed the padding.
Review of attachment 334065 [details] [review]: Seems good to me now. Should we get this in for 1.10? Seems relatively safe but we're in feature freeze now. Opinions? I would defer it to 1.12. ::: gst/rtsp-server/rtsp-client.h @@ +142,2 @@ /*< private >*/ + gpointer _gst_reserved[GST_PADDING_LARGE-16]; GST_PADDING_LARGE is 20... 4 more to go
As eager as we are to get rid of local patches, I understand your concern, and I guess we can wait until after 1.10. And yes, we're getting low on padding ;-)
Attachment 334065 [details] pushed as 8425ea6 - client: emit signal in the beginning of each rtsp request