GRAPHICS: Speed up and refactor SVG handling #4773

mikrosk · 2023-03-05T19:00:12Z

Maybe you are aware of other places in code like this?

(I found this one the hard way)

lephilousophe

LGTM.
I am not sure other cases would be easy to find.
The policy has always been to use Surface::free after Surface::create and not to use Surface::free for other cases.

sev- · 2023-03-05T21:56:53Z

I am confused by this change. Yes, tmp.free() is redundant. But what are those _cache-related changes about?

mikrosk · 2023-03-06T06:37:13Z

@sev- tmp is initialised with _cache (which is allocated a few lines above): https://github.com/scummvm/scummvm/pull/4773/files#diff-42f6acbe42c71148cf49bffeec7be5d8572dd200e36f539149cf4a5398c60ccbR100

sev- · 2023-03-06T16:27:46Z

Well, I understand that, but shouldn't it be split into two commits, then?

mikrosk · 2023-03-06T16:58:58Z

Now I am confused. :) tmp.free() isn't redundant, Surface's d-tor doesn't call ::free() automatically (it has a default d-tor). Perhaps you are confusing it with a ManagedSurface ?

So if I'm guessing right, tmp.free() was supposed to release the initialised _cache buffer, therefore this commit does that, the right way

mikrosk · 2023-03-06T17:03:14Z

On a second look... perhaps there is something to it. That _cache isn't just a temporary buffer, it is supposed to hold the rasterized image of size dw * dh and if the size is the same, there is no reason to dealloc and alloc again and again.

Good catch!

mikrosk · 2023-03-06T17:09:16Z

@sev- it seems there even was a memory leak, please take a look at the latest version whether the fix is good to go now.

lephilousophe · 2023-03-06T18:40:15Z

I would like to take time to look at it on a proper screen.
Thanks

lephilousophe · 2023-03-07T17:17:37Z

In fact we are leaking memory and I don't see why we need _cache, _cachedX, _cachedY at all.
I have a commit on lephilousophe/scummvm@ec2f1a9 which seems to work correctly and avoids the intermediate blitting.

mikrosk · 2023-03-08T07:45:25Z

@lephilousophe isn't possible that the intermediate surface has been used for implicit conversion from, say, 16bpp to 32bpp (which the nvg rasterizer expects) ?

lephilousophe · 2023-03-08T08:24:26Z

I thought that at first but both surfaces (tmp which uses _cache and _render) are created with _pixelformat which is always 32bpp as set in constructor and is never changed (no setter).
But I ask for confirmation because I don't know well this part of the code, maybe I missed something.

mikrosk · 2023-03-11T23:53:37Z

Encouraged by @lephilousophe's analysis I have taken a further step, removed all the shenanigans with allocating and blitting, code is much faster and cleaner now.

As a bonus, I have also reverted one commit which doesn't seem to be necessary, so we are caching bitmaps during initial startup again.

All together this brought a speedup of 8 seconds on an intentionally weak hardware so I think it's worth applying. Please take a look whether I didn't miss something obvious.

lephilousophe

So, all in all, before this change we had a SVGBitmap class which would render on any surface size with a cache for the last rendered resolution.
This implied that we keep a decoder around and a rasterized cache.
This code was used in 2 places: theme engine and grid widget where the SVGBitmap was rendered once and destroyed. A texture of the same pixel format was used to store the data.

With new code, the SVG is not a renderer anymore but directly a surface with a size set at creation time and a fixed pixel format.
This is OK for current uses but may reduce hypothetical future use cases.
The SVGSurface now consists only in a ManagedSurface so the memory footprint is reduced.
There are now less blitting too.

Except from my inline comments, code looks good to me and the features removal seem acceptable to me.

lephilousophe · 2023-03-12T10:04:16Z

graphics/svg_surface.cpp

 #ifdef SCUMM_BIG_ENDIAN
-	_pixelformat = new Graphics::PixelFormat(4, 8, 8, 8, 8, 24, 16, 8, 0);
+const Graphics::PixelFormat PIXELFORMAT = Graphics::PixelFormat(4, 8, 8, 8, 8, 24, 16, 8, 0);


This requires a global constructor: this is not allowed as per our coding conventions.
Please move it back in the constructor.

Right. I've changed it in a slightly different way, using a #define as it serves basically the same purpose.

Otherwise I can inline the #ifdef in the constructor but it wont look as good.

lephilousophe · 2023-03-12T10:04:16Z

gui/ThemeEngine.cpp

-		surf = nullptr;
-
-		_bitmaps.erase(filename);
+		return true;


I am really not sure about this one: @sev- disabled cache because the it wasn't cleared when _scaleFactor changed.
I don't see anything new in code which would clear the cache in this case.
This must be added in setBaseResolution.
If _scaleFactor is different from s: kill the cache like in refresh()

Yes, I have been worrying about the same thing. However I have also been unable to find a use case where this causes trouble (I was switching themes via the GUI, maybe there is some other way?). @sev- if you can remember a scenario which was broken without this change, that would help.

