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 626795 - Port EogScrollView to cairo
Port EogScrollView to cairo
Status: RESOLVED FIXED
Product: eog
Classification: Core
Component: image viewer
git master
Other Linux
: Normal normal
: GNOME3.0
Assigned To: Claudio Saavedra
EOG Maintainers
Depends on:
Blocks: 626688
 
 
Reported: 2010-08-13 08:04 UTC by Claudio Saavedra
Modified: 2010-09-08 16:44 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Factor out the code to store a pixbuf in EogScrollView (2.17 KB, patch)
2010-08-13 11:29 UTC, Claudio Saavedra
none Details | Review
Store a cairo_surface_t in EogScrollView for the GdkPixbuf to be rendered (2.48 KB, patch)
2010-08-13 11:29 UTC, Claudio Saavedra
none Details | Review
Replace GDK rendering in EogScrollView with cairo (8.94 KB, patch)
2010-08-13 11:29 UTC, Claudio Saavedra
none Details | Review
Use gdk_window_create_similar_surface() for EogScrollView (1.86 KB, patch)
2010-08-16 20:37 UTC, Claudio Saavedra
none Details | Review
Fully rely on cairo to render the image in EogScrollView (6.01 KB, patch)
2010-08-16 20:37 UTC, Claudio Saavedra
none Details | Review
Enable double-buffer in EogScrollView (1.11 KB, patch)
2010-08-16 21:05 UTC, Claudio Saavedra
none Details | Review
Fix the SVG rendering (2.66 KB, patch)
2010-08-17 13:09 UTC, Claudio Saavedra
none Details | Review
Remove unneeded code (4.65 KB, patch)
2010-08-17 23:21 UTC, Claudio Saavedra
none Details | Review
Replace GdkInterpType with cairo_filter_t in EogScrollView (6.49 KB, patch)
2010-08-17 23:21 UTC, Claudio Saavedra
none Details | Review
Honour the antialiasing-in and antialiasing-out EogScrollView properties (1.01 KB, patch)
2010-08-17 23:21 UTC, Claudio Saavedra
none Details | Review
screenshot showing the borders (41.82 KB, image/png)
2010-08-23 15:23 UTC, Felix Riemann
  Details
Improve the rendering of scaled images (1.96 KB, patch)
2010-08-25 09:33 UTC, Claudio Saavedra
none Details | Review

Description Claudio Saavedra 2010-08-13 08:04:52 UTC
With the latest changes in GTK+ 3, EogScrollView doesn't compile anymore. We need to port it to use cairo for rendering the image.

I have work in progress patches; right now I'm only missing drawing the background for images with transparency. Once I'm done with that I'll post them here.

On a related note, I notice that a lot of the code that's currently used for svg rendering can be reused for this. I'm not gonna do it now, but I think that at some point we need to rework everything to avoid code duplication.
Comment 1 Claudio Saavedra 2010-08-13 11:29:26 UTC
Created attachment 167790 [details] [review]
Factor out the code to store a pixbuf in EogScrollView
Comment 2 Claudio Saavedra 2010-08-13 11:29:31 UTC
Created attachment 167791 [details] [review]
Store a cairo_surface_t in EogScrollView for the GdkPixbuf to be rendered

Using this later during the rendering should improve performance.
Comment 3 Claudio Saavedra 2010-08-13 11:29:35 UTC
Created attachment 167792 [details] [review]
Replace GDK rendering in EogScrollView with cairo

This is still missing proper background drawing for images with alpha
channel. Also it becomes awfully slow when rendering an image to fit
completely in the screen and this is big enough (3888×2592).
Comment 4 Claudio Saavedra 2010-08-13 11:31:24 UTC
Adding Benjamin to the CC, so that he can tell me that I'm doing it all wrong :-)
Comment 5 Felix Riemann 2010-08-16 13:10:19 UTC
(In reply to comment #0)
> On a related note, I notice that a lot of the code that's currently used for
> svg rendering can be reused for this. I'm not gonna do it now, but I think that
> at some point we need to rework everything to avoid code duplication.

I was guessing something like that as well lately. Good to know! :)

