Assertion in DocumentBroker::sendRequestedTiles fails on running cypr…

…ess impress tests make -C cypress_test check-desktop asserts seen in cypress_test/cypress/wsd_logs/coolwsd_output.log of: coolwsd: wsd/DocumentBroker.cpp:3134: void DocumentBroker::sendTileCombine(const TileCombined&): Assertion `!newTileCombined.hasDuplicates()' failed. If we check for, and don't reuse, an old request with a different NormalizedViewId then we could end up with multiple requests with different NormalizedViewIds that end up in the same final tilecombine. similarly there was no check for different modes ending up in the same tilecombine. just split out the logic we have to see if two tiles have the same properties that appear as a shared set of properties for tilecombine and use that in the two relevant places. Signed-off-by: Caolán McNamara <[email protected]> Change-Id: Ieb2ee0e85f124dd57c6b050e5b669dd808cf6bbf
CollaboraOnline · Oct 17, 2023 · 395d6c9 · 395d6c9
1 parent bb24631
commit 395d6c9
Show file tree

Hide file tree

Showing 2 changed files with 15 additions and 16 deletions.
diff --git a/wsd/DocumentBroker.cpp b/wsd/DocumentBroker.cpp
@@ -3206,10 +3206,7 @@ void DocumentBroker::handleTileCombinedRequest(TileCombined& tileCombined, bool
             {
                 if(oldTile.getTilePosX() == newTile.getTilePosX() &&
                    oldTile.getTilePosY() == newTile.getTilePosY() &&
-                   oldTile.getNormalizedViewId() == newTile.getNormalizedViewId() &&
-                   oldTile.getPart() == newTile.getPart() &&
-                   oldTile.getTileWidth() == newTile.getTileWidth() &&
-                   oldTile.getTileHeight() == newTile.getTileHeight())
+                   oldTile.sameTileCombineParams(newTile))
                 {
                     oldTile.setVersion(newTile.getVersion());
                     oldTile.setOldWireId(newTile.getOldWireId());
@@ -3328,13 +3325,6 @@ void DocumentBroker::handleMediaRequest(std::string range,
     }
 }
 
-static bool samePartAndSize(const TileDesc& tileA, const TileDesc& tileB)
-{
-    return tileA.getPart() == tileB.getPart() &&
-           tileA.getTileWidth() == tileB.getTileWidth() &&
-           tileA.getTileHeight() == tileB.getTileHeight();
-}
-
 void DocumentBroker::sendRequestedTiles(const std::shared_ptr<ClientSession>& session)
 {
     std::unique_lock<std::mutex> lock(_mutex);
@@ -3419,7 +3409,7 @@ void DocumentBroker::sendRequestedTiles(const std::shared_ptr<ClientSession>& se
                         LOG_TRC("Forcing keyframe for tile was oldwid " << tile.getOldWireId());
                         tile.setOldWireId(0);
                     }
-                    allSamePartAndSize &= tilesNeedsRendering.empty() || samePartAndSize(tilesNeedsRendering.back(), tile);
+                    allSamePartAndSize &= tilesNeedsRendering.empty() || tile.sameTileCombineParams(tilesNeedsRendering.back());
                     tilesNeedsRendering.push_back(tile);
                     _debugRenderedTileCount++;
                 }
@@ -3434,12 +3424,12 @@ void DocumentBroker::sendRequestedTiles(const std::shared_ptr<ClientSession>& se
         {
             if (allSamePartAndSize)
             {
-                // typically all requests are for the same part and tilesize
+                // typically all requests match sufficiently to form a single tilecombine
                 sendTileCombine(TileCombined::create(tilesNeedsRendering));
             }
             else
             {
-                // but if not, split them by part+tilesize to send a separate
+                // but if not, split them by matching groups of requests to send a separate
                 // tilecombine for each group
                 std::vector<std::vector<TileDesc>> groupsNeedsRendering(1);
                 auto it = tilesNeedsRendering.begin();
@@ -3451,7 +3441,7 @@ void DocumentBroker::sendRequestedTiles(const std::shared_ptr<ClientSession>& se
                     // check if tile should go into an existing group bucket
                     for (size_t i = 0; i < groupsNeedsRendering.size(); ++i)
                     {
-                        if (samePartAndSize(*it, groupsNeedsRendering[i][0]))
+                        if (it->sameTileCombineParams(groupsNeedsRendering[i][0]))
                         {
                             groupsNeedsRendering[i].push_back(*it);
                             inserted = true;

diff --git a/wsd/TileDesc.hpp b/wsd/TileDesc.hpp
@@ -163,7 +163,9 @@ class TileDesc final
         return intersects(other);
     }
 
-    bool onSameRow(const TileDesc& other) const
+    // return false if the TileDesc cannot appear in the same TileCombine
+    // because their fields differ for the shared tilecombine case
+    bool sameTileCombineParams(const TileDesc& other) const
     {
         if (other.getPart() != getPart() ||
             other.getEditMode() != getEditMode() ||
@@ -175,6 +177,13 @@ class TileDesc final
         {
             return false;
         }
+        return true;
+    }
+
+    bool onSameRow(const TileDesc& other) const
+    {
+        if (!sameTileCombineParams(other))
+            return false;
 
         return other.getTilePosY() + other.getTileHeight() >= getTilePosY() &&
                other.getTilePosY() <= getTilePosY() + getTileHeight();