Setting a Playlist ID to an M4A file does not work properly.

[dominicap]

Hi!

First off, I want to say thanks for this great wrapper for TagLib! However, there is a problem when trying to set a playlist ID to a m4a file. If I execute the code below:

TagLib::MP4::File.open("song.m4a") do |tune|
  item = TagLib::MP4::Item.from_int(123456789)
  tune.tag.item_list_map.insert('plID', item)
  tune.save
end

The playlist id then gets assigned a long random number which is not the number supplied (In this case I checked using mp4info which yielded 72057594161384725 for the playlist id, using a random sample m4a).

Its quite strange as it works on occasion, but more times than not it fails and stores a very large, random number for the song. Any help would be appreciated.

Thanks.

[jacobvosmaer]

What exact numbers are you using? I wonder if this is some sort of int overflow problem.

Can you give an example of a number that gets stored correctly, and one that does not?

[dominicap]

@jacobvosmaer Generally, I am using numbers that are around 9 digits long as I am using the iTunes Search API to tag m4a files. However, even when using the number "1" as the playlist id, it places the number "2316958406050578433" for the playlist id. I actually could not replicate a number that gets stored correctly, thus, the one time I did get it working might have unfortunately been a fluke.

[jacobvosmaer]

I'm not an M4A expert (wrote the person who added M4A support to this gem :) ) but... looking at https://github.com/taglib/taglib/blob/46483948413c1330c036240ed89429a1c6905843/taglib/mp4/mp4tag.cpp#L84-L85 I get the distinct impression that we need a TagLib::MP4::Item.from_long_long function for plID atoms in M4A tags.

That would explain the garbage results you're seeing. Int goes in, gets stored, gets decoded as long long -> garbage.

[jacobvosmaer]

Sounds fixable but it may take me a while, I'm a little rusty working on this code.

[robinst]

@jacobvosmaer sounds like that's the problem, yeah. Would need to add a wrapper for TagLib::MP4::Item::Item(long long) here: https://github.com/robinst/taglib-ruby/blob/master/ext/taglib_mp4/taglib_mp4.i#L201

[jacobvosmaer]

Yep, and a test here. https://github.com/robinst/taglib-ruby/blob/master/test/mp4_items_test.rb#L103

The part of this I'm rusty at is setting up SWIG, using the correct TagLib header files (so that the generated wrapper code targets what is it, TagLib 1.7?) etc. And I switched from Homebrew to MacPorts.

Stuff like that. :) I can figure it out, I just need to find a moment to putter with this.

[robinst]

👍. I haven't tried this, but can we just run swig against a local copy of the headers from a checkout/download of the specific taglib version?

[jacobvosmaer]

I've been trying so set up my development environment with macOs 10.12 and MacPorts. Everything compiles but I get a number of odd test failures.

Failure: test: TagLib::FLAC::File audio properties should contain basic information. (FlacFileTest)
/private/tmp/taglib-ruby/test/flac_file_test.rb:40:in `block (3 levels) in <class:FlacFileTest>'
/private/tmp/taglib-ruby/test/flac_file_test.rb:51:in `instance_exec'
/private/tmp/taglib-ruby/test/flac_file_test.rb:51:in `block in create_test_from_should_hash'
<212> expected but was
<209>

And similar for Ogg and MP4. Sounds like a TagLib version mismatch to me.

And a different one:

Failure: <""> was expected to be nil.
test: A new ID3v2::Tag should have nil for string attributes. (TestID3v2Tag)

[jacobvosmaer]

Ah, but the tests pass OK on Ubuntu 16.04. Will use that machine.

[jacobvosmaer]

Well. I've been trying this change:

diff --git a/ext/taglib_mp4/taglib_mp4.i b/ext/taglib_mp4/taglib_mp4.i
index 7016e35..0d48b51 100644
--- a/ext/taglib_mp4/taglib_mp4.i
+++ b/ext/taglib_mp4/taglib_mp4.i
@@ -203,6 +203,10 @@ namespace TagLib {
     return new TagLib::MP4::Item(n);
   }
 
+  static TagLib::MP4::Item * from_long_long(long long n) {
+    return new TagLib::MP4::Item(n);
+  }
+
   static TagLib::MP4::Item * from_bool(bool q) {
     return new TagLib::MP4::Item(q);
   }
diff --git a/test/mp4_items_test.rb b/test/mp4_items_test.rb
index 543d6df..c59c2ca 100644
--- a/test/mp4_items_test.rb
+++ b/test/mp4_items_test.rb
@@ -107,6 +107,12 @@ class MP4ItemsTest < Test::Unit::TestCase
         assert_equal -42, item.to_int
       end
 
+      should "be creatable from a long long" do
+        item = TagLib::MP4::Item.from_long_long(1234567890)
+        assert_equal TagLib::MP4::Item, item.class
+        assert_equal 1234567890, item.to_long_long
+      end
+
       should "be creatable from a boolean" do
         item = TagLib::MP4::Item.from_bool(false)
         assert_equal TagLib::MP4::Item, item.class

But it causes all hell to break loose.

If I run rake after only applying that change I get a failing test because Item.from_long_long does not exist.

Then if I run rake clean and then rake, I get loads of errors on the new taglib_mp4_wrap.cxx file. My guess is that the generated wrapper code on my system is incompatible with the other wrap.cxx files in the repository. :(

[jacobvosmaer]

Would it make sense to have a Dockerfile that sets up a container with exactly the right swig and taglib versions?

[jacobvosmaer]

Looking at the headers of the wrap.cxx files they were generated with swig 3.0.7. @robinst what taglib version are we using at the moment?

I can write a dockerfile that installs taglib and swig from source at the exact required version. That should make it easier to recompile during development. And it beats trying to get the right versions installed on random developer's operating systems.

[jacobvosmaer]

OK. So TagLib 1.7 does not support MP4 items from 'long long' integers.

https://github.com/taglib/taglib/blob/v1.7.2/taglib/mp4/mp4item.h#L49-L53

Seems 'long long' support was introduced in TagLib 1.8.

@dominicap as long as taglib-ruby is based on TagLib 1.7, I don't think we can solve this issue.

[dominicap]

@jacobvosmaer Thats a bummer, hopefully taglib-ruby will be updated to be based off a more recent version of TagLib. Thanks for trying to help though!

[robinst]

@jacobvosmaer Using Docker to pin the version (and install swig) is a really good idea. Let's move the discussion about updating to a newer version of TagLib to the just opened #78.