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 576666 - YouTube Extension
YouTube Extension
Status: RESOLVED FIXED
Product: banshee
Classification: Other
Component: Other Extensions
git master
Other All
: Normal enhancement
: 1.x
Assigned To: Banshee Maintainers
Banshee Maintainers
Depends on:
Blocks: 568585
 
 
Reported: 2009-03-25 07:45 UTC by Kevin Duffus
Modified: 2010-03-12 08:45 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Screenshot of YouTube source after searching for "firefox" (339.41 KB, image/png)
2009-03-25 07:50 UTC, Kevin Duffus
  Details
YouTube extension source (124.06 KB, application/x-compressed-tar)
2009-03-27 05:10 UTC, Kevin Duffus
  Details
YouTube page in the content pane (345.79 KB, image/png)
2009-05-23 04:36 UTC, Kevin Duffus
  Details
Patch to add youtube page to the content pane (36.45 KB, patch)
2009-05-23 04:37 UTC, Kevin Duffus
needs-work Details | Review
Apply second patch to show message when no videos found (3.09 KB, patch)
2009-05-23 17:49 UTC, Kevin Duffus
needs-work Details | Review
This patch should cover all the requested changes (37.89 KB, patch)
2009-05-29 08:17 UTC, Kevin Duffus
none Details | Review
Visual glitch on the Darklooks theme (150.06 KB, image/png)
2009-06-03 07:45 UTC, Alexander Kojevnikov
  Details
Fix glitch in dark themes and use Banshee.IO instead of System.IO (38.08 KB, patch)
2009-06-03 18:55 UTC, Kevin Duffus
none Details | Review
This one should apply cleanly (37.63 KB, patch)
2009-06-04 08:27 UTC, Kevin Duffus
needs-work Details | Review
Patch cleaned up with respect to Gabriel's input (47.41 KB, patch)
2009-11-20 20:57 UTC, Kevin Duffus
none Details | Review
Screenshot of YouTube video playing back in Banshee (605.25 KB, image/png)
2009-11-20 21:06 UTC, Kevin Duffus
  Details
Updated patch (41.36 KB, patch)
2009-11-21 04:51 UTC, Kevin Duffus
needs-work Details | Review
Updated patch from last review (44.69 KB, patch)
2010-01-14 05:26 UTC, Kevin Duffus
needs-work Details | Review
Latest patch (44.42 KB, patch)
2010-02-03 04:12 UTC, Kevin Duffus
none Details | Review
Updated to git master (42.85 KB, patch)
2010-03-04 11:23 UTC, Alexander Kojevnikov
committed Details | Review

Description Kevin Duffus 2009-03-25 07:45:54 UTC
It would be nice to have a plugin so the user can search and playback YouTube videos from within Banshee. I've started with a rather crude plugin using Google's YouTube GData API. Patch coming soon.
Comment 1 Kevin Duffus 2009-03-25 07:50:37 UTC
Created attachment 131322 [details]
Screenshot of YouTube source after searching for "firefox"

Currently clicking buttons just open the video's URL in the default web browser.
Comment 2 Kevin Duffus 2009-03-27 05:10:05 UTC
Created attachment 131484 [details]
YouTube extension source
Comment 3 Andrés G. Aragoneses (IRC: knocte) 2009-03-30 15:53:00 UTC
Hey Kevin, this looks promising.

Just wanted to mention that here's a similar idea that I didn't have time for:
http://idea.opensuse.org/content/ideas/party-extension-for-banshee-youtubeflash-streaming-mixed-with-current-music-in-the-user-s-library
Comment 4 Kevin Duffus 2009-05-23 04:36:52 UTC
Created attachment 135221 [details]
YouTube page in the content pane
Comment 5 Kevin Duffus 2009-05-23 04:37:56 UTC
Created attachment 135222 [details] [review]
Patch to add youtube page to the content pane
Comment 6 Kevin Duffus 2009-05-23 04:38:54 UTC
Updating bug summary to reflect extension changes
Comment 7 Kevin Duffus 2009-05-23 17:49:07 UTC
Created attachment 135244 [details] [review]
Apply second patch to show message when no videos found
Comment 8 Gabriel Burt 2009-05-27 23:20:09 UTC
Thanks Kevin, this looking pretty good.

