lvgl: Incorrect code in img_get_srctype
Perform all steps below and tick them with [x]
- Check the related part of the Documentation
- Update lvgl to the latest version
- Reproduce the issue in a Simulator
Describe the bug
Currently, when setting a src to a lv_img_t
, there are multiple places where the source type is queried to figure out if it’s a file, a symbol or the data itself.
The issue comes from the fact that, in order to assert what kind of src it is, the “parser” estimate that ASCII char in range 32-127 is a file, 0 to 31 is the data and above 127 is a symbol IIUC.
I think using the same information for storing 3 different things is prone to error anyway. Also, by not storing the actual source type explicitly leads to a performance degradation, since every place where the src_type is requested, the heuristics have to be run again (and this often implies hitting the file system or at least doing a string compare which is slow on a long string).
To Reproduce
Expected behavior
Instead of having guess code going on here, we should have a structure describing the type of the data to use, with an enum for the supported types (the enum is the actual src_type) and the sub-type (what kind of decoder is required to decoder it (since we’ll have them in the decoder’s context soon).
This would avoid the strlen(filename) + strcmp(filename, extension
which are both O(N) and O(N*M) each time the src is parsed (that is, for each frame of an animation).
I think the lv_img_set_src
should specify the type of the source to use. In my partial_rlottie PR #2904, I’ve added a lv_img_set_src_ex
function, and I think we could add the src type as an argument here so that:
- No misdetection can happen (upon multiple file format supporting like json for rlottie / rive / whatever)
- No need for IMG_DECLARE macro anymore (this is clunky anyway, IMHO, since this object is only required for bin format, all other formats can find their image description by themselves). This would save precious flash space by not repeating the information. We can pass the image header as an optional argument too to the builtin decoder context.
- No query of the src type anymore in all functions later on the chain.
Typically, lv_img_set_src
would do (for backward compatibility):
void lv_img_set_src(obj, src) {
src_type_t src_type = lv_get_src_type(src);
dec_type_t dec_type = LV_BUILTIN_ID;
if (src_type == SRC_FILE) dec_type = find_decoder_for_path(src);
lv_img_set_src_ex(obj, src, src_type, dec_type, NULL);
}
Ideally, the linked list for the decoders could then be changed from a linked list to a map (decoder id=>decoder instance). That way, instead of doing a O(N) search in the decoder for a given type, use a O(1) map for a decoder pointer directly. Or, at least, have a decoder index in the list stored in the img_t so the O(N) does not imply calling N-1 decoder’s get_info
function that are very expensive.
What do you think?
About this issue
- Original URL
- State: closed
- Created 3 years ago
- Comments: 20 (20 by maintainers)
I’ve worked on dithering PR and likely finished it, please review. I’m going to resume the work on the partial rlottie support that’ll integrate the changes discussed above.
Just a remark: There is no reason why an image format couldn’t have multiple (virtual) frames, since we can store each frame vertically. So adding a
.abin
support for animated button on stored binary image is very possible.