Merge lp:~mc-return/compiz/compiz.merge-fix771875-screenshot-overlay-on-screenshots-with-enabled-fbos into lp:compiz/0.9.10
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Sam Spilsbury | ||||
Approved revision: | 3669 | ||||
Merged at revision: | 3679 | ||||
Proposed branch: | lp:~mc-return/compiz/compiz.merge-fix771875-screenshot-overlay-on-screenshots-with-enabled-fbos | ||||
Merge into: | lp:compiz/0.9.10 | ||||
Diff against target: |
536 lines (+318/-145) 2 files modified
plugins/opengl/include/opengl/opengl.h (+4/-0) plugins/screenshot/src/screenshot.cpp (+314/-145) |
||||
To merge this branch: | bzr merge lp:~mc-return/compiz/compiz.merge-fix771875-screenshot-overlay-on-screenshots-with-enabled-fbos | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
PS Jenkins bot (community) | continuous-integration | Approve | |
Sam Spilsbury | Approve | ||
MC Return | Needs Information | ||
Review via email: mp+160751@code.launchpad.net |
Commit message
Refactor screenshot code to make it clearer and also to allow us to take
screenshots in glPaintOutput as opposed to paint ().
Taking screenshots in paint () was probably correct pre-GLES, but isn't
really correct now - we want to be able to read off of the currently
bound scratch framebuffer where we last painted the frame. Reading off
the frontbuffer is an imprecise operation because the contents of both
buffers are really undefined after a swap. In the case where we are
not painting to a scratch framebuffer object, we'll just end up reading
from the backbuffer anyways, which would be correct to do mid-frame.
Also added the new static const GLenums
DRAW_FRAMEBUFFE
READ_FRAMEBUFFE
(LP: #771875)
Description of the change
\o/
I declare Screenshot to be 100% ready now.
@Sam: Thanks 4 the tip with glReadBuffer. The GL_FRONT version did the
trick. :) &&
thanks for the additional refactoring :)
Sam Spilsbury (smspillaz) wrote : | # |
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:/
> team/compiz/
>
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.
MC Return (mc-return) wrote : | # |
Set to WIP.
Sam Spilsbury (smspillaz) wrote : | # |
Cheers. Will get on to this as soon as it makes sense to do so.
On Thu, Apr 25, 2013 at 3:53 PM, MC Return <email address hidden> wrote:
> Set to WIP.
> --
> https:/
> Your team Compiz Maintainers is requested to review the proposed merge of lp:~mc-return/compiz/compiz.merge-fix771875-screenshot-overlay-on-screenshots-with-enabled-fbos into lp:compiz.
--
Sam Spilsbury
MC Return (mc-return) wrote : | # |
> Cheers. Will get on to this as soon as it makes sense to do so.
>
+1. No stress with that one. Every user can get it to work correctly already.
MC Return (mc-return) wrote : | # |
Sam, just 2 questions regarding this matter:
1. Are you aware that glReadBuffer (GL_FRONT); will just be called once per screenshot and just in the case FBOs are enabled, that means if the user is veryveryvery fast, maximally 3 times per second ?
2. Would it help to call glReadBuffer (GL_BACK); after glReadPixels (...) if (GL::fboEnabled) to change it back, or is
this a stupid and useless idea ?
Sam Spilsbury (smspillaz) wrote : | # |
Actually, I think you're probably right on this one. Though I think if
I remember correctly now the right approach is actually to read off of
the currently bound framebuffer object and not the frontbuffer. The
reason why the overlay persists is because the backbuffer is always
one-frame-behind. I'm not really sure why there is no overlay on the
frontbuffer, perhaps it is a race condition. In any case, those pixels
will be at least one frame out of date.
The way screenshot works is that it paints the overlay, and then upon
"deciding" to take the screenshot, damages that region and then calls
glReadPixels on the next paint pass. The pixels for that frame will be
painted into the currently bound framebuffer object.
So the correct approach would be:
GLuint readBufferBinding = 0;
if (GL::fboEnabled)
{
glGetIntegerv (GL_READ_
/* We want to read off of the last frame, so bind the backbuffer */
GL:
}
...
glReadPixels
...
if (GL::fboEnabled)
{
/* Bind the last read buffer */
GL:
}
You're right. Since this only happens in screenshot mode, I don't
think performance is a huge concern anways. glReadPixels is a far
slower operation than glBindFramebuff
that.
On Fri, Apr 26, 2013 at 6:14 PM, MC Return <email address hidden> wrote:
> Review: Needs Information
>
> Sam, just 2 questions regarding this matter:
>
> 1. Are you aware that glReadBuffer (GL_FRONT); will just be called once per screenshot and just in the case FBOs are enabled, that means if the user is veryveryvery fast, maximally 3 times per second ?
>
> 2. Would it help to call glReadBuffer (GL_BACK); after glReadPixels (...) if (GL::fboEnabled) to change it back, or is
> this a stupid and useless idea ?
> --
> https:/
> Your team Compiz Maintainers is requested to review the proposed merge of lp:~mc-return/compiz/compiz.merge-fix771875-screenshot-overlay-on-screenshots-with-enabled-fbos into lp:compiz.
--
Sam Spilsbury
MC Return (mc-return) wrote : | # |
> Actually, I think you're probably right on this one. Though I think if
> I remember correctly now the right approach is actually to read off of
> the currently bound framebuffer object and not the frontbuffer. The
> reason why the overlay persists is because the backbuffer is always
> one-frame-behind. I'm not really sure why there is no overlay on the
> frontbuffer, perhaps it is a race condition. In any case, those pixels
> will be at least one frame out of date.
>
> The way screenshot works is that it paints the overlay, and then upon
> "deciding" to take the screenshot, damages that region and then calls
> glReadPixels on the next paint pass. The pixels for that frame will be
> painted into the currently bound framebuffer object.
>
> So the correct approach would be:
>
> GLuint readBufferBinding = 0;
>
> if (GL::fboEnabled)
> {
> glGetIntegerv (GL_READ_
> /* We want to read off of the last frame, so bind the backbuffer */
> GL::bindFramebuffer (GL_READ_
> }
>
> ...
>
> glReadPixels
>
> ...
>
> if (GL::fboEnabled)
> {
> /* Bind the last read buffer */
> GL::bindFramebuffer (GL_READ_
> }
>
> You're right. Since this only happens in screenshot mode, I don't
> think performance is a huge concern anways. glReadPixels is a far
> slower operation than glBindFramebuff
> that.
>
Cool, I'll fix it that way then :)
MC Return (mc-return) wrote : | # |
>
> So the correct approach would be:
>
> GLuint readBufferBinding = 0;
>
> if (GL::fboEnabled)
> {
> glGetIntegerv (GL_READ_
> /* We want to read off of the last frame, so bind the backbuffer */
> GL::bindFramebuffer (GL_READ_
> }
>
> ...
>
> glReadPixels
>
> ...
>
> if (GL::fboEnabled)
> {
> /* Bind the last read buffer */
> GL::bindFramebuffer (GL_READ_
> }
>
This does not work:
/home/mcr2010/
/home/mcr2010/
glGetIntegerv (GL_READ_
/home/mcr2010/
GL:
MC Return (mc-return) : | # |
Sam Spilsbury (smspillaz) wrote : | # |
Did you define the readBufferBinding variable? Compiler says that you didn't ;-)
On Fri, Apr 26, 2013 at 9:34 PM, MC Return <email address hidden> wrote:
>>
>> So the correct approach would be:
>>
>> GLuint readBufferBinding = 0;
>>
>> if (GL::fboEnabled)
>> {
>> glGetIntegerv (GL_READ_
>> /* We want to read off of the last frame, so bind the backbuffer */
>> GL::bindFramebuffer (GL_READ_
>> }
>>
>> ...
>>
>> glReadPixels
>>
>> ...
>>
>> if (GL::fboEnabled)
>> {
>> /* Bind the last read buffer */
>> GL::bindFramebuffer (GL_READ_
>> }
>>
> This does not work:
>
> /home/mcr2010/
> /home/mcr2010/
> glGetIntegerv (GL_READ_
> ^
> /home/mcr2010/
> GL::bindFramebuffer (GL_READ_
> ^
>
>
> --
> https:/
> Your team Compiz Maintainers is requested to review the proposed merge of lp:~mc-return/compiz/compiz.merge-fix771875-screenshot-overlay-on-screenshots-with-enabled-fbos into lp:compiz.
--
Sam Spilsbury
MC Return (mc-return) wrote : | # |
urgh - sorry
MC Return (mc-return) wrote : | # |
GLint readBufferBinding = 0;
instead of
GLuint readBufferBinding = 0;
now did compile -> testing now... :)
MC Return (mc-return) wrote : | # |
The overlay is on the screenshot again :(
MC Return (mc-return) wrote : | # |
I used this code:
GLint readBufferBinding = 0;
/* If we have fbos enabled we
* need to clear the overlay
* before reading the pixels */
if (GL::fboEnabled)
{
glGetIntegerv (GL_READ_
/* We want to read off of the last frame, so bind the backbuffer */
GL::
}
glReadPixels (x1, ::screen->height () - y2, w, h,
GL_RGBA, GL_UNSIGNED_BYTE,
(GLvoid *) buffer);
if (GL::fboEnabled)
/* Bind the last read buffer */
GL::
MC Return (mc-return) wrote : | # |
Okay, updated the commit message here.
The last thing missing here seems to be your +1, Sam ;)
Sam Spilsbury (smspillaz) : | # |
PS Jenkins bot (ps-jenkins) : | # |
Preview Diff
1 | === modified file 'plugins/opengl/include/opengl/opengl.h' |
2 | --- plugins/opengl/include/opengl/opengl.h 2013-01-10 09:23:24 +0000 |
3 | +++ plugins/opengl/include/opengl/opengl.h 2013-04-27 11:54:25 +0000 |
4 | @@ -449,6 +449,8 @@ |
5 | /* OpenGL|ES does not support different draw/read framebuffers */ |
6 | static const GLenum DRAW_FRAMEBUFFER = GL_FRAMEBUFFER; |
7 | static const GLenum READ_FRAMEBUFFER = GL_FRAMEBUFFER; |
8 | + static const GLenum DRAW_FRAMEBUFFER_BINDING = FRAMEBUFFER_BINDING; |
9 | + static const GLenum READ_FRAMEBUFFER_BINDING = FRAMEBUFFER_BINDING; |
10 | |
11 | static const GLenum FRAMEBUFFER_COMPLETE = GL_FRAMEBUFFER_COMPLETE; |
12 | static const GLenum FRAMEBUFFER_UNDEFINED = 0; |
13 | @@ -484,6 +486,8 @@ |
14 | |
15 | static const GLenum DRAW_FRAMEBUFFER = GL_DRAW_FRAMEBUFFER_EXT; |
16 | static const GLenum READ_FRAMEBUFFER = GL_READ_FRAMEBUFFER_EXT; |
17 | + static const GLenum DRAW_FRAMEBUFFER_BINDING = GL_DRAW_FRAMEBUFFER_BINDING_EXT; |
18 | + static const GLenum READ_FRAMEBUFFER_BINDING = GL_READ_FRAMEBUFFER_BINDING_EXT; |
19 | static const GLenum FRAMEBUFFER_COMPLETE = GL_FRAMEBUFFER_COMPLETE_EXT; |
20 | static const GLenum FRAMEBUFFER_UNDEFINED = GL_FRAMEBUFFER_UNDEFINED; |
21 | static const GLenum FRAMEBUFFER_INCOMPLETE_ATTACHMENT = GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT_EXT; |
22 | |
23 | === modified file 'plugins/screenshot/src/screenshot.cpp' |
24 | --- plugins/screenshot/src/screenshot.cpp 2013-04-24 15:42:11 +0000 |
25 | +++ plugins/screenshot/src/screenshot.cpp 2013-04-27 11:54:25 +0000 |
26 | @@ -23,6 +23,9 @@ |
27 | * Author: David Reveman <davidr@novell.com> |
28 | */ |
29 | |
30 | +#include <sstream> |
31 | +#include <boost/scoped_array.hpp> |
32 | + |
33 | #include "screenshot.h" |
34 | |
35 | #include <dirent.h> |
36 | @@ -104,8 +107,6 @@ |
37 | action->setState (action->state () & ~(CompAction::StateTermKey | |
38 | CompAction::StateTermButton)); |
39 | |
40 | - gScreen->glPaintOutputSetEnabled (this, false); |
41 | - |
42 | return false; |
43 | } |
44 | |
45 | @@ -148,79 +149,298 @@ |
46 | ShotScreen::paint (CompOutput::ptrList &outputs, |
47 | unsigned int mask) |
48 | { |
49 | - cScreen->paint (outputs, mask); |
50 | - |
51 | if (mGrab) |
52 | { |
53 | if (!mGrabIndex) |
54 | { |
55 | - int x1 = MIN (mX1, mX2); |
56 | - int y1 = MIN (mY1, mY2); |
57 | - int x2 = MAX (mX1, mX2); |
58 | - int y2 = MAX (mY1, mY2); |
59 | - |
60 | - int w = x2 - x1; |
61 | - int h = y2 - y1; |
62 | - |
63 | - if (w && h) |
64 | + /* Taking screenshot, enable full paint on |
65 | + * this frame */ |
66 | + |
67 | + outputs.clear (); |
68 | + outputs.push_back (&screen->fullscreenOutput ()); |
69 | + } |
70 | + } |
71 | + |
72 | + cScreen->paint (outputs, mask); |
73 | +} |
74 | + |
75 | +namespace |
76 | +{ |
77 | + bool paintSelectionRectangleFill (const CompRect &rect, |
78 | + unsigned short *fillColor, |
79 | + GLVertexBuffer *streamingBuffer, |
80 | + const GLMatrix &transform) |
81 | + { |
82 | + GLfloat vertexData[12]; |
83 | + GLushort colorData[4]; |
84 | + |
85 | + int x1 = rect.x1 (); |
86 | + int y1 = rect.y1 (); |
87 | + int x2 = rect.x2 (); |
88 | + int y2 = rect.y2 (); |
89 | + |
90 | + const float MaxUShortFloat = std::numeric_limits <unsigned short>::max (); |
91 | + |
92 | + /* draw filled rectangle */ |
93 | + float alpha = fillColor[3] / MaxUShortFloat; |
94 | + |
95 | + colorData[0] = alpha * fillColor[0]; |
96 | + colorData[1] = alpha * fillColor[1]; |
97 | + colorData[2] = alpha * fillColor[2]; |
98 | + colorData[3] = alpha * MaxUShortFloat; |
99 | + |
100 | + vertexData[0] = x1; |
101 | + vertexData[1] = y1; |
102 | + vertexData[2] = 0.0f; |
103 | + vertexData[3] = x1; |
104 | + vertexData[4] = y2; |
105 | + vertexData[5] = 0.0f; |
106 | + vertexData[6] = x2; |
107 | + vertexData[7] = y1; |
108 | + vertexData[8] = 0.0f; |
109 | + vertexData[9] = x2; |
110 | + vertexData[10] = y2; |
111 | + vertexData[11] = 0.0f; |
112 | + |
113 | + streamingBuffer->begin (GL_TRIANGLE_STRIP); |
114 | + |
115 | + streamingBuffer->addColors (1, colorData); |
116 | + streamingBuffer->addVertices (4, vertexData); |
117 | + |
118 | + if (streamingBuffer->end ()) |
119 | + { |
120 | + glEnable (GL_BLEND); |
121 | + |
122 | + streamingBuffer->render (transform); |
123 | + |
124 | + glDisable (GL_BLEND); |
125 | + |
126 | + return true; |
127 | + } |
128 | + |
129 | + return false; |
130 | + } |
131 | + |
132 | + bool paintSelectionRectangleOutline (const CompRect &rect, |
133 | + unsigned short *outlineColor, |
134 | + GLVertexBuffer *streamingBuffer, |
135 | + const GLMatrix &transform) |
136 | + { |
137 | + GLfloat vertexData[12]; |
138 | + GLushort colorData[4]; |
139 | + |
140 | + int x1 = rect.x1 (); |
141 | + int y1 = rect.y1 (); |
142 | + int x2 = rect.x2 (); |
143 | + int y2 = rect.y2 (); |
144 | + |
145 | + const float MaxUShortFloat = std::numeric_limits <unsigned short>::max (); |
146 | + |
147 | + /* draw outline */ |
148 | + float alpha = outlineColor[3] / MaxUShortFloat; |
149 | + |
150 | + colorData[0] = alpha * outlineColor[0]; |
151 | + colorData[1] = alpha * outlineColor[1]; |
152 | + colorData[2] = alpha * outlineColor[2]; |
153 | + colorData[3] = alpha * MaxUShortFloat; |
154 | + |
155 | + vertexData[0] = x1; |
156 | + vertexData[1] = y1; |
157 | + vertexData[2] = 0.0f; |
158 | + vertexData[3] = x1; |
159 | + vertexData[4] = y2; |
160 | + vertexData[5] = 0.0f; |
161 | + vertexData[6] = x2; |
162 | + vertexData[7] = y2; |
163 | + vertexData[8] = 0.0f; |
164 | + vertexData[9] = x2; |
165 | + vertexData[10] = y1; |
166 | + vertexData[11] = 0.0f; |
167 | + |
168 | + streamingBuffer->begin (GL_LINE_LOOP); |
169 | + |
170 | + streamingBuffer->addColors (1, colorData); |
171 | + streamingBuffer->addVertices (4, vertexData); |
172 | + |
173 | + if (streamingBuffer->end ()) |
174 | + { |
175 | + glEnable (GL_BLEND); |
176 | + glLineWidth (2.0); |
177 | + |
178 | + streamingBuffer->render (transform); |
179 | + |
180 | + glDisable (GL_BLEND); |
181 | + |
182 | + return true; |
183 | + } |
184 | + |
185 | + return false; |
186 | + } |
187 | + |
188 | + void |
189 | + ensureDirectoryForImage (CompString &directory) |
190 | + { |
191 | + /* If dir is empty, use user's desktop directory instead */ |
192 | + if (directory.length () == 0) |
193 | + directory = getXDGUserDir (XDGUserDirDesktop); |
194 | + } |
195 | + |
196 | + int |
197 | + getImageNumberFromDirectory (const CompString &directory) |
198 | + { |
199 | + struct dirent **namelist; |
200 | + |
201 | + int n = scandir (directory.c_str (), &namelist, shotFilter, shotSort); |
202 | + |
203 | + if (n >= 0) |
204 | + { |
205 | + int number = 0; |
206 | + |
207 | + if (n > 0) |
208 | + sscanf (namelist[n - 1]->d_name, |
209 | + "screenshot%d.png", |
210 | + &number); |
211 | + |
212 | + ++number; |
213 | + |
214 | + if (n) |
215 | + free (namelist); |
216 | + |
217 | + return number; |
218 | + } |
219 | + else |
220 | + { |
221 | + perror ("scandir"); |
222 | + return 0; |
223 | + } |
224 | + } |
225 | + |
226 | + CompString |
227 | + getImageAbsolutePath (const CompString &directory, |
228 | + int number) |
229 | + { |
230 | + std::stringstream ss; |
231 | + ss << directory << "/screenshot" << number << ".png"; |
232 | + return ss.str (); |
233 | + } |
234 | + |
235 | + bool |
236 | + saveBuffer (const boost::scoped_array <GLubyte> &buffer, |
237 | + int w, |
238 | + int h, |
239 | + const CompString &path) |
240 | + { |
241 | + CompSize imageSize (w, h); |
242 | + |
243 | + if (!::screen->writeImageToFile (const_cast <CompString &> (path), |
244 | + "png", |
245 | + imageSize, |
246 | + buffer.get ())) |
247 | + { |
248 | + compLogMessage ("screenshot", CompLogLevelError, |
249 | + "failed to write screenshot image"); |
250 | + return false; |
251 | + } |
252 | + |
253 | + return true; |
254 | + } |
255 | + |
256 | + bool |
257 | + launchApplicationAndTakeScreenshot (const CompString &app, |
258 | + const CompString &directory) |
259 | + { |
260 | + if (app.length () > 0) |
261 | + { |
262 | + ::screen->runCommand (app + " " + directory); |
263 | + return true; |
264 | + } |
265 | + |
266 | + return false; |
267 | + } |
268 | + |
269 | + bool |
270 | + readFromGPUBufferToCPUBuffer (const CompRect &rect, |
271 | + boost::scoped_array <GLubyte> &buffer) |
272 | + { |
273 | + int x1 = rect.x1 (); |
274 | + int y1 = rect.y1 (); |
275 | + int x2 = rect.x2 (); |
276 | + int y2 = rect.y2 (); |
277 | + |
278 | + int w = x2 - x1; |
279 | + int h = y2 - y1; |
280 | + |
281 | + if (w && h) |
282 | + { |
283 | + size_t size = w * h * 4; |
284 | + buffer.reset (new GLubyte[size]); |
285 | + |
286 | + if (buffer.get ()) |
287 | { |
288 | - GLubyte *buffer; |
289 | - CompString dir (optionGetDirectory ()); |
290 | - |
291 | - /* If dir is empty, use user's desktop directory instead */ |
292 | - if (dir.length () == 0) |
293 | - dir = getXDGUserDir (XDGUserDirDesktop); |
294 | - |
295 | - buffer = (GLubyte *)malloc (sizeof (GLubyte) * w * h * 4); |
296 | - |
297 | - if (buffer) |
298 | + GLint drawBinding = 0; |
299 | + GLint readBinding = 0; |
300 | + |
301 | + /* Bind the currently bound draw framebuffer to |
302 | + * the read framebuffer and read from it */ |
303 | + if (GL::fboEnabled) |
304 | { |
305 | - struct dirent **namelist; |
306 | - |
307 | - glReadPixels (x1, ::screen->height () - y2, w, h, |
308 | - GL_RGBA, GL_UNSIGNED_BYTE, |
309 | - (GLvoid *) buffer); |
310 | - |
311 | - int n = scandir (dir.c_str (), &namelist, shotFilter, shotSort); |
312 | - |
313 | - if (n >= 0) |
314 | - { |
315 | - char name[256]; |
316 | - int number = 0; |
317 | - |
318 | - if (n > 0) |
319 | - sscanf (namelist[n - 1]->d_name, |
320 | - "screenshot%d.png", |
321 | - &number); |
322 | - |
323 | - ++number; |
324 | - |
325 | - if (n) |
326 | - free (namelist); |
327 | - |
328 | - snprintf (name, 256, "screenshot%d.png", number); |
329 | - |
330 | - CompString app (optionGetLaunchApp ()); |
331 | - CompString path (dir + "/" + name); |
332 | - CompSize imageSize (w, h); |
333 | - |
334 | - if (!::screen->writeImageToFile (path, "png", |
335 | - imageSize, buffer)) |
336 | - compLogMessage ("screenshot", CompLogLevelError, |
337 | - "failed to write screenshot image"); |
338 | - else if (app.length () > 0) |
339 | - ::screen->runCommand (app + " " + path); |
340 | - } |
341 | - else |
342 | - perror (dir.c_str ()); |
343 | - |
344 | - free (buffer); |
345 | + glGetIntegerv (GL::DRAW_FRAMEBUFFER_BINDING, &drawBinding); |
346 | + glGetIntegerv (GL::READ_FRAMEBUFFER_BINDING, &readBinding); |
347 | + (GL::bindFramebuffer) (GL::READ_FRAMEBUFFER, drawBinding); |
348 | } |
349 | + |
350 | + glGetError (); |
351 | + glReadPixels (x1, ::screen->height () - y2, w, h, |
352 | + GL_RGBA, GL_UNSIGNED_BYTE, |
353 | + (GLvoid *) buffer.get ()); |
354 | + |
355 | + if (GL::fboEnabled) |
356 | + (GL::bindFramebuffer) (GL::READ_FRAMEBUFFER, readBinding); |
357 | + |
358 | + if (glGetError () != GL_NO_ERROR) |
359 | + return false; |
360 | + |
361 | + return true; |
362 | } |
363 | - /* Disable screen capture */ |
364 | - cScreen->paintSetEnabled (this, false); |
365 | - mGrab = false; |
366 | - } |
367 | + } |
368 | + |
369 | + return false; |
370 | + } |
371 | + |
372 | + /* We need to take directory by copy because |
373 | + * it may be modified later */ |
374 | + bool saveScreenshot (CompRect rect, |
375 | + CompString directory, |
376 | + const CompString &alternativeApplication) |
377 | + { |
378 | + ensureDirectoryForImage (directory); |
379 | + |
380 | + int number = getImageNumberFromDirectory (directory); |
381 | + CompString path = getImageAbsolutePath (directory, number); |
382 | + |
383 | + boost::scoped_array <GLubyte> buffer; |
384 | + |
385 | + bool success = readFromGPUBufferToCPUBuffer (rect, |
386 | + buffer); |
387 | + |
388 | + if (success) |
389 | + { |
390 | + success = saveBuffer (buffer, |
391 | + rect.width (), |
392 | + rect.height (), path); |
393 | + } |
394 | + else |
395 | + { |
396 | + compLogMessage ("screenshot", CompLogLevelWarn, "glReadPixels failed"); |
397 | + } |
398 | + |
399 | + if (!success) |
400 | + success = |
401 | + launchApplicationAndTakeScreenshot (alternativeApplication, |
402 | + directory); |
403 | + |
404 | + return success; |
405 | + |
406 | } |
407 | } |
408 | |
409 | @@ -231,12 +451,6 @@ |
410 | CompOutput *output, |
411 | unsigned int mask) |
412 | { |
413 | - GLVertexBuffer *streamingBuffer = GLVertexBuffer::streamingBuffer (); |
414 | - GLMatrix transform (matrix); |
415 | - GLfloat vertexData[12]; |
416 | - GLushort colorData[4]; |
417 | - GLushort *color; |
418 | - |
419 | bool status = gScreen->glPaintOutput (attrib, matrix, region, output, mask); |
420 | |
421 | if (status && mGrab) |
422 | @@ -245,87 +459,42 @@ |
423 | * we are grabbed, the size has changed and the CCSM |
424 | * option to draw it is enabled. */ |
425 | |
426 | + CompRect selectionRect (std::min (mX1, mX2), |
427 | + std::min (mY1, mY2), |
428 | + std::abs (mX2 - mX1), |
429 | + std::abs (mY2 - mY1)); |
430 | + |
431 | if (mGrabIndex && |
432 | selectionSizeChanged && |
433 | optionGetDrawSelectionIndicator ()) |
434 | { |
435 | - int x1 = MIN (mX1, mX2); |
436 | - int y1 = MIN (mY1, mY2); |
437 | - int x2 = MAX (mX1, mX2); |
438 | - int y2 = MAX (mY1, mY2); |
439 | - |
440 | - const float MaxUShortFloat = std::numeric_limits <unsigned short>::max (); |
441 | - |
442 | - /* draw filled rectangle */ |
443 | - float alpha = optionGetSelectionFillColorAlpha () / MaxUShortFloat; |
444 | - color = optionGetSelectionFillColor (); |
445 | - |
446 | - colorData[0] = alpha * color[0]; |
447 | - colorData[1] = alpha * color[1]; |
448 | - colorData[2] = alpha * color[2]; |
449 | - colorData[3] = alpha * MaxUShortFloat; |
450 | - |
451 | - vertexData[0] = x1; |
452 | - vertexData[1] = y1; |
453 | - vertexData[2] = 0.0f; |
454 | - vertexData[3] = x1; |
455 | - vertexData[4] = y2; |
456 | - vertexData[5] = 0.0f; |
457 | - vertexData[6] = x2; |
458 | - vertexData[7] = y1; |
459 | - vertexData[8] = 0.0f; |
460 | - vertexData[9] = x2; |
461 | - vertexData[10] = y2; |
462 | - vertexData[11] = 0.0f; |
463 | - |
464 | - transform.translate (-0.5f, -0.5f, -DEFAULT_Z_CAMERA); |
465 | - transform.scale (1.0f / output->width (), |
466 | - -1.0f / output->height (), |
467 | - 1.0f); |
468 | - transform.translate (-output->region ()->extents.x1, |
469 | - -output->region ()->extents.y2, |
470 | - 0.0f); |
471 | - |
472 | - glEnable (GL_BLEND); |
473 | - |
474 | - streamingBuffer->begin (GL_TRIANGLE_STRIP); |
475 | - |
476 | - streamingBuffer->addColors (1, colorData); |
477 | - streamingBuffer->addVertices (4, vertexData); |
478 | - |
479 | - streamingBuffer->end (); |
480 | - streamingBuffer->render (transform); |
481 | - |
482 | - /* draw outline */ |
483 | - alpha = optionGetSelectionOutlineColorAlpha () / MaxUShortFloat; |
484 | - color = optionGetSelectionOutlineColor (); |
485 | - |
486 | - colorData[0] = alpha * color[0]; |
487 | - colorData[1] = alpha * color[1]; |
488 | - colorData[2] = alpha * color[2]; |
489 | - colorData[3] = alpha * MaxUShortFloat; |
490 | - |
491 | - vertexData[6] = x2; |
492 | - vertexData[7] = y2; |
493 | - vertexData[9] = x2; |
494 | - vertexData[10] = y1; |
495 | - |
496 | - glLineWidth (2.0); |
497 | - |
498 | - streamingBuffer->begin (GL_LINE_LOOP); |
499 | - |
500 | - streamingBuffer->addColors (1, colorData); |
501 | - streamingBuffer->addVertices (4, vertexData); |
502 | - |
503 | - streamingBuffer->end (); |
504 | - streamingBuffer->render (transform); |
505 | - |
506 | - glDisable (GL_BLEND); |
507 | + GLMatrix transform (matrix); |
508 | + GLVertexBuffer *streamingBuffer (GLVertexBuffer::streamingBuffer ()); |
509 | + transform.toScreenSpace (output, -DEFAULT_Z_CAMERA); |
510 | + |
511 | + paintSelectionRectangleFill (selectionRect, |
512 | + optionGetSelectionFillColor (), |
513 | + streamingBuffer, |
514 | + transform); |
515 | + |
516 | + paintSelectionRectangleOutline (selectionRect, |
517 | + optionGetSelectionOutlineColor (), |
518 | + streamingBuffer, |
519 | + transform); |
520 | |
521 | /* we finished painting the selection box, |
522 | * reset selectionSizeChanged now */ |
523 | selectionSizeChanged = false; |
524 | } |
525 | + else if (!mGrabIndex) |
526 | + { |
527 | + /* Taking a screenshot */ |
528 | + saveScreenshot (selectionRect, |
529 | + optionGetDirectory (), |
530 | + optionGetLaunchApp ()); |
531 | + cScreen->paintSetEnabled (this, false); |
532 | + gScreen->glPaintOutputSetEnabled (this, false); |
533 | + } |
534 | } |
535 | |
536 | return status; |
I don't think reading off the front buffer is correct. It should actually be the back buffer.
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'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.
So - we'll get back to this as soon as the right frameworks are in place to enable it properly. Sorry about that :/