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

PDFImage quality does not work as expected #78

Closed
omiz opened this issue Jul 9, 2018 · 10 comments
Closed

PDFImage quality does not work as expected #78

omiz opened this issue Jul 9, 2018 · 10 comments
Assignees

Comments

@omiz
Copy link

omiz commented Jul 9, 2018

From the documentation:

 /**
 JPEG quality of image.

 Value ranges between 0.0 and 1.0, maximum quality equals 1.0
 */
 public var quality: CGFloat

But the current behaviour is

  • 0 will not apply any compression
  • 1 will apply compression
  • values in between behave correctly as the documentation

expected behaviour

  • 0 will have the most compression or least quality
  • 1 will have the least compression or full quality (no compression)
philprime added a commit that referenced this issue Jul 12, 2018
@philprime philprime self-assigned this Jul 12, 2018
@philprime philprime modified the milestone: Version 1.1.0 Jul 12, 2018
@philprime
Copy link
Member

Fixed in release 1.1.2

@omiz
Copy link
Author

omiz commented Jul 12, 2018

just a question: so zero wasn't adding any compression before right.
what about making it an optional and if it's nil then no compression and if some then it has some?

@philprime philprime removed the pending label Jul 12, 2018
@philprime
Copy link
Member

The image is resized in any case to fit the given frame, therefore TPPDF has to calculate and convert anyway.
Making it an Optional would require us to set a fallback value, which is 1.0 to have the best quality/least compression. If we require the developer to set the quality explicitly, it is easier to understand what this value does.

@omiz
Copy link
Author

omiz commented Jul 12, 2018

this is not exactly what I want
I still prefer to have no compression at all for some images

there was this code that makes zero without any compression

if resizeFactor == 0 { return image }

I don't see why we can't make it optional with

if resizeFactor == nil { return image }

sorry I didn't read the full implementing so it will be something like that I hope you know what I mean

@philprime
Copy link
Member

I think I do understand what you mean.
I want to give an example, maybe you are thinking about an use-case I don't know:

  • The image has a width of 1500px and height of 1000px.
  • The layout engine inside TPPDF calculates a desired frame size of 150px x 100px, because of the structure of the document.
  • Now we can either draw the full-sized original image into the frame or we can resize it first to get rid of the extraneous bytes.
  • Additionally if you do not care so much about the quality of your image, you can set a quality which results in compression.

PDFGraphcis.resizeAndCompressImage() resizes and compresses the image. A quality of 1.0 should return the same quality image as if we skip compression. In my opinion it is easier to understand if you can only set a value between 0.0 and 1.0, without the additional nil value. If this is absolutely necessary I would recommend adding a enum value optimisation to PDFImage with the following cases:

enum PDFImageOptimisation {
    case .none
    case .resize
    case .compress
    case .all
}

Now my question:

  • Do you want to skip resizing, compression or both?

@omiz
Copy link
Author

omiz commented Jul 12, 2018

lets not make it complicated.

if you get any image needs resizing then resize do it.
if you get a quality (not nil) then add a compression.

in my case I sometimes need to crop, resize and keep ratio, stretch. so it's not about the resizing process but more about not losing the quality in some of the images.

So if resized the image already then resizing it again will mean extra un-needed work.
if I don't want to compress the image and don't care about the size of some images then I can pass nil for the quality and will not be changed.

I don't see the need of adding any extra option.

But if you really really want to do this PDFImageOptimisation then here is a suggestion take it or leave it

struct PDFImageOptimisation: OptionSet {
    
    let rawValue: Int
}

extension PDFImageOptimisation {
    
    static let resize = PDFImageOptimisation(rawValue: 1 << 0)
    
    static let compress = PDFImageOptimisation(rawValue: 1 << 1)
}

var options: PDFImageOptimisation

options = [.resize, .compress] // all

options = .resize // single

options = [] // none

if options.contains(.resize) {
    // do resize
}

@philprime philprime reopened this Jul 12, 2018
@philprime
Copy link
Member

So it is about not modifying the image if not necessary. Sounds reasonable.

I like the OptionSet approach as it gives precise control with high readability.
I will implement this and release an update.
The default options will be [.resize, .compress] therefore it won't change existing documents.

@philprime
Copy link
Member

Can you please take a look at the latest commit in the develop branch and let me know about your thoughts.

@omiz
Copy link
Author

omiz commented Jul 12, 2018

You did a great job thank you man!

Just a small question: do you think that it might be better to separate the implementation of resize and compress?

Here is what I mean

instead of this:

static func resizeAndCompressImage(image: UIImage, frame: CGRect, shouldResize: Bool, shouldCompress: Bool, quality: CGFloat) -> UIImage

use this:

static func resize(image: UIImage, frame: CGRect, quality: CGFloat) -> UIImage {
    
    let factor: CGFloat = min(3 * quality, 1)
    
    let resizeFactor = factor.isZero ? 0.2 : factor
    
    let size = CGSize(width: frame.width * resizeFactor, height: frame.height * resizeFactor)
    
    UIGraphicsBeginImageContext(size)
    
    image.draw(in: CGRect(origin: .zero, size: size))
    
    let finalImage = UIGraphicsGetImageFromCurrentImageContext()
    
    UIGraphicsEndImageContext()
    
    return finalImage ?? image
}

static func compress(image: UIImage, quality: CGFloat) -> UIImage {

    guard let data = UIImageJPEGRepresentation(image, quality) else { return image }
    
    guard let compressed = UIImage(data: data) else { return image }

    return compressed
}

so it can be easily used as a different action and it will be scalable (if someone asked for crop action or something else)

@philprime
Copy link
Member

Great input! Thank you! I am preparing a release 1.2.0 right now, going to be available soon!

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

No branches or pull requests

2 participants