Merge lp:~michihenning/thumbnailer/add-preview-image-extraction into lp:thumbnailer/devel

[Michi Henning]

vs-thumb now looks for both image and preview image tags to extract artwork.

vs-thumb now looks for both image and preview image tags to extract artwork.

[James Henstridge (jamesh)]

This looks it works by accident. A file can contain multiple images, so the code in question is iterating through them to find one that represents the cover. It stops when it hits the last image.

If there are no IMAGE tags in the file, it will ask for the 0th PREVIEW_IMAGE and generally do the right thing.

If there is a single image in the file, the first iteration of the loop will find that image, and on the second iteration it will get an error and look for PREVIEW_IMAGE at index 1. So if there is a single PREVIEW_IMAGE tag, we'll ignore it.

I think we want to iterate through all IMAGE tags and then all PREVIEW_IMAGE tags here.

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:273
http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/451/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-amd64-ci/261
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/266
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/266/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-i386-ci/260

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-devel-ci/451/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:279
http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/456/
Executed test runs:
    ABORTED: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-amd64-ci/266/console
    ABORTED: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/271/console
    ABORTED: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-i386-ci/265/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-devel-ci/456/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:277
http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/457/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-amd64-ci/267/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/272/console
    ABORTED: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-i386-ci/266/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-devel-ci/457/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:277
http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/458/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-amd64-ci/268
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/273
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/273/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-i386-ci/267

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-devel-ci/458/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:280
http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/459/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-amd64-ci/269
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/274
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/274/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-i386-ci/268

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-devel-ci/459/rebuild

review: Approve (continuous-integration)

[James Henstridge (jamesh)]

I think I've discovered the image type handling bug. Since you're modifying the code in question, I'll just note it here rather than creating a conflicting MP.

The following code:

        auto caps = gst_sample_get_caps(sample);
        auto structure = gst_caps_get_structure(caps, 0);

Needs to be changed to:

        auto structure = gst_sample_get_info(sample);

It seems the sample code I based this on was doing it wrong.

[Michi Henning (michihenning)]

Bloody awesome, thank you! Works like a charm, we are getting coverage on the cover art branch now.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:281
http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/460/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-amd64-ci/270
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/275
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/275/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-i386-ci/269

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-devel-ci/460/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:282
http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/462/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-amd64-ci/272
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/277
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/277/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-i386-ci/271

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-devel-ci/462/rebuild

review: Approve (continuous-integration)

[lp:~michihenning/thumbnailer/add-preview-image-extraction]

283. By Michi Henning on 2015-09-18

Added missing gstreamer initialization to stress test.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:283
http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/463/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-amd64-ci/273
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/278
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/278/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-i386-ci/272

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-devel-ci/463/rebuild

review: Approve (continuous-integration)

[lp:~michihenning/thumbnailer/add-preview-image-extraction]

284. By Michi Henning on 2015-09-20

Merged devel.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:284
http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/466/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-amd64-ci/276
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/281
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/281/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-i386-ci/275/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-devel-ci/466/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:284
http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/469/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-amd64-ci/279
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/284
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/284/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-i386-ci/278

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-devel-ci/469/rebuild

review: Approve (continuous-integration)

[James Henstridge (jamesh)]

This mostly looks good now. I've left a few inline comments about areas that could be improved though.

[Michi Henning (michihenning)]

Not happy about that either. It's awkward. I'm planning to re-factor vs-thumb anyway, so we get better diagnostics with time stamps. That'll be a good time to do something about the image extraction too, so could we let this slide for now? Functionally, it's correct, I believe.

[James Henstridge (jamesh)]

Lets merge this.

review: Approve

=== modified file 'debian/control'
--- debian/control	2015-09-15 03:22:34 +0000
+++ debian/control	2015-09-20 10:12:45 +0000
@@ -7,6 +7,8 @@
                cmake-extras (>= 0.4),
                debhelper (>= 9),
                google-mock,
+               gstreamer1.0-fluendo-mp3,
+               gstreamer1.0-plugins-bad-faad,
                gstreamer1.0-plugins-good,
                libapparmor-dev,
                libboost-filesystem-dev,

=== modified file 'src/thumbnailer.cpp'
--- src/thumbnailer.cpp	2015-09-15 09:37:17 +0000
+++ src/thumbnailer.cpp	2015-09-20 10:12:45 +0000
@@ -629,7 +629,6 @@
         Image full_size(string(raw_data.data(), raw_data.size()));
         return RequestBase::ImageData(full_size, RequestBase::CachePolicy::cache_fullsize, Location::remote);
     }
