GNOME Bugzilla – Bug 749417
rtsp-client: add API to allow application to decide what requirements are supported
Last modified: 2015-06-23 14:19:32 UTC
This is one of TODO list in gst-rtsp-server. In addition, in my project(miracast), application has to check requirement in OPTION request. So far, I commented check_request_requirements call unfortunately.
Created attachment 303407 [details] rtsp-client: allow application to decide what requirements are supported signal SIGNAL_CHECK_REQUIREMENTS is added to allow application to check requirements
Created attachment 303408 [details] [review] rtsp-client: allow application to decide what requirements are supported signal SIGNAL_CHECK_REQUIREMENTS is added to allow application to check requirements
There's something changed by gst-indent, which is not related to this issue.
Comment on attachment 303408 [details] [review] rtsp-client: allow application to decide what requirements are supported >+ gst_rtsp_client_signals[SIGNAL_CHECK_REQUIREMENTS] = >+ g_signal_new ("check-requirements", G_TYPE_FROM_CLASS (klass), >+ G_SIGNAL_RUN_LAST, G_STRUCT_OFFSET (GstRTSPClientClass, >+ check_requirements), NULL, NULL, g_cclosure_marshal_generic, >+ G_TYPE_BOOLEAN, 1, G_TYPE_POINTER); G_TYPE_POINTER is not something that should be used in public API. It won't work with bindings, nor does anyone know what the type of the argument is. >--- a/gst/rtsp-server/rtsp-client.h >+++ b/gst/rtsp-server/rtsp-client.h >@@ -126,6 +126,7 @@ struct _GstRTSPClientClass { > > void (*announce_request) (GstRTSPClient *client, GstRTSPContext *ctx); > void (*record_request) (GstRTSPClient *client, GstRTSPContext *ctx); >+ void (*check_requirements) (GstRTSPClient *client, GstRTSPContext *ctx); > > /*< private >*/ > gpointer _gst_reserved[GST_PADDING_LARGE-5]; If you add a vfunc you need to remove one pointer from the reserved array, i.e. change this to GST_PADDING_LARGE-6.
Created attachment 303499 [details] [review] rtsp-client: allow application to decide what requirements are supported - signal SIGNAL_CHECK_REQUIREMENTS is added to allow application to check requirements. - Following tim's review.
This is likely missing a gtk-doc block for the new signal with a Since: 1.6 mark.
Created attachment 305106 [details] [review] rtsp-client: allow application to decide what requirements are supported Add descriptions for new signal and rebased patch
Created attachment 305107 [details] [review] rtsp-client: allow application to decide what requirements are supported Add descriptions for new signal and rebased patch
Comment on attachment 305107 [details] [review] rtsp-client: allow application to decide what requirements are supported >- /* for now we don't commit to supporting anything, so will just report >- * all of the required options as unsupported */ >- *unsupported_reqs = g_strjoinv (", ", (gchar **) arr->pdata); >+ g_signal_emit (ctx->client, >+ gst_rtsp_client_signals[SIGNAL_CHECK_REQUIREMENTS], 0, arr, >+ &check_result); >+ >+ if (!check_result) { >+ /* for now we don't commit to supporting anything, so will just report >+ * all of the required options as unsupported */ >+ *unsupported_reqs = g_strjoinv (", ", (gchar **) arr->pdata); >+ } This doesn't really seem right or complete to me: the application may support some requirements but not others. The server is supposed to return which requirements are unsupported, but it can't because it doesn't know. In the header file Class structure you have to reduce the padding by 1. I'd rather add this functionality in response to someone actually using it to make sure it works properly..
> This doesn't really seem right or complete to me: the application may > support some requirements but not others. The server is supposed to return > which requirements are unsupported, but it can't because it doesn't know. It's obviously better than current patch. I start to think how to implement to support your idea. > In the header file Class structure you have to reduce the padding by 1. Oops, it's my mistake. I'm going to attach new patch fixing this. > I'd rather add this functionality in response to someone actually using it > to make sure it works properly.. That'll be good :-)
*** Bug 751049 has been marked as a duplicate of this bug. ***
How about adding the GstRTSPContext to the signal as it is done for all other client signals? My application will be validating options based on url, the request may also be of interest.
IMHO, adding RTSPContext or Request msg to the signal could be a good option. In your case, it should be included. And I'm wondering how to get supported reqs and unsupported reqs from client to send exact unsupported requirements in response.
Instead of a boolean, make it return either a gchar ** (NULL terminated string array) with the unsupported options or a gchar * (comma-separated list) of unsupported options. Or supported options. One of those.
Created attachment 305542 [details] [review] rtsp-client: allow application to decide what requirements are supported What do you thin about this one? I slightly changed Hyunjun's patch.
Created attachment 305556 [details] [review] rtsp-client: allow application to decide what requirements are supported removed auto indented code in original patch, also adjusted the doc line for check_request_requirement()
Comment on attachment 305556 [details] [review] rtsp-client: allow application to decide what requirements are supported I'd prefer a G_TYPE_STRV for the last signal argument here. I think the distinction between a NULL return value and an empty string return is quite subtle, and you should make that explicit in the documentation what it should return if all options are supported. I also wonder if we need an accumulator for the signal.
(In reply to Tim-Philipp Müller from comment #17) > Comment on attachment 305556 [details] [review] [review] > rtsp-client: allow application to decide what requirements are supported > > I'd prefer a G_TYPE_STRV for the last signal argument here. > > I think the distinction between a NULL return value and an empty string > return is quite subtle, and you should make that explicit in the > documentation what it should return if all options are supported. ouch, i have really missed that, will fix it > > I also wonder if we need an accumulator for the signal. in case different modules in the application handle different features?
> > I also wonder if we need an accumulator for the signal. > > in case different modules in the application handle different features? I guess it can only happen on programmer/application-developer error/oversight? Probably not so important then.
Created attachment 305904 [details] [review] rtsp-client: allow application to decide what requirements are supported fixed review comments: - G_TYPE_STRV for the last signal argument - updated doc for the signal
Comment on attachment 305904 [details] [review] rtsp-client: allow application to decide what requirements are supported Looks good to me, just two more small niggles: >+ /** >+ * GstRTSPClient::check-requirements: >+ * @auth: a #GstRTSPClient >+ * @context: a #GstRTSPContext >+ * @arr: a #GPtrArray of lines of Require in RTSP header -> NULL-terminated array of strings >@@ -126,9 +126,10 @@ struct _GstRTSPClientClass { > > void (*announce_request) (GstRTSPClient *client, GstRTSPContext *ctx); > void (*record_request) (GstRTSPClient *client, GstRTSPContext *ctx); >+ gchar* (*check_requirements) (GstRTSPClient *client, GstRTSPContext *ctx, GPtrArray * options); Doesn't the last arg here need updating too?
Created attachment 305908 [details] [review] rtsp-client: allow application to decide what requirements are supported opps, sorry :( now fixed
This patch would be good. I think I'm going to use right after this patch is pushed. :-)
Created attachment 305919 [details] [review] rtsp-client: allow application to decide what requirements are supported Changed committer and added "Based on patch from Hyunjun Ko <zzoon.ko@samsung.com>" to the commit message
Thank you both, pushed to master: commit 8922afb88dafa7b724a92796b2916161bf5375fc Author: Ognyan Tonchev <ognyan@axis.com> Date: Thu Jun 18 13:12:04 2015 +0200 rtsp-client: allow application to decide what requirements are supported Add "check-requirements" signal and vfunc to allow application (and subclasses) to check the requirements. Based on patch from Hyunjun Ko <zzoon.ko@samsung.com> https://bugzilla.gnome.org/show_bug.cgi?id=749417