Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GIF Thumbnail creation fails, breaking the image picker window #5029

Closed
bytebone opened this issue May 23, 2024 · 12 comments
Closed

GIF Thumbnail creation fails, breaking the image picker window #5029

bytebone opened this issue May 23, 2024 · 12 comments

Comments

@bytebone
Copy link

bytebone commented May 23, 2024

Describe the Bug

When uploading a gif, Bookstack often fails to extract/generate a thumbnail image. This is not communicated in the GUI or the realtime application logs. But it is visible on the filesystem, where the original file is saved, but no thumbnail gets created in the respective subfolder, and an error gets logged in the laravel.log file (see below).
Once this has happened, the image picker no longer loads any images and gets stuck on an infinite loading animation. In the dev panel, after about a second of the image picker being open, the gallery endpoint returns a 502 error code.

To resolve the issue, the original gif file needs to be deleted from the uploads folder. The image picker will load again and display a broken image item, which can then be deleted from the picker/database.

Steps to Reproduce

  1. Go to any page and start editing
  2. open the image picker
  3. drag a gif into the picker for uploading
  4. The upload finishes but does not let you place the image in the page
  5. Close the image picker, then open it again
  6. The images no longer load

Expected Behaviour

Should be obvious

Screenshots or Additional Context

I am using the Linuxserver Docker image to run Bookstack, but by the nature of the error assume that its not an issue with their Docker wrapper, but with the core app.

Error Log:

Details

