After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 526214 - Cheese should not use "xgamma" for flash effect - it unsets ICC calibration
Cheese should not use "xgamma" for flash effect - it unsets ICC calibration
Status: RESOLVED FIXED
Product: cheese
Classification: Applications
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Alexander “weej” Jones
Cheese Maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2008-04-04 19:15 UTC by Alexander “weej” Jones
Modified: 2008-05-25 08:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
proposed utility to do the "flashing" (3.43 KB, text/x-csrc)
2008-05-22 20:38 UTC, Thomas Perl
  Details
new version, with my edits (3.22 KB, text/x-csrc)
2008-05-22 23:21 UTC, Alexander “weej” Jones
  Details
Patch (5.19 KB, patch)
2008-05-23 09:31 UTC, Alexander “weej” Jones
needs-work Details | Review
Patch (10.69 KB, patch)
2008-05-23 12:56 UTC, Alexander “weej” Jones
reviewed Details | Review
Patch (10.96 KB, patch)
2008-05-23 13:24 UTC, Alexander “weej” Jones
none Details | Review
Patch (10.95 KB, patch)
2008-05-23 14:21 UTC, Alexander “weej” Jones
none Details | Review
Patch (10.99 KB, patch)
2008-05-23 14:33 UTC, Alexander “weej” Jones
none Details | Review
Patch (11.35 KB, patch)
2008-05-23 14:52 UTC, Alexander “weej” Jones
none Details | Review
Patch (11.66 KB, patch)
2008-05-23 15:04 UTC, Alexander “weej” Jones
none Details | Review
Updated patch which conforms to coding style of cheese (11.74 KB, patch)
2008-05-24 17:17 UTC, Jaap A. Haitsma
none Details | Review
This patch works :-) (12.56 KB, patch)
2008-05-24 19:34 UTC, Jaap A. Haitsma
committed Details | Review

Description Alexander “weej” Jones 2008-04-04 19:15:02 UTC
Steps to reproduce:
Generate an ICC profile for your monitor, via some means.
Run "xcalib yourprofile.icc"
Run Cheese
Take a picture

Expected results:
Flash!

Actual results:
Flash! Your display is now restored to its uncalibrated state!

The issue lies where Cheese uses xgamma to determine the current state of the monitor. When, e.g. xcalib has been used to calibrate properly, xgamma has no chance of representing the state correctly, so it just returns 1.0, 1.0, 1.0.

It is worth pointing out that "gamma curves" are simple response functions, rarely representative of a display's actual response.

alex@fizz:~$ xcalib lcd.icc 
alex@fizz:~$ xgamma
-> Red  1.000, Green  1.000, Blue  1.000

So we need to come up with another way of flashing the screen. Can we detect if a compositor is running, create a full screen RGBA window and use window manager hints to modify its opacity?
Comment 1 Jaap A. Haitsma 2008-04-04 19:20:26 UTC
Confirming. Don't know how to solve it though
Comment 2 Andrea Cimitan 2008-05-07 16:35:09 UTC
I confirm this (I use an ICC profile for color calibration)
Comment 3 Thomas Perl 2008-05-22 20:38:12 UTC
Created attachment 111362 [details]
proposed utility to do the "flashing"

I'm experiencing the same issue here, the X server calibration is lost after taking a photo with Cheese.

Attached is a quick hack that simulates the described effect and doesn't mess with the gamma values, which leaves the settings of xcalib intact.

You probably should get the code directly into Cheese, and not in a seperate binary like this flasher.c is currently, but I did it as a standalone app so you can see the resulting effect and can play around with it a bit more.

Compile with:
gcc -o flasher flasher.c `pkg-config --libs --cflags gtk+-2.0`

I'd be happy if you could get this into Cheese. You can still fallback to the "old" way of messing with the gamma in case the user does not have a compositing window manager running.
Comment 4 Alexander “weej” Jones 2008-05-22 23:19:18 UTC
Hi Thomas

Thanks for making this effort. I've made some changes.

