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

revert auto crop fix #11325

Merged
merged 5 commits into from Feb 17, 2024
Merged

revert auto crop fix #11325

merged 5 commits into from Feb 17, 2024

Conversation

hugleo
Copy link
Contributor

@hugleo hugleo commented Jan 7, 2024

the first commit I revert the changes from #11194 in favor of #11291

the second commit is another try


This change is Reviewable

Copy link
Member

@Frenzie Frenzie left a comment

Choose a reason for hiding this comment

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

Fine by me in principle, but just looking at the diff I find it a bit hard to grok and I don't really want to read deeper at the moment. :-)

@NiLuJe
Copy link
Member

NiLuJe commented Jan 7, 2024

The or thing crossed my mind while looking at the original check. I can't speak to the rest (i.e., tweaking the coords), though, and am not able to even look up the rest of the code for a few days ;).

@hugleo hugleo marked this pull request as draft February 12, 2024 00:05
@hugleo
Copy link
Contributor Author

hugleo commented Feb 12, 2024

This tweaking will also cause some problems. The values will be overridden here: https://github.com/koreader/koreader/blob/master/frontend/apps/reader/modules/readerview.lua#L668.

But I found the cause of the problem is because of the margins:

Test with the uncropped cbz:
Real mupdf page size = page_size "849x1200+0+0"

When we enter the area of kopt (koptcrop.c - https://github.com/koreader/libk2pdfopt):

The source page (0,0) - (848,1199)
The source page after being cropped: (96,0) - (777,1199)
margin value of 10.396947
The source page with the margin applied: (85.603050,-10.396947) - (787.396973,1209.396973)

You can see that originally only the width was cropped from 848 to 777. This is because there is no white area to be cropped in the height direction. However, the margin of 10.396947 is applied uniformly and extends to the height direction as well, increasing it from 1199 to 1209.396973. This causes the bbox to be larger than the page, resulting in the original page being rendered instead of the bbox.

I found a workaround for this problematic cbz file: we set the koreader margin to 0 so that no margin will be added, and the cropped bbox will be rendered.

margin

It would be better to support separate margins for the w and h positions (having some margins in the w from 848 to 777 would be good) but for now since the cbz cropped issue can be fixed with the workaround will revert to the original here.

@hugleo hugleo marked this pull request as ready for review February 12, 2024 04:46
hugleo added a commit to hugleo/libk2pdfopt that referenced this pull request Feb 12, 2024
A change in koptcrop for better margins, as mentioned in the comment: koreader/koreader#11325 (comment). 
The bbox shouldn't be smaller than the page again, unless there's a DPI problem, who knows... 
I didn't exhaustively test the semi-auto and manual crop modes...
But only if @poire-z finds if is logical :D
Frenzie pushed a commit to koreader/libk2pdfopt that referenced this pull request Feb 14, 2024
A change in koptcrop for better margins, as mentioned in the comment: koreader/koreader#11325 (comment).
@poire-z poire-z merged commit 041117c into koreader:master Feb 17, 2024
3 checks passed
@poire-z poire-z added this to the 2024.02 milestone Feb 17, 2024
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.

None yet

4 participants