Add support for TagLib::MP4 (AAC, ALAC) #3

Closed
sfsekaran opened this Issue Oct 17, 2011 · 28 comments

Comments

Projects
None yet
4 participants

No description provided.

Owner

robinst commented Oct 19, 2011

This is also blocked by the SWIG bug in #1.

Owner

robinst commented Mar 26, 2012

It's no longer blocked and is next on my list after FLAC (which is nearly done).

I wish I could help.

Collaborator

jacobvosmaer commented Sep 7, 2012

I would like to help! I started an experimental branch which currently has a non-trivial test that passes in my local development environment.

Let me know if I could be of help.

Owner

robinst commented Sep 14, 2012

Very nice! I also already had a start (sorry for not publishing and causing duplicate work).

The problem where I was stuck last was wrapping TagLib::MP4::Tag::itemListMap. It returns an ItemListMap which is a typedef:

typedef TagLib::Map<String, TagLib::MP4::Item> ItemListMap;

The problem is that we can't just convert it to a Ruby hash, as the only way to add a custom item is to call insert on the returned map. Converting it to a hash was ok for e.g. XiphComment, as it also provides other accessors such as addField.

So it needs to be wrapped, but I haven't yet figured out how to do it. Any ideas?

Collaborator

jacobvosmaer commented Sep 14, 2012

I don't mind the duplicate work; it helped me to get to know your project a little.

I also don't know yet what to do about the ItemListMap. Would it be an option to expose the C++ API through SWIG and then augment the resulting Ruby objects with Hash's roughly corresponding instance methods? E.g. #[], #[]=, #each, #length, #empty?, #has_key?, #delete and their aliases.

Owner

robinst commented Sep 14, 2012

Yes, I think that would be the way to go. As a first version, I think even just having the C++ API properly exposed would be enough.

BTW, I started watching your fork. Don't know why I wasn't already, must have missed the notification when the fork happened.

Collaborator

jacobvosmaer commented Sep 24, 2012

I have made a little progress and I now have some more concrete thoughts about the C++ API of ItemListMap. What about the following:

  • rename begin() to to_hash, and collect the contents of the Iterator in a Ruby hash in a typemap;
  • ignore end();
  • unravel the Iterator returned by find and collect its contents in a Ruby array;
  • ignore operator[] and operator=; and leave the other method signatures as is.

What do you think?

Owner

robinst commented Sep 29, 2012

Sorry, did not yet have time for this, but will look at it ASAP. Looks promising!

Owner

robinst commented Sep 30, 2012

Good work so far! See commit comments.

  • rename begin() to to_hash, and collect the contents of the Iterator in a Ruby hash in a typemap;

I'd implement to_hash directly in an %extend, see the other files for examples. I think when we return a VALUE there SWIG should not try to wrap the result value. This way, begin() can stay as it is or we can ignore it.

  • ignore end();

Yes, probably a good idea.

  • unravel the Iterator returned by find and collect its contents in a Ruby array;

I'd change it to return the value, or nil if not found (it only finds one result). This way we could also alias [] in Ruby to find.

  • ignore operator[] and operator=; and leave the other method signatures as is.

Yes.

So, the following aliases could be defined in Ruby:

  • [] for find
  • []= for insert
  • include? and has_key? for contains
  • delete for erase

Any missing?

Collaborator

jacobvosmaer commented Sep 30, 2012

Sounds like a good plan! I'll write some tests and continue in that direction.

Owner

robinst commented Dec 5, 2012

@jacobvosmaer Any updates on this?

Collaborator

jacobvosmaer commented Dec 5, 2012

Thanks for asking! I have been stuck for some time trying to add methods to ItemListMap using SWIG's %extend directive; tonight I finally made some new progress. I have added a fetch method to ItemListMap in taglib_mp4.i, with some additional wrapper code in taglib/mp4.rb. I think my next steps will be to add memory management stuff to free_taglib_mp4_file and to add the TagLib::MP4::Item class into the mix.

Getting the %extend to work on the template was kind of frustrating; I hope things get a little easier from here on.

Collaborator

jacobvosmaer commented Dec 29, 2012

I cleared another hurdle: talking to a Ruby Item-instance after its corresponding C++-instance was freed by ItemListMap#erase now raises an exception instead of causing a segfault. This was another tough nut to crack.

Collaborator

jacobvosmaer commented Dec 30, 2012

I am currently trying to pry apart the different constructors of TagLib::MP4::Item. I have tried the following:

%rename("from_string_list") TagLib::MP4::Item::Item(const StringList &);
%include <taglib/mp4item.h>

To my surprise, the effect of this is that Item(const StringList &) becomes the only constructor in taglib_mp4_wrap.cxx:

  SwigClassItem.klass = rb_define_class_under(mMP4, "Item", rb_cObject);
  SWIG_TypeClientData(SWIGTYPE_p_TagLib__MP4__Item, (void *) &SwigClassItem);
  rb_define_alloc_func(SwigClassItem.klass, _wrap_from_string_list_allocate);
  rb_define_method(SwigClassItem.klass, "initialize", VALUEFUNC(_wrap_new_from_string_list), -1);

Any thoughts? Do you think I am looking in the right direction?

Owner

robinst commented Jan 5, 2013

I'm not sure if SWIG supports renaming only some of the overloaded constructors. Any warnings? Have you tried with TagLib::StringList (fully-qualified)? What happens when you rename all of them?

The documentation doesn't say much about overloaded constructors: http://www.swig.org/Doc2.0/SWIGDocumentation.html#SWIGPlus_overloaded_methods

Collaborator

jacobvosmaer commented Jan 6, 2013

I am not getting any warnings related to Item. When I use the fully-qualified name, SWIG silently ignores the %rename. When I rename all of the constructors, SWIG chooses TagLib::MP4::Item::Item(int), even if I reorder the %rename directives. I got the idea to use %rename in the first place based on this:
http://www.swig.org/Doc2.0/Ruby.html#Ruby_nn20

I am going to try if maybe I can sneak in a new Ruby class method on TagLib::MP4::Item instead using an %extend.

Collaborator

jacobvosmaer commented Jan 26, 2013

I got some of the constructors to work and I have been looking at ItemListMap::insert(). I am starting to become concerned about the memory management situation.

  • The free-function for MP4::File is already reaching quite deep, and we are not even cleaning up CoverArtLists yet.
  • The behavior of ItemListMap#delete and #erase is IMHO unintuitive from a Ruby (GC'ed) point of view, and I do not see how to make '#insert' behave much better.

This makes me wonder if creating 9 pairs of fetch and insert methods on ItemListMap, which would accept Ruby objects, would be the lesser evil. E.g.

TagLib::MP4::File.new('track.m4a') do |mp4|
  mp4.tag.item_list_map.insert_string_list("\u00A9cmt", ['Comment'])
end

We are currently heading in this direction:

TagLib::MP4::File.new('track.m4a') do |mp4|
  item = TagLib::MP4::Item.from_string_list(['Comment'])
  mp4.tag.item_list_map.insert("\u00A9cmt", item)
end

What do you think?

Owner

robinst commented Feb 9, 2013

I think it's a good idea to have insert_string_list, etc, for ease of use.

On the other hand, the Item API can also be useful, e.g. for passing a list of them around and then later inserting them.

So, could we keep both?

Or are you thinking about not exposing MP4::Item at all? Wouldn't this make certain things inacessible, e.g. AtomDataType atomDataType () const on MP4::ITEM?

Collaborator

jacobvosmaer commented Feb 10, 2013

I was indeed thinking of not exposing MP4::Item at all, but I had not thought about Item::atomDataType and Item::isValid yet.

What bothers me about exposing MP4::Item is the following scenario:

TagLib::MP4::File.new('track.m4a') do |mp4|
  old_title = mp4.tag.item_list_map.fetch("\u00A9nam")
  # the call to #insert forces us/taglib-ruby to unlink old_title, because it will no longer 
  # get cleaned up by the freefunc of MP4::File
  mp4.tag.item_list_map.insert("\u00A9nam", TagLib::MP4::Item.from_string_list(['New title']))
  # the next line will raise an exception: ObjectPreviouslyDeleted
  puts old_title.to_string_list.first
end

What I don't like about the above scenario is that the exception makes no sense to an unsuspecting Ruby user. Does this make sense or am I missing a way around this? Or am I seeing too much of a problem in this?

Collaborator

jacobvosmaer commented Feb 19, 2013

On second thought, I see no reason not to expose the C++ class hierarchy. It would not stop us from adding Ruby accessors later.

I have captured the ItemListMap#insert scenario I was hinting at above in a test.

Owner

robinst commented Feb 23, 2013

We also have the behavior in other methods, e.g. TagLib::ID3v2::Tag#remove_frame. I don't think we can do much about it (we wrap a C++ library after all), we just have to make sure this is properly documented in docs.

By the way, very nice work, keep it up. Do you have an estimate on when you'll send the big pull request? :)

Collaborator

jacobvosmaer commented Feb 23, 2013

Thanks for the encouragement!

I currently see three topics I would like to work on before submitting the pull request:

  • documentation;
  • typemaps for CoverArtList and ByteVectorList;
  • tests and a wrapper for CoverArt.

I hope to finish these three things some time next month.

@robinst robinst added a commit that referenced this issue Apr 8, 2013

@robinst robinst Merge pull request #20 from jacobvosmaer/mp4
Add support for TagLib::MP4 (AAC, ALAC) (addresses #3)
a7b44a3
Owner

robinst commented Apr 9, 2013

So, thanks again to @jacobvosmaer for doing all the hard work to make this happen. Would you like to be added as a collaborator on the repository?

I'll leave this open until this is in a release, maybe I'll have the time to do it on Friday.

Collaborator

jacobvosmaer commented Apr 10, 2013

I would be honored. :)

Owner

robinst commented Apr 20, 2013

Done :). Would be nice if you would still create pull requests. (Maybe I should also start doing things as pull requests, hmm.)

By the way, just waiting on some feedback from other issues now before rolling the release.

Owner

robinst commented Apr 26, 2013

Ok, this is now released in taglib-ruby 0.6.0: http://rubygems.org/gems/taglib-ruby

robinst closed this Apr 26, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment