D16671: Refactor embedded image extractor for greater extensibility

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> embeddedimagedata.cpp:134
>          TagLib::FLAC::File flacFile(&stream, TagLib::ID3v2::FrameFactory::instance(), true);
> -        TagLib::List<TagLib::FLAC::Picture *> lstPic = flacFile.pictureList();
> -
> -        if (!lstPic.isEmpty()) {
> -            for (TagLib::List<TagLib::FLAC::Picture *>::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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20181204/e595bc69/attachment-0001.html>