GNOME Bugzilla – Bug 322268
Integrated ripping and transferring of tracks (including DAAP shares)
Last modified: 2007-03-27 03:14:54 UTC
Like sound-juicer (probably also using s-j's code) but integrated in RB.
Integrated ripping could allow us to do drag and drop from audio CDs to the library. That would be pretty cool.
Maybe we can could convince Ross to make a libsoundjuicer so that we can use it to make an integrated riping. sri
I had a chat with Ross about that a while back, about making a lib containing SJ's metadata lookup and extraction code. Neither of us have gotten around to actually doing anything about it though.
I agree it would be cool and useful. It would for sure be an intuitive way to download tracks to the media library.
Bug 325895 is about splitting S-J's metadata lookup and ripping code into a library. This will depend on that, unless we want to copy the code (which wouldn't be as nice as using the library).
Created attachment 59813 [details] [review] use libjuicer for metadata lookup This makes us use libjuicer instead of the copied source files (requires SJ compiled with patch from bug 325895). It doesn't do any cd-ripping stuff yet.
Created attachment 59814 [details] [review] use libjuicer for metadata lookup This makes us use libjuicer instead of the copied source files (requires SJ compiled with patch from bug 325895). It doesn't do any cd-ripping stuff yet.
Created attachment 60153 [details] [review] very preliminary patch This is a preliminary patch to add CD-ripping and transfer stuff. Using this, I can rip CDs and transfer track to my library from any source. There will be many bugs. Originally I was doing this using "libjuicer" which is exported by the patch on the related S-J bug. However the code needed for transcoding is very similar, so if we did use libjuicer we would need a lot of the code duplicated anyway (or make libjuicer handle transcoding, etc). If people want to test/look at this you can, but there are several things to note: * no UI, apart from error messages. The prefs widgets aren't hooked up, so you will need to set several gconf keys. * to do anything, select tracks and drag them to the library source. * it will fail if the destination file already exists * it does not queue operations, and will attempt them all at once * only supports gstreamer0.10 I've also played around with a different layout/method of backends; whether or not this is a good idea, I'm not sure. Main backend files go in backend/, and the gstreamer implementation under backends/gstreamer/. rb_encoder_new in practice would use a preference (gconf key) or ranking to pick the implementation, which would make them a runtime rather than compile time decision.
Oops, I missed one of the configure.ac patch sections. To compile you need to use the following as well diff -u -r1.212 configure.ac --- configure.ac 22 Feb 2006 20:05:46 -0000 1.212 +++ configure.ac 27 Feb 2006 00:00:29 -0000 @@ -52,7 +52,8 @@ libgnomeui-2.0 \ libglade-2.0 \ gnome-vfs-2.0 >= $GNOME_VFS_REQS \ - gnome-vfs-module-2.0) + gnome-vfs-module-2.0 \ + gnome-media-profiles) PKG_CHECK_MODULES(TOTEM_PLPARSER, totem-plparser >= $TOTEM_PLPARSER_REQS, have_totem_plparser=yes, have_totem_plparser=no) if test x$have_totem_plparser != xyes; then
Created attachment 60553 [details] [review] better patch * queues tracks for transfer (doesn't try to do all at one any more) * preferences widgets now work * "copy tracks to library" action for audio cds (in toolbar and popup menu) * don't copy iradio or podcasts * several bug fixes As before you can copy tracks from any source (except iradio and podcasts) to the library via copy&paste, dragging to the library source, or the "copy all" action for audio cds. Still no transfer UI besides error reporting, or gst 0.8 port.
When trying to apply the latest patch against CVS HEAD, it wants to create a new file named source/rb-library-source.c, and a new schema file, which obviously conflict with the existing files.
Created attachment 61098 [details] [review] Makefile.am changes I had to make the following small changes in addition to your patch to get it to link.
Created attachment 61108 [details] [review] At last... And with that patch on top of yours, I can write files to my iPod :) (totally off-topic, and this support is still really rough, but full iPod support is getting closer).
Re attachment 61108 [details] [review]: Looks good. You can just use rb_true_function for impl_can_paste, if it will never be false. I'm still not sure what to do about transcoding; at the moment your ipod stuff will leave tracks in their original format, which won't work if they're oggs or whatever.
Created attachment 61116 [details] [review] updated ripping/transfer patch This version of the ripping/transfer patch adds progress indication in the status bar, and asks you about replacing existing files (when using gnomevfssink).
(In reply to comment #15) > Created an attachment (id=61116) [edit] > updated ripping/transfer patch Patch doesn't apply cleanly against HEAD anymore: patching file sources/rb-library-source.c Hunk #3 succeeded at 90 (offset 2 lines). Hunk #4 FAILED at 111. Hunk #5 FAILED at 126. Hunk #7 succeeded at 191 (offset 5 lines). Hunk #9 succeeded at 264 (offset 13 lines). Hunk #10 FAILED at 274. Hunk #11 succeeded at 287 (offset 1 line). Hunk #12 succeeded at 388 (offset 22 lines). Hunk #13 succeeded at 556 with fuzz 1 (offset 50 lines). Hunk #14 succeeded at 754 (offset 28 lines). Hunk #15 succeeded at 787 (offset 50 lines). Hunk #16 succeeded at 857 (offset 28 lines). Hunk #17 succeeded at 917 (offset 50 lines). Hunk #18 succeeded at 954 (offset 28 lines). Hunk #19 succeeded at 1142 (offset 50 lines). Hunk #20 succeeded at 1228 with fuzz 2 (offset -23 lines). 3 out of 20 hunks FAILED -- saving rejects to file sources/rb-library-source.c.rej
Created attachment 61422 [details] [review] better transfer patch * better error reporting * several bug fixes * almost working gst 0.8 port. I can't figure out why it doesn't actually work (gst logs show no errors)
Created attachment 61447 [details] [review] fix warning Tiny update to fix: cc1: warnings being treated as errors rb-encoder-gst.c: In function 'add_tags_from_entry': rb-encoder-gst.c:312: warning: dereferencing type-punned pointer will break stri ct-aliasing rules make[3]: *** [rb-encoder-gst.lo] Error 1 However, when I try to copy files to library I get: ERROR (0x99ba870 - 0:00:31.843293000) GST_PIPELINE(13484) ./grammar.y(533):gst_parse_perform_link: could not link audioresample0 to enc Where it seems to work with sound-juicer.
(In reply to comment #18) > Created an attachment (id=61447) [edit] > fix warning This patch is missing the new backends subdirectory.
What is the package gnome-media-profiles? It isn't present on FC-4 and configure fails on checking for this so I can't test this patch on FC-4. :-( Is this another only in very recent GNOME thing?
Created attachment 61476 [details] [review] updated patch Fixes the audioresample issue and adds the backends directory (which was missing in the last patch). We could use gst 0.10's "progressreport" element to do accurate progress reporting.
Two problems with patch: (1) lib/rb-marshal.{h,c} get generated by changes to rb-marshal.list, but if your build directory is not the same as a the source directory, it can't find those generated include files. (2) g_atomic_int_set was added in glib2-2.10, but FC-4 only had glib2-2.6 or so, so it will fail compilation on that. Christophe recommended using gst_atomic_int_set (in gst 0.10), which works for me.
More problems, after compiling, get this seg fault when attempting to open Preferences in menu: Program received signal SIGSEGV, Segmentation fault.
+ Trace 67026
Thread NaN (LWP 23183)
Created attachment 61479 [details] [review] Fix automake to work for separate builddir Add $(top_builddir)/lib for finding generated rb-marshal.h in build area, if not building inside source tree.
OK successfully built and installed patch, and can read CD information, now I click "Import" and I get a segfault: Program received signal SIGSEGV, Segmentation fault.
+ Trace 67045
Thread NaN (LWP 17725)
Fixed problem in comment #25 (wasn't using a valid import type), but now have the problem that only the first track on CD is imported, it stops ripping after that.
Created attachment 61539 [details] [review] better patch * fix the preferences issue * deal with missing gconf keys (should fix other crasher) * fix typo in file-name substitution
Request, could you use gst_atomic_int_set() rather than g_atomic_int_set(), because g_atomic_int_set() requires a very recent glib2 which isn't available on many distributions (notably FC-4), and it fails compilation: /home/alex/src/remote-cvs/gnome.org/rhythmbox/shell/rb-removable-media-manager.c: In function 'do_transfer': /home/alex/src/remote-cvs/gnome.org/rhythmbox/shell/rb-removable-media-manager.c:889: warning: implicit declaration of function 'g_atomic_int_set' /home/alex/src/remote-cvs/gnome.org/rhythmbox/shell/rb-removable-media-manager.c:889: warning: nested extern declaration of 'g_atomic_int_set'
Also a minor typo in the Preferences UI: "Folder Heirarchy" should be "Folder Hierarchy"
Created attachment 61640 [details] [review] updated patch Fixes a logic error in build_filename, boldifies Browser Views label, makes example path label in library prefs work. Doesn't fix the rip only one track problem.
Patch works again for ripping (still only one track gets ripped). Outstanding problems: 1) please, please replace g_atomic_int_set with gst_atomic_int_set, otherwise it will fail to compile on FC-4 (g_atomic_int_set only added in glib >= 2.10). 2) switching to CD source and back again will reset "Browse" status in library 3) can't see any kind of progress bar in status bar, just "Transferring tracks" in status bar. 4) ID3 tag isn't added after the track is ripped and added to the library (could be related to the fact that it stops after only one track)
Created attachment 61665 [details] [review] yet another patch (1) is fixed, as I replaced that stuff with just making queue_transfer only callable from the main thread. (2) is probably caused by the removable media (and daap) sources being derived from RBLibrarySource. The patch on bug 335185should fix that. (3) should be fixed, to a file accuracy. I still haven't added the in-file progress. As discussed on irc (4) was a missing "id3mux" in your pipeline. Should we add a warning if the pipeline doesn't have any GstTagSetter-implementing elements? * the completed signal is now emitted when you say "no" to replacing the file. As well as the accurate progress, we should probably also add a button to the prefs for editing profiles (like S-J).
Created attachment 61671 [details] New crash New crash with most recent patch, happens after end of ripping first track.
Alex, try commenting out the offending line in rb-removable-media-manager.c like so: data = g_async_queue_try_pop (priv->transfer_queue); if (data == NULL) { priv->transfer_total = 0; priv->transfer_done = 0; /*emit_progress (data->manager);*/ return; }
*** Bug 335407 has been marked as a duplicate of this bug. ***
Updated summary to make it clear that the patch does more than just CD ripping so that people don't file dupes on DAAP sharing.
Attachment #61665 [details] needs to be updated to take into account that attachment #61479 [details] has now been committed, otherwise it won't work against current CVS.
(In reply to comment #34) > Alex, try commenting out the offending line in rb-removable-media-manager.c > like so: Thanks, that makes the patch not crash. However I can only get it to rip more than one track if I include the "-D removable_media_manager" command-line option. If I don't it won't rip more than one track. Weird.
Yes, it's very strange that it is trying to dereference a known NULL pointer, should that really be: data = g_async_queue_try_pop (priv->transfer_queue); if (data == NULL) { priv->transfer_total = 0; priv->transfer_done = 0; emit_progress (manager); return; }
(In reply to comment #39) > However I can only get it to rip more than one track if I include the > "-D removable_media_manager" command-line option. If I don't it won't > rip more than one track. Weird. When I don't run it with the command-line option the status bar reads: "Transferring track 1 of 1" but with the "-D" option the status bar reads: "Transferring track 1 of 12"
Created attachment 61751 [details] [review] better patch Fixes the crash, adds a button to edit audio profiles and a few minor fixes. I can't reproduce the "only copy first track" bug, and I have no idea why it wouldn't happen if you use -d.
Setting library_layout_filename to "%tN - %tt" in gconf editor doesn't work. It resets the gconf key to blank, even if the prefs are never edited. I vote to make "%tN - %tt" the default for the "Title - Artist" filelayout rather than "%tn - %tt" because most programs pad track numbers to 2 digits with leading zero "0" for sorting ability.
With this new patch Edit button in the Library Preferences widget opens up the gnome-audio-properties, but if you click "Edit" within the new gnome-audio-properties dialog, it crashes: libglade-WARNING **: unknown widget class 'GMAudioProfileEdit'
Sound-Juicer used to be hit by the same bug if I'm not mistaken, a fix can be found there I guess ;)
Created attachment 61770 [details] backtrace To reproduce: 1. Import first track from album 2. Import again 3. At prompt to overwrite track - click No Also, I'm only able to import the first track from the album.
Even with my Makefile.am patch for building in a separate directory, with a clean build directory and a run of ./autogen.sh in the source directory, I still get compilation errors. This is strange because the Makefile contains $(top-builddir)/lib which is where: rb-marshal.h and it contains the "rb_marshal_VOID__INT_INT_DOUBLE" function. /home/alex/src/remote-cvs/gnome.org/rhythmbox/shell/rb-removable-media-manager.c: In function 'rb_removable_media_manager_class_init': /home/alex/src/remote-cvs/gnome.org/rhythmbox/shell/rb-removable-media-manager.c:212: error: 'rb_marshal_VOID__INT_INT_DOUBLE' undeclared (first use in this function) /home/alex/src/remote-cvs/gnome.org/rhythmbox/shell/rb-removable-media-manager.c:212: error: (Each undeclared identifier is reported only once /home/alex/src/remote-cvs/gnome.org/rhythmbox/shell/rb-removable-media-manager.c:212: error: for each function it appears in.) make[3]: *** [rb-removable-media-manager.lo] Error 1 make[3]: Leaving directory `/home/alex/build/rhythmbox-tag-writing/shell' make[2]: *** [all] Error 2 make[2]: Leaving directory `/home/alex/build/rhythmbox-tag-writing/shell' make[1]: *** [all-recursive] Error 1 make[1]: Leaving directory `/home/alex/build/rhythmbox-tag-writing' make: *** [all] Error 2
(In reply to comment #47) > This is strange because the Makefile contains > > $(top-builddir)/lib I meant $(top_builddir)/lib (where "top_builddir=..")
It needs a -I$(top_builddir)/lib in something that tells it it's to be added to the include flags. I can't tell at all if that is the case from your comment...
That's exactly what my patch (since committed) does, here is shell/Makefile.am (after the ripping/xfer patch): INCLUDES = \ -DGNOMELOCALEDIR=\""$(datadir)/locale"\" \ -DG_LOG_DOMAIN=\"Rhythmbox\" \ -I$(top_srcdir) \ -I$(top_srcdir)/lib \ -I$(top_srcdir)/metadata \ -I$(top_srcdir)/player \ -I$(top_srcdir)/rhythmdb \ -I$(top_srcdir)/widgets \ -I$(top_srcdir)/sources \ -I$(top_srcdir)/iradio \ -I$(top_srcdir)/podcast \ -I$(top_srcdir)/remote \ -I$(top_srcdir)/backends \ -I$(top_builddir)/remote \ -I$(top_builddir)/lib \ -I$(top_srcdir)/plugins \
Created attachment 61913 [details] [review] re fixes Hopefully fixes a few more issues
Issues still present with patch in attachment #61913 [details]: 1) The default schema for the library_layout_filename and the GUI setting for "Track - Title" still don't match. The former uses "%tN - %tt" (as I suggested as a default, i.e. "08 - Title.mp3") but the GUI setting still uses "%tn - %tt". This can be fixed in rb-library-source.c, replacing: {N_("Track - Title"), "%tn - %tt"}, by: {N_("Track - Title"), "%tN - %tt"}, 2) Still can't rip more than one track. 3) The crash mentioned in comment #46 still happens for me.
Created attachment 61918 [details] Backtrace of crash when clicking "no" This happens when I click "No" using patch on attachment #61913 [details].
When clicking on Edit inside the gnome-audio-profiles as in comment #44, get another crash, but different message: gnome-media-profiles-ERROR **: No such widget profile-name-entry aborting...
The reason it is only transferring one track is because rb-removable-media-manager.c:copy_entry() is defined as void and it should be gboolean and always return FALSE. http://developer.gnome.org/doc/API/2.0/gtk/GtkTreeModel.html#gtk-tree-model-foreach
Created attachment 61958 [details] [review] update patch * fixes the return-type problem with copy_track (comment 55) * should fix crash from comment 53 * adds accurate progress reporting * adds more library path/filename options (the ones from S-J that make sense) I've made a small modification to the edit-profile stuff, which is now identical to the S-J code. If this still has the problem mentioned in comment 54, I have no idea.
Doesn't compile with gtk-doc :( gcc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i686 -mtune=generic -fasynchronous-unwind-tables -Wcomment -Wformat -Wnonnull -Wimplicit-int -Wimplicit -Wmain -Wmissing-braces -Wparentheses -Wsequence-point -Wreturn-type -Wswitch -Wtrigraphs -Wunused-function -Wunused-label -Wunused-value -Wchar-subscripts -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wcast-align -Wall -Werror -std=gnu89 -o rhythmbox-scan .libs/rhythmbox-scan.o -pthread -pthread -Wl,--export-dynamic -pthread ../../shell/.libs/librbshell.a ../../sources/.libs/libsources.a ../../sources/.libs/libsourcesimpl.a ../../rhythmdb/.libs/librhythmdb.a ../../iradio/.libs/librbiradio.a ../../podcast/.libs/librbpodcast.a ../../remote/.libs/librbremote.a ../../player/.libs/librbplayer.a ../../metadata/.libs/librbmetadata.a ../../widgets/.libs/librbwidgets.a ../../lib/.libs/librb.a ../../daapsharing/.libs/libdaapsharing.a ../../plugins/.libs/librbplugins.a -ltotem-plparser -lhal -lnautilus-burn -lsoup-2.2 -lgnutls -lavahi-common -lavahi-client -lavahi-glib -lgpod -lnotify -ldbus-glib-1 -ldbus-1 -lgnomeui-2 -lSM -lICE -lbonoboui-2 -lgnome-keyring -lgnomecanvas-2 -lart_lgpl_2 -lpangoft2-1.0 -lgnome-media-profiles -lglade-2.0 -lgnome-2 -lpopt -lgtk-x11-2.0 -lgdk-x11-2.0 -latk-1.0 -lgdk_pixbuf-2.0 -lpangocairo-1.0 -lpango-1.0 -lcairo -lgnomevfs-2 -lbonobo-2 -lgconf-2 -lbonobo-activation -lORBit-2 -lgstbase-0.10 -lgstreamer-0.10 -lgmodule-2.0 -lgthread-2.0 -lxml2 -lmusicbrainz -lstdc++ -lm ../../bindings/python/.libs/rb.a -lpython2.4 -lpthread -ldl -lutil -lgobject-2.0 -lglib-2.0 -lz ../../shell/.libs/librbshell.a(rb-removable-media-manager.o): In function `error_cb':/usr/src/redhat/BUILD/rhythmbox-0.9.3.99.cvs200603242123/shell/rb-removable-media-manager.c:849: undefined reference to `rb_encoder_cancel' ../../shell/.libs/librbshell.a(rb-removable-media-manager.o): In function `do_transfer':/usr/src/redhat/BUILD/rhythmbox-0.9.3.99.cvs200603242123/shell/rb-removable-media-manager.c:907: undefined reference to `rb_encoder_new' :/usr/src/redhat/BUILD/rhythmbox-0.9.3.99.cvs200603242123/shell/rb-removable-media-manager.c:917: undefined reference to `rb_encoder_encode' collect2: ld returned 1 exit status Compilation of scanner failed make[3]: *** [scan-build.stamp] Error 1 make[3]: Leaving directory `/usr/src/redhat/BUILD/rhythmbox-0.9.3.99.cvs200603242123/doc/reference'
Works much better for me, thanks, all crashes gone and "%tN - %tt" works as expected. A few remaining issues: 1) still get crash upon editing an audio profile (this is with GNOME 2.10, so it probably doesn't affect 2.14 and later) 2) if I rip an entire CD, then attempt to rip it again, it asks me "Yes/No", I say No, then attempt to rip again it doesn't do anything. But every time after that I click "Import" it increments the number of tracks to transfer in the status bar by the number of tracks on the CD, e.g. if have 12, it says "Transferring track 1 of 24 (0%)", "Transferring track 1 of 36 (0%)", ... etc. Further, it appears that the status bar isn't reset after if I say "No" as above, it still displays "Transferring track 1 of 84 (0%)" and stays that way even when going from source to source.
Created attachment 61974 [details] Write date (Year) and genre metadata to encoded file I uploaded a patch on bug #335947 to support dates for CDs, here is a modified rb-encoder-gst.c to actually add the metadata to the file. (don't know how to add a patch to a patch?)
Though there are still some GUI issues and other issues that still need to get worked out, this seems to work pretty well now. I say we get it in to CVS and start getting others (people who don't want to patch their CVS) to test and get the rest of the issues (and any new ones that appear) worked out in HEAD. Thoughts?
I'm all for it. No major crashes anymore, can rip the entire CD. It would beat having to make sure the various patches I have don't overlap each other. But the developers James et al. would have to be happy to do so. I guess it all depends on how close we are to making a 0.9.4 release.
Another buglet here. Finally played with this on my network at work. If I right click on a song in a DAAP share, Copy is greyed out. From my understanding, I should be able to say Copy here and then Paste in my library. I was able to just drag that song to my Library and it worked correctly.
Ryan: that's probably due to bug 335185 (which has a msotly working patch attached).
I've committed this to cvs, with Alex's year fix. This will let us get some moe widespread user feddback on UI stuff, and it would be trivial to disable if we wanted to do 0.9.4 soon without it. Christophe: this should let you work on the ipod stuff if you want. Since that is bug 310774, work on it is probably better put there. I'm leaving the bug open for a little while, for reports of brokenness to go into.
Created attachment 62931 [details] [review] RBEncoderGst transcoding support Hi, this patch implements transcoding support in RBEncoderGst. It works here with gstreamer 0.10 but is totally untested with 0.8 (although i expect it to compile fine with 0.8).
Created attachment 62959 [details] [review] RBEncoderGst transcoding support the previous patch broke copy_track. This fixes it.
I haven't tested it yet, but I've got a few comments: The completion signal shouldn't be emitted when an error is handled. This is because the completion signal must never be emitted more than once, and the error handler can be run multiple times (messages waiting in the queue etc). The completion signal /should/ be emitted before the finaliser runs, however I had the warning+emission there instead of an assertion just in case. The program shouldn't abort for something trivially recoverable from. Checking the mime-type of the output from the encoding pipeline sounds like a good way to determine the correct profile, but it will have practical problems. If you want to encode an MP3, the correct pipeline would be "lame ! id3mux", however that doesn't output audio/mpeg, it outputs application/x-id3. I haven't figured out how to make this work properly.
(In reply to comment #67) > I haven't tested it yet, but I've got a few comments: great > The completion signal shouldn't be emitted when an error is handled. This is > because the completion signal must never be emitted more than once, and the > error handler can be run multiple times (messages waiting in the queue etc). I don't think you get messages once the pipeline has been set to NULL (_gst_cancel) if you don't explicitly flush the bus. But i may be wrong and i see your point. > The completion signal /should/ be emitted before the finaliser runs, Why should and not *must*? It is emitted when an error occurs. It's emitted when EOS occurs. The only case I can see now which doesn't emit the completion signal is rb_encoder_gst_cancel. Anyway with that check in the finalizer, you always emit the completion signal. So what's the point in not emitting it in _gst_cancel? What i'm trying to say is: if the completion signal is *in any case* emitted as a fallback in the finalizer, why not just emit it at the right place? > however I had the warning+emission there instead of an assertion just in case. Ahhh so I guess it shouldn't be emitted on _cancel, which makes perfectly sense. > The program shouldn't abort for something trivially recoverable from. Yeah the assertion is a bit too much ;) > Checking the mime-type of the output from the encoding pipeline sounds like a > good way to determine the correct profile, but it will have practical problems. > If you want to encode an MP3, the correct pipeline would be "lame ! id3mux", > however that doesn't output audio/mpeg, it outputs application/x-id3. > I haven't figured out how to make this work properly. GStreamer elements are categorized. lame is an encoder, id3mux is a muxer. Getting the encoder's src caps should work for every profile.
(In reply to comment #68) > > The completion signal shouldn't be emitted when an error is handled. This is > > because the completion signal must never be emitted more than once, and the > > error handler can be run multiple times (messages waiting in the queue etc). > > I don't think you get messages once the pipeline has been set to NULL > (_gst_cancel) if you don't explicitly flush the bus. But i may be wrong and i > see your point. You might be right; but in any case, there is nothing stopping the error signal being connected to twice, and each of the handlers calling _cancel. > > The completion signal /should/ be emitted before the finaliser runs, > Why should and not *must*? You're right, it should be *must*, but sometimes it doesn't happen. > It is emitted when an error occurs. It's emitted when EOS occurs. > The only case I can see now which doesn't emit the completion signal is > rb_encoder_gst_cancel. Anyway with that check in the finalizer, you always emit > the completion signal. So what's the point in not emitting it in _gst_cancel? > What i'm trying to say is: if the completion signal is *in any case* emitted as > a fallback in the finalizer, why not just emit it at the right place? As mentioned above, there is nothing stopping _cancel being called twice. Also, the error handler may be run after an EOS message is placed on the bus, but before it is processed. > > If you want to encode an MP3, the correct pipeline would be "lame ! id3mux", > > however that doesn't output audio/mpeg, it outputs application/x-id3. > > I haven't figured out how to make this work properly. > > GStreamer elements are categorized. lame is an encoder, id3mux is a muxer. > Getting the encoder's src caps should work for every profile. Are profile pipelines required to set the name of the actual encoding element to "enc"? If so, then feel free to ignore what I said. One idea for a handy plugin (I think it's on another bug somewhere) and a good test for this would be to add a "Transcode track" action that let you convert the file to a different format, and save it somewhere.
(In reply to comment #69) > You might be right; but in any case, there is nothing stopping the error > signal being connected to twice, and each of the handlers calling _cancel. And what if that happen? _cancel is guaranteed to run only once as it checks for the pipeling being already NULL (and so already canceled). Plus as of now it doesn't even emit the completion signal. Sorry i don't think I understand what you're trying to say here. > > > The completion signal /should/ be emitted before the finaliser runs, > > Why should and not *must*? > You're right, it should be *must*, but sometimes it doesn't happen. Well if it must, and it doesn't, is it a programming error in RBEncoderGST? > As mentioned above, there is nothing stopping _cancel being called twice. Yeah but _gst_cancel checks if it has already been called (by checking priv->pipeline). > Also, the error handler may be run after an EOS message is placed on the bus, > but before it is processed. What error handler? > Are profile pipelines required to set the name of the actual encoding element > to "enc"? If so, then feel free to ignore what I said. I think so. If not, it'd be just matter of iterating the profile bin searching for the encoder. > One idea for a handy plugin (I think it's on another bug somewhere) and a good > test for this would be to add a "Transcode track" action that let you convert > the file to a different format, and save it somewhere. Well, now i'm testing this with my ipod write patch. I know Christophe is working on it too. Unfortunately when i started a couple months ago i think he wasn't, and so now it seems we're both working on the same thing :)
« I know Christophe is working on it too. Unfortunately when i started a couple months ago i think he wasn't, and so now it seems we're both working on the same thing :) » A couple of months ago, I wasn't, if you had told earlier that you were working on that, I wouldn't have hacked before hearing from you ;) Anyway, I'm currently working on improving other things in the iPod source (context menu, eject, ...) and what I have done so far wrt writing is in bug #310774 (and that was only a few hours of work, so it's not that big a deal if we duplicated some work). If you have something working wrt writing to the iPod, it would be nice if you could post a work in progress patch to bug #310774 so that we can synchronize our efforts best (my iPod plans are to get copying to the iPod working, then looking at what needs to be done to copy playlists and podcasts, and then maybe look at real sync and transcoding, so if you have only done transcoding so far, our work is actually complementary)
I've committed the transcoding patch to cvs (post 0.9.4) with a few fairly trivial changes.
Created attachment 64200 [details] [review] transcoding support find the encoder iterating the profile bin rather than getting the element named "enc", as that won't work with user defined profiles. Notice i removed again the g_signal_emit from _finalize as you can't do that. At that point the object's refcount is already 0, so emitting a signal gives assertion errors.
I've committed this to cvs, thanks. (In reply to comment #73) > Notice i removed again the g_signal_emit from _finalize as you can't do that. > At that point the object's refcount is already 0, so emitting a signal gives > assertion errors. Good point.
That broke the build. I've committed the following fix. 2006-04-25 William Jon McCann <mccann@jhu.edu> * backends/gstreamer/rb-encoder-gst.c (profile_bin_find_encoder): Fix compiler error from last commit.
Created attachment 64286 [details] [review] podcasts transcoding Add support to transcode podcast entries
Created attachment 64523 [details] [review] emit the completed signal when a pipeline can't be built emit completed when copy_track, extract_track, and transcode_track fail before creating the pipeline
I've committed the second of the patches. The first isn't needed any more, as the issue was fixed in a better way by the patch from bug 330226.
When using Swedish the songs on a CD are called Okänd ... (Unknown) That is a unicode 'ä' I guess and it got messed up when transferred to the library. Also track 4 was named differently than all other tracks when I transferred them. Unfortunately I don't have internet at home right now so I can't give you all the details.
Ernst: was the 'ä' that got messed up the one in the file/folder names, or in the tags? With respect to track 4, what was it called and what should it have been called?
This is in the tags, but it's reflected in the filenames I think. Since I still don't have net at home this is only improvised. It was something like this: Okänd Sång 1 Okänd Sång 2 Okänd Sång 3 Okänd Sång 4 Okänd Sång 5 DnD all songs to library, And became: Ok$€nd S@£ng 1 Ok$€nd S@£ng 2 Ok$€nd S@£ng 3 04 - Ok$€nd S@£ng.mp3 Ok$€nd S@£ng 5
Being bold and closing this bug since the basic functionality has been in CVS for a while and is now compiled by default. New bugs should be filed for new issues. I opened bug #350034 for the issue reported above by Ernst.
*** Bug 423183 has been marked as a duplicate of this bug. ***