(In reply to comment #3)
> This is still missing proper background drawing for images with alpha
> channel. Also it becomes awfully slow when rendering an image to fit
> completely in the screen and this is big enough (3888×2592).

Hmm, seems to be not filtering at all in this case here although the debug out says it does. Strange.
Comment 6 Felix Riemann 2010-08-16 13:49:11 UTC
(In reply to comment #5)
> (In reply to comment #3)
> > This is still missing proper background drawing for images with alpha
> > channel. Also it becomes awfully slow when rendering an image to fit
> > completely in the screen and this is big enough (3888×2592).
> 
> Hmm, seems to be not filtering at all in this case here although the debug out
> says it does. Strange.

Correction: it does filter, but not with the same quality as before.
Comment 7 Claudio Saavedra 2010-08-16 20:37:26 UTC
Created attachment 168005 [details] [review]
Use gdk_window_create_similar_surface() for EogScrollView
Comment 8 Claudio Saavedra 2010-08-16 20:37:33 UTC
Created attachment 168006 [details] [review]
Fully rely on cairo to render the image in EogScrollView

This is an initial patch to showcase how EogScrollView performs when
we discard all the internal optimizations and rely on cairo to render
the image. In general, performance is quite good, with a few caveats:

- Rendering svg is broken (librsvg is not building because of the
  changes in API in GTK+ 3)
- There is flickering in the rendering of the background for images
  with an alpha channel
- Images bigger than 4096×4096 seems to be quite slow (at least with
  an nvidia card)
Comment 9 Claudio Saavedra 2010-08-16 20:39:41 UTC
Comment on attachment 167792 [details] [review]
Replace GDK rendering in EogScrollView with cairo

This patch would be obsolete by the patches I have attached today.
Comment 10 Claudio Saavedra 2010-08-16 21:05:06 UTC
Created attachment 168010 [details] [review]
Enable double-buffer in EogScrollView

Now that we rely on cairo, there is no reason to try to be über-smart.
Enabling double-buffer fixes the flickering and the slowness with huge
images.
Comment 11 Claudio Saavedra 2010-08-17 13:09:01 UTC
Created attachment 168067 [details] [review]
Fix the SVG rendering

Take the transformation bits from the existing code and use them to
add proper svg rendering support.
Comment 12 Claudio Saavedra 2010-08-17 13:12:20 UTC
So, that's it. I am pretty satisfied with the performance, at least with my nvidia card. I would love to get feedback from more people as for the performance, specially when it comes to big images and svg.

Felix, you commented something on irc, about svg being slow. For me, these patches work quite well. Can you give them a try?
Comment 13 Felix Riemann 2010-08-17 21:45:47 UTC
(In reply to comment #12)
> So, that's it. I am pretty satisfied with the performance, at least with my
> nvidia card. I would love to get feedback from more people as for the
> performance, specially when it comes to big images and svg.
> 
> Felix, you commented something on irc, about svg being slow. For me, these
> patches work quite well. Can you give them a try?

I guess that was a mostly because I didn't do the clipping mask right yesterday (it was just a quick hack reenabling the old code) and thus the image was redrawn completely all the time. Also I was used to the "old" style of rendering SVGs which hides the small delay a little bit.

Re-enabling double buffering helps too.

Performance seems to be good as well (though my box is not really slow; I'll try and see what my laptop has to about this), GIF-Animations work filtered in fullscreen now. We should still keep the possibility to disable filtering, IIRC there were usecases where people wanted to view their images unfiltered.
Comment 14 Claudio Saavedra 2010-08-17 23:21:14 UTC
Created attachment 168157 [details] [review]
Remove unneeded code

Actually, #ifdef it out and comment it. It's nice code anyway.
Comment 15 Claudio Saavedra 2010-08-17 23:21:20 UTC
Created attachment 168158 [details] [review]
Replace GdkInterpType with cairo_filter_t in EogScrollView
Comment 16 Claudio Saavedra 2010-08-17 23:21:25 UTC
Created attachment 168159 [details] [review]
Honour the antialiasing-in and antialiasing-out EogScrollView properties
Comment 17 Claudio Saavedra 2010-08-17 23:34:34 UTC
(In reply to comment #13)

> Performance seems to be good as well (though my box is not really slow; I'll
> try and see what my laptop has to about this), GIF-Animations work filtered in
> fullscreen now. We should still keep the possibility to disable filtering, IIRC
> there were usecases where people wanted to view their images unfiltered.

I hadn't honoured the properties for this. Thanks for pointing it out. Now it's working fine with new the patches above.
Comment 18 Felix Riemann 2010-08-22 09:13:19 UTC
A little thing I noticed:

When zooming the images tend to get a small black border.
Comment 19 Benjamin Otte (Company) 2010-08-22 09:47:12 UTC
(In reply to comment #18)
> When zooming the images tend to get a small black border.

Assuming you're talking about zooming out:
That can be fixed by using cairo_pattern_set_extend (cairo_get_source (cr), CAIRO_EXTEND_PAD)
Comment 20 Felix Riemann 2010-08-22 11:57:28 UTC
(In reply to comment #19)
> Assuming you're talking about zooming out:
> That can be fixed by using cairo_pattern_set_extend (cairo_get_source (cr),
> CAIRO_EXTEND_PAD)

No, it affects both directions (the image just needs to be small enough to see it when zooming in).
Interestingly doesn't affect SVGs or GIF animations (alpha channel?).

cairo_pattern_set_extend didn't help, at least in the places I tried (e.g directly after setting the image as source surface).
Comment 21 Claudio Saavedra 2010-08-23 10:47:16 UTC
I can't reproduce that; can you attach an image that exposes this?
Comment 22 Felix Riemann 2010-08-23 15:23:02 UTC
Created attachment 168560 [details]
screenshot showing the borders

You can take attachment 158376 [details] from bug 615356. It gets some black borders on zooming (see attached screenshot)
Comment 23 Claudio Saavedra 2010-08-23 15:35:02 UTC
Ok, now I see them :) I had the black background enabled, that's why they weren't noticeable. I'll have a look to the cause soon.
Comment 24 Claudio Saavedra 2010-08-24 20:59:00 UTC
-       if (gdk_pixbuf_get_has_alpha (pixbuf)) {
-               format = CAIRO_FORMAT_ARGB32;
-               content = CAIRO_CONTENT_COLOR | CAIRO_CONTENT_ALPHA;
-       } else {
-               format = CAIRO_FORMAT_RGB24;
-               content = CAIRO_CONTENT_COLOR;
-       }
+       format = CAIRO_FORMAT_ARGB32;
+       content = CAIRO_CONTENT_COLOR | CAIRO_CONTENT_ALPHA;


This is basically the way to fix it.
Comment 25 Claudio Saavedra 2010-08-25 09:33:19 UTC
Created attachment 168713 [details] [review]
Improve the rendering of scaled images

Not using CAIRO_CONTENT_COLOR added a black border to images, when
painted scaled. Also, a extend mode of CAIRO_EXTEND_PAD, clipped to
the image rectangle, prevents the image borders to be drawn blending
out with the background.
Comment 26 Claudio Saavedra 2010-08-25 09:37:03 UTC
(In reply to comment #25)
> Not using CAIRO_CONTENT_COLOR ...

This should be CAIRO_CONTENT_ALPHA. Fixed the comment locally.
Comment 27 Claudio Saavedra 2010-08-31 12:00:01 UTC
Felix, do you have any objection to committing this to master?
Comment 28 Felix Riemann 2010-08-31 17:14:00 UTC
(In reply to comment #27)
> Felix, do you have any objection to committing this to master?

CLAMP (xofs, 0, xofs) == MAX (0, xofs) in attachment 168005 [details] [review]?


Go ahead. I was wondering already. :)
We should see to get a 2.91.0 test-release out soonish (let's see if r-t has some plans already) to get this (hopefully) tested more widely.
Comment 29 Felix Riemann 2010-08-31 17:14:55 UTC
(In reply to comment #28)
> (In reply to comment #27)
> > Felix, do you have any objection to committing this to master?
> 
> CLAMP (xofs, 0, xofs) == MAX (0, xofs) in attachment 168005 [details] [review]?

Uh, actually attachment 168006 [details] [review] ;)
Comment 30 Claudio Saavedra 2010-09-08 16:44:46 UTC
Ok, fixed the above and also did a few extra cleanups. Committed to master. Let's see what we get now..