Differential D7100

Support using ANGLE for OpenGL canvas on Windows (also OpenGL ES 3.0 support)
ClosedPublic
Actions

Authored by alvinhochun on Aug 3 2017, 4:41 PM.

Details

Reviewers
dkazakov
Group Reviewers
Krita
Maniphest Tasks
T6696: Add ANGLE renderer support on Windows
Summary

Part of the changes of the branch alvin/T6696-opengl-angle (up to e3b45ab77508e59e3d9929d3fb212519603f7b7b)

These are the changes for making ANGLE work.

Things to note:

  • The run-once check in KisOpenGL::setDefaultFormat is only to remove a warning message in Qt. There should be no functional differences.
    • The warning message is: Warning: Setting a new default format with a different version or profile after the global shared context is created may cause issues with context sharing.
  • OpenGL ES 3 does not have mandatory support for GL_BGRA, but the extension GL_EXT_texture_format_BGRA8888 supports it and it is present in ANGLE. Fallback for OpenGL ES implementations without GL_EXT_texture_format_BGRA8888 is to use RGBA8 with texture swizzle mask to swap the red and blue channels.
    • The checkerboard falls back to GL_RGBA instead. Since it should be grayscale it shouldn't matter. Correct me if I am wrong.
  • I haven't tried to look for 16-bit integer support in OpenGL ES 3 or ANGLE so I'm using 8-bit integer instead. Might need to fix this to support 10-bit display...
  • I assumed the texture formats I used will map to D3D11 texture formats directly so there shouldn't be any conversion done by ANGLE, but I'm only certain on this with BGRA.
  • If I'm reading correctly, LoD is enabled. I think OpenGL ES 3 does support LoD.
  • Looks like KisOpenGL::hasOpenGL3() would return false. I haven't checked if this does any weird things.
  • OpenGL ES 3 does not support glLogicOp, so the cursor outline is going green again :( I changed the cursor to use a certain blend mode so it shows up better than no blend mode at all.
Test Plan

Just a suggestion:

  • Test on Windows with a PC that is known to do fine with OpenGL, using QT_OPENGL=desktop and make sure it works with OpenGL.
  • Test on Windows with a PC that is known to do fine with OpenGL, with QT_OPENGL not set and check if it uses OpenGL. (Qt might have blacklisted a driver even if it works with Krita, so this might not work 100%.)
  • Test on Windows with a PC that is known to do fine with OpenGL, using QT_OPENGL=angle and make sure it is using the D3D11 renderer with ANGLE.
  • Test on Windows with a PC that is known to not do fine with OpenGL, with QT_OPENGL not set and check if it uses ANGLE automatically (Qt should have it in the built-in blacklist, but if it's not then we have another problem.)
  • Test on Linux with OpenGL.
  • Test on Mac with OpenGL.

For all above configurations, test these:

  • Rendering of a coloured file should work fine, colour channels must be correct (e.g. red and blue channels are not swapped). Check the use of colour profile if possible.
  • Alpha blending of the layers should work.
  • Background checkerboard is rendered properly.
  • Transformation of the canvas works.
  • Image gets updated properly when drawing.
  • Cursor outline is rendered (I know, it's green on ANGLE.)
  • All image pixel formats should render well.
  • Test opening multiple documents, multiple windows, etc.

Diff Detail

Repository
R37 Krita
Lint
Lint Skipped
Unit
Unit Tests Skipped
alvinhochun created this revision.Aug 3 2017, 4:41 PM
alvinhochun edited the test plan for this revision. (Show Details)Aug 3 2017, 6:58 PM
alvinhochun updated this revision to Diff 17753.Aug 5 2017, 2:49 PM
alvinhochun edited the summary of this revision. (Show Details)
alvinhochun edited the test plan for this revision. (Show Details)
dkazakov requested changes to this revision.Aug 7 2017, 3:27 PM

I have tested the patch on NVidia and Intel GPUs and it seem to work fine. There are only two notes:

  1. [minor] Subjectively, I have a feeling that Instant Preview finalizing update is slower in Angle, than in raw openGL. The only cause I could think about is that glGenerateMipmap() works slower on Angle than on openGL. Though I have no proof of is.
  1. [major] See the inlined comment. As far as I can tell, the code will crash if Krita will be compiled without openEXR support and the canvas code will fall-back to Float16 colorspace.
libs/ui/opengl/kis_opengl_image_textures.cpp
652

As far as I remember, we can have a Float16bit color space iff Krita is compiled with openEXR. In other branches f16 format is selected without checks, because the incoming data already have this format, therefore supported. In this fallback case it might cause a crash, if the destination color space will not be found

This revision now requires changes to proceed.Aug 7 2017, 3:27 PM
alvinhochun added a comment.EditedAug 7 2017, 5:11 PM
In D7100#133402, @dkazakov wrote:

I have tested the patch on NVidia and Intel GPUs and it seem to work fine. There are only two notes:

  1. [minor] Subjectively, I have a feeling that Instant Preview finalizing update is slower in Angle, than in raw openGL. The only cause I could think about is that glGenerateMipmap() works slower on Angle than on openGL. Though I have no proof of is.

I cannot tell... for this I'll need to check how ANGLE implements this (and it might not be easy to follow). But I'd be surprised if it isn't mapped to equivalent Direct3D11 feature.

It could also be that the ANGLE that comes with Qt is behind the latest version for more than a year and it might be improved in more recent versions. You can try copying libEGL.dll and libGLESv2.dll from the latest stable/beta version of Chrome/Chromium (make sure to download the 64-bit version, you can find Chromium builds in zip format) and place it within Krita's bin folder (rename existing ones first) to test. (But note that Chrome's DLLs are compiled with MSVC2015 which might contribute to some performance differences so it's not a fair comparison.)

You can also compare with the ones from the official Qt 5.9.1 Windows binaries that uses MSVC2015 but should use the same version of ANGLE, to see if any performance is caused by compiler optimization.

  1. [major] See the inlined comment. As far as I can tell, the code will crash if Krita will be compiled without openEXR support and the canvas code will fall-back to Float16 colorspace.

I didn't know this... But is there a reason why this isn't a compile-time ifdef check? (Or maybe it actually is but I can't see the full code on mobile and I don't remember.)

Also, replied to inline comments and added two.

libs/ui/opengl/kis_opengl_image_textures.cpp
618

@dkazakov What do you think about this, should I change the code to convert it to 16-bit float if openEXR is present to support 10-bit display, or should it always convert to 8-bit integer no matter what?

626

@dkazakov Does 16-bit integer need openEXR or is this also not fine?

652

I see. if there is no openEXR I can fall back to BGRA8 wheb GL_EXT_texture_format_BGRA8888 is present. But if the extension is not there it means I'll have to use RGBA8, and that requires byte swapping... Since the initial target is ANGLE which does have GL_EXT_texture_format_BGRA8888, how about I leave this specific fall back unimplemented for now and put in an assert or something?

rempt added a subscriber: rempt.Aug 7 2017, 6:23 PM
rempt added inline comments.
libs/ui/opengl/kis_opengl_image_textures.cpp
626

No, 16 bit integer doesn't need openexr (or, more accurately, libhalf) -- that is only needed for 16f

alvinhochun added inline comments.Aug 7 2017, 6:57 PM
libs/ui/opengl/kis_opengl_image_textures.cpp
626

Oh, so 32-bit float is actually fine? Converting to 32-bit float is way worse than doing byte swapping for 8-bit so it's not a choice, but if I don't implement the byte swapping now I can perhaps use it as a fallback instead of just making a plain assert (of course still need to be fixed properly if we want to target real OpenGL ES devices in the future).

652

The byte swapping shouldn't be too complicated to implement (a slow non-vectorized operation), but I haven't yet found where to implement it. I don't really have an alternative though.

alvinhochun added a comment.Aug 7 2017, 7:06 PM
In D7100#133402, @dkazakov wrote:
  1. [minor] Subjectively, I have a feeling that Instant Preview finalizing update is slower in Angle, than in raw openGL. The only cause I could think about is that glGenerateMipmap() works slower on Angle than on openGL. Though I have no proof of is.

Could it be the use of PBO? I haven't checked ANGLE's implementation though.

Since you can use VTune (I think my trial has expired) you might be able to find out by profiling.

alvinhochun updated this revision to Diff 17960.Aug 10 2017, 11:55 AM
alvinhochun edited edge metadata.
alvinhochun marked 7 inline comments as done.
alvinhochun edited the summary of this revision. (Show Details)
  • Added qt.qpa.gl logging on Windows
  • Removed the possibly crashing conversions to 16f
  • Left the missing GL_EXT_texture_format_BGRA8888 fallback unimplemented, replaced by a safe assert and leaving the image rendering with blue and red channels swapped
  • The change will be fine for ANGLE
alvinhochun planned changes to this revision.Aug 10 2017, 5:36 PM

Upon further (casual) research, it seems that we might be able to use a swizzle mask to swap the red and blue channels after all. I should be able to implement a fallback this way...

glTexParameter OpenGL ES 3 reference
https://www.khronos.org/opengl/wiki/Texture#Swizzle_mask

alvinhochun updated this revision to Diff 17978.Aug 10 2017, 7:13 PM
alvinhochun edited the summary of this revision. (Show Details)

Add texture format fallback for OpenGL ES 3 implementations without GL_EXT_texture_format_BGRA8888 support by using texture swizzle mask to swap the red and blue channels.

alvinhochun updated this revision to Diff 18100.Aug 13 2017, 5:25 PM

Removed enable logging qt.qpa.gl due to merge from master

alvinhochun edited the summary of this revision. (Show Details)Aug 13 2017, 5:28 PM
alvinhochun edited the summary of this revision. (Show Details)Aug 14 2017, 4:58 PM
dkazakov accepted this revision.Aug 15 2017, 8:24 AM

The patch looks fine and seems to run fine on Linux. Please check my inline comments. I'm not sure they are valid though. Feel free to discard them if you feel the code is correct, especially the one with glDisable()

libs/ui/opengl/kis_opengl_canvas2.cpp
373

Can we check somehow more precisely about XOR op? That is a long story, so it could probably help with AMD and other problems...

Btw, the effect of GL_ONE_MINUS_DST_COLOR looks quite nice :)

424

Shouldn't you also disable GL_BLEND in openGL ES case?

libs/ui/opengl/kis_texture_tile.cpp
42

Could you add a comment telling that it is needed for openGL ES/Angle only? :)

This revision is now accepted and ready to land.Aug 15 2017, 8:24 AM
dkazakov added a comment.Aug 15 2017, 8:25 AM

Btw, I've checked your branch and saw you using foreach. Please use Q_FOREACH instead, just for consistency sake :)

alvinhochun added a comment.Aug 15 2017, 9:29 AM
In D7100#135740, @dkazakov wrote:

Btw, I've checked your branch and saw you using foreach. Please use Q_FOREACH instead, just for consistency sake :)

Ok. It said to use foreach in the HACKING file so I followed that when writing the code.

libs/ui/opengl/kis_opengl_canvas2.cpp
373

glLogicOp is not in the OpenGL ES specs, so if we really want to use XOR we'll need to do that by rendering the canvas onto a texture and use shaders to do it. It seems like a lot of work for a cosmetic change...

GL_ONE_MINUS_DST_COLOR isn't quite close enough to XOR for certain colours, actually, but it's the best I can get.

424

Probably, but both KisOpenGLCanvas2::drawGrid and KisOpenGLCanvas2::drawImage calls glEnable(GL_BLEND) without disabling it at the end. There seems to be little reason to specifically disable it here. Or I can disable it for all three, not a big deal.

libs/ui/opengl/kis_texture_tile.cpp
42

Or rather, just OpenGL ES because I'm certain it won't get used for ANGLE. I'll add to it.

(But I did test this code using ANGLE by commenting out the hasExtension check.)

dkazakov added inline comments.Aug 15 2017, 9:59 AM
libs/ui/opengl/kis_opengl_canvas2.cpp
424

I vote for consistency :)

libs/ui/opengl/kis_texture_tile.cpp
42

Thanks! :)

Revision Contents
Changeset List

Diff 18100

View Options

3rdparty/ext_qt/CMakeLists.txt

Loading...
View Options

3rdparty/ext_qt/gerrit-189539-ANGLE-mingw-fix.patch

Loading...
View Options

3rdparty/ext_qt/qopengldebug-gles.patch

Loading...
View Options

libs/ui/opengl/kis_opengl.h

Loading...
View Options

libs/ui/opengl/kis_opengl.cpp

Loading...
View Options

libs/ui/opengl/kis_opengl_canvas2.cpp

Loading...
View Options

libs/ui/opengl/kis_opengl_image_textures.cpp

Loading...
View Options

libs/ui/opengl/kis_opengl_shader_loader.cpp

Loading...
View Options

libs/ui/opengl/kis_texture_tile.cpp

Loading...