-    qDebug() << "common_fetch(): Request failed:" << artreply->error_string();
     if (artreply->not_found_error())
     {
         return RequestBase::ImageData(RequestBase::FetchStatus::not_found, Location::remote);
@@ -638,6 +637,7 @@
     {
         return RequestBase::ImageData(RequestBase::FetchStatus::no_network, Location::remote);
     }
+    qCritical() << "common_fetch(): Request failed:" << artreply->error_string();
     return RequestBase::ImageData(RequestBase::FetchStatus::error, Location::remote);
 }
 

=== modified file 'src/vs-thumb/thumbnailextractor.cpp'
--- src/vs-thumb/thumbnailextractor.cpp	2015-09-10 06:58:42 +0000
+++ src/vs-thumb/thumbnailextractor.cpp	2015-09-20 10:12:45 +0000
@@ -171,14 +171,59 @@
 
 struct ThumbnailExtractor::Private
 {
+    void find_audio_cover(GstTagList* tags, char const* tag_name);
+
+    enum ImageType { cover, other };
+
     gobj_ptr<GstElement> playbin;
     gint64 duration = -1;
 
     std::unique_ptr<GstSample, decltype(&gst_sample_unref)> sample{nullptr, gst_sample_unref};
     GdkPixbufRotation sample_rotation = GDK_PIXBUF_ROTATE_NONE;
     bool sample_raw = true;
+    ImageType image_type;
 };
 
