Perf lazy loading

[letsfindaway]

This PR aims to improve performance of scene handling especially with documents with many pages, where each page has many items. This occurs when pages contain a lot of handwriting. Such scenes could be several megabytes as SVG and also occupy a lot of memory when loaded as scene.

Also loading of such pages takes considerable time. Since all pages of a document are currently loaded when the document is opened in order to assign the scenes to the board thumbnails, opening such heavy documents can currently take many seconds.

Currently the scene cache only grows. Scenes are never deleted from the cache. This can lead to memory exhaustion and crashing of OpenBoard.

The PR implements the following in three commits:

• The first commit addresses the long loading times when opening a document. As we have thumbnails for all pages available in the document folder, we can initially start to use these thumbnails for all pages except the current one. Only the current page thumbnail is associated with the corresponding scene and updated when the scene changes. All other thumbnails are not associated with any scene and just use the last status of the pixmap. This improves the time needed to open a document. Detaching scenes from thumbnails also avoids that releasing a scene is blocked by a pointer from the thumbnail to the scene.
• The second commit enables predictive loading of scenes in the background. Currently there was some (unused) attempt to at least read the files on another thread in the background. We now construct scenes on the main thread, but in small steps, which are executed whenever the main thread is idle. This avoids any complexity with multi-threading. Due to the Qt internal implementation of QGraphicsItem data it is impossible to construct these objects on another thread, so this approach is the only possible solution. We now load the previous, next and next but one scene using this approach, which leads to a smooth page change behavior even on heavy documents. At the same time, we re-enable a cache size limit (default: 20 scenes) to limit memory usage. The already implemented LRU policy in the scene cache is re-enabled.
• The third commit removes the now obsolete code for loading scene data in a worker thread.

For me this PR fixes #918 and #921, but I would like to have more testers.

[letsfindaway]

Changes in the user experience and implementation

This PR introduces some small changes in the user experience:

• Previously, the aspect ratio (AR) of all board thumbnails was always 4:3. As the board thumbnails are now initialized from the document page thumbnails, they inherit their AR. So for a page imported from an A4 PDF, the thumbnail has that AR.
• The scaling of the thumbnail content is now adjusted according to the content of the page. It always includes the area of all items on the page. Nevertheless, the AR always stays the same. This is somewhat similar to what we're doing for PDF export.
• When opening a document, the first document page is now much faster visible on the board, as we're only loading the current scene. All other thumbnails only require to load the document thumbnail image files.
• Due to the background preloading of the previous, next and next but one page flipping through pages is typically as fast as before. Only if you jump to a completely different page you might experience a small delay when loading that scene.

And more technically:

• Memory consumption on large heavy documents is greatly reduced, as we now cache only a limited number of scenes.
• Reading the scene SVG file is now done on the main thread and immediately when the scene loading was requested. At the same time a cache entry is created for that scene. In contrast to loading on a different thread, this avoids any problems which could occur when the user adds, deletes or moves a page before the other thread has completed reading the file.
• Construction of a scene from the SVG file content is now done in a large number of very small steps. Previously the data was loaded (well, in fact should have been loaded) in another thread, but construction of the scene - which is typically far more time consuming - was done in one step on the man thread, which blocked the UI for that time.
• The scene cache now not only stores loaded scenes, but also scenes which are in the process of loading. When such a scene is requested from the cache, the loading can be finished on the main thread. Previously, such a scene would have been loaded a second time from the beginning, as an incomplete scene was not part of the cache.
• Deletion of a scene is now much faster for heavy documents. Previously each item delegate had a connection to the UBBoardController::zoomChanged() signal. For heavy documents there could be ten thousands of such connections. Deleting a scene means deleting all connections of the item delegates of that scene. And for each connection Qt has to go through that large list of connections to find the right one to remove. This leads to the strange behavior that deleting a scene could even take much longer than creating it. We now send this signal first to the scene itself, which forwards it to its items. Now all these connections can be deleted in one single step, which is way faster.
• Note that all of these points except for the first one where not obvious before, as all scenes have been loaded in the beginning and a scene was never deleted. They all became obvious only while I was trying to improve the initial loading time and memory consumption.

