Type mismatch when adding an AttachedPictureFrame

[goulvench]

Hi!
When running code straight from the readme, I get the following error:

# `add_frame': Expected argument 1 of type TagLib::ID3v2::Frame *, 
#  but got TagLib::ID3v2::AttachedPictureFrame #<TagLib::ID3v2::AttachedPictu... (ObjectPreviouslyDeleted)
#    in SWIG method 'addFrame'

# Here's the code I'm using:
apic = TagLib::ID3v2::AttachedPictureFrame.new
apic.type = TagLib::ID3v2::AttachedPictureFrame::FrontCover
apic.picture = File.open(image_path, "rb") { |f| f.read }
apic.mime_type = "image/jpeg"
apic.description = "Cover"

TagLib::MPEG::File.open("file.mp3") do |file|
  tag = file.id3v2_tag
  tag.artist = metadata[:artist]
  tag.title = metadata[:album]
  tag.year = metadata[:year].to_i
  tag.track = index
  tag.add_frame(apic)
  file.save
end

Any idea why this happens, or what I should change in my code for it to work?

Versions

• ruby 3.1.4 on an Intel Mac (same error with 3.2.0).
• gem version 1.1.3.
• brew info taglib gives taglib: stable 1.13.1 (bottled), HEAD

[goulvench]

I found a workaround!
I realized that this happens for the second file only, because the AttachedPictureFrame is memoized —even if I call dup on it.

So I need to build a whole new AttachedPictureFrame for each file, even if the image is exactly the same. Maybe this is a limitation of the underlying C++ library, I don't know.

[jacobvosmaer]

I think this might be the problem:

taglib-ruby/ext/taglib_mpeg/taglib_mpeg.i

Lines 60 to 71 in 189fa6b

	TagLib::ID3v2::Tag *id3v2tag = file->ID3v2Tag(false);
	if (id3v2tag) {
	TagLib::ID3v2::FrameList frames = id3v2tag->frameList();
	for (TagLib::ID3v2::FrameList::ConstIterator it = frames.begin(); it != frames.end(); it++) {
	TagLib::ID3v2::Frame *frame = (*it);
	SWIG_RubyUnlinkObjects(frame);
	SWIG_RubyRemoveTracking(frame);
	}
	SWIG_RubyUnlinkObjects(id3v2tag);
	SWIG_RubyRemoveTracking(id3v2tag);
	}

When the TagLib::MPEG::File.open block is done, Ruby may garbage-collect the TagLib::MPEG::File instance used by the block. That instance has a cleanup function which iterates through all the ID3v2 frames attached to the tag of the file and frees their underlying C++ objects.

[jacobvosmaer]

@robinst looking at the SWIG documentation I wonder if we should move away from this pattern of using %freefunc and use %markfunc instead.

If we use %markfunc correctly then we can make individual wrapper objects (like TagLib::ID3v2::Frame) responsible for freeing C++ objects, rather than iterating over collections like we do now.

[jacobvosmaer]

Ruby may garbage-collect

In fact, closing the file forces freeing of the objects:

taglib-ruby/ext/taglib_mpeg/taglib_mpeg.i

Lines 87 to 89 in 189fa6b

	void close() {
	free_taglib_mpeg_file($self);
	}

[jacobvosmaer]

Never mind, I don't think markfunc is applicable here.

@goulvench it appears it is indeed because of the C++ library that you cannot reuse a frame. This program crashes with a segfault:

// testowner.cpp
// To compile: LDLIBS='-ltag -lz' make testowner
//
#include <iostream>
#include <cstdlib>

#include <taglib/taglib.h>
#include <taglib/mpegfile.h>
#include <taglib/id3v2tag.h>
#include <taglib/textidentificationframe.h>

using namespace TagLib;

int main(int argc, char **argv) {
  if (argc != 2) {
    std::cerr << "usage: " << argv[0] << " file.mp3" << std::endl;
    return EXIT_FAILURE;
  }

  char *filename = argv[1];

  ID3v2::TextIdentificationFrame *frame2 = new ID3v2::TextIdentificationFrame("TIT2");
  frame2->setText("Title");
  if(1){ // <- change to 0 and we no longer crash
    MPEG::File file(filename);
    ID3v2::Tag *tag = file.ID3v2Tag();
    tag->addFrame(frame2);
  }
  std::cout << frame2->toString() <<std::endl;
}

[robinst]

Spot on, see the docs for addFrame: https://taglib.org/api/classTagLib_1_1ID3v2_1_1Tag.html#ae090718d9acff1341530aa6438759b40

Add a frame to the tag. At this point the tag takes ownership of the frame and will handle freeing its memory.