+// Look for an image with the specified tag. If we find a cover image, image_type is set to cover,
+// and sample points at the image. If we find some other (non-cover) image, image_type is set to other,
+// and sample points at the image. Otherwise, if we can't find any image at all, sample is set to nullptr.
+
+void ThumbnailExtractor::Private::find_audio_cover(GstTagList* tags, const char* tag_name)
+{
+    image_type = ThumbnailExtractor::Private::other;
+
+    bool found_cover = false;
+    for (int i = 0; !found_cover; i++)
+    {
+        GstSample* s;
+        if (!gst_tag_list_get_sample_index(tags, tag_name, i, &s))
+        {
+            break;
+        }
+        assert(s);
+        // Check the type of this image
+        int type = GST_TAG_IMAGE_TYPE_UNDEFINED;
+        auto structure = gst_sample_get_info(s);
+        if (structure)
+        {
+            gst_structure_get_enum(structure, "image-type", GST_TYPE_TAG_IMAGE_TYPE, &type);
+        }
+        if (type == GST_TAG_IMAGE_TYPE_FRONT_COVER)
+        {
+            sample.reset(gst_sample_ref(s));
+            found_cover = true;
+            image_type = ThumbnailExtractor::Private::cover;
+        }
+        else if (type == GST_TAG_IMAGE_TYPE_UNDEFINED && !sample)
+        {
+            // Save the first unknown image tag, but continue scanning
+            // in case there is one marked as the cover.
+            sample.reset(gst_sample_ref(s));
+        }
+        gst_sample_unref(s);
+    }
+}
+
 /* GstPlayFlags flags from playbin.
  *
  * GStreamer does not install headers for the enums of individual
@@ -294,6 +339,7 @@
     g_signal_emit_by_name(p->playbin.get(), "get-video-tags", 0, &tags);
     if (tags)
     {
+        // TODO: The "flip-rotate-*" transforms defined in gst/gsttaglist.h need to be added here.
         char* orientation = nullptr;
         if (gst_tag_list_get_string_index(tags, GST_TAG_IMAGE_ORIENTATION, 0, &orientation) && orientation != nullptr)
         {
@@ -326,32 +372,26 @@
     p->sample.reset();
     p->sample_rotation = GDK_PIXBUF_ROTATE_NONE;
     p->sample_raw = false;
-    bool found_cover = false;
-    for (int i = 0; !found_cover; i++)
+
+    p->find_audio_cover(tags, GST_TAG_IMAGE);
+    if (!p->sample || p->image_type == ThumbnailExtractor::Private::other)
     {
-        GstSample* sample;
-        if (!gst_tag_list_get_sample_index(tags, GST_TAG_IMAGE, i, &sample))
-        {
-            break;
-        }
-        // Check the type of this image
-        auto caps = gst_sample_get_caps(sample);
-        auto structure = gst_caps_get_structure(caps, 0);
-        int type = GST_TAG_IMAGE_TYPE_UNDEFINED;
-        gst_structure_get_enum(structure, "image-type", GST_TYPE_TAG_IMAGE_TYPE, &type);
-        if (type == GST_TAG_IMAGE_TYPE_FRONT_COVER)
-        {
-            p->sample.reset(gst_sample_ref(sample));
-            found_cover = true;
-        }
-        else if (type == GST_TAG_IMAGE_TYPE_UNDEFINED && !p->sample)
-        {
-            // Save the first unknown image tag, but continue scanning
-            // in case there is one marked as the cover.
-            p->sample.reset(gst_sample_ref(sample));
-        }
-        gst_sample_unref(sample);
+        // Save whatever the first look-up returned (possibly nothing).
+        auto provisional_sample = std::move(p->sample);
+
+        // We didn't find a full-size image, or we found an image that is not marked as the cover.
+        // Try to find a preview image instead.
+        p->find_audio_cover(tags, GST_TAG_PREVIEW_IMAGE);
+        if (p->image_type == ThumbnailExtractor::Private::other && provisional_sample)
+        {
+            // We found a preview image that is *not* a cover, and the
+            // first look-up for a normal image returned something
+            // full-size that is *also* not a cover. Return the image
+            // from the first look-up in preference to this one.
+            p->sample = std::move(provisional_sample);
+        }
     }
+
     gst_tag_list_unref(tags);
     return bool(p->sample);
 }

=== modified file 'tests/stress/stress_test.cpp'
--- tests/stress/stress_test.cpp	2015-07-31 06:23:59 +0000
+++ tests/stress/stress_test.cpp	2015-09-20 10:12:45 +0000
@@ -23,6 +23,19 @@
 #include <utils/dbusserver.h>
 #include <utils/supports_decoder.h>
 
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wold-style-cast"
+#pragma GCC diagnostic ignored "-Wcast-qual"
+#pragma GCC diagnostic ignored "-Wcast-align"
+#ifdef __clang__
+#pragma clang diagnostic ignored "-Wparentheses-equality"
+#endif
+#include <gst/gst.h>
+#ifdef __clang__
+#pragma clang diagnostic pop
+#endif
+#pragma GCC diagnostic pop
+
 #include <boost/filesystem.hpp>
 #include <gtest/gtest.h>
 #include <QSignalSpy>
@@ -296,6 +309,9 @@
 
 int main(int argc, char** argv)
 {
+    // Need this because we are using a static test fixture and can't rely an global constructor order.
+    gst_init(&argc, &argv);
+
     QCoreApplication app(argc, argv);
 
     setenv("GSETTINGS_BACKEND", "memory", true);

=== modified file 'tests/vs-thumb/vs-thumb_test.cpp'
--- tests/vs-thumb/vs-thumb_test.cpp	2015-09-10 06:58:42 +0000
+++ tests/vs-thumb/vs-thumb_test.cpp	2015-09-20 10:12:45 +0000
@@ -50,6 +50,8 @@
 const char MP4_LANDSCAPE_TEST_FILE[] = TESTDATADIR "/gegl-landscape.mp4";
 const char MP4_PORTRAIT_TEST_FILE[] = TESTDATADIR "/gegl-portrait.mp4";
 const char VORBIS_TEST_FILE[] = TESTDATADIR "/testsong.ogg";
+const char AAC_TEST_FILE[] = TESTDATADIR "/testsong.m4a";
+const char MP3_TEST_FILE[] = TESTDATADIR "/testsong.mp3";
 
 class ExtractorTest : public ::testing::Test
 {
@@ -181,6 +183,48 @@
     EXPECT_EQ(gdk_pixbuf_get_height(image.get()), 200);
 }
 
+TEST_F(ExtractorTest, extract_aac_cover_art)
+{
+    if (!supports_decoder("audio/mpeg"))
+    {
+        fprintf(stderr, "No support for AAC decoder\n");
+        return;
+    }
+
+    ThumbnailExtractor extractor;
+
+    std::string outfile = tempdir + "/out.jpg";
+    extractor.set_uri(filename_to_uri(AAC_TEST_FILE));
+    ASSERT_FALSE(extractor.has_video());
+    ASSERT_TRUE(extractor.extract_audio_cover_art());
+    extractor.save_screenshot(outfile);
+
+    auto image = load_image(outfile);
+    EXPECT_EQ(gdk_pixbuf_get_width(image.get()), 200);
+    EXPECT_EQ(gdk_pixbuf_get_height(image.get()), 200);
+}
+
+TEST_F(ExtractorTest, extract_mp3_cover_art)
+{
+    if (!supports_decoder("audio/mpeg"))
+    {
+        fprintf(stderr, "No support for MP3 decoder\n");
+        return;
+    }
+
+    ThumbnailExtractor extractor;
+
+    std::string outfile = tempdir + "/out.jpg";
+    extractor.set_uri(filename_to_uri(MP3_TEST_FILE));
+    ASSERT_FALSE(extractor.has_video());
+    ASSERT_TRUE(extractor.extract_audio_cover_art());
+    extractor.save_screenshot(outfile);
+
+    auto image = load_image(outfile);
+    EXPECT_EQ(gdk_pixbuf_get_width(image.get()), 200);
+    EXPECT_EQ(gdk_pixbuf_get_height(image.get()), 200);
+}
+
 TEST_F(ExtractorTest, file_not_found)
 {
     ThumbnailExtractor extractor;