GNOME Bugzilla – Bug 686003
Grilo-plugin: youtube search show nothing, browsing youtube categories crash totem
Last modified: 2012-10-12 14:57:47 UTC
Using grilo-plugin in 3.6.0 version of totem I found two problem: 1) youtube search gives no result (only an error message) "InvalidParameterValue" for GData; 2) using grilo yuotube browse, clicking on an item to view the movie listed leads to crash.
Created attachment 226300 [details] [review] 0001 - patch Consider following patches to solve the issues
Created attachment 226301 [details] [review] 0002-grilo-plugin-fix-crash-in-youtube-plugin.patch
Created attachment 226302 [details] [review] 0003-grilo-plugin-fix-off-by-one-bug-in-search.patch
Review of attachment 226300 [details] [review]: ::: src/plugins/grilo/totem-grilo.c @@ +200,3 @@ } + Gratuitous line feed. @@ +570,3 @@ + + /* If source does not support resolve() operation, then use the current I don't get this. It should be impossible for the plugin to return a slow key if it doesn't have a resolve function. @@ +660,3 @@ grl_operation_options_set_skip (default_options, (self->priv->search_page - 1) * PAGE_SIZE); grl_operation_options_set_count (default_options, PAGE_SIZE); + grl_operation_options_set_type_filter (default_options, GRL_TYPE_FILTER_VIDEO); That looks like a separate fix. @@ +666,3 @@ self->priv->search_remaining = PAGE_SIZE; + + grl_operation_options_obey_caps (default_options, I don't understand the purpose of this.
Review of attachment 226301 [details] [review]: ::: src/plugins/grilo/totem-grilo.c @@ +585,3 @@ if (g_list_find ((GList *) slow_keys, GINT_TO_POINTER (GRL_METADATA_KEY_URL)) != NULL) { url_keys = grl_metadata_key_list_new (GRL_METADATA_KEY_URL, NULL); + grl_source_resolve (source, media, url_keys, default_resolve_options, resolve_url_cb, self); Why does it crash?
Created attachment 226316 [details] [review] grilo: Search page is 0-indexed Fix possible crash when skipping -PAGE_SIZE items when doing a search. Spotted by Marco Piazza <mpiazza@gmail.com>
Comment on attachment 226302 [details] [review] 0003-grilo-plugin-fix-off-by-one-bug-in-search.patch Good catch, but we really want indexes to start at 0.
Review of attachment 226301 [details] [review]: If you pass default_resolve_option=0 libgrilo crash in grl_operation_options_get_count.
(In reply to comment #8) > Review of attachment 226301 [details] [review]: > > If you pass default_resolve_option=0 libgrilo crash in > grl_operation_options_get_count. Smells like a bug in grilo.
Created attachment 226321 [details] [review] grilo: Make sure to only ever show videos Set the filter type to videos for both search and browse.
Created attachment 226326 [details] [review] grilo: Make sure to only ever show videos Set the filter type to videos for search.
Created attachment 226328 [details] [review] grilo: Add sanity check before resolving URLs Make sure that we only try to resolve a URL when the source supports it, otherwise fallback to the media itself, or a simple warning.
Comment on attachment 226300 [details] [review] 0001 - patch Split up in the 2 patches below.
Created attachment 226331 [details] [review] grilo: Fix resolve() crashing We cannot pass NULL options to grl_source_resolve()
Comment on attachment 226301 [details] [review] 0002-grilo-plugin-fix-crash-in-youtube-plugin.patch Replaced with another version (this one was leaking the "default_resolve_options" object, amongst other things).
Attachment 226316 [details] pushed as 0dc7035 - grilo: Search page is 0-indexed Attachment 226326 [details] pushed as 5f3cf3e - grilo: Make sure to only ever show videos Attachment 226328 [details] pushed as e539cc9 - grilo: Add sanity check before resolving URLs Attachment 226331 [details] pushed as 6383cef - grilo: Fix resolve() crashing
In the following snippet of first patch: @@ -646,33 +653,42 @@ static void search_more (TotemGriloPlugin *self) { GrlOperationOptions *default_options; + GrlOperationOptions *supported_options; default_options = grl_operation_options_new (NULL); grl_operation_options_set_flags (default_options, BROWSE_FLAGS); grl_operation_options_set_skip (default_options, (self->priv->search_page - 1) * PAGE_SIZE); grl_operation_options_set_count (default_options, PAGE_SIZE); + grl_operation_options_set_type_filter (default_options, GRL_TYPE_FILTER_VIDEO); gtk_widget_set_sensitive (self->priv->search_entry, FALSE); self->priv->search_page++; self->priv->search_remaining = PAGE_SIZE; + + grl_operation_options_obey_caps (default_options, + grl_source_get_caps (GRL_SOURCE (self->priv->search_source), GRL_OP_SEARCH), + &supported_options, + NULL); I wanted to check if corrent source, support the options we give (set_count, set_skip, set_filter). I think some grilo-plugins does not support such operations. Then we need to estract only the supported ones (with grl_operation_options_obey_caps).
(In reply to comment #17) > In the following snippet of first patch: <snip> > I wanted to check if corrent source, support the options we give (set_count, > set_skip, set_filter). I think some grilo-plugins does not support such > operations. Which ones? I couldn't find any, so I ignored that hunk. > Then we need to estract only the supported ones (with > grl_operation_options_obey_caps).
(In reply to comment #18) > Which ones? I couldn't find any, so I ignored that hunk. OK. It was only a paranoia check.