search

Martchus / tagparser

C++ library for reading and writing MP4/M4A/AAC (iTunes), ID3, Vorbis, Opus, FLAC and Matroska tags
GNU General Public License v2.0
124 stars 17 forks source link

Support reading/writing Matroska cover as tag (rather than an attachment) #13

Open sandsmark opened 4 years ago

sandsmark commented 4 years ago

I'm not that familiar with matroska, so I don't know if there's a defined standard for how they are stored, but would it be possible to have a convenience function for getting the cover (or support for KnownField::Cover in the matroska parser somehow, I guess), instead of checking the attachments manually?

Martchus commented 4 years ago

Here's my comment regarding this question for the tag editor which also contains a link to the standard: https://github.com/Martchus/tageditor/issues/55#issuecomment-637152712

I suppose the mentioned problems apply to the library-level as well. However, on library-level I could provide a convenience function to trigger the mapping from attachments to tags with or from tags to attachments. These functions could be optionally invoked after parsing or before applying changes. It would be a no-op for all formats but Matroska. It needed to be well documented so it is clear what it is doing (considering the questions raised in my comment for tag editor).

Martchus commented 4 years ago

By the way, are you planning to use the library in some project?

sandsmark commented 4 years ago

I plan on using it in https://invent.kde.org/multimedia/ffmpegthumbs (the default KDE AV thumbnailer)

Someone added support for mp4 covers recently, but using taglib.

I got ebml/matroska supported merged in taglib ages ago, unfortunately in taglib2 since I assumed that was the active branch. I have opened a new PR with matroska support for the normal branch, but I'd rather use this library because the taglib code is a bit wonky at times. And I don't know how long it will take for a new taglib release with matroska support either.

sandsmark commented 4 years ago

And I was thinking of something like a function that just returns the most appropriate cover data. now it's some time since I looked at the tagparser code, but maybe in MediaFileInfo? Not sure if it should return a TagValue or just the contained data (in a std::vector or whatever), though.

I'm not sure how many tags have a mapping to attachments, if there are several that people use I guess a more generic tags <-> attachment mapping makes sense.

Martchus commented 4 years ago

Sounds interesting. I'll implement a convenience function as explained when I'll find the time. (I'll assign myself to this issue when I start working on it. In the meantime you might come up with a PR yourself of course.)

About tagparser vs. taglib: I'd like to give you an honest note that the tagparser library also has some shortcomings compared to taglib. It does not support some of the formats taglib supports. The ABI isn't as stable as well because I usually release a new version of taglib and tageditor at the same time so no effort was put into this aspect so far. This library is also not packaged for Debian-based distros so far.

But I like of course that there's some interest in the library and would like to see it used elsewhere.

I haven't thought about where this mapping would fit best. Maybe an additional virtual function within AbstractContainer which woud do nothing by default but in case of MatroskaContainer do the appropriate mapping? I would actually create two additional functions - one for each direction of the mapping. You're likely only interested to map attachments to tags so having this direction would be sufficient to get started. There could also be mapping functions on MediaFileInfo-level which would call the AbstractContainer-level functions if there's a container (and do nothing otherwise).

I'm not sure how the cover should be returned. I'd use a TagValue for sure because that way it would be consistent with how the cover is usually returned. However, I'm not so sure whether the mapping function from attachments to tags should directly return the cover. It might make more sense if it would add the cover to the most fitting tag within the same container. That way one would really just read the tags of the container as usual after calling the mapping function.

Martchus commented 4 years ago

Wait - as much as I like my library being used in another project: Isn't ffmpeg able to do the job as well? At least for MP4 I'm pretty sure it detects the cover as an additional track. I remember that people used my Tag Editor to add a cover to their MP4 files and then noticed that ffmpeg displays a warning related to that additional "cover track". Maybe the same works for Matroska as well? And if not, maybe it would be possible to add the functionality directly to ffmpeg?