Deflect stereo streaming API refactoring #153

eile · 2017-03-07T14:12:15Z

FYI @rdumusc @tribal-tec: This is WIP and not fully tested yet. Just opening the PR in case you want to comment already.

rdumusc · 2017-03-07T15:07:10Z

deflect/SegmentParameters.h

    /** Is the image raw pixel data or compressed in jpeg format */
    bool compressed = true;
+
+    View view = View::mono; //!< Eye pass for the segment


I had started like this but this breaks compatibility with older clients because the serialization code uses sizeof(SegmentParameters). I don't think a workaround is possible for all use cases without a proper protocol version exchange, which we don't have.

eile · 2017-03-21T09:55:42Z

Should be 90% there now. Please review.

eile · 2017-03-21T09:56:05Z

Btw, DesktopStreamer async streaming helps perf :)

rdumusc · 2017-03-21T10:04:03Z

Ouch, I hadn't noticed that DesktopStreamer was NOT using the async send before...! That must help indeed... Will have a look at the changes.

rdumusc · 2017-03-21T10:32:34Z

deflect/ReceiveBuffer.h

-     *         finishFrameForSource() has already been called for all existing
-     *         sources (TODO DISCL-241).
+     * finishFrameForSource()
+     *         has already been called for all existing source (TODO DISCL-241).


broken indentation

rdumusc · 2017-03-21T10:34:02Z

deflect/Segment.h

    /** Parameters of the segment. */
    SegmentParameters parameters;

+    View view = View::mono; //!< Eye pass for the segment


comment style consistency

It's just a style remark but all other 3 fields in this class use the /** ... */ syntax, and this one in the middle has a //!< ... on the side. The //!< syntax is more compact, but then I would expect all fields to follow the same style... but it's not a big deal ;-)

rdumusc · 2017-03-21T10:37:02Z

deflect/Stream.h

+     *              referenced must remain valid until the send is finished
+     * @return true if the image data could be sent, false otherwise.
+     * @see send()
+     * @version 1.1


@Version 1.6 also for send() and finishFrame()

rdumusc · 2017-03-21T10:47:41Z

deflect/StreamSendWorker.h

-    Stream::Future enqueueImage(const ImageWrapper& image);
+    Stream::Future enqueueImage(const ImageWrapper& image, bool finish);
+
+    Stream::Future enqueueFinish(); //!< Enqueue a finishFrame()


doc style consistency

I don't understand what you mean here. It's standard doxygen, and we use it everywhere?

This is new to me though, I've never used that anywhere ;-) what rule do you apply to choose between //!< and /** ... */, you check if it can fit on one line? It just does not look very consistent to me when I look at the entire file but I can live with it if you like it :-)

rdumusc · 2017-03-21T10:50:10Z

doc/StereoStreaming.md

+The ImageWrapper takes an additional View parameter.
+
+API changes to the Stream class:
+* asyncSend is deprecated but work as before, but only for mono views


work -> works

rdumusc · 2017-03-21T10:52:49Z

doc/StereoStreaming.md

 On the server side, no changes to the Server API (except some cleanups). Each
-Frame dispatched now contains the View information.
+Frame dispatched now contains the View information. A frame is considered
+complete when all connected clients did send a finish.


did send -> have sent

rdumusc · 2017-03-21T10:54:00Z

doc/StereoStreaming.md

 ## Examples

-Example of a stereo 3D client application using the blocking Stream API:
+Example of a stereo 3D client application using synchronous operations:


this section must be corrected to .wait() or be removed

must they? If executed as is, each call should be synchronous, or am I missing something?

Actually you made me realize yesterday that std::future destructor does not wait(), execpt in the special case of std::async(std::launch::async). http://en.cppreference.com/w/cpp/thread/future/%7Efuture
So I think this is not the same as the previous synchronous api. In fact, if the sending takes longer than the rendering this will queue up send requests indefinitely, or more likely crash because an ImageWrapper will point to invalid data...

Gah, you're right. Changing it.

rdumusc · 2017-03-21T11:00:27Z

doc/StereoStreaming.md

-    leftFuture = deflect::qt::make_ready_future<bool>( true );
-    rightFuture = deflect::qt::make_ready_future<bool>( true );
-
-    ImageData leftData, rightData; // must remain valid until sending completes


why remove this? I think it is important to show the correct usage of the api. Without those variables the renderLoop below does not make sense because the "auto future" goes out of scope.

Furthermore there should be a third future for the deflectStream->finishFrame();

Ah, I interpreted it differently. Re-adding.

rdumusc · 2017-03-21T11:04:54Z

tests/cpp/perf/benchmarkStreamer.cpp

        deflectImage.compressionPolicy = deflect::COMPRESSION_OFF;

-        return _stream->send(deflectImage) && _stream->finishFrame();
+        return _stream->send(deflectImage).get() &&


sendAndFinish().get()

damn, one slipped through.

rdumusc · 2017-03-21T11:05:01Z

tests/cpp/perf/benchmarkStreamer.cpp

        deflectImage.compressionQuality = _options.quality;

-        return _stream->send(deflectImage) && _stream->finishFrame();
+        return _stream->send(deflectImage).get() &&


rdumusc · 2017-03-22T09:00:55Z

FYI Tide adaptation: rdumusc/Tide@0e4d1b4

eile · 2017-03-22T10:11:50Z

