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

image API: respect order of derived image features #524

Closed
bertsky opened this issue Jun 22, 2020 · 3 comments · Fixed by #525
Closed

image API: respect order of derived image features #524

bertsky opened this issue Jun 22, 2020 · 3 comments · Fixed by #525
Assignees
Labels

Comments

@bertsky
Copy link
Collaborator

bertsky commented Jun 22, 2020

The functions Workspace.image_from_page and image_from_segment must crop according to Page/Border/Coords or TextRegion/Coords, and they must rotate according to Page/@orientation or TextRegion/@orientation.

But what if the segment has both?

  1. crop-first:
    • find the bbox of the border polygon, mask the parts outside of the polygon with background, and crop to that bbox
    • then rotate the cropped image, which increases the image size
  2. rotate-first:
    • rotate, which increases the image size
    • find the bbox of the (rotated) border polygon in the rotated image, mask the parts outside of the polygon with background, and crop to that bbox

These 2 approaches create different images and different coordinates. So whatever gets used, must be used consistently.

Now core is always responsible for the coordinate transformations, but not always responsible for producing derived images (as processors can create and annotate AlternativeImage themselves, which will have priority over core-generated ones if they meet all requested features). Thus, it has to know which ordering of both operations has been applied when some AlternativeImage has both.

The only choice we have IMHO is making AlternativeImage/@comments order-sensitive. That is, 'binarized,cropped,deskewed' is 1 and 'binarized,deskewed,cropped' is 2.

When there is no AlternativeImage yet, the decision is up to core.

For the implementation, this unfortunately means a large restructuring of both functions.

Or am I missing something?

@bertsky bertsky added the bug label Jun 22, 2020
@kba
Copy link
Member

kba commented Jun 22, 2020

I understand the general problem and that we'll have to adapt image_from* to accomodate different order of cropping+rotation. I am not sure what the default behavior should be. Is one superior to the other? Solution 1 (crop, then rotate) seems simpler. And we should add that decision to the specs (maybe combining definition with the preprocessing extensions you proposed. I've added it to the agenda for our call tomorrow so we can agree on a way forward for a default and then adapt the image_from_* methods.

@bertsky
Copy link
Collaborator Author

bertsky commented Jun 22, 2020

To elaborate: core currently only implements 1, and this is fine as long as there is no workflow with deskewing before cropping. This can only happen on the page level, but since ocrd-anybaseocr-crop recommends deskewing first (because it can only handle rectangular margins), and the workflow guide of course follows that, it's still a frequent case.

Also, I don't think it would be a good idea to just forbid 2. When cropping after deskewing, the resulting image will need much less (masked) margin if rotation happens first.

@bertsky
Copy link
Collaborator Author

bertsky commented Jun 22, 2020

I am not sure what the default behavior should be. Is one superior to the other? Solution 1 (crop, then rotate) seems simpler.

Yes it is, and for the levels below page, it's the only one (because rotation on region level needs to be around the center of the region polygon, so you virtually always crop first; plus we have no 'cropped' for the region level, anyway).

And we should add that decision to the specs

the problem is that this section is hopelessly underspecified. We should have cast the solution from core into spec long ago...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants