Tighten size check #8562

brunoabinader · 2017-03-29T14:12:42Z

Some platforms provide their own size type defaulting to -1 in both width and height e.g. Qt's QSize. We should special case this and make this part of the assertion check.

mention-bot · 2017-03-29T14:12:44Z

@brunoabinader, thanks for your PR! By analyzing this pull request, we identified @tmpsantos, @1ec5 and @jfirebaugh to be potential reviewers.

kkaefer

I disagree with this change. util::Size is used internally in Mapbox GL, and converting platform-specific size types to util::Size is the platform binding's responsibility: The Qt wrapper should convert QSize to util::Size and back.

brunoabinader · 2017-03-29T15:58:01Z

@kkaefer I've added a sanitizer for the Qt wrapper when converting from QSize to mbgl::Size. I've also modified the approach to not touch mbgl::Size check, but instead failing in Transform::resize for zero-ed sizes and asserting a valid size in mbgl::util::tileCover.

jfirebaugh · 2017-03-30T16:46:28Z

include/mbgl/util/projection.hpp

@@ -13,7 +13,7 @@ namespace mbgl {
 class Projection {
 public:
    // Map pixel width at given scale.
-    static double worldSize(double scale) {
+    static constexpr double worldSize(double scale) {


What's the rationale for this change?

~~Nothing special besides the fact util::tileSize is already known in compile time - as requested below I'll subtract this cosmetic change from this PR 👍~~

Actually this is needed if we want to keep TransformState ctors constexpr - Projection::worldSize is used to initialize Bc and Cc here.

Is there a need for TransformState to be constexpr? Based on the history with GCC 4.9 it sounded like something we should avoid using unless it's truly necessary.

@jfirebaugh my understanding is that using constexpr allows clearer understanding of what we want to do, and what the compiler is possible of calculating in compile time - I personally find class/struct ctors using constexpr to be a more elegant pattern too.

There are specific cases where constexpr usage is bogus on GCC 4.9 - though this is not the case for simple cases like the ones above i.e. we still use plain constexpr in lots of places in our https://github.com/mapbox/mapbox-gl-native/tree/qt-staging branch.

As example, there are cases where constexpr works fine in GCC 4.9 - like this example, and cases where it doesn't.

Probably the best way to make sure we're compliant with GCC 4.9 is to have a GCC 4.9-based build bot running as part of our CI, but I guess that's out of scope for this PR.

using constexpr allows clearer understanding of what we want to do

How so? We don't inherently intend that these calculations or constructors happen at compile time, and the fact that they (currently) can is not an essential feature of the code.

I don't think we should go around adding constexpr everywhere it's possible to do so. If we're talking about introducing coding guidelines for C++ features, then I would say my guideline for constexpr is "use only in cases where compile-time evaluation is an intended or essential feature of the code, or for general-purpose library functions which may need to be used in constexpr contexts."

jfirebaugh · 2017-03-30T16:47:29Z

src/mbgl/map/transform.cpp

@@ -46,6 +46,10 @@ Transform::Transform(MapObserver& observer_,
 #pragma mark - Map View

 bool Transform::resize(const Size size) {
+    if (!size) {
+        return false;


If (0, 0) is an invalid size, shouldn't this throw rather than silently doing nothing?

I supposed that was the meaning of the return bool - should we make it void instead and throw on such cases?

I think at one point the return value was used to check if the size was changing, but it looks like this may be used only in tests. If so, yes, void and throw on invalid input.

We can use try/catch in the tests 👍

jfirebaugh · 2017-03-30T16:49:13Z

src/mbgl/map/transform_state.hpp

-    TransformState(ConstrainMode = ConstrainMode::HeightOnly, ViewportMode = ViewportMode::Default);
+    constexpr TransformState() = default;
+
+    constexpr TransformState(ConstrainMode constrain_, ViewportMode viewport_)


This removes the ability to construct with a single argument. (Could you please limit your changes to fixing the main issue, and propose cosmetic changes in a separate PR?)

This removes the ability to construct with a single argument.

Do we have a need for this? If so, we could add an extra ctor that receives a single ConstrainMode.

Could you please limit your changes to fixing the main issue, and propose cosmetic changes in a separate PR?

Fair enough - though I've seen this as a common behavior in some of our PRs.

jfirebaugh · 2017-03-30T16:49:27Z

src/mbgl/map/transform_state.hpp

-    double min_scale = std::pow(2, 0);
-    double max_scale = std::pow(2, 20);
+    double min_scale = 1.0; // 2^0
+    double max_scale = 1048576.0; // 2^20


What's the rationale for this change?

These values are known and std::pow is not constexpr, causing the TransformState ctor to not be constexpr-able too.

jfirebaugh · 2017-03-30T16:51:01Z

src/mbgl/map/transform_state.hpp

@@ -74,13 +77,17 @@ class TransformState {
    double zoomScale(double zoom) const;
    double scaleZoom(double scale) const;

+    constexpr explicit operator bool() const {


Let's not use operator bool() for validity checks. If we need a validity check, name it bool valid() const. But first, try to eliminate the possibility of an invalid state.

Let's not use operator bool() for validity checks.

We've discussed this before, yet we still use explicit operator bool in other parts of the code e.g. Size::operator bool.

I don't have a strong opining about replacing it with something like valid(), however I'd prefer to have a separate PR that replaces every explicit operator bool usage for validation in our code 👍

👉 #8601.

kkaefer · 2017-03-29T16:00:22Z

platform/qt/src/qmapboxgl.cpp

 {
+    mapObj = std::make_unique<mbgl::Map>(
+            *this, sanitizedSize(size),


why is size invalid here and needs to be sanitized in the first place?

Because QSize default constructor initializes witdh and height with -1. This causes an overflow when converting from int to uint32_t- using std::numeric_limits<uint32_t>::max() for width and height has proven to cause undefined behavior in places like tileCover.

Qt sometimes initializes a dummy map plugin as means of discovering which map plugins it has available - when it does, it uses a default QSize for initializing the item size.

kkaefer · 2017-03-29T16:01:04Z

src/mbgl/map/transform.cpp

@@ -46,6 +46,10 @@ Transform::Transform(MapObserver& observer_,
 #pragma mark - Map View

 bool Transform::resize(const Size size) {
+    if (!size) {
+        return false;
+    }


Why do we need to return early here?

Because if we allow Transform to have a zero-ed size, it can result in division-by-zero issues e.g. here.

brunoabinader · 2017-04-03T14:10:05Z

@jfirebaugh I've ~~moved the constexpr changes to a specific patch~~ removed the constexpr-related changes and made Transform::resize throw in case it receives an invalid size.

ivovandongen

Android changes look 👍

tmpsantos

Qt 👍

tmpsantos · 2017-04-06T14:00:34Z

platform/qt/src/qmapboxgl.cpp

+mbgl::Size sanitizedSize(const QSize& size) {
+    return mbgl::Size {
+        size.width() <= 0 ? 0 : static_cast<uint32_t>(size.width()),
+        size.height() <= 0 ? 0 : static_cast<uint32_t>(size.height())


You could use std::max here, but I'm OK with ternary.

Good point, I'll make the change 👍

brunoabinader · 2017-04-06T18:54:14Z

@kkaefer @jfirebaugh can you please re-review?

jfirebaugh · 2017-04-06T19:06:06Z

Can you replace valid with empty or isEmpty (and flip the meaning)? There's nothing inherently invalid about an empty size.

jfirebaugh

👍 on operator bool ⇢ isEmpty changes. IIRC @1ec5 had some concerns about the 64⨯64 minimum size, so let's me sure he weighs in.

1ec5 · 2017-04-06T22:01:06Z

On iOS and macOS, it’s possible to create a view with zero size then give it size at some point before the view appears on screen, such as after the view is added to the view hierarchy. This pattern is especially common in iOS applications that build the interface programmatically using manual layout such as -[UIView sizeToFit], which causes the view to resize to fit its subviews.

As reported in #1572, the current implementation on iOS and macOS accommodates this pattern only partially. On master, the following code:

MGLMapView *mapView = [[MGLMapView alloc] initWithFrame:CGRectZero];
mapView.frame = CGRectMake(0, 0, 300, 300);
[self.view addSubview:mapView];

results in the following console spew:

2017-04-06 13:32:11.888 Mapbox GL[72997:2170914] Failed to bind EAGLDrawable: <CAEAGLLayer: 0x60800042e7e0> to GL_RENDERBUFFER 1
2017-04-06 13:32:11.889 Mapbox GL[72997:2170914] Failed to make complete framebuffer object 8cd6

but otherwise everything is perfectly fine.

However, swap two lines so that the view is added to the view hierarchy before it gets some size, and the application crashes with an OpenGL error:

2017-04-06 13:32:56.752 Mapbox GL[73050:2172273] Failed to bind EAGLDrawable: <CAEAGLLayer: 0x600000234e00> to GL_RENDERBUFFER 1
2017-04-06 13:32:56.753 Mapbox GL[73050:2172273] Failed to make complete framebuffer object 8cd6
2017-04-06 13:32:56.756 Mapbox GL[73050:2172273] Failed to bind EAGLDrawable: <CAEAGLLayer: 0x600000234e00> to GL_RENDERBUFFER 1
2017-04-06 13:32:56.756 Mapbox GL[73050:2172273] Failed to make complete framebuffer object 8cd6
libc++abi.dylib: terminating with uncaught exception of type mbgl::gl::Error: glGetIntegerv(GL_VIEWPORT, viewport): Error GL_INVALID_ENUM at /Users/mxn/hub/mapbox-gl-native/src/mbgl/gl/value.cpp:264

As I understand it, this PR would ensure that the crash occurs earlier with a more informative error message. Unfortunately, it also causes the first case (setting the size right after initialization) to crash in the same way:

2017-04-06 13:40:43.244 Mapbox GL[73386:2182614] Failed to bind EAGLDrawable: <CAEAGLLayer: 0x60000043bbc0> to GL_RENDERBUFFER 1
2017-04-06 13:40:43.244 Mapbox GL[73386:2182614] Failed to make complete framebuffer object 8cd6
libc++abi.dylib: terminating with uncaught exception of type std::runtime_error: failed to resize: size is empty

I think this would be a surprising change for developers who build their interface programmatically. Is there any way to modify mbgl::Map so that it only trips this new size assertion when the map is visible?

If not, I suppose we could implement #1572 (comment) by making 64×64 points the minimum size in -[MGLMapView size], which is used when creating mbgl::Map and when the view itself is resized:

diff --git a/platform/ios/src/MGLMapView.mm b/platform/ios/src/MGLMapView.mm
index d80e741c0..67b5ac09f 100644
--- a/platform/ios/src/MGLMapView.mm
+++ b/platform/ios/src/MGLMapView.mm
@@ -576,8 +576,12 @@ public:
 
 - (mbgl::Size)size
 {
-    return { static_cast<uint32_t>(self.bounds.size.width),
-             static_cast<uint32_t>(self.bounds.size.height) };
+    CGSize size = CGSizeMake(MAX(self.bounds.size.width, 64),
+                             MAX(self.bounds.size.height, 64));
+    return {
+        static_cast<uint32_t>(size.width),
+        static_cast<uint32_t>(size.height),
+    };
 }
 
 - (mbgl::Size)framebufferSize
diff --git a/platform/macos/src/MGLMapView.mm b/platform/macos/src/MGLMapView.mm
index e19755044..bccc7cff9 100644
--- a/platform/macos/src/MGLMapView.mm
+++ b/platform/macos/src/MGLMapView.mm
@@ -307,8 +307,12 @@ public:
 }
 
 - (mbgl::Size)size {
-    return { static_cast<uint32_t>(self.bounds.size.width),
-             static_cast<uint32_t>(self.bounds.size.height) };
+    CGSize size = CGSizeMake(MAX(self.bounds.size.width, 64),
+                             MAX(self.bounds.size.height, 64));
+    return {
+        static_cast<uint32_t>(size.width),
+        static_cast<uint32_t>(size.height),
+    };
 }
 
 - (mbgl::Size)framebufferSize {

Methods like -[MGLMapView convertCoordinate:toPointToView:] will return incorrect values until the map is resized beyond 64×64 points, but that’s still better than crashing. Besides -size, would -framebufferSize also need to be limited like this?

/cc @boundsj

1ec5 · 2017-04-06T22:15:38Z

Note that, even with the diff proposed in #8562 (comment), the console spew about failing to bind the drawable persists but appears to be harmless:

2017-04-06 15:13:01.489 Mapbox GL[75226:2237986] Failed to bind EAGLDrawable: <CAEAGLLayer: 0x608000225780> to GL_RENDERBUFFER 1
2017-04-06 15:13:01.490 Mapbox GL[75226:2237986] Failed to make complete framebuffer object 8cd6

brunoabinader · 2017-04-07T08:37:18Z

On iOS and macOS, it’s possible to create a view with zero size then give it size at some point before the view appears on screen

We are not going to modify this pattern - as you mentioned the size can be silently updated to 64x64 inside size getter (or initWithFrame).

The suggested patch for iOS is no different from the approach took by MGLStyleTests and MGLSourceQueryTests.

Is there any way to modify mbgl::Map so that it only trips this new size assertion when the map is visible?

Initializing a map with a valid (by valid I mean at least the minimum texture size as specified by the OpenGL ES 2.0 which is 64x64) size is currently mandatory in our code - because we are immediately supposed to be able to call render(). One alternative would be to have a check in core to ignore render() calls until we have a valid texture size set.

Complexity aside, I believe the less painful change would be to just set the size to 64x64 silently when passing it to the Map object - this way developers won't notice the change and we can be safeguarded. This approach is i.e. the proposed one for Qt and Android already.

outdated review

1ec5 · 2017-04-08T04:47:45Z

Alright, let’s go with silently upping the size to 64×64 points. Does -framebufferSize need to have a minimum of 64×64 points as well?

brunoabinader · 2017-04-08T08:33:42Z

Does -framebufferSize need to have a minimum of 64×64 points as well?

The only restriction I'm aware of for framebuffer sizes is that their width and height values must be a power of 2 for OpenGL versions prior to 2.0.

… size

…size

brunoabinader added Core The cross-platform C++ core, aka mbgl Qt Mapbox Maps SDK for Qt, aka Qt Location Mapbox GL labels Mar 29, 2017

brunoabinader self-assigned this Mar 29, 2017

kkaefer previously requested changes Mar 29, 2017

View reviewed changes

brunoabinader force-pushed the tighten-size-check branch from 56572b1 to b076cf5 Compare March 29, 2017 15:53

brunoabinader force-pushed the tighten-size-check branch from b076cf5 to 6701aed Compare March 29, 2017 16:27

brunoabinader requested review from jfirebaugh and tmpsantos March 30, 2017 13:52

jfirebaugh suggested changes Mar 30, 2017

View reviewed changes

kkaefer reviewed Mar 30, 2017

View reviewed changes

brunoabinader force-pushed the tighten-size-check branch from 6701aed to 05d1f9f Compare April 3, 2017 14:08

brunoabinader force-pushed the tighten-size-check branch 2 times, most recently from a44bbf9 to c67c189 Compare April 3, 2017 15:05

brunoabinader mentioned this pull request Apr 3, 2017

Refactor explicit operator bool usage for some cases #8601

Merged

brunoabinader force-pushed the tighten-size-check branch 2 times, most recently from d0a8a74 to 6ab94e9 Compare April 4, 2017 17:27

brunoabinader mentioned this pull request Apr 4, 2017

MGLMapView fails to bind EAGLDrawable when initialised with CGRect with no width and height #1572

Closed

brunoabinader force-pushed the tighten-size-check branch 2 times, most recently from 413e34c to 94ecc6a Compare April 6, 2017 12:52

ivovandongen approved these changes Apr 6, 2017

View reviewed changes

brunoabinader force-pushed the tighten-size-check branch from 94ecc6a to 28ac4c4 Compare April 6, 2017 13:23

tmpsantos approved these changes Apr 6, 2017

View reviewed changes

brunoabinader force-pushed the tighten-size-check branch from 28ac4c4 to d94cb91 Compare April 6, 2017 14:17

brunoabinader requested a review from 1ec5 April 6, 2017 14:25

brunoabinader force-pushed the tighten-size-check branch from d94cb91 to b05e149 Compare April 6, 2017 18:23

brunoabinader force-pushed the tighten-size-check branch from b05e149 to 7bcb6b7 Compare April 6, 2017 20:02

jfirebaugh approved these changes Apr 6, 2017

View reviewed changes

brunoabinader added 6 commits April 10, 2017 17:18

[core] Updated Size::isEmpty and TransformState::valid checks

fa029aa

[Qt] Sanitize QSize → mbgl::Size conversion

2fb6178

[ios] Initialize MGLMapView with OpenGL ES 2.0 minimum texture size

a3fb462

[android] Initialize NativeMapView with OpenGL ES 2.0 minimum texture…

973b5b9

… size

[android] Recalculate source tile cache size when appropiate

aeeda02

[ios, macos] MGLMapView size check for OpenGL 2.0 ES minimum texture …

f044cb4

…size

brunoabinader force-pushed the tighten-size-check branch from 7bcb6b7 to f044cb4 Compare April 10, 2017 14:18

brunoabinader merged commit a9e09c7 into master Apr 10, 2017

brunoabinader deleted the tighten-size-check branch April 10, 2017 15:01

jfirebaugh mentioned this pull request Apr 11, 2017

Error in the iOS SDK 3.3.7 #8344

Closed

1ec5 mentioned this pull request Apr 12, 2017

Update and reorganize changelogs #8723

Merged

Tighten size check #8562

Tighten size check #8562

Conversation

brunoabinader commented Mar 29, 2017 • edited Loading

mention-bot commented Mar 29, 2017

kkaefer left a comment

Choose a reason for hiding this comment

brunoabinader commented Mar 29, 2017

Choose a reason for hiding this comment

brunoabinader Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunoabinader commented Apr 3, 2017 • edited Loading

ivovandongen left a comment

Choose a reason for hiding this comment

tmpsantos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunoabinader commented Apr 6, 2017

jfirebaugh commented Apr 6, 2017

jfirebaugh left a comment

Choose a reason for hiding this comment

1ec5 commented Apr 6, 2017 • edited Loading

1ec5 commented Apr 6, 2017 • edited Loading

brunoabinader commented Apr 7, 2017

1ec5 commented Apr 8, 2017

brunoabinader commented Apr 8, 2017

brunoabinader commented Mar 29, 2017 •

edited

Loading

brunoabinader Mar 30, 2017 •

edited

Loading

brunoabinader commented Apr 3, 2017 •

edited

Loading

1ec5 commented Apr 6, 2017 •

edited

Loading

1ec5 commented Apr 6, 2017 •

edited

Loading