[2024-05-22 15:08:18] production.ERROR: BookStack\Uploads\ImageRepo::getEntityFiltered(): Argument #5 ($uploadedTo) must be of type int, null given, called in /app/www/app/Uploads/Controllers/GalleryIma
geController.php on line 31 {"userId":1,"exception":"[object] (TypeError(code: 0): BookStack\\Uploads\\ImageRepo::getEntityFiltered(): Argument #5 ($uploadedTo) must be of type int, null given, called i
n /app/www/app/Uploads/Controllers/GalleryImageController.php on line 31 at /app/www/app/Uploads/ImageRepo.php:79)
[stacktrace]
#0 /app/www/app/Uploads/Controllers/GalleryImageController.php(31): BookStack\\Uploads\\ImageRepo->getEntityFiltered()
#1 /app/www/vendor/laravel/framework/src/Illuminate/Routing/Controller.php(54): BookStack\\Uploads\\Controllers\\GalleryImageController->list()
#2 /app/www/vendor/laravel/framework/src/Illuminate/Routing/ControllerDispatcher.php(43): Illuminate\\Routing\\Controller->callAction()
#3 /app/www/vendor/laravel/framework/src/Illuminate/Routing/Route.php(259): Illuminate\\Routing\\ControllerDispatcher->dispatch()
#4 /app/www/vendor/laravel/framework/src/Illuminate/Routing/Route.php(205): Illuminate\\Routing\\Route->runController()
#5 /app/www/vendor/laravel/framework/src/Illuminate/Routing/Router.php(806): Illuminate\\Routing\\Route->run()
#6 /app/www/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(144): Illuminate\\Routing\\Router->Illuminate\\Routing\\{closure}()
#7 /app/www/app/Http/Middleware/Authenticate.php(23): Illuminate\\Pipeline\\Pipeline->Illuminate\\Pipeline\\{closure}()
#8 /app/www/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(183): BookStack\\Http\\Middleware\\Authenticate->handle()
#9 /app/www/app/Http/Middleware/Localization.php(32): Illuminate\\Pipeline\\Pipeline->Illuminate\\Pipeline\\{closure}()
#10 /app/www/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(183): BookStack\\Http\\Middleware\\Localization->handle()
#11 /app/www/app/Http/Middleware/RunThemeActions.php(26): Illuminate\\Pipeline\\Pipeline->Illuminate\\Pipeline\\{closure}()
#12 /app/www/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(183): BookStack\\Http\\Middleware\\RunThemeActions->handle()
#13 /app/www/app/Http/Middleware/CheckEmailConfirmed.php(47): Illuminate\\Pipeline\\Pipeline->Illuminate\\Pipeline\\{closure}()
#14 /app/www/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(183): BookStack\\Http\\Middleware\\CheckEmailConfirmed->handle()
#15 /app/www/vendor/laravel/framework/src/Illuminate/Foundation/Http/Middleware/VerifyCsrfToken.php(78): Illuminate\\Pipeline\\Pipeline->Illuminate\\Pipeline\\{closure}()
#16 /app/www/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(183): Illuminate\\Foundation\\Http\\Middleware\\VerifyCsrfToken->handle()
#17 /app/www/vendor/laravel/framework/src/Illuminate/View/Middleware/ShareErrorsFromSession.php(49): Illuminate\\Pipeline\\Pipeline->Illuminate\\Pipeline\\{closure}()
#18 /app/www/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(183): Illuminate\\View\\Middleware\\ShareErrorsFromSession->handle()
#19 /app/www/vendor/laravel/framework/src/Illuminate/Session/Middleware/StartSession.php(121): Illuminate\\Pipeline\\Pipeline->Illuminate\\Pipeline\\{closure}()
#20 /app/www/vendor/laravel/framework/src/Illuminate/Session/Middleware/StartSession.php(64): Illuminate\\Session\\Middleware\\StartSession->handleStatefulRequest()
#21 /app/www/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(183): Illuminate\\Session\\Middleware\\StartSession->handle()
#22 /app/www/vendor/laravel/framework/src/Illuminate/Cookie/Middleware/AddQueuedCookiesToResponse.php(37): Illuminate\\Pipeline\\Pipeline->Illuminate\\Pipeline\\{closure}()
#23 /app/www/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(183): Illuminate\\Cookie\\Middleware\\AddQueuedCookiesToResponse->handle()
#24 /app/www/vendor/laravel/framework/src/Illuminate/Cookie/Middleware/EncryptCookies.php(67): Illuminate\\Pipeline\\Pipeline->Illuminate\\Pipeline\\{closure}()
#25 /app/www/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(183): Illuminate\\Cookie\\Middleware\\EncryptCookies->handle()
#26 /app/www/app/Http/Middleware/ApplyCspRules.php(33): Illuminate\\Pipeline\\Pipeline->Illuminate\\Pipeline\\{closure}()
#27 /app/www/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(183): BookStack\\Http\\Middleware\\ApplyCspRules->handle()
#28 /app/www/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(119): Illuminate\\Pipeline\\Pipeline->Illuminate\\Pipeline\\{closure}()
#29 /app/www/vendor/laravel/framework/src/Illuminate/Routing/Router.php(805): Illuminate\\Pipeline\\Pipeline->then()
#30 /app/www/vendor/laravel/framework/src/Illuminate/Routing/Router.php(784): Illuminate\\Routing\\Router->runRouteWithinStack()
#31 /app/www/vendor/laravel/framework/src/Illuminate/Routing/Router.php(748): Illuminate\\Routing\\Router->runRoute()
#32 /app/www/vendor/laravel/framework/src/Illuminate/Routing/Router.php(737): Illuminate\\Routing\\Router->dispatchToRoute()
#33 /app/www/vendor/laravel/framework/src/Illuminate/Foundation/Http/Kernel.php(200): Illuminate\\Routing\\Router->dispatch()
#34 /app/www/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(144): Illuminate\\Foundation\\Http\\Kernel->Illuminate\\Foundation\\Http\\{closure}()
#35 /app/www/app/Http/Middleware/PreventResponseCaching.php(21): Illuminate\\Pipeline\\Pipeline->Illuminate\\Pipeline\\{closure}()
#36 /app/www/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(183): BookStack\\Http\\Middleware\\PreventResponseCaching->handle()
#37 /app/www/vendor/laravel/framework/src/Illuminate/Http/Middleware/TrustProxies.php(39): Illuminate\\Pipeline\\Pipeline->Illuminate\\Pipeline\\{closure}()
#38 /app/www/app/Http/Middleware/TrustProxies.php(41): Illuminate\\Http\\Middleware\\TrustProxies->handle()
#39 /app/www/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(183): BookStack\\Http\\Middleware\\TrustProxies->handle()
#40 /app/www/vendor/laravel/framework/src/Illuminate/Foundation/Http/Middleware/TransformsRequest.php(21): Illuminate\\Pipeline\\Pipeline->Illuminate\\Pipeline\\{closure}()
#41 /app/www/vendor/laravel/framework/src/Illuminate/Foundation/Http/Middleware/TrimStrings.php(40): Illuminate\\Foundation\\Http\\Middleware\\TransformsRequest->handle()
#42 /app/www/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(183): Illuminate\\Foundation\\Http\\Middleware\\TrimStrings->handle()
#43 /app/www/vendor/laravel/framework/src/Illuminate/Foundation/Http/Middleware/ValidatePostSize.php(27): Illuminate\\Pipeline\\Pipeline->Illuminate\\Pipeline\\{closure}()
#44 /app/www/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(183): Illuminate\\Foundation\\Http\\Middleware\\ValidatePostSize->handle()
#45 /app/www/vendor/laravel/framework/src/Illuminate/Foundation/Http/Middleware/PreventRequestsDuringMaintenance.php(99): Illuminate\\Pipeline\\Pipeline->Illuminate\\Pipeline\\{closure}()
#46 /app/www/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(183): Illuminate\\Foundation\\Http\\Middleware\\PreventRequestsDuringMaintenance->handle()
#47 /app/www/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(119): Illuminate\\Pipeline\\Pipeline->Illuminate\\Pipeline\\{closure}()
#48 /app/www/vendor/laravel/framework/src/Illuminate/Foundation/Http/Kernel.php(175): Illuminate\\Pipeline\\Pipeline->then()
#49 /app/www/vendor/laravel/framework/src/Illuminate/Foundation/Http/Kernel.php(144): Illuminate\\Foundation\\Http\\Kernel->sendRequestThroughRouter()
#50 /app/www/public/index.php(52): Illuminate\\Foundation\\Http\\Kernel->handle()
#51 {main}
"} 

