django-cms / django-filer

File and Image Management Application for django
https://django-filer.readthedocs.io/
Other
1.75k stars 579 forks source link

Uploading bigger SVG files blocks the UI #1489

Closed benzkji closed 3 weeks ago

benzkji commented 1 month ago

Users think the system is broken. I've had this issue several times, with architecture projects.

I wonder if the old "create 4 thumbnails for every image right during/after upload" system is still in place? This could be replaced by just creating ONE thumbnail, during rendering of the change list? Thus taking of quite a bit of load, during upload, and making everything a bit more reactive?

fsbraun commented 3 weeks ago

@jrief Do I remember correctly that currently no thumbnails are generated for SVG files?

benzkji commented 3 weeks ago

there is https://github.com/django-cms/django-filer/blob/master/filer/settings.py#L41

I can confirm that I have these 4 sizes on my system. Further, I get a 40x40px version in the changelist, that is generated as well, somewhen.

Without SVG support (ie reportlab), SVGs are File objects and don't have a thumbnail. But that's not an option in my current project.

benzkji commented 3 weeks ago

...so, one could just set FILER_ADMIN_ICON_SIZES, to contain only one size. I'll check this, it could reduce processing time a lot. It might still take twice the time as needed (40x40, plus FILER_ADMIN_ICON_SIZES size - one would be enough). If it's already sketchy, twice/half the time is worth a lot ;)

fsbraun commented 3 weeks ago

I'll try to look into this, too. Later though...

fsbraun commented 3 weeks ago

Also, I wonder if "do not create thumbnails upon upload but only upon request" may be the better strategy if and only if there is a separate request for each thumbnail needed:

fsbraun commented 3 weeks ago

OK, what I found so far:

benzkji commented 3 weeks ago

filer creates “thumbnails” for SVG files which are identical to the original and only differ in the size attributes (width and height attribute)

that is probably true. the issue is the generation of those, as it takes a long time to read xml/svg of large files, with thousands of path elements

I'll have to check, but I doubt it is useful to create those files. The size can probably be overwritten by an tag.

I guess not, yes. I could live with a version that creates ONE thumbnail. As all thumbnails in filer are quadratic, it would make sense (that is what easy-thumbnail does: modify "viewBox" of the SVG).

@benzkji likely needs thumbnails as rendered PNG/WEBP (bitmap with transparency) (if the bitmap image size is smaller than the original SVG) to avoid large data transfers and browser rendering times

It's hard to decide, I'm not really sure. Without a crop, we could always use the original SVG. As we did before. But since SVGs are images in filer, it has become different ;) Sure, a few 100'000 path SVGs in the changelist does not make it rendered faster...

I have not confirmed, but I think the thumbnails created automatically are not those actually needed by the current admin.

That's what I was thinking as well...

fsbraun commented 3 weeks ago

I can imagine not using thumbnails for SVG in the admin, but doing some CSS magic (object-fit etc.) instead. This way, no thumbnails would need to be created for SVG files unless you use those thumbnails elsewhere.

If you then also had an option to not create thumbnails upon upload - especially if they are not used - (the last point of my previous comment, sort of), you might be fine.

Finally, one could set a max image size for svg thumbnails and just show an icon for very large svg files. I assume a few 100k paths don't render so clearly on 40x40 or 150x150?

benzkji commented 3 weeks ago

I would be fine with any of these solutions, but would prefer your first, most eleganct one, IMHO. Max image size could be difficult, there are SVGs with binary data (raster images) withing with only a few paths, that are 2mb, etc etc.

By the way, you could also drop the quadratic thumbnails, and use object-fit: contain, to still have a quadrat. I've had many people complain about those - you are not able to distinguish similar image when the change is cropped away ;). But that's another issue.

fsbraun commented 3 weeks ago

@benzkji Can you confirm my observations on thumbnail generation upon upload within the bundled filer admin?

If this were true, then the 180x180 thumbnail can be fully disabled. The widget might request its preview, so the thumbnail is only created on demand. Maybe it could also be 160x160?

It might need to be decided that the ('16', '32', '48', '64') setting can be deprecated.

I can start working on an admin view that does not generate thumbnails for SVG at all.

benzkji commented 3 weeks ago

Will do so, thanks! @fsbraun

fsbraun commented 3 weeks ago

Also, you can try

pip install git+https://github.com/fsbraun/django-filer@fix/no-admin-svg-thumbnails

This should streamline thumbnails in the admin, and avoid them altogether for SVG files. (Existing thumbnails do not disappear!)

benzkji commented 3 weeks ago

Hi @fsbraun . Thanks. I notice that the change list becomes very very unresponsive, if many big SVGs are listed, this seems unusable, even when not using thumbnails at all (100'000 paths problem). My browser tab required 3gb of RAM!

this is the conclusion, for me. I am not sure if PNGs can be generated from SVGs, though?!

benzkji commented 3 weeks ago

I can confirm that no 8x8/16x16/etc thumbnails are generated, when using the default filer uploading mechanisms.

benzkji commented 3 weeks ago

THUMBNAIL_EXTENSION = "webp" THUMBNAIL_PRESERVE_EXTENSIONS = ("gif",)

SVGs are still thumbnailed as SVGs. So I guess this would need to be adressed in easy-thumbnails?

benzkji commented 3 weeks ago

I really like the ability to have SVGs in FilerImageFields. But if I knew what it means in a real world, I'm not so sure anymore ;)

benzkji commented 3 weeks ago

I'll probably just deny SVGs bigger than 200kb or so. That's what it was meant for...fair enough.

The PNG/WEBP thumbnail solution would be nice, but yea, honestly this is a nice to have (and an easy-thumbnails thing).

@fsbraun thanks for all the efforts and investigations. If we get a change list without thumbnailed SVGs, nice, but for me the problem ist "SVG" in general ;). At least we have another setting to deprecate :)

fsbraun commented 3 weeks ago

Just to make sure I understand: The changelist remains "very, very unresponsive". It was not more responsive with thumbnailed SVG?

I conclude that we should look into thumbnailing SVG to webp or something (no idea, if that is possible).

benzkji commented 3 weeks ago

Yes, it remains unresponsive in both cases (FF and Chrome on Linux). Thumbnailing only changes the viewBox attribute in the svg, the whole XML needs to be parsed, always.

fsbraun commented 3 weeks ago

OK, for now, we save the storage space and the need for the backend to parse the SVG. Maybe have a fallback icon for large SVG?

benzkji commented 3 weeks ago

Fallback: Yes, until we can thumbnail to PNG/WEBP?

fsbraun commented 3 weeks ago

OK, I implemented the fallback: static/filer/icons/file-picture.svg is shown if the SVG is larger than 1MB (configurable)

benzkji commented 3 weeks ago

Hero, thanks alot!