Skip to content

Commit

Permalink
deltas: fold the update: command back into an empty delta: command.
Browse files Browse the repository at this point in the history 
Also: address parts of CollaboraOnline#6897, primarily:

* remove the problematic aspect of bumping the last wid in our
  TileData, when this could trigger a re-send of a previously
  sent delta, causing tile corruption.
    * instead append an empty wid entry.
    * as an optimization - if the last entry is empty update
      the wid - since re-sending an empty delta is of no
      concern.

* simplify a number of code-paths that special-case zero length
  deltas. All deltas now commence with 'D'.

* still track updates in the JS - by detecting empty deltas.

* shares more code and simplifies various paths.

Signed-off-by: Michael Meeks <michael.meeks@collabora.com>
Change-Id: I02af6d4b152524c201b6985b7a3497da7f08a517
  • Loading branch information
@mmeeks @rparth07
mmeeks authored and rparth07 committed Aug 5, 2023
1 parent 2475e50 commit 14e1020
Show file tree
Hide file tree
Showing 8 changed files with 71 additions and 94 deletions.
35 changes: 19 additions & 16 deletions browser/src/layer/tile/CanvasTileLayer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1552,8 +1552,7 @@ L.CanvasTileLayer = L.Layer.extend({
_onMessage: function (textMsg, img) {
this._saveMessageForReplay(textMsg);
// 'tile:' is the most common message type; keep this the first.
if (textMsg.startsWith('tile:') || textMsg.startsWith('delta:') ||
textMsg.startsWith('update:')) {
if (textMsg.startsWith('tile:') || textMsg.startsWith('delta:')) {
this._onTileMsg(textMsg, img);
}
else if (textMsg.startsWith('commandvalues:')) {
Expand Down Expand Up @@ -6807,12 +6806,14 @@ L.CanvasTileLayer = L.Layer.extend({
tile.deltaCount = 0;
tile.updateCount = 0;
}
else if (rawDelta.length === 0)
{
tile.updateCount++;
return; // that was easy
}
else
tile.deltaCount++;

if (rawDelta.length === 0)
return; // that was easy!

var traceEvent = app.socket.createCompleteTraceEvent('L.CanvasTileLayer.applyDelta',
{ keyFrame: isKeyframe, length: rawDelta.length });

Expand All @@ -6825,6 +6826,11 @@ L.CanvasTileLayer = L.Layer.extend({
tile.rawDeltas.length = 0;
tile.rawDeltas = rawDelta; // overwrite
}
else if (!tile.rawDeltas)
{
window.app.console.log('Unusual: attempt to append a delta when we have no keyframe.');
return;
}
else // assume we already have a delta.
{
// FIXME: this is not beautiful; but no concatenate here.
Expand Down Expand Up @@ -7068,17 +7074,14 @@ L.CanvasTileLayer = L.Layer.extend({
L.Log.log(textMsg, 'INCOMING', key);

// updates don't need more chattiness with a tileprocessed
if (!textMsg.startsWith('update:'))
{
this._applyDelta(tile, img.rawData, img.isKeyframe);
this._tileReady(coords);

// Queue acknowledgment, that the tile message arrived
var mode = (tileMsgObj.mode !== undefined) ? tileMsgObj.mode : 0;
var tileID = tileMsgObj.part + ':' + mode + ':' + tileMsgObj.x + ':' + tileMsgObj.y + ':'
+ tileMsgObj.tileWidth + ':' + tileMsgObj.tileHeight + ':' + tileMsgObj.nviewid;
this._queuedProcessed.push(tileID);
}
this._applyDelta(tile, img.rawData, img.isKeyframe);
this._tileReady(coords);

// Queue acknowledgment, that the tile message arrived
var mode = (tileMsgObj.mode !== undefined) ? tileMsgObj.mode : 0;
var tileID = tileMsgObj.part + ':' + mode + ':' + tileMsgObj.x + ':' + tileMsgObj.y + ':'
+ tileMsgObj.tileWidth + ':' + tileMsgObj.tileHeight + ':' + tileMsgObj.nviewid;
this._queuedProcessed.push(tileID);
},

_sendProcessedResponse: function() {
Expand Down
5 changes: 3 additions & 2 deletions kit/Delta.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -450,8 +450,9 @@ class DeltaGenerator {
{
// The tile content is identical to what the client already has, so skip it
LOG_TRC("Identical / un-changed tile");
// Return a zero byte image to inform WSD we didn't need that.
// This allows WSD side TileCache to free up waiting subscribers.
// Return a zero length delta to inform WSD we didn't need that.
// This allows WSD side TileCache to send updates to waiting subscribers.
outStream.push_back('D');
return true;
}

Expand Down
11 changes: 6 additions & 5 deletions test/TileCacheTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -280,11 +280,12 @@ void TileCacheTests::testSimpleCombine()
LOK_ASSERT_MESSAGE("did not receive a tile: message as expected", !tile1b.empty());

sendTextFrame(socket1, "tilecombine nviewid=0 part=0 width=256 height=256 tileposx=0,3840 tileposy=0,0 oldwid=42,42 tilewidth=3840 tileheight=3840");
tile1a = getResponseMessage(socket1, "update:", testname + "1 ");
// no content in an update:
LOK_ASSERT_MESSAGE("did not receive a update: message as expected", tile1a.empty());
tile1b = getResponseMessage(socket1, "update:", testname + "1 ");
LOK_ASSERT_MESSAGE("did not receive a update: message as expected", tile1b.empty());
tile1a = getResponseMessage(socket1, "delta:", testname + "1 ", std::chrono::seconds(10));
// TST_LOG("Response is: " + Util::dumpHex(tile1a) << "\n");
// no content in an update delta: - so ends with a '\n'
LOK_ASSERT_MESSAGE("did not receive an update delta: message as expected", !tile1a.empty() && tile1a.back() == '\n');
tile1b = getResponseMessage(socket1, "delta:", testname + "1 ");
LOK_ASSERT_MESSAGE("did not receive an update delta: message as expected", !tile1b.empty() && tile1b.back() == '\n');

// Second.
TST_LOG("Connecting second client.");
Expand Down
12 changes: 12 additions & 0 deletions test/WhiteBoxTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -677,6 +677,18 @@ void WhiteBoxTests::testTileData()
out.clear();
LOK_ASSERT_EQUAL(data.appendChangesSince(out, 43), true);
LOK_ASSERT_EQUAL(std::string("baabaz"), Util::toString(out));

// append an empty delta
data.appendBlob(52, "D", 1);
LOK_ASSERT_EQUAL(data.size(), size_t(9));
LOK_ASSERT_EQUAL(data._wids.size(), size_t(4));
LOK_ASSERT_EQUAL(data._wids.back(), unsigned(52));

// the next empty delta should pack into the last one
data.appendBlob(54, "D", 1);
LOK_ASSERT_EQUAL(data.size(), size_t(9));
LOK_ASSERT_EQUAL(data._wids.size(), size_t(4));
LOK_ASSERT_EQUAL(data._wids.back(), unsigned(54));
}

void WhiteBoxTests::testRectanglesIntersect()
Expand Down
17 changes: 5 additions & 12 deletions wsd/ClientSession.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,20 +117,13 @@ class ClientSession final : public Session
// FIXME: performance - optimize away this copy ...
std::vector<char> output;

// copy in the header
output.resize(header.size());
std::memcpy(output.data(), header.data(), header.size());
if (tile->appendChangesSince(output, tile->isPng() ? 0 : lastSentId))
{
LOG_TRC("Sending tile message: " << header << " lastSendId " << lastSentId);
return sendBinaryFrame(output.data(), output.size());
}
else
{
LOG_TRC("wasteful redundant tile request: " << lastSentId);
header = desc.serialize("update:", "\n");
return sendTextFrame(output.data(), output.size());
}
return true;

bool hasContent = tile->appendChangesSince(output, tile->isPng() ? 0 : lastSentId);
LOG_TRC("Sending tile message: " << header << " lastSendId " << lastSentId << " content " << hasContent);
return sendBinaryFrame(output.data(), output.size());
}

bool sendBlob(const std::string &header, const Blob &blob)
Expand Down
41 changes: 1 addition & 40 deletions wsd/TileCache.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,34 +184,6 @@ void TileCache::saveTileAndNotify(const TileDesc& desc, const char *data, const

std::shared_ptr<TileBeingRendered> tileBeingRendered = findTileBeingRendered(desc);

if (size <= 0)
{
LOG_TRC("Zero sized cache tile: " << cacheFileName(desc));

if (tileBeingRendered)
{
updateWidInCache(desc);

const size_t subscriberCount = tileBeingRendered->getSubscribers().size();

// notify that the tile was re-rendered, with no change.
for (size_t i = 0; i < subscriberCount; ++i)
{
auto& subscriber = tileBeingRendered->getSubscribers()[i];
std::shared_ptr<ClientSession> session = subscriber.lock();
if (session)
session->sendUpdateNow(desc);
}

LOG_DBG("STATISTICS: tile " << desc.getVersion() << " internal roundtrip to empty tile " <<
tileBeingRendered->getElapsedTimeMs());

// un-subscribe subscribers, if any.
forgetTileBeingRendered(desc, tileBeingRendered);
}
return;
}

// Save to in-memory cache.

// Ignore if we can't save the tile, things will work anyway, but slower.
Expand All @@ -228,7 +200,7 @@ void TileCache::saveTileAndNotify(const TileDesc& desc, const char *data, const
const size_t subscriberCount = tileBeingRendered->getSubscribers().size();

// sendTile also does enqueueSendMessage underneath ...
if (tile && size > 0 && subscriberCount > 0)
if (tile && subscriberCount > 0)
{
for (size_t i = 0; i < subscriberCount; ++i)
{
Expand Down Expand Up @@ -511,17 +483,6 @@ Tile TileCache::findTile(const TileDesc &desc)
return Tile();
}

// Used when we get a zero delta - to update the wire-id
void TileCache::updateWidInCache(const TileDesc& desc)
{
Tile tile = _cache[desc];
if (tile)
{
LOG_TRC("Bumped wid on " << desc.serialize());
tile->bumpLastWid(desc.getWireId());
}
}

Tile TileCache::saveDataToCache(const TileDesc &desc, const char *data, const size_t size)
{
if (_dontCache)
Expand Down
36 changes: 22 additions & 14 deletions wsd/TileCache.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,12 +93,28 @@ struct TileData

size_t oldSize = size();

// FIXME: too many/large deltas means we should reset -
// but not here - when requesting the tiles.
_wids.push_back(id);
_offsets.push_back(_deltas.size());
_deltas.resize(oldSize + dataSize - 1);
std::memcpy(_deltas.data() + oldSize, data + 1, dataSize - 1);
// If we have an empty delta at the end - then just
// bump the associated wid. There is no risk to sending
// an empty delta twice.x
if (dataSize == 1 && // just a 'D'
_offsets.size() > 1 &&
_offsets.back() == _deltas.size())
{
LOG_TRC("received empty delta - bumping wid from " << _wids.back() << " to " << id);
_wids.back() = id;
}
else
{
// FIXME: too many/large deltas means we should reset -
// but not here - when requesting the tiles.
_wids.push_back(id);
_offsets.push_back(_deltas.size());
if (dataSize > 1)
{
_deltas.resize(oldSize + dataSize - 1);
std::memcpy(_deltas.data() + oldSize, data + 1, dataSize - 1);
}
}

// FIXME: possible race - should store a seq. from the invalidation(s) ?
_valid = true;
Expand Down Expand Up @@ -139,13 +155,6 @@ struct TileData
return since < _wids[0];
}

/// we now know this wid is really the latest
void bumpLastWid(TileWireId since)
{
assert(_wids.size() > 0);
_wids.back() = since;
}

bool appendChangesSince(std::vector<char> &output, TileWireId since)
{
size_t i;
Expand Down Expand Up @@ -291,7 +300,6 @@ class TileCache
static bool intersectsTile(const TileDesc &tileDesc, int part, int mode, int x, int y,
int width, int height, int normalizedViewId);

void updateWidInCache(const TileDesc& desc);
Tile saveDataToCache(const TileDesc& desc, const char* data, size_t size);
void saveDataToStreamCache(StreamType type, const std::string& fileName, const char* data,
size_t size);
Expand Down
8 changes: 3 additions & 5 deletions wsd/protocol.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,9 @@ tilecombine <parameters>
comma-separated lists, and the number of elements in each must be
same.

if oldwid=0 this forces returning from the previous keyframe
for this tile - suitable for force-refreshing the data.

dialog <command>

<command> is unique identifier for the dialog that needs to be painted.
Expand Down Expand Up @@ -528,11 +531,6 @@ delta: part=<partNumber> width=<width> height=<height> tileposx=<xpos> tileposy=
A delta command is like a tile: command but the payload is purely
an incremental patch on top of a previous tile.

update: <as delta>

An update command simply informs the client of any updated wid
ie. this tile was re-rendered and returned an empty delta.

commandresult: <payload>

This is used to acknowledge the commands from the client.
Expand Down

0 comments on commit 14e1020

Please sign in to comment.