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

add auto orient, auto fit, and standard page sizes #33

Closed
wants to merge 1 commit into from
Closed

add auto orient, auto fit, and standard page sizes #33

wants to merge 1 commit into from

Conversation

xiota
Copy link
Contributor

@xiota xiota commented Mar 7, 2015

Added auto orient, auto fit, and standard page size recognition to --pagesize option.

Auto orient will ensure that the output page orientation matches the input orientation so that, for instance, landscape images are not distorted into a portrait page. This option is activated by appending a '+' to the output dimensions.

Auto fit will resize an image within specified boundaries while retaining the aspect ratio. This option is activated by appending a '^' to the output dimensions.

Recognized page sizes include: letter, tabloid, ledger, legal, statement, executive, a0 - a5, folio, and quarto.

Added auto orient, auto fit, and standard page size recognition to --pagesize option.

Auto orient will ensure that the output page orientation matches the input orientation so that, for instance, landscape images are not distorted into a portrait page. This option is activated by appending a '+' to the output dimensions.

Auto fit will resize an image within specified boundaries while retaining the aspect ratio. This option is activated by appending a '^' to the output dimensions.

Recognized page sizes include: letter, tabloid, ledger, legal, statement, executive, a0 - a5, folio, and quarto.
@josch
Copy link
Owner

josch commented Mar 7, 2015

Hi, didn't we agree to implement an imagemagick compatible --pagesize parameter? Why is the AxB+ variant needed?

@xiota
Copy link
Contributor Author

xiota commented Mar 7, 2015

Auto orientation and auto fit both have value and would be difficult to replicate using bash scripting or other pdf manipulation tools.

Also, with auto orientation, extra options to specify portrait or landscape can be avoided when standard page sizes (letter, a4, etc) are specified.

Imagemagick does not seem to use '+' by itself at the end of image dimensions, so someone coming from an imagemagick background is unlikely to use it expecting something else to happen.

@xiota
Copy link
Contributor Author

xiota commented Mar 7, 2015

Another way to implement this without having to figure out orientation and fit is to use the page size to calculate and set dpi. But then you lose 'full' control over the output size.

@josch
Copy link
Owner

josch commented Mar 7, 2015

Here some comments on your patch:

  • you can indent one level less in valid_size() if you use except: pass instead of putting everything under except:
  • instead of silently failing when the first or second argument cannot be interpreted as float, an error should be thrown and execution should stop
  • instead of having a big try/except IndexError, you should handle the parsing either using a regular expression or if statements. I see nine occasions inside the try block that could fail with an IndexError. Instead, catch problems early and report them accurately.
  • extra_option is a string. It should really be a list or set because there can be more than one extra option like ! for exact, % for percentage, < and > for resizing only when the input is smaller or larger...

@josch
Copy link
Owner

josch commented Mar 7, 2015

Olso, the resize handling code in convert() becomes really big. You could break it out into its own function for more clarity.

@xiota
Copy link
Contributor Author

xiota commented Mar 7, 2015

Calculating dpi from pagesize now seems attractive because it would simplify the code and achieve everything I've been trying to replicate.

Should I abandon this route in favor of calculating dpi based on pagesize or attempt to make the changes you've suggested?

@josch
Copy link
Owner

josch commented Mar 7, 2015

Can you express in more detail what you mean by going the dpi based route?

@xiota
Copy link
Contributor Author

xiota commented Mar 7, 2015

In a folder with mixed landscape and portrait images...

  • img2pdf --pagesize 500x -o output.pdf *.jpg would use the width of the first image to calculate dpi based on a 500pt length. That dpi would be used for all subsequent images.
  • img2pdf --pagesize x500 -o output.pdf *.jpg would use the height of the first image to calculate dpi.
  • img2pdf --pagesize 250x500 -o output.pdf *.jpg ... couple options. Could use the diagonal to determine dpi. Or could fit the image inside the boundary and use the fitted edge.

I'll need to brush up on regular expressions, but a regular expression could probably be used to parse out 'in', 'cm', 'mm', and 'pt' for the dpi calculation.

Also, I did look at the !, %, <, >, and @ options for imagemagick, but they don't really apply to converting images into pdf because the units are different and incomparable without a set dpi.

@josch
Copy link
Owner

josch commented Mar 7, 2015

One thing I do not understand is, why the first image has a special place in all your proposals. Why is the first image different from all the others? Why can the others not be?

You also are overlooking that the default dpi is the one of the input image and if that is not set, 96 dpi.

As for the units, yes it might make sense to allow other units like in, cm, mm and pt but right now the unit is PDF units where one unit is 1/72 inch. In this light I don't see how the other imagemagick options are not applicable?

And I still do not understand how your last message solves the problem for you.

Maybe the problem is that I do not yet know what your problem is. What exactly do you want to achieve? Maybe we should start discussing from that.

@xiota
Copy link
Contributor Author

xiota commented Mar 7, 2015

The problem with the current implementation of --pagesize (which follows the implementation of -x and -y), is when images with mixed orientation are given to img2pdf, the page sizes do not match (eg, landscape images are on smaller pages) or the images are distorted (eg, landscape images forced to fit into a portrait orientation). Does anyone actually want that type of behavior?

The easy solution is simply to use --dpi, since the pages will always be oriented accordingly. The issue with this is when I'm trying to fit an image to a pre-existing pdf. I can get the width and height in points with pdfinfo. If I'm working with a single image, then I can copy and paste from pdfinfo, but if I'm working with multiple images and they happen to be different orientations, then I would need to calculate dpi... which involves opening the image to get its dimensions and typing out the appropriate equation.

Eventually, after calculating dpi often enough, I'd get sick enough of it to write some code to open an image and calculate dpi for me... Somehow I went for auto orientation, auto fit, etc. before realizing that I should probably be looking at the dpi.

The significance of the first image is simply that it is the first one processed. If any other image in the sequence were used, that image would end up being processed twice. The output would also be unpredictable if, for instance, an image were randomly selected.

@josch
Copy link
Owner

josch commented Mar 7, 2015

I think you are conflating several issues here.

Firstly, currently the --pagesize option right now does not mimic the one of imagemagick but if it would (as I planned) then if you give an option of the format AxB then it would still keep the aspect ratio and not distort the images. The distortion would only happen if you specify AxB! (with an exclamation mark).

Secondly, why is the xA^ format of imagemagick not applicable in your case? Picture the following scenario:

You have two images, one called portrait.png with dimensions (480x640) and one called landscape.png with dimensions (1280x960). You can see they are of different size but have the same aspect ratio. If I understand you correctly, then what you want is, to put them into a pdf in the right scale without distortion but still scale them such that they occupy the same page size. Try to run the following imagemagick command on both images:

convert portrait.png -resize 'x240^' out1.png
convert landscape.png -resize 'x240^' out2.png

Now out1.png will have dimensions 240x320 and out2.png will have dimensions 320x240. Is that not what you want?

@xiota
Copy link
Contributor Author

xiota commented Mar 7, 2015

The dpi idea might not work b/c what if the images are mixed scans at 300dpi and 600dpi...

Will clean up the code as per your suggestions tomorrow.

@josch
Copy link
Owner

josch commented Mar 7, 2015

Any comments about my message?

@xiota
Copy link
Contributor Author

xiota commented Mar 7, 2015

I still feel some type of orientation control would be useful. Imagemagick has some rotation options that maybe you'll go for in the future =)

Closing this pull request and opening another...

@xiota xiota closed this Mar 7, 2015
@xiota xiota deleted the patch-1 branch March 7, 2015 14:16
@josch
Copy link
Owner

josch commented Mar 7, 2015

@xiota Which rotation options are you specifically talking about?

You do not need to close and re-open a pull request every time. Just edit the existing one by rebasing your local branch.

@xiota
Copy link
Contributor Author

xiota commented Mar 7, 2015

I will look for more info about rebasing. For now, already created a new pull request.

The imagemagick rotation options I was looking at are -rotate degrees{<}{>}, where < and > rotate only when width < height or width > height. It's a bit more than I want to mess with right now though.

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