GNOME Bugzilla – Bug 557470
YouTube problems with multiple simultaneous searches
Last modified: 2008-11-02 10:02:28 UTC
Please describe the problem: In short, as it stands the youtube plugin can create multiple threads on the same object at the same time. For me it locks up totem, and a few times it locked up xorg, when searching for videos. Steps to reproduce: 1. load totem, start youtube plugin 2. search for video 3. search for different video If not frozen yet go to step 2 Actual results: The gui stops redrawing, although it all appears to work in the background (I click on the white area the thumbnails are supposed to show up and it plays the video i searched for) Expected results: videos to show up on the list Does this happen every time? nearly, not every, it's race conditions and or memory problems Other information: I created a patch which solves the problems for me. Where can I attach it?
Created attachment 121153 [details] [review] patch -p1 < youtubefixup.patch // it patches /usr/lib/totem/plugins/youtube/youtube.py patch -p1 < youtubefixup.patch it patches /usr/lib/totem/plugins/youtube/youtube.py
Which version of Totem are you using? Make sure you use at least 2.24.2 to get the Python threading fixes.
I am using 2.24.2 from Ubuntu Intrepid
(A little more information on http://ubuntuforums.org/showthread.php?p=6013411.)
(Reassigning to the right component. This has nothing to do with the browser plugin.)
If it only hangs because of a dead lock, then we'd need to have a python backtrace of the hang, or see whether it fixes itself up after a while (when things have finished downloading). If the X server hangs, then it's not really our problem, X should be fixed...
This approach is wrong. As you said in your e-mail, what needs to be done is for a previous search to be cancelled in favour of the new one…_unless_ the second search is simply fetching more results, in which case we just cancel that search. I'd welcome a patch to do this; if not, I'll see if I can give it a go sometime soon. I wouldn't say this is a blocking bug either — out of all the users of the YouTube plugin, you're (unfortunately) the only one I've heard of who's had problems like this.
Took me a while, since threads can't explicitly be killed in python because it might leave the program in a bad state. But here it is, it cancels the search and starts a new one. Applies the same way as the old patch ( patch -p1 < youtubefixup2.patch ) differences from the original patch: adds 3 new counter variables to count the active threads and idle_adds to prevent data mixups, gets rid of the progress bar assertions when doing a search in the middle of a search by clearing the entry's, and one new function added to an idle_add to re-call get_results when it's in a safe state to do so adds last_url string to compare to current url to eliminate redundant calls to get_data (in case the user hits enter twice or the search button twice) other differences from original plugin: drop calls to get_data if get_data is already running on that request (happens when scrolling durring "Fetch more videos") clear self.entry to eliminate (totem:4111): Gtk-CRITICAL **: gtk_progress_set_percentage: assertion `percentage >= 0 && percentage <= 1.0' failed""" when stopping a search in the middle and starting a new one A couple more print statements at error (like 503) and two debug print statements for search_count A couple visual changes - print done in progress bar when it's done and keep it filled up instead of erasing the text and setting the progress to 0.0 (just personal taste I don't know)
Created attachment 121163 [details] [review] Latest and greatest patch patch -p1 < youtubefixup2.patch
sorry I don't know how i switched it back to blocker, should be critical
I have also similar problems with youtube plugin , it stops on fetching search results . Its not working at all for me . I am using archlinux and version is 2.24.2
(In reply to comment #11) > I have also similar problems with youtube plugin , it stops on fetching search > results . Its not working at all for me . I am using archlinux and version is > 2.24.2 > You can try the latest patch, so far it's been working 100% for me, when I started it completely failed, well, not completely, just about 99%. get the latest and greatest patch ;), open it in a text editor before applying, change the filename at the top to point to /usr/lib/totem/plugins/youtube.py and apply it with patch -p1 < patchname.patch the latest has the required cancel download in favor of new search, the only real problem I have now is that it appears to first download the related video list before playing which can take some time, don't know if this was my fault, I made it wait a lot to complete tasks in order to avoid race conditions and bad program states so it very well could be..
I haven't been able to test this properly, as it appears YouTube have changed their URL format, rendering the plugin broken. I'll fix that, then give this a more thorough test. In the mean time, here are some issues simply by looking at the patch: - self.progress_bar.set_text (_("Fetching more videos...")) + self.progress_bar.set_text ("Fetching more videos...") Please don't remove localisation functions. + self.progress_bar.set_text ("Generating Thumbnails") If this is to be committed to the 2.24.x branch, we can't add any new UI strings since we're both UI and string frozen for eternity. (Regardless, I don't think we would want this string anyway.) - self.progress_bar.set_fraction (0.0) - self.progress_bar.set_text ("") + self.progress_bar.set_fraction (1.0) + self.progress_bar.set_text ("Done") Likewise, this can't be changed (and shouldn't anyway). + self.search_count = self.search_count - 1; Rogue semicolon‽ - self.progress_bar.set_text (_("Fetching search results...")) + self.progress_bar.set_text ("Fetching search results...") Removing another localisation function. + def wait_on_threads(self, url, treeview_name, clear): + """Make sure no threads are downloading, there's only 1 search running at a time, and wait for any images being appended to finish before moving on""" + if self.download_threads != 0 or self.search_count != 0 or self.callback_threads != 0 or self.appendings_pending != 0: + return True + self.get_results(url, treeview_name, clear) + return False Can we have spaces before the brackets in function calls and definitions please? There are minor coding style problems like this throughout the patch. + """get rid of (totem:4111): Gtk-CRITICAL **: gtk_progress_set_percentage: assertion `percentage >= 0 && percentage <= 1.0' failed""" + if (self.entry[treeview_name]) != None: + if len(self.entry[treeview_name]) != 0: + self.entry[treeview_name] = [] + self.entry[treeview_name] = None This doesn't look right to me. Can't we just clamp the progress value when we set it?
Created attachment 121266 [details] [review] Latest patch - still needs work I went over and fixed the things you requested. After reworking it, the Generating Thumbnails text is necessary - not with that exact text itself but the call to progress_bar.set_text() at least. When canceling a search for a new one without it there it left the progress bar blank because process_next_thumbnail was forced to quit after the text was set by on_search_button_click - but not only that I think that's where all my problems were originally coming from - because I was getting the gui lockup symptoms again. May indicate a bug in gtk? when moving a progress bar that has no text inside it (It pulses fine without text but doesn't seem to like set_fraction..). As soon as I put text in there (I replaced it with (_("Fetching search results...")) the gui "lockups" stopped again. Can we just add a string called progress_text to the youtube object and set that to the strings? How would we do localazation with that, just one call to self.progress_bar.set_text(_(progress_text)) or do the strings need to be something special when defined? and the last self.entry = Non thing basically just forces process_next_thumbnail function to return False, set the progress text to (blank again) and reduce the counter so get_results can be called again - so that it's not still running when canceling a search in favor of a new one, and it does get rid of that Gtk-CRTICAL from the console. You were right though, it was sloppy and twice as large as needed, I shortened it to just if (self.entry[treeview_name]) != None: self.entry[treeview_name] = None this is the first time I've messed with python, sorry about the sloppyness and syntax errors I'm trying my best and still learning. My heavy c/c++ background explains the rogue semicolon, used to typing those in ^^ I've been testing it with the mrl patch on bug 557681. Gtk versions: ii libgtk1.2 1.2.10-18.1build2 The GIMP Toolkit set of widgets for X ii libgtk1.2-common 1.2.10-18.1build2 Common files for the GTK+ library ii libgtk2-gladexml-perl 1.006-1build1 Perl interface to use user interfaces create ii libgtk2-perl 1:1.183-1 Perl interface to the 2.x series of the Gimp ii libgtk2.0-0 2.14.4-0ubuntu1 The GTK+ graphical user interface library ii libgtk2.0-bin 2.14.4-0ubuntu1 The programs for the GTK+ graphical user int ii libgtk2.0-cil 2.12.1-1ubuntu2 CLI binding for the GTK+ toolkit 2.12 ii libgtk2.0-common 2.14.4-0ubuntu1 Common files for the GTK+ graphical user int ii libgtk2.0-dev 2.14.4-0ubuntu1 Development files for the GTK+ library
Created attachment 121291 [details] [review] multi search patch patches against svn version, notes: changed to location = self.resolve_t_param(youtube_id) to remove extra connection to youtube changed readline() to read(), since the entire file is already in memory at that point I figured less iterations would be better for performance? Removed some prints I had in other patches There still seems to be a problem when running a related search at the same time as it's downloading more videos, but it doesn't lock up the gui now.
Created attachment 121296 [details] [review] multi search patch + HUGE search speedup sorry for all the patches :P, but in this version I moved the actual getting of the mrl to when the video is played, which GREATLY increases the speed at which the video lists are downloaded at, as it doesn't have to go downloading each video's entire webpage just to get the mrl even if you don't play it.
lame, last patch breaks right click/add to playlist by moving the location where the mrl is set, how can I make it get the mrl when doing right click/add to playlist as well? Is there an irc I can ask questions like this in about this?
Created attachment 121315 [details] [review] Fix behaviour with multiple searches, and sort out some threading issues Here's a rewrite of Matthew's patch which sorts out all the issues with multiple simultaneous searches, and also stops GTK+ functions being called in a thread, punting them back to the main thread for execution. This solves Matthew's problems. As far as behaviour with multiple simultaneous searches goes, I've implemented what I described in comment #7, with the addition of some extra logic to stop simultaneous searches from both the results and related videos tabs overwriting each others' statuses in the progress bar. The search mutex (if I can call it that) is held per-tab. The patch has had good testing from me, and Matthew's said he'll test it thoroughly too and comment here if he finds any problems.
I'd rather have had the new lines and spacing changes done separately, but this looks good.
Committed to trunk and gnome-2-24. 2008-10-25 Philip Withnall <philip@tecnocode.co.uk> * src/plugins/youtube/youtube.py: Fix YouTube plugin behaviour when attempting to perform multiple searches simultaneously, and fix some threading problems where GTK+ functions were being called from a spawned thread. (Closes: #557470)
*** Bug 558824 has been marked as a duplicate of this bug. ***
is it really fixed? it is so slow in totem 2.24.3. and i don't think i have network problem , am i ?
Unfortunately, Google changed YouTube to make it harder to retrieve the FLV files to play in something like Totem. We now have to make an extra HTTP request for each video, which slows everything round. Bug #557824 is about postponing as much of the loading process until it's actually needed, but we haven't been able to make much progress in that area.