D16671: Refactor embedded image extractor for greater extensibility
This revision was automatically updated to reflect the committed changes. Closed by commit R286:7929a896b5e1: Refactor embedded image extractor for greater extensibility (authored by astippich). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D16671?vs=46880&id=46982#toc REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D16671?vs=46880&id=46982 REVISION DETAIL https://phabricator.kde.org/D16671 AFFECTED FILES src/embeddedimagedata.cpp To: astippich, mgallien, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D16671: Refactor embedded image extractor for greater extensibility
bruns accepted this revision.
bruns added inline comments.
This revision is now accepted and ready to land.
INLINE COMMENTS
> embeddedimagedata.cpp:207
> +int dataPosition = pictureData.find('\0');
> +if (dataPosition == -1) {
> +return QByteArray();
You can check for `>= 0` and let the `-1` case fall through ...
> astippich wrote in embeddedimagedata.cpp:134
> Docs don't say, so I fed a musepack file into this codepath and it still ran
> without issues (no cover extracted of course). Good enough?
Yes, thanks.
REPOSITORY
R286 KFileMetaData
BRANCH
refactor_embedded_image
REVISION DETAIL
https://phabricator.kde.org/D16671
To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun,
ngraham, bruns, abrahams
D16671: Refactor embedded image extractor for greater extensibility
astippich marked 2 inline comments as done. astippich added inline comments. INLINE COMMENTS > bruns wrote in embeddedimagedata.cpp:134 > Does Taglib signal an error when it fails to parse the file? Or is calling > pictureList() always safe? Docs don't say, so I fed a musepack file into this codepath and it still ran without issues (no cover extracted of course). Good enough? > bruns wrote in embeddedimagedata.cpp:148 > Not sure if this is an issue, but the old code only called > `tag()->pictureList()` after `!tag()->isEmpty()` - is this safe without? > Dito for oggFile above. Yes, the list will be empty. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D16671 To: astippich, mgallien, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D16671: Refactor embedded image extractor for greater extensibility
astippich updated this revision to Diff 46880. astippich added a comment. - new lines, handle dataPosition == -1 REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D16671?vs=46860&id=46880 BRANCH refactor_embedded_image REVISION DETAIL https://phabricator.kde.org/D16671 AFFECTED FILES src/embeddedimagedata.cpp To: astippich, mgallien, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D16671: Refactor embedded image extractor for greater extensibility
bruns added inline comments. INLINE COMMENTS > embeddedimagedata.cpp:181 > } > +QByteArray > +EmbeddedImageData::Private::getFrontCoverFromMp4(TagLib::MP4::Tag* mp4Tags) > const Nitpick - add an empty line in-between functions REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D16671 To: astippich, mgallien, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D16671: Refactor embedded image extractor for greater extensibility
bruns added inline comments.
INLINE COMMENTS
> embeddedimagedata.cpp:134
> TagLib::FLAC::File flacFile(&stream,
> TagLib::ID3v2::FrameFactory::instance(), true);
> -TagLib::List lstPic =
> flacFile.pictureList();
> -
> -if (!lstPic.isEmpty()) {
> -for (TagLib::List::Iterator it =
> lstPic.begin(); it != lstPic.end(); ++it) {
> -TagLib::FLAC::Picture *picture = *it;
> -if (picture->type() == picture->FrontCover) {
> -return QByteArray(picture->data().data(),
> picture->data().size());
> -}
> -}
> -}
> +return getFrontCoverFromFlacPicture(flacFile.pictureList());
> +
Does Taglib signal an error when it fails to parse the file? Or is calling
pictureList() always safe?
> embeddedimagedata.cpp:148
> +TagLib::Ogg::Opus::File opusFile(&stream, true);
> +if (opusFile.tag()) {
> +return
> getFrontCoverFromFlacPicture(opusFile.tag()->pictureList());
Not sure if this is an issue, but the old code only called
`tag()->pictureList()` after `!tag()->isEmpty()` - is this safe without?
Dito for oggFile above.
> astippich wrote in embeddedimagedata.cpp:204
> The code also works if only the picture data is there without the name.
> I could return an empty QByteArray if dataPosition == -1 and pictureData.size
> == 0.
> Do you prefer if I do that separately?
Is it safe to call find on an empty ByteVector - most likely yes ...
Then return QByteArray on dataPosition == -1, so it is obvious the error case
is handled, and keep the rest as is.
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D16671
To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun,
ngraham, bruns, abrahams
D16671: Refactor embedded image extractor for greater extensibility
astippich added a dependent revision: D17357: extend list of supported mimetypes for embedded image extractor. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D16671 To: astippich, mgallien, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D16671: Refactor embedded image extractor for greater extensibility
astippich updated this revision to Diff 46860. astippich added a comment. - rebase REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D16671?vs=44982&id=46860 BRANCH refactor_embedded_image REVISION DETAIL https://phabricator.kde.org/D16671 AFFECTED FILES src/embeddedimagedata.cpp To: astippich, mgallien, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D16671: Refactor embedded image extractor for greater extensibility
astippich added a comment. anyone? I would like to extend the support to the same file types as supported by the taglibextractor REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D16671 To: astippich, mgallien, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D16671: Refactor embedded image extractor for greater extensibility
astippich added a comment. ping REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D16671 To: astippich, mgallien, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D16671: Refactor embedded image extractor for greater extensibility
astippich added inline comments. INLINE COMMENTS > bruns wrote in embeddedimagedata.cpp:204 > I know you have only moved this code here ... > > `TagLib::ByteVector::find` returns `-1` on not found, and we add 1, so this > is safe here. But maybe we should return `QByteArray()` instead? The code also works if only the picture data is there without the name. I could return an empty QByteArray if dataPosition == -1 and pictureData.size == 0. Do you prefer if I do that separately? REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D16671 To: astippich, mgallien, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D16671: Refactor embedded image extractor for greater extensibility
bruns added inline comments.
INLINE COMMENTS
> embeddedimagedata.cpp:204
> +TagLib::ByteVector pictureData = (*itApe).second.binaryData();
> +int dataPosition = pictureData.find('\0') + 1;
> +return QByteArray(pictureData.data() + dataPosition,
> pictureData.size() - dataPosition);
I know you have only moved this code here ...
`TagLib::ByteVector::find` returns `-1` on not found, and we add 1, so this is
safe here. But maybe we should return `QByteArray()` instead?
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D16671
To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun,
ngraham, bruns, abrahams
D16671: Refactor embedded image extractor for greater extensibility
astippich updated this revision to Diff 44982. astippich added a comment. - more cleanup REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D16671?vs=44868&id=44982 BRANCH refactor_embedded_image REVISION DETAIL https://phabricator.kde.org/D16671 AFFECTED FILES src/embeddedimagedata.cpp To: astippich, mgallien, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D16671: Refactor embedded image extractor for greater extensibility
astippich created this revision. astippich added reviewers: mgallien, bruns. Herald added projects: Frameworks, Baloo. Herald added subscribers: Baloo, kde-frameworks-devel. astippich requested review of this revision. REVISION SUMMARY refactor the embedded image data extractor so that support for different mime types can be added in the future more easily REPOSITORY R286 KFileMetaData BRANCH refactor_embedded_image REVISION DETAIL https://phabricator.kde.org/D16671 AFFECTED FILES src/embeddedimagedata.cpp To: astippich, mgallien, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
