Code review comment for lp:~mc-return/compiz/compiz.merge-fix771875-screenshot-overlay-on-screenshots-with-enabled-fbos

Revision history for this message
MC Return (mc-return) wrote :

> I don't think reading off the front buffer is correct. It should actually be
> the back buffer.
>

Of course I tried the back buffer version first -> Did not work.
I also tried to flush, clear, clearcolor, forcing transparency to 1.0, but
nothing worked, if FBOs are enabled... :(

> In any case I'm a little hesitant to ack this right now as I mentioned
> earlier. There are other plugins (namley unity) that assume that the current
> read buffer binding hasn't been touched. The only way they'd be able to know
> whether or not the binding should be switched is by querying OpenGL directly,
> and such queries are usually quite slow.
>

I did try to optimize behaviour by just changing the buffer to front if FBOs are
enabled.
Would it help to set it to back buffer again, after glReadPixels has finished ?

> I'd much rather have a framework to track changes in the buffer bindings, a-la
> something like this: https://code.launchpad.net/~compiz-
> team/compiz/compiz.fix_1040478/+merge/156465 before we do anything here.
>

Okay, no problem -> this can wait. Folks now already have the ability to set the
opacity of the screenshot overlay manually, so they can workaround this nasty
bug in trunk version of Screenshot already.
So no need for any hurry here...

> So - we'll get back to this as soon as the right frameworks are in place to
> enable it properly. Sorry about that :/

No problem at all. This is just the point of the i.

« Back to merge proposal