opencv4nodejs icon indicating copy to clipboard operation
opencv4nodejs copied to clipboard

The `.imwrite` method does not respect the `params` flags.

Open issuefiler opened this issue 2 years ago • 6 comments

Probably bug

This is driving me crazy. I couldn’t even locate where it went wrong.

The imwrite method’s supposed to spit out a 1-bit binary PNG image file (PNG supports the bit depth of 1.) if the IMWRITE_PNG_BILEVEL flag is set, but it keeps generating 8-bit images.

                    if( params[i] == IMWRITE_PNG_BILEVEL )
                    {
                        isBilevel = params[i+1] != 0;
                    }
                    png_set_IHDR( png_ptr, info_ptr, width, height, depth == CV_8U ? isBilevel?1:8 : 16,

The minimum proof-of-concept

import OpenCV from "@u4/opencv4nodejs";

OpenCV.imwrite(
	"why.png",
	new OpenCV.Mat(0x100, 0x100, OpenCV.CV_8UC1),
	[OpenCV.IMWRITE_PNG_BILEVEL, 1]
);

Could anyone please help me find what’s so wrong about this piece of code?

issuefiler avatar Nov 19 '22 07:11 issuefiler

Environment

I’m using a 64-bit Intel CPU, Windows 10, OpenCV 4.6.0 & @u4/opencv4nodejs 6.2.5.

The OpenCV was built with this command:

npx build-opencv rebuild
	--version 4.6.0
	--cuda
	--nocontrib
	--nobuild
	--binDir "opencv-4.6.0/build/x64/vc15/bin/"
	--libDir "opencv-4.6.0/build/x64/vc15/lib/"
	--incDir "opencv-4.6.0/build/include/"
	--keepsources

issuefiler avatar Nov 19 '22 07:11 issuefiler

import FileSystem from "node:fs/promises";
import OpenCV from "@u4/opencv4nodejs";

const imencode_image_data = await OpenCV.imencodeAsync(
	".png",
	new OpenCV.Mat(0x100, 0x100, OpenCV.CV_8UC1),
	[OpenCV.IMWRITE_PNG_BILEVEL, 1]
);
await FileSystem.writeFile("imencode.png", imencode_image_data);

await OpenCV.imwriteAsync(
	"imwrite.png",
	new OpenCV.Mat(0x100, 0x100, OpenCV.CV_8UC1),
	[OpenCV.IMWRITE_PNG_BILEVEL, 1]
);

process.exit();

This bug only happens with the OpenCV.imwrite method. OpenCV.imencode respects the OpenCV.IMWRITE_PNG_BILEVEL flag. There might be something wrong with how OpenCV::imwrite was bound.

Results comparison: .imwrite and .imencode.

issuefiler avatar Nov 19 '22 19:11 issuefiler

Bug

I’ve found what is wrong. Compare the two:

https://github.com/UrielCh/opencv4nodejs/blob/66d7d42abb17ae5c5f8b5cfe6bf7cb5409baf24a/cc/io/ioBindings.h#L56-L71

cv::imencode(ext, img, dataVec, flags);

and

https://github.com/UrielCh/opencv4nodejs/blob/66d7d42abb17ae5c5f8b5cfe6bf7cb5409baf24a/cc/io/ioBindings.h#L35-L44

cv::imwrite(path, img);

.

When the .imwrite binding method calls cv::imwrite, it does not supply the flags variable for the params argument (cv::imwrite(path, img);).

issuefiler avatar Nov 19 '22 20:11 issuefiler

Fix suggestion

This line

https://github.com/UrielCh/opencv4nodejs/blob/66d7d42abb17ae5c5f8b5cfe6bf7cb5409baf24a/cc/io/ioBindings.h#L42

should be

      cv::imwrite(path, img, flags);

.

issuefiler avatar Nov 19 '22 20:11 issuefiler

Look's correct; one more deep breath, and you can open your first PR.

Spooler alert: To be merged, a PR must contain a test function covering the new code (should be eight more lines)

👍

UrielCh avatar Nov 21 '22 05:11 UrielCh

Extra note, small PR, are much more appreciated than bigger ones, so put each fix in a different PR.

You can create multiple branches on your fork to keep it clear.

I'll do my best to merge them quickly.

UrielCh avatar Nov 21 '22 05:11 UrielCh