loolwsd/MessageQueue.cpp | 35 +++++++++++++++- loolwsd/MessageQueue.hpp | 4 + loolwsd/test/TileQueueTests.cpp | 83 +++++++++++++++++++++++++++++++++++++--- 3 files changed, 113 insertions(+), 9 deletions(-)
New commits: commit a1e9c1eb42c711bbb2ab8293979daeb18c1b6094 Author: Jan Holesovsky <ke...@collabora.com> Date: Tue Oct 4 14:23:42 2016 +0200 Handle the Impress slide previews with low priority + unit test. After processing a tile request for a slide preview, move the rest of them to the end of the queue, so that other events are processed before we render the next preview. Change-Id: I82aa411406d6731b1c0bf3b849780220b5e68110 diff --git a/loolwsd/MessageQueue.cpp b/loolwsd/MessageQueue.cpp index f06f670..ffa5f79 100644 --- a/loolwsd/MessageQueue.cpp +++ b/loolwsd/MessageQueue.cpp @@ -143,6 +143,9 @@ void TileQueue::removeDuplicate(const std::string& tileMsg) // input from clients, but strings we have created ourselves here in C++ code, so probably we // can be sure that the "ver" parameter is always in such a location that this does what we // mean. + // FIXME: also the ver=... is only for debugging from what I can see, so + // double-check if we can actually avoid the 'ver' everywhere in the non-debug + // builds const std::string newMsg = tileMsg.substr(0, tileMsg.find(" ver")); for (size_t i = 0; i < _queue.size(); ++i) @@ -173,19 +176,43 @@ int TileQueue::priority(const std::string& tileMsg) return -1; } +void TileQueue::deprioritizePreviews() +{ + for (size_t i = 0; i < _queue.size(); ++i) + { + const auto front = _queue.front(); + const std::string message(front.data(), front.size()); + + // stop at the first non-tile or non-'id' (preview) message + std::string id; + if (LOOLProtocol::getFirstToken(message) != "tile" || !LOOLProtocol::getTokenStringFromMessage(message, "id", id)) + break; + + _queue.pop_front(); + _queue.push_back(front); + } +} + MessageQueue::Payload TileQueue::get_impl() { - std::vector<TileDesc> tiles; const auto front = _queue.front(); auto msg = std::string(front.data(), front.size()); std::string id; - if (LOOLProtocol::getFirstToken(msg) != "tile" || LOOLProtocol::getTokenStringFromMessage(msg, "id", id)) + bool isTile = (LOOLProtocol::getFirstToken(msg) == "tile"); + bool isPreview = isTile && LOOLProtocol::getTokenStringFromMessage(msg, "id", id); + if (!isTile || isPreview) { // Don't combine non-tiles or tiles with id. Log::trace() << "MessageQueue res: " << msg << Log::end; _queue.pop_front(); + + // de-prioritize the other tiles with id - usually the previews in + // Impress + if (isPreview) + deprioritizePreviews(); + return front; } @@ -201,7 +228,7 @@ MessageQueue::Payload TileQueue::get_impl() // avoid starving - stop the search when we reach a non-tile, // otherwise we may keep growing the queue of unhandled stuff (both // tiles and non-tiles) - if (LOOLProtocol::getFirstToken(prio) != "tile") + if (LOOLProtocol::getFirstToken(prio) != "tile" || LOOLProtocol::getTokenStringFromMessage(prio, "id", id)) break; int p = priority(prio); @@ -220,6 +247,7 @@ MessageQueue::Payload TileQueue::get_impl() _queue.erase(_queue.begin() + prioritized); + std::vector<TileDesc> tiles; tiles.emplace_back(TileDesc::parse(msg)); // Combine as many tiles as possible with the top one. @@ -254,6 +282,7 @@ MessageQueue::Payload TileQueue::get_impl() if (tiles.size() == 1) { msg = tiles[0].serialize("tile"); + Log::trace() << "MessageQueue res: " << msg << Log::end; return Payload(msg.data(), msg.data() + msg.size()); } diff --git a/loolwsd/MessageQueue.hpp b/loolwsd/MessageQueue.hpp index 2a13484..8ce2287 100644 --- a/loolwsd/MessageQueue.hpp +++ b/loolwsd/MessageQueue.hpp @@ -131,6 +131,10 @@ private: /// Search the queue for a duplicate tile and remove it (if present). void removeDuplicate(const std::string& tileMsg); + /// De-prioritize the previews (tiles with 'id') - move them to the end of + /// the queue. + void deprioritizePreviews(); + /// Priority of the given tile message. /// -1 means the lowest prio (the tile does not intersect any of the cursors), /// the higher the number, the bigger is priority [up to _viewOrder.size()-1]. diff --git a/loolwsd/test/TileQueueTests.cpp b/loolwsd/test/TileQueueTests.cpp index efe13cf..964df48 100644 --- a/loolwsd/test/TileQueueTests.cpp +++ b/loolwsd/test/TileQueueTests.cpp @@ -43,6 +43,7 @@ class TileQueueTests : public CPPUNIT_NS::TestFixture CPPUNIT_TEST(testTileCombinedRendering); CPPUNIT_TEST(testTileRecombining); CPPUNIT_TEST(testViewOrder); + CPPUNIT_TEST(testPreviewsDeprioritization); CPPUNIT_TEST_SUITE_END(); @@ -50,6 +51,7 @@ class TileQueueTests : public CPPUNIT_NS::TestFixture void testTileCombinedRendering(); void testTileRecombining(); void testViewOrder(); + void testPreviewsDeprioritization(); }; void TileQueueTests::testTileQueuePriority() @@ -131,6 +133,15 @@ void TileQueueTests::testTileCombinedRendering() CPPUNIT_ASSERT_EQUAL(payloadFull, queue.get()); } +namespace { + +std::string payloadAsString(const MessageQueue::Payload& payload) +{ + return std::string(payload.data(), payload.size()); +} + +} + void TileQueueTests::testTileRecombining() { TileQueue queue; @@ -142,8 +153,7 @@ void TileQueueTests::testTileRecombining() CPPUNIT_ASSERT_EQUAL(3, static_cast<int>(queue._queue.size())); // but when we later extract that, it is just one "tilecombine" message - MessageQueue::Payload payload(queue.get()); - std::string message(payload.data(), payload.size()); + std::string message(payloadAsString(queue.get())); CPPUNIT_ASSERT_EQUAL(std::string("tilecombine part=0 width=256 height=256 tileposx=7680,0,3840 tileposy=0,0,0 imgsize=0,0,0 tilewidth=3840 tileheight=3840"), message); @@ -174,18 +184,79 @@ void TileQueueTests::testViewOrder() for (auto &tile : tiles) queue.put(tile); - CPPUNIT_ASSERT_EQUAL(4, static_cast<int>(tiles.size())); + CPPUNIT_ASSERT_EQUAL(4, static_cast<int>(queue._queue.size())); // should result in the 3, 2, 1, 0 order of the tiles thanks to the cursor // positions for (size_t i = 0; i < tiles.size(); ++i) { - MessageQueue::Payload payload(queue.get()); - std::string message(payload.data(), payload.size()); + CPPUNIT_ASSERT_EQUAL(tiles[3 - i], payloadAsString(queue.get())); + } +} + +void TileQueueTests::testPreviewsDeprioritization() +{ + TileQueue queue; + + // simple case - put previews to the queue and get everything back again + const std::vector<std::string> previews = + { + "tile part=0 width=180 height=135 tileposx=0 tileposy=0 tilewidth=15875 tileheight=11906 ver=-1 id=0", + "tile part=1 width=180 height=135 tileposx=0 tileposy=0 tilewidth=15875 tileheight=11906 ver=-1 id=1", + "tile part=2 width=180 height=135 tileposx=0 tileposy=0 tilewidth=15875 tileheight=11906 ver=-1 id=2", + "tile part=3 width=180 height=135 tileposx=0 tileposy=0 tilewidth=15875 tileheight=11906 ver=-1 id=3" + }; - CPPUNIT_ASSERT_EQUAL(tiles[3 - i], message); + for (auto &preview : previews) + queue.put(preview); + + for (size_t i = 0; i < previews.size(); ++i) + { + CPPUNIT_ASSERT_EQUAL(previews[i], payloadAsString(queue.get())); } + // stays empty after all is done + CPPUNIT_ASSERT_EQUAL(0, static_cast<int>(queue._queue.size())); + + // re-ordering case - put previews and normal tiles to the queue and get + // everything back again but this time the tiles have to interleave with + // the previews + const std::vector<std::string> tiles = + { + "tile part=0 width=256 height=256 tileposx=0 tileposy=0 tilewidth=3840 tileheight=3840 ver=-1", + "tile part=0 width=256 height=256 tileposx=0 tileposy=7680 tilewidth=3840 tileheight=3840 ver=-1" + }; + + for (auto &preview : previews) + queue.put(preview); + + queue.put(tiles[0]); + + CPPUNIT_ASSERT_EQUAL(previews[0], payloadAsString(queue.get())); + CPPUNIT_ASSERT_EQUAL(tiles[0], payloadAsString(queue.get())); + CPPUNIT_ASSERT_EQUAL(previews[1], payloadAsString(queue.get())); + + queue.put(tiles[1]); + + CPPUNIT_ASSERT_EQUAL(previews[2], payloadAsString(queue.get())); + CPPUNIT_ASSERT_EQUAL(tiles[1], payloadAsString(queue.get())); + CPPUNIT_ASSERT_EQUAL(previews[3], payloadAsString(queue.get())); + + // stays empty after all is done + CPPUNIT_ASSERT_EQUAL(0, static_cast<int>(queue._queue.size())); + + // cursor positioning case - the cursor position should not prioritize the + // previews + queue.updateCursorPosition(0, 0, 0, 0, 10, 100); + + queue.put(tiles[1]); + queue.put(previews[0]); + + CPPUNIT_ASSERT_EQUAL(tiles[1], payloadAsString(queue.get())); + CPPUNIT_ASSERT_EQUAL(previews[0], payloadAsString(queue.get())); + + // stays empty after all is done + CPPUNIT_ASSERT_EQUAL(0, static_cast<int>(queue._queue.size())); } CPPUNIT_TEST_SUITE_REGISTRATION(TileQueueTests); _______________________________________________ Libreoffice-commits mailing list libreoffice-comm...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits