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

fix: Fix performances on the photos gallery #5447

Merged
merged 3 commits into from
Jun 28, 2024

Conversation

g123k
Copy link
Collaborator

@g123k g123k commented Jun 27, 2024

Hi everyone!

I have investigated a bit the UI jank on the photos' gallery:

  • The memory footprint is high
  • The color palette is expensive

Potential fixes:

  • Use the color palette extractor in a queue-like mechanism: problem fixed, but feels weird to have the color being visible 1 sec later
  • Use the algorithm in an Isolate: impossible, dart:ui is not available 🤬
  • Use cacheWidth/cacheHeight to lower memory footprint: 👌

That's why I have removed the color extractor algorithm (there are many complaints on the Flutter GitHub).
cacheHeight is used to lower the memory footprint

On my Pixel 8 Pro, in profile mode, scrolling the grid have an average of 115 fps.
The memory footprint is a bit reduced.
Still not perfect, but faaaaar better.

@g123k g123k requested a review from a team as a code owner June 27, 2024 07:41
@g123k g123k linked an issue Jun 27, 2024 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented Jun 27, 2024

Codecov Report

Attention: Patch coverage is 0% with 26 lines in your changes missing coverage. Please review.

Project coverage is 7.18%. Comparing base (4d9c7fc) to head (fdbbb34).
Report is 230 commits behind head on develop.

Files Patch % Lines
...p/lib/generic_lib/widgets/images/smooth_image.dart 0.00% 14 Missing ⚠️
...ooth_app/lib/pages/image/product_image_widget.dart 0.00% 8 Missing ⚠️
.../pages/image/product_image_gallery_other_view.dart 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           develop   #5447      +/-   ##
==========================================
- Coverage     9.54%   7.18%   -2.37%     
==========================================
  Files          325     391      +66     
  Lines        16411   20377    +3966     
==========================================
- Hits          1567    1464     -103     
- Misses       14844   18913    +4069     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +40 to +41
final int? cacheWidth;
final int? cacheHeight;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those cache parameters may be helpful regarding memory, but not network latency.
Please consider implementing my suggestion about ImageSize.DISPLAY. That will dramatically help for memory too!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will.
I think, I will also add all variants of ImageSize

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good. Btw I don't think all variants are always implemented; I would focus on 400 and FULL only.

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @g123k!
LGTM!
I have minor comments; feel free to ignore them.
Are the performances better now?

final Widget image = SmoothImage(
width: widget.squareSize,
height: widget.squareSize,
cacheHeight:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No cacheWidth?

It's a bit strange to say "you can force ImageSize if you want, but regardless we will force a cacheHeight even if you don't want to and there's nothing you can do about it"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we have a square, but the server's photo may have a different ratio.
If I say, cacheWidth == cacheHeight, it means the server picture is a square.
That's why, I only provide one of the argument

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the square effect.

I don't know how relevant this cache option would be, performance-wise.
My assumption is that the best performance improvement is with smaller images (400).

I also fear that this cache option entail side-effects: what if I download a full image with that cache option for a thumbnail, and then display the full image on a full screen after a tap? From the docs, I assume that the cached image will be stored as resized, and then reused, which would render a pixelized image.

At least, you could add a comment about why cacheHeight but not cacheWidth.

Comment on lines +155 to +156
ImageSize.THUMB,
ImageSize.SMALL,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ImageSize.THUMB,
ImageSize.SMALL,

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's OK with THUMB and SMALL, I don't really understand why you want to ditch them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to ditch them because I have experience in images and I remember that not all values are always populated, only 400 and FULL.
Just as an example, I've tried the latest barcode I used (https://fr.openfoodfacts.org/api/v3/product/3661344961326?fields=images):

{
  "code": "3661344961326",
  "errors": [],
  "product": {
    "images": {
      "1": {
        "sizes": {
          "100": {
            "h": 100,
            "w": 75
          },
          "400": {
            "h": 400,
            "w": 300
          },
          "full": {
            "h": 3264,
            "w": 2448
          }
        },
        "uploaded_t": 1428607891,
        "uploader": "openfoodfacts-contributors"
      },
      "12": {
        "sizes": {
          "100": {
            "h": 100,
            "w": 99
          },
          "400": {
            "h": 400,
            "w": 396
          },
          "full": {
            "h": 2475,
            "w": 2448
          }
        },
        "uploaded_t": 1550253015,
        "uploader": "moon-rabbit"
      },

And so on.
It looks like "raw" images only have 100, 400 and full, never 200.

That's why I recommend 400 or FULL:

  • 200 doesn't seem to exist for "raw" images, at least on a regular basis
  • it's highly unlikely that user screens are less than 3*100 wide
  • if we summon a 200 image that doesn't exist, that will look like unknown image
  • the code would be much easier to maintain in a simple 400 or FULL choice
  • the added value of the current code is "optimized image versus full image" (regardless of 100, 200 or 400) - and the added value of 100 versus 200 or 400 is probably less important

@teolemon teolemon merged commit 1c4b959 into openfoodfacts:develop Jun 28, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

The photo list performs very badly
4 participants