read the embedded image of audio files as binary data
Details
Diff Detail
- Repository
- R286 KFileMetaData
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
This revision is only one possibility and is mainly there to start the discussion again on how to read the embedded picture data from e.g. audio files. Before this can land, Baloo has to be patched in order to cope with binary data.
I think for Baloo there are two possibilities: (a) Put it in the database same as all other data or (b) ignore the binary data. In the latter case, applications would have to query the metadata separately in order to get the cover.
For KFileMetaData I also see two possible implementation: Either in the taglibextractor, or in a completely new extractor that only reads the cover files (may be also useful for ebooks, but I don't know if such things exist there). If we agree on (a), I would opt to implement it in taglibextractor, if we chose (b) I think a new extractor makes sense such that applications would not have to read all the metadata again with the taglibextractor, but could specifically just read the picture data and get the rest from Baloo.
Any other suggestions? Opinions?
Baloos DBs should only contain searchable data. So until someone teaches baloo how to compare images, it should be kept separate.
Thanks a lot to continue to push this work.
I am also not convinced that Baloo should index the cover images. They are quite a lot of data. At the same time, I am really happy to have support for it in KFileMetaData.
It should then be easy to add support in a file preview or in applications.
It is also disqualified by the fact that it is not in frameworks. I think a nice solution is to implement a separate "extractor" that is not an extractor plugin like taglib, epub, etc. but implemented like the xattr tags (usermetadata) as a separate, exported class. This way, baloo doesn't have to be changed in any way and still applications using kfilemetadata can query the cover files specifically.
How does behave Baloo if you add properties of type EmbeddedPicture ?
Is it not a good idea to fix Baloo to not index everything but only searchable properties (like text and numeric properties and ignore binary data) ?
The current way to manage user rating mush have had a good rationale for its current design but I have failed to understand it.
It would also look quite odd to have a particular way to fetch properties of type EmbeddedPicture.
It may be useful to store some "EmbeddedPicture exists" flag inside Baloo. It should store strings, numeric values, dates, but as it has to handle each basic type individually (for storing data, parsing queries and executing queries) bytearrays should be ignored.
So, this is a next attempt trying to create a solution that works for everyone. It is not yet perfect, but I'd like to get some feedback if such an approach is favorable.
A new class is created that will handle the cover art. This way, there won't be any interference with other applications such as baloo, but applications relying on KFileMetaData can still query the cover art separately if desired.
Currently missing is documentation. I did the Cmake changes to my best knowledge, but some feedback here is very welcome. Same goes for the API.
src/embeddedimagedata.h | ||
---|---|---|
37 | return type QMap<ImageType, QByteArray>? And instead of an enum imageType a QFlags<ImageType> imagetype? The mimeDatabase should not be part of the method signature, its an implementation detail |
Thanks for your work. Please find a few remarks inline.
src/embeddedimagedata.h | ||
---|---|---|
26 | You can do a forward declaration of QMimeDatabase because it is only referred through a reference. | |
41 | You should be using a std::unique_ptr instead of a raw pointer. You also should take care of either forbidding copy (operator= and copy constructor) or implement them to perform a deep copy including duplicating the private object. | |
43 | The private method can go to the private class. |
src/embeddedimagedata.h | ||
---|---|---|
41 | std::make_unique is C++14, right? According to https://community.kde.org/Frameworks/Policies#Frameworks_Qt_requirements |
src/embeddedimagedata.cpp | ||
---|---|---|
70 | should be types & EmbeddedImageData::FrontCover | |
150 | missing space after , | |
171 | This is identical to the "audio/flac" case, can you use the same variable names etc. in both places? | |
src/embeddedimagedata.h | ||
41 | std::make_unique is just a convenience helper auto foo = std::make_unique<Foo> is exactly the same as auto foo = std::unique_ptr<Foo>(new Foo()); |
I do *same* thing in KIO-Extras https://phabricator.kde.org/source/kio-extras/browse/master/thumbnail/audiocreator.cpp
Yeah, I found that when I was looking into what's needed for a preview for audio files and was quite surprised.
Unfortunately, found it only after I wrote all that myself :/
Anyways, KFIleMetaData needs such a thing.
Off topic: The file preview does only work for folder previews, but for the individual files the previews are not shown. Is that intended behavior?
Sorry i don't see RFC earlier.
It *should* work for files as well as folders. Recently was added a patch that disable file preview for small sizes, can you increase icon size by slider?
I am away from keyboard. Can we move forward ?
Does it make sense to aim for having only one copy of this code and makes the kio code depends on KFileMetaData ?
Does not work here, I will report a bug.
That would make sense, I think. Btw, no need to rush here, Kf 5.47 is still some weeks away. I missed the 5.46 deadline unfortunately.
I still have one question: In case there is no cover found, should the QMap contain an entry with an empty bytearray or should it contain no corresponding entry at all? In the first case, an application has to test if the byte array is empty, in the latter it has to test if the map contains an entry. What's the better solution?
Currently it is doing the first.
@anthonyfieroni
The preview is actually working perfectly fine, my configuration was wrong. I swear I checked it before :) sorry for the noise
src/embeddedimagedata.cpp | ||
---|---|---|
68 | If you follow the suggestion above, you can drop the second or'ed statement. | |
69 | missing space after , | |
src/embeddedimagedata.h | ||
46 | I would prefer FrontCover = 0x1, AllImages = FrontCover, where AllImages is extended later when other types are supported. |
This looks good now, but haven't checked to thoroughly.
Did you mean 5.47 or 5.48 - 5.48 is almost a month away ...
as the remaining issues are formatting only, this is an accept by me save the requested changes.
@mgallien - if i am late to accept, can you do it?
src/embeddedimagedata.cpp | ||
---|---|---|
60 | nitpick - excessively long line QMap<EmbeddedImageData::ImageType, QByteArray> EmbeddedImageData::imageData(const QString &fileUrl, const EmbeddedImageData::ImageTypes types) const { | |
74 | dito, long line | |
96 | long line, just make it auto, the type is obvious from the static_cast<T> | |
src/embeddedimagedata.h | ||
36 | Can you add one or two more lines what format to expect inside the byte array? | |
52 | nitpick - singular, front cover |
Yes sure. I have just seen your message. I also believe Alex is mostly away from keyboard this weekend.
This change broke the build on Windows, which due to the CI notification being ignored has now become a maintainability issue.
The failure log can be found at https://build.kde.org/job/Frameworks%20kfilemetadata%20kf5-qt5%20WindowsMSVCQt5.10/33/consoleText
@dfaure This issue should be considered a release blocker for Frameworks
D13630 should fix the Windows build. Sorry for not noticing this before. I was mostly away from keyboard for the last two weeks after having been sick.