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

feat(display): add save screenshot to file support #5481

Merged
merged 2 commits into from
Feb 20, 2024

Conversation

FASTSHIFT
Copy link
Collaborator

Description of the feature or fix

Added screenshot saving file function to facilitate debugging of images during the drawing process.

Thanks to draw_buf, which unifies the data structure of the drawing buffer, it is extremely convenient to expand related functions later.

Notes

Copy link
Collaborator

@PGNetHun PGNetHun left a comment

Choose a reason for hiding this comment

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

Sorry, but I think this PR should not be added to display and draw_buf, because:

  1. display/draw_buf should contain only tightly related functions, and not any unrelated things. Meaning: do not add function to create a file in file system to write actual screen buffer into it. That should be done in a separate code (higher level of architecture).
  2. -> Please avoid the "god object" anti-pattern!
  3. There is already a feature to take screenshots: snapshot
  4. Do not add a function that will be used (only) for testing / debugging
  5. display/draw_buf should not know about FS module
  6. if FS is not enabled in lv_conf.h, then saving would throw error

@XuNeo
Copy link
Collaborator

XuNeo commented Jan 27, 2024

That should be done in a separate code (higher level of architecture).

How about adding utils directly in lvgl/src/others/utils/lv_utils.h

It's really useful to debug drawing issues, we have been using the same method since v8 together with obj screenshot.

The code is not only used for debugging but can also be used by application. For example, current app view can be saved to file system as an app preview image, and then be used in background task manager.

@PGNetHun
Copy link
Collaborator

PGNetHun commented Jan 27, 2024

Yes, if that is really needed, then it can be added to lv_utils.h; but FS-related calls have definitely nothing to do in display module / drawing layer (from archictecture perspective). If that function would save/duplicate the draw_buf into a separate memory address, then we could keep it there, but a file system saving function should not be there.
These not core functionalities should go definitely in separate modules, like lv_utils.h, etc.
Keep architecture and code structure clean, and separate core functionality from "higher-level" codes.

@XuNeo XuNeo force-pushed the feat_disp_screenshot branch 2 times, most recently from bcbcdea to 3793f74 Compare January 29, 2024 08:13
@XuNeo XuNeo requested a review from PGNetHun January 29, 2024 08:15
src/misc/lv_utils.c Show resolved Hide resolved
src/misc/lv_utils.c Show resolved Hide resolved
@kisvegabor
Copy link
Member

What about making it a snapshot feature, like lv_snapshot_take_to_file? It would be more generic, not only for a display.

It would be even better to have a lv_draw_buf_save_to_file helper.

@XuNeo
Copy link
Collaborator

XuNeo commented Jan 29, 2024

What about making it a snapshot feature, like lv_snapshot_take_to_file? It would be more generic, not only for a display.

It could also be really useful. Let's prepare it in another PR after #5487 is reviewed.

Copy link
Collaborator

@PGNetHun PGNetHun left a comment

Choose a reason for hiding this comment

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

Because file operations (especially saving many kilobytes to flash) can take many hundred miliseconds, it is better to duplicate the draw buf in memory, and saving that to file. Memory copy finishes faster, so even if source draw buf is not locked, the chance of “transient” state is less than saving the source directly to file system.

So, you have to decide: block drawing for a few ten-hundred miliseconds to save draw buf to filesystem, or duplicate it (more memory needed) and save that snapshot.

@lvgl-bot
Copy link

We need some feedback on this pull request.

Now we mark this as "stale" because there was no activity here for 14 days.

Remove the "stale" label or comment else this will be closed in 7 days.

@lvgl-bot lvgl-bot added the stale label Feb 14, 2024
@FASTSHIFT FASTSHIFT removed the stale label Feb 14, 2024
Signed-off-by: pengyiqiang <pengyiqiang@xiaomi.com>
@XuNeo
Copy link
Collaborator

XuNeo commented Feb 19, 2024

LVGL operates on a single thread, which prevents the use of its API in a separate thread for file writing. It is advisable to allow the user to determine the best approach, whether that involves duplicating a new draw buffer for a different thread or creating a static loading UI that displays saving file....

This API could serve as an initial step for users who wish to optimize it. Additionally, this API is not solely beneficial for debugging. It can also be utilized to save PNG decoded images as .bin images, for instance.

Signed-off-by: Xu Xingliang <xuxingliang@xiaomi.com>
@XuNeo XuNeo requested a review from PGNetHun February 19, 2024 12:35
@XuNeo XuNeo merged commit b7b8d8f into lvgl:master Feb 20, 2024
16 checks passed
@FASTSHIFT FASTSHIFT deleted the feat_disp_screenshot branch February 22, 2024 04:21
HongChao6 pushed a commit to HongChao6/lvgl that referenced this pull request Oct 18, 2024
Signed-off-by: pengyiqiang <pengyiqiang@xiaomi.com>
Signed-off-by: Xu Xingliang <xuxingliang@xiaomi.com>
Co-authored-by: pengyiqiang <pengyiqiang@xiaomi.com>
Co-authored-by: Xu Xingliang <xuxingliang@xiaomi.com>
HongChao6 pushed a commit to HongChao6/lvgl that referenced this pull request Oct 18, 2024
Signed-off-by: pengyiqiang <pengyiqiang@xiaomi.com>
Signed-off-by: Xu Xingliang <xuxingliang@xiaomi.com>
Co-authored-by: pengyiqiang <pengyiqiang@xiaomi.com>
Co-authored-by: Xu Xingliang <xuxingliang@xiaomi.com>
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.

5 participants