GNOME Bugzilla – Bug 323693
[PATCH] [lame] accept track-count tag
Last modified: 2006-03-13 10:16:44 UTC
Lame's CVS HEAD added support for putting a total track count into mp3s' id3v2 tags about a month ago. This patch adds support to gstlame to accept the "track-count" tag and pass it on to libmp3lame. It's backwards compatible, since it uses the same id3tag_set_track call, and older versions of lame will just drop the extra information on the floor. That means this patch can be applied immediately, even though it "depends" on an unreleased feature of lame.
Created attachment 55832 [details] [review] Patch as described in initial report. Applied cleanly to CVS HEAD today.
Thanks for the patch. Only had a quick look at it, but that trackNumInfo structure with file scope doesn't look like it would work safely with multiple instances of the lame element to me (or would it?).
Ooh, I hadn't thought of that. It actually would work, so long as all the instances ran in the same thread. If the two elements could be executing concurrently, though, they could smash each others' trackNumInfo. I'll look into making it thread-safe in the next couple days. I wasn't really happy with having to use a global structure anyway. The data only needs to exist for the duration of the call to gst_lame_set_metadata, but I didn't see an easy way to get it to be local to that stack frame and available to gst_lame_set_track and gst_lame_set_trackcount, whose one argument was already used up by the lame_global_flags pointer. Would a palatable solution be to wrap the lame_global_flags pointer in another struct that included the track info, and then wrap all of the lame id3tag_* functions with simple wrappers that pulled that pointer back out again? (It's 4:30am, so I appologize if I've made less than perfect sense. I'll look at this with fresh eyes later.)
Created attachment 56618 [details] [review] Re-entrant version of the previous patch This new patch should aleviate any concerns about re-entrancy. As a trade-off, it has to do a lot more wrapping of functions and data structures, as mentioned in passing at the end of my previous post. There is one note to the maintainer (grep MAINTAINER: gstlame.c) because the intentions behind one line of code weren't clear, and I had to try to map those intentions onto my modified code. I think I chose correctly, but I left the alternate interpretation in a comment, just in case. Also, I've been brainwashed by Paul Graham into loving macros, and I think that my use of the DEFINE_GST_LAME_ID3TAG_WRAPPER macro makes the code more concise, readable, and comprehensible. If this is not the consensus view, however, I'd be happy to do the grunt work of hand-expanding all the macro invocations and then posting a new patch.
Patch is generally okay, but: * gst_lame_id3tag_set_track() has 'static char str[6]', which is very easy to fix in this context by just removing the static keyword * as far as I can tell the code only works if the TRACK_COUNT field comes before the TRACK_NUMBER field in the tag list. Not really sure how to best get around the second issue. Maybe we should just special-case track count and track number and avoid rewriting all the other stuff.
> gst_lame_id3tag_set_track() has 'static char str[6]' Woops. Oversight. Absolutely right about removing the 'static' keyword. > only works if the TRACK_COUNT field comes before the TRACK_NUMBER I don't see why that would be. a TRACK_NUMBER field will trigger a call to gst_lame_set_track, which just sticks the track number in the TagInfo struct it's passed and returns. Likewise for TRACK_COUNT. Then, after all of the functions in tag_matches[] have been called and all available track info has been collected, a call to gst_lame_id3tag_set_track (as opposed to gst_lame_set_track -- I probably could have picked better names) actually puts together the proper argument to id3tag_set_track and passes it in. The order of the calls to the other two functions shouldn't matter. Am I missing something?
No, you're absolutely right, my oversight. The patch is fine, thanks for your work on that. However, we're probably not going to apply it, since the consensus that has recently emerged is that tag writing is going to be removed from the lame element entirely and moved to special-purpose tag-writing elements. This allows us to support a wider range of tags and to provide decent unicode support. See bug #329184 and the other lame-related bugs in bugzilla for a range of issues. Sorry for the bad timing. I hope that doesn't discourage you from contributing patches in the future :)