[kaamui]

I had some feedback today about users experiencing freezes on Debian. It's not sure it is related to OpenBoard, but even if I think we should improve cache system to take memory usage into account instead of an arbitrary number of pages, having it reintroduced with all the improvements you made on this PR could probably help a lot mitigating this possibility.

So I'm going to merge it, and I'll come back to you after our internal meetings so we can go further on the different aspects raised by this PR, as discussed by mail.

Edit:

Currently the scene cache only grows. Scenes are never deleted from the cache. This can lead to memory exhaustion and crashing of OpenBoard.

Do you think that reaching memory exhaustion could lead to a freeze of the OS instead of a crash of OpenBoard ?

[letsfindaway]

Do you think that reaching memory exhaustion could lead to a freeze of the OS instead of a crash of OpenBoard ?

Yes, if you have a swap on a lame disk. This can really feel like freezing.

[kaamui]

Testing a bit the patch, I see the active thumbnail is not always updating. Do you experience this too ?

Example of reproduction scenario :

1. launch OpenBoard
2. draw (everything is OK)
3. add Page
4. draw (not updating)

[letsfindaway]

Testing a bit the patch, I see the active thumbnail is not always updating. Do you experience this too ?

Example of reproduction scenario :

1. launch OpenBoard
2. draw (everything is OK)
3. add Page
4. draw (not updating)

Yes, I can reproduce that. It is ok again after switching to page 1 and then page 2. Will have a look.

[letsfindaway]

Yes, I can reproduce that. It is ok again after switching to page 1 and then page 2. Will have a look.

fixed

[kaamui]

• This leads to the strange behavior that deleting a scene could even take much longer than creating it. We now send this signal first to the scene itself, which forwards it to its items. Now all these connections can be deleted in one single step, which is way faster.

This on python.ubz goes crazy ^^, and it can really gives the impression the OS is completely frozen (1 CPU goes 100% for minutes)

[kaamui]

Hi @letsfindaway,

I'm going to change the cacheNeighboringScenes logic so it caches as much pages as authorized by the pageCacheSize setting, like following :

int lowerLimitNeighbor = std::max(0, sceneIndex - (UBSettings::settings()->pageCacheSize->get().toInt()/2));
for (int i = lowerLimitNeighbor; i< std::max(UBSettings::settings()->pageCacheSize->get().toInt() - lowerLimitNeighbor, proxy->pageCount()-1); i++)
{
    mSceneCache.prepareLoading(proxy,i);
}

Did you have some reasons not to do so ? Do you have any suggestions about this change ?

Also, I found, holding arrow keys to go to others pages where a video item can be found in some of them, that the application crashes (systematically) when the cache is full and that a page containing a media has to be removed:

cache full, size 20
removing page 27 of "OpenBoard Document 2024-09-27 10-51-26.623"
ASSERT failure in UBGraphicsVideoItem: "Called object is not of the correct type (class destructor may have already run)",

Could you look at this ?

[letsfindaway]

Did you have some reasons not to do so ? Do you have any suggestions about this change ?

No specific reason. One argument could however be that currently the prepareLoading does not maintain a queue, but starts loading all scenes in parallel. This causes that it takes longer until the first page was loaded. So it might be counterproductive to load all pages immediately.

Also, I found, holding arrow keys to go to others pages where a video item can be found in some of them, that the application crashes (systematically) when the cache is full and that a page containing a media has to be removed:

Cannot reproduce this with a document where I have put an audio and a video on some of the pages. In the log I can see that these pages are also deleted from the cache. Can you provide me with a document where this happens?

But during testing I found that dragging the object to the thumbnail instead to the board causes an immediate crash.

[letsfindaway]

For the crash when dragging an object to the thumbnail view:

OpenBoard/src/gui/UBBoardThumbnailsView.cpp

Lines 327 to 340 in fe2bbd3

	void UBBoardThumbnailsView::dragEnterEvent(QDragEnterEvent *event)
	{
	mDropBar->show();
	if (event->source() == this)
	{
	event->setDropAction(Qt::MoveAction);
	event->accept();
	}
	else
	{
	event->acceptProposedAction();
	}
	}

