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.

Sign up for GitHub

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

Jump to bottom

Images for emotes, badges, etc. are not lazily loaded as intended #3929

Open
4 tasks done
dnsge opened this issue Aug 17, 2022 · 0 comments · May be fixed by #3950
Open
4 tasks done

Images for emotes, badges, etc. are not lazily loaded as intended #3929

dnsge opened this issue Aug 17, 2022 · 0 comments · May be fixed by #3950
Labels
enhancement Feature: Emotes Issues related to Emotes

Comments

@dnsge
Copy link
Contributor

dnsge commented Aug 17, 2022 •
edited

Checklist

  • I'm reporting a problem with Chatterino
  • I've verified that I'm running the most recent nightly build or stable release
  • I've looked for my problem on the wiki
  • I've searched the issues and pull requests for similar looking reports

Describe your issue

As @pajlada hinted at in #3915 (comment), lazy loading for images that aren't visible (e.g. in a different split tab) is broken.

Currently, images are supposed to be accessed by the pixmapOrLoad method in Image:

chatterino2/src/messages/Image.cpp

Lines 334 to 341 in c204332

boost::optional<QPixmap> Image::pixmapOrLoad() const
{
assertInGuiThread();
this->load();
return this->frames_->current();
}

This lazy loading works as long as no other code calls Image::load. However, in ImageSet, we call load manually:

chatterino2/src/messages/ImageSet.cpp

Lines 87 to 92 in c204332

const ImagePtr &ImageSet::getImageOrLoaded(float scale) const
{
auto &&result = getImagePriv(*this, scale);
// get best image based on scale
result->load();

The intent behind calling load is that we will attempt to load the highest resolution of the image but fall back to any already loaded resolutions if they exist (see rest of method body).

This wouldn't be a problem if getImageOrLoaded was called in the rendering logic which only runs for the visible splits. However, this method is actually called in the message layout logic, which runs for all splits as messages are added:

chatterino2/src/messages/MessageElement.cpp

Lines 179 to 189 in c204332

void EmoteElement::addToContainer(MessageLayoutContainer &container,
MessageElementFlags flags)
{
if (flags.hasAny(this->getFlags()))
{
if (flags.has(MessageElementFlag::EmoteImages))
{
auto image =
this->emote_->images.getImageOrLoaded(container.getScale());
if (image->isEmpty())
return;

I wanted to detail this problem and my investigation into it here so we can find a good solution for properly doing lazy loading. I couldn't immediately think of a solution that uses already-loaded images while queueing up higher resolution images.

This problem reduces the effectiveness of #3915 too, and fixing this problem should also lead to a reduction in memory usage by images.

OS and Chatterino Version

Chatterino 7.3.5 DEBUG (commit 2234670) built with Qt 5.15.2 Running on macOS 12.3, kernel: 21.4.0
@dnsge dnsge added the issue-report An issue reported by a user. label Aug 17, 2022
@Felanbird Felanbird added enhancement Feature: Emotes Issues related to Emotes and removed issue-report An issue reported by a user. labels Aug 17, 2022
@dnsge dnsge linked a pull request Sep 5, 2022 that will close this issue
Open
2 tasks
@Nerixyz Nerixyz mentioned this issue Sep 7, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement Feature: Emotes Issues related to Emotes
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Lazily load images on paint, not layout dnsge/chatterino2
2 participants
@dnsge @Felanbird