Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
sandsmark opened this issue Aug 13, 2020 · 6 comments

Comments

@sandsmark
Copy link

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
Copy link
Owner

Here's my comment regarding this question for the tag editor which also contains a link to the standard: Martchus/tageditor#55 (comment)

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
Copy link
Owner

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

@sandsmark
Copy link
Author

sandsmark commented Aug 22, 2020

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Owner

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?

@Martchus Martchus changed the title matroska cover support Support reading/writing Matroska cover as tag (rather than an attachment) Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants