Merge lp:~vinipsmaker/inkscape/backgroundpixelart into lp:~inkscape.dev/inkscape/trunk

[Vinipsmaker]

I'm using Glib::Thread to run the expensive libdepixelize process in a background thread.

Communication with the GUI thread is done through Glib::Dispatcher.

I'm enabling GLIBMM deprecated features to have access to Glib::Thread at the beginning of pixelartdialog.cpp file.

[lp:~vinipsmaker/inkscape/backgroundpixelart]

12669. By Vinipsmaker on 2013-10-07

Removing useless comment

[Martin Owens (doctormo)]

The code looks nice and clean. I see some improvements to the original code.

I'm a little bit confused as to what the branch /does/. Is it so inkscape doesn't block while it's working on the pixelart process?

review: Approve (code)

[Vinipsmaker (vinipsmaker)]

> I'm a little bit confused as to what the branch /does/. Is it so inkscape
> doesn't block while it's working on the pixelart process?

Yup.

The branch changes will move the libdepixelize call to a different thread, then the main thread (the UI thread) won't block.

But I'm not sure if you're being sarcastic. "The code looks nice and clean"..."[...] what the branch /does/". Something is not right here. I can rewrite it using nice C++11 threading features after Johan Engelen convices Inkscape team to allow C++11 on the release after 0.91.
=P

Strangely I cannot compile this (backgroundpixelart) branch cleanly on my PC anymore. Some tweakings regarding GC and freetype were needed. I think the branch needs to be rebased against a recent revision of main Inkscape branch.

[Martin Owens (doctormo)]

Oh sorry, no sarcasm. Complete sincerity here. I was just confused between it being something to do with server based inkscape deployments and something that stopped blocking.

Code review doesn't need much functional review. :-)

Would you like to rebase it?

[Vinipsmaker (vinipsmaker)]

> Would you like to rebase it?

I don't care. I doubt any changes on the pixelart happenend in the main branch and a merge would probably apply clean.

[Vinipsmaker (vinipsmaker)]

s/pixelart/pixelart dialog/g

[Martin Owens (doctormo)]

I've merged in the code, although it needed fixing as some of the code had changed. There was a slight issue with the new pixbuf size check and a change in the notion for move -> Dialog::move which didn't apply any more.

Functional tests all pass. I'm considering this done.

review: Approve (functional)

=== modified file 'src/ui/dialog/pixelartdialog.cpp'
--- src/ui/dialog/pixelartdialog.cpp	2013-09-23 17:55:42 +0000
+++ src/ui/dialog/pixelartdialog.cpp	2013-10-07 03:31:47 +0000
@@ -16,6 +16,16 @@
 # include <config.h>
 #endif
 
+#ifdef GLIBMM_DISABLE_DEPRECATED
+#  undef GLIBMM_DISABLE_DEPRECATED
+#  include <glibmm/thread.h>
+#  include <glibmm/dispatcher.h>
+#  define GLIBMM_DISABLE_DEPRECATED 1
+#else // GLIBMM_DISABLE_DEPRECATED
+#  include <glibmm/thread.h>
+#  include <glibmm/dispatcher.h>
+#endif // GLIBMM_DISABLE_DEPRECATED
+
 #include "pixelartdialog.h"
 #include <gtkmm/radiobutton.h>
 #include <gtkmm/stock.h>
@@ -54,18 +64,6 @@
 namespace UI {
 namespace Dialog {
 
-template<class T>
-T move(T &obj)
-{
-#ifdef LIBDEPIXELIZE_ENABLE_CPP11
-    return std::move(obj);
-#else
-    T ret;
-    std::swap(obj, ret);
-    return ret;
-#endif // LIBDEPIXELIZE_ENABLE_CPP11
-}
-
 /**
  * A dialog for adjusting pixel art -> vector tracing parameters
  */
@@ -77,6 +75,23 @@
     ~PixelArtDialogImpl();
 
 private:
+    struct Input
+    {
+        Glib::RefPtr<Gdk::Pixbuf> pixbuf;
+        SVGLength x;
+        SVGLength y;
+    };
+    struct Output
+    {
+        Output(Tracer::Splines splines, SVGLength x, SVGLength y) :
+            splines(splines), x(x), y(y)
+        {}
+
+        Tracer::Splines splines;
+        SVGLength x;
+        SVGLength y;
+    };
+
     void setDesktop(SPDesktop *desktop);
     void setTargetDesktop(SPDesktop *desktop);
 
@@ -89,7 +104,8 @@
     Tracer::Kopf2011::Options options();
 
     void vectorize();
-    void processLibdepixelize(SPImage *img);
+    void processLibdepixelize(const Input &image);
+    void importOutput(const Output &out);
     void setDefaults();
     void updatePreview();
 
@@ -133,9 +149,21 @@
     Gtk::RadioButton optimizeRadioButton;
 #endif // LIBDEPIXELIZE_INKSCAPE_ENABLE_SMOOTH
 
+    //############ UI Logic data
+
     SPDesktop *desktop;
     DesktopTracker deskTrack;
     sigc::connection desktopChangeConn;
+
+    //############ Threads
+    void workerThread();
+    void onWorkerThreadFinished();
+    Glib::Thread *thread;
+    volatile gint abortThread; // C++11's atomic stuff is sooo much nicer
+    Glib::Dispatcher dispatcher;
+    std::vector<Input> queue;
+    std::vector<Output> output;
+    Tracer::Kopf2011::Options lastOptions;
 };
 
 void PixelArtDialogImpl::setDesktop(SPDesktop *desktop)
@@ -288,6 +316,9 @@
     deskTrack.connect(GTK_WIDGET(gobj()));
 
     signalResponse().connect(sigc::mem_fun(*this, &PixelArtDialogImpl::responseCallback));
+
+    dispatcher.connect(
+        sigc::mem_fun(*this, &PixelArtDialogImpl::onWorkerThreadFinished) );
 }
 
 void PixelArtDialogImpl::responseCallback(int response_id)
@@ -295,7 +326,8 @@
     if (response_id == GTK_RESPONSE_OK) {
         vectorize();
     } else if (response_id == GTK_RESPONSE_CANCEL) {
-        // TODO
+        // libdepixelize's interface need to be extended to allow aborts
+        g_atomic_int_set(&abortThread, true);
     } else if (response_id == GTK_RESPONSE_HELP) {
         setDefaults();
     } else {
@@ -340,60 +372,68 @@
         return;
     }
 
-    bool found = false;
-
     for ( GSList const *list = desktop->selection->itemList() ; list
               ; list = list->next ) {
         if ( !SP_IS_IMAGE(list->data) )
             continue;
 
-        found = true;
-
-        processLibdepixelize(SP_IMAGE(list->data));
+        SPImage *img = SP_IMAGE(list->data);
+        Input input;
+        input.pixbuf = Glib::wrap(img->pixbuf->getPixbufRaw(), true);
+        input.x = img->x;
+        input.y = img->y;
+
+        if ( input.pixbuf->get_width() > 256
+             || input.pixbuf->get_height() > 256 ) {
+            char *msg = _("Image looks too big. Process may take a while and is"
+                          " wise to save your document before continue."
+                          "\n\nContinue the procedure (without saving)?");
+            Gtk::MessageDialog dialog(msg, false, Gtk::MESSAGE_WARNING,
+                                      Gtk::BUTTONS_OK_CANCEL, true);
+
+            if ( dialog.run() != Gtk::RESPONSE_OK )
+                continue;
+        }
+
+        queue.push_back(input);
     }
 
-    if ( !found ) {
+    if ( !queue.size() ) {
         char *msg = _("Select an <b>image</b> to trace");
         msgStack->flash(Inkscape::ERROR_MESSAGE, msg);
         return;
     }
 
-    DocumentUndo::done(desktop->doc(), SP_VERB_SELECTION_PIXEL_ART,
-                       _("Trace pixel art"));
-
-    // Flush pending updates
-    desktop->doc()->ensureUpToDate();
+    mainCancelButton->set_sensitive(true);
+    mainOkButton->set_sensitive(false);
+
+    lastOptions = options();
+
+    g_atomic_int_set(&abortThread, false);
+    thread = Glib::Thread::create(
+        sigc::mem_fun(*this, &PixelArtDialogImpl::workerThread) );
 }
 
-void PixelArtDialogImpl::processLibdepixelize(SPImage *img)
+void PixelArtDialogImpl::processLibdepixelize(const Input &input)
 {
-    Tracer::Splines out;
-
-    Glib::RefPtr<Gdk::Pixbuf> pixbuf
-        = Glib::wrap(img->pixbuf->getPixbufRaw(), true);
-
-    if ( pixbuf->get_width() > 256 || pixbuf->get_height() > 256 ) {
-        char *msg = _("Image looks too big. Process may take a while and is"
-                      " wise to save your document before continue."
-                      "\n\nContinue the procedure (without saving)?");
-        Gtk::MessageDialog dialog(msg, false, Gtk::MESSAGE_WARNING,
-                                  Gtk::BUTTONS_OK_CANCEL, true);
-
-        if ( dialog.run() != Gtk::RESPONSE_OK )
-            return;
-    }
-
     if ( voronoiRadioButton.get_active() ) {
-        out = Tracer::Kopf2011::to_voronoi(pixbuf, options());
+        output.push_back(Output(Tracer::Kopf2011::to_voronoi(input.pixbuf,
+                                                             lastOptions),
+                                input.x, input.y));
     } else {
-        out = Tracer::Kopf2011::to_splines(pixbuf, options());
+        output.push_back(Output(Tracer::Kopf2011::to_splines(input.pixbuf,
+                                                             lastOptions),
+                                input.x, input.y));
     }
+}
 
+void PixelArtDialogImpl::importOutput(const Output &output)
+{
     Inkscape::XML::Document *xml_doc = desktop->doc()->getReprDoc();
     Inkscape::XML::Node *group = xml_doc->createElement("svg:g");
 
-    for ( Tracer::Splines::iterator it = out.begin(), end = out.end()
-              ; it != end ; ++it ) {
+    for ( Tracer::Splines::const_iterator it = output.splines.begin(),
+              end = output.splines.end() ; it != end ; ++it ) {
         Inkscape::XML::Node *repr = xml_doc->createElement("svg:path");
 
         {
@@ -421,7 +461,7 @@
             sp_repr_css_attr_unref(css);
         }
 
-        gchar *str = sp_svg_write_path(move(it->pathVector));
+        gchar *str = sp_svg_write_path(it->pathVector);
         repr->setAttribute("d", str);
         g_free(str);
 
@@ -433,14 +473,20 @@
     {
         group->setAttribute("transform",
                             (std::string("translate(")
-                             + sp_svg_length_write_with_units(img->x)
-                             + ' ' + sp_svg_length_write_with_units(img->y)
+                             + sp_svg_length_write_with_units(output.x)
+                             + ' ' + sp_svg_length_write_with_units(output.y)
                              + ')').c_str());
     }
 
     desktop->currentLayer()->appendChildRepr(group);
 
     Inkscape::GC::release(group);
+
+    DocumentUndo::done(desktop->doc(), SP_VERB_SELECTION_PIXEL_ART,
+                       _("Trace pixel art"));
+
+    // Flush pending updates
+    desktop->doc()->ensureUpToDate();
 }
 
 void PixelArtDialogImpl::setDefaults()
@@ -481,6 +527,29 @@
     pendingPreview = false;
 }
 
+void PixelArtDialogImpl::workerThread()
+{
+    for ( std::vector<Input>::iterator it = queue.begin(), end = queue.end()
+              ; it != end && !g_atomic_int_get(&abortThread) ; ++it ) {
+        processLibdepixelize(*it);
+    }
+    queue.clear();
+    dispatcher();
+}
+
+void PixelArtDialogImpl::onWorkerThreadFinished()
+{
+    thread->join();
+    thread = NULL;
+    for ( std::vector<Output>::const_iterator it = output.begin(),
+              end = output.end() ; it != end ; ++it ) {
+        importOutput(*it);
+    }
+    output.clear();
+    mainCancelButton->set_sensitive(false);
+    mainOkButton->set_sensitive(true);
+}
+
 /**
  * Factory method.  Use this to create a new PixelArtDialog
  */