-
-
Notifications
You must be signed in to change notification settings - Fork 307
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
base: master
Are you sure you want to change the base?
Adding a new emscripten js method to parse depth images #1029
Conversation
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. |
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"))); |
There was a problem hiding this comment.
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"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another leftover
@EzraH442 Could you please do a review on this PR (only the changes in the file |
{ | ||
depth_result.set("err", "could not get handle of depth image"); | ||
depth_result.set("id", depth_image_id); | ||
return depth_result; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same issue as above
heif_item_id *ids = (heif_item_id *)malloc(n_images * sizeof(heif_item_id)); | ||
|
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
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"
Adding a new emscripten js method to parse depth images