Implement software cursor in OpenGL backend
ClosedPublic

Authored by Kanedias on Jun 11 2017, 7:33 PM.

Details

Summary

Implement software cursor in OpenGL backend

This change is needed for Wayland screen recording apps to work
correctly. With this change the cursor is actually visible using GBM
buffer passing protocol.

Previously OpenGL backend did not support software cursor. If launching
in DRM/OpenGL mode with flicked on software cursor it only rendered
scene, but not the cursor image.

Diff Detail

Repository
R108 KWin
Lint
Lint Skipped
Unit
Unit Tests Skipped
Kanedias created this revision.Jun 11 2017, 7:33 PM
Restricted Application added a project: KWin. · View Herald TranscriptJun 11 2017, 7:33 PM
Restricted Application added subscribers: KWin, kwin, plasma-devel. · View Herald Transcript

Hi Martin, David.

This is my first change in OpenGL-related stuff, where I don't yet have firm ground.
I've tested this change on DRM session in my vt2 and it works, but I have some concerns,

  • Is this the correct way of doing such stuff?
  • Should cursor update be included in damage regions?
  • How can I write an autotest for it?

Is this the correct way of doing such stuff?

Pretty much.
Seems a sensible patch, screen recording aside we could be using a GL on a different backend that can't do planes.

The obvious optimisation is that we don't need to make the new cursor texture every time, but it can be a member var reset on cursorChanged()

Should cursor update be included in damage regions?

I would have thought so. Though I can't see where scene_qpainter does it. (could be a bug there?)

How can I write an autotest for it?

A lot of the GL stuff isn't autotested. and even if you did write a test, our current CI infrastructure couldn't actually run it.

scene_opengl.cpp
705 ↗(On Diff #15364)

i don't think you need these 3 lines, you're not doing anything with this vbo.

You do need vertex info from somewhere, but GLTexture has its own vbo.

Kanedias updated this revision to Diff 15396.Jun 12 2017, 8:27 PM
  • Made cursor texture local var with lazy-init
  • Made changes to support cursor shape update
  • Added cursor hotspot handling
  • Removed redundant vbo calls
Kanedias updated this revision to Diff 15397.Jun 12 2017, 8:30 PM

Whoops... :)

Kanedias marked an inline comment as done.Jun 12 2017, 8:34 PM

I'm still unsure about cursor damage region (digging through cursor-related signals now)

Cursor itself works fine, tested on kms Mesa 17.1.2 amdgpu (and in KRfb session)

scene_opengl.cpp
705 ↗(On Diff #15364)

Thanks, ripped it off

davidedmundson accepted this revision.Jun 12 2017, 8:37 PM
This revision is now accepted and ready to land.Jun 12 2017, 8:37 PM
Kanedias marked an inline comment as done.Jun 18 2017, 8:45 PM

@graesslin , are you here? Can you look at this submission please if you have time?

luebking added inline comments.
scene_opengl.cpp
699 ↗(On Diff #15397)

The entire head should probably be in some init function, not in every paint call.
If you go for a lazy init, you should seekt to prevent double connects (but I don't know whether Qt::UniqueConnection works with functors), but I'd discourage that approach, because it prevents shortcutting the function if there's no cursor image (ie. "!m_cursorTexture")

709 ↗(On Diff #15397)

At this time, this seems superfluous, because you fetch the current image with every paint call anyway.

711 ↗(On Diff #15397)

There's no .isNull() test here - in contrast to the assignment some lines above

734 ↗(On Diff #15397)

This needs a comment from Martin, but you might have to glGet with GL_BLEND_SRC and GL_BLEND_DST as well as glIsEnabled(GL_BLEND)

Kanedias added inline comments.Jun 18 2017, 9:18 PM
scene_opengl.cpp
699 ↗(On Diff #15397)

Will fix that

734 ↗(On Diff #15397)

You mean, to check whether BLEND was enabled prior to calling this func?

luebking added inline comments.Jun 18 2017, 9:20 PM
scene_opengl.cpp
734 ↗(On Diff #15397)

You might have to - and in addition possibly reset the actual blend function from before. Please wait for Martins comment on this, idk which assumptions can be safely made here.

Kanedias updated this revision to Diff 15554.Jun 18 2017, 9:47 PM

Fixes for lazy-init (thanks T.L.)

Kanedias updated this revision to Diff 15555.Jun 18 2017, 9:48 PM

Whoops, wrong patch

Kanedias marked 3 inline comments as done.EditedJun 18 2017, 9:49 PM

Fixed lazy-init, @luebking have a look now

luebking added inline comments.Jun 19 2017, 5:30 AM
scene_opengl.cpp
713 ↗(On Diff #15555)

You still update the cursor texture with every paint() - there should be no need to hook this signal.
Actually, the patch got much worse:

a) you're fetching the image and reset the texture with each! paint()
b) you're creating a nerw connection to cursorChanged() with each! paint()

Kanedias added inline comments.Jun 19 2017, 7:25 AM
scene_opengl.cpp
713 ↗(On Diff #15555)

How come! There's a condition on !m_cursorTexture for that, no? Or is Cursor::cursorChanged itself called on every paint?

luebking added inline comments.Jun 19 2017, 11:34 AM
scene_opengl.cpp
713 ↗(On Diff #15555)

Sorry, I missed that the "if (!m_cursorTexture) {" condition still covers these calls - to my excuse: inline comments in phabricator are rather messy :-P

So the above worries are void - the only remainders are
a) possible glBlend restorage
b) null'ing m_cursorTexture on empty images to skip rendering (no idea how relevant that is) ie. not using the pointer vaildity as indicator for a required reset.

aacid added a subscriber: aacid.Jun 25 2017, 4:07 PM

If this indeed needs fixing, can someone please mark it as "request changes" so it doesn't show up in the list of "patches that have been accepted but have not been commited"?

graesslin requested changes to this revision.Jun 25 2017, 7:14 PM

Looks good to me. Just a minor comment regarding where to put the new virtual method. Setting to reject changes as requested by Albert :-)

scene_opengl.cpp
682–685 ↗(On Diff #15555)

Instead of adding an empty method you could make it pure virtual

scene_opengl.h
103 ↗(On Diff #15555)

Given that we have also a method called paintCursor in SceneQPainter, I suggest to move it up to Scene directly and mark the call in SceneQPainter as override.

141 ↗(On Diff #15555)

you don't need to readd the virtual keyword when using override.

This revision now requires changes to proceed.Jun 25 2017, 7:14 PM
Kanedias updated this revision to Diff 15860.Jun 25 2017, 8:18 PM
Kanedias edited edge metadata.
Kanedias marked an inline comment as done.

Review comments fixes

Kanedias marked 2 inline comments as done.Jun 25 2017, 8:19 PM
Kanedias added inline comments.
scene_opengl.cpp
682–685 ↗(On Diff #15555)

That moment when I noticed we have no SceneOpenGL instantiations. Tx!

scene_opengl.h
141 ↗(On Diff #15555)

Should we heal other methods suffering from this? I can prepare such patch.

graesslin added inline comments.Jun 26 2017, 4:26 AM
scene_opengl.h
141 ↗(On Diff #15555)

No, we don't change the existing code. Only new code gets override.

graesslin accepted this revision.Jun 26 2017, 4:27 AM
This revision is now accepted and ready to land.Jun 26 2017, 4:27 AM
This revision was automatically updated to reflect the committed changes.
Kanedias marked 2 inline comments as done.