Change the else in line 336 to else if (mDropSource) to avoid that DnD is executed without a valid drop source.

However I wonder why this else exists: can you drag any object from outside the thumbnails view to that view? I thought that the event source must always be the same widget as the drop target.

[letsfindaway]

Can you provide me with a document where this happens?

Thanks for the documents. I tried it with all three of them. Still cannot reproduce the problem with the current dev branch. That's strange.

What I'm doing:

• Load the document from the Documents mode.
• Press and hold "arrow right" to flip through all pages.
• Same in the other direction, even multiple times.
• I also started the video on one of the pages and then moved away from that page. Also no crash.

[kaamui]

Can you provide me with a document where this happens?

Done.

No specific reason. One argument could however be that currently the prepareLoading does not maintain a queue, but starts loading all scenes in parallel. This causes that it takes longer until the first page was loaded. So it might be counterproductive to load all pages immediately.

Oh I indeed thought loading requests were queued and performed during idle time one at a time. So I thought it would not directly impact the user, just potentially indirectly because of the memory and cpu usage.

Do you think anything is possible with slight changes or will it take more important changes than what I'm doing ?

[kaamui]

Can you provide me with a document where this happens?

Thanks for the documents. I tried it with all three of them. Still cannot reproduce the problem with the current dev branch. That's strange.

What I'm doing:

• Load the document from the Documents mode.
• Press and hold "arrow right" to flip through all pages.
• Same in the other direction, even multiple times.
• I also started the video on one of the pages and then moved away from that page. Also no crash.

Ok so it's probably Qt related. Happening with the Qt 6.7.2 version

[kaamui]

Also happening with Qt 6.4.2 on a Debian virtual machine. Did you try with a Qt 5 based openboard ?

[letsfindaway]

Ok, confirmed and it is an issue with Qt 6. The reason is here:

OpenBoard/src/domain/UBGraphicsMediaItem.cpp

Lines 170 to 201 in fe2bbd3

	#if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0))
	connect(mMediaObject, &QMediaPlayer::hasVideoChanged,
	this, &UBGraphicsVideoItem::hasVideoChanged);
	connect(mMediaObject, &QMediaPlayer::playbackStateChanged,
	this, &UBGraphicsVideoItem::mediaStateChanged);
	connect(mMediaObject, &QMediaPlayer::errorOccurred,
	this, &UBGraphicsVideoItem::mediaError);
	#else
	connect(mMediaObject, SIGNAL(videoAvailableChanged(bool)),
	this, SLOT(hasVideoChanged(bool)));
	connect(mMediaObject, SIGNAL(stateChanged(QMediaPlayer::State)),
	this, SLOT(mediaStateChanged(QMediaPlayer::State)));
	connect(mMediaObject, qOverload<QMediaPlayer::Error>(&QMediaPlayer::error),
	this, &UBGraphicsVideoItem::mediaError);
	#endif
	setAcceptHoverEvents(true);
	update();
	}
	UBGraphicsMediaItem::~UBGraphicsMediaItem()
	{
	if (mMediaObject) {
	mMediaObject->stop();
	delete mMediaObject;
	}
	}

The connections are different between Qt5 and Qt6. When deleting a media object, the destructor calls mMediaObject->stop(). This triggers the playbackStateChanged signal which is - when using Qt 6 - connected to UBGraphicsVideoItem::mediaStateChanged. But at this stage the destructor of UBGraphicsVideoItem has already run, so the object is no longer an instance of that class.

The connection is still alive, even if it was created in a subclass. This is because connections are released in the QObject destructor, which runs later. In fact this might also be a problem for Qt5, but it seems not to be fatal as in Qt6.

I will work on a solution later this weekend, most probably by disconnecting these signals in the destructor.

[letsfindaway]

See letsfindaway#183 for some thoughts on how to improve scene caching and background loading in the next iteration. I would be happy to get your feedback and to collect more ideas there.