Introduce generic Output class
ClosedPublic

Authored by romangg on Mar 29 2018, 2:37 PM.

Details

Summary

In order to separate high-level properties of individual outputs from
hardware-specific ones and access these, introduce a new generic class Output.

Also make the DrmOutput class directly a child class of this generic class.

The long-term goal is to get rid of the Screens global object on Wayland and
instead directly work with Output objects on compositing level.

This should enable us long-term to do direct scanout to hardware planes, what
I predict needs this generic output representation at one point.

Test Plan

Manually.

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
romangg updated this revision to Diff 32268.Apr 16 2018, 9:44 AM
  • Merge branch 'master' into outputGeneralize
  • No protected variables
romangg updated this revision to Diff 32269.Apr 16 2018, 9:59 AM

Arc retry with rebase.

romangg updated this revision to Diff 32270.EditedApr 16 2018, 10:01 AM

Arc retry.

Note to self: arc diff <parent branch> --update Dxxxxx

romangg updated this revision to Diff 32279.Apr 16 2018, 1:21 PM
  • Rename getter functions
romangg marked 2 inline comments as done.Apr 16 2018, 1:30 PM

Is there something else to do? Otherwise I would like to merge the series this week.

If you don't check qobject_cast against nullptr it's better and faster to use static_cast.

zzag added inline comments.Apr 30 2018, 6:20 PM
plugins/platforms/drm/drm_output.cpp
90–91

Is it safe? Shouldn't you test the pointer for validity?
Also, it looks like DrmOutput deletes objects that it doesn't own. Is it a good idea?

plugins/platforms/drm/drm_output.h
78

virtual is redundant. It's already virtual.

romangg added inline comments.Apr 30 2018, 6:51 PM
plugins/platforms/drm/drm_output.cpp
90–91

You can delete a nullptr.

But in general yes, the waylandOutput() and waylandOutputDevice() data fields should be deleted on Output level. I just wanted to keep the logical changes minimal and delete them at the same position as before. But yea, accepting this logical change in this case probably makes more sense.

romangg updated this revision to Diff 33434.May 1 2018, 7:50 PM
  • Rebase on master
  • Remove redundant virtual keyword
  • Cleanup Output fields in Output destructor
  • static_cast instead of qobject_cast
romangg marked 3 inline comments as done.May 1 2018, 7:52 PM
romangg added a comment.EditedMay 4 2018, 12:25 PM

Ping for this one and D11782.

Currently this one still has two protected member variables m_outputs and m_enabledOutputs. But I would like to merge it as it is and later on change these to become private because the stack of changes is currently too large to manage without hassle.

XdgOutput should be managed in this class.

If you have to resolve conflicts you may as well move it whilst you rebase.
Otherwise you can ship this and we'll deal with it after.

output.h
66 ↗(On Diff #33434)

Don't refer to anything in code/comments as scaled.

It could mean scaled from logical to device or could mean scaled from device to logical.

romangg updated this revision to Diff 35361.Jun 1 2018, 6:10 PM

Rebase on master. Include XdgOutput changes.

romangg added inline comments.Jun 1 2018, 6:13 PM
output.h
66 ↗(On Diff #33434)

Ok, let me change this after merge. I forgot it now on the rebase and all the rebasing is nerve wrecking.

As you can see from the review comments I'm not too happy with the constant need of static_cast. To me that always looks wrong. If one needs to cast something is wrong in the design. I think this can be changed so that in the subclasses there is no need to static_cast or at least have the static_cast in a save way combined with static_asserts to make sure that it is save. The problem with static_cast is that further refactoring could break it really bad. It might still compile but fail at runtime. Just like a c-cast it kind of has a smell.

Also what I really don't want to see in one of our central abstractions is the usage of protected variables. Which is part of the problem with the casts here. If there variables would be inaccessable you would not cast. You would have looked for a different solution. It's one of the small things leading to the problems with protected variables and why they should be avoided.

output.h
51 ↗(On Diff #35361)

I'm missing a ctor in this class. If you don't specify the default ctor is added which is not typical for a QObject derived class. You want to have a ctor taking a QObject *parent = nullptr argument.

131–134 ↗(On Diff #35361)

I have problems understanding the ownership concept of these pointers. On the one hand they are QPointer, which would indicate they are owned by another class (otherwise QPointer just doesn't make any sense, the only advantage is that it tracks when a QObject gets destroyed), on the other hand the pointers are passed in as raw pointers and last but not least they are deleted in the dtor.

If you want to have ownership of the pointers in this class (the delete in dtor) I suggest using a smart pointer which supports this concept (e.g. std::unique_ptr), if you want to have shared pointers, you could use std::shared_ptr or QSharedPointer.

platform.h
524

there is no such thing as a protected variable in modern object oriented code. Variables are always private. If you need to access it should be through getters and setters. Please see for example https://softwareengineering.stackexchange.com/questions/162643/why-is-clean-code-suggesting-avoiding-protected-variables

Also I'm not sure whether you are doing yourself a favor with how you did this. By making the variables protected and accessing them from the subclass, you have to constantly to static_cast. This looks like suboptimal object oriented design. If you would pass the m_outputs from subclass to parent class, you could pass the QVector<DrmOutput*> for the QVector<Output*> (it downcasts just fine) and you would not need to cast from the subclasses by either keeping the vector there as well or do something nice here like:

template <typename T>
QVector<T> outputs() const
{
    static_assert(std::is_base_of<Output, T>::value, "Template T must be derived from Output");
   return static_cast<QVector<T*>>(m_outputs);
}

(code is not compile checked)

plugins/platforms/drm/drm_backend.cpp
148

static_cast? That sounds wrong. I doubt you need it. Just do:

auto o = *it;
159

same here, static_cast looks wrong

188

here as well the static cast looks like not needed

216

and here as well

391

I don't understand why you changed from QVector<DrmOutput*> to QVector<Output*>. The list still gets filled with DrmOutput and afterwards you make your life hard by having to static_cast back to DrmOutput.

419

Please no static_casts. Either change the signature of outputRemoved signal or do a safe cast like qobject_cast or dynamic_cast. Static_cast is evil :-)

447

If connectedOutputs would still be QVector<DrmOutput*> you would not need the cast here.

494–495

same here. The change would not be needed if the signature were not changed.

516

please a safe cast like qobject_cast or dynamic_cast

528

same here

532

and here

plugins/platforms/drm/egl_gbm_backend.cpp
147

Does the class need a DrmOutput or could it be changed to operate on Output instead?

plugins/platforms/drm/scene_qpainter_drm_backend.h
50 ↗(On Diff #35361)

just as an idea: maybe change name of the new Output to AbstractOutput to not need to have to rename here. I personally like to call abstract classes AbstractOutput. But I leave that to you, it's just an idea I got while seeing this change.

romangg added inline comments.Jun 6 2018, 3:40 PM
output.h
51 ↗(On Diff #35361)

Hmm, this was similar to how DrmOutput was. But I will add one.

131–134 ↗(On Diff #35361)

The QPointers were also copied from DrmOutput. Were they used differently there? I wanted to copy as much semantic as possible without changes to minimize risk for regressions with this large patch.

The objects are created in the Display class of KWayland, which shares ownership and for example KWayland::Server::Display::createOutput only returns a raw pointer. I assume that's why these were QPointers?

platform.h
524

As mentioned earlier I wanted to keep these protected variables in this patch series as an intermediate solution and change them later in private variables with appropriate getters and protected setters. The reasons are to not again do massive rebasing of dependent diffs and keep the semantic changes in this patch here low.

you could pass the QVector<DrmOutput*> for the QVector<Output*> (it downcasts just fine)

This is not working for me. I have to cast the vector elements individually.

I don't want to keep the vector in parent and child class, so I don't have to keep two vectors in sync all the time. The templated function is a nice idea for a protected getter. The problem is that at the moment the child classes assign the vectors though. So one need also several setter functions.

I have now decided to put the QVectors into the child classes of Platform and copy the vector to QVector<AbstractOutput> if outputs() is called. This seems currently like the most straightforward solution.

romangg updated this revision to Diff 35687.Jun 6 2018, 3:40 PM
  • Add ctor to Output
  • Save outputs in Platform subclasses directly
  • Rename Output to AbstractOutput
  • Use std::copy for outputs getters
  • Rename output files to abstract_output
romangg updated this revision to Diff 35690.Jun 6 2018, 4:43 PM
romangg marked 11 inline comments as done.

Small cleanup

davidedmundson added inline comments.Jun 29 2018, 1:27 PM
platform.h
426

What's this method going to be for? It's currently unused

Guessing at use cases, you'd probably want to sync it with Screens, at which point you're going to want something more like

AbstractOutput* Screens::nativeOutput(int)

which would make this a non-issue.

romangg added inline comments.Jun 29 2018, 5:29 PM
platform.h
426

The methods are used in the follow-up patches. The sync with Screens is done already, but in the end we want to get rid of the Screens class and not depend on it through a AbstractOutput getter.

davidedmundson requested changes to this revision.Jul 4 2018, 9:21 PM

From IRC today.

<d_ed> OutputScreens is exactly how I pictured it would be, size(int index); scale(int index)
<d_ed> is there any benefit to returning a list rather than a virtual int count() virtual Ouput* outputAt(int); ?
<d_ed> it would resolve the discussion, and it's a common pattern for models

<romangg> d_ed: I thought about moving into this direction in the future (but then directly in Platform with an Platform::outputAt(int) ), but I first would like to land the series, so I don't have to change all the patches again and rebase the dependent ones which I find difficult together with Phab/arc.


If you're thinking of moving in that direction we should move in that direction.
I don't buy the rationale that we should merge first. Phab can be a slight pain, but it's not that bad.

This revision now requires changes to proceed.Jul 4 2018, 9:21 PM

From IRC today.

<d_ed> OutputScreens is exactly how I pictured it would be, size(int index); scale(int index) 
<d_ed> is there any benefit to returning a list rather than a virtual int count() virtual Ouput* outputAt(int); ? 
<d_ed> it would resolve the discussion, and it's a common pattern for models
 
<romangg> d_ed: I thought about moving into this direction in the future (but then directly in Platform with an Platform::outputAt(int) ), but I first would like to land the series, so I don't have to change all the patches again and rebase the dependent ones which I find difficult together with Phab/arc.

If you're thinking of moving in that direction we should move in that direction.
I don't buy the rationale that we should merge first. Phab can be a slight pain, but it's not that bad.

One disadvantage of this approach is that we double the number of virtual getters from 2 to 4 (outputAt, enalbedOutputAt, outputsCount, enabledOutputsCount). Another one is that we then can only loop by integers and not range-based or with iterators.

Because of these disadvantages this approach is in my opinion not that much better than the current solution, such that it demands a new revision of this diff and all dependent ones. This factors in the uncertainty if this other solution is really better and the avoidable overhead of rebasing the stack once again. Doing it in a separate future patch with a discussion specific to the issue would be more simple and in my opinion also more fruitful because we can concentrate the discussion on this specific issue and have the direct comparison between the then landed current solution of a vector copy and the virtual count / element getters. Maybe someone else will then also chime in who knows of an overall better solution for working with inheritance relations in containers.

Is this a valid rationale for you?

davidedmundson commandeered this revision.Jul 19 2018, 2:05 PM
davidedmundson edited reviewers, added: romangg; removed: davidedmundson.

Modded slightly so we have the conversion wrapper in
only one place for all backends and keep the typecast
implicit.

It features a template which means MF will love it ;)

@Roman
Check you're ok with the mod, and then I'll ack the rest.

  • Fixup DRM cursor hotspot after rebase merge
zzag added inline comments.Jul 19 2018, 3:00 PM
abstract_output.cpp
63
return m_physicalSize.transposed();
abstract_output.h
27–32

Nitpick: it would be great to sort them.

platform.h
71

Why do we need it?

72

typename T

428

I think one could do just

return {};

@Roman
Check you're ok with the mod, and then I'll ack the rest.

Wasn't so sure if it really makes it better and not just more complicated. But yea, this is a neat trick. I'm ok with it. Should I accept it and you push or commandeer again and you accept then?

romangg added inline comments.Jul 19 2018, 4:27 PM
platform.h
68

Is the KWIN_EXPORT really needed?

romangg commandeered this revision.Jul 20 2018, 11:39 AM
romangg edited reviewers, added: davidedmundson; removed: romangg.

David told me to commandeer back.

platform.h
71

Backends have vector<Derived> in their private implementations.

We want to have a
virtual vector<Base>() in the

You can't nicely cast a list, and someone above didn't want to unnicely cast the list.
Roman had effectively this code in all backends.

This new class does the list upcasting in a more implicit and central way.

zzag added inline comments.Jul 20 2018, 12:00 PM
platform.h
71

I'm about default constructor. We don't need it, right?

platform.h
71

Oh I see, sorry.

There's a base implementation in platform.cpp which returns an always empty list.
Without this constructor that line becomes quite messy.

zzag added inline comments.Jul 20 2018, 2:27 PM
platform.h
71

There's a base implementation in platform.cpp which returns an always empty list.
Without this constructor that line becomes quite messy.

Why?

Compiler will generate default constructor(and other special member functions) for us. Also, you could return {}. :-)

davidedmundson added inline comments.Jul 20 2018, 3:09 PM
platform.h
71

Compiler will only implicitly declare a default constructors if I haven't made any other custom constructors, so not in this case.

/home/david/projects/kde5/src/kde/workspace/kwin/kcmkwin/kwinrules/../../platform.h:432:24: error: no matching function for call to ‘KWin::Outputs::Outputs()’
         return Outputs();

You are correct that I could change it to Outputs() = default; and use the generated definition, but it still needs exlicitly declaring.

Just changing the return value to {} instead of the default constructor won't work.

The compiler would have to guess the template of the "QVector<T> other" from our inner constructor which it can't do, even when the list is empty.

/home/david/projects/kde5/src/kde/workspace/kwin/platform.h: In member function ‘virtual KWin::Outputs KWin::Platform::outputs() const’:
/home/david/projects/kde5/src/kde/workspace/kwin/platform.h:428:17: error: could not convert ‘<brace-enclosed initializer list>()’ from ‘<brace-enclosed initializer list>’ to ‘KWin::Outputs’
         return {};

A default constructor and {} works, but that's not really any better.

zzag added inline comments.Jul 20 2018, 3:45 PM
platform.h
71

Oh, yeah, you're right. I forgot that if any constructor is declared, then default constructor is not generated. Sorry, my bad.

Anyway, let's stick with =default.

romangg marked an inline comment as done.Jul 20 2018, 4:05 PM
romangg added inline comments.
abstract_output.cpp
63

Ok. On commit.

abstract_output.h
27–32

Ok. On commit.

platform.h
72

Either is fine.

davidedmundson accepted this revision.Jul 23 2018, 9:06 AM
davidedmundson added inline comments.
platform.h
72

Both do the same, but typename is the new "correct" one. I'm just very ancient.
Please can you change it for me when merging

This revision is now accepted and ready to land.Jul 23 2018, 9:06 AM
romangg added inline comments.Aug 30 2018, 8:49 PM
platform.h
74

This must be a call to resize. Otherwise std::copy doesn't copy anything.

zzag added inline comments.Aug 31 2018, 7:44 AM
platform.h
74

No, std::copy has to copy "to" std::back_inserter.

anthonyfieroni added inline comments.Aug 31 2018, 7:49 AM
platform.h
74

Yes, it should be resize for sure https://en.cppreference.com/w/cpp/algorithm/copy

zzag added inline comments.Aug 31 2018, 7:57 AM
platform.h
74

Could you please explain why? (I don't see any reference on that page why we have to resize vectors before copying)

anthonyfieroni added inline comments.Aug 31 2018, 8:21 AM
platform.h
74

Sure it has:

template<class InputIt, class OutputIt>
OutputIt copy(InputIt first, InputIt last, 
              OutputIt d_first)
{
    while (first != last) {
        *d_first++ = *first++;
    }
    return d_first;
}

There is no back_inserter here and never have., you can try it on online compiler.

zzag added inline comments.Aug 31 2018, 8:44 AM
platform.h
74

std::copy(other.constBegin(), other.constEnd(), std::back_inserter(*this));

I don't see anything wrong with this.

romangg added inline comments.Aug 31 2018, 9:10 AM
platform.h
74

Does using std::back_inserter have advantages over calling resize?

platform.h
74

It has the advantage that you don't initialise the space with the default value only to replace it.

This revision was automatically updated to reflect the committed changes.
romangg marked 7 inline comments as done.