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

Adding a new emscripten js method to parse depth images #1029

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

Conversation

Dakantz
Copy link

@Dakantz Dakantz commented Nov 4, 2023

Adding a new emscripten js method to parse depth images

@Dakantz Dakantz changed the title Master Adding a new emscripten js method to parse depth images Nov 4, 2023
@farindk
Copy link
Contributor

farindk commented Nov 4, 2023

Thanks. One thing, though: your PR contains a lot of code reformatting in unrelated parts. Could you please clean up the PR such that only your new code is contained? Also make sure not to merge "master" into your branch. This will make the history unnecessarily complicated.

@Dakantz
Copy link
Author

Dakantz commented Nov 27, 2023

Sorry for the merge and formatting mistake, should be fixed now!

This is one of my first PRs 😅

I hope it can be merged now!

return depth_result;
}

depth_result.set("testdata", emscripten::val(emscripten::typed_memory_view(10, "123456890")));
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like this is a leftover from testing

depth_result.set("data", emscripten::val(emscripten::typed_memory_view(stride * height, plane)));
depth_result.set("buffersize", stride * height);
depth_result.set("channel", channel);
depth_result.set("testdata", emscripten::val(emscripten::typed_memory_view(10, "123456890")));
Copy link
Contributor

Choose a reason for hiding this comment

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

another leftover

@farindk
Copy link
Contributor

farindk commented Jul 10, 2024

@EzraH442 Could you please do a review on this PR (only the changes in the file libheif/heif_emscripten.h.

Comment on lines +268 to +272
{
depth_result.set("err", "could not get handle of depth image");
depth_result.set("id", depth_image_id);
return depth_result;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to return an heif_error here since heif_error is registered as an value object see here, that way it won't be necessary to manually delete the object on the JS side see here

Comment on lines +284 to +287
depth_result.set("err", "could not get image of depth image");
depth_result.set("msg", err.message);
depth_result.set("id", depth_image_id);
return depth_result;
Copy link
Contributor

Choose a reason for hiding this comment

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

same issue as above

Comment on lines +338 to +339
heif_item_id *ids = (heif_item_id *)malloc(n_images * sizeof(heif_item_id));

Copy link
Contributor

Choose a reason for hiding this comment

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

Also here there should be a check to make sure the malloc didn't return a null pointer, similar to here

depth_result.set("id", depth_image_id);
return depth_result;
}
static emscripten::val heif_js_get_depth_imgs_decoded(struct heif_image_handle *handle)
Copy link
Contributor

Choose a reason for hiding this comment

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

based on what this function does a better name might be heif_js_image_handle_get_depth_img_handle, since it doesn't actually do any image decoding

}
}

depth_result.set("err", "could get no plane of depth image");
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be a little bit pedantic but I'd rephrase this error message to "could not get plane of depth image"

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