Browser Details

Brave 1.64.113 / Safari (Version unknown)

Exact BookStack Version

v24.05.1

@kalebalebachew
Copy link

I've submitted a pull request that tries to fix it on #5030

@ssddanbrown
Copy link
Member

Hi @bytebone,

  • Do you see an error logged every time a GIF is uploaded and/or you load the image manager? Just asking as the error log shared appears to be from a while before this was opened, so just want to verify the error is actually related instead of adjacent or unrelated.
  • Do you have any proxies/servers active in front of the container?
  • Are you able to share the specific gif file? Or otherwise describe it's filesize and dimensions?
  • Are you able to replicate with this gif: https://en.wikipedia.org/wiki/GIF#/media/File:Rotating_earth_(large).gif

@bytebone
Copy link
Author

bytebone commented May 23, 2024

Hey Dan,
thanks for the prompt reply.

  • Good that you asked. The log I initially posted does roughly line up with the first time we encountered the GIF issue, though in todays testing I was unable to get it repeat this. I took some more time monitoring all kinds of logs in realtime when attempting to upload gifs. For any of my tests, nothing was logged in the laravel.log file. All I got is an error from the mariadb instance
    [Warning] Aborted connection 492 to db: 'bookstackapp' user: 'bookstack' host: '172.28.0.3' (Got an error reading communication packets)
    and from the nginx error log
    349#349: *1023 recv() failed (104: Connection reset by peer) while reading response header from upstream, client: 172.19.0.4, server: _, request: "POST /images/gallery HTTP/1.1", upstream: "fastcgi://127.0.0.1:9000", host: "wiki.mydomain.com", referrer: "https://wiki.mydomain.com/books/tisax-prep-ca-wip/draft/108"
  • The wiki is being proxied through Cloudflare. The gifs that are being uploaded are usually between 500KB and 2MB in size, so the upload should be going through fine. I've also checked that the byte filesize is equal on my local system and the server after uploading - checks out, even for files at 3,1 MB. So for now I'm ruling out file corruption during transfer.
  • Through these tests, I've found plenty of gifs that work fine, so the issue is not universal to all GIFs. Here is a file that has caused the errors above. I've also found gifs that cause a multitude of memory errors, though I assume that is another issue, so for now I won't go into detail.
  • The Rotating Earth gif you linked worked fine

Heres a quick list of gifs that I tested:

Resolution Filesize Error Link
500x250 200 KB No 26.gif
800x600 149 KB No 25.gif
800x600 97 KB No 17.gif
500x500 872 KB No 2.gif
600x407 597 KB Yes
800x600
(linked above)
938 KB Yes 41.gif
800x600 1433KB Yes 38.gif
800x600 3086 KB Yes 19.gif

I cannot see a specific pattern in these results. I'm curious to hear your opinion. If you have an idea where else I could find more helpful error logs, let me know.

In hindsight, I also want to note that we've upgrade from 23.12.2 straight to the newest 24.5.1. GIF upload has not been an issue on 23.12.2 as far as I can remember. Since I cannot bear the dataloss from a database rollback (lots of edits since the upgrade), I am unable to go back to that working version.

@ssddanbrown
Copy link
Member

Thanks for the info @bytebone,

Looking just at the difference between the working earth GIF and the provided drippy zig-zag gif, although a similar filesize and dimensions, the latter appears more complex. Opening in GIMP (as a rough test) the drippy zig-zag is 215 frames at 200MB of memory overall vs the earth having 44 frames at 8MB of memory.

Therefore I think this may be a memory exhaustion scenario during resize attempt.
Within the php/php-local.ini file that's in the folder you have mounted at /config for the app container, add memory_limit=256M the save, then restart the app container.
See if that makes a difference (Maybe try doubling again to 512M if it does not).

@bytebone
Copy link
Author

Interesting. After setting it to 512M, some of the formerly failing gifs can now be uploaded, but still not all. When trying to upload the 41.gif from above, i see a notable spike in CPU and RAM usage while BookStack attempts to extract a thumbnail, but after ~2.3 seconds I still get the 502 error response with the same behaviour as before. After doubling yet again to 1024M (equal to half of the total RAM on this small VPS) the 41.gif is still too complex for the thumbnail extractor. I can see the RAM filling up roughly by 1 GB - as soon as it does, the BookStack task fails and the issues continue as seen before.

How can the extractor require over a gigabyte of RAM when working with a 900 MB gif?

By doing this testing, I've found that the error flow described before only applies when the memory_limit is filled without hitting the machines RAM limit. With a memory_limit of 1024M, the machines RAM is full before (or at the same time as) the PHP limit, leading to the following flow:

  • after opening the picker, while the loading animation plays, the RAM usage spikes
  • once the memory runs full and the 502 is returned, I get a whole bunch of thumbnail-less image tiles with a "could not generate thumbnails" warning above.
    image
  • when clicking the broken gifs tile, another RAM spike, and no file information to the right until the memory is full
  • as soon as its full, the image preview shows up, allowing me to start the file deletion process
  • when clicking on delete, yet another RAM spike while waiting for memory to fill, which takes a while but does not stop me from ultimately deleting the file
  • as soon as the file is deleted, multiple hundreds of MB RAM (2-3x of the filesize) are freed up very quickly, as if there was some process still sitting in memory, only dying once the file is deleted.

