search

DanBloomberg / leptonica

Leptonica is an open source library containing software that is broadly useful for image processing and image analysis applications. The official github repository for Leptonica is: danbloomberg/leptonica. See leptonica.org for more documentation.
Other
1.74k stars 387 forks source link

24bpp PIX to BMP would fail - fixes #684 #685

Closed GerHobbelt closed 1 year ago

GerHobbelt commented 1 year ago

fixes #684

😞 I now see indentation is a wreck (editor set to TABS here, sorry)

DanBloomberg commented 1 year ago

Thanks!

Does this really fix the windows issue with the test code that I provided for 4 component bmp image read/write? The puzzle for me is that it works on linux but gets the color components borked on windows.

Note that leptonica uses 4 space indent without tabs.

GerHobbelt commented 1 year ago

Does this really fix the windows issue with the test code that I provided for 4 component bmp image read/write? The puzzle for me is that it works on linux but gets the color components borked on windows.

Nope. (I take it you mean #675?)

summary

DanBloomberg commented 1 year ago

You have done a huge amount of sleuthing, and I'm amazed that you were able to do it with so little contact with leptonica!!

The PR #684 for converting 24 to 32 is really a belt-&-suspenders thing, because with one tiny exception, there should be no 24-bit pix in leptonica. All RGB pix are 32 bpp. See the comments in pixConvert24To32() for the extremely limited use of 24-bit pix. You closed 684, but I'll put that check in anyway.

As for #675, are you saying that you are not getting any error in Windows, and that sinall is the only one seeing this? If so, we can end that part of the investigation, because the code works on linux and in any event, problems with components getting shifted around should not occur on just windows. Low-level pixel stuff should be platform independent.

And as for the postulated problem with pixConvert24To32(), I will check that out. It is not covered in any of the 146 regression tests, and is barely used in the library, so it could be buggy.

GerHobbelt commented 1 year ago

Thanks for the compliment. :-) I still only somewhat grok the periphery; the important stuff, where the math becomes important, is a real struggle still -- not your code, mind you, just me hitting my own math limitations (edu + grasp) pretty hard. :'-(

example: i don't get why the counts in the histograms are multiplied by their index (= grayscale tone 0..255) when producing the sum. this is done when looking for a threshold in there (double hump in histogram -> seek valley in between). :-( that multiply feels like a triangle of weighting which I don't see the benefit or use for yet; meanwhile I'm scanning otsu and related papers to see if somebody says something in there that makes my brain +click+ but thus far: no luck. alas, maybe today. one can hope. :-)

re 684: I was fast on the trigger as I assumed you'ld pick up 685 (the fix) from the noises you made. :-)

(and, yes, I gathered that 24bpp is an oddity -- 4 bytes per pixel makes a lot of sense hardware wise, while 3 bytes per px is just a lot of old skool byte shuffling -- but I found convertTo24 in all headers while hunting ways to make 675 happen, and then it helps when write\io interfaces are orthogonal (if that's the right word for this): accept anything, either directly or by implicit conversion, only reject the obviously wrong. Your library behaviour is like that, which makes it relatively easy to go through (few exceptions to keep in mind) and use. I just grabbed what I found, than hit a snag, 's all.

re 675: yes. I cannot reproduce, nor reason through code (leptonica & his + data images provided) why it might happen. sinall is the only one. alas.

(AFK, responding via mobi. apologies for degraded msg fmt)

On Wed, Mar 22, 2023, 02:07 Dan Bloomberg @.***> wrote:

You have done a huge amount of sleuthing, and I'm amazed that you were able to do it with so little contact with leptonica!!

The PR #684 https://github.com/DanBloomberg/leptonica/issues/684 for converting 24 to 32 is really a belt-&-suspenders thing, because with one tiny exception, there should be no 24-bit pix in leptonica. All RGB pix are 32 bpp. See the comments in pixConvert24To32() for the extremely limited use of 24-bit pix. You closed 684, but I'll put that check in anyway.

As for #675 https://github.com/DanBloomberg/leptonica/issues/675, are you saying that you are not getting any error in Windows, and that sinall is the only one seeing this? If so, we can end that part of the investigation, because the code works on linux and in any event, problems with components getting shifted around should not occur on just windows. Low-level pixel stuff should be platform independent.

And as for the postulated problem with pixConvert24To32(), I will check that out. It is not covered in any of the 146 regression tests, and is barely used in the library, so it could be buggy.

— Reply to this email directly, view it on GitHub https://github.com/DanBloomberg/leptonica/pull/685#issuecomment-1478783241, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADCIHTYHXKAI3HCE2ZNNHLW5JGFJANCNFSM6AAAAAAWCM7KTU . You are receiving this because you authored the thread.Message ID: @.***>

DanBloomberg commented 1 year ago

OK, at this time I can assume that the BMP code is working properly for 4 spp on all platforms.

Small amount of background on this. I implemented the BMP I/O shims years ago because people needed it, but I did so with minimal features. For example, I didn't even allow reading of RGBA. My mantra was: if you want to use the transparency component, use PNG. Then about 6 months ago people started complaining that it didn't handle RGBA that they were getting from screen grabs. I still didn't want to mess with it, so just checked for 4 spp and returned with an error message. Then someone sent some code (isn't github great!) that worked for reading a BMP RGBA file and successfully tossed out the A layer. At that point I gave in, and implemented both reading and writing RGBA with BMP, including keeping the transparency. (I included writing both for completeness and to check that the reading was correct. As you saw in the code snippet (and you did this as well with your testing), the files were read and written several times, and also with write/read in PNG to make sure the images were all the same.

But I only tested on linux; hence the concern with issue #675. Will leave this open in case sinall reports what he is seeing in detail (as requested).

As for Otsu thresholding, it's commonly used but not very useful because it is a global threshold. I made some modifications to make it work a little better, but it's still very poor. leptonica has adaptive thresholding for doing a decent job on grayscale images where there is a large variation in background intensity. You can see examples of this by running: adaptnorm_reg display

DanBloomberg commented 1 year ago

This PR is no longer relevant.

I want to thank Ger again for taking the issue seriously, and also for providing a very useful modification to leptonica error handling -- allowing a second string in the error message, most useful when there is a failure to read a file.