You should switch the scale factor, not the theme.

That, too - at least I couldn't find anything else than the large/medium/small scale options. And I don't see any difference in behavior.

OK. I see the why of the commit.
It was done in April 2021.
At that time (before ad31dfc in August 2021), the _scaleFactor was depending on the height of the window in gui/gui-manager.cpp.

A resize of the window implied a _scaleFactor change which would have resulted in garbled images.
Changing the scale factor in GUI forces a ThemeEngine destroy and recreate, that's why we can't replicate the issue.

I still think we should take the safe side and clean the cache in setBaseResolution: changing the _scaleFactor has to invalidate the cache even if today we don't see the bug.

As I'm working on some backend code now I can't fully focus on the changes you've mentioned; would it be OK if I push the other two commits to master (which I need for my backend changes) and and let the third one present in this PR to keep the existing discussion visible?

sev-

I see no reasoning provided, nor what exactly does this speed up. Please, explain.

This reverts commit mikrosk@04f040a which claims to fix a theme reload bug but
I wasn't able to reproduce this behavior at all (with changed scale
and/or renderer ThemeEngine is reset and its _bitmap cache doesn't
contain any leftovers).

I do not understand why you express this discontent edging aggression in this commit log message. I read it as: "I was not able to reproduce it, the commit log is lying and the commit is useless. Reverting it"

sev- · 2023-03-16T13:59:36Z

graphics/module.mk

@@ -48,7 +48,7 @@ MODULE_OBJS := \
 	scaler/normal.o \
 	sjis.o \
 	surface.o \
-	svg.o \
+	svg_surface.o \


What is the reason behind renaming this?

Just to follow the convention, so SVGSurface is located in svg_surface.

sev- · 2023-03-16T13:59:36Z

graphics/svg_surface.cpp

+#define PIXELFORMAT Graphics::PixelFormat(4, 8, 8, 8, 8, 0, 8, 16, 24)
+#endif
+
+Graphics::SVGSurface::SVGSurface(Common::SeekableReadStream *in, int dw, int dh)


What is the reason behind this rename? We convert SVG into a bitmap. All of our graphics formats return Surface

Well, it's inherited from ManagedSurface now, would you prefer to keep its old name despite this change?

Yes, I prefer to keep the old name for consistency.

Why is it now inherited from ManagedSurface and not a plain Surface?

Both ThemeEngine's _bitmaps[filename] and grid's loadSurfaceFromFile work with ManagedSurface so it was a nice optimization -- we can directly store/return newly created instances.

sev- · 2023-03-16T13:59:36Z

graphics/svg_surface.h

+ * A derived graphics surface, which renders bitmap data from a SVG stream.
+ */
+class SVGSurface : public ManagedSurface
+{


This does not conform to our code formatting guidelines

sev- · 2023-03-16T13:59:36Z

graphics/svg_surface.cpp

+	in->read(data, size);
+	data[size] = '\0';
+
+	NSVGimage *svg = nsvgParse(data, "px", 96);


Where does the cache go? Why? It is used extensively by our GUI code, and re-rendering it every time is not optimal, particularly in the icons list.

Rendering SVG is very expensive because it uses double-precision FPU arithmetics.

Indeed. However from what I could see from the profiling, this bitmap is not reused. So while it looks good on the first sight, in reality this cache didn't provide any speedup, on the contrary.

As written in the other comment, I physically measured the speed this brought and it was enormous.

I guess your discomfort with this change is related to the debate I had with @lephilousophe earlier -- whether it's acceptable to narrow SVGBitmap's features to its current usage (load svg file, render, copy and trash) or to keep it as generic as possible.

As the former approach brought significant speedup and simplified other parts of ScummVM code, too it looked like a good idea to go ahead.

mikrosk · 2023-03-16T14:07:17Z

I do not understand why you express this discontent edging aggression in this commit log message. I read it as: "I was not able to reproduce it, the commit log is lying and the commit is useless. Reverting it"

That's definitely not the way I intended to be understood. It's only what I wrote there - this was fixing something but that something seems to be gone, bitmap loading is working (and cached) without this commit, period. As you can see, I tried to verify it with others to be sure that this claim is correct.

No need to read between the lines. :)

This cache buffer has no reason to be here and it is leaked when the class is destroyed.

mikrosk · 2023-03-16T16:18:14Z

After talking to @sev-:

rebased
agreed that getting rid of _cache is good so I kept @lephilousophe's change
agreed to keep the original name SVGBitmap for easier review
rephrased the offending commit's language to more neutral tone + added @lephilousophe's observation

Since the only use case for SVGBitmap is blitting it to a ManagedSurface, combine both into one to avoid unnecessary allocation and blitting.