1) Please use the same copyright header you find elsewhere, eg make sure you have the // Copyright (C) 2009 ... line.  You of course keep your copyright.

2) public class VerticalTile: Button  => put a space before the :

3) You've got some slightly off indentation, eg:
+            Relief = ReliefStyle.None;
+        }
+       
+       public string PrimaryText {

4) The "+    copyright="© Kevin Duffus." in the addin.xml needs to have a year

5) In your ContextPage.cs and YouTubePane.cs files you have some tabs for indentation

6) Please combine both patches
Comment 9 Kevin Duffus 2009-05-29 08:17:55 UTC
Created attachment 135543 [details] [review]
This patch should cover all the requested changes
Comment 10 Alexander Kojevnikov 2009-06-03 07:42:07 UTC
A few minor nitpicks:

* We use Banshee.IO.File instead of System.IO.File:
    if (File.Exists (cache_file)) {
    using (FileStream file_stream = File.Open (cache_file, FileMode.Create)) {
    etc...

* There's a small visual glitch on dark themes, I will attach a screen-shot.

Other than that, the patch looks (and works) great, well done!
Comment 11 Alexander Kojevnikov 2009-06-03 07:45:14 UTC
Created attachment 135863 [details]
Visual glitch on the Darklooks theme
Comment 12 Kevin Duffus 2009-06-03 18:55:56 UTC
Created attachment 135903 [details] [review]
Fix glitch in dark themes and use Banshee.IO instead of System.IO
Comment 13 Kevin Duffus 2009-06-03 19:13:56 UTC
Hmm. I'm not sure but would the following be more appropriate for saving a file?

+            using (HttpWebResponse response = (HttpWebResponse) request.GetResponse ()) {
+                using (Stream stream = GetResponseStream (response)) {
+                    using (FileStream file_stream = Banshee.IO.File.OpenWrite (uri, true)) {
+                            Banshee.IO.StreamAssist.Save (stream, file_stream);
+                    }
+                }
+            }
Comment 14 Alexander Kojevnikov 2009-06-04 01:04:03 UTC
(In reply to comment #13)
> Hmm. I'm not sure but would the following be more appropriate for saving a
> file?

Yes, it does exactly the same.
Comment 15 Kevin Duffus 2009-06-04 08:27:34 UTC
Created attachment 135924 [details] [review]
This one should apply cleanly
Comment 16 Gabriel Burt 2009-06-09 22:37:32 UTC
Getting

Making all in Banshee.YouTube
  MCS   ../../../bin/Banshee.YouTube.dll
error CS2007: Unrecognized command-line option: `-r/usr/lib/mono/GData-Sharp/Google.GData.Extensions.dll'

any ideas?
Comment 17 Kevin Duffus 2009-06-09 22:51:23 UTC
Thats odd. It seems that a make file wasn't generated correctly. There should be a space following the '-r' switch. Perhaps running autogen.sh again could fix it?
Comment 18 Gabriel Burt 2009-06-09 23:02:47 UTC
ah, the missing colon, right; must be a bug in my package or version of gdata-sharp
Comment 20 Kevin Duffus 2009-09-21 18:31:27 UTC
Does anyone believe this is worthy of being pushed upstream or rather packaged separately? I currently don't have a storage facility online for it at the moment.
Comment 21 Gabriel Burt 2009-11-13 00:52:16 UTC
Review of attachment 135924 [details] [review]:

Looks really good, Kevin.  Just a few niticks.

What's the status of being able to play videos within Banshee - does YouTube expose the actual video URL yet?  I heard rumors about them supporting the HTML5 <video> tag, thought maybe the story is better than when I last checked.

I'm fine with this going into master and the default plugins set.

::: configure.ac
@@ +94,3 @@
 BANSHEE_CHECK_WEBKIT
 
+dnl webkit (optional through --enable-gdata)

s/webkit/gdata/

::: src/Extensions/Banshee.YouTube/Banshee.YouTube.Data/YouTubeData.cs
@@ +54,3 @@
+    {
+        public Feed<Video> videoResults;
+        public List<string> allCategories = new List<string> ();

Why are these two public?  If they really need to be, make them properties.

Also, please name all non-public members like_this, and public members LikeThis

::: src/Extensions/Banshee.YouTube/Banshee.YouTube/YouTubePane.cs
@@ +97,3 @@
+                
+                if (!String.IsNullOrEmpty (query))
+                {

put this on the same line as the if

@@ +202,3 @@
+
+                    foreach (Video entry in videoFeed.Entries)
+                    {

put { on foreach line

@@ +203,3 @@
+                    foreach (Video entry in videoFeed.Entries)
+                    {
+                        if (result_display_count++ >= max_results_display) {

change to if (entry.IsDraft) { continue; } else if (result_display_count++...)

@@ +207,3 @@
+                        }
+
+                        Log.DebugFormat ("YouTube: Adding tile for video \"{0}\"", entry.Title);

This is probably a little verbose logging

@@ +213,3 @@
+                            YouTubeTile tile = new YouTubeTile (entry);
+                            results_tv.AddWidget (tile);
+                            results_tv.ShowAll ();

put ShowAll outside/beneath the foreach
Comment 22 Kevin Duffus 2009-11-13 23:06:59 UTC
I've made the suggested changes and did a little bit of cleaning up. I wasn't able to dig up any information regarding HTML5 video exposed via the GData API. However, there are a lot of videos that have RSTP URLs (*.3gp files for mobile) that could be retrieved. But I'm not sure exactly how to pass the Uri directly to the player engine to have it queued or played.
Comment 23 Kevin Duffus 2009-11-20 20:57:37 UTC
Created attachment 148190 [details] [review]
Patch cleaned up with respect to Gabriel's input

Taking a queue from the YouTube plugin from Totem, I was able to find a means of playing the videos directly within Banshee =). Known issues:

1. I've noticed that an initial click of the video button may result in a timeout waiting for a response from the server. Here I'm simply passing the URI to Banshee.Streaming.RadioTrackInfo.OpenPlay () so I'll need to figure out how to increase the default timeout value.

2. After the video finishes playing. Banshee returns to the playing from the top of the previous playback source. Not sure what would be the preferable starting point here (next song after the that which was playing before the video was clicked?)
Comment 24 Kevin Duffus 2009-11-20 21:06:12 UTC
Created attachment 148191 [details]
Screenshot of YouTube video playing back in Banshee
Comment 25 Bertrand Lorentz 2009-11-20 22:00:17 UTC
Review of attachment 148190 [details] [review]:

I think there are several file that should not be in the patch :
- Banshee.YouTube.sln
- Banshee.YouTube.usertasks
- bk.csproj
Comment 26 Kevin Duffus 2009-11-20 22:03:57 UTC
My apologies. I realised this soon after uploading.  I'll make sure to review more carefully before attaching next time.
Comment 27 Gabriel Burt 2009-11-20 22:10:19 UTC
Also, the UI seems to be blocked while the YouTube extension is loading items.
Comment 28 Kevin Duffus 2009-11-21 04:51:18 UTC
Created attachment 148210 [details] [review]
Updated patch

Latest patch fixes the following issues:
1. Stream fail dialogue no longer appears due to timeout.
2. Tiles are loaded/removed in a separate thread so UI is no longer blocked

Also, track info display now shows the YouTube video's title and uploader's username instead of "Unknown..."
Comment 29 Samuel Gyger (IRC: thinkabout) 2009-11-30 20:25:45 UTC
Kevin I love the afford you put into this. Thank you very much, looking forward for this feature.
Another Improvement:
Could the Video be an element in the playlist like a normal music or videofile so I can just create a playlist also with music not on my computer? Or just save Element on my harddisk. With support for HD on youtube audio quality is quite good.
Create an easy interface to add support for other media sites in the same plugin.
Comment 30 Gabriel Burt 2009-12-24 19:50:44 UTC
Review of attachment 148210 [details] [review]:

::: src/Extensions/Banshee.YouTube/Banshee.YouTube.Data/DataFetch.cs
@@ +78,3 @@
+                
+                while (count > 0) {
+                    contents = String.Concat (contents, new String(read_buffer, 0, count));

Any reason not to use stream_reader.ReadToEnd () instead of doing all this buffer stuff?

::: src/Extensions/Banshee.YouTube/Banshee.YouTube.Gui/YouTubeTile.cs
@@ +72,3 @@
+            if (!match.Success) { return null; }
+            
+            t_param = Regex.Unescape(match.Result ("$1"));

=> Unescape (
that is, add a space

@@ +94,3 @@
+
+                foreach (MediaContent m in yt_video.Media.Contents)
+                {

put { on same line as foreach

@@ +112,3 @@
+                }
+            }
+            else

change to } else {

::: src/Extensions/Banshee.YouTube/Banshee.YouTube.addin.xml
@@ +6,3 @@
+    copyright="© Kevin Duffus. Licensed under the MIT X11 license."
+    name="YouTube"
+    category="User Interface"

Change category to "Context Pane"

@@ +7,3 @@
+    name="YouTube"
+    category="User Interface"
+    description="View results of searching YouTube for playing song"

Change description to "Show related YouTube videos in the context pane." maybe?

::: src/Extensions/Banshee.YouTube/Banshee.YouTube/YouTubePane.cs
@@ +160,3 @@
+        }
+
+        private class RefreshVideosJob : Banshee.Kernel.Job

Do not use Banshee.Kernel.Job - instead use Hyena.Jobs.

@@ +200,3 @@
+                    foreach (Video entry in video_feed.Entries) {
+                        // Don't include videos that are not live
+                        if (entry.IsDraft) { continue; }

change all single-line if's like this to be if (..) {\n .. \n}

@@ +202,3 @@
+                        if (entry.IsDraft) { continue; }
+                        else if (result_display_count++ < max_results_display) {
+                            YouTubeTile tile = new YouTubeTile (entry);

Should not do GUI/GTK work off the main thread.  You can use ThreadAssist.ProxyToMain to run code on the main loop.
Comment 31 Kevin Duffus 2010-01-08 21:50:43 UTC
Sorry, I've been out of town. Gabriel, thanks for the review. I should have the fixes in by this weekend.
Comment 32 Kevin Duffus 2010-01-14 05:26:21 UTC
Created attachment 151380 [details] [review]
Updated patch from last review
Comment 33 Bertrand Lorentz 2010-01-17 18:43:32 UTC
Review of attachment 151380 [details] [review]:

Thanks Kevin, looks pretty good. See my comments.

I still haven't been able to test it, I'm still fighting to get gdata-sharp installed properly on my system.

::: src/Extensions/Banshee.YouTube/Banshee.YouTube.Gui/VideoStreamTile.cs
@@ +233,3 @@
+        }
+
+        /*private void OnAddToPlayQueue (object o, EventArgs args)

Why is the "Add to play queue" stuff commented out ?
You might want to add a TODO explaining it, or remove the code.

::: src/Extensions/Banshee.YouTube/Banshee.YouTube.Gui/YouTubeTile.cs
@@ +64,3 @@
+ 
+            if (String.IsNullOrEmpty (watch_page_contents)) {
+				return null;

Indent with 4 spaces, not tabs. Also happens several times further down in the file and maybe in other files.
Comment 34 Kevin Duffus 2010-02-03 04:12:02 UTC
Created attachment 152904 [details] [review]
Latest patch

Commented out "Add to Play Queue" code removed as I couldn't figure out how to do this cleanly. Also, tabs switched to 4 spaces.
Comment 35 Alexander Kojevnikov 2010-03-04 11:23:37 UTC
Created attachment 155218 [details] [review]
Updated to git master

Looks good and works well. I adapted it slightly to apply to git master.

The only issue I'm seeing is in the Makefile, somehow one of the gdata libs has an incorrect parameter (should be -r:/usr/...):

GDATASHARP_LIBS = ... -r/usr/local/lib/mono/GData-Sharp/Google.GData.Extensions.dll ...

Can't figure out what's causing it, could be a problem with my tarball install.

Gabriel, any chance this can be committed before the feature freeze?
Comment 36 Bertrand Lorentz 2010-03-04 11:38:36 UTC
(In reply to comment #35)
> Created an attachment (id=155218) [details] [review]
> Updated to git master
> 
> Looks good and works well. I adapted it slightly to apply to git master.
> 
> The only issue I'm seeing is in the Makefile, somehow one of the gdata libs has
> an incorrect parameter (should be -r:/usr/...):
> 
> GDATASHARP_LIBS = ...
> -r/usr/local/lib/mono/GData-Sharp/Google.GData.Extensions.dll ...
> 
> Can't figure out what's causing it, could be a problem with my tarball install.

It was a bug in the gdata-sharp .pc file :
http://code.google.com/p/google-gdata/issues/detail?id=336
Comment 37 Alexander Kojevnikov 2010-03-04 12:47:54 UTC
(In reply to comment #36)
> It was a bug in the gdata-sharp .pc file :
> http://code.google.com/p/google-gdata/issues/detail?id=336

Thanks Bertrand!
Comment 38 Gabriel Burt 2010-03-04 16:51:49 UTC
Comment on attachment 155218 [details] [review]
Updated to git master

Yeah, let's get this in now.
Comment 39 Gabriel Burt 2010-03-04 17:03:59 UTC
Hrm, let's put this in a youtube branch.  I think there are still some issues with it w/ threading.
Comment 40 Gabriel Burt 2010-03-04 17:05:40 UTC
For example:

+            ThreadAssist.Spawn (delegate {
+                if (video_feed.TotalResults > 0) {
+                    if (!showing_results) {
+                        Remove (no_results_label);
+                        Add (results_sw);
+                        ShowAll ();
+                    }

This appears to be calling Remove, Add, and ShowAll off the GUI thread.
Comment 41 Gabriel Burt 2010-03-04 17:11:26 UTC
I got to a state where Banshee's UI was frozen, which can happen when you do GUI updates off the GUI thread.
Comment 42 Alexander Kojevnikov 2010-03-04 23:53:05 UTC
Review of attachment 155218 [details] [review]:

I committed this patch to a new branch called `youtube`
Comment 43 Kevin Duffus 2010-03-05 00:05:55 UTC
Glad to see movement on this patch. Alex, thanks for updating to master. Gabriel, my apologies for the threading issues as its the first time I'm working with this language.
Comment 44 Gabriel Burt 2010-03-05 00:47:39 UTC
Kevin, no problem at all - it's a tricky thing to get right for all of us.
Comment 45 Alexander Kojevnikov 2010-03-05 02:47:24 UTC
(In reply to comment #41)
> I got to a state where Banshee's UI was frozen, which can happen when you do
> GUI updates off the GUI thread.

Should be fixed now.
Comment 46 Gabriel Burt 2010-03-05 04:34:08 UTC
Thanks Alexander.  What is being cached?  Is anything cached across restarts of Banshee?  It's loading fairly slowly for me (granted not on a great internet connection atm) - Wireshark shows it issues two GETs per each 12 videos (one for metadata, one for thumbnail), so 25 GETs total, and no keepalive action I don't think (maybe within the GData api, but not for thumbnails).
Comment 47 Alexander Kojevnikov 2010-03-05 04:59:01 UTC
(In reply to comment #46)
> Thanks Alexander.  What is being cached?  Is anything cached across restarts of
> Banshee?

The thumbnails are cached for two hours, may be the period should be increased.

> It's loading fairly slowly for me (granted not on a great internet
> connection atm) - Wireshark shows it issues two GETs per each 12 videos (one
> for metadata, one for thumbnail), so 25 GETs total, and no keepalive action I
> don't think (maybe within the GData api, but not for thumbnails).

Same here, it takes up to 10 seconds to show the list of videos.
Comment 48 Alexander Kojevnikov 2010-03-12 08:45:11 UTC
The youtube branch has been merged into master a few days ago and the extension is available in 1.5.5