@ssddanbrown
Copy link
Member

After setting it to 512M, some of the formerly failing gifs can now be uploaded, but still not all. When trying to upload the 41.gif from above, i see a notable spike in CPU and RAM usage while BookStack attempts to extract a thumbnail, but after ~2.3 seconds I still get the 502 error response with the same behaviour as before.

On my dev instance that I'm testing on there's a PHP memory_limit of 512M.
This is on a strong 16 thread system though. Testing on my smaller test virtual instances, they appeared to lock up in CPU/memory usage using this gif.

I think this GIF is particularly problematic due to the sheer amount of frames (over 200) at reasonable resolution.
Looking into the library we use and PHP itself, the native PHP gd-based image handling lacks support for animated gifs, so these are managed via a combination of PHP-based encoder to manage the animation frames backed by native PHP gd render functions to do the manipulation work. Therefore this is like resize 200 images in one go (and then I'm guessing some likley heavy interpolation & compression steps afterwards). The PHP gd functions can use more memory than the PHP-based limit, and I wouldn't be surprised if this GIF finds some edge-case memory leaks in this process.

Whatever's going on here, it's probably more sensible if we avoid handling animation during this process and instead just create a thumbnail based upon a single frame.
I think this is what used to happen when we originally added GIF support, but the library we use added animation support since.

@bytebone
Copy link
Author

Whatever's going on here, it's probably more sensible if we avoid handling animation during this process and instead just create a thumbnail based upon a single frame.

I fully agree, and since its only a preview for the image picker (presumably the gif gets natively rendered by the browser whenever its shown, without the need for processing by bookstack) IMO its perfectly fine to take the first frame of the gif and pull that out as the thumbnail.

Right now I cannot really tell if this is something you'd consider fixing soon-ish or some time later down the road. If this isn't important enough in the grander scheme to fix quickly, do you think some better error handling could be done in an update soon, so that the app doesn't behave so crazily whenever it happens? Having to dig around in the filesystem whenever a gif fails to generate a thumbnail gets tedious over time...

@ssddanbrown ssddanbrown added this to the BookStack v24.05.2 milestone May 25, 2024
@ssddanbrown
Copy link
Member

I'll assign to the next patch milestone. No time assurance for when that would be but should not be too far in the future.
Gracefully dealing with these types of scenerios can get fairly tricky, as it's going to depend quite significantly on environment and often we lack control in these events (PHP process jammed or OS kills the process). It'll be easier & less complex for us to make the change.

@jacobslutsky-blacksmith

Hello. I'm running into this issue with animated .gifs as well. The actual file size of the .gif is tiny, but it is also around 200 frames.
We currently have a VM with 4GB of RAM, running a PHP memory_limit of 256M.
Would you recommend increasing both the available RAM and the PHP memory_limit to say 1GB to get around this issue?
Or is there a patch for the single frame thumbnail generation?
Thanks.

ssddanbrown added a commit that referenced this issue Jun 9, 2024
Changes GIF image thumbnail handling to direcly load via gd instead of
going through interventions own handling (which supports frames) since
we don't need animation for our thumbnails, and since performance issues
could arise with GIFs that have large frame counts.

For #5029
@ssddanbrown
Copy link
Member

I've now updated our handling to only resize via native methods without animation support, via commit 3406846, with tests added to cover. This will be part of the next upcoming patch release so I'll therefore close this off.

@bytebone
Copy link
Author

Just tested the newest release, reset the memory limit by commenting out the added line (assuming the default is 256M), and uploaded the GIF that broke everything earlier, as well as a huge 9 MB gif. Both work very fast, instantly displaying in the document and image picker. Great work, thank you very much for the fast assistance and fix.

@jacobslutsky-blacksmith

Much thanks @ssddanbrown
We are using the latest release as well and all GIF's that previously caused the error are not doing so anymore.
Very nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants