After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 557470 - YouTube problems with multiple simultaneous searches
YouTube problems with multiple simultaneous searches
Status: RESOLVED FIXED
Product: totem
Classification: Core
Component: Browser plugin (obsolete)
2.24.x
Other All
: Normal critical
: ---
Assigned To: General Totem maintainer(s)
General Totem maintainer(s)
: 558824 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2008-10-22 18:21 UTC by Matthew Adams
Modified: 2008-11-02 10:02 UTC
See Also:
GNOME target: ---
GNOME version: 2.23/2.24


Attachments
patch -p1 < youtubefixup.patch // it patches /usr/lib/totem/plugins/youtube/youtube.py (3.96 KB, patch)
2008-10-22 18:23 UTC, Matthew Adams
rejected Details | Review
Latest and greatest patch (6.14 KB, patch)
2008-10-22 22:23 UTC, Matthew Adams
needs-work Details | Review
Latest patch - still needs work (5.78 KB, patch)
2008-10-24 12:40 UTC, Matthew Adams
none Details | Review
multi search patch (6.22 KB, patch)
2008-10-24 17:02 UTC, Matthew Adams
none Details | Review
multi search patch + HUGE search speedup (6.96 KB, patch)
2008-10-24 17:32 UTC, Matthew Adams
none Details | Review
Fix behaviour with multiple searches, and sort out some threading issues (11.77 KB, patch)
2008-10-24 23:30 UTC, Philip Withnall
committed Details | Review

Description Matthew Adams 2008-10-22 18:21:59 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?
Comment 1 Matthew Adams 2008-10-22 18:23:11 UTC
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
Comment 2 Bastien Nocera 2008-10-22 18:32:38 UTC
Which version of Totem are you using?

Make sure you use at least 2.24.2 to get the Python threading fixes.
Comment 3 Matthew Adams 2008-10-22 18:36:28 UTC
I am using 2.24.2 from Ubuntu Intrepid
Comment 4 Philip Withnall 2008-10-22 18:53:25 UTC
(A little more information on http://ubuntuforums.org/showthread.php?p=6013411.)
Comment 5 Christian Persch 2008-10-22 19:32:38 UTC
(Reassigning to the right component. This has nothing to do with the browser plugin.)
Comment 6 Bastien Nocera 2008-10-22 19:41:17 UTC
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...
Comment 7 Philip Withnall 2008-10-22 19:41:55 UTC
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.
Comment 8 Matthew Adams 2008-10-22 22:21:38 UTC
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)
Comment 9 Matthew Adams 2008-10-22 22:23:51 UTC
Created attachment 121163 [details] [review]
Latest and greatest patch

patch -p1 < youtubefixup2.patch
Comment 10 Matthew Adams 2008-10-23 00:30:43 UTC
sorry I don't know how i switched it back to blocker, should be critical
Comment 11 Rene Dohan 2008-10-23 07:51:08 UTC
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
Comment 12 Matthew Adams 2008-10-23 08:05:34 UTC
(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..
Comment 13 Philip Withnall 2008-10-24 06:00:29 UTC
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?
Comment 14 Matthew Adams 2008-10-24 12:40:18 UTC
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
Comment 15 Matthew Adams 2008-10-24 17:02:05 UTC
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.
Comment 16 Matthew Adams 2008-10-24 17:32:28 UTC
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.
Comment 17 Matthew Adams 2008-10-24 17:37:06 UTC
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? 
Comment 18 Philip Withnall 2008-10-24 23:30:42 UTC
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.
Comment 19 Bastien Nocera 2008-10-25 00:32:50 UTC
I'd rather have had the new lines and spacing changes done separately, but this looks good.
Comment 20 Philip Withnall 2008-10-25 14:03:02 UTC
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)
Comment 21 solsTiCe d'Hiver 2008-11-01 17:44:12 UTC
*** Bug 558824 has been marked as a duplicate of this bug. ***
Comment 22 solsTiCe d'Hiver 2008-11-01 19:43:41 UTC
is it really fixed? it is so slow in totem 2.24.3. and i don't think i have network problem , am i ?
Comment 23 Philip Withnall 2008-11-02 10:02:28 UTC
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.