From 395d6c9f1284d8e6b3548b0ed3ba30d2bcb15057 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Caol=C3=A1n=20McNamara?= <caolan.mcnamara@collabora.com>
Date: Tue, 17 Oct 2023 10:28:06 +0100
Subject: [PATCH] Assertion in DocumentBroker::sendRequestedTiles fails on
 running cypress impress tests
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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 <caolan.mcnamara@collabora.com>
Change-Id: Ieb2ee0e85f124dd57c6b050e5b669dd808cf6bbf
---
 wsd/DocumentBroker.cpp | 20 +++++---------------
 wsd/TileDesc.hpp       | 11 ++++++++++-
 2 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/wsd/DocumentBroker.cpp b/wsd/DocumentBroker.cpp
index 2ae582afe1e5..c16abfbdc069 100644
--- 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
index 467470bdc5b6..25f1ea6e0da7 100644
--- 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();