Various fixes by dnachbaur · Pull Request #177 · BlueBrain/Deflect

dnachbaur · 2017-08-07T16:12:14Z

Throw on failed connection to server in stream ctor; refactor test deflect servers
Transform error message printing into exceptions
Catch errors regarding invalid JPEG quality values
Allow uncompressed small tile sending
Move sendSizeHints and sendData also to Observer

rdumusc

Good in general!

rdumusc · 2017-08-07T16:57:51Z

deflect/ImageSegmenter.cpp

+
+    try
+    {
+        future.waitForFinished();


No, we shouldn't wait for the result. The sending should start while the compression is in progress (see comments below).

Of course. I just focused on getting the exception from the async execution. Too bad

rdumusc · 2017-08-07T17:03:50Z

deflect/ImageJpegCompressor.cpp

+        std::stringstream msg;
+        msg << "libjpeg-turbo image conversion failure: " << tjGetErrorStr()
+            << std::endl;
+        throw std::runtime_error(msg.str());


OK but the header should mention that the function can throw.

dnachbaur · 2017-08-08T09:24:07Z

I kept the empty bytearray for indicating errors and check it accordingly.

rdumusc · 2017-08-08T09:43:46Z

tests/cpp/StreamTests.cpp

+// Note: the following send tests only work as the small segment send does not
+// need the sendWorker thread to be present. So fortunate for us we can skip
+// setting up a stream server.
+BOOST_AUTO_TEST_CASE(testErrorOnUncompressedRGBA)


Great, this was indeed missing! Let's be fully complete and split this in three test cases:

testSendUncompressedRGBA

testErrorOnUnsupportedUncompressedFormats

testErrorOnNullUncompressedImage

rdumusc · 2017-08-08T10:00:32Z

tests/cpp/ServerTests.cpp

+
+        deflect::ImageWrapper bigImage(nullptr, 1000, 1000, deflect::ARGB);
+        bigImage.compressionPolicy = deflect::COMPRESSION_ON;
+        BOOST_CHECK(!stream.send(bigImage).get());


I that the only line we are truly testing? The amount of boilerplate code in this test suite is starting to be worrying, we should look for a way to mock the server / connection or at least reuse a shared Fixture.

rdumusc · 2017-08-08T10:07:29Z

deflect/ImageJpegCompressor.h

     * @param sourceImage The source image containing uncompressed image data.
     * @param imageRegion The region of the image to be compressed. Must not
     *        exceed image dimensions.
+     * @return compressed image if successful, otherwise QByteArray.isEmpty()


+1, this was missing

bbpbuildbot · 2017-08-08T16:28:00Z

Build finished.

rdumusc · 2017-08-08T17:25:45Z

deflect/ImageJpegCompressor.cpp

-        return QByteArray();
+        std::stringstream msg;
+        msg << "libjpeg-turbo image conversion failure: " << tjGetErrorStr();
+        throw std::invalid_argument(msg.str());


I would say this is more a runtime_error at this point

rdumusc · 2017-08-08T17:27:08Z

deflect/ImageJpegCompressor.h

-     * @return compressed image if successful, otherwise QByteArray.isEmpty()
+     * @return compressed image
+     * @throw std::invalid_argument if sourceImage.data is nullptr or
+     *                              tjCompress2 failed


don't mention tjCompress2 in the doc, it's an implementation detail and has the potential to become outdated

rdumusc · 2017-08-08T17:32:51Z

deflect/ImageSegmenter.h

     *
     * @param image The image to be compressed
     * @return the compressed segment
+     * @throw std::runtime_error if image is too big


too big -> invalid_argument?

rdumusc · 2017-08-08T17:41:44Z

deflect/StreamSendWorker.cpp

+                        break;
+                    }
+                    if (request.promise)
+                        request.promise->set_value(true);


I don't think this works, does it pass unit tests? you have changed the logic and doing set_value() for each task, which should fail at the second set (and be caught again by the catch (...)). Recommendation: Keep the logic as it was before, with the try/catch surrounding the entire for-loop.