The flash actually serves a practical purpose here, too, in that the white light from your screen /does/ act as a white flash. Thus, you need to co-ordinate the white light with the image capture. I have simply done away with the ramp-up, instantly (or at least as quickly as possible) flooding the screen with white, with a 250ms or so pause and then ramp-down. This should ensure that the small amount of round trip lag between the screen and the camera is accounted for.

Using the RGBA colourmap explicitly is unnecessary when you're dealing with window opacity. I dropped it.

I added code to stop the window from taking any focus, and to make it pass input events (e.g. mouse clicks etc.).

Outstanding issues:

In Compiz, the "fade windows" and "animations" plugins get in the way and do annoying things themselves. Also, "blur" makes things look blurry under this window -- we don't want that effect. We should find a way to hint at the window manager not to do any kind of effect on these windows.

Need to drop the realize() call and put the input shaping into a callback for the realize signal.

I can take care of integrating this all into Cheese.
Comment 5 Alexander “weej” Jones 2008-05-22 23:21:31 UTC
Created attachment 111372 [details]
new version, with my edits
Comment 6 Thomas Perl 2008-05-23 04:26:50 UTC
Thanks for your feedback and input.

I tried your version, but it doesn't do the alpha fading, so I think the RGBA colormap-setting code is still needed (all my packages are from Ubuntu Hardy, using Compiz Fusion for compositing).

Adding the colormap-setting code from my original version to your code makes things work again, so it does serve a purpose, after all, I assume.
Comment 7 Jaap A. Haitsma 2008-05-23 06:20:45 UTC
Very nice improvement for the flashing. When you make the patch for cheese my suggestion is to remove the flash code from cheese-webcam.c and put it in a cheese-flash.c file which just exports one function "void cheese_flash ()"
Comment 8 Alexander “weej” Jones 2008-05-23 09:31:54 UTC
Created attachment 111393 [details] [review]
Patch

Work in progress. Unfortunately I'm blocked from testing because of a segfault in the current HEAD.
Comment 9 Alexander “weej” Jones 2008-05-23 09:33:16 UTC
Comment on attachment 111393 [details] [review]
Patch

Oh, and I still need to get to the bottom of the need for the RGBA colourmap... Anyone have any ideas? If not, I'll just add the code with a comment.
Comment 10 Alexander “weej” Jones 2008-05-23 12:56:33 UTC
Created attachment 111408 [details] [review]
Patch

OK, I'm pretty happy with this. Please review.
Comment 11 daniel g. siegel 2008-05-23 13:13:56 UTC
thanks for your help!

just two things:
- the "take a photo" button does not get back into sensitive state, please check your return values
- would it be possible to check if a compositing manager is running, and if not using the old xgamma way?
Comment 12 Alexander “weej” Jones 2008-05-23 13:23:03 UTC
Strange that the take photo button doesn't work for you. In the cheese_webcam_flash function I have a g_signal_emit (webcam, webcam_signals[PHOTO_SAVED], 0); (with a comment), and indeed it does seem to work for me. Maybe it's a race condition?

Also, I've added a non-composited alternative that simply avoids the fade. I am reluctant to restore the XGamma stuff because, like the original bug comment states, it unsets calibration LUTs. And it looks ugly, as it doesn't really make the screen white, and black/

My observations:

Metacity, no compositor: Screen flashes, no fade, but panels appear on top still. Odd.
Metacity, with compositor: Screen flashes, fade is a little bit uneven on my system (it's using XRender, remember), again, panels on top.
Compiz: Screen flashes, fade is smooth, but a few of the effects get in the way. Their devs suggest that there is no standard way to request a window be exempt from effects, so we'll (they'll) just have to deal with it by special casing or something until they figure something out for this and other OSD-style windows.
Comment 13 Alexander “weej” Jones 2008-05-23 13:24:24 UTC
Created attachment 111410 [details] [review]
Patch

Added the code path for non-composited cases. Simply avoid the fade-out.
Comment 14 Alexander “weej” Jones 2008-05-23 14:21:32 UTC
Created attachment 111412 [details] [review]
Patch

