Get TagLib 2.0.1 working #143

jacobvosmaer · 2024-04-11T10:36:53Z

This pulls together various commits into something reasonably clean that:

uses TagLib 2.0.1
passes our test suite

jacobvosmaer · 2024-04-11T10:43:50Z

@robinst what do you think about merging main into taglib-2? That would shrink this PR to about 4-5 commits.

jacobvosmaer · 2024-04-11T11:51:05Z

This isn't quite there yet; we need to fix _wrap_picture_type_to_string so that it doesn't return TagLib::String but a Ruby string instead (and vice versa).

jacobvosmaer · 2024-04-11T12:21:51Z

This isn't quite there yet; we need to fix _wrap_picture_type_to_string so that it doesn't return TagLib::String but a Ruby string instead (and vice versa).

I think I fixed this now by replacing a lot of #include <taglib/tpicturetype.h> lines with a few targeted %include <taglib/tpicturetype.h>.

jacobvosmaer · 2024-04-11T14:14:40Z

Still not sure if I'm handling TagLib::offset_t correctly. TagLib uses an ifdef to define it at compile time (its definition depends on whether we're on WIN32). But Swig runs before compile time so what do we tell it?

For now I have told Swig to use long long.

robinst · 2024-04-11T14:34:44Z

@robinst what do you think about merging main into taglib-2? That would shrink this PR to about 4-5 commits.

Sounds good. Also fine with rebasing if that's easier.

This will help with the switch to TagLib 2 because that version has a dependency in a Git submodule.

jacobvosmaer

I wrote some note about the changes.

jacobvosmaer · 2024-04-11T17:27:32Z

ext/taglib_aiff/taglib_aiff.i

@@ -20,6 +20,7 @@

 // Ignore IOStream and all the constructors using it.
 %ignore IOStream;
+%ignore TagLib::RIFF::AIFF::File::File(IOStream *, bool, Properties::ReadStyle, ID3v2::FrameFactory *);


This is a new constructor we have to ignore because it uses IOStream.

jacobvosmaer · 2024-04-11T17:29:25Z

ext/taglib_base/taglib_base.i

@@ -26,6 +26,7 @@ namespace TagLib {
  typedef unsigned char uchar;
  typedef unsigned int  uint;
  typedef unsigned long ulong;
+  using offset_t = long long;


This seems to do the job of telling Swig that offset_t is an integer type and not a C++ object. On platforms where offset_t is smaller than long long, this will use an intermediate long long variable before converting to offset_t.

jacobvosmaer · 2024-04-11T18:51:51Z

ext/taglib_base/taglib_base_wrap.cxx

@@ -3211,17 +3325,19 @@ _wrap_File_find__SWIG_0(int argc, VALUE *argv, VALUE self) {
    tmp2 = ruby_string_to_taglib_bytevector(argv[0]);
    arg2 = &tmp2;
  }
-  ecode3 = SWIG_AsVal_long(argv[1], &val3);
+  ecode3 = SWIG_AsVal_long_SS_long(argv[1], &val3);


This is an example of how the Swig definition of offset_t as long long turns out. First Swig converts a ruby object argv[1] to a long long stored in val3.

jacobvosmaer · 2024-04-11T18:52:36Z

ext/taglib_base/taglib_base_wrap.cxx

  } 
-  arg3 = static_cast< long >(val3);
+  arg3 = static_cast< TagLib::offset_t >(val3);


Here long long val3 is cast to TagLib::offset_t arg3.

jacobvosmaer · 2024-04-11T18:53:21Z

ext/taglib_base/taglib_base_wrap.cxx

-  vresult = SWIG_From_long(static_cast< long >(result));
+  result = (TagLib::offset_t)(arg1)->find((TagLib::ByteVector const &)*arg2,arg3,(TagLib::ByteVector const &)*arg4);
+  {
+    vresult = taglib_offset_t_to_ruby_int(result);


On the way out, an offset_t value is passed to our taglib_offset_t_to_ruby_int function which uses the Ruby LL2NUM or OFFT2NUM macro depending on the compile constant _WIN32.

jacobvosmaer · 2024-04-11T19:02:18Z

ext/taglib_mp4/taglib_mp4.i

 %freefunc TagLib::MP4::File "free_taglib_mp4_file";

 // Ignore IOStream and all the constructors using it.
 %ignore IOStream;
 %ignore TagLib::MP4::File::File(IOStream *, bool, Properties::ReadStyle);
+%ignore TagLib::MP4::File::File(IOStream *, bool, Properties::ReadStyle, ItemFactory *);


Another new constructor for us to ignore.

jacobvosmaer · 2024-04-11T19:03:08Z

ext/taglib_mpeg/taglib_mpeg.i

@@ -14,6 +14,7 @@
 %ignore TagLib::MPEG::Header::operator=;
 %include <taglib/xingheader.h>

+%include <taglib/id3v2.h>
 %include <taglib/mpegheader.h>


As before in id3v2.i we need this new header file.

jacobvosmaer · 2024-04-11T19:03:22Z

ext/taglib_mpeg/taglib_mpeg.i

@@ -30,6 +31,10 @@
 %ignore TagLib::MPEG::File::File(IOStream *, ID3v2::FrameFactory *, bool, Properties::ReadStyle);
 %ignore TagLib::MPEG::File::File(IOStream *, ID3v2::FrameFactory *, bool);
 %ignore TagLib::MPEG::File::File(IOStream *, ID3v2::FrameFactory *);
+%ignore TagLib::MPEG::File::File(IOStream *, bool, Properties::ReadStyle, ID3v2::FrameFactory *);


More ignored IOStream constructors.

jacobvosmaer · 2024-04-11T19:03:39Z

ext/taglib_ogg/taglib_ogg.i

@@ -4,8 +4,6 @@
 #include <taglib/oggfile.h>
 #include <taglib/flacpicture.h>
 #include <taglib/xiphcomment.h>
-// Help find FLAC::
-using namespace TagLib;


Everything still works without this line.

jacobvosmaer · 2024-04-11T19:51:20Z

ext/taglib_wav/taglib_wav.i

@@ -4,7 +4,7 @@
 #include <taglib/wavfile.h>
 #include <taglib/wavproperties.h>
 #include <taglib/id3v2tag.h>
-using namespace TagLib::RIFF;
+using StripTags = TagLib::File::StripTags;


I think we're doing something wrong here. Swig is not "seeing" TagLib::File so it doesn't know what StripTags is. The proper solution would be to %include <taglib/rifffile.h> but then we run afoul of namespace flattening.

Maybe we need a separate .i for riff?

Since the test pass now, maybe we should fix this in a later PR.

I figured out how to do it, it was a matter of %ignore TagLib::RIFF::File.

jacobvosmaer · 2024-04-11T20:20:44Z

ext/taglib_aiff/taglib_aiff.i

@@ -14,12 +14,16 @@
 %ignore TagLib::RIFF::AIFF::Properties::length;
 %ignore TagLib::RIFF::AIFF::Properties::sampleWidth;

+%ignore TagLib::RIFF::File;
+%include <taglib/rifffile.h>


This fixes a Swig warning:

Warning 401: Nothing known about base class 'TagLib::RIFF::File'. Ignored.

jacobvosmaer · 2024-04-11T20:22:56Z

ext/taglib_wav/taglib_wav.i

@@ -4,7 +4,6 @@
 #include <taglib/wavfile.h>
 #include <taglib/wavproperties.h>
 #include <taglib/id3v2tag.h>
-using namespace TagLib::RIFF;


I replaced this line with %include <taglib/rifffile.h> below.

jacobvosmaer · 2024-04-12T07:11:55Z

@ufleisch I have turned your patch from #129 (comment) into a commit in this PR, I hope that is OK. ee55a43

@robinst I think this PR is ready for review.

ext/taglib_flac_picture/taglib_flac_picture_wrap.cxx

ext/taglib_id3v2/taglib_id3v2_wrap.cxx

ufleisch · 2024-04-13T09:52:28Z

@ufleisch I have turned your patch from #129 (comment) into a commit in this PR, I hope that is OK. ee55a43

I am not really a great help concerning Ruby and SWIG, so I am just trying to comment about what was discussed concerning TagLib:

The rationale behind offset_t is that we need 64-bit integers to work with large files. These offsets were long in TagLib 1.x, which was not a problem on 64-bit Linux because long has 64 bits there. However, on Windows, long is only 32 bits, also on 64-bit architectures, therefore, it was not possible to work with large files on Windows. off_t would be a good solution on POSIX, but it does not exist on Windows, therefore offset_t was introduced. If you use long long instead, this will be correct on 64-bit architectures.
It would be probably nice to support properties and complexProperties with taglib-ruby, these should be enough for most use cases, so users do not have to deal with the format specific classes.

jacobvosmaer · 2024-04-13T10:06:00Z

@ufleisch thanks for taking a look!

I didn't mean to complain about offset_t, I am just clueless about SWIG and I was trying to convince myself and @robinst that the current code correctly handles the ifdef. (It's too late now but I wonder if int64_t would have done what you wanted without needing an ifdef?)

Regarding the properties interfaces: I agree it would be good to support them for the reason you state. We never did until now because we wanted to remain compatible with older taglib 1.x versions that didn't have properties. The reason I'm letting SWIG ignore them in this PR is that I don't want to do too many things in one PR.

My main question was if you are OK with the fact that I made a commit with your name in it (by feeding the patch you posted to git am).

ufleisch · 2024-04-13T10:21:28Z

My main question was if you are OK with the fact that I made a commit with your name in it (by feeding the patch you posted to git am).

Of course this is OK, thanks for all your work!

robinst

Changes LGTM! Great work, and thanks to @ufleisch for the help too!

I'll let you merge this one first, then I'll have a look at #129 later to resolve any conflicts.

jacobvosmaer force-pushed the jv-taglib-2 branch 2 times, most recently from 434d8a7 to 6be8db5 Compare April 11, 2024 11:43

jacobvosmaer force-pushed the jv-taglib-2 branch from 6be8db5 to 5578b75 Compare April 11, 2024 12:21

jacobvosmaer force-pushed the jv-taglib-2 branch 4 times, most recently from 5962a86 to a60bfb4 Compare April 11, 2024 14:06

jacobvosmaer force-pushed the jv-taglib-2 branch from a60bfb4 to e147a19 Compare April 11, 2024 14:24

jacobvosmaer mentioned this pull request Apr 11, 2024

Update taglib-2 from main #145

Merged

jacobvosmaer and others added 3 commits April 11, 2024 17:51

rake vendor: use git to download taglib

9e9590a

This will help with the switch to TagLib 2 because that version has a dependency in a Git submodule.

Undefine TAGLIB_MSVC_SUPPRESS_WARNING_NEEDS_TO_HAVE_DLL_INTERFACE

ed827e7

Fix build with TagLib 2.0

ee55a43

jacobvosmaer force-pushed the jv-taglib-2 branch 2 times, most recently from bbdd6ed to c06a8ec Compare April 11, 2024 15:55

jacobvosmaer commented Apr 11, 2024

View reviewed changes

More taglib 2 build fixes

2e0a425

jacobvosmaer force-pushed the jv-taglib-2 branch from c06a8ec to 2e0a425 Compare April 11, 2024 20:19

jacobvosmaer commented Apr 11, 2024

View reviewed changes

jacobvosmaer mentioned this pull request Apr 12, 2024

add utf8cpp git submodule taglib/taglib#1142

Merged

jacobvosmaer requested a review from robinst April 12, 2024 10:06

jacobvosmaer commented Apr 12, 2024

View reviewed changes

ext/taglib_flac_picture/taglib_flac_picture_wrap.cxx Show resolved Hide resolved

jacobvosmaer commented Apr 12, 2024

View reviewed changes

ext/taglib_id3v2/taglib_id3v2_wrap.cxx Show resolved Hide resolved

Ignore unwanted module functions

998ac5b

robinst approved these changes Apr 17, 2024

View reviewed changes

jacobvosmaer merged commit 989c619 into taglib-2 Apr 17, 2024
4 checks passed

jacobvosmaer mentioned this pull request Apr 17, 2024

TagLib 2: ignore MP4 const iterator begin and end #146

Merged

ufleisch mentioned this pull request Apr 20, 2024

Question regarding Filename on Windows (Ruby bindings...) taglib/taglib#1219

Closed

robinst mentioned this pull request Oct 26, 2024

Support properties and complexProperties #148

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get TagLib 2.0.1 working #143

Get TagLib 2.0.1 working #143

jacobvosmaer commented Apr 11, 2024

jacobvosmaer commented Apr 11, 2024

jacobvosmaer commented Apr 11, 2024

jacobvosmaer commented Apr 11, 2024

jacobvosmaer commented Apr 11, 2024

robinst commented Apr 11, 2024

jacobvosmaer left a comment

jacobvosmaer Apr 11, 2024

jacobvosmaer Apr 11, 2024

jacobvosmaer Apr 11, 2024

jacobvosmaer Apr 11, 2024

jacobvosmaer Apr 11, 2024

jacobvosmaer Apr 11, 2024

jacobvosmaer Apr 11, 2024

jacobvosmaer Apr 11, 2024

jacobvosmaer Apr 11, 2024

jacobvosmaer Apr 11, 2024

jacobvosmaer Apr 11, 2024

jacobvosmaer Apr 11, 2024

jacobvosmaer Apr 11, 2024

jacobvosmaer commented Apr 12, 2024 •

edited

Loading

ufleisch commented Apr 13, 2024

jacobvosmaer commented Apr 13, 2024

ufleisch commented Apr 13, 2024

robinst left a comment

Get TagLib 2.0.1 working #143

Get TagLib 2.0.1 working #143

Conversation

jacobvosmaer commented Apr 11, 2024

jacobvosmaer commented Apr 11, 2024

jacobvosmaer commented Apr 11, 2024

jacobvosmaer commented Apr 11, 2024

jacobvosmaer commented Apr 11, 2024

robinst commented Apr 11, 2024

jacobvosmaer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobvosmaer commented Apr 12, 2024 • edited Loading

ufleisch commented Apr 13, 2024

jacobvosmaer commented Apr 13, 2024

ufleisch commented Apr 13, 2024

robinst left a comment

Choose a reason for hiding this comment

jacobvosmaer commented Apr 12, 2024 •

edited

Loading