GNOME Bugzilla – Bug 501512
Multi-screen fade affects all screens during slideshow
Last modified: 2009-09-29 18:09:49 UTC
See bug 327245 for some background info. On dual-screen Xinerama setups, the slideshow fade function affects all screens. It should just affect the one with the slideshow. Patches welcome from Xinerama users! The main developers don't have dual screen systems to play with. - Mike
*** Bug 501511 has been marked as a duplicate of this bug. ***
*** Bug 531381 has been marked as a duplicate of this bug. ***
Unfortunately gs-fade.c does this by gamma settings of the X11 screen (which is composed of all Xinerama monitors). This in inherently incompatible with Xinerama (as well as any user without X11 or the XF86VidMode extension). Thus, the current mechanism for fading needs to be completely scrapped. Alpha-blending the image onto the surface would achieve the desired functionality, and Cairo provides XRender/OpenGL acceleration. It would also allow for fancier effects such as cross-fading of images. Now that real-life isn't swallowing up all my time, I've started looking into what it would take to implement such functionality into gthumb; hopefully I'll have something working in a week or or so.
Currently a slideshow is just a fullscreen mode with a timeout the automatically changes image; in order to simplify the work, the slideshow could be implemented in a different class that doesn't allow the high level of interactivity present in the fullscreen mode. That is, when the slideshow is running the user could not be allowed to scroll the image or to change the zoom level, and the gif animations could be turned off if they are too much an issue. The only thing the user could be allowed to do is to pause the slideshow or to advance to the next image, interrupting the transition effect, this way the implementation should be much simplier. Maybe we can offer the user an option between interactivity and graphic effects, that is, if the user chooses to don't use the transition effects we can use the current fullscreen class that gives the user an higher level of interactivity, on the other hand if the user chooses to use the transiotion effects the new slideshow class is used, and he will have a lesser interactive slideshow.
Created attachment 114229 [details] [review] Patch 001: Correct closure for GtkWidget's set_scroll_adjustments_signal This bug doesn't seem to manifest itself in trunk as I have not observed the set_scroll_adjustments signal being sent, but in my work-in-progress (debugging with G_DEBUG=fatal_warnings), I've noticed gthumb_marshal_VOID__POINTER_POINTER throwing assertions because the parameters were passed as G_TYPE_OBJECTs instead of G_TYPE_POINTERS. This patch adds and uses an appropriate marshaller.
Created attachment 114231 [details] [review] Patch 002: Remove XF86 Gamma-based fading I went through and started rewriting my changes such that they could be reviewed in a more piecewise manner since they were getting to be rather substantial in size. This patch only removes the non-Ximerama supporting fading code, and the related checkbox in the preferences dialog. Hopefully I'll have worked out the remaining issues with the next patch, which is a major refactoring of libgthumb/image-viewer.c, before the end of the week. It currently has issues with animations and scrolling. Then I'll hopefully be able to resume work on re-enabling transitions.
gthumb_marshal_VOID__OBJECT_OBJECT patch committed to trunk. Thanks! - Mike
Geoffrey, Why did you remove the fading preference? Won't the new optional fading code still require this preference? - Mike
I was planning on replacing it with a combo-box with 3 options regarding transitions: none, fade to blade, and crossfade. I just didn't include the new widget in the patch since the supporting transition code isn't ready for review yet.
Created attachment 114284 [details] [review] Patch 003: Massive refactor of ImageViewer And in this corner, a patch weighing in at well over 4.5 thousand lines and a hefty 123kb is a patch that massively refactors the ImageViewer widget. Mike, Gdk Pixbufs and Animations, offsets, zoom-levels & fits, and anything else that is per-image has been moved to an ImageViewerImage GObject. This way, while a transition is taking place, the ImageViewer widget can refer to 2 ImageViewerImage objects to paint a blend between them. I've tested this patch rather thoroughly, on animations, static images, images that fail to load, the red-eye dialog, crop dialog, full-screen, slideshow, etc (with G_DEBUG=fatal_warnings) and it seems to be perfectly stable. Feel free to take your time reviewing it though; it's a rather massive change. I'll be without internet access for the next week or so, so I'm afraid I won't be able to answer any questions you have for a little while. My next step is to finish my conversion the ImageViewer painting code to be cairo-based. Once that's done, work on the transitions themselves should be all that remains. -Geoffrey
Patch 2 committed, thanks! I will also be without internet or computer access next week. I'll play with the patch when I can. Perhaps Paolo will have time to check it too. - Mike
Patch 3 needs some cleaning: [mjc@flay trunk]$ ./autogen.sh --prefix=/usr CFLAGS=-Wall > eraseme; make > eraseme ... image-viewer.c:1091:8: warning: extra tokens at end of #endif directive image-viewer.c: In function 'scroll_relative_no_scrollbar': image-viewer.c:995: warning: unused variable 'replay_animation' image-viewer.c:993: warning: unused variable 'event' image-viewer.c:992: warning: unused variable 'drawable' image-viewer.c: In function 'set_zoom': image-viewer.c:1485: warning: implicit declaration of function 'image_viewer_image_zoom_at_point' image-viewer.c: In function 'image_viewer_get_adjustments': image-viewer.c:1603: warning: unused variable 'viewer' image-viewer.c: In function 'load_from_image_loader__step2': image-viewer.c:1758: warning: unused variable 'viewer' image-viewer.c: In function 'image_viewer_get_image_height': image-viewer.c:1877: warning: unused variable 'pixbuf' image-viewer.c: In function 'image_viewer_get_image_bps': image-viewer.c:1893: warning: unused variable 'pixbuf' image-viewer.c: In function 'image_viewer_get_has_alpha': image-viewer.c:1909: warning: unused variable 'pixbuf' image-viewer.c: In function 'image_loaded': image-viewer.c:2058: warning: implicit declaration of function 'image_viewer_image_set_zoom_level' image-viewer.c:2087: warning: passing argument 3 of 'g_signal_connect_data' from incompatible pointer type image-viewer.c:2042: warning: unused variable 'anim' image-viewer.c: At top level: image-viewer.c:172: warning: 'scroll_to' declared 'static' but never defined image-viewer.c:920: warning: 'expose_area' defined but not used image-viewer-image.c: In function 'clamp_offsets': image-viewer-image.c:238: warning: unused variable 'buf' image-viewer-image.c: In function 'set_zoom_at_point': image-viewer-image.c:341: warning: passing argument 1 of 'gtk_widget_queue_resize' from incompatible pointer type image-viewer-image.c: In function 'image_viewer_image_set_fit_mode': image-viewer-image.c:652: warning: unused variable 'buf' gth-fullscreen.c: In function ‘hide_comment_on_image’: gth-fullscreen.c:1038: warning: unused variable ‘viewer’ - Mike
Created attachment 114350 [details] [review] 114284: Patch 003: Massive refactor of ImageViewer (cleaned up) All right. This one compiles cleanly. Dead code removed, warnings corrected, out-of-date comments to myself pruned.
Could you tidy up the patch so it applies cleanly against svn? [mjc@flay trunk]$ patch -p0 < 003-imageviewer-refactor.patch patching file ChangeLog Hunk #1 FAILED at 1. Hunk #2 succeeded at 85 (offset 40 lines). 1 out of 2 hunks FAILED -- saving rejects to file ChangeLog.rej patching file libgthumb/Makefile.am The next patch would create the file libgthumb/image-viewer-enums.h, which already exists! Assume -R? [n] n Apply anyway? [n] y patching file libgthumb/image-viewer-enums.h Patch attempted to create file libgthumb/image-viewer-enums.h, which already exists. Hunk #1 FAILED at 1. 1 out of 1 hunk FAILED -- saving rejects to file libgthumb/image-viewer-enums.h.rej The next patch would create the file libgthumb/image-viewer-image.c, which already exists! Assume -R? [n] n Apply anyway? [n] y patching file libgthumb/image-viewer-image.c Patch attempted to create file libgthumb/image-viewer-image.c, which already exists. Hunk #1 FAILED at 1. 1 out of 1 hunk FAILED -- saving rejects to file libgthumb/image-viewer-image.c.rej The next patch would create the file libgthumb/image-viewer-image.h, which already exists! Assume -R? [n] n Apply anyway? [n] y patching file libgthumb/image-viewer-image.h Patch attempted to create file libgthumb/image-viewer-image.h, which already exists. Hunk #1 FAILED at 1. 1 out of 1 hunk FAILED -- saving rejects to file libgthumb/image-viewer-image.h.rej patching file libgthumb/image-viewer.c patching file libgthumb/image-viewer.h patching file src/gth-browser.c patching file src/gth-fullscreen.c patching file src/gth-viewer.c
Sorry, ignore the last post. My error. - Mike
*** Bug 398649 has been marked as a duplicate of this bug. ***
Created attachment 115603 [details] [review] Patch 004: Rewrite painting code to use Cairo This patch builds upon patch 3. It provides an image_viewer_image_paint function to command ImageViewerImage objects to paint to a given cairo_surface (either directly to the widget, or to buffers for usage by the transitions themselves). Next up is the infrastructure for the transitions themselves (abstract GObject with virtual public functions) and related support in ImageViewer and the preferences dialogs I suppose.
I have committed both patches to trunk, since they seem to mostly work. I have noticed some zoom problems in fullscreen mode: 1) I have "Zoom to Fit if Larger" enabled. 2) I go to fullscreen mode. The first image is a small 640x480 image, and it is centered at 100% scale. 3) I PgDn through a bunch of images. 4) I hit "Home" to return to the initial image. It's position shifts (not centered), and the image may shrink. So something is broken there. On the up side, the image repaint time seems noticeably faster now. - Mike
Created attachment 115636 [details] [review] Patch 005: Fix image placement in fullscreen mode Yes, one of the things I was focusing on was using Cairo's clipping support to only paint the invalidated area rather than repainting the whole widget so the whole screen doesn't have to repaint when a menu, tooltip, etc gets closed. As for this bug, it was occurring whenever the user returns to whichever file the fullscreen mode was started at, as gth-fullscreen keeps a copy of the (potentially modified) pixbuf rather than reloading it from disk but image dimensions where taken from the previously used image_loader->animation. Clearing the animation when setting a static pixbuf ensures this won't happen again.
Looks good now! Patch committed to svn rev 2369. One glitch: the ChangeLog did not list every changed file; I updated that. - Mike
Geoffrey, 1) Do we need any cairo version checks in configure.in? 2) You deleted some preferences in an earlier patch, but they were not deleted from data/gthumb.schemas.in. That needs to be updated at some point. - Mike
(In reply to comment #21) > 1) Do we need any cairo version checks in configure.in? GThumb's trunk currently depends on GTK 2.10.0 as a minimum version; the first stable version of GTK to require Cairo was 2.8, and GTK 2.10 already requires at least Cairo 1.2.0, and I haven't used any symbols added since then so we shouldn't need to include our own check for Cairo as pkg-config/GTK should take care of it. > 2) You deleted some preferences in an earlier patch, but they were not deleted > from data/gthumb.schemas.in. That needs to be updated at some point. There are also checks in configure.in for the old XF86Gamma stuff removed in patch 2 that I noticed when I was just adding another Makefile to configure.in. I'll have a patch up to remove them shortly.
Created attachment 115642 [details] [review] Patch 006: GConf schema and configure.in cleanup
Patch committed to svn rev 2371. Thanks! po/POTFILES.in should be updated to reflect the new files. - Mike
Geoffrey, Something in your patches has broken the image_modified logic. If I modify an image (say, Image > Negative) the title bar no longer says "[modified]", and clicking "Next" no longer triggers the "Do you want to save..." dialog. Can you take a look at that? - Mike
Created attachment 116085 [details] [review] Patch 007: Fix 'Do you want to save...' prompts for modified images (In reply to comment #25) Easy enough to fix; gth-browser resets image_modified to FALSE on the "image_loaded" signal, so I just had to change image_viewer_set_pixbuf to once again not emit that signal when called. The transitions framework is mostly done; once I get the preferences dialog working there shouldn't be too much left to do (I hope).
Hmm, I'm segfaulting... Open image, Image>Negative, click "Close", click ".." in the folder bar (to go up one level) = segfault. Do you see that? - Mike
Created attachment 116089 [details] [review] Patch 008: Fix a segfault when changing directories after modifying an image Alright, it no longer crashes. However, I notice it doesn't produce a prompt before changing directories but only after changing directories and clicking on another image. To me, it would make more sense to produce the prompt before the image list is changed so the user can still see what the current image is. Is this another regression, or has gthumb always behaved this way?
It is not a regression; 2.10.8 behaves that way too. It would make more sense to show the prompt right away, as you say. - Mike
Anyway, 007 and 008 are committed. Thanks! - Mike
Geoffrey, If you are messing with the image-modified prompting code anyway, could you take a peek at bug 376308? Image changes need to be saved or dumped before using those tool functions that act on files (rather than the in-memory pixbuf). - Mike
Another small regression: Browse images with in zoom-to-fit-if-larger mode. Large images, which have been scaled down to fit the window, have vertical and horizontal scroll bars shown. They shouldn't, because the image is supposed to fir the window exactly. It works OK in 2.10.9 (no scroll bars shown). - Mike
Created attachment 116187 [details] [review] Patch 009: Fix off-by-2 error in scrollbar range This one I'd known about for a while; the scrollbar range was always 2px bigger than it should be, but I just couldn't find the offending lines for the longest time.
Committed, thanks! - Mike
Created attachment 116316 [details] [review] Patch 010: Add transition support to the ImageViewer widget This patch adds the much desired transition support for the ImageViewer widget. Additional transitions can be made by creating additional GObject types that derive from GthTransition and implementing the 'compose' virtual function, and wrapping the whole thing into a dynamically loadable module. Currently, everything works, but it's not finely optimized. On my slow 1.8GHz machine with a 1680x1050 resolution, I'm only getting 4-6 fps in full-screen with both transitions so optimizing the expose code seems pretty crucial. In a smalled window (i.e. gth-browser with an additional 1-line patch) I'm getting 10+ fps, but there is some flickering visible. Currently, there are a few places in image_viewer_expose that can easily be optimized... right now, for each frame of the transition, both images are rescaled and then composited together; however, for static images, the scaled version could be cached. Similarly, the temporary drawing buffers could be cached instead of reallocated each frame.
Some comments: 1) Yes, the speed is a big problem. The transitions are much too choppy currently (on my Core Duo 3.4 GHz ...) 2) I don't like the preferences UI - the mode combo box seems unnecessary. The number of selected transitions should control the mode (i.e., no selections = disabled, one selected - single mode, more than one = random mode). 3) I'll ask Paolo to review the module structure, to make sure it fits his thinking. - Mike
...and on the last point, Paolo is OK with the module structure... - Mike
Yes, the combobox is functionally unnecessary; it purely serves as a convenience for those users who want the exact same transition for each image, or a simple two-click method to disable all transitions at once. I've seen at least one other program do things this way... It might have been kscreensaver or eog some such; I don't quite remember. It really doesn't matter with just two transitions, but throw in 'Dissolve' 'Slide Up', 'Slide Down' etc and just about anything else one might find in the 'slide transitions' of a presentation program and the list will grow rather quickly. It doesn't matter to me if it stays or goes; it's trivial to remove, but it does have a convenience aspect that should be considered. -Geoffrey
I want to avoid entering the same information twice. I would suggest sorting the list of transitions so that the currently-enabled ones are listed first, followed by the disabled ones (i.e., sort by status, then alphabetically). That should improve the convenience without adding redundant UI elements. Another thought: Could you add a low-cpu transition effect - maybe a checkerboard blanking, or whatever you like, so that we have a highly-usable default transition while the fades are being optimized? Lastly: great work! As a reviewer, I have to be critical and point out flaws... but I want to say thanks too! So... thanks! - Mike
Small regression: Run with --g-fatal-warnings, and switch from folder view to catalog view:
+ Trace 205100
Thread 1 (Thread 0xb8097770 (LWP 11700))
- Mike
Geoffrey, How are things going here? - Mike
Sorry for the delay; I recently moved and haven't had as much free time as I anticipated. However, I suspect the problem is quite similar to another issue I saw when trying to call g_object_unref(NULL). (i.e. calling image_viewer_set_void when there is no image on display) Hopefully I'll get an hour or two to look into that crash and another one I've found this holiday weekend. Unfortunately I don't have any ETA on the speed optimizations as of yet.
Created attachment 117725 [details] [review] Patch 011: Fix image_viewer_set_void by not dereferencing NULL pointers This should fix the crash you were seeing Mike. -Geoffrey
I still get object_unref warnings when switching between folder and catalog views. Do you? - Mike
No, I'm not getting any warnings when switching between the two views, regardless of whether an image is selected before switching or not (with glib 2.16.3 and gtk+ 2.12.11). (Previously I was getting warnings, but they were completely unrelated -- one of my catalog files had gotten corrupted and partway through had random binary non-UTF8 gibberish that glib was complaining about) I've also tried with both keep previous zoom and zoom to fit options with the above combinations to no avail. I've have partially implemented support for caching a scaled copy of the selected image each time the zoom changes. I noticed mixed in with those changes, I checked to make sure priv->current_image isn't NULL before calling i_v_i_zoom_at_point in set_zoom in image-viewer.c, but that shouldn't be g_object_unref warnings. (The ImageViewerImage needs notification from the ImageViewer when it is resized so that it can rescale the cached image; once that is fixed, I'll send it up) Can you repro your problem with fatal_warnings under GDB and get me a backtrace? -Geoffrey
I issued bug 551210 today - I was told that the slideshow is reworked currently and I might put a hint for my bug here. Maybe someone has a look :-) thanks, stephan
Argh, can someone remind me what magic is needed to compile gthumb keeping the debugging symbols for gdb? - Mike
OK, here's a partial backtrace when running with patch 117725 (but the earlier patch is not in):
+ Trace 206610
Thread 1 (Thread 0x7f0ca7a69800 (LWP 3509))
Created attachment 118497 [details] [review] Patch 011: Fix image_viewer_set_void by not dereferencing NULL pointers CFLAGS="-Wall -g" ./autogen.sh ... && make Ah, so the important difference is you're not using the transitions patch, in which image_viewer_set_void was rewritten (and thus fixed an incorrect assumptions on my part that image_viewer_set_void would never be called on a void image). I tested this one off of trunk rather than my branch with the transitions so it should really work for you this time. Hopefully this weekend I'll have time to finish the caching optimizations for the transitions (which will be beneficial to any sort of transition, checkering or more intensive alpha-blending); it's mostly done now. If that doesn't speed things up enough, I'll write an incredibly simple screen wipe transition and/or a checkerboard effect until further optimizations are found.
Thanks, patch committed to 2409! - Mike
*** Bug 551210 has been marked as a duplicate of this bug. ***
Mike, Do you have any experience with gprof or using any other profiler with gthumb? Things are still slower than they should be, and it would be nice to be able to pinpoint the bottlenecks. -Geoffrey
No, I haven't. Perhaps Paolo has... - Mike
Geoffrey, Nudge, nudge :-) Can we get a working patch? - Mike
I've been fiddling around the OProfile user & kernel mode profiler. GProf doesn't work with shared libraries, and trying to statical link Gtk & Gdk & Cairo & Pixman etc together was a pain and wasn't showing the heavy lifting done by X or the video driver. OProfile is a little more helpful in that it profiles the entire system (kernel, apps, and libraries) and can show more than CPU usage. Surprisingly, I'm only seeing 10-20% CPU usage spent by libpixman, Cario's pixel-pushing library, but and 30-40% of the CPU usage spent by the kernel, so the bottleneck doesn't seem to be in the alpha-blending itself. However, I have identified a few unnecessary image blits and per-frame buffers in image_viewer_expose that could theoretically persist across the entire lifetime of the ImageViewer widget. (Currently, two cairo_surface_t's are allocated for each frame of the transition and free'd after the image is painted to the screen). I think I've come up with a way to cache those images, which should reduce of paging the kernel may be doing. Idealistically, it's a simple change, but there are a few corner cases like resizing windows or errors loading images that are going to complicate things. That, and I have to find the memory leak I've managed to introduce by a small part of the caching I've already written.
Geoffrey, Can we get something in over the holidays? Otherwise, we may need to revert or branch... - Mike
Geoffrey, a small crasher was reported & fixed in bug 566377. - Mike
Closing this report as obsolete, due to the work in ext branch (http://live.gnome.org/gthumb). Please file a new bug if dual-screens do not work with ext branch (or release 2.11.0 or later). - Mike