D5417: add an extractor using qtmultimedia

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  If this extractor is worse than the taglib one, shouldn't the cmake run issue a warning about that? Or does it already do so because "taglib not found"?

INLINE COMMENTS

> qtmultimediaextractortest.cpp:2
> +/*
> +<<<<<<< HEAD
> + * TagLibExtractor tests.

you probably don't want to commit a merge conflict...

> qtmultimediaextractortest.cpp:8
> + *
> + * Copyright (C) 2015  Juan Palacios <jpalaciosdev at gmail.com>
> + *

+ your own copyright?

> qtmultimediaextractortest.cpp:39
> +{
> +    return QLatin1String(INDEXER_TESTS_SAMPLE_FILES_PATH) + QDir::separator() + fileName;
> +}

What if the path contains non-latin characters?
This should use QFile::decodeName instead.

> qtmultimediaextractor.cpp:2
> +/*
> +    <one line to give the library's name and an idea of what it does.>
> +    Copyright (C) 2012  Vishesh Handa <me at vhanda.in>

todo

> qtmultimediaextractor.cpp:78
> +        if (available && mMetadataExtractor->isMetaDataAvailable()) {
> +            album = mMetadataExtractor->metaData(QMediaMetaData::AlbumTitle);
> +            title = mMetadataExtractor->metaData(QMediaMetaData::Title);

The mutex needs to be locked for all these writes.

> qtmultimediaextractor.cpp:210
> +
> +    auto extractionResult = mSynchronizeCondition.wait(&mSynchronizeMutex, 400);
> +

If setMediaSource calls wakeAll before this thread gets here, we will wait forever (QWaitCondition = if you're not sleeping, you miss the wakeup call).

Use a QSemaphore instead of a QWaitCondition to fix this problem.

> qtmultimediaextractor.cpp:213
> +    if (!extractionResult) {
> +        mSynchronizeMutex.unlock();
> +        return;

Use QMutexLocker instead of manual lock/unlock, to make sure no unlock is forgotten.

> qtmultimediaextractor.h:2
> +/*
> +    <one line to give the library's name and an idea of what it does.>
> +    Copyright (C) 2012  Vishesh Handa <me at vhanda.in>

todo ;)

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D5417

To: mgallien, #frameworks, dfaure
Cc: dfaure, #frameworks
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170612/e0cc7480/attachment-0001.html>