rdumusc · 2017-08-08T17:45:02Z

deflect/StreamSendWorker.cpp

                << image.compressionQuality << std::endl;
-            return make_ready_future(false);
+            return make_exception_future<bool>(
+                std::make_exception_ptr(std::invalid_argument(msg.str())));


move std::make_exception_ptr inside make_exception_future to avoid repeating it?

There is make_exception_future<bool>(std::current_exception()); in StreamSendWorker which wouldn't work then

maybe we could have two, or an overload for Exception=std::exception_ptr, something like:

template <typename T> std::future<T> make_exception_future(std::exception_ptr&& e) { std::promise<T> promise; promise.set_exception(std::move(e)); return promise.get_future(); } template <typename T, typename Exception> std::future<T> make_exception_future(Exception&& e) { return make_exception_future(std::make_exception_ptr(std::move(e))); }

That would work. Looks nice, thx!

rdumusc · 2017-08-09T07:20:22Z

tests/cpp/StreamTests.cpp

+    deflect::Stream stream("id", "dummyhost");
+    std::vector<unsigned char> pixels(4 * 4 * 4);
+
+    std::vector<deflect::PixelFormat> unsupportedUncompressedFormats = {


name it allFormats here? I think you can even use auto for the variable type (init list).

rdumusc

This improves the readability quite a bit, good job!

rdumusc · 2017-08-09T07:34:47Z

tests/cpp/StreamTests.cpp

+    BOOST_CHECK_THROW(deflect::Stream("id", "localhost"), std::runtime_error);
 }

-BOOST_AUTO_TEST_CASE(testDefaultConstructorReadsEnvironmentVariables)


this test case got lost, but I still valid

It cannot be replicated if a server is used that does not run on port 1701, which shouldn't be hardcoded in a unit test. DEFLECT_PORT as an env variable can be the only solution then.

rdumusc · 2017-08-09T07:41:30Z

tests/cpp/ServerTests.cpp

-    serverThread.wait();
+    processServerMessage();

-    BOOST_CHECK_EQUAL(openedStreams, 0);


no longer checked?

I wasn't fully done yet yesterday, you reviewed too early :) I have not adapted all tests in here yet

should be checked again

rdumusc · 2017-08-09T14:04:11Z

deflect/Observer.h

 {
 public:
+    /** The default communication port */
+    static const unsigned short defaultPortNumber;


use this constant also in the 2 constructors with params (Observer + Stream)

rdumusc · 2017-08-09T14:05:22Z

deflect/StreamSendWorker.cpp

-        return make_exception_future<bool>(std::make_exception_ptr(
-            std::invalid_argument("Pending finish, no send allowed")));
+        return make_exception_future<bool>(
+            std::invalid_argument("Pending finish, no send allowed"));


std::runtime_error here? Does not depend on the arguments

rdumusc · 2017-08-09T14:20:24Z

tests/cpp/StreamTests.cpp

+    deflect::Stream stream("id", "dummyhost");
+    std::vector<unsigned char> pixels(4 * 4 * 4);
+
+    std::vector<deflect::PixelFormat> unsupportedUncompressedFormats = {


rdumusc · 2017-08-09T14:56:49Z

tests/mock/DeflectServer.cpp

+    _thread.wait();
+}
+
+void DeflectServer::processServerMessage()


currently this is waitForMessage(), no processing happens here

rdumusc · 2017-08-09T14:57:38Z

tests/mock/MinimalDeflectServer.h

+
+#include <QThread>
+
+class MinimalDeflectServer


I was going to suggest letting DeflectServer derive from MinimalDeflectServer, but realized they are not the same, because this one is in fact a wrapper for the MockServer. We should find clearer names...

No, we should treat as good now and merge this :)

rdumusc · 2017-08-09T15:14:32Z

tests/mock/MinimalDeflectServer.h

