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.

Sign up for GitHub

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

Jump to bottom

Downgrading webGL precision if target hardware does not support highest #4433

Merged
merged 2 commits into from
Oct 31, 2017
Merged

Downgrading webGL precision if target hardware does not support highest #4433

merged 2 commits into from
Oct 31, 2017

Conversation

ozooner
Copy link
Contributor

@ozooner ozooner commented Oct 31, 2017

So there are multiple ways to go about fixing the precision change, this is the one I came up with and like the most. If you think there's more elegant way of doing it - let me know and I can refactor and test it.

@ozooner ozooner mentioned this pull request Oct 31, 2017
Closed
asturur
asturur reviewed Oct 31, 2017
View reviewed changes
src/filters/base_filter.class.js Outdated
@@ -20,6 +20,8 @@ fabric.Image.filters.BaseFilter = fabric.util.createClass(/** @lends fabric.Imag
*/
type: 'BaseFilter',

webGlPrecision: 'highp',
Copy link
Member

Choose a reason for hiding this comment

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

can we make this fabric global property? as we have fabric.webGlprecision. texture size is also there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on it

asturur
asturur reviewed Oct 31, 2017
View reviewed changes
src/filters/webgl_backend.class.js Outdated
@@ -19,6 +37,19 @@
if (gl) {
fabric.maxTextureSize = gl.getParameter(gl.MAX_TEXTURE_SIZE);
isSupported = fabric.maxTextureSize >= tileSize;
var precision;
var precisions = ['highp', 'mediump', 'lowp'];
Copy link
Member

Choose a reason for hiding this comment

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

are there outside implementation that works but do not support lowp? how do they work at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know actually - I just wrote it to be as safe as possible for any scenario.

@asturur
Copy link
Member

asturur commented Oct 31, 2017

Do you have any webgl skill that go beyond the complete beginner that i am?
I would like someone to take a look at the current webgl implementation built for fabric.
There is also a lanzcos resize filter in an open PR that change something in the backend itself ( like switching between textures of different sizes )

asturur
asturur reviewed Oct 31, 2017
View reviewed changes
src/filters/webgl_backend.class.js
}
return true;
}

Copy link
Member

Choose a reason for hiding this comment

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

unsure about the position of this function.
fabric.isWebglSupported can be overridden by a dev with something more fancy.

testPrecision is here with a jsdoc, but will not be catched is not attached to any object, and can't be used from outside or from an overridden isWebglSupported. can you think of something better than calling it fabric.testPrecision?

does not block merging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I don't follow. Do you want to move testPrecision to fabric object as well?
Like all developers, I am not good at naming things :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I should move the testprecision function inside isWebGlSupported function?

Copy link
Member

Choose a reason for hiding this comment

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

no i would leave it here. i was just wondering if it makes sense to have a non accessible function that can't be reused if a dev change the implementation of isWebglSupported. not a problem at all.

@ozooner
Copy link
Contributor Author

ozooner commented Oct 31, 2017

I changed a bit, now I am no longer handling a scenario where none of the precision are supported - I think if it passes the isWebGlSupported test, at least one precision should be supported as well.

When it comes to WebGL skill, then my entire contact with webgl is from reading your source...

@asturur
Copy link
Member

asturur commented Oct 31, 2017

oh ok, so consider this is my first webgl experience before reusing this code. i have been helped lot from @plainview ( fifty fifty maybe ? )

@asturur
Copy link
Member

asturur commented Oct 31, 2017

everything looks good. i ll wait some minute and merge, just in case you spot something else.

@ozooner
Copy link
Contributor Author

ozooner commented Oct 31, 2017 •
edited
Loading

Maybe I should move the testprecision function inside isWebGlSupported function since it's not used anywhere else as you say?

EDIT: nvm, saw your response

@asturur
Copy link
Member

asturur commented Oct 31, 2017

that is not a big deal. there is added value here, is fine as it is.

@asturur
Copy link
Member

asturur commented Oct 31, 2017

Do you want a fabricjs sticker? do you work with other devs that would stick it on some laptop/hardware?

@ozooner
Copy link
Contributor Author

ozooner commented Oct 31, 2017 •
edited
Loading

Absolutely! 2 other devs beside me...

@ghost
Copy link

ghost commented Oct 31, 2017

Hey @ozooner did you consider already the gl.getShaderPrecisionFormat() method? Was it unsuitable for this use-case?

https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/getShaderPrecisionFormat

@ozooner
Copy link
Contributor Author

ozooner commented Oct 31, 2017

Good catch, I'll try it out!

@asturur
Copy link
Member

asturur commented Oct 31, 2017

what is the performance of that mali gpu anyway? what it can actually filter and at what speed?

@ozooner
Copy link
Contributor Author

ozooner commented Oct 31, 2017 •
edited
Loading

@plainview - so this is what the hardware in question returns for that function:
image

gl.getShaderPrecisionFormat(gl.FRAGMENT_SHADER, gl.LOW_FLOAT)
gl.getShaderPrecisionFormat(gl.FRAGMENT_SHADER, gl.MEDIUM_FLOAT)
gl.getShaderPrecisionFormat(gl.FRAGMENT_SHADER, gl.HIGH_FLOAT)

As you can see, I could use that function to detect if returnObject.precision === 0. But if I go down that path, I don't know if there could be any hardware that would return 0 for highp, but still actually work? What do you think, should I use that function to detect precision support or the one currently in use?

@asturur Amazon is using this GPU, because it is highly optimized for h264 decoding. For fabric I am just using tint filter to paint SVG's once, so it is not a problem for me. I am also using animations - certain animations, such as rotate/scaleX/scaleY work around 20 fps, but animating position properties (left/top) is rather laggy

@ghost
Copy link

ghost commented Oct 31, 2017

According to pyalot a precision of 0 always indicates that a float type is not supported: http://codeflow.org/entries/2013/feb/22/how-to-write-portable-webgl/#how-to-query-fragment-shader-precision

It will also always be 0 for integer types but I don't believe that is something we need to worry about here. I am no expert on this though.

@asturur
Copy link
Member

asturur commented Oct 31, 2017

let's keep what we have.
I'm wondering if using highp make sense for texture that goes maximum to 8k size ( texel size 1/8000 ) and for texture color that at the end are 1/255.

This if fine and introduce the added knowledge that this thing must be checked.

i would like to understand what those numbers mean before using them.

@ozooner if you send me an address i ll send some stickers as soon as i cumulate some shipping ( a whopping 5 request for now, not worth a travel to post office ).

@asturur
Copy link
Member

asturur commented Oct 31, 2017

image

this new travis thing is making me crazy.

@ozooner
Copy link
Contributor Author

ozooner commented Oct 31, 2017

E-mailed my address

@asturur asturur merged commit 1435e86 into fabricjs:master Oct 31, 2017
@asturur asturur mentioned this pull request Nov 19, 2017
Merged
thiagocunha pushed a commit to thiagocunha/fabric.js that referenced this pull request Nov 18, 2019
@ozooner @asturur
Downgrading webGL precision if target hardware does not support highe…
a7c5fc6 
…st (fabricjs#4433)

* Downgrading webGL precision if target hardware does not support highest

* moved webGlPrecision to fabric property
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asturur asturur asturur left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ozooner @asturur