GNOME Bugzilla – Bug 609333
Support for Rai.tv
Last modified: 2012-12-16 15:52:01 UTC
hello to everybody, i was just wondering that i'd be very useful if you could provide support for italian broadcaster rai (www.rai.tv) in the same way you support bbc. rai has both live (indluding hd channels) and non-live streams watched daily by hundreds thousand users. thank you, vince
Moving to grilo, as that's where the support should be added.
Created attachment 227846 [details] [review] [PATCH] -Add plugin to search/browse Rai.tv Consider the attached new plugin to search/browse from www.rai.tv. This is from a totally newbie in gnome developing: feel free to change and correct it. I tried to mimick raismth firefox plugin in the resolve operation.
Review of attachment 227846 [details] [review]: I suggest to group functions implementing the Grilo API (_search(), _browse(), _supported_keys(), ...) all together, and put the remaining private helping functions also all together. This will make easier to maintain the code. ::: src/raitv/Makefile.am @@ +6,3 @@ +# Copyright (C) 2011 Intel Corporation. +# Copyright (C) 2011 Igalia S.L. + Bein ::: src/raitv/grl-raitv.c @@ +101,3 @@ + const gchar *container_id; + guint count; + guint lenght; I guess it is a typo :) @@ +170,3 @@ + +static void raitv_setup_search_mapping (void); +static void raitv_setup_browse_mapping (void); I think these mappings would fit better in the private structure. So you can initialize them in the source initialization, and free them in the source finalization @@ +270,3 @@ + source_class->resolve = grl_raitv_source_resolve; + + g_type_class_add_private (klass, sizeof (GrlRaitvSourcePrivate)); You are adding the private twice @@ +277,3 @@ +{ + self->priv = RAITV_SOURCE_PRIVATE (self); + self->priv->async_session = soup_session_async_new (); Are you using this session? @@ +544,3 @@ + + if (!doc) { + GRL_DEBUG ("Documento non valido"); Let's use English for the debug messages :) @@ +1094,3 @@ + gchar *start; + + op->proxy = rest_proxy_new ("http://www.ricerca.rai.it/search", FALSE); I suggest to define the server as a #define, so it is easier to update if server changes @@ +1142,3 @@ + return; + + if ( grl_media_get_url (rs->media) != NULL) { Not sure if you are understanding the goal of resolve() operation... resolve() is used when you have a GrlMedia with some keys, and you want to request more keys from it. Thus, you could have a GrlMedia already obtained from GrlRaitv, with only its ID and TITLE, and invoke resolve() to request the value of URL, THUMBNAIL, or any other key supported by the plugin. Thus, you can't check straightforwardly if the media has URL or not; in fact, the keys in the spec are not in the media. Rather, you should go through all the keys in rs and try to get them from the rai.tv service. @@ +1219,3 @@ + caps = grl_caps_new (); + + return caps; As you are not really defining any specific capability for this source, I suggest to get rid of grl_raitv_source_get_caps(). The default implementation already does what you wrote. ::: src/raitv/grl-raitv.h @@ +5,3 @@ + * + * Authors: Lionel Landwerlin <lionel.g.landwerlin@linux.intel.com> + * I think you can update this header and put yourself as the author/copyright
I've notice that sometimes, the thumbnail value is incomplete: it lacks the "http://www.rai.tv" first part
I've also noticed that mime-type is always "Video". This doesn't provide any information, as we already know the content is a Video (you're returning a GrlMediaVideo). If you can't set a real mime-type, I suggest to remove that key from the supported ones.
I see you are not using GrlNet to perform the network invokations. I really suggest to use it (you could see it is very simple to use, and you wouldn't need to do many changes in the code). It is based on libsoup, also, and provides several interesting features, like mocking the network results for testing support, caching in the libsoup side, and so on.
While testing it, sometimes I got errors in the console about problems in the XML parsing. This usually happens when invoking the resolve() function.
> @@ +1142,3 @@ > + return; > + > + if ( grl_media_get_url (rs->media) != NULL) { > > Not sure if you are understanding the goal of resolve() operation... > > resolve() is used when you have a GrlMedia with some keys, and you want to > request more keys from it. Thus, you could have a GrlMedia already obtained > from GrlRaitv, with only its ID and TITLE, and invoke resolve() to request the > value of URL, THUMBNAIL, or any other key supported by the plugin. > > Thus, you can't check straightforwardly if the media has URL or not; in fact, > the keys in the spec are not in the media. Rather, you should go through all > the keys in rs and try to get them from the rai.tv service. > I understand the goal of resolve(), but in raitv case the search() operation returns immediately an url for the media, while the browse() operation does not return an url. So I implement resolve() that "has a meaning" only for media coming from browse operation: when the media is coming from the search() op, it already has an url.
(In reply to comment #7) > While testing it, sometimes I got errors in the console about problems in the > XML parsing. This usually happens when invoking the resolve() function. I know: that is due to the fact that resolve operation tries to parse an html page like it was an xml (this is why I used the xmlparsememory and not xmlreadmemory what will always return false). And noramlly html pages are far from "well-formed". I think this is the simplest way to achieve the goal (a dom inspector would be too expensive just for reading a tag in a html page).
(In reply to comment #8) > I understand the goal of resolve(), but in raitv case the search() operation > returns immediately an url for the media, while the browse() operation does not > return an url. > So I implement resolve() that "has a meaning" only for media coming from browse > operation: when the media is coming from the search() op, it already has an > url. And what about the other supported keys? Can you guarantee they are always present? Because if not, users could invoke resolve() with those keys. Also, if browse() doesn't return inmediately an URL, then you should declare URL as slow key, and works accordingly. That is, if user uses FAST_ONLY flag when browsing, you don't return URL; if not, then you perform the additional query to get the URL. For the search() case, you can always return URL if you want.
(In reply to comment #9) > I know: that is due to the fact that resolve operation tries to parse an html > page like it was an xml (this is why I used the xmlparsememory and not > xmlreadmemory what will always return false). > And noramlly html pages are far from "well-formed". > I think this is the simplest way to achieve the goal (a dom inspector would be > too expensive just for reading a tag in a html page). Well, if that works and you don't get wrong information, it is fine for me.
Created attachment 230497 [details] [review] [PATCH V2] - Add Raitv plugin I tried to address all the comments receiceved. Raitv plugins V2 changelog: - Typos and copyright fixes; - Port to GrlNet; - Small fixes;
Thanks for the changes! I've added some small fixes too. commit 74284713b91cb1c0307a4bc3cdd71926b9ac5569 Author: Marco Piazza <mpiazza@gmail.com> Date: Sun Dec 2 23:56:43 2012 +0100 Raitv: Add Rai.tv plugin Retrieves movies information from Rai website (www.rai.tv). RAI, or Radio Televisione Italiana, founded in 1954 is the Italian state owned public service broadcaster. https://bugzilla.gnome.org/show_bug.cgi?id=609333 Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com> configure.ac | 44 +++ src/Makefile.am | 4 + src/raitv/Makefile.am | 34 ++ src/raitv/grl-raitv.c | 1264 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/raitv/grl-raitv.h | 76 ++++ src/raitv/grl-raitv.xml | 10 + 6 files changed, 1432 insertions(+)
Created attachment 231635 [details] [review] Fix mismatching in tags request for most popular videos Thanks for commiting the work. Just tried half an hour ago e find the first mistake (my bad..) Please consider the patch attached to fix a mismatch in browsing "most popular videos", requesting wrong tags.
commit 930eeb162010c01e8368823952356123e76c756d Author: Marco Piazza <mpiazza@gmail.com> Date: Sat Dec 15 23:49:06 2012 +0100 raitv: Fix browsing for most popular videos Signed-off-by: Marco Piazza <mpiazza@gmail.com> https://bugzilla.gnome.org/show_bug.cgi?id=609333 src/raitv/grl-raitv.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) The following fixes have been pushed: