Make the diffPath optional

[serpent7776]

Images can be compared without saving the diff file.

Main.ml doesn't seem to have tests, so unsure if I should add any.

Fixes #64

[eWert-Online]

I would change this to be a match instead.

Suggested change

	if Option.is_some diffPath then
	IO1.saveImage diffOutput @@ Option.get diffPath;
	match diffPath with
	| None -> ()
	| Some path -> IO1.saveImage diffOutput path;

or even just an Option.iter:

Suggested change

	if Option.is_some diffPath then
	IO1.saveImage diffOutput @@ Option.get diffPath;
	diffPath |> Option.iter (IO1.saveImage diffOutput);

I am on my phone currently. So its not tested and formatting might be wrong.

[serpent7776]

I updated the code to use Option.iter.

[eWert-Online]

odiff.js should probably also be changed 🙂

[serpent7776]

I copied the binary manually to npm_package/raw_binaries/, and run npx odiff /path/to/pic1 /path/to/pic2 and I got expected output

Failure! Images are different.
Different pixels: 4030 (0.026666%)

so I'm not sure if odiff.js needs to be changed, but maybe I'm doing something wrong.

[eWert-Online]

At least the type definition in npm_package/odiff.d.ts would need to be changed, so the diffPath parameter is optional.

I guess @dmtrKovalenko can say more regarding changes to the odiff.js implementation.

[dmtrKovalenko]

Firstly thank you so much for your contribution. This is amazing!

Yes we definitely need to change the types of the odiff.d.ts file and regenerate readme.

Also it would be good to add e2e JavaScript test to cover this functionality and check that it actually works without passing file argument.

[serpent7776]

I guess I'd need some help with the odiff.d.ts change. I have no idea about typescript. I don't even know what's the purpose of this file :)

[dmtrKovalenko]

Yeah I can push something tomorrow

[serpent7776]

Thanks, I updated README. Let me know if it sounds OK.

[dmtrKovalenko]

Oh yeah and after thinking a bit about this: in case there is no output we should not actually write and output buffer so we can save performance a little bit.

[serpent7776]

I made the change to not write to the diffOutput image if output path is not specified.

I moved diffOutput image creation out of compare to main. I did it this way to avoid an extra bool parameter and also because outputDiffMask and diffOutput are closely related. This has caused some other changes, particularly is the tests.
Let me know if you like this change or would rather have some other approach to this.

The difference is quite noticeable. For an 1914x7896 image it's ~862.1 ms when writing output diff and ~340.1 ms without writing.

Also, as I understand there was an issue in the case of antialiasing = false, because diffOutput was shallow copy of img1. So whenever difOutput was modified, img1 was modified as well. Normally this doesn't matter, but for antialiased case it does, because we look back at the pixels we compared earlier. I added clone function to do a deep copy of the image data to fix the issue. This has caused Core antialiased tests to fail, so I refreshed expects, but now I see that the second test hasn't changed on the CI 🤷 .

There was also small issue in the TIFF tests.

[dmtrKovalenko]

Also, as I understand there was an issue in the case of antialiasing = false, because diffOutput was shallow copy of img1. So whenever difOutput was modified, img1 was modified as well. Normally this doesn't matter, but for antialiased case it does, because we look back at the pixels we compared earlier. I added clone function to do a deep copy of the image data to fix the issue. This has caused Core antialiased tests to fail, so I refreshed expects, but now I see that the second test hasn't changed on the CI 🤷 .

this is okay because the alghoritm of checking antialiased images again calculates the changed pixel so if we already changed the pixel to the 255 255 255 the chance of it being again 255 255 255 is very small and it doesn't really harm the logic

[serpent7776]

But I did notice difference is results when specifying output and when it wasn't specified:

$ ./_build/install/default/bin/ODiffBin  test/test-images/aa/antialiasing-on.png test/test-images/aa/antialiasing-off-small.png --antialiasing
Failure! Images are different.
Different pixels: 417 (1.042500%)

$ ./_build/install/default/bin/ODiffBin  test/test-images/aa/antialiasing-on.png test/test-images/aa/antialiasing-off-small.png --antialiasing /tmp/diff.png
Failure! Images are different.
Different pixels: 421 (1.052500%)

[serpent7776]

Oh, after clean rebuild the second test hasn't changed locally I reset expect for that test.

[eWert-Online]

I am against moving the diffOutput to the main function.

I am using odiff as a library in osnap, so i am using the diff function directly. With this change I would not be able to access the output anymore.

[serpent7776]

You are still able to access output in ?diffOutput parameter, but yes, it's a breaking change. Waiting for different design suggestion.

[dmtrKovalenko]

yes, we should keep sticking with a good useful design for the main function as I also want to publish this is a standalone OPAM package at some point. And yes keeping it useful for @eWert-Online is a top priority.

So my thought: what if we will make 2 top level wrapper functions like diff and diffWithoutOutput and will keep the api for diff the same but diffWithOutput will pass some kind of flag into the underlying function (yes boolean flag would be fine) and if the flag captureDiff is false we simply do not capturing the output

[dmtrKovalenko]

But I did notice difference is results when specifying output and when it wasn't specified:

Yes, I know this, but this is not a big deal in a real-world scenario, as the test is a rigorous edge case if we compare 2 images and we know exactly that one pixel was changed and it is not an antialiasing we can not be 100% that any other adjustment pixel around it is not anti-aliasing.

That's just my understanding of the anti-aliasing detection alghoritm. There might be some points I miss so would be good to see a real world image which makes the odiff to provide wrong results because of this optimization

[serpent7776]

OK, should I revert the changes related to anti-aliasing and image cloning?

[serpent7776]

I updated the code and introduced diffWithoutOutput. This is the same as diff, but without outputDiffMask and diffPixel, which doesn't make sense for this function.

I had to add Option.get to diff to match existing interface.

Let me know if it looks OK or you like some other approach more.

[serpent7776]

@dmtrKovalenko Have you had an occasion to review the updated changes?