rmtheis / tess-two

Fork of Tesseract Tools for Android
Apache License 2.0
3.76k stars 1.38k forks source link

WriteFile.writeBytes altering the source image endiannes and producing artefacts #228

Closed ratcashdev closed 5 years ago

ratcashdev commented 6 years ago

Warning: I am not sure if this is really a Leptonica problem, please help track id dow.

Summary: Trying to convert a PIX image to an openCV Mat image and back using the following code:

public static Mat pix2GrayscaleMat(Pix pix) {
        if (pix.getDepth() != 8)
            throw new IllegalArgumentException("Only 8 bpp images are supported.");
        return new MatOfByte(WriteFile.writeBytes8(pix)).reshape(0, pix.getHeight());
    }

public static Pix mat2Pix(Mat mat) {
        if (mat.type() != CvType.CV_8UC1 && mat.type() != CvType.CV_8UC3)
            throw new IllegalArgumentException("Unsupported Mat type: " + mat.type());
        final byte[] bytes = new byte[(int) mat.total()];
        mat.get(0, 0, bytes);
        return ReadFile.readBytes8(bytes, mat.width(), mat.height());
    }

Steps to reproduce the issue:

  1. load a bitmap, if necessary, convert to grayscale PIX, dump it to file (1_input_gray_pix.png)
  2. call pix2GrayscaleMat() above
  3. dump the resulting Mat object to file using Imgcodecs.imwrite("2_mat.png", mat);
  4. Convert the MAT back to PIX and save the image to a PNG file

Expected result: The three saved images will look exactly the same.

Actual result: the 2nd image (saved from the MAT Object) shows strange artefacts which I believe to be JPEG encoding artefacts. The PIX image saved as PNG is OK. The 3rd image - while converted in-memory from Mat to Pix, still contains the artefacts.

Tess-two version: 8.0

OpenCV version: 3.2.0

Android version: 7.0

Phone/device model: Galaxy S6

Phone/device architecture (armeabi, armeabi-v7a, x86, mips, arm64-v8a, x86_64, mips64): arm64-v8, but the JNI lib used is armeabi-v7a (since 8.0 does not offer 64bit for android)

1st PIX image saved as PNG: 1_input_gray_pix

2nd image - converted to MAT (make sure to zoom-in to see the artefacts) and saved as PNG: 2_mat

ratcashdev commented 6 years ago

More strangeness. Let's assume I have a 32bpp Bitmap. I convert it to Pix and save as a file;

final Pix pix = ReadFile.readBitmap(bitmap);
WriteFile.writeImpliedFormat(pix, new File(debugDir, "0_input_bitmap_asPix.png"));

At this stage, the PIX image written to the filesystem is a GRAYSCALE image, not a color one.

ratcashdev commented 6 years ago

The problem with readBitmap is in fact explained by readfile.ccp:

      pixel8 = (l_uint8)((r + g + b) / 3);
      // Set pixel to LUMA_8
      SET_DATA_BYTE(dst_line, x, pixel8);

Still this behaviour should be documented.

ratcashdev commented 6 years ago

OK i think I got it. The artefacts displayed are caused by wrong endiannes of the image caused by WriteFile.writeBitmap.

Inside it, it is calling https://github.com/rmtheis/tess-two/blob/master/tess-two/jni/com_googlecode_leptonica_android/writefile.cpp,

specifically the method Java_com_googlecode_leptonica_android_WriteFile_nativeWriteBitmap. This method on line 108 contains the following: pixEndianByteSwap(pixs); which is altering the source image. Obviously, using the source image after such an operation is not recommended. We should either convert back the endiannes at the end, or clearly state in the documentation that the PIX is altered, and should not be used afterwards.

PS: in the initial description the writeBitmap was not mentioned. It was just a debugging line in my own code that I used to see the alterations in real-time. It turns out this debugging line was introducing the unexpected results.

ratcashdev commented 6 years ago

One more comment related to the gray-scale conversion going on in readBitmap. Using the established calculation by ITU-R for Y component for the YUV color-space may be more appropriate: Y = 0.299 * R + 0.587 * G + 0.114 * B or Y = 0.22 × R + 0.72 × G + 0.06 × B Here's the same image converted to grayscale using the latter conversion above using GIMP (called luminance). Contrast is clearly better. luminance

rmtheis commented 6 years ago

Nice catch. Thank you for your thorough analysis and description of the problem.

I think the best solution here is to convert back the endianness on the source image at the end, as you have suggested. If you're inclined to submit a pull request to fix the issue, it would be welcome of course.

Thank you also for pointing out the issue with the gray-scale conversion in readBitmap. Should we consider that issue as a separate issue from #87, or is that the same issue?

ratcashdev commented 6 years ago

Instead of converting the endiannes twice (back and forth) I think it would be better to convert it only once, doing it on the memory section allocated for the destination bitmap (obviously after the bytes were copied from the PIX). This way we don't alter the original PIX whatsoever, and don't incur an additional delay.

ratcashdev commented 6 years ago

as for #87: in there the same issue is discovered (conversion to gray-scale) so let's deal with that there. However, that conversion does not really explain why the white turns to gray. Maybe also some endiannes issue there as well?

alexcohn commented 5 years ago

nativeWriteBitmap() changes the source pix data. I don't think it's a good design, but it also manifests in tests of WriteFileTest class. The easy fix for these tests,

private void testWriteBitmap(int width, int height) {
    Pix pix = TestUtils.createTestPix(width, height);
    Pix pcopy = pix.copy();                      // keep unchanged copy
    Bitmap bmp = WriteFile.writeBitmap(pix);     // pix changed

    assertNotNull(bmp);
    assertEquals(pix.getWidth(), bmp.getWidth());
    assertEquals(pix.getHeight(), bmp.getHeight());

    float match = TestUtils.compareImages(pcopy, bmp); // compare against the unchanged copy
    pix.recycle();
    bmp.recycle();

    assertTrue("Images do not match. match=" + match, (match >= 0.99f));
}
alexcohn commented 5 years ago

I have fixed this and similar ReadFile tests in my fork, which uses Tesseract 4.0. Should I create a special Pull Request for these changes, or maybe it would be wise to move the whole project to Tesseract 4.0 ?

alexcohn commented 5 years ago

I believe this issue can be closed now, except the endians swap in WriteFile.nativeWriteBitmap() is unexpected. It should at least be documented, but better make a change that will keep the Pix unchanged by writeBitmap().

rmtheis commented 5 years ago

I've added a comment to the Javadoc to point out the weird behavior--hopefully that will keep developers from having to go on a wild goose chase to track down the problem in the future.

Robyer commented 5 years ago

@rmtheis I think it's better to fix the behavior by converting the endianness twice. It doesn't break anything else and the slowdown shouldn't be big.

Also you might want to include fix for *bytes8 tests - https://github.com/adaptech-cz/Tesseract4Android/commit/b763242b31863c3c2cc323138162a734d2cab65d

You merged commit with change to read files not in grayscale but in rgba, and as a result these *bytes8 tests are broken now. You might not notice it when you use only the black/white test image, but if you use more colors, it will fail (actually also some jpeg tests will fail, because of the jpeg compression color artifacts).

alexcohn commented 5 years ago

Converting endianness again after write would be a breaking change; having no way to query of endianness on Pix, people had to assume the Pix was changed after write (on little-endian platforms).