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

struct refmvs_frame: Make fields safer without changing layout #822

Closed
2 tasks done
Tracked by #706
randomPoison opened this issue Mar 15, 2024 · 4 comments · Fixed by #983
Closed
2 tasks done
Tracked by #706

struct refmvs_frame: Make fields safer without changing layout #822

randomPoison opened this issue Mar 15, 2024 · 4 comments · Fixed by #983
Assignees

Comments

@randomPoison
Copy link
Collaborator

randomPoison commented Mar 15, 2024

Also cleanup rav1d_refmvs_init and rav1d_refmvs_clear.

  • frm_hdr - Pointer to f.frame_hdr. We may be able to pass frame_hdr into the places where it's used. frm_hdr is only used from rav1d_refmvs_find and add_temporal_candidate, which is only called from rav1d_refmvs_find. So if we can pass f.frame_hdr into rav1d_refmvs_find and then down into add_temporal_candidate we should be able to remove this field. Since frm_hdr is not used by load_tmvs_c (and thus not accessed by asm), we should change it to a *const (), signifying that it should never be used while keeping the layout the same. It is dangerous to keep it as a *const Rav1dFrameHeader, since it's a *const Dav1dFrameHeader in C. And passing the &Rav1dFrameHeader through the callstack removes the lifetime issue as well.
  • rp - Points to f.mvs. rp is also not accessed in load_tmvs_c, so hopefully we can change it (in a layout preserving manner).
@randomPoison randomPoison changed the title refmvs_frame refmvs_frame: Make fields safe Mar 15, 2024
@fbossen
Copy link
Collaborator

fbossen commented Mar 15, 2024

There may be a conflict with #821 where a pointer to a struct refmvs_frame is passed to an asm function.

@randomPoison
Copy link
Collaborator Author

Ah, that's a good point. Even before #821 refmvs_frame is being passed into asm functions, so I'm not sure we can actually make changes to it safely. The sister struct refmvs_tile seems to only be used from Rust so I think we can still clean that one up.

@randomPoison randomPoison closed this as not planned Won't fix, can't repro, duplicate, stale Mar 15, 2024
@kkysen
Copy link
Collaborator

kkysen commented Mar 19, 2024

I think we should do part of this. At least, frame_hdr should be made a *const () and passed separately as a reference through the callstack as @randomPoison proposed above. It seems dangerous to leave a *const Rav1dFrameHeader when C has a *const Dav1dFrameHeader. We can make refmvs_frame safer; we just can't change its layout at all, and we can't fields that are accessed in load_tmvs_c (and thus what's accessed by asm). See also #821 (comment).

@kkysen kkysen reopened this Mar 19, 2024
@kkysen kkysen changed the title refmvs_frame: Make fields safe struct refmvs_frame: Make fields safer without changing layout Mar 19, 2024
@kkysen kkysen self-assigned this Mar 27, 2024
kkysen added a commit that referenced this issue Mar 27, 2024
* Fixes `frm_hdr` field of #822.

This removes the `*const Rav1dFrameHeader` in the field, passing it
through the call stack. But we can't change the field's layout since
`*const refmvs_frame` is passed to asm, so we make the field `*const ()`
and `ptr::null()`. This also fixes the problem of accidentally
conflating `*const Rav1dFrameHeader` and `*const Dav1dFrameHeader` in
that field.
@kkysen
Copy link
Collaborator

kkysen commented May 2, 2024

@kkysen kkysen closed this as completed May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants