Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[ios, macos] Fix snapshot scale #10105

Merged
merged 1 commit into from
Oct 4, 2017
Merged

Conversation

frederoni
Copy link
Contributor

@frederoni frederoni commented Oct 2, 2017

Fixes #10089
Fixes the scale of the snapshot on iOS by letting CGImage resize the output.
Snapshotting on macOS already works as intended.

@1ec5 👀
@fabian-guerra feel free to cherry-pick this into #10020

@fabian-guerra
Copy link
Contributor

fabian-guerra commented Oct 2, 2017

@frederoni I realized that the following code made the image on macOS be resized 4x

[compositedImage lockFocus];
                    [logoImage drawInRect:CGRectMake(MGLLogoImagePosition.x, MGLLogoImagePosition.y, logoImage.size.width,logoImage.size.height)];
                    [compositedImage unlockFocus];

I'm changing that.

@1ec5 @frederoni thinking about #10089 makes me realize that if we don't keep the scale and we make 456×480 be 1:1 pixel ratio that would be misleading developers since that would be a scale back for retina screens and images won't look good. Consider the scenario where someone takes a screenshot on an iPhone 7 (375×667) then they present the screenshot in the next controller (full screen) someone would expect a 750×1334 size image that will look good, instead we will return the 375×667 version and the OS will have to scale up resulting on a quality loss.

@frederoni
Copy link
Contributor Author

frederoni commented Oct 2, 2017

I realized that the following code made the image on macOS be resized 4x

[compositedImage lockFocus];
[logoImage drawInRect:CGRectMake(MGLLogoImagePosition.x, MGLLogoImagePosition.y, logoImage.size.width,logoImage.size.height)];
[compositedImage unlockFocus];

I don't get how that could result in 4x but I assume

_scale = [NSScreen mainScreen].backingScaleFactor;

mainScreen could be an issue if you have multiple screens.

Consider the scenario where someone takes a screenshot on an iPhone 7 (375×667) then they present the screenshot in the next controller (full screen) someone would expect a 750×1334 size image that will look good, instead we will return the 375×667 version and the OS will have to scale up resulting on a quality loss.

The UIImage should remain at 375×667 but exporting the image should result in size×scale. Which happens correctly in this branch. As you can see below, there's no rendering/resizing artifact in either of the versions.

Snapshotter Logical resolution Resolution img
MGLMapSnapshotter (this branch) 375 x 667 750 x 1334 image_1 tiff
OS Snapshot 375 x 667 750 x 1334 image_1 tiff

@fabian-guerra
Copy link
Contributor

@frederoni thank you for clarifying this. That's what I meant the resulting image should be 750 x 1334. @1ec5 is this what you mean?

This is what is get in my current branch could you please test if still happens in your branch?:

--->[compositedImage lockFocus];
(lldb) po compositedImage
<NSImage 0x60800047f580 Size={2052, 1434} Reps=(
    "<NSCGImageSnapshotRep:0x608000272b00 cgImage=<CGImage 0x6080001c3660>\n\t<<CGColorSpace 0x618000025e60> (kCGColorSpaceDeviceRGB)>\n\t\twidth = 2052, height = 1434, bpc = 8, bpp = 32, row bytes = 8208 \n\t\tkCGImageAlphaPremultipliedLast | 0 (default byte order) \n\t\tis mask? No, has mask? No, has matte? No, should interpolate? No>"
)>

--->[compositedImage unlockFocus];
(lldb) po compositedImage
<NSImage 0x60800047f580 Size={2052, 1434} Reps=(
    "<NSCGImageSnapshotRep:0x60800006d140 cgImage=<CGImage 0x6080001c2ee0>\n\t<<CGColorSpace 0x618000025ea0> (kCGColorSpaceICCBased; kCGColorSpaceModelRGB; Color LCD)>\n\t\twidth = 4104, height = 2868, bpc = 8, bpp = 32, row bytes = 16448 \n\t\tkCGImageAlphaPremultipliedFirst | kCGImageByteOrder32Little \n\t\tis mask? No, has mask? No, has matte? No, should interpolate? Yes>"
)>

@fabian-guerra
Copy link
Contributor

@frederoni I can confirm that the 4x scaling on macOS happens with this branch too.

When I comment this code:

[compositedImage lockFocus];
[logoImage drawInRect:CGRectMake(MGLLogoImagePosition.x, MGLLogoImagePosition.y, logoImage.size.width,logoImage.size.height)];
[compositedImage unlockFocus];

I get an image sized 2052 × 1434
light-v9_2x

Instead of 4104 × 2868
light-v9_4x

(Download and compare please)

@frederoni
Copy link
Contributor Author

Yep, this change only fixes the iOS version.

@fabian-guerra
Copy link
Contributor

fabian-guerra commented Oct 2, 2017

@frederoni I fixed the issue https://github.com/mapbox/mapbox-gl-native/pull/10020/files#diff-56bc5c1e41273957efd873aa83e6c522R159 I had to do something similar to what you did for iOS.

@friedbunny friedbunny added this to the ios-v3.7.0 milestone Oct 3, 2017
@friedbunny friedbunny added the iOS Mapbox Maps SDK for iOS label Oct 3, 2017
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

65936d7 fixes the macOS implementation to produce a correctly sized image.


// Process image watermark in a work queue
dispatch_queue_t workQueue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0);
dispatch_async(workQueue, ^{
#if TARGET_OS_IPHONE
UIImage *logoImage = [UIImage imageNamed:@"mapbox" inBundle:[NSBundle mgl_frameworkBundle] compatibleWithTraitCollection:nil];

UIGraphicsBeginImageContext(mglImage.size);
UIGraphicsBeginImageContextWithOptions(mglImage.size, NO, self.options.scale);
Copy link
Contributor

Choose a reason for hiding this comment

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

The map isn’t necessarily opaque. As in #7859, if the background style layer has a non-opaque fill color, an alpha channel may be needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I created this ticket to follow up the tail work.

@1ec5 1ec5 changed the title [ios] Fix snapshot scale [macos, ios] Fix snapshot scale Oct 3, 2017
@1ec5 1ec5 added bug macOS Mapbox Maps SDK for macOS labels Oct 3, 2017
@1ec5 1ec5 changed the title [macos, ios] Fix snapshot scale [ios, macos] Fix snapshot scale Oct 3, 2017
@fabian-guerra fabian-guerra merged commit 4bb25e8 into master Oct 4, 2017
@fabian-guerra fabian-guerra deleted the fred-fix-snapshot-scale branch October 4, 2017 18:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants