[Bug] Asynchronous loading of adjacent pages fails

[letsfindaway]

Describe the bug

When opening a document with multiple pages from the document view, you can see the following message in the log output:

QObject::connect: Cannot queue arguments of type 'std::shared_ptr<UBDocumentProxy>'
(Make sure 'std::shared_ptr<UBDocumentProxy>' is registered using qRegisterMetaType().)

The reason is that adjacent pages are asynchronously loaded using the UBPersistenceWorker. When it is finished, it emits

OpenBoard/src/core/UBPersistenceWorker.cpp

Line 81 in 0e69a0a

emit sceneLoaded(UBSvgSubsetAdaptor::loadSceneAsText(info.proxy,info.sceneIndex), info.proxy, info.sceneIndex);

The second parameter to this signal is of type std::shared_ptr<UBDocumentProxy>, which is not known to the meta type system and can therefore not be sent over a queued connection.

To Reproduce

1. Start OpenBoard, create a multi-page document and close it again
2. Start OpenBoard a second time
3. Go to document mode and select the previously created document
4. Click on "Opening Board"

Expected behavior

The first (or the selected) document page should appear. Adjacent pages should be loaded asynchronously to the scene cache.

Actual behavior

The first (or the selected) document page appears as expected. But loading adjacent pages fails and is broken in several aspects:

• First the UBPersistenceWorker reads the scene file text using UBSvgSubsetAdaptor::loadSceneAsText.
• Then the signal described above is emitted, but will never arrive because the shared_ptr cannot be sent over a queued connection.
• But if it arrives, then UBPersistenceManager::onSceneLoaded would be called:
  

  OpenBoard/src/core/UBPersistenceManager.cpp

  Lines 175 to 183 in 0e69a0a

  	void UBPersistenceManager::onSceneLoaded(QByteArray scene, std::shared_ptr<UBDocumentProxy> proxy, int sceneIndex)
  	{
  	Q_UNUSED(scene);
  	qDebug() << "scene loaded " << sceneIndex;
  	QElapsedTimer time;
  	time.start();
  	mSceneCache.insert(proxy, sceneIndex, loadDocumentScene(proxy, sceneIndex, false));
  	qDebug() << "millisecond for sceneCache " << time.elapsed();
  	}

• Here the loaded scene is not used at all. Instead loadDocumentScene would be called on the main thread, which again reads the file text.

Additional resources

The following are just some assumptions, but might provide background for further analysis.

• The initial idea of loading adjacent pages in the background is perfect and should improve user experience when switching pages.
• It may be that a first attempt was to load the UBGraphicsScene in the worker thread. But if we do that, then the scene is owned by the worker thread and cannot easily be transferred to the main thread, as it consists of many objects.
• So the second idea was to do just the I/O in the background and to send the data as QByteArray over a connection to the main thread. Here it should be scanned and the scene created from that.
• For some unknown reason however loadDocumentScene is used, which again reads the scene from disk. Why not using
  

  OpenBoard/src/adaptors/UBSvgSubsetAdaptor.cpp

  Lines 341 to 345 in 0e69a0a

  	std::shared_ptr<UBGraphicsScene> UBSvgSubsetAdaptor::loadScene(std::shared_ptr<UBDocumentProxy> proxy, const QByteArray& pArray)
  	{
  	UBSvgSubsetReader reader(proxy, UBTextTools::cleanHtmlCData(QString(pArray)).toUtf8());
  	return reader.loadScene(proxy);
  	}

  
  which is exactly doing this?

Context

• reproducibility : systematic
• OpenBoard version :
  • bug happens with : 1.7.0 1.7.1a-x.y.z
  • didn't check : 1.6.4
• OS :
  • bug happens with : Linux
  • didn't check : Windows macOS
• Installation source : own build
• If you built OpenBoard from source, please provide the branch you're working on : dev

[letsfindaway]

Registering the metatype solves the issue with the signal:

qRegisterMetaType<std::shared_ptr<UBDocumentProxy>>("std::shared_ptr<UBDocumentProxy>");

But I would also like to use the loaded text for creating the scene. Are there any known reasons why this was not done and instead the loaded text explicitly marked as Q_UNUSED?

The line in the code comes from commit 7dea264, where @kaamui restored Watsaig's work on persistence, activating the UBPersistenceWorker.

[letsfindaway]

Scene identification by index is unsafe

Note also that in this asynchronous case the scene index does not uniquely identify the scene. In the (unlikely) case that the user inserted another scene before the worker thread is ready and emitted the signal, the scene indexes have shifted by one and the index delivered in the signal no longer identifies that scene.

We should think about another scene identification which can be created for each index after reading the metadata (and for each newly created scene) and which is unique and does not depend on the index of the scene.

As a scene is a UBItem and the UUID is also stored in the SVG document, would it be better to identify them by their UUID, at least in this case? But we don't know the UUID before we have loaded the scene. One approach could be to "peek" into the document file, reading the first two lines as we know that the UUID is in the second line with the <svg> element.

Note that a scene index is perfect in synchronous use cases, so I don't propose to change that in general. It is just not sufficient in an asynchronous use case.

[kaamui]

• For some unknown reason however loadDocumentScene is used, which again reads the scene from disk. Why not using

I remember that I had to do this change to prevent data loss, but can't remember the use cases.

[letsfindaway]

• For some unknown reason however loadDocumentScene is used, which again reads the scene from disk. Why not using

I remember that I had to do this change to prevent data loss, but can't remember the use cases.

One could argue that after reading the file content once in the worker thread, it is most probably cached, so that subsequent reads are faster. In this case it is not necessary to send the data over the signal. The trigger using the proxy and scene index is sufficient.

Then the above mentioned problem with the scene index in asynchronous scenarios vanishes. The worst thing which can happen is that the wrong page is cached.

The above-mentioned commit where this code was added occurred before we switched to shared_ptr for the proxy. So at this time the signal was delivered. To restore this situation I would propose to

• remove the QByteArray with the scene data from the signal
• register the metatype for the shared_ptr as shown above
• and leave everything else untouched

[letsfindaway]

... or we just remove all that stuff. See #163 (comment). The creation of the thumbnails loads all document scenes anyway. So there is no benefit from trying to load adjacent pages in a worker thread.

If we would like to improve this, then we have to use the static pixmaps of the pages to initially fill the board thumbnails and only replace them with the live view when a scene becomes active.

[letsfindaway]

To decide how to proceed it is first important to ask what the problem actually is. It it just a log message we want to get rid of? Or is it a performance issue in networked environments? If yes, is it actually the problem to load all the SVG files over the network? what about the images, videos and other objects, which are currently not addressed by asynchronous loading? This is currently not clear to me, but the solution depends on the answer to that question. It could be

• Just register the metatype to get rid of the log message. But I think this cannot be the answer, as the asynchronous loading at all is questionable when all scenes are loaded synchronously anyway when building the board thumbnails.
• Asynchronously access and read all files of the document to make sure they are in the OS filesystem cache.
• Modify thumbnail building and use the pixmaps in a first step and replace them by the scene only when a page is active on the board. The pixmaps are probably loaded anyway for the document mode.

My current view on this:

• I assume the slow part is network access, so the second point might be an interesting approach, which does not interfere with any operations on the main thread.
• We could combine that with the original idea of loading adjacent pages and actually do that caching for those scenes only.
• After loading the SVG, we can use the RegEx from the cleanupDocument() to find referenced objects and load only those. This would also address access to the objects, but avoid unnecessary data transfer.
• Loading should have a very generous error handling: If a file cannot be accessed or if there is an error during reading, we just ignore that and skip it.
• It is not necessary to give any feedback about the operation to the main thread. We just fill the filesystem cache in the background on a best effort basis to make life easier for it.
• This could be combined with an optimized board thumbnail building as described above.

To come back to the first sentence of this comment: This all is only valid if the assumption is correct that network data transfer is a bottleneck and affects user experience.

[letsfindaway]

The issue OpenBoard-org#918 opens another perspective to this issue - and increases importance. With this huge document provided by the issue author you can see that the performance problem not only occurs in networked environments, but also with local data. Creating ten thousands of stroke objects requires some time. All the documents together have more than 340000 strokes!

grep "<polygon" *.svg |wc -l
346529

So my conclusion is now:

• We must not load all pages when creating the thumbnails, as proposed here [Bug] Asynchronous loading of adjacent pages fails #171 (comment) and as third alternative in my previous comment.

Modify thumbnail building and use the pixmaps in a first step and replace them by the scene only when a page is active on the board. The pixmaps are probably loaded anyway for the document mode.

• Asynchronous loading of adjacent pages is useful. We should therefore apply the fixes to deliver the signal by registering the metatype. See [Bug] Asynchronous loading of adjacent pages fails #171 (comment). Note however that at least for documents with many strokes the big performance impact is not reading the data from disk, but creating all the items. We currently have no solution for asynchronous loading in this case due to thread ownership of objects.
• Scene identification by index is unsafe. See [Bug] Asynchronous loading of adjacent pages fails #171 (comment). I could imagine that the crash reported in [Bug] Openboard crashes/locks up the desktop when importing a file .ubz OpenBoard-org/OpenBoard#918 could be caused by that. A page was deleted, the indexes shifted by one, but probably some asynchronous operation still uses an index to the now removed last page.

The author of that bug also mentions:

Also it eats up all my memory which is why it locks up.

This moves #135 into my focus. To avoid such situations where OpenBoard uses all available memory we have to improve the retention policy of the scene cache. So additionally of what I wrote above:

• We should check memory usage when adding a scene to the scene cache and remove other scenes when they are not actively used. To be discussed in Retention policy for UBSceneCache #135 how this can be done.
• This involves removing these scenes from the corresponding thumbnail and use the last rendered pixmap again instead.
• It might actually be useful to only use a live thumbnail for the currently active scene.