GNOME Bugzilla – Bug 76524
Add tag editing support
Last modified: 2009-02-27 11:08:23 UTC
Add tag editing support to MonkeySound
this would be a really interesting feature, as other program (say.. easytag?) aren't good at it, with rm it would be easy to see what tag have to be edited, simply: using the Browser and selecting "unknown", doulbe click on the tag and edit it stright in the list.. would be really very very cool :o)
*** Bug 122918 has been marked as a duplicate of this bug. ***
*** Bug 87600 has been marked as a duplicate of this bug. ***
*** Bug 151073 has been marked as a duplicate of this bug. ***
the ability of the browser to actually sort songs is very limited by this, for example the genre field is almost useless unless the user can specify what genre they consider the music to be of.
Well, I'd tend to mark this bug as WONTFIX since monkey-media is dead in the 0.9 branch and GStreamer will be used for tag editing ;) The 0.9 branch has basic tag editing support for flac and mp3 but it needs work.
*** Bug 154227 has been marked as a duplicate of this bug. ***
*** Bug 169845 has been marked as a duplicate of this bug. ***
When will tag-editing be enabled by default? IMHO we should enable it during 0.9.x, if we don't get it working until 1.0.0, just disable it by default when releasing.
May be when it works? ;-) I built rhytmbox with tags editing (0.9.0, gstreamer backend) and whenever i try to write id3 tags it would truncate the file to 0 size. I guess the problem is in gstreamer. May be tag-editing should enable itself if working version of gstreamer is detected? (I have gstreamer 8.11.0, the rpm from gstreamer's own repository). Also as far as I can see this version (as well as CVS HEAD) only supports tag writing for mp3 or flac. But it's not very obvious from the file properties dialog which renders the edit fields with the same color wether on not tag writing is supported.
Currently tag-editing only work for mp3 and flac files, and it occasionally destroys files. Perhaps after creating the tag-edited temporarly file, we could try to play it with "... ! decodebin ! fakesink", and if that can't then we know tag-writing failed (and so wouldn't replace the original).
after some experimentation i've found that the problem is both mine and rhytmbox's: i had removed gstreamer mp3 support plugins while building rhythmbox this morning (i had a mess of different versions of gstreamer available from different repos and didn't notice that) rhythmbox built with tag writing support doesn't check if for either of the two supported for writing tag formats (id3 and flac) there is a corresponding plugin. if there isn't it will try to write tags and fail, truncating the file. I guess the correct(er) approach would be to check if the necessary plugins are available when doing this bit: add_supported_type (md, "application/x-id3", rb_add_id3_tagger, "MP3"); add_supported_type (md, "audio/mpeg", rb_add_id3_tagger, "MP3"); add_supported_type (md, "application/ogg", NULL, "Ogg"); add_supported_type (md, "audio/x-flac", rb_add_flac_tagger, "FLAC"); add_supported_type (md, "audio/x-mod", NULL, "MOD"); add_supported_type (md, "audio/x-wav", NULL, "WAV"); like it is now it adds tagger-functions which would either create a tagger element or return NULL. but if you would do something like: ... tagger_func = (id3_plugin_present) ? rb_add_id3_tagger : NULL; add_supported_type (md, "application/x-id3", tagger_func, "MP3"); ... also in the bit that uses the tagger function it should check if it has returned NULL (better safe then sorry - this is something which actually shouldn't happen). I think I'll add this two bits after lunch (hope I can figure out the gstreamer bits). .... so i installed the -extra-audio rpm and now it reads the tags and doesn't crash when i change them. It saves the tags I have entered in its database and displays them when playing the song. But it strips all tags from the file itself. this means what you propose (try to play tmp file) won't help - the file is perfectly playable, it just has no tags anymore. Again, could be my fault of course.
Created attachment 53654 [details] [review] check tagger could be created This patch makes it abort tag-writing, if the tagger element can't be created. This should also stop it truncating the file, when the necessary plugin isn't installed.
Created attachment 53657 [details] [review] check write support is backed by corresponding gst plugin this patch makes sure the tag support will be enabled only for media types for which the necessery plugins exist. (doesn't obsolete previous patch for element creation could have failed for another reason).
Created attachment 53672 [details] [review] another patch This patch includes both the two previous ones, and adds a post-write check to ensure that the file hasn't been corrupted and amde unplayable. It should be fairly easy to add checks to ensure that the tags have been written correctly, although this is made more compilated by the fact that some file type don't support any/all metadata.
I'm trying to investigate why it removes the tags in the first place, but all the gstreamer stuff is greek to me. BTW, can you confirm that for you it doesn't remove the tags (if you look at the file with some other software, e.g. id3info or easytag)? we could then compare our setups to pinpoint the offending part. I run rhythmbox 0.9.0 with your latest patch (which applies cleanly) and gstreamer 0.8.11 from nrpms repository (gstreamer, gstreamer-plugins, gstreamer-plugins-audio) + gstreamer-plugins-extra-audio from gstreamer repository (this is the only relevant bit of gstreamer that nrpms doesn't provide, but i don't think there should be anyproblem - gstreamer in nrpms is a standard build).
Now that I've actually got it writing the tags, yes it is stripping them out. Previously it was failing to write the tags, and removing the temporary silently. I don't know a that much about gstreamer tag writing, but I'll check how Sound Juicer adds tags to make sure we're not doing something stupid.
while you're at it: like many people posting to this and duplicate bugs I have hundreds of mp3/ogg/wav files with inconsistent/wrong/missing metadata. They mess the database pretty bad, while it is the main reason for me to use rhythmbox. May be the better way to deal with the metadata/tags is the following: let the song properties be editable whether they can be written or not, so user can organise her collection, if tags are writable - wonderfull - save them. If not - show somehow that the database and the file tags are out of sync. When that's the case and the tags are writable (e.g. user has upgraded the corresponding plugins) - provide the interface (in properties dialog?) to sync them.
Sound Juicer creates the encoder pipeline automagically using "audioconvert" and then searches the result for an element implementing GstTagSetter interface. I wonder if audioconvert can build "audio pass through" pipelines which would leave encoded audio packets as is and but still allow to change the metadata?
That's basically what RB does now, most of the differences stem from the fact that it's just changing the tags and not encoding. The rb_add_*_tagger functions return an element that implements GstTagSetter (flactag and id3mux). On furthur inspection the flac tagger works perfectly for me - it's just the td3 tagger that isn't working. I'm going to do some tracing of what exactly is happening with the id3 tagger, and I'll report what I find here.
I'm doing the same, hopefully looking in other places ;-) Have you seen retag example in gstreamer? it's in examples/retag/ <blockquote> This example shows how to use interfaces and the tag subsystem. It takes an mp3 file as input, and makes an ogg file out of it. While doing this, it parses the filename and sets artist and title in the ogg file. It assumes the filename to be "<artist> - <title>.mp3" </blockquote> despite what the comment sais it doesn't actually make an ogg (the comment is copy pasted from the neighbour example - transcode.c). they insert "id3tag" element in the recoding pipeline. I assume this element receives tags on one end and spits on the other. they don't use "spider" like rhythmbox. i built the example and it fails miserably just like rhythmbox (deletes the tags altogether). I think we can safely file a bug to gstreamer. (or check if id3tag from gstreamer 0.9 works?)
which i did: http://bugzilla.gnome.org/show_bug.cgi?id=319316
Patch committed to cvs.
i tried to implement the solution from http://bugzilla.gnome.org/show_bug.cgi?id=319316 gstreamer bug in rb, but it still strips tags. Could someone have a look at the patch (attaching...)
Created attachment 53977 [details] [review] not working solution to id3tag edititng this is the patch which doesn't work and I can't see why.
The patch works for me, and successfully sets the ID3 tags.
Patch committed, ID3 tag writing seems to work well now. However it may write v2.4 tags which a lot of non-libid3tag software can't read.
Some interface notes (1) pressing Next / Previous in properties discards whatever changes you made to the current tag (2) no indication of whether tags can be edited and saved to file (3) genres box could suggest genres, e.g. standard id3v1 genres, genres from my library etc. Autofill would be better then combo box here. (4) It seems that the genre doesn't get saved - i gave up entering genres because they keep returning to whatever there was originally. (5) The usual "batch editing, more tags (year? total album tracks? custom?), bla" wishlist. I'd like to work on these after consensus is reached on wether RB continues to use GStreamer for tagging or not.
I think you can go ahead with polishing the UI, even if we choose to use something different to write tags, the UI shouldn't change, so your work wouldn't get lost. (except maybe for 4) For 3), I'd look at what sound-juicer does wrt genres for extra consistency, I don't think s-j exposes all the "standard" genres. And for 5), if it adds much to the UI, it may need to be discussed first.
The metadata backend can be made a compile-time options, as the playback backend is (although we only have one playback backend at the moment). That means we could add a taglib backend, and then choose the default based on what we think is best. But as Christophe said, the UI can be improved regardless of the backend.
Created attachment 54053 [details] [review] grey out non-editable text fields in track properties solves (2) from my list
About (1) what do you think should happen: (a) changes get saved as soon as user clicks Next/Prev (b) if changes were made and user clicks Next / Previous confirmation dialog appears (c) changes are remembered and only applied when Close is pressed I prefer (b), I would also like to add a confirmation dialog to save on close. I don't like that saving happens on "Close" button. Shouldn't there rather be Apply Cancel Previous Next ?
Created attachment 54055 [details] [review] check for id3tag instead of id4mux even though they come from the same plugin, it's more correct for the element rb actually uses.
I've committed the "id3tag" patch. Simply making the background gray looks horrible with any theme that does not use that exact shade of gray for window background (especially those where the background isn't gray at all). One of the other bugs (I can't find which one atm) said that we should use GtkLabels for uneditable propreties. That will make everything correct, but will make setting up the dialog much more complicated, because we can't put it into glade.
Well, I tried to make "grey" theme agnostic - I copy fg/bg into text/base. But these styles seem to differ for GtkEntry and other chrome. I think I can copy fg/bg from some GtkLabel.
Wouldn't gtk_editable_set_editable (widget, FALSE) do the right thing to disable non-editable fields? (though a label would be even nicer but more complicated to code).
That's what we currently do, but (in most themes) it looks exactly the same.
Which would be theme bugs. They should look different... (maybe disabling the label attached to the entry would help show that the entry is disabled).
Is there any theme where non-editable entries have different colours then the editable ones? I think it's the theme engine that would have to take care of that because non-editable doesn't exist as a separate style. What does exist is "non-sesitive" style, but setting entries to nonsensitive will result in that people can't select text in them (and again not every theme uses different colours for non sensitive GtkEntries). That's why I tried to circumvent the problem by copying the fg/bg colours to text/base. But apparently these colours are wrong shades of grey. I'll try to steal from GtkLabel now.
*** Bug 303908 has been marked as a duplicate of this bug. ***
Rhythmbox doesn't use Monkey Media anymore AFAIK, switching component to "User Interface". There is ongoing work at bug #309609 to get tag editing working for gstreamer 0.10. Currently tag editing works but is largely broken for gstreamer 0.8, see bug #323658. Perhaps this bug could be closed as a duplicate or retitled to be more about the user interface side of tag editing.
*** Bug 333468 has been marked as a duplicate of this bug. ***
Tag support has been added for ID3 tagging in: bug #309609. However it can be a lossy in that tags unknown to gstreamer are removed in the retagging, see: bug #334375. Also we currently don't have OGG retagging ability, see bug #335635.
Removing blocker bugs that were empty in any case.
*** Bug 336859 has been marked as a duplicate of this bug. ***
*** Bug 338374 has been marked as a duplicate of this bug. ***
Eh... I was hit by the "can't edit but the entry looks like I should" bug, so I thought I'd both report this ui glitch as a bug and ask for the editing as an enhancement. Now I know better. The problem I still see is that the file is a .flac, to which I have both read&write permission... and according to some comments here, it should work! This is with debian unstable's 0.9.8-1 package ; is there something I can do?
(In reply to comment #47) > The problem I still see is that the file is a .flac, to which I have both > read&write permission... and according to some comments here, it should work! > This is with debian unstable's 0.9.8-1 package ; is there something I can do? I think those comments were probably referring to gstreamer 0.8. As I understand it, the flactag element (that retags flac files) has not yet been ported to gstreamer 0.10. Can't find a bug on that right now, but if one doesn't exist, one should be opened about that.
Well, from comment #6 : "The 0.9 branch has basic tag editing support for flac and mp3 but it needs work." (that was in early 2005!) And 0.9.8 is in this branch... I'll check in gstreamer if I find it.
(In reply to comment #49) > Well, from comment #6 : "The 0.9 branch has basic tag editing support for flac > and mp3 but it needs work." (that was in early 2005!) > And 0.9.8 is in this branch... I'll check in gstreamer if I find it. 0.9.x of *rhythmbox* does have support for flac tagging, but you need to be using gstreamer 0.8 to take advantage of it.
(In reply to comment #50) > 0.9.x of *rhythmbox* does have support for flac tagging, but you need to be > using gstreamer 0.8 to take advantage of it. i.e. rhythmbox needs to be compiled against gstreamer 0.8. But I wouldn't recommend that because gstreamer 0.8 is way more deficient than 0.10 in so many other areas.
Yes, that's it ; filled as bug #413841.
Tag editing is working fine for me with latest rhythmbox for Ogg and MP3. I'm closing this bug (and it's also a bit messy). Feel free to open a new bug if you have a particular issue with tag editing. Please also add a way to reproduce the bug (attaching a particular file if needed). Thanks.