so it doesn't sound that serious like previous message
and is less generic and more easy to find in the code
Signed-off-by: Szymon Kłos <szymon.klos@collabora.com>
Change-Id: I3eb746af8aef39b566d3213ab2935372219e8d98
this is visible even with hello-world.odt in the debugging overlay where
I have 9 cols and 7 rows visible.
Load hello-world.odt and from the 4th row, 8th col onwards each tile has
a "upd: 1" of an additional empty delta update to the original tile
browser-side:
a) we have one OUTGOING: tilecombine request which which requests an
initial 72 tiles (9 cols, 8 rows)
b) we then receive 72 tiles as requested
c) and browser sends back tileprocessed for each
d) but we then get a series of (38) delta: requests after that which are
unexplained
server-side:
a) on the initial tilecombine, DocumentBroker::handleTileCombinedRequest
sends the 72 requested tiles for rendering and registers to send each when
ready.
for (auto& tile : tileCombined.getTiles())
{
...
tilesNeedsRendering.push_back(tile);
...
tileCache().subscribeToTileRendering(tile, session, now);
}
// Send rendering request, prerender before we actually send the tiles
if (!tilesNeedsRendering.empty())
sendTileCombine(TileCombined::create(tilesNeedsRendering));
and stores what tiles it want to send in session->getRequestedTiles()
before calling sendRequestedTiles(session);
b) at this sendRequestedTiles (also later when tileprocessed is seen from
each tile response from the browser which also calls sendRequestedTiles), then:
c) DocumentBroker::sendRequestedTiles loops over existing requests and drops
from session->getRequestedTiles() both the tiles that it can send immediately,
and those that are queued to get rendered.
d) But it only does this for a max amount of tiles, based on beingRendered, up to
a tilesOnFlyUpperLimit. beingRendered is bumped for each tile not ready yet,
on the assumption that it needs to be rendered.
e) But we already have some getting rendered, and bump beingRendered anyway,
so tilesOnFlyUpperLimit can easily get exceeded on a first page, typically this
first sendRequestedTiles loop stops early, and stops dropping tiles from the
request queue that are already queued to be rendered.
f) at some point we get a tileprocessed and sendRequestedTiles is called again,
the request queue wasn't emptied, and by now it is likely the tile cache has
results for them (which were already sent) and sendTileNow is used to send those,
resulting in additional empty deltas sent for fulfilled queries.
logs will show "Redundant request to subscribe on tile" warnings in this case
Here as a conservative improvement only increase beingRendered if the sendRequestedTiles
subscribeToTileRendering actually does anything.
There is a mismatch in what handleTileCombinedRequest does vs what
sendRequestedTiles does. Maybe handleTileCombinedRequest should leave it
to sendRequestedTiles to do the sendTileCombine, or maybe
handleTileCombinedRequest shouldn't add those tiles to the session
requestedTiles.
Signed-off-by: Caolán McNamara <caolan.mcnamara@collabora.com>
Change-Id: I3044f4b3e47f00c680aa5b87dd7bdad2f27e8c73
Also: address parts of #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
also re-work parseInvalidateMsg to not use std::pair<> returns.
Change-Id: I724783b7050a1ee251dfbe9b2f1e0ea02e7d45f2
Signed-off-by: Michael Meeks <michael.meeks@collabora.com>
A zero size delta means we're effectively up-to-date for this new
wireId.
Change-Id: I96abdf00648f7c2f833ffd56bd43980265b8d490
Signed-off-by: Michael Meeks <michael.meeks@collabora.com>
This stopped working well a long time ago, and is already disabled
on the coolwsd side, so dung out the rest of the code & docs on this.
Change-Id: I2e0b73fe9780e16c3cc74ae3a38ae6b04434717a
Signed-off-by: Michael Meeks <michael.meeks@collabora.com>
This merges multiple implementations of
assertCorrectThread and simplifies its
usage.
Change-Id: I7be5dea62c6046fb0412d7f885fcbcc4b66e3fd9
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
If we are not caching tiles, it is misleading to log "Saved cache tile".
Signed-off-by: Tor Lillqvist <tml@collabora.com>
Change-Id: I0e0ec446d5edf266ea4acb42883c49d5f7063763
It seems we were inadvertently clearing copies
of the stream caches rather than the real ones.
Change-Id: I3fcce77850e0a8f082bdb926f85ab173148c15db
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
We need to return a real Tile for _dontCache, and an empty one
if we want to ignore the canceltiles race.
Change-Id: Iad57fc1e3a2718aec34c0eeff97d5f58e215d174
Signed-off-by: Michael Meeks <michael.meeks@collabora.com>
It ensures that we pair un-subscription for a tile change with the
request - since we can have multiple tile requests in flight at a
given time.
Change-Id: I3a00ff7f43fc8ef203b8e38c169938438286d75f
Signed-off-by: Michael Meeks <michael.meeks@collabora.com>
These will get newer wids later of course.
Signed-off-by: Michael Meeks <michael.meeks@collabora.com>
Change-Id: I906f47255854eb065d41e629508867ffa6e9f953
Squashed from feature/deltas-expanded.
TileCache changes:
+ add montonic sequence (wid) numbers to TileData
+ account for sizes of TileData with multiple blobs
+ simplify saving and notifying of tiles
Sends updates (via appendChanges) based on the sequence the
right mix of keyframes and/or deltas required as a single
message, and parse and apply those on the JS side.
We continue to use PNG for slide previews and dialogs,
but remove PngCache - used by document tiles only.
Annotates delta: properly as a binary package for the websocket.
Distinguishes between deltas and keyframes we get from
the Kit based on an initial un-compressed prefix
character which we then discard.
kit can be forced to render a keyframe by oldWid=0
Track invalidity on tiles themselves - to keep the keyframe around.
We need to be able to track that a tile is invalid, and so subscribe
to the updated version as/when it is ready - but we also want to
store the keyframe underneath any deltas.
force rendering of a keyframe for an empty slot in the TileCache.
force tile sequence to be zero for combinedtiles - so the client can
always request standalone tiles with explicit combinedtiles, or tile
requests.
move Blob to Common.hpp
use zero size for un-changed tiles.
remove obsolete render-id, and color deltas in debug mode.
cleanup unit tests for non-png tile results.
Change-Id: I987f84ac4e58004758a243c233b19a6f0d60f8c2
Signed-off-by: Michael Meeks <michael.meeks@collabora.com>
For each tile, the wire-id will grow incrementally for each key-frame
or delta appended; so we can tell which deltas to send.
Signed-off-by: Michael Meeks <michael.meeks@collabora.com>
Change-Id: Ic5f201d23273c37eed232c5932b9da854e80d06a
Signed-off-by: Michael Meeks <michael.meeks@collabora.com>
In an attempt to reduce the size of Util.{c,h}pp
which has grown to contain all sorts of unrelated
helpers, we move StringVector helpers into
the StringVector.{c,h}pp files.
This makes the code better organized.
Change-Id: I152f341606807ae66253415b951bc9f89b09df57
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
In 'debug' log-level we expect a detailed, but
still readable output. Having one area with
disproportionately large number of logs reduces
the overall utility of the log output.
This patch reduces a number of redundant log
entries, including errors that are already
logged. It also reduces the level of some
others from 'information' to 'debug' and
from 'debug' to 'trace'.
The goal is to make 'debug' level as useful as
possible to read the progress and be able to
understand what was going on, such that one is
able to decide which area to dig deeper into.
Then, trace level could be used to get more
insight into that area, if necessary. For
example, when investigating a test failure,
one first enables 'debug' logs and reads through.
Once a section between two debug entries is
identified as being of interest, enabling 'trace'
level logs becomes more productive as it's
now possible to easily reach the first DBG
entry and read through until the second one.
It's unfortunate that we don't have per-area
control for enabling/disabling logs, so it
is common to see more and more 'debug' log
entries added all around, making logs
less and less readable.
It is also a limitation of the levels we have
that we really only have 3 usable levels:
one, two, many. That is, 'information' for
the most important events, 'debug' for
technical details needed to investigate issues,
and 'trace' for everything else. ('warning'
and 'error' aren't really 'levels'; they have
semantics that makes them special-cases.)
So we have to avoid degrading one into the
other, or have differences without distinction.
If any of these entries are needed to be
displayed more frequently, changing them
back to 'debug' or even 'information' should
be done. Though for me they seem special
cases that don't benefit most log readings.
Change-Id: Id2c6a9dc027483b81a066b0b4b50a298c5eff449
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
This adds dumpState to DocumentState, SaveManager,
and StorageManager classes, and dumps all the
missing members.
Also, normalize the format and make it symmetric
and consistent.
Change-Id: Ie0cc8e07d13de60c33d64cd621abf4e815a4ef94
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
Avoiding some debug code ending up in the product.
Change-Id: If37b2986f134986a33b9dd5b4729b50c56cbc248
Signed-off-by: Michael Meeks <michael.meeks@collabora.com>
Also, makes the logging of units much less error prone.
The overloaded streaming operators are temporary as
they are provided in C++20. The ones here (though
incomplete) are fashioned after the C++20 specs.
Change-Id: Ieb499282ccb6e63fa939ba07bed3e5a4fbef1bd0
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
They are especially efficient for small lookups.
Change-Id: Ia005f40127cf222debe185515fc45cd92b8ae752
Reviewed-on: https://gerrit.libreoffice.org/c/online/+/100413
Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoffice@gmail.com>
Tested-by: Jenkins
Reviewed-by: Michael Meeks <michael.meeks@collabora.com>
When we get a wid match, this helps WSD to cleanup its tile
subscriber list effectively.
Change-Id: I6517039fb3d8c9ad8f53aef549b8adbb79961ce1
Reviewed-on: https://gerrit.libreoffice.org/c/online/+/100348
Tested-by: Jenkins
Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoffice@gmail.com>
Reviewed-by: Michael Meeks <michael.meeks@collabora.com>
Stale tiles were still being counted, unhelpfully. Avoid doing lots
of ::now() calls, and yet detect this.
Change-Id: Ib1e4b2f1968c1994849bb23ec54e28f6706230ee
Reviewed-on: https://gerrit.libreoffice.org/c/online/+/100347
Tested-by: Jenkins
Reviewed-by: Michael Meeks <michael.meeks@collabora.com>
* Excised TileCacheDesc to improve performance and simplify code.
* clang-tidy suggestions and auto-rewrite fixes.
* Const-correctness.
* Inlined and improved a couple of trivial functions (that are called
often).
* Reduced some logs from INF to DBG as they are only meaningful to devs.
Change-Id: I1c4eb8c63da49aa061afbf3eb68cae23d4d5e7f3
Reviewed-on: https://gerrit.libreoffice.org/c/online/+/98661
Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoffice@gmail.com>
Tested-by: Jenkins
Reviewed-by: Michael Meeks <michael.meeks@collabora.com>
emplace_back avoids copy-construction when
the argument is a temporary instance created
at call-site.
Change-Id: I127fddd308d710af9ea65a86db1b03347e9c3d87
Reviewed-on: https://gerrit.libreoffice.org/c/online/+/96829
Tested-by: Jenkins
Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoffice@gmail.com>
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
The tokenizer(s) are more generic than the protocol
logic, and are used from contexts that don't involve
the protocol as such.
Change-Id: Ie8c256bf11a91e466bff794021f41603c9596a7f
More readable and typically more efficient.
Change-Id: I9bd5bfc91f4ac255bb8ae0987708fb8b56b398f8
Reviewed-on: https://gerrit.libreoffice.org/c/online/+/95285
Reviewed-by: Michael Meeks <michael.meeks@collabora.com>
Tested-by: Jenkins
Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoffice@gmail.com>
Weak pointers can be null and must be
checked before using. This fixes at least
one segfault and prevents a number of others.
Also, minimizes locking of weak pointers
in the message handlers.
Change-Id: I306501c26c3441d7bd6812d51fa17e7356126f32
Reviewed-on: https://gerrit.libreoffice.org/c/online/+/92828
Reviewed-by: Michael Meeks <michael.meeks@collabora.com>
Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoffice@gmail.com>
Allows comparing tokens with C strings without a heap allocation. Do the
same when comparing two tokens from two different StringVectors.
And use it at all places where operator ==() has an argument, which is a
StringVector::operator []() result.
Change-Id: Id36eff96767ab99b235ecbd12fb14446a3efa869
Reviewed-on: https://gerrit.libreoffice.org/c/online/+/90201
Reviewed-by: Michael Meeks <michael.meeks@collabora.com>
Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoffice@gmail.com>
The bulk of this commit just changes std::vector<std::string> to
StringVector when we deal with tokens from a websocket message.
The less boring part of it is the new StringVector class, which is a
wrapper around std::vector<std::string>, and provides the same API,
except that operator[] returns a string, not a string&, and this allows
returning an empty string in case that prevents reading past the end of
the underlying array.
This means in case client code forgets to check size() before invoking
operator[], we don't crash. (See the ~3 previous commits which fixed
such crashes.)
Later the ctor could be changed to take a single underlying string to
avoid lots of tiny allocations, that's not yet done in this commit.
Change-Id: I8a6082143a8ac0b65824f574b32104d7889c184f
Reviewed-on: https://gerrit.libreoffice.org/c/online/+/89687
Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoffice@gmail.com>
Reviewed-by: Miklos Vajna <vmiklos@collabora.com>
