buddy-compiler / buddy-benchmark

Benchmark Framework for Buddy Projects
Apache License 2.0
46 stars 39 forks source link

[DIP] Add benchmark for Buddy & OpenCV resize2d operation. #61

Open taiqzheng opened 1 year ago

taiqzheng commented 1 year ago

The command line: ./image-processing-resize-benchmark <image path> <Scale option> <RowNum> <ColNum> <InterpolationOption>

An example: ./image-processing-resize-benchmark ../../benchmarks/ImageProcessing/Images/YuTu.png SCALE_FACTOR 0.33 0.35 NEAREST_NEIGHBOUR_INTERPOLATION

Problem: When using SCALE_FACTOR option, the output.size should be [input.row RowNum, input.col ColNum], but result shape ended in [input.row / RowNum, input.col / ColNum]. In the benchmark implementation, the factors(factorsOutputBuddyResize2D) were sent directly to the func below: dip::Resize2D(&input, dip::INTERPOLATION_TYPE::NEAREST_NEIGHBOUR_INTERPOLATION, factorsOutputBuddyResize2D) So, the problem was not caused by this implementation.

meshtag commented 1 year ago

When using SCALE_FACTOR option, the output.size should be [input.row RowNum, input.col ColNum], but result shape ended in [input.row / RowNum, input.col / ColNum].

I think the latter is the expected behaviour. The way scaling ratios are defined is

scaling_ratio = Input_Image_Dimension / Output_Image_Dimension

So one would expect

Output_Image_Dimension = Input_Image_Dimension / scaling_ratio

This can be referenced from this comment here.

taiqzheng commented 1 year ago

This benchmark will compare the resize operation for both Buddy & OpenCV implementation. Here is the problem, when using the SCALE_FACTOR option,Buddy used division operation;OpenCV used multiplication operation.

The command I use: ./image-processing-resize-benchmark ../../benchmarks/ImageProcessing/Images/YuTu.png SCALE_FACTOR 0.5 0.5 NEAREST_NEIGHBOUR_INTERPOLATION

Result for OpenCV, 513*513 pixels. OpenCVResize_0 5_0 5_NNI

Result for Buddy, 2052*2052 pixels. BuddyResize_0 5_0 5_NNI

taiqzheng commented 1 year ago

Another problem, when using SCALE_LENGTH option with the command like this: ./image-processing-resize-benchmark ../../benchmarks/ImageProcessing/Images/YuTu.png SCALE_LENGTH 400 300 NEAREST_NEIGHBOUR_INTERPOLATION OR like this: ./image-processing-resize-benchmark ../../benchmarks/ImageProcessing/Images/YuTu.png SCALE_LENGTH 400 300 BILINEAR_INTERPOLATION

Result for OpenCV contains 400 300 pixels, result for Buddy contains 300 400 pixels. The dimension order is incorrect when handling with SCALE_LENGTH option. I also tried the SCALE_FACTOR option, this one have the right dimension order with both interpolation option.

meshtag commented 1 year ago

Here is the problem, when using the SCALE_FACTOR option,Buddy used division operation;OpenCV used multiplication operation.

Result for OpenCV contains 400 300 pixels, result for Buddy contains 300 400 pixels.

Both of these things can be changed if you want it to become exactly like OpenCV. Shall I change them?

taiqzheng commented 1 year ago

Here is the problem, when using the SCALE_FACTOR option,Buddy used division operation;OpenCV used multiplication operation.

Result for OpenCV contains 400 300 pixels, result for Buddy contains 300 400 pixels.

Both of these things can be changed if you want it to become exactly like OpenCV. Shall I change them?

I highly recommend to modify this part, thanks a lot! This will make the benchmark design more concise and understandable without ambiguity.

taiqzheng commented 1 year ago

Will delete the OpenCV enum class after exams(before the weekend).

meshtag commented 1 year ago

Please mark the PR as "Ready for review" once you think it is ready.

meshtag commented 1 year ago

I think it makes sense to benchmark this function directly instead of the user interface itself.

The user interface is just a single layer of syntactic sugar which makes it easier for users to utlize the underlying implementation without worrying about the output. It also involves costly "pass by value" situations which are very bad from the performance perspective. I wish to highlight the fact that this is not inherently required by the implementation itself, but is just added for more user convenience.

Same goes for #63 as well.

What is your opinion on this? (/cc @zhanghb97 @FlagerLee )