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

refactor(adapter): extract image_area from image_show #1900

Closed
wants to merge 1 commit into from

Conversation

gaesa
Copy link
Contributor

@gaesa gaesa commented Nov 11, 2024

Exposing image_area separately allows future Lua integration to handle image placement more flexibly, without needing to invoke full rendering. This change promotes modularity and simplifies obtaining image dimensions alone.

This refactor provides the foundation for feature PR #1897, preparing for easier integration.

Exposing `image_area` separately allows future Lua integration to handle
image placement more flexibly, without needing to invoke full rendering.
This change promotes modularity and simplifies obtaining image dimensions alone.

This refactor provides the foundation for feature PR sxyazi#1897, preparing for easier integration.
@sxyazi
Copy link
Owner

sxyazi commented Nov 11, 2024

Why not implement the feature directly in image_show() so that it supports alignment, instead of requiring an extra step to get image_size() first?

To me, this is a significant performance loss, as now two I/O reads are necessary. Plus, for large images that haven't been downscaled, an additional downscale() operation is needed.

@gaesa
Copy link
Contributor Author

gaesa commented Nov 11, 2024

Why not implement the feature directly in image_show() so that it supports alignment

This approach is indeed feasible, and I’ve considered it as well. At present, I see two possible ways to implement it. However, it would require more extensive changes to the codebase, which I’m uncertain would be ideal. For instance, adding an alignment parameter to image_show would require altering the function signature. Alternatively, we could use the global PREVIEW variable to access alignment, which would be a less intrusive change but less flexible, as it would limit direct control over image placement in the Lua API. If modifying the image_show function signature is acceptable, I could consider submitting a new PR with these changes. I’m definitely open to other potential approaches if they better suit the codebase.

This is a significant performance loss

Yes, this would be true if both operations (getting the image size and displaying the image) are applied to the same image.

An additional downscale() operation is needed

Currently, in all different image display protocols, image_area is extracted directly from image_show. As long as the original image_show performs downscaling, any image_area extracted from it will already have this downscaling applied.

@sxyazi
Copy link
Owner

sxyazi commented Nov 11, 2024

If modifying the image_show function signature is acceptable, I could consider submitting a new PR with these changes. I’m definitely open to other potential approaches if they better suit the codebase

Sure, I can accept changing the function signature as long as it benefits performance.

As long as the original image_show performs downscaling, any image_area extracted from it will already have this downscaling applied.

If I understand correctly, from another PR, image_show_aligned() API calls both image_area() and image_show(), and both of these functions ultimately call downscale(). The issues here are:

  1. The image gets fully decoded twice, even if it doesn't need to be downscaled
    pub(super) async fn downscale(path: &Path, rect: Rect) -> Result<DynamicImage> {
    let (mut img, orientation, _) = Self::decode_from(path).await?;
  2. Even when image_area() has already downscaled the image, image_show() still uses the original image without utilizing the already downscaled result. This leads to images that need downscaling getting downscaled twice.

@gaesa
Copy link
Contributor Author

gaesa commented Nov 11, 2024

Regarding downscaling, if the Lua side only calls image_show without using image_area or image_show_aligned, there won’t be any change in performance. However, it is correct that if image_area is used directly or indirectly in conjunction with image_show, duplicate processing would occur in this case.

I believe this PR can be closed for now. Since modifying the function signature is acceptable, I’ll plan to adjust the original draft PR’s feature implementation directly.

@gaesa gaesa closed this Nov 11, 2024
@gaesa gaesa deleted the refactor-image-show branch November 11, 2024 03:21
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants