Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

define Standard Parameters for dpi, input-level, output-level #136

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kba
Copy link
Member

@kba kba commented Jan 3, 2020

Basis for #134 and OCR-D/core#376

Will require a new major version and some seriously frantic pull requesting to all the processors but it's worth it for sustainability IMHO.

Copy link
Collaborator

@bertsky bertsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will require a new major version and some seriously frantic pull requesting to all the processors but it's worth it for sustainability IMHO.

Let's please discuss use-cases for standard parameters and implementation mechanisms first (cf. comments).

Also, I think most other spec issues should have priority over this. As should core issues like OCR-D/core#342 OCR-D/core#330 OCR-D/core#319 OCR-D/core#306 over forcing processors into adoption. (Correctness is a safer way to sustainability than uniformity IMHO.)

Comment on lines +13 to +15
## Standard parameters

There is a number of parameters common to all processors that MUST be supported by processors.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the idea was actually to provide extra parameters automatically to all processors (without any need for specification/validation and implementation in the individual processors). So this would have to enter the spec at CLI.md, not here – right?

And besides using extra parameters, I have proposed other mechanisms (like extra CLI options, and environment variables), too. Shouldn't we at least discuss these options first?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, the point here was to have a place for that discussion. I wrote down what came to mind at the place where it fitted best. I wouldn't want to necessarily force everyone to implement a dpi parameter they do not need (core might help here though). Could be overrideable via env var, getopt-style extension etc. We just need a solution soon and a PR is more urgent than issues.

So this would have to enter the spec at CLI.md, not here – right?

Again, I'm open for changing the details, as long as there is one place with a fragid #standard-parameters that describes these "special" "parameters"/"flags"/"settings".

Comment on lines +17 to +19
### `dpi`

Custom DPI to assume for pixel density of images.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have already proposed a better solution for DPI overrides: by static annotation in the respective PAGE-XML attributes. OcrdExif would then have to prefer that over the binary meta-data if available. And dedicated processors could estimate DPI from the image, or allow user overrides. I thought you were going along with that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do. I'd still offer it as a standard parameter with parameter > annotation > EXIF metadata.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Makes sense (as long as we make sure these standard parameters are really automatic for tools).

Comment on lines +21 to +22
MUST default to 300.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A default here? If you don't make this optional, then trusting the binary image meta-data density value becomes impossible.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. But we need a fallback somewhere to activate if

  • no param provided
  • no annotation in PAGE
  • none or garbage EXIF data

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. The only place I have seen in the spec for that was your recent addition to mets.md:

However, since technical metadata about pixel density is so often lost in
conversion or inaccurate, processors should assume 300 ppi for images with
missing or suspiciously low pixel density metadata.

In practise, this is all just a contract around behaviour in core's OcrdExif (or whatever that will be named after we respect the PAGE-XML overrides).

ocrd_tool.md Outdated
Comment on lines 23 to 36
### `input-level`

On what level of typography should input images be processed?

Processors MAY define a `default` value.

`enum` MUST be a list of one or more of:

* `page`
* `block`
* `line`
* `word`
* `glyph`

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO we should only have output-level. See arguments here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the section for input-level here and added an example what behavior should be like if the expected input-level isn't provided.

It's probably more productive to continue that discussion on input/output-level in #134. I've assgned it to the reviewers here, so let's settle that first and then I'll update the PR.

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