image-js / image-js-typescript

Temporary repository to work on the migration of image-js to TypeScript
https://image-js.github.io/image-js-typescript/
MIT License
5 stars 5 forks source link

test: add testing cases for transform function #441

Closed EscapedGibbon closed 3 months ago

EscapedGibbon commented 4 months ago

close: https://github.com/image-js/image-js-typescript/issues/438

stropitek commented 4 months ago

Please document the opencv call that was used to generate the opencv images

stropitek commented 4 months ago

Why is this still in draft?

You can merge with main to get rid of the CI errors

targos commented 4 months ago

@EscapedGibbon What commands/actions did you do exactly to end up with these three new commits in your branch?

EscapedGibbon commented 4 months ago

@EscapedGibbon What commands/actions did you do exactly to end up with these three new commits in your branch?

git fetch+git rebase origin/main

EscapedGibbon commented 4 months ago

I can see what I did wrong, I didn't git push -f the commits. My bad. I can revert them though.

targos commented 4 months ago

Ok, now I suggest you use git merge origin/main and git push to avoid any issues. git rebase is a nice tool but shouldn't be used if it can be avoided.

EscapedGibbon commented 4 months ago

Ok, now I suggest you use git merge origin/main and git push to avoid any issues. git rebase is a nice tool but shouldn't be used if it can be avoided.

Ok, got it.

EscapedGibbon commented 4 months ago

The transform function doesn't work exactly the way it is supposed to, I think. Here is the transformation with fullImage:

let imageReflect = image.transform(
  [
    [1, 0, 0],
    [0, -1, 0],
  ],
  { inverse: false, fullImage: true },
);

and without fullImage:

let imageReflect = image.transform(
  [
    [1, 0, 0],
    [0, -1, image.height],
  ],
  { inverse: false, fullImage: false },
);

CleanShot 2024-04-17 at 13 49 27

EscapedGibbon commented 4 months ago

Same thing with scaling: CleanShot 2024-04-17 at 13 55 47

stropitek commented 4 months ago

Our test result is in line with my mental model of how transform works, and to make it work we should use height-1 in the matrix instead of height. But there is maybe something I am missing since opencv does not create the same result.

Can you use warpAffine instead of warpPerspective like in the translation test?

targos commented 4 months ago

As a reference, here are the Python scripts that were used to generate the test images: https://github.com/image-js/image-js/tree/master/test/python

EscapedGibbon commented 4 months ago

As was discussed, I looked at the border types and opencv parameters and they are normally the same as in image-js(borderType = "constant", borderValue=0,interpolationType=bilinear). The scaling test is passing now, but it passes only if I scale by 2 or by 4. In other cases the output differs from the opencv image. The one with affine transformation doesn't work as well, and it's not the issue of border or interpolation type.

stropitek commented 4 months ago

CleanShot 2024-04-22 at 14 48 51

Does the reporter report just the first mismatch in the image or all the wrong pixels?

In other words is the reported wrong pixel the only one that is wrong?

EscapedGibbon commented 4 months ago

CleanShot 2024-04-22 at 14 48 51

Does the reporter report just the first mismatch in the image or all the wrong pixels?

In other words is the reported wrong pixel the only one that is wrong?

No, it's not the only one. It's just the first one.

targos commented 3 months ago

@EscapedGibbon We looked into OpenCV's implementation of the bilinear interpolation yesterday (https://github.com/opencv/opencv/blob/4.x/modules/imgproc/src/opencl/warp_affine.cl#L123-L129) and it is actually not exactly correct, for performance reasons. So it's normal that we don't get the exact same results and I added a margin of error in https://github.com/image-js/image-js-typescript/pull/441/commits/b8b61ca40fc22f21cabc15bfa3b4eae34147a4aa