Merge lp://qastaging/~compiz-team/compiz/compiz.performance_1040478 into lp://qastaging/compiz/0.9.8

Note: I haven't updated the abi version yet. Will do that tomorrow.

PASSED: Continuous integration, rev:3375
http://jenkins.qa.ubuntu.com/job/compiz-ci/69/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-ci/./build=pbuilder,distribution=quantal,flavor=amd64/69/console

Wow, this is much bigger than it really should be, by a factor of a least 10.

Initial thoughts:

1. Indenting for four levels of namespace is a bit crazy. If you really need four then please merge all of these into a single line:

namespace compiz
{
    namespace opengl
    {
        namespace fb
        {
            namespace impl
            {

2. FramebufferDirectDraw is not a helpful name for a class. That seems to tell me what it does (verb), not what it is (noun). Please try to think of a better name.

BTW, which graphics drivers have been tested so far? I will probably test the rest...

3. FIXME needs removing from screen.cpp

        // FIXME: does not work if screen dimensions exceed max texture size
        // We should try to use glBlitFramebuffer instead.
        gScreen->glPaintCompositedOutput (screen->region (), scratchFbo, mask);

> Wow, this is much bigger than it really should be, by a factor of a least 10.
>
> Initial thoughts:
>
> 1. Indenting for four levels of namespace is a bit crazy. If you really need
> four then please merge all of these into a single line:
>
> namespace compiz
> {
> namespace opengl
> {
> namespace fb
> {
> namespace impl
> {
>

We can probably remove the "fb" namespace.

My understanding is that compiz::plugin and compiz::plugin::impl was more or less the convention now.

> 2. FramebufferDirectDraw is not a helpful name for a class. That seems to tell
> me what it does (verb), not what it is (noun). Please try to think of a better
> name.

Possible alternatives:

 * RectangleFramebufferPainter
 * DirectFramebufferPainter (yes, I know these are er's)

I don't think its possible to give this class a noun-name as it describes a process. Maybe it could exist better as a function?

I've tested it on nvidia, nouveau and intel.

Hmm, how do I know it's working? On my benchmark system I'm seeing zero change in performance :(

Yes, if it can be a function then it probably should be.

This is over-complicated:

    static const struct FilterToGLFilter filterToGLFilter[] =
    {
        { cglfb::Fast, GL_NEAREST },
        { cglfb::Good, GL_LINEAR }
    };

    if (!mask)
        return;

    GLbitfield glMask = 0;

    for (unsigned int i = 0; i < infoToGLInfoSize; ++i)
    {
        if (mask & infoToGLInfo[i].info)
            glMask |= infoToGLInfo[i].mask;
    }

Should just be:

    GLbitfield glMask =
        (mask & cglfb::Fast ? GL_NEAREST : 0) |
        (mask & cglfb::Good ? GL_LINEAR : 0);

* "Platform" is a redundant word and possibly poorly chosen. Not sure. All the code is for one platform or another...

* priv->mMyMemberName is an over-complication. It's obviously a member by virtual of being on the right hand side of "priv->". So they should probably just be priv->myMemberName.

* Please fit your code into 80 columns. If it doesn't fit then that's a good sign that it needs fixing.

* GLXFramebuffer::blit should use GL_NEAREST for performance. Not GL_LINEAR.

GL::blitFramebuffer is always NULL (!?)

On Wed, 12 Sep 2012, Daniel van Vugt wrote:

> * "Platform" is a redundant word and possibly poorly chosen. Not sure. All the code is for one platform or another...
>

Well, the "platform" in this case is either EGL or GLX. What would you
call it?

> * priv->mMyMemberName is an over-complication. It's obviously a member by virtual of being on the right hand side of "priv->". So they should probably just be priv->myMemberName.
>

Likely, I can change that.

> * Please fit your code into 80 columns. If it doesn't fit then that's a good sign that it needs fixing.
>

Clean Code suggests 120 columns for C++. Its not feasible to stick to the
80 line rule for C++ where namespaces are in use and you have descriptive
function names.

Any ideas on what to call the FramebufferDirectDraw class? Or should we
change both that and PlatformFramebuffer to be boost::function objects?

>
> --
> https://code.launchpad.net/~compiz-team/compiz/compiz.performance_1040478/+merge/123798
> Your team Compiz Maintainers is subscribed to branch lp:compiz.
>

No wonder there was no change in performance :)

On Wed, 12 Sep 2012, Daniel van Vugt wrote:

> * GLXFramebuffer::blit should use GL_NEAREST for performance. Not GL_LINEAR.

Yep, nice catch I got the two confused.

> --
> https://code.launchpad.net/~compiz-team/compiz/compiz.performance_1040478/+merge/123798
> Your team Compiz Maintainers is subscribed to branch lp:compiz.
>

On Wed, 12 Sep 2012, Daniel van Vugt wrote:

> GL::blitFramebuffer is always NULL (!?)

No. it isn't.

GL::blitFramebuffer is to null for the initial "not supported" state and
then set to getProcAddress ("glBlitFramebufferEXT") when
GL_EXT_blit_framebuffer is in the opengl extensions.

> --
> https://code.launchpad.net/~compiz-team/compiz/compiz.performance_1040478/+merge/123798
> Your team Compiz Maintainers is subscribed to branch lp:compiz.
>

> Hmm, how do I know it's working? On my benchmark system I'm seeing zero change
> in performance :(

Its not a magical performance cure, and really depends on what the implementation is doing.

If fill rate is already maxed out on your platform then it isn't going to help too much. Also if your platform implements it in terms of a texture operation it won't work.

Check if its working by changing one of the co-ordinate values in the glBlitFramebuffer call and you'll get a broken system.

It is always NULL. Looks like you haven't pushed all your changes:

grep -r blitFramebuffer *
plugins/opengl/include/opengl/opengl.h: extern GLBlitFramebufferProc blitFramebuffer;
plugins/opengl/src/screen.cpp: GLBlitFramebufferProc blitFramebuffer = NULL;
plugins/opengl/src/screen.cpp: if (!GL::blitFramebuffer)
plugins/opengl/src/screen.cpp: (*GL::blitFramebuffer) (src.x1 (), // sx0

And on that note, it appears GLXFramebuffer::blit is never called for some reason.

OK, I have left too many comments now. Please try to address most of them then resubmit without all this history.

PASSED: Continuous integration, rev:3376
http://jenkins.qa.ubuntu.com/job/compiz-ci/71/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-ci/./build=pbuilder,distribution=quantal,flavor=amd64/71/console

I'm going to assume this is still in progress. Please set back to Needs review when ready.

Tested r3376. GL::blitFramebuffer is now called and it works. Unfortunately no measurable speed-up on intel.

Please review the other comments, fix and resubmit later...

r3379 has a simple conflict with lp:compiz

Test results from r3379:

intel: No difference whatsoever.
radeon: No measurable difference. However expo zoom is slow and stuttery with this branch. Not with trunk.
fglrx: Significantly lower benchmark results, but not visibly different.

So far it looks like this is something we should maybe target for next cycle.

Retested at low resolution (1024x768) whereas the above tests were at 1920x1200.

At low-res the results are identical, except:
radeon: expo zoom does not stutter at low res. Only high res.
fglrx: is very erratic and often slower at low res than high res. Maybe ignore fglrx results for now.