This reverts commit 04f040a which forced a bitmap reloading to prevent reusing already up/downscaled images in case that _scaleFactor has changed. However after commit ad31dfc this no longer applies as changing the scale factor in GUI forces a ThemeEngine destroy and recreate. So _bitmaps[filename] is safe to keep its cached image which is reused e.g. during initial theme loading.

sev- · 2023-03-16T18:20:46Z

Thanks!

mikrosk force-pushed the surface_free_fix branch from 02abb8f to d99a366 Compare March 5, 2023 19:00

mikrosk changed the title ~~GRAPHICS: Don't assume Surface::free == ::free()~~ GRAPHICS: Don't assume Surface::free() == ::free() Mar 5, 2023

mikrosk mentioned this pull request Mar 5, 2023

GRAPHICS: ATARI: Align surface on a 16-byte boundary #4771

Merged

lephilousophe approved these changes Mar 5, 2023

View reviewed changes

lephilousophe self-requested a review March 6, 2023 18:39

mikrosk force-pushed the surface_free_fix branch from 4abe3ec to d18ad30 Compare March 11, 2023 23:49

mikrosk changed the title ~~GRAPHICS: Don't assume Surface::free() == ::free()~~ GRAPHICS: Speed up and refactor SVG handling Mar 11, 2023

lephilousophe reviewed Mar 12, 2023

View reviewed changes

mikrosk force-pushed the surface_free_fix branch from d18ad30 to 6f87f19 Compare March 12, 2023 13:13

sev- reviewed Mar 16, 2023

View reviewed changes

GRAPHICS: Don't allocate a temporary buffer for SVG rendering

0355c2c

This cache buffer has no reason to be here and it is leaked when the class is destroyed.

mikrosk force-pushed the surface_free_fix branch from 6f87f19 to 40337da Compare March 16, 2023 16:14

mikrosk added 2 commits March 16, 2023 18:36

GRAPHICS: SVGBitmap now inherits from ManagedSurface

4d132af

Since the only use case for SVGBitmap is blitting it to a ManagedSurface, combine both into one to avoid unnecessary allocation and blitting.

mikrosk force-pushed the surface_free_fix branch from 40337da to 48f4f26 Compare March 16, 2023 17:38

sev- merged commit e00c558 into scummvm:master Mar 16, 2023
8 checks passed

mikrosk deleted the surface_free_fix branch March 16, 2023 18:24

GRAPHICS: Speed up and refactor SVG handling #4773

GRAPHICS: Speed up and refactor SVG handling #4773

mikrosk commented Mar 5, 2023

lephilousophe left a comment

sev- commented Mar 5, 2023

mikrosk commented Mar 6, 2023 •

edited

sev- commented Mar 6, 2023 •

edited

mikrosk commented Mar 6, 2023

mikrosk commented Mar 6, 2023

mikrosk commented Mar 6, 2023

lephilousophe commented Mar 6, 2023

lephilousophe commented Mar 7, 2023

mikrosk commented Mar 8, 2023

lephilousophe commented Mar 8, 2023

mikrosk commented Mar 11, 2023

lephilousophe left a comment

lephilousophe Mar 12, 2023

mikrosk Mar 12, 2023

lephilousophe Mar 12, 2023

mikrosk Mar 12, 2023

lephilousophe Mar 12, 2023

mikrosk Mar 12, 2023

lephilousophe Mar 12, 2023

mikrosk Mar 16, 2023 •

edited

sev- left a comment •

edited

sev- Mar 16, 2023

mikrosk Mar 16, 2023

sev- Mar 16, 2023

mikrosk Mar 16, 2023

sev- Mar 16, 2023

mikrosk Mar 16, 2023

sev- Mar 16, 2023

sev- Mar 16, 2023

mikrosk Mar 16, 2023

mikrosk Mar 16, 2023

mikrosk commented Mar 16, 2023

mikrosk commented Mar 16, 2023

sev- commented Mar 16, 2023

GRAPHICS: Speed up and refactor SVG handling #4773

GRAPHICS: Speed up and refactor SVG handling #4773

Conversation

mikrosk commented Mar 5, 2023

lephilousophe left a comment

Choose a reason for hiding this comment

sev- commented Mar 5, 2023

mikrosk commented Mar 6, 2023 • edited

sev- commented Mar 6, 2023 • edited

mikrosk commented Mar 6, 2023

mikrosk commented Mar 6, 2023

mikrosk commented Mar 6, 2023

lephilousophe commented Mar 6, 2023

lephilousophe commented Mar 7, 2023

mikrosk commented Mar 8, 2023

lephilousophe commented Mar 8, 2023

mikrosk commented Mar 11, 2023

lephilousophe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikrosk Mar 16, 2023 • edited

Choose a reason for hiding this comment

sev- left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikrosk commented Mar 16, 2023

mikrosk commented Mar 16, 2023

sev- commented Mar 16, 2023

mikrosk commented Mar 6, 2023 •

edited

sev- commented Mar 6, 2023 •

edited

mikrosk Mar 16, 2023 •

edited

sev- left a comment •

edited