Ready to squash & merge, as far as I can see.

eile · 2017-03-23T12:22:57Z

Please review two last commits, RTM from my side.

rdumusc

Good otherwise

rdumusc · 2017-03-23T12:32:47Z

deflect/StreamSendWorker.cpp

        const Request& request = _requests.front();
+        const ImageWrapper image(request.image);
+        const uint32_t tasks(request.tasks);
+        PromisePtr promise(request.promise);


I find the way the request is copied quite confusing. I didn't see at first that "image" was a copy, I thought it was wrapping around "request.image". I would simply do in two lines:

const auto request = _requests.front(); _requests.pop_front();

Or if for some reason you prefer having individual variables then I would still suggest C++11 style with auto for readability:

const auto& request = _requests.front(); const auto image = request.image; const auto tasks = request.tasks; const auto promise = request.promise; _requests.pop_front();

Not really needed as those objects are cheap to copy, but it could even be:

auto& request = _requests.front(); const auto image = std::move(request.image); const auto tasks = std::move(request.tasks); const auto promise = std::move(request.promise); _requests.pop_front();

rdumusc · 2017-03-23T12:44:01Z

deflect/StreamSendWorker.cpp

            {
-                request.promise->set_value(false);
+                promise->set_value(false);
                _requests.pop_back();


I think those two pop_back() should no longer be here!

eile · 2017-03-24T10:54:02Z

@rdumusc : I'll let you manage the merge of this, Equalizer and Tide.

rdumusc · 2017-03-24T13:46:13Z

@eile I'll handle Tide later today, but it would be easier if you could just update the sha1 of Deflect in the Equalizer PR and merge

eile · 2017-03-24T14:22:10Z

Will do

eile changed the title ~~FYI @rdumusc @tribal-tec: This is WIP and not fully tested yet. Just opening the PR in case you want to comment already.~~ Defect stereo streaming API refactoring Mar 7, 2017

eile force-pushed the master branch 2 times, most recently from 94d695a to 68168b1 Compare March 7, 2017 14:48

rdumusc reviewed Mar 7, 2017

View reviewed changes

eile force-pushed the master branch 2 times, most recently from 611595d to 3cbdfbd Compare March 21, 2017 09:40

eile assigned rdumusc and ppodhajski Mar 21, 2017

rdumusc reviewed Mar 21, 2017

View reviewed changes

eile pushed a commit to eile/Deflect that referenced this pull request Mar 21, 2017

CR BlueBrain#153

8151752

eile force-pushed the master branch from 8151752 to e0d961e Compare March 21, 2017 12:39

eile pushed a commit to eile/Deflect that referenced this pull request Mar 21, 2017

CR BlueBrain#153

e0d961e

rdumusc pushed a commit to rdumusc/Deflect that referenced this pull request Mar 21, 2017

CR BlueBrain#153

001188f

rdumusc pushed a commit to rdumusc/Deflect that referenced this pull request Mar 21, 2017

CR BlueBrain#153

672c344

eile force-pushed the master branch from e0d961e to b1e3359 Compare March 22, 2017 10:11

eile pushed a commit to eile/Deflect that referenced this pull request Mar 22, 2017

CR BlueBrain#153

b1e3359

eile changed the title ~~Defect stereo streaming API refactoring~~ Deflect stereo streaming API refactoring Mar 22, 2017

eile pushed a commit to eile/Tide that referenced this pull request Mar 22, 2017

Missing adaptation for BlueBrain/Deflect#153

331bf62

rdumusc approved these changes Mar 22, 2017

View reviewed changes

eile force-pushed the master branch from 01498f5 to 6882491 Compare March 23, 2017 10:36

Stefan Eilemann added 4 commits March 23, 2017 13:23

Update stereo streaming spec as per discussions

887194a

test for refactored stereo streaming API

6649fb2

Refactored stereo streaming API part 1: finish semantics

d0c701f

Address SegmentParameter ABI protocol compat

24135a6

Stefan Eilemann and others added 6 commits March 23, 2017 13:23

Refactored stereo streaming API part 2: async stream API

7b51c6b

Fix potential race in DesktopStream async send

deca8fb

CR BlueBrain#153

4686012

Bugfix

84f2b8b

Bugfix: FIFO sending, not stack

7129cce

Optimize concurrency of Stream asynchronous send

20a419c

eile force-pushed the master branch from ad452e7 to 20a419c Compare March 23, 2017 12:24

rdumusc reviewed Mar 23, 2017

View reviewed changes

CR BlueBrain#153

1bb3396

rdumusc approved these changes Mar 23, 2017

View reviewed changes

rdumusc merged commit 6c3d21c into BlueBrain:master Mar 24, 2017

eile pushed a commit to eile/Tide that referenced this pull request Mar 24, 2017

Missing adaptation for BlueBrain/Deflect#153

b99b835

Deflect stereo streaming API refactoring #153

Deflect stereo streaming API refactoring #153

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eile commented Mar 21, 2017

Uh oh!

eile commented Mar 21, 2017

Uh oh!

rdumusc commented Mar 21, 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!

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!

rdumusc commented Mar 22, 2017

Uh oh!

eile commented Mar 22, 2017

Uh oh!

eile commented Mar 23, 2017

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eile commented Mar 24, 2017

Uh oh!

eile commented Mar 7, 2017 •

edited

Loading