Skip to content

Conversation

msluszniak
Copy link
Member

@msluszniak msluszniak commented Jul 17, 2025

Description

This PR make ImageProcessing module cleaner and more readable.

Needs to be tested - implement tests and run them

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update (improves or adds clarity to existing documentation)
  • Improving code style

Tested on

  • iOS
  • Android

Testing instructions

Screenshots

Related issues

Checklist

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the documentation accordingly - No changes needed
  • My changes generate no new warnings

Additional notes

@msluszniak msluszniak marked this pull request as draft July 17, 2025 07:22
@msluszniak
Copy link
Member Author

I'll wait for writing tests for this one till merging #438

@msluszniak msluszniak force-pushed the @ms/improve_image_processing_utils branch from 408c1ae to b380ba8 Compare August 30, 2025 16:28
@msluszniak msluszniak marked this pull request as ready for review September 1, 2025 21:26
@msluszniak
Copy link
Member Author

I had a problem with adding ImageProcessing.cpp to testing setup. I cannot force cmake to find executorch/extension/tensor/tensor.h I got the same error:

[ 13%] Built target gtest
[ 26%] Built target gtest_main
[ 33%] Building CXX object CMakeFiles/RNExecutorchTests.dir/ImageProcessingTest.cpp.o
In file included from /react-native-executorch/packages/react-native-executorch/common/rnexecutorch/tests/ImageProcessingTest.cpp:2:
/react-native-executorch/packages/react-native-executorch/common/rnexecutorch/tests/../data_processing/ImageProcessing.h:3:10: fatal error: 'executorch/extension/tensor/tensor.h' file not found
    3 | #include <executorch/extension/tensor/tensor.h>
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
make[2]: *** [CMakeFiles/RNExecutorchTests.dir/ImageProcessingTest.cpp.o] Error 1
make[1]: *** [CMakeFiles/RNExecutorchTests.dir/all] Error 2
make: *** [all] Error 2

I left one dummy test for this file and commented lines that adds the file to testing via cmake. If someone wants to play with it and potentially fix the issue just uncomment these commented lines in CMakeLists.txt in tests and give it a try ;)

@msluszniak msluszniak force-pushed the @ms/improve_image_processing_utils branch from 61c58dd to 41b915d Compare September 2, 2025 07:43
@mkopcins mkopcins changed the title Improve image processing utils chore: Improve image processing utils Sep 16, 2025
for (size_t i = 0; i < numChannels; ++i) {
backgroundScalar[i] = cvFloor(backgroundScalar[i]);
}
cv::Scalar backgroundScalar = computeBackgroundScalar(corners);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
cv::Scalar backgroundScalar = computeBackgroundScalar(corners);
const cv::Scalar backgroundScalar = computeBackgroundScalar(corners);

Comment on lines +25 to +29
auto imageAsMatrix = image_processing::readImageToMatrix(imageSource);
const auto tensorDims = getAllInputShapes()[0];
image_processing::adaptImageForTensor(tensorDims, imageAsMatrix);
auto inputTensor =
image_processing::readImageToTensor(imageSource, getAllInputShapes()[0])
.first;
image_processing::getTensorFromMatrix(tensorDims, imageAsMatrix);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not quite sure why we can't have a single function for this like we did before? Maybe restore readImageToTensor and place those four lines inside?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because original original scope of function readImageToTensor is not getting tensor from matrix, but also resizing, converting to accurate format etc. My intention by splitting this was to get more self-explanatory code. The second thing is that original function returns ugly std::pair and one value is often unused. Having code splitted internally inside image_processing::readImageToTensor might be a way to go

Comment on lines +37 to 39
for (int i = 0; i < pixelCount; ++i) {
int row = i / mat.cols;
int col = i % mat.cols;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
for (int i = 0; i < pixelCount; ++i) {
int row = i / mat.cols;
int col = i % mat.cols;
for (size_t i = 0; i < pixelCount; ++i) {
size_t row = i / mat.cols;
size_t col = i % mat.cols;

Comment on lines +61 to 64
for (int i = 0; i < pixelCount; ++i) {
const int row = i / matSize.width;
const int col = i % matSize.width;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
for (int i = 0; i < pixelCount; ++i) {
const int row = i / matSize.width;
const int col = i % matSize.width;
for (size_t i = 0; i < pixelCount; ++i) {
const size_t row = i / matSize.width;
const size_t col = i % matSize.width;

Copy link
Member Author

@msluszniak msluszniak Sep 17, 2025

Choose a reason for hiding this comment

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

Look that below I use cv::Mat::at(int x, int y) which will introduce implicit conversion from size_t to int:
image
I will think about changing it to size_t and static casting in cv::Mat::at to int

Comment on lines +228 to 230
constexpr int kMinCornerPatchSize = 1;
constexpr int kCornerPatchFractionSize = 30;
int cornerPatchSize =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
constexpr int kMinCornerPatchSize = 1;
constexpr int kCornerPatchFractionSize = 30;
int cornerPatchSize =
constexpr uint32_t kMinCornerPatchSize = 1;
constexpr uint32_t kCornerPatchFractionSize = 30;
uint32_t cornerPatchSize =

Comment on lines +93 to +112
static cv::Mat handleBase64Data(const std::string &imageURI) {
std::stringstream uriStream(imageURI);
std::string stringData;
std::size_t segmentIndex{0};
while (std::getline(uriStream, stringData, ',')) {
++segmentIndex;
}
if (segmentIndex != 1) {
throw std::runtime_error("Read image error: invalid base64 URI");
}
auto data = base64_decode(stringData);
cv::Mat encodedData(1, data.size(), CV_8UC1,
static_cast<void *>(data.data()));
return cv::imdecode(encodedData, cv::IMREAD_COLOR);
}

static cv::Mat handleLocalFile(const std::string &imageURI) {
auto url = ada::parse(imageURI);
return cv::imread(std::string{url->get_pathname()}, cv::IMREAD_COLOR);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: handle these via anonymous namespaces, not static functions

@msluszniak msluszniak self-assigned this Oct 6, 2025
@msluszniak msluszniak changed the title chore: Improve image processing utils Improve image processing utils Oct 6, 2025
@msluszniak msluszniak added chore PRs that are chores style improvement labels Oct 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore PRs that are chores style improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants