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

go: Call 'heif_context_read_from_memory_without_copy' instead of deprecated function #67

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

leonklingele-work
Copy link
Contributor

heif.c:319 says:
// DEPRECATED: use heif_context_read_from_memory_without_copy() instead.

@farindk
Copy link
Contributor

farindk commented Aug 21, 2018

Please make sure that this does not introduce hard to find conflicts with the garbage collector.
If you use *from_memory_without_copy, it means that libheif assumes that the memory stays valid while the heif_context is still alive. If the Go GC decides to release the byte[] before that, you get into problems.

…ecated function

heif.c:319 says:
    // DEPRECATED: use heif_context_read_from_memory_without_copy() instead.
@leonklingele-work
Copy link
Contributor Author

@farindk ptal. This now allocates C memory and frees it again before exiting.

@farindk
Copy link
Contributor

farindk commented Aug 21, 2018

Doesn't that just set the finalizer for the copied array? I.e. it can still garbage-collect the array before the heif_context? I think the finalizer should be attached to the context, such that the array lifetime is at least as long as the context.

Then again, I have no idea about Go... So I might be wrong.

@@ -321,7 +321,12 @@ func (c *Context) ReadFromFile(filename string) error {

func (c *Context) ReadFromMemory(data []byte) error {
// TODO: Use reader API internally.
err := C.heif_context_read_from_memory(c.context, unsafe.Pointer(&data[0]), C.size_t(len(data)), nil)
cData := C.CBytes(data)
Copy link
Member

Choose a reason for hiding this comment

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

Why not store a reference to data in the Context? This would make sure it is kept alive until the Context is released.

If you do that, make sure to clear the referene manually if it is no longer needed (e.g. when loading from a file in the same context).

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.

3 participants