add ability to read embedded cover files
ClosedPublic

Authored by astippich on Apr 18 2018, 5:34 PM.

Details

Summary

read the embedded image of audio files as binary data

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.
astippich created this revision.Apr 18 2018, 5:34 PM
Restricted Application added projects: Frameworks, Baloo. · View Herald TranscriptApr 18 2018, 5:34 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
astippich requested review of this revision.Apr 18 2018, 5:34 PM

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?

bruns added a subscriber: bruns.Apr 19 2018, 2:18 AM

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.

Baloos DBs should only contain searchable data. So until someone teaches baloo how to compare images, it should be kept separate.

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.

michaelh added a comment.EditedApr 19 2018, 7:01 PM

-2
https://cgit.kde.org/ffmpegthumbs.git/ should be useable, not sure though.

Too bad ffmpegthumbs is for videos only.

-2
https://cgit.kde.org/ffmpegthumbs.git/ should be useable, not sure though.

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.

-2
https://cgit.kde.org/ffmpegthumbs.git/ should be useable, not sure though.

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.

-2
https://cgit.kde.org/ffmpegthumbs.git/ should be useable, not sure though.

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.

astippich updated this revision to Diff 33375.May 1 2018, 9:46 AM
  • new reader for embedded image data
astippich retitled this revision from add ability to read embedded cover files to {RFC] add ability to read embedded cover files.May 1 2018, 9:46 AM

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.

bruns added inline comments.May 1 2018, 7:00 PM
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.
Currently, it will be implicitly shared between instances.
In many classes in KDE repository you do not need to do it because QObject inheritance gives you exactly that.

43

The private method can go to the private class.

astippich updated this revision to Diff 33934.May 10 2018, 10:05 AM
  • adjust to feedback
Restricted Application edited subscribers, added: Baloo, kde-frameworks-devel; removed: Frameworks. · View Herald TranscriptMay 10 2018, 10:05 AM
astippich retitled this revision from {RFC] add ability to read embedded cover files to [RFC] add ability to read embedded cover files.May 10 2018, 10:06 AM
astippich marked 2 inline comments as done.May 10 2018, 10:08 AM
astippich added inline comments.
src/embeddedimagedata.h
41

std::make_unique is C++14, right? According to https://community.kde.org/Frameworks/Policies#Frameworks_Qt_requirements
C++11 is the currently minimum version, so I left it unchanged. This is also similar to the other classes in KFileMetaData

bruns requested changes to this revision.May 10 2018, 1:34 PM
bruns added inline comments.
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());
see http://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique

This revision now requires changes to proceed.May 10 2018, 1:34 PM
astippich updated this revision to Diff 33955.May 10 2018, 4:51 PM
  • fix usage of qflags
  • use unique_ptr
astippich marked 4 inline comments as done.May 10 2018, 4:53 PM

Thanks a lot!

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?

Unfortunately, found it only after I wrote all that myself :/

Sorry i don't see RFC earlier.

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?

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 ?

astippich updated this revision to Diff 34012.May 12 2018, 10:54 AM
  • add an AllImages flag
  • add simple documentation

Unfortunately, found it only after I wrote all that myself :/

Sorry i don't see RFC earlier.

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?

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?

Does not work here, I will report a bug.

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 ?

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

any more comments?

bruns added inline comments.May 23 2018, 5:46 PM
src/embeddedimagedata.cpp
68

If you follow the suggestion above, you can drop the second or'ed statement.

69

missing space after ,
you can also directly pass in fileMimeType.name()

src/embeddedimagedata.h
46

I would prefer FrontCover = 0x1, AllImages = FrontCover, where AllImages is extended later when other types are supported.

astippich updated this revision to Diff 34829.May 24 2018, 5:19 PM
  • simplify code

any more comments? would be nice to ship it with Kf 5.48

any more comments? would be nice to ship it with Kf 5.48

@bruns you requested changes. Can you have a look when you have time ?

bruns added a comment.May 31 2018, 8:40 PM

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 ...

Sorry, I meant Kf 5.47, so deadline on Saturday.

bruns added a comment.Jun 2 2018, 12:37 AM

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

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?

Yes sure. I have just seen your message. I also believe Alex is mostly away from keyboard this weekend.

astippich updated this revision to Diff 35934.Jun 10 2018, 7:11 AM
  • implement feedback
  • rebase
astippich marked 8 inline comments as done.Jun 10 2018, 7:12 AM
astippich retitled this revision from [RFC] add ability to read embedded cover files to add ability to read embedded cover files.
astippich updated this revision to Diff 35940.Jun 10 2018, 9:38 AM
  • rebase again after test data changes
bruns accepted this revision.Jun 10 2018, 1:22 PM

Thanks, good to go ...

This revision is now accepted and ready to land.Jun 10 2018, 1:22 PM
This revision was automatically updated to reflect the committed changes.

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.

Thanks for sorting that out.

sorry for the caused inconvenience