-
Notifications
You must be signed in to change notification settings - Fork 5
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.)
## Standard parameters | ||
|
||
There is a number of parameters common to all processors that MUST be supported by processors. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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".
### `dpi` | ||
|
||
Custom DPI to assume for pixel density of images. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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).
MUST default to 300. | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
### `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` | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
…put-level discrepancy
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.