Moved the signal emission from webcam_flash to webcam_photo_data_cb. Fixes the race condition for when you take photos without a countdown.
Comment 15 Alexander “weej” Jones 2008-05-23 14:33:30 UTC
Created attachment 111413 [details] [review]
Patch

Added:
	gtk_window_set_keep_above (window, TRUE);
Comment 16 Alexander “weej” Jones 2008-05-23 14:52:30 UTC
Created attachment 111414 [details] [review]
Patch

Manage the timeouts properly so that flashes that fire in rapid succession don't break.
Comment 17 Alexander “weej” Jones 2008-05-23 15:04:20 UTC
Created attachment 111415 [details] [review]
Patch

Renamed cheese_flash_flash to cheese_flash_fire -- less of a silly name.

Got rid of cheese_webcam_flash and replaced its use with a direct call to cheese_flash_fire.
Comment 18 Jaap A. Haitsma 2008-05-24 17:17:46 UTC
Created attachment 111467 [details] [review]
Updated patch which conforms to coding style of cheese

I updated the patch so it conforms to our coding style. Please this as a reference for future patches.

There are still problems with the patch.
If I run cheese with patch the following happens

Compiz disabled
---------------
Segfault when taking a picture. BTW I don't have a camera connected but use the test image

Compiz enabled
-------------
Crash at startup
cheese: ../../src/xcb_lock.c:33: _XCBUnlockDisplay: Assertion `xcb_get_request_sent(dpy->xcb->connection) == dpy->request' failed.
Aborted
Comment 19 Jaap A. Haitsma 2008-05-24 17:21:48 UTC
Forgot to mention that in the updated patch I also change configure.ac because we don't use xxf86vm anymore.

Forgot to mention as well that the initial flasher.c program works fine for me



Daniel I'm in favor of ripping out xxf86m because of the bug reports we're getting. More and more people will run composited desktops so most of them will get the nice flash.
Comment 20 Alexander “weej” Jones 2008-05-24 18:45:13 UTC
Jaap

I unloaded my camera module and started cheese up, it waited for a few seconds then gave me the test card with the yellow information box, just fine. WORKSFORME, so I can't debug it. You'll have to get a trace and look into it yourself. Note, though, that I saw that XCBUnlockDisplay crasher once, though it was a one-off and went away as soon as I tried it again. I assumed it had nothing to do with my changes.
Comment 21 Jaap A. Haitsma 2008-05-24 19:34:07 UTC
Created attachment 111476 [details] [review]
This patch works :-)

Alex,

you're calling gtk functions in the cheese_flash_init code. This gets called in cheese_webcam_new, but cheese_webcam_new is running in another thread so cheese_webcam_new has to be run in a critical section. I added that to the patch.

Daniel,

Do you agree to add the patch?
Comment 22 daniel g. siegel 2008-05-25 01:12:24 UTC
sure! awesome work of everybody!

just one comment: when running with metacity (not composited) and i click on the "take a photo" button, the button looses its focus and the window too. i would have to click on the window once and then on the button to take a photo again.
Comment 23 Jaap A. Haitsma 2008-05-25 08:42:48 UTC
If I run metacity in non composited mode the button does not loose focus. I committed the patch can you double check if it works.

2008-05-25  Jaap Haitsma <jaap@haitsma.org>

	* configure.ac,
	  src/cheese-flash.c,
	  src/cheese-flash.h,
	  src/cheese-webcam.c,
	  src/cheese-window.c,
	  src/Makefile.am: 
	  Add new flash and remove changing gamma for simulating flash. The 
	  new flash works best if you run a composited desktop. Patch by 
	  Alex "weej" Jones and Thomas Perl. Fixes #526214
Comment 24 Thomas Perl 2008-05-25 08:52:22 UTC
Did you forget to "svn add" the cheese-flash files?

Trying to build from an up-to-date SVN trunk checkout gives me:

cheese-webcam.c:37:26: error: cheese-flash.h: No such file or directory
Comment 25 Jaap A. Haitsma 2008-05-25 08:55:58 UTC
(In reply to comment #24)
> Did you forget to "svn add" the cheese-flash files?
> 
Yes :-(

I now added them