search

jamsinclair / jSquash

Browser & Web Worker focussed image codec wasm bundles derived from the Squoosh App.
Apache License 2.0
250 stars 19 forks source link

Resize component memory leak #51

Closed seabye closed 7 months ago

seabye commented 7 months ago

The resize component may have a memory leak. I have tried executing it consecutively in both the browser and Deno environment, and the memory usage rapidly increases without being reclaimed.

Another issue is with the PNG decoder, which consistently encounters the error "Failed to construct 'ImageData': The input data length is not a multiple of 4." for certain images. This requires relying on the browser's canvas for PNG decoding (which works fine in the browser), but the problem arises when running on the server-side in Deno where a usable canvas is not available.

jamsinclair commented 7 months ago

Thanks @seabye, I believe the PNG decoding issue should be fixed with the version I just published, 3.0.1.

I'll leave this open as there is likely still a memory leak issue with the resize module. I'll need to set aside some time in the future to look into that 👍

seabye commented 7 months ago

Image resize has a small issue. Sometimes it can throw a 'RangeError: offset is out of bounds' error. My solution on the browser was to add a setTimeout function to the source image's 'src' load event. I also encountered this error on the backend. In theory, the data should be ready when inputting the imageData into the resize function, and adding a delay shouldn't make a difference. However, I'm not sure why adding a delay seems to prevent this problem. I'm not entirely certain about the conditions that trigger this error and whether setTimeout can completely avoid this issue, but it does occur. Perhaps this is an error related to memory leakage, as you have already noticed.

--

Oh, let me correct myself. This issue doesn't seem to be related to image resize because on the browser side, I'm currently using canvas to resize the image, and I'm still encountering this error. My code is a bit messy, and after some time, it's getting difficult to distinguish things clearly.

jamsinclair commented 7 months ago

Thanks @seabye. It sounds more like asynchronous code issue. I hope cleaning it up makes it easier to work with 👍

jamsinclair commented 7 months ago

I've also found the source of the memory leak. Looks like this is the culprit is https://github.com/rustwasm/wee_alloc/issues/106. I'll remove that dependency later in the week. It'll add an additional 10KB or so to the wasm file.

I also will note that WASM memory can only grow. For example, if you started a resizing a 2000px by 2000px image it'll grow its memory to allow it to operate on that size. It won't be be able deallocate that memory after it's reserved. This may also make it appear as a "leak". Subsequent resize calls shouldn't grow the memory unless the resize operation is larger than previously reserved memory. The WASM module can be destroyed and this will free up the memory.

One easy way around this is to do the operations in a short-lived WebWorker. This would also speed up your web application as the resizing would happen off the main thread.

seabye commented 7 months ago

It looks like it will be resolved soon. I also noticed that, at first glance in the task manager, even though it increases memory usage when I process thousands or tens of thousands of images in a row, it doesn't seem to crash. However, on the other hand, this kind of mistake might also be considered a memory leak.

Now, I am processing a small number of images on a scheduled basis, and the server is running. To avoid continuous growth, I have made it a sub-thread that closes after the scheduled processing is completed, and currently, this usage seems to be fine. This matter is not urgent for me at the moment, but a few weeks ago, when I was doing a lot of operations, it looked quite alarming. At that time, there were significant issues with PNG, which forced me to handle it with canvas on the browser side. It seems that you have already solved the PNG issue, but I haven't updated and tried that part yet.

By the way, I thought of the magic number. When I was processing a large number of images, I encountered cases where the file extensions did not match the actual formats, and I used the magic number to handle them. Perhaps this is a good idea to enhance the capability of correctly identifying images in order to facilitate matching the decoders. This kind of incorrect file extension might be a common situation, and adding recognition functionality would be convenient for the users. (In the process of handling network images, I would need to create a copy of response.arrayBuffer() in fetch and then determine the correct decoder based on that.)

Thank you so much for your hard work!

jamsinclair commented 7 months ago

Thanks I've just published v2.0.0 that should fix the memory leak issue. Let me know if you have any issues with it!

By the way, I thought of the magic number

That's a great idea! I believe there's already some good solutions out there, at this stage I don't think I'll be considering it. I did find this package that seems actively maintained and may be of use to you

Good luck with your project! 🎉

seabye commented 7 months ago

Sure. I forgot to search, and inadvertently ended up implementing the format recognition for images myself. If it already exists, there's no need to reinvent the wheel.

🎉 That's great! Now I don't have to consider using Rust FFI.

I'll find some time to run an update again, and if there are any issues, I'll provide feedback.