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(animimage): add NULL pointer check #6206

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

yinntian
Copy link

@yinntian yinntian commented May 9, 2024

Description of the feature or fix

When dsc sets an incorrect NULL value, it can cause the animation's callback function to incorrectly take an array subscript value for the NULL value and cause the program to crash, which should prompt an error message and return.

Notes

Copy link
Collaborator

@XuNeo XuNeo left a comment

Choose a reason for hiding this comment

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

Could you also add a commit to fix the value error in lv_animimg_set_src?

I think it should be lv_anim_set_values(&animimg->anim, 0, (int32_t)num);

https://github.com/lvgl/lvgl/pull/6206/files#diff-e02308a3e1e1189a9fb9cc6f0e8408f740eb84b5685ee8c7e5bc17b6c72a2543L69-L76

XuNeo
XuNeo previously approved these changes May 9, 2024
liamHowatt
liamHowatt previously approved these changes May 9, 2024
Copy link
Collaborator

@liamHowatt liamHowatt left a comment

Choose a reason for hiding this comment

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

@XuNeo the lv_example_animimg_1 now looks more correct after removing the - 1.

@kisvegabor
Copy link
Member

If you modify lv_example_animimg_1 like this:

lv_animimg_set_repeat_count(animimg0, 1);

On the last frame it will set image[0] again due to this in index_change:

static void index_change(lv_obj_t * obj, int32_t index)
{
    int32_t idx;
    lv_animimg_t * animimg = (lv_animimg_t *)obj;

    idx = index % animimg->pic_count;

For good timing we should really remove the -1 but use this in index_change:

if(index >= animimg->pic_count) index =  animimg->pic_count - 1; 

@XuNeo
Copy link
Collaborator

XuNeo commented May 11, 2024

if(index >= animimg->pic_count) index = animimg->pic_count - 1;

This is better. The anim value is mapped directly to the image index, thus lv_anim_set_values(&animimg->anim, 0, (int32_t)num - 1); sounds correct. if(index >= animimg->pic_count) index = animimg->pic_count - 1; makes sure if animation has overshoot, the index is still in range.

@yinntian Could you please update the PR accordingly?

@yinntian
Copy link
Author

@XuNeo
Now here's the state of my attempts:

lv_anim_set_values(&animimg->anim, 0, (int32_t)num);
static void index_change(lv_obj_t * obj, int32_t index)
{
    int32_t idx;
    lv_animimg_t * animimg = (lv_animimg_t *)obj;

    if(animimg->dsc == NULL){
        return ;
    }
    
    if(index >= animimg->pic_count){
        index = animimg->pic_count - 1;
    }
    idx = index % animimg->pic_count;
    
    // debug code -------
    static int debug_count = -2;
    printf("%d ", idx);
    debug_count++;
    if(debug_count % 4 == 0){
        printf("\n");
    }
    //-------

    lv_image_set_src(obj, animimg->dsc[idx]);
}

Looks like the animation is normal

动画2

But by the output message, the change of 'idx' is a cycle of 0, 1, 2, 2, but the ‘2’ output will immediately output ‘0’, is it possible that there is something wrong with anim in this behavior?

@FASTSHIFT FASTSHIFT changed the title fix(animimage): null pointer check fix(animimage): add NULL pointer check May 12, 2024
@kisvegabor
Copy link
Member

idx = index % animimg->pic_count; is not required. It looks like this for a 3 second animation with 3 images:

  0s        1s          2s         3s (stop)
  img0      img1        img2       img2
   ---------- ----------- ----------
       1s         1s          1s

As you can see this way each image were shown for 1 sec.

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.

None yet

5 participants