D16671: Refactor embedded image extractor for greater extensibility

2018-12-06 Thread Alexander Stippich
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=46982#toc

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16671?vs=46880=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

2018-12-05 Thread Stefan Brüns
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

2018-12-04 Thread Alexander Stippich
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

2018-12-04 Thread Alexander Stippich
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=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

2018-12-04 Thread Stefan Brüns
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

2018-12-04 Thread Stefan Brüns
bruns added inline comments.

INLINE COMMENTS

> embeddedimagedata.cpp:134
>  TagLib::FLAC::File flacFile(, 
> 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(, 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

2018-12-04 Thread Alexander Stippich
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

2018-12-04 Thread Alexander Stippich
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=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

2018-12-02 Thread Alexander Stippich
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

2018-11-24 Thread Alexander Stippich
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

2018-11-10 Thread Alexander Stippich
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

2018-11-08 Thread Stefan Brüns
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

2018-11-06 Thread Alexander Stippich
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=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

2018-11-04 Thread Alexander Stippich
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