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

Make "devicePixelRatio" user-configurable #1953

Open
kristfal opened this issue Jan 20, 2016 · 14 comments
Open

Make "devicePixelRatio" user-configurable #1953

kristfal opened this issue Jan 20, 2016 · 14 comments
Labels
feature 🍏 good first issue performance ⚡ Speed, stability, CPU usage, memory usage, or power usage

Comments

@kristfal
Copy link

Hey,

Summary:

On (mobile) devices with very high ppi, render at a lower resolution than the device's native ppi and upscale it.

Long story:

Mapbox-gl-js fares very well on iOS devices performance wise (with #1606).

The story is a bit different on Android devices.

The most interesting findings we've had so far is that a Samsung Galaxy S3 (Released March 2013) is roughly 10% slower FPS wise than a Samsung Galaxy S6 (Released Q2 2015) under similar circumstances.

The GL draw calls take roughly equal time on the S3 and S6, despite a much faster CPU and GPU on the S6. The reason for the S6's lack of performance over the S3 seems to be the newer device's PPI. The S6 has a PPI of 577 vs 306 for the S3, and as a ref, 326 for iPhone 6.

This pattern repeats itself for almost all new Android devices, and the race is still going to 800 ppi and beyond. Android screen resolution is often 'unnecessary' high, which in turn leads to poor web-gl performance.

It is possible to downscale the rendering of mapbox-gl-js in html/css without touching the internal js code, but it also requires highjacking the touch / mouse handlers and scale down symbols and text in the style to make it work properly. We're following this approach, and do downscaling to an effective 326 ppi on any device with a higher native ppi. In the case of the S6, we're seeing an effective 3.5x increase in performance without any notices from users.

This workaround is probably outside the scope of your average mapbox-gl-js user. For gl-js not to be a performance hog on mobile (android) devices by default, device PPI should probably be handled.

@mourner mourner added feature 🍏 performance ⚡ Speed, stability, CPU usage, memory usage, or power usage api labels Jan 20, 2016
@mourner
Copy link
Member

mourner commented Jan 20, 2016

That's a valid feature request. Want to look into turning the suggestion into a pull request? I wonder how easy it is to make the ratio configurable as opposed to always on based on the screen.

@kristfal
Copy link
Author

@mourner I was leaning towards 'max PPI / devicePixelRatio' option in the map config that would downscale only if the threshold was surpassed, but do think it would be better to add a 'target PPI / devicePixelRatio' option that would force a ratio regardless of native screen spec?

I haven't done any changes in the mapbox-gl code yet, just hacks in the app containing the map, but I'll look into it – probably starting this weekend.

@SandroGrzicic
Copy link

Hi,

we are using mapbox gl js for both android and iOS devices and we just want to add a huge +1 on this feature request. Sometimes the map is visibly struggling to keep up on our Androids and a 3.5x or even a 1.5-2x speed increase would transform the user experience into "why is this app so slow" into "this thing is great!".

@kristfal any pointers on what is needed to accomplish this? Thanks!

[UPDATE] Apparently it's enough to change both instances of "window.devicePixelRatio" to 1 (or perhaps 1.5? Do fractional values work?) and the map will automatically be shown in a lower resolution, without any artifacts (that I can see). Please correct me if I'm doing something wrong. The performance seems higher now, but unsure how much exactly. So far 1.5 seems to be the sweet spot between quality and performance.

@lucaswoj lucaswoj removed the api label Mar 10, 2016
@lucaswoj lucaswoj changed the title Android devices, performance and ppi Make "devicePixelRatio" user-configurable Jul 29, 2016
@wprater
Copy link

wprater commented May 2, 2017

Is this solution one that's still being considered?

@psyrendust
Copy link

@mourner @lucaswoj, why has this not been addressed after being open for 2 years? Are you still seeking an external contribution for this to get done?

@lucaswoj
Copy link
Contributor

Our sincere apologies @psyrendust. To be honest this issue hasn't come up often enough to hit the top of our todo list. We are definitely open to an external contribution.

@psyrendust
Copy link

@lucaswoj thanks for the quick reply. I'll see if I can get some time to do a pr for this. Off the top of your head, are there a lot of touch points in the code that this would affect?

@lucaswoj
Copy link
Contributor

No problem @psyrendust. As best I can tell, the PR should be straightforward.

  • Add a devicePixelRatio setting to config.js which defaults to browser.devicePixelRatio
  • Add a devicePixelRatio getter and setter to index.js
  • Change all uses of browser.devicePixelRatio to config.devicePixelRatio
  • 🍰

@kkaefer
Copy link
Member

kkaefer commented Dec 4, 2019

Copied from #9014:

The problem with devicePixelRatio is that it's a global property that could also affect other maps on the same page. Instead of messing with devicePixelRatio, we could introduce a pixelRatio property on the map constructor that defaults to devicePixelRatio, but that also allows passing in other properties to render high resolution maps. We could also seed its initial value based on el.clientWidth / el.getBoundingClientRect().width.

@pakastin
Copy link
Contributor

pakastin commented Feb 18, 2020

I'm handling custom devicePixelRatio like this:

Object.defineProperty(browser, 'devicePixelRatio', {
  get () {
    return window.devicePixelRatio * scale;
  }
});

Here's details: #9014

And here's it in use: https://aviamaps.com/map (click print button to see map scaled)

@fionawhim
Copy link

I’d like to take a stab at implementing the pixelRatio property on the map constructor, if you’d accept that as a PR.

We’re looking to get consistent pixel ratios when using toDataURL on the Mapbox canvas, regardless of the screen that the browser is running on.

@danielwhatmuff
Copy link

+1 for this. We are seeing major performance issues when running on a 27inch 5k screens - almost unusably slow.

@leonardoviada
Copy link

Any update?

@beniaminrychter
Copy link

Any updates on this? That would be very nice feature!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 🍏 good first issue performance ⚡ Speed, stability, CPU usage, memory usage, or power usage
Projects
None yet
Development

Successfully merging a pull request may close this issue.