some bugs in sane.py 2.81 (bitdepth-1 and others)

[jaka384]

In the past days I have been using this sane.py to connect to my scanner and did find some "quirks" (while learning a lot). See the list below. Except for point "f)", I think I have also found the reason and/or a solution. Could you please verify my comments, and make repairs where you think it is necessary. (running python 2.7.6, on ubuntu 14.04/ linux 3.13.0-58 , 64 bit, but I don't think this matters much).

To summarize: bitdepth-1 images are garbled and black/white reversed. While trying to resolve these, I met some other, minor, inconveniences. I start with those, the easy ones.

Thanks.

= = = = = =

sane.py refers to version = '2.8.1' author = ['Andrew Kuchling', 'Ralph Heinkel', 'Sandro Mani']

a) in snap() the error Document feeder out of docs... will never be risen in sane.py line 125 : if e == 'Document feeder out of documents': should be: if str(e) == 'Document feeder out of documents':

b) in Option.init() line 65: self.pyname = self.name.replace("-", "") somewhere else a similiar replacement is necessary, as the sanedev getoptions() still shows the "-" i.e. minus signs e.g. tl-x, tl-y, which causes confusion. The replacement of minus by underscore should be mentioned in the documentation, as it is confusing when the same scanner is approched by different libsane frontends

c) in _sane.c line 488: imgBuf[imgBufOffset + i] = lineBuf[i / 8] & (0x80 >> (i % 8)) ? 0 : 255; the Sane spec (version 1.05, end of section 3.2.1) states that for 1-bit images zero is white and one is black, while for other (gray, R, G, B) zero is darkest and one 1 brightest should be: imgBuf[imgBufOffset + i] = lineBuf[i / 8] & (0x80 >> (i % 8)) ? 255 : 0;

d) in section 4.3.8 of the sane spec (v1.05) it is stated that the number of bytes per line should be calculated differently in case if bit-depth 1. It should be rounded up to nearest 8-fold. byter per line = nrchannles * ( (nrpixelsperline + 7) / 8 ) if bitdepth == 1 this influences _sane.c line 428: int imgBytesPerLine = imgPixelsPerLine * imgSampelsPerPixel * imgSampleSize; which is only correct for bitdepths > 1 (I do not know whether bitdepths 2 through 7 can be found "in the wild".) Bitdepth-1 images are returned by sane packed 8 pixels in a byte, padded to the next byte boundary. _sane.c returns bitdepth-1 images (PIL(LOW) style) as byte per pixel, which conversion, IMHO, should be a separate step or option. In fact the bytes_per_line returned by the get_params() should be used and checked against one own calculation, as sane backends may return more bytes_per_line than the minimum required amount.

e) in -sane.c lines 411,412: int allow16bitsamples = 0; if(!PyArg_ParseTuple(args, "|ii", &noCancel, &allow16bitsamples)) This "allow16bitsamples" option is not in the documentation (or is it not meant to be a documented feature?)

f) the number of bytes read, as returnred by get_params() is not equal to the size of the PIL.Image, even if mode = gray. in _sane.c nRead and lineBufUsed seem - to me - not to be checked against a get_params immediately after the scan. I could not find the reason for this, even though I expect that an integer division where a float division is needed might be the cause.

[manisandro]

Hello and many thanks for the code review:

a) Yes indeed b) I'm personally unaware of the reasons behind the decision to replace dashes with underscores (that part of the code pre-dates my involvement here), I can only assume user friendliness, but is it really more user friendly, since as you mention other front-ends can behave differently? Perhaps it would be best to just drop that replacement. c) Yes true d) Do you by any chance have a device which supports 1bit scanning? If so I'd appreciate if you could fix the code and test it. e) This is used in the arr_snap method in sane.py. It is not meant to be exposed in the public API f) Could specify with which mode and depth this is incorrect? With my basic scanner at my disposal currently I cannot reproduce the issue

Thanks, Sandro

[jaka384]

On 07/28/2015 12:23 AM, Sandro Mani wrote:

Hello and many thanks for the code review:

a) Yes indeed

b) I'm personally unaware of the reasons behind the decision to replace dashes with underscores (that part of the code pre-dates my involvement here), I can only assume user friendliness, but is it really more user friendly, since as you mention other front-ends can behave differently? Perhaps it would be best to just drop that replacement. Well,it was convenient, once I found out how it worked. In python the 'tl-x' attribute of my scanner (for top-left x offset) was an unfortunate name from the manufacturer, as it is tl minus x when used as an attribute name. But if "-" is replaced by "_" in Option.init, something similar should be done in SaneDev.getoptions(), and it should be documented.

c) Yes true

d) Do you by any chance have a device which supports 1bit scanning? If so I'd appreciate if you could fix the code and test it. Yes, of course, otherwise I probably would never have noticed the 1-bit behaviour. And I am happy to test your software, but I am not able to write it. It is many years ago (> 15) that I have written C, and I would probably make a mess of it. I remember enough of C, however, to understand or guess most of your source.

The key point is (from sane spec) that the result of sane_read() (a buffer and length) should be combined with a get_params() immediately after the sane_read. For 1-bit both the pixels per line and the bytes per line, as returned by get_params() are needed, in order to read the buf linewise and unpack the bits. In the current _sane.c the number of bytes per line is calculated, with a "formula" that is only valid for >1 bit depth.

e) This is used in the arr_snap method in sane.py. It is not meant to be exposed in the public API

OK

f) Could specify with which mode and depth this is incorrect? With my

basic scanner at my disposal currently I cannot reproduce the issue

I will make a demo with some output from get_params() and from PIL.image attributes, but it will be tomorrow or even the day after. (I am tired now). For this moment I can say that get_params (gray, lastframe 1, pixperline 3398, nrlines 5598, depth 8, bytesperline 3398 )

gives a PIL image (after .snap()) of width: 3296, height: 5505, samples: 1, samplesize: 1, and a buffer length of 18144480. The scan was at 400 dpi, so its needs a bit fiddling to convert to both inches and millimeters. the area is ((0,0),(215.899, 355.599)) ( the limits of the glass plate scanner).

Thanks, Sandro

— Reply to this email directly or view it on GitHub https://github.com/python-pillow/Sane/issues/13#issuecomment-125365670.

[manisandro]

Ok I'll try to look into the 1bit image issue this weekend - if I make the changes in a branch are you able to build from that and test the changes? And yes, an example for point f) would be appreciated.

[jaka384]

On 07/29/2015 11:29 PM, Sandro Mani wrote:

Ok I'll try to look into the 1bit image issue this weekend - if I make the changes in a branch are you able to build from that and test the changes? Yes, I think so, if it is outside of the github infrastructure. If you send me an updated _sane.c (and possibly an updated sane.py) I can do the setup.py install. I tried it by modifying _sane.c, which worked.

And yes, an example for point f) would be appreciated. It will become weekend before I get to that. Have to clean up my python script, so that outsiders will understand it.

— Reply to this email directly or view it on GitHub https://github.com/python-pillow/Sane/issues/13#issuecomment-126100737.

[jaka384]

First (blush) I have to mention that one of the earlier corrections I proposed was incorrect. The reversal of "0" and "255" was probably correct as it was a week ago or so.

And, restudying the line of C code (line 490 and following of _sane.c, as it was before you modification of last week): if(p.depth == 1) { for(i = 0; i < imgBytesPerLine; ++i) imgBuf[imgBufOffset + i] = lineBuf[i / 8] & (0x80 >> (i % 8)) ? 0 : 255; {

the &-ing of two bits requires that the Linebuf bit was 1 to be true, and that is converted to a \000 byte (black) for grayscale.

So, I mixed things up, probably because I did see a garbled mostly black display of a scan for which the original was predominatnly white.

Later today I will write you about the other issues, but confessing my above error should not wait.

In the meantime, I can report about one experiment that can help you finding the error in _sane.c if I multiply "_8" in the two lines marked /_JAKA384*/, in lines 487 and following

  int imgBufOffset = imgBufCurLine * imgBytesPerLine *8; /*JAKA384*/
  /* Copy data to image buffer */
  if(p.format == SANE_FRAME_GRAY || p.format == SANE_FRAME_RGB)
    {
      if(p.depth == 1)
        {
          for(i = 0; i < imgBytesPerLine*8; ++i) /*JAKA384*/
            imgBuf[imgBufOffset + i] = (lineBuf[i / 8] & (0x80 >> (i

% 8))) ? 0 : 255;

then the displayed results become MUCH better for 1-bit depth-only (!), but it is not enough, as the image does not have enough scanlines (is cut off). So there must be another dependency that bytes-per-line and pixels-per-line are different for depth-1 images...

One remark: it all started a few weeks ago, when I found that scanning with Xsane sometimes is slow, sometimes fast. With Xsane you don't know what's happening under the hood. With sane.py I wanted to try and time different scanning modes and resolutions. The reason that i want to use the depth-1 setting, is that I hope it will be fast at 600 dpi. Regards, Jan

On 07/29/2015 11:29 PM, Sandro Mani wrote:

Ok I'll try to look into the 1bit image issue this weekend - if I make the changes in a branch are you able to build from that and test the changes? And yes, an example for point f) would be appreciated.

— Reply to this email directly or view it on GitHub https://github.com/python-pillow/Sane/issues/13#issuecomment-126100737.

[jaka384]

After reading SaneDev_snap in _sane.c yet one more time, I start to see what is the underlying problem, even though I do not have a fix:

there is only one variable (imgByterPerLine) for the number of bytes per scan line, when two slighty different ones are need: the imgBytesPerlIne is the "PIL-side" view of the image, which is different from the "sane-side" view in case of depth-1.

Everything below is only valid for for depth-1 saneBytesPerline = (imgBytesPerLine +7) / 8 [integer division] and may include some padding bits.

In the current _sane.c imgBytesPerLine is approx 8 times larger that the sane-side number and "/* Read one line */" [at line 450] reads 8 lines at a time (except for the last ones if height is not an 8-fold.)

and "/* Copy data to image buffer */" [line 488 and following] process 8 scan lines at a time (however ignoring padding), but imgBuffOffset is only incremented for 1 line at a time. So, 7 lines out of 8 are overwritten, only every 8-th is is displayed.

That's it for the moment. Time for a break and some fresh air.

[manisandro]

Reading the spec in detail, the error is the following: the current _sane.c code incorrectly assumes that for RGB 1bit images, the RGB bits are stored interleaved, one sample after the other, i.e.

rgbrgbrg brgbrgbr gbrgbrgb ...
byte1    byte2    byte3    ...

But section 3.2.1 states: For a bit depth of 1, each byte contains 8 sample values of a single channel. In other words, a bit depth 1 frame is transmitted in a byte interleaved fashion. This means that i.e. a line of 12 RGB pixels in depth-1 format will look like:

rrrrrrrr gggggggg bbbbbbbb 000000rr 000000gg 000000bb
byte1    byte2    byte3    byte4    byte5    byte6

Working on a fix.

[manisandro]

Please test the following branch:

https://github.com/python-pillow/Sane/tree/depth1fix

[jaka384]

update: the 3 MB attachment (pysaneimgGRAY.png) was too large for Github, the mail bounced. Therefore I replaced it with a file (pysaneimgGRAYtoBW.tif) that I manually converted by Gimp grom GRAY to BW and compressed it with CCITT-G4. I is small enough (now you understand why I like depth-1 images.) It can be viewed by ImageMagick.

Jan

-------- Forwarded Message -------- Subject: Re: [Sane] some bugs in sane.py 2.81 (bitdepth-1 and others) (#13) Date: Mon, 03 Aug 2015 10:23:48 +0200 From: Jan Kardaun kardaun@archa.nl To: python-pillow/Sane reply@reply.github.com

I tested it, results are better but not OK yet.

Resulting B/W scan is pysaneimg000.png, comapare with the p*GRAY.png for gray mode scanning. [In future I will not send again a gray mode scan for comparison, future B/W scans will have similar originals - random page of _sane.c listing) ]

pysane.log is the progress/info of the BW scan, to be compared with the *gray.log. it should be reasonably self-explanatory.

I will have a look at your modifications in _sane.c, but that needs time and concentration, likely this evening.

Jan

On 08/02/2015 01:14 AM, Sandro Mani wrote:

Please test the following branch:

https://github.com/python-pillow/Sane/tree/depth1fix

— Reply to this email directly or view it on GitHub https://github.com/python-pillow/Sane/issues/13#issuecomment-126960968.

[jaka384]

Sandro,

After some more experimenting:

The imgBytesPerLine is used for both the input(=sane-side) and the output=(PIL-side) line. [This works when 1 byte = 1 sample (and for depth 16 ithe different size is handled correctly.) ]

In case of depth-1 they are not the same; so one need to malloc() two different sizes for the input and the output line. The current _sane.c does not segfault only because if N bytes are requested from sane_read() for N pixels (resulting in reading 8 scanlines), only N bits are copied to the imgBuf. Your latest change was an improvement (because if pays attention to the padding bits), but still only 1 out of every 8 scanlines is copied to imgBuf.

Comment: in practice I have never seen a depth-1 RGB scan (but this depends on the scanner model and driver). So, it is better to make _sane.c handle this, but I cannot test it.

Jan

On 08/02/2015 01:14 AM, Sandro Mani wrote:

Please test the following branch:

https://github.com/python-pillow/Sane/tree/depth1fix

— Reply to this email directly or view it on GitHub https://github.com/python-pillow/Sane/issues/13#issuecomment-126960968.

[manisandro]

Hi Jan,

Correct, I called it a day (night) too early and completely forgot to check that sane_read actually reads data correctly in the first place... I've added one more commit which should fix this: f425e5251

Thanks again for your efforts.

[jaka384]

Sandro,

"Close, but no cigar". (For the origin of this expression see Wikipedia).

We're almost there:

1) the leftmost pixel is the MSB bit (see SANE spec, 3.2.1). Easiest fix is to reorder your bitMasks and count down from 128, 64,...,1.

2) as I wrote some days ago, one of my initial requests was wrong. The "old" mapping of bits to 0 and 255 was right. it must be :if( imagebyte & bitmask : 0 : 255) (occurs twice in the program).

Later, as I have some other things to do, I will return with the question of different image sizes in get_params() and by PIL.

And, after having spent this time on understanding how Sane_Dev.snap() works, I wondered whether there should be a core function (let's call it Sane_Dev.readPage() and/or Sane_Dev.readLine() that handles reading from the backend and returns the data from sane_read() and get_params(). Then do the rescaling and reordering in a separate C-(or python?)function. 'Cause now snap() behaves like the greek mythological Procrustes: it chops off 16 bits samples (to 8) and stretches 1-bit samples (to 8). Especially, numpy knows bit-arrays, but with arr_snap, one cannot use this.

But everything in due time.

Thanks for your help so far,

Jan

On 08/04/2015 12:06 AM, Sandro Mani wrote:

Hi Jan,

Correct, I called it a day (night) too early and completely forgot to check that |sane_read| actually reads data correctly in the first place... I've added one more commit which should fix this: f425e52 https://github.com/python-pillow/Sane/commit/f425e52519798552fcf7442fef95653073b34144

Thanks again for your efforts.

— Reply to this email directly or view it on GitHub https://github.com/python-pillow/Sane/issues/13#issuecomment-127418554.

[manisandro]

Hi Jan

The two issues you pointed out are fixed (still in the depth1fix branch).

As for reorganizing the code, I'm open to suggestions, but I'd keep it in C since there is much less overhead in doing low-level manipulation there.

What layout should the bits in a numpy array have to be most useful?

[jaka384]

After this mail I downloaded the depth-1 branch fully, did setyp.py and it worked.

I scanned e.g. a 10x15cm (approx, aimed at 4 x 6 inches) part and it gives the image and progress file in the attachments. (I hope I did not send the wrong attachments)

1) depth-1 scanning works 2) there are still differences between the width/height of params() and PILL. These are also present in depth-8 mode. See the log file. Once more: after each scan one should ask get_params() to see what happens, and _sane.c only checks the SANE_STATUS.

I goofed with bitmaps in numpy: numpy-booleans use one byte, not one bit. The computer-buddy whom I would like to consult about this is on holiday.

Perhaps you could try to check the get_params() with the PILL size using your own scanner. That would indicate whether it is something inherently in _sane.c, or just happens with my scanner/driver combo.

Greeetings, Jan

On 08/04/2015 05:53 PM, Sandro Mani wrote:

Hi Jan

The two issues you pointed out are fixed (still in the depth1fix branch).

As for reorganizing the code, I'm open to suggestions, but I'd keep it in C since there is much less overhead in doing low-level manipulation there.

What layout should the bits in a numpy array have to be most useful?

— Reply to this email directly or view it on GitHub https://github.com/python-pillow/Sane/issues/13#issuecomment-127656926.

[manisandro]

Ok cool.

As for the params inconsistency:

Firstly, you'll need to send me the attachments directly (manisandro at gmail dot com) as they are lost when replying to github messages.

Regarding the issue: width (aka pixels per line) is taken directly from the struct populated by get_parameters, so it really should be consistent. The height is dynamically determined, depending on when sane_read returns EOF (indeed, some scanners might not even report the height in get_parameters). Perhaps the attachments you intended to send provided this, but I'd really need an example of such a misbehaving case. You might also want to check whether the values width and height returned by dev.snap (and used in snap() in sane.py) are also wrong or whether it is merely the returned PIL image reporting a different dimension.

[manisandro]

Issues mentioned here have since been fixed in version 2.8.2.