GNOME Bugzilla – Bug 549568
DVD support
Last modified: 2011-09-25 20:57:38 UTC
With Banshee also being a video management application it would be nice to have (video) dvd support. I just found my Franz Ferdinand live dvd in the mailbox and fired up Totem to watch/listen. Awesome. But wait.. wouldn't it be nice for Banshee to have such features? As Banshee already supports audio cd and video, why not video dvd?
Well. It would create from banshee an ultimate media center ;). Probable the best one.
Is it possible to help you on this patch. Is there a branch or github/gitorious to help on that feature?
The work is very preliminary. If you'd like to help, fork me on github, work from there, and propose pull requests of your work. Basically it's as far as recognizing dvds, but only as audio cds. Your help would be massively appreciated. https://github.com/lamalex/Banshee#
Status update, it now recognizes dvds and tries to play them, but gstreamer won't go into the PLAYING state. Once I figure that out, DVDs should be 'playing' in Banshee.
OK found why dvd support is not working... You have to set the device of source element of playbin2 with a "notify::source" callback. From doc of playbin2: " Specifying which CD/DVD device to use The device to use for CDs/DVDs needs to be set on the source element playbin creates before it is opened. The only way to do this at the moment is to connect to playbin's "notify::source" signal, which will be emitted by playbin when it has created the source element for a particular URI. In the signal callback you can check if the source element has a "device" property and set it appropriately. In future ways might be added to specify the device as part of the URI, but at the time of writing this is not possible yet. " I will provide a patch soon (tonight if %I have time) with this fix.
Created attachment 175805 [details] [review] fix gstreamer pipeline for DVD must be working but can not test because I have no dvd player on my pc...
1) I already tried this and it doesn't fix it. 2) Patch doesn't actually apply
Created attachment 175821 [details] [review] gstreamer dvd pipeline Anyway, must be done to work... To apply patch just get the last git from gnome repo and apply. I have done "git pull origin (gnome)" after having get your repo. I send again the patch if it was an other issue...
Created attachment 175824 [details] [review] dvd backend patch I found a missing thing too: must change PlayerEngine to update capabilities: source_capabilities = { "file", "http", "cdda", "dvd", "vcd" }; My previous patch was wrong too because parse of uri for device depend of scheme... Maybe another issue : I read totem source to see what s wrong with our code and I see that totem set player->media_device to "/dev/dvd" when started...
That shouldn't matter but I'll try it anyway. gst-launch -v playbin2 uri=dvd:///dev/sr0 works fine
/dev/dvd is just a symlink anyway
Just a quick help for people on that bug. Have to do on ubuntu: sudo apt-get install libdvdread4 sudo /usr/share/doc/libdvdread4/install-css.s a small patch: in HasVideo property of HalBackend DiskVolume getter: return HalDevice.GetPropertyBoolean ("volume.disc.is_videodvd");
ok : gone further. So it do not work thanks to vis system. If I comment vis pipe build function call in pipeline, it work ;) I have troubleshoot and found that the main difference is that regular video triger a GST_EVENT_NEWSEGMENT in _bp_vis_pipeline_event_probe whereas dvd video trigger GST_EVENT_CUSTOM_DOWNSTREAM and GST_EVENT_CUSTOM_DOWNSTREAM_OOB anyway adding them do not solve the issue...
Created attachment 176243 [details] [review] Fix pipeline and add an early navigation system This patch need some cleanup but I have no more time... - Fix pipeline (vis part) - Add navigation on backend - Add got to menu in actions - Add mouse and keyboard notification for dvd menu - Add hal support for dvd Anyway, DVD menu appear but navigation do not work. Have to work on it ;) But I share the code if lamalex (or other) have time to go further before my next coding time ;)
Created attachment 176311 [details] [review] dvd support This patch make navigation working but buggy because lost focus cause of other widget of the UI. If you do not succeed to apply patch, you can get from github :https://github.com/dufoli/banshee A part have been done by lamalex. I have just fix pipeline and add navigation basic system.
Created attachment 176607 [details] [review] dvd A new patch which fix next/prev for chapter and fix focus lost on enter/down/up. Still need work but is ready for test and feedback ;) TODO: Go to menu do not appear in actions Click on now playing content do not get focus back mouse menu navigation do not work I have a strange behaviour git diff -p origin > dvd.patch get some origin commit in the patch.... so best is to pull from github.
For context of the current problems with this patch regarding fullscreen and mouse events: http://mail.gnome.org/archives/banshee-list/2010-December/msg00124.html
Heya, new patch still with the issue of getting commit from origin whereas I do "git diff -p origin". So please, get last version on github or give me a tips to remove all bad commit. I have try git rebase -i but it give me a lot of merge to do... Fix the "go to menu" option because dvdService is not loaded in fact. Fix the mouse event when not in fullscreen. It fix the double click to get fullscreen too ;) The 2cd fix was hard to found. First, I put eventbox just over the overlay and not after the table and I set above-child to true. EDIT: can not send the patch because over 1000kb. so only github: https://github.com/dufoli/banshee/tree/dvd
Created attachment 177077 [details] [review] dvd support patch this patch seems clean and have not the not needed things. So need a review now ;)
Fantastic work, it (mostly) applies and builds However this contains a Hyena bump, on purpose? patch: **** File src/Hyena is not a regular file -- can't patch Build warnings: banshee-player.c: In function ‘bp_open’: banshee-player.c:162:5: warning: implicit declaration of function ‘_bp_dvd_handle_uri’ Build failures: Making all in Banshee.Gio MCS ../../../bin/Banshee.Gio.dll ./Banshee.Hardware.Gio/LowLevel/GioVolumeMetadataSource.cs(85,21): error CS0029: Cannot implicitly convert type `string' to `string[]' Compilation failed: 1 error(s), 0 warnings I haven't looked much at the code but it somehow feels like it could be broken up to be easier to read.
You need GIO from git. I fixed a bug in the bindings' api that this patch relies on the fix for.
Confirmed, now it makes me worried that we will now be facing: https://bugzilla.gnome.org/show_bug.cgi?id=635501
Created attachment 177085 [details] [review] dvd support patch fix the warning by adding a missing include
Review of attachment 177085 [details] [review]: It looks good pretty much, there are a few things- see inline review. I don't have a dvd on me so I can't actually test right now. If someone could do some testing with different DVDs and just make sure everything works ok, I think we should be good to go with dvd support. There is definitely some room for improvement, for instance adding metadata fetching support, dvd art, etc., but these are good patches for later on. I don't think that they should be blockers for this. ::: libbanshee/banshee-player-dvd.c @@ +91,3 @@ + + g_object_unref (cdda_src); + return FALSE; Please remove this commented out method if it's not needed. @@ +131,3 @@ + + GError *error = NULL; + new_dvd_device = uri + 6; Could you put a comment here just to make it clear that the + 6 is from dvd:// @@ +136,3 @@ + g_error_free (error); + return FALSE; + } How would error not be null? You just declared error = NULL, if it's not NULL something much bigger is going wrong. @@ +158,3 @@ + return FALSE; + } + This block looks completely pointless. ::: libbanshee/banshee-player-pipeline.c @@ +243,3 @@ + } + gst_query_unref (query); + } This needs to be re-factored to not nest so deeply. Do some if(false) return, instead of if (true) { if (true) { } } @@ +451,1 @@ _bp_video_pipeline_setup (player, bus); Should we set up the dvd pipeline after the video pipeline? Even if it doesn't matter technically, I think it would just read better. ::: src/Backends/Banshee.Gio/Banshee.Hardware.Gio/DiscVolume.cs @@ +50,3 @@ + static DiscVolume () + { + video_mime_types = new string[] {"x-content/video-dvd", Put this one on a new line; looks nicer. ::: src/Extensions/Banshee.Discs/Banshee.Discs.Dvd/DvdSource.cs @@ +72,3 @@ + private void UpdateActions () + { + // TODO Update dvd actions Should probably do this before committing. What actions need updating? If it's none, remove the method. @@ +92,3 @@ + // Go to menu ? + ServiceManager.PlayerEngine.NavigateToMenu (); + return true; I think this is good go to first behavior. Makes sense to me. Maybe want to do some user testing if you have time. Is there any dvd player that just has a "start movie over" option that isn't the dvd menu? Is that a useful feature? We already have a go to menu, if jumping to the beginning of the movie would be useful maybe we should think about this. @@ +101,3 @@ + } + // Do nothing if not in menu + return true; You mean, "Do nothing if in the menu" @@ +110,3 @@ + } + // Do nothing if not in menu + return true; Same as above. ::: src/Extensions/Banshee.Discs/Banshee.Discs.addin.xml @@ +8,3 @@ + category="Core" + description="Watch DVDs; listen to and rip Audio CDs." + author="Aaron Bockover" Add the two of us to this :)
Review of attachment 177085 [details] [review]: It looks good pretty much, there are a few things- see inline review. I don't have a dvd on me so I can't actually test right now. If someone could do some testing with different DVDs and just make sure everything works ok, I think we should be good to go with dvd support. There is definitely some room for improvement, for instance adding metadata fetching support, dvd art, etc., but these are good patches for later on. I don't think that they should be blockers for this. ::: libbanshee/banshee-player-dvd.c @@ +91,3 @@ + + g_object_unref (cdda_src); + return FALSE; Please remove this commented out method if it's not needed. @@ +131,3 @@ + + GError *error = NULL; + new_dvd_device = uri + 6; Could you put a comment here just to make it clear that the + 6 is from dvd:// @@ +136,3 @@ + g_error_free (error); + return FALSE; + } How would error not be null? You just declared error = NULL, if it's not NULL something much bigger is going wrong. @@ +158,3 @@ + return FALSE; + } + This block looks completely pointless. ::: libbanshee/banshee-player-pipeline.c @@ +243,3 @@ + } + gst_query_unref (query); + } This needs to be re-factored to not nest so deeply. Do some if(false) return, instead of if (true) { if (true) { } } @@ +451,1 @@ _bp_video_pipeline_setup (player, bus); Should we set up the dvd pipeline after the video pipeline? Even if it doesn't matter technically, I think it would just read better. ::: src/Backends/Banshee.Gio/Banshee.Hardware.Gio/DiscVolume.cs @@ +50,3 @@ + static DiscVolume () + { + video_mime_types = new string[] {"x-content/video-dvd", Put this one on a new line; looks nicer. ::: src/Extensions/Banshee.Discs/Banshee.Discs.Dvd/DvdSource.cs @@ +72,3 @@ + private void UpdateActions () + { + // TODO Update dvd actions Should probably do this before committing. What actions need updating? If it's none, remove the method. @@ +92,3 @@ +// + ServiceManager.PlayerEngine.NavigateToMenu (); + return true; I think this is good go to first behavior. Makes sense to me. Maybe want to do some user testing if you have time. Is there any dvd player that just has a "start movie over" option that isn't the dvd menu? Is that a useful feature? We already have a go to menu, if jumping to the beginning of the movie would be useful maybe we should think about this. @@ +101,3 @@ + } + // Do nothing if not in menu + return true; You mean, "Do nothing if in the menu" @@ +110,3 @@ + } + // Do nothing if not in menu + return true; Same as above. ::: src/Extensions/Banshee.Discs/Banshee.Discs.addin.xml @@ +8,3 @@ + category="Core" + description="Watch DVDs; listen to and rip Audio CDs." + author="Aaron Bockover" Add the two of us to this :)
Created attachment 177912 [details] [review] dvd support patch Fix from feedback.
Created attachment 177960 [details] [review] dvd support patch previous patch was wrong because of git but here is a new one which is good this time.
Review of attachment 177960 [details] [review]: I believe the preferred comment style is // foo not //foo. Aside that I just wonder what the Fixup change is in there for. ::: src/Extensions/Banshee.Fixup/Banshee.Fixup.csproj @@ +92,3 @@ <Compile Include="Banshee.Fixup\Tests.cs" /> <Compile Include="Banshee.Fixup\View.cs" /> + <Compile Include="Banshee.Fixup\GenreDuplicateSolver.cs" /> This seems unrelated to the DVD support patch
Review of attachment 177960 [details] [review]: Also the Hyena bump probably shouldn't be in there.
For hyena, I do not know how remove the that. git done it automatically. Same for the other GenreDuplicateSolver, Do not know how it come here... git is great but sometime it do some things which is buggy... I think the only solution is done git rebase origin -i but it failed a lot so have 42 patchs to fix. Too much work to just fix a git bug.
OK, I have done the git rebase -i and this remove all wrong patch.
Do you need a patch or it is oki to have review on github. https://github.com/dufoli/banshee/tree/dvd else I can send a patch.
This all looks pretty good to me. Someone else, who didn't write a substantial amount of this code, may also want to review before we merge it in. But it looks good to me!
I still worry about the leak fix since merging this will mean distributions will have to cherrypick the change or they will mindlessly update to a version that while correct will break Banshee and likely other applications.
what leak? memory leak? in that code? in gstreamer?
In gio-sharp git which we will require when this gets merged. https://bugzilla.gnome.org/show_bug.cgi?id=549568#c23
Well shouldn't those commits be reverted until a fix without a crash is found?
In that case, shouldn't this be marked as Depending on Bug #635501?
(In reply to comment #42) > Well shouldn't those commits be reverted until a fix without a crash is found? +1 But wait, has this been committed at all? Isn't Olivier just working in his own repo? I don't see the patches being marked as 'committed' either.
I mean the GIO commits.
Good news, with Alan we have worked on gobject introspection and gio now compile with it. I hope that we will be able to test its working for GTK 3 and repalce old GIO-sharp with a better one comming from gir files which will remove lot of bug (hopefully this one).
I would not count on those bindings becoming available right now. Mike Kestner, Alan and others are currently debating what is the best path from here but a large part has been done. But it does mean that there are going to be delays and that we are likely to have to deploy this using the currently gio bindings for 2.0.
Are we still waiting for GTK3's GIO bindings, or are we planning to get this into 2.0 (1.9.5 is feature-freeze) first?
The most current patch does not apply cleanly to git master.
I have just migrate the patch to last git and add dvd support to gst# backend. this backend do not work because video is unactivate for moment but it will be ready for futur. https://github.com/dufoli/banshee/tree/dvd Now need a review to get that in master ;-)
Created attachment 194288 [details] [review] dvd patch here you have new patch if you prefer that to review code. The patch is really big. good luck!
Created attachment 194290 [details] [review] dvd patch previous patch bring some commit from gnome with it. So this on is clean.
Created attachment 194952 [details] crash while startign dvd Attempted to play "Lord of the Beans", disc plays fine in Totem, but crashed banshee with patch applied. I'm currently running on Fedora15, but can test on openSUSE or *buntu via a virtual machine if needed.
hmm, I need a better log: 1) can you remove Banshee.AudioCd from bin directory? Because it is renamed so you have duplicate addon assembly. 2) Can you do a new log with GST_DEBUG=4; banshee --debug > log 2>&1 I need to know what s wrong with this code because must be ok to free this var...
Created attachment 195257 [details] banshee log Ran with: GST_DEBUG=4; mono bin/Banshee.exe --debug > ~/banshee.log 2>&1
Created attachment 195280 [details] [review] fix subtitle crash ok. So few questions: what is the version of gstreamer? I guess lesser than 0.10.24 what subtile tracks are on this dvd ? Is the bug present on other dvd ? I have maybe a small patch linked. This patch is to apply over dvd patch because this bug can be address out of dvd patch. It fix a memory leak too...
Comment on attachment 195280 [details] [review] fix subtitle crash Hey Olivier, this 2-liner looks good to commit, can you push it to master so it's easier for William to re-test? Thanks!
it is pushed to master. So can you test the patch on last version.
Sorry, I've been busy the last couple of days and haven't been able to test the new patch. I'll rebuild with it tonight and test.
(In reply to comment #59) > Sorry, I've been busy the last couple of days and haven't been able to test the > new patch. I'll rebuild with it tonight and test. Note: there's no new patch, just git pull from master to get Olivier's fix. Thanks for testing.
Created attachment 195452 [details] banshee log during playback I have DVD playback working with the update to banshee. So to answer your questions: gstreamer-0.10.34 from the Fedora 15 repos I think there is a bug in gstreamers's subtitle and audio track handling because both banshee & totem only lists one set of subs and audio for any movie I play. But vlc lists many. This occurred for me on both "Lord of the Beans" and "Dr. Strangelove". Some usability issues: - Method for playback seems awkward: 1.) Click on DVD 2.) double click on untitled video 3.) Watch dvd Perhaps double clicking on the DVD should start playback at the menu, while single clicking shows a list of titles, or better yet chapters in the main title. - Menu doesn't work, from the comments above it seems like this is a known issue, but DVD playback is not ready for primetime until it works. One is forced to next through titles until you reach the movie. - Clicking next goes to the next chapter, while clicking back always goes to the previous title. Previous should probably never go to the previous title, only to the previous chapter or restart chapter 1. You may even want back to restart the current chapter unless you are in the first 10-15 seconds of the chapter. - Goto menu is a great context menu item for the DVD source, but you need to add an eject item, and perhaps subtitle and audio track selections. log was captured while recording this screencast: http://www.vimeo.com/28486304
- For the DVD UI, I agree, it can be improve but on that part, I want some design sketch/idea to found the best UI because do not know what is the best solution... What is solution on other media player? - Menu must worked I have fixed it... what s wrong with it ? keyboard naviguation? mouse naviguation ? seems it on vimeo... fullscreen or not? - Previous button bug has to be troubleshooted... First view do not enlight it quickly... - eject dvd and language/ audio track selection is definitly a good idea for menu For subtitles, I will see... Anyway, I will do new tests maybe the rebase have make the menu naviguation not working ;(
Thanks for testing. Given that the patch is working (you can play a DVD with it) but with minor problems to be solved, I think the patch should be merged and we should file each of those issues as individual bugs, to have more community involved on each decision to solve each separate issue (UI for example). Especially, considering that for the next 2.1.x release only bugfixes are allowed (not new features), so then we can only target this patch to be merged to master for 2.3.x (after 2.2.0 is released), but after that we have a whole new time period of development releases to take care of these issues. Sounds good? If yes, let's try to find a reviewer for the patch that concentrates on the GStreamer parts (as Bertrand on IRC said he was fine with a gstreamer-expert reviewing it).
(In reply to comment #62) > - Menu must worked I have fixed it... > what s wrong with it ? > keyboard naviguation? > mouse naviguation ? seems it on vimeo... > fullscreen or not? I've tried both keyboard and mouse DVD nav in both full screen and not full screen(windowed?). I run dual screens and have tried it on both screens. I am running on GNOME 3, but have XFCE installed if you want me to try there.
I just remember that I have a focus issue in past. I do not remember if I have solve it on past So, you have to focus the video area... Can you test with focus ? If it is that, it is another bug to solve...
Comment on attachment 195280 [details] [review] fix subtitle crash This was committed to git master
Sorry for the delays, busy time at work. Try as I might, I could not get nav to work. Tried keyboard and mouse, both windowed and fullscreen. Tried to tab my way through the controls, but did not have any luck.
Review of attachment 194290 [details] [review]: The C GStreamer part looks good to me ::: libbanshee/banshee-player-pipeline.c @@ +216,3 @@ } + if ( gst_navigation_message_get_type (message) != GST_NAVIGATION_MESSAGE_COMMANDS_CHANGED) { + _bp_missing_elements_process_message (player, message); if (gst_is_missing_plugin_message (message)) probably. Or does this also handler other messages?
Thanks for taking the time to review it. Yep, I agree. I have even fixed it on gst# backend as you can see : [code] if (NavigationMessage.MessageGetType (msg) == NavigationMessageType.CommandsChanged) [/code] but forget to fix it on libbanshee part. I will do it before the big merge.
ok it is fixed on github: https://github.com/dufoli/banshee/commit/b830d713905b5db54436308c8bdce07485dcd2b1 I move the dvd message processing in dvd file too. Test on message for missing codec was done in missing coec message processing.
I have just committed the patch on git master, along with Olivier's latest changes, and some other modifications, clean-ups and bug fixes. I've noticed a few issues, mainly with menu navigation: using the mouse doesn't work most of the time, using the keyboard works better. I'll be filling those issues in new bugs. I encourage everyone to test this new DVD support and report any issues by filling new bugs. Thanks everyone for you work on this !