Skip to content

New feature: black and white color mode - #229

Open
toolicious wants to merge 1 commit into
pynicolas:mainfrom
toolicious:issue150-pr
Open

New feature: black and white color mode#229
toolicious wants to merge 1 commit into
pynicolas:mainfrom
toolicious:issue150-pr

Conversation

@toolicious

Copy link
Copy Markdown

Implements the black and white mode from #150. Measurements are in the issue comment.

Adds ColorMode.BLACK_AND_WHITE, selectable per page and as a default in the settings. The binarization runs on the output of the existing grayscale pipeline and adds a Sauvola threshold. PDF export embeds these pages as 1 bit per pixel with CCITT group 4, rebuilt from the original capture. Automatic detection never selects the new mode, page geometry and the other two modes are untouched, and there is no new dependency.

Three things you may want to decide differently:

  • Resolution for this mode is expressed in dpi (150 / 300 / 450) rather than as a pixel budget, because dots per inch is what decides whether 1 bit per pixel looks sharp. It reuses the physical page size the app already estimates for the PDF page box, and falls back to A4 when that estimate is not available. The highest setting interpolates beyond the capture resolution to get finer edges.
  • The name, "Black & white" next to "Grayscale".
  • I put the three new strings into all locale files, machine translated, the way the other string commits in the repo do it. Happy to drop those and leave you the base file only.

While measuring I also noticed two things unrelated to the feature: during export the page image was fetched twice per page, and the OCR bitmap was decoded even when no OCR language is enabled. Both are fixed in this branch, since they distorted my numbers. I can split them into a separate PR if you prefer.

Tested on a device and with ./gradlew clean license check assembleRelease.

@pynicolas

Copy link
Copy Markdown
Owner

Thanks a lot for your PR!
I will have a look at it but probably not in the next few days.

@toolicious

Copy link
Copy Markdown
Author

I just received an email with an error:

1 error
Android CI
This workflow run required approval but was not approved before it expired.

Looks like this has been pending for a month and expired now. Is this still on your radar?
For what it's worth: I've been using this feature many times now for my own purposes during that time and it works very well.

@pynicolas

Copy link
Copy Markdown
Owner

It is on my radar, but I didn't manage to work on it so far. I'm sorry for that.
I hope I manage to do that soon.

@pynicolas

Copy link
Copy Markdown
Owner

Thanks again for this pull request!
I can tell that you put quite some effort to come up with a good binarization and to stay true to the spirit of the app.
Nice job!

Here are my thoughts:

  • The binarization itself looks good, it goes much further than what I had tried myself before. However, I must say I haven't looked into it deeply yet. I noticed some small issues (like disappearing dots with an 8pt font). We may discuss this later.
  • CCITT G4 gives good results in terms of PDF size: around 40-80kB (vs 300kB for grayscale JPEG) for ExportQuality.BALANCED.
  • Performance is my biggest concern, especially at export time. So far, I tried to make the export almost instantaneous (<100ms) for ExportQuality.BALANCED. For BLACK_AND_WHITE, it takes more than 1 second per page on my device, because the full black-and-white processing is recomputed at export time. I guess I know why you did that: FairScan uses JPEG for its internal storage, and that would have introduced artifacts. Beyond that, the binarization itself takes about 800ms at capture time, which is significant.
  • DPI vs pixel budget: I don't see that as related to this PR, so I suggest we put it aside.
  • black_and_white_jpeg_hint: I understand the rationale but I prefer not to have it, I can explain.

About the "two things unrelated to the feature":

during export the page image was fetched twice per page

I don't see that. Did I miss something?

the OCR bitmap was decoded even when no OCR language is enabled

Yes, but it's minor (25ms per page), so we can handle it separately.

I started a new branch based on your code to:

  • Refactor internal storage to use PNG (8 bits per pixel) for BLACK_AND_WHITE
  • Revert other changes I would prefer not to take

It's not ready to be merged:

  • my refactoring definitely requires some tests and adjustments.
  • It seems to give slightly different results compared to what you did (everything looks slightly "bolder") and I don't know why.

Would you like to have a look at it and tell me whether this could be a good starting point for further discussion?

No rush. And sorry for the time it took me.
Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants