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

ImageDecoder+LibGfx: collate decoded bitmaps before sending over IPC #1435

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zack466
Copy link

@zack466 zack466 commented Sep 18, 2024

There is an issue (#425) where gifs with many frames cannot be loaded, as each bitmap is sent over IPC using a separate file descriptor, and there is limit on the maximum number of descriptors per IPC message. Thus, trying to load gifs with more than 64 frames (the current limit) causes the image decoder process to die.

This commit introduces the BitmapSequence class, which is a thin wrapper around the type Vector<Optional<NonnullRefPtr<Gfx::Bitmap>>> and provides an IPC encode/decode routine that collates all bitmap data into a single buffer so that only a single file descriptor is required per IPC transfer, even if multiple frames are being sent. This seems to fix the issue with large gifs (at least on my machine).

Right now, I'm using memcpy and pointer math to move data between AnonymousBuffers, which is probably not great for memory safety. If there's a better way of doing this that's more in line with project standards, please let me know.

@ladybird-bot
Copy link
Collaborator

Hello!

One or more of the commit messages in this PR do not match the Ladybird code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

There is an issue where gifs with many frames cannot be loaded, as each
bitmap is sent over IPC using a separate file descriptor, and there is
limit on the maximum number of descriptors per IPC message. Thus, trying
to load gifs with more than 64 frames (the current limit) causes the
image decoder process to die.

This commit introduces the BitmapSequence class, which is a thin wrapper
around the type Vector<Optional<NonnullRefPtr<Gfx::Bitmap>>> and
provides an IPC encode/decode routine that collates all bitmap data into
a single buffer so that only a single file descriptor is required per
IPC transfer, even if multiple frames are being sent.
@nico
Copy link
Contributor

nico commented Sep 18, 2024

Didn't #688 fix #425 already?

@zack466
Copy link
Author

zack466 commented Sep 18, 2024

Doesn't seem so? At least I was still experiencing the issue in #425 locally (as in the image decoder process would die when loading gifs with many frames).

@zack466
Copy link
Author

zack466 commented Sep 18, 2024

Also, even with the changes in #688, the issue seems to be that the image decoder is trying to send the type Vector<Optional<NonnullRefPtr<Gfx::Bitmap>>> over IPC, which boils down to sending one Gfx::Bitmap per frame, and each Gfx::Bitmap gets serialized as its own file with a file descriptor, which is why so many file descriptors get opened for large gifs.

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

Successfully merging this pull request may close these issues.

3 participants