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

Check if the frame is writable before copyPlanes #84

Merged
merged 3 commits into from
Oct 14, 2024

Conversation

kurochan
Copy link
Contributor

For safety of frame operations, check if the frame is writable before copyPlanes.

If this seems to be a performance overhead, I would like input as this PR may not be necessary.

Copy link
Owner

@asticode asticode left a comment

Choose a reason for hiding this comment

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

At first I forced the frame to be writable by adding .MakeWritable() when copying planes but then I realized the developer should be free to decide when to make the frame writable. However checking whether the frame is writable seems like a good idea even though I don't really know whether this is a big performance drop 👍

Could you also add tests for this use case? Something like the following added here(after a newline)

f3 := astiav.AllocFrame()
defer f3.Free()
require.NoError(t, f3.Ref(f2))
require.Error(t, fd2.FromImage(i1))
require.Error(t, fd2.SetBytes(b1, align))

frame_data.go Outdated Show resolved Hide resolved
frame_data.go Show resolved Hide resolved
frame_data_test.go Outdated Show resolved Hide resolved
@kurochan kurochan requested a review from asticode October 14, 2024 13:12
require.NoError(t, f3.Ref(f2))
require.Error(t, fd2.FromImage(i1))
require.Error(t, fd2.SetBytes(b1, align))
f3.MakeWritable()
Copy link
Owner

Choose a reason for hiding this comment

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

f2 should be the frame that we make writable here

@kurochan kurochan requested a review from asticode October 14, 2024 14:38
@asticode asticode merged commit 2a48086 into asticode:master Oct 14, 2024
3 checks passed
@asticode
Copy link
Owner

Thanks for the PR! ❤️

Let me know whether you need a tag 👍

@kurochan
Copy link
Contributor Author

Please tag this as well 🚀

@asticode
Copy link
Owner

I've created a v0.23.0 tag 👍

asticode pushed a commit that referenced this pull request Oct 23, 2024
* check if the frame is writable before copyPlanes

* apply review

* make f2 writable
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.

2 participants