+
+#include <QThread>
+
+class MinimalDeflectServer


…flect servers

Also report stream connection errors in the GUI instead of cerr.

This bug was introduced in BlueBrain#177 and could result in the global QThreadPool threads being locked forever.

This bug was introduced in #177 and could result in the global QThreadPool threads being locked forever.

Also report stream connection errors in the GUI instead of cerr.

Move sendSizeHints and sendData also to Observer

210ea32

dnachbaur requested a review from rdumusc August 7, 2017 16:12

rdumusc reviewed Aug 7, 2017

View reviewed changes

Allow uncompressed small tile sending

476754a

dnachbaur force-pushed the master branch from e3cbb65 to ff5f3d7 Compare August 8, 2017 09:23

rdumusc reviewed Aug 8, 2017

View reviewed changes

dnachbaur force-pushed the master branch from ff5f3d7 to 68f7797 Compare August 8, 2017 16:14

rdumusc reviewed Aug 9, 2017

View reviewed changes

dnachbaur force-pushed the master branch 4 times, most recently from bb78046 to ee4128c Compare August 9, 2017 10:12

Catch errors regarding invalid JPEG quality values

d2a0cd3

dnachbaur force-pushed the master branch from ee4128c to b2058e7 Compare August 9, 2017 14:19

rdumusc reviewed Aug 9, 2017

View reviewed changes

dnachbaur force-pushed the master branch from b2058e7 to 892701d Compare August 9, 2017 15:13

rdumusc approved these changes Aug 9, 2017

View reviewed changes

tests/mock/MinimalDeflectServer.h

#include <QThread>

class MinimalDeflectServer

Copy link

rdumusc Aug 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK :-)

Transform error message printing into exceptions

9007cf4

dnachbaur force-pushed the master branch from 892701d to 0328874 Compare August 9, 2017 15:18

Throw on failed connection to server in stream ctor; refactor test de…

d4a4cdd

…flect servers

dnachbaur force-pushed the master branch from 0328874 to d4a4cdd Compare August 9, 2017 15:23

dnachbaur merged commit 6111f35 into BlueBrain:master Aug 9, 2017

rdumusc pushed a commit to rdumusc/Deflect that referenced this pull request Aug 30, 2017

DesktopStreamer: fix uncaught exception after BlueBrain#177

b34d8ba

Also report stream connection errors in the GUI instead of cerr.

rdumusc pushed a commit to rdumusc/Deflect that referenced this pull request Aug 30, 2017

DesktopStreamer: fix uncaught exception after BlueBrain#177

4dc2db9

Also report stream connection errors in the GUI instead of cerr.

rdumusc pushed a commit to rdumusc/Deflect that referenced this pull request Aug 30, 2017

Fixed unit test deadlock due to incorrect execption handling

20a51de

This bug was introduced in BlueBrain#177 and could result in the global QThreadPool threads being locked forever.

rdumusc mentioned this pull request Aug 30, 2017

Fixed unit test deadlock due to incorrect execption handling #181

Merged

rdumusc pushed a commit that referenced this pull request Aug 30, 2017

Fixed unit test deadlock due to incorrect execption handling

a9c51a1

This bug was introduced in #177 and could result in the global QThreadPool threads being locked forever.

rdumusc pushed a commit to rdumusc/Deflect that referenced this pull request Aug 30, 2017

DesktopStreamer: fix uncaught exception after BlueBrain#177

9f521cd

Also report stream connection errors in the GUI instead of cerr.

rdumusc pushed a commit that referenced this pull request Aug 30, 2017

DesktopStreamer: fix uncaught exception after #177 (#180)

6e260aa

Also report stream connection errors in the GUI instead of cerr.

Conversation

dnachbaur commented Aug 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rdumusc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnachbaur commented Aug 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbpbuildbot commented Aug 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdumusc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnachbaur Aug 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

dnachbaur commented Aug 7, 2017 •

edited

Loading

dnachbaur Aug 9, 2017 •

edited

Loading