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

Firefox WebGL warning (Alpha-premult and y-flip are deprecated for non-DOM-Element uploads). #2030

Open
jwoodwardtfx opened this issue Jan 3, 2023 · 23 comments
Labels
benchmark Related to testing performance 💰 bounty L Large Bounty, USD 500 bug Something isn't working enhancement New feature or request PR is more than welcomed Extra attention is needed

Comments

@jwoodwardtfx
Copy link
Contributor

When running in Firefox we get the following warning:
WebGL warning: texImage: Alpha-premult and y-flip are deprecated for non-DOM-Element uploads.

Google shows up the following:
https://bugzilla.mozilla.org/show_bug.cgi?id=1400077
mapbox/mapbox-gl-js#5292

Is this an issue or a performance limitation we should be concerned about (as the Firefox developers say this path isn't optimized and should not be used)?

@morgoth
Copy link

morgoth commented Mar 23, 2023

Since the issue is known at least from 2017, vide mapbox/mapbox-gl-js#5292 is there any plan to fix/workaround/address this problem?

@HarelM
Copy link
Collaborator

HarelM commented Mar 23, 2023

Can you please share a reproduction of this issue?
Generally speaking, Firefox has a very low usage statistics when it comes to browsers, but if someone would like to invest in fixing this, please do.
If this is a performance concern I might be able to assign a bounty to it, but I'll need a clear reproduction example first

@HarelM HarelM added the need more info Further information is requested label Mar 23, 2023
@morgoth
Copy link

morgoth commented Mar 23, 2023

You can just open this page in Firefox https://maplibre.org/maplibre-gl-js-docs/example/simple-map/ and look into the console.

I'm using Firefox 110.0 (64-bit) on Ubuntu

@HarelM
Copy link
Collaborator

HarelM commented Mar 23, 2023

Would you be interested in working on this issue?

@HarelM
Copy link
Collaborator

HarelM commented Mar 23, 2023

Thanks for the info!

Assigned L bounty.
Boundy direction: maplibre/maplibre#192

Here's a screenshot of the warning:
image

@HarelM HarelM added bug Something isn't working enhancement New feature or request benchmark Related to testing performance 💰 bounty L Large Bounty, USD 500 PR is more than welcomed Extra attention is needed and removed need more info Further information is requested labels Mar 23, 2023
@morgoth
Copy link

morgoth commented Mar 23, 2023

@HarelM I'm not even an user yet ;-) - just investigating future possibilities for the integration and that was the first "warning" to see the known issue opened since 2017 without addressing.

@IvanSanchez
Copy link
Contributor

I can have a look at this.

I'm having a look at the code paths triggering the warnings, and am currently considering skipping the creation of textures when the atlases are empty. This won't get rid of all the warnings, but it should avoid telling the GPU to allocate 1x1 px textures needlessly.

@HarelM
Copy link
Collaborator

HarelM commented Mar 23, 2023

Would be very nice if you could push this forward. Let me know if you would like me to assign this issue to you.

@IvanSanchez
Copy link
Contributor

@HarelM Sure thing!

Have a look at #2297 - it gets rid of all warnings except for the first one. I'll have to look at the root cause of that one.

@WebMechanic
Copy link

WebMechanic commented Apr 17, 2023

@IvanSanchez thanks for working on this!

Is this on the 3.0 roadtrack only or can it be "backported" to the 2.4 branch?
Firefox userbase in Europe is at least on par with Edge (varies by stats). These warnings are super annoying poluting the console and Mozilla apparently doesn't care.

They show up with the basic-preview or osm-bright styles (local tileserver.org 8.x instance)
also with https://demotiles.maplibre.org/style.json

Is there something a "map server admin" can do to fix these missing sprites/textures?
Hints welcome.

thank you.

@IvanSanchez
Copy link
Contributor

@WebMechanic The #2297 & #2299 PRs are targeting main, which means it'd be into the 3.0 roadmap.

The actual changes are rather minimal, so I guess it'd be possible to backport them. I'd rather not do so unless @HarelM asks me to.

@HarelM
Copy link
Collaborator

HarelM commented Apr 17, 2023

I can't think of a strong argument to backport this into 2.4. we should be releasing 3.0 shortly (hopefully).

@WebMechanic
Copy link

I can't think of a strong argument to backport this into 2.4. we should be releasing 3.0 shortly (hopefully).

If 3.0 is virtually a seamless upgrade from 2.4 w/o any BC breaks, then sure.

However, if 3.0 introduces any significant API changes, we (and likely other "users") might neither have the time nor resources to make our current component code work with this major release.

Looking forward to the 3.0 release.
Thanks.

@IvanSanchez
Copy link
Contributor

@WebMechanic You can check the current changelog for yourself: https://github.com/maplibre/maplibre-gl-js/blob/main/CHANGELOG.md - see changes marked with [breaking]. Those shouldn't have a big impact on the userbase.

@birkskyum
Copy link
Member

birkskyum commented May 19, 2023

Checked the debug pages to make sure - This console warning still appear after the move to webgl2

WebGL warning: texImage: Alpha-premult and y-flip are deprecated for non-DOM-Element uploads.

@HarelM
Copy link
Collaborator

HarelM commented Nov 29, 2023

There is still an open PR which was not merged unfortunately.
If this is something that interests you feel free to push it forward.
Backporting stuff is very time consuming, and we are focusing on latest and greatest main. If someone from the community is interested in spending the time to backport something I'll be the last one to object. :-)

@WebMechanic
Copy link

We eventually upgraded to v3 which went very smooth! 👍🏻

However, I still get these warnings with v3.6.2 (current as of this writing) and the amount of warnings per identical request appears to be pretty random too.

Thanks.

@garma83
Copy link

garma83 commented Mar 27, 2024

confirming that this is not fixed in 3.0.1. Im a bit surprised this is left unfixed. Imho map box should not throw warnings when a user uses it correctly and as documented.

@HarelM
Copy link
Collaborator

HarelM commented Mar 27, 2024

@garma83 the PR is still open and you are welcome to push it forward if this is something that is truly important to you.

@garma83
Copy link

garma83 commented Mar 29, 2024

@garma83 the PR is still open and you are welcome to push it forward if this is something that is truly important to you.

TBH an answer like that is not very helpful IMHO. I have no idea about the code base, nor would it be feasible for me to dedicate time during work hours. Im just flagging it here because it was said that it would be fixed in 3.0

@sbachinin
Copy link
Collaborator

Tried to fix these warnings but failed.
I'll share some insights, in case someone tries to solve this in the future.

  1. I wouldn't recommend to proceed with PR Skip creating textures for empty atlases #2297 because it's only a partial fix and it's kind of misleading because empty atlases aren't the real cause of warnings.
    I tried this solution on several example pages. Warnings disappeared on some pages, simple-map for example. On many other pages console was still full of warnings.
    Apparently the problem is not only with empty atlases but with any atlases.

  2. The actual problem is experssed in the warning itself and I think the only way to fix the problem is to do what Firefox tells us to do.
    Currenly FF is unhappy because, when updating a texture, we call:

gl.pixelStorei(gl.UNPACK_PREMULTIPLY_ALPHA_WEBGL, true);

and then provide a non-DOM-Element source to gl.texImage2D.
Firefox assumes that non-DOM-Element image data should be premultiplied manually by a consuming program instead of delegating it to webgl. FF states that they "do not optimize this path".
That is, doing it manually in theory mustn't lead to performance degradation in FF. Premult algorithm is pretty simple and fast. And it won't be called very often anyway.

So basically a proper solution must be something like (pseudocode):

if (firefox && !isDOMElement(imageData)) {
	imageData = getPremultiplied(imageData); // premultiply manually
} else {
        // current behaviour: delegate premult to webgl
	gl.pixelStorei(gl.UNPACK_PREMULTIPLY_ALPHA_WEBGL, true);
}

Problems with this approach:

a) doing special stuff for Firefox alone is problematic from testing pov. In such case we need a dedicated render tests for FF.

b) real implementation turns out quite messy. It's mostly because there are different kinds of non-DOM-Element data.
It's easy to premultiply "plain data" (an array of numbers). But then there is an interesting case of ImageBitmaps: sometimes ImageBitmaps cause warnings and sometimes they don't. And premultiplication of ImageBitmaps is a different algorithm. Everything is doable but that will be too much code.

@HarelM
Copy link
Collaborator

HarelM commented Apr 3, 2024

Thanks for looking into this @sbachinin!
What other alternatives do we have? Can we change the gl code somehow to use an optimized path in FF using a different API maybe?

@sbachinin
Copy link
Collaborator

At the moment I have no other ideas.
Asked them to issue this warning only once. https://bugzilla.mozilla.org/show_bug.cgi?id=1889315
(because the problem is the amount of warnings first of all, afaics)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Related to testing performance 💰 bounty L Large Bounty, USD 500 bug Something isn't working enhancement New feature or request PR is more than welcomed Extra attention is needed
Projects
None yet
Development

No branches or pull requests

8 participants