Skip to content

Conversation

AaaK00
Copy link
Contributor

@AaaK00 AaaK00 commented Dec 23, 2020

Found out that zero cannot be given as parameter to WebP options. For example, code:

let options = [.encodeWebPMethod: 0, .encodeWebPAlphaCompression: 100]
let data = SDImageWebPCoder.shared.encodedData(with: image, format: .webP, options: options)

Does not work correctly. Default value for WebP method (4) is used instead of 0.

Issue is caused by the fact that comparison

if ([options[SDImageCoderEncodeWebPMethod] intValue])

results to false when intValue is 0.

Fixed by changing the logic of how it is concluded whether value for certain option has been provided. Also updated unit tests so they catch similar issues in future.

@AaaK00 AaaK00 force-pushed the fix_zero_value_in_webp_options_ignored branch from 299ea78 to a197fa1 Compare December 23, 2020 15:41
@codecov
Copy link

codecov bot commented Dec 23, 2020

Codecov Report

Merging #52 (415e6b6) into master (864a71e) will decrease coverage by 0.25%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #52      +/-   ##
==========================================
- Coverage   77.36%   77.11%   -0.26%     
==========================================
  Files           2        2              
  Lines         813      804       -9     
==========================================
- Hits          629      620       -9     
  Misses        184      184              
Flag Coverage Δ
ios 77.11% <100.00%> (-0.26%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
SDWebImageWebPCoder/Classes/SDImageWebPCoder.m 77.00% <100.00%> (-0.26%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 864a71e...415e6b6. Read the comment docs.

@dreampiggy
Copy link
Contributor

dreampiggy commented Dec 24, 2020

That helper method getIntValueFor:defaultValue:options: looks really strange. Not a good design for SDK development. (Only works in this context, why not more generic)

Use something like this:

static int GetIntValueForKey(NSDictionary * _Nonnull dictionary, NSString * _Nonnull key, int defaultValue) {
   // ... same implementation
}

Or even a extension to NSDictionary, but I think since SDWebImageWebPCoder is simple codec wrapper. Adding extension to NSDictionary class will increase binary size and have to use something like prefix style API sd_intValueForKey:, which I dislike...

So a single inline or static C function is better.

@AaaK00
Copy link
Contributor Author

AaaK00 commented Dec 24, 2020

That helper method getIntValueFor:defaultValue:options: looks really strange. Not a good design for SDK development. (Only works in this context, why not more generic)

Use something like this:

static int GetIntValueForKey(NSDictionary * _Nonnull dictionary, NSString * _Nonnull key, int defaultValue) {
   // ... same implementation
}

Or even a extension to NSDictionary, but I think since SDWebImageWebPCoder is simple codec wrapper. Adding extension to NSDictionary class will increase binary size and have to use something like prefix style API sd_intValueForKey:, which I dislike...

So a single inline or static C function is better.

Yeap, good point. Probably better to use plain static C function. Better now?

@dreampiggy
Copy link
Contributor

LGTM now. I'll merge and release new version.

@dreampiggy dreampiggy merged commit 25f5908 into SDWebImage:master Dec 24, 2020
@dreampiggy
Copy link
Contributor

Released v0.8.1

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.

2 participants