sxyazi / yazi

💥 Blazing fast terminal file manager written in Rust, based on async I/O.
https://yazi-rs.github.io
MIT License
16.72k stars 389 forks source link

refactor(adapter): extract `image_area` from `image_show` #1900

Closed gaesa closed 1 week ago

gaesa commented 1 week ago

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.

sxyazi commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 https://github.com/sxyazi/yazi/blob/c477f68058ca3d546aca0c1533cc582e270cc3ef/yazi-adapter/src/image.rs#L44-L45
  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 commented 1 week ago

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.