I also have the same issue in 3.10.x on a highDPI screen.

May any of you try to replicate the error with poppler-glib-demo
from master?

Considering you already have the dependencies for 3.10,
you only need the development packages from your distro,
grab poppler from master and that is.

Thanks for the reply.  Tried and still appears to be the same.

Evince > About gives 
Document Viewer
Using poppler/cairo (0.25.1)

The problem lies in the fact that we should override the Scale setting, as the evince view is already supposed to handle real dpi of the screen. However, with the moving form X11 to Wayland, it is not clear that everything is working out (we are using some API that does not even guarantee to give you proper DPI on X11, see the bug about Properly detecting DPI).

Okay thanks.  Will there be a fix before Wayland comes out?  It is cumbersome to open using GDK_SCALE=1 every time.

Just wanted to note that I'm working on this - I have some rough patches that get evince to smooth rendering, but don't handle dynamic scale changes, etc. Hopefully I'll have something I can post within the next week.

Question for evince developers - is a dependency on GTK+ 3.10 OK?

The sidebar with thumbnails is in particular rather tricky to handle without a 3.10 dependency since gtkcellrendererpixbuf only gained cairo_surface_t support in 3.10, and pixbufs are inherently rendered lo-dpi. So without a 3.10 requirement, there will messy conditionalization for pre-3.10 and GTK+ 3.10.

(In reply to comment #7)
> Question for evince developers - is a dependency on GTK+ 3.10 OK?

Yes.

> The sidebar with thumbnails is in particular rather tricky to handle without a
> 3.10 dependency since gtkcellrendererpixbuf only gained cairo_surface_t support
> in 3.10, and pixbufs are inherently rendered lo-dpi. So without a 3.10
> requirement, there will messy conditionalization for pre-3.10 and GTK+ 3.10.

Just bump the requirements

Created attachment 270528 [details] [review]
EvSidebarThumbnails: switch to using cairo_surface_t for images

To support high resolution thumbnails on hi-dpi displays, we need to
use cairo surfaces instead of using GdkPixbuf for images. In preparation
for high resolution thumbnails, switch over to cairo_surface_t. This
adds a GTK+ 3.10 dependency.

Add new:
ev_document_misc_render_loading_thumbnail_surface()
ev_document_misc_render_thumbnail_surface_with_frame()

To avoid converting surface => pixbuf => surface.

Created attachment 270529 [details] [review]
Add a configure check for hi-dpi support

In order to support hi-dpi (GDK_SCALE) we need new-enough versions
of both GTK+ and cairo. Add a configure check for the appropriate
functions and define HAVE_HIDPI_SUPPORT.

Created attachment 270530 [details] [review]
EvSidebarThumbnails: render thumbnails correctly for hi-dpi displays

Render thumbnails with resolution from gtk_widget_get_scale_factor() and
recreate them when the scale change. Make the thumbnail-rendering helper
functions create a hi-resolution surface when the widget has a scale
factor.

Created attachment 270531 [details] [review]
EvView: render correctly on hi-dpi displays

Make EvPixbufCache generate surfaces with extra resolution based on
gtk_widget_get_scale_factor(). Handle cairo surfaces with a device
scale in ev_view_draw(). Trigger an update of the pixbuf cache when
the scale factor changes.

Created attachment 270532 [details] [review]
EvPresentationView: Render correctly on hi-dpi displays

Create rendering jobs with a scale that incorporate the scale factor of
the widget, and then use cairo_surface_set_device_scale() to make the
resulting surfaces render at the correct size. Handle changes to the scale
factor both for the cached surfaces, and also for the monitor dimensions,
which are reported in scaled coordinates.

So this is very nice, but for some reason, with GDK_SCALE=2 the rendering looks a little blurried with respect to the rendering at GDK_SCALE=1.

Created attachment 270533 [details]
test case

to the left evince with GDK_SCALE=1 and to the right evince with the GDK_SCALE=2

(In reply to comment #13)
> Created an attachment (id=270532) [details] [review]
> EvPresentationView: Render correctly on hi-dpi displays
> 
> Create rendering jobs with a scale that incorporate the scale factor of
> the widget, and then use cairo_surface_set_device_scale() to make the
> resulting surfaces render at the correct size. Handle changes to the scale
> factor both for the cached surfaces, and also for the monitor dimensions,
> which are reported in scaled coordinates.

Hi,

Thanks for working on this.  I am happy to test out any changes if you like.

Thanks,

Darcy.

I can confirm the slight fuzziness, but note that if I select some text and deselect it, it's re-displayed noticeably sharper. There's also a slight discontinuity with the underlying rendering, like things are positioned or scaled off by a pixel somewhere.

Review of attachment 270528 [details] [review]:

Looks good to me, thanks. Please add the new symbols to libevdocument-sections.txt.

::: shell/ev-sidebar-thumbnails.c
@@ +829,1 @@
+        thumbnail = gdk_cairo_surface_create_from_pixbuf (job->thumbnail,

Now we could avoid this conversion too, since the thumbnail job used pixbufs to avoid the conversion here. In the pdf backend, thumbnails are converted to pixbuf form the original cairo surface. But we can do this in a different commit.

Review of attachment 270529 [details] [review]:

Yes, we really need a new cairo release.

(In reply to comment #14)
> So this is very nice, but for some reason, with GDK_SCALE=2 the rendering looks
> a little blurried with respect to the rendering at GDK_SCALE=1.

I've tracked this down now - the problem is that the actual size of the surface we end up with has more precision than the size we assume it has in the rendering code - the rendering code assumes everything is integral in logical pixels, but the surface ends up with a size in device pixels.

So we might think that our target size is 150px by 200px (logical) but the scale times the size of page is closer to 301 device pixels by 399 device pixels - 150.5px by 199.5px.  

So we draw with a scale in draw_surface. There's actually a bug in draw_surface() that results in fuzziness instead of a duplicated or omitted line in the result - it calls:

        cairo_pattern_set_filter (cairo_get_source (cr),
                                  CAIRO_FILTER_FAST);

*before*:

        cairo_set_source_surface (cr, surface, 0, 0);

So it has no effect. However, we can't fix the fuzziness by fixing the filter bug, because duplicating or omitting a line looks awful too.

(You'll have to figure out if you want to fix this when scaling the content before a new version renders... I think it's probably more attractive to use the default filter than FILTER_FAST, and the default bilinear filter won't be slow)

We also can't fix the fuzziness by just rendering the odd-sized surface unscaled - because we count on being able to closely wrap GTK+ drawn boundaries of the page around the surface using gtk_render_frame() - and that requires integer logical pixels.

So it seems like the right tweak might be to adjust the scale that we pass in the EvRenderContext so that we get *exactly* the integer-logical-pixels size that we want. But this also has a problem - there's no guarantee that the same scale will give us integer logical pixels in both the X and Y direction.

So this would require allowing passing in (very slightly) different X and Y scales to EvRenderContext. Which requires adjustment of every backend :-( - though in a quick look there shouldn't be any fundamental problem making that adjustment for any of the backends.

I don't have any other ideas. Anybody else have an idea for an easier fix?

(In reply to comment #20)
> (In reply to comment #14)
> > So this is very nice, but for some reason, with GDK_SCALE=2 the rendering looks
> > a little blurried with respect to the rendering at GDK_SCALE=1.
> 
> I've tracked this down now - the problem is that the actual size of the surface
> we end up with has more precision than the size we assume it has in the
> rendering code - the rendering code assumes everything is integral in logical
> pixels, but the surface ends up with a size in device pixels.
> 
> So we might think that our target size is 150px by 200px (logical) but the
> scale times the size of page is closer to 301 device pixels by 399 device
> pixels - 150.5px by 199.5px.  
> 
> So we draw with a scale in draw_surface. There's actually a bug in
> draw_surface() that results in fuzziness instead of a duplicated or omitted
> line in the result - it calls:
> 
>         cairo_pattern_set_filter (cairo_get_source (cr),
>                                   CAIRO_FILTER_FAST);
> 
> *before*:
> 
>         cairo_set_source_surface (cr, surface, 0, 0);
> 
> So it has no effect. However, we can't fix the fuzziness by fixing the filter
> bug, because duplicating or omitting a line looks awful too.

Yes, this is only expected to happen when zoom changes, and the surface at the new scale hasn't been rendered yet by the backend. We render the current page scaled using a fast filter to provide early feedback, but at some point the new surface with the right size is rendered by the backend and it's what we use without any additional scale. Of course this doesn't work id the surface we get from the backedn doesn't have the expected size.

> (You'll have to figure out if you want to fix this when scaling the content
> before a new version renders... I think it's probably more attractive to use
> the default filter than FILTER_FAST, and the default bilinear filter won't be
> slow)

Yes, the idea of using fast was to render it faster, but the fact that this has never worked and nobody has noticed it, proofs that default filter is fast enough :-)

> We also can't fix the fuzziness by just rendering the odd-sized surface
> unscaled - because we count on being able to closely wrap GTK+ drawn boundaries
> of the page around the surface using gtk_render_frame() - and that requires
> integer logical pixels.
> 
> So it seems like the right tweak might be to adjust the scale that we pass in
> the EvRenderContext so that we get *exactly* the integer-logical-pixels size
> that we want. But this also has a problem - there's no guarantee that the same
> scale will give us integer logical pixels in both the X and Y direction.
> 
> So this would require allowing passing in (very slightly) different X and Y
> scales to EvRenderContext. Which requires adjustment of every backend :-( -
> though in a quick look there shouldn't be any fundamental problem making that
> adjustment for any of the backends.
> 
> I don't have any other ideas. Anybody else have an idea for an easier fix?

I think we can change EvRenderContext to use scale_x and scale_y.

Created attachment 270843 [details] [review]
libevdocument-sections.txt: Add new symbols

Add symbols for rendering as a cairo surface rather than a pixbuf.

Created attachment 270844 [details] [review]
EvPixbufCache: Handle backends that don't render a selection

Handle the case where ev_selection_render_selection() doesn't actually
render a surface - this happens when a backend doesn't implement
render_selection().

Created attachment 270845 [details] [review]
Switch to specifying rendered output in pixels, not as a scale

Here's a patch that implements a variation of the idea of scale_x/scale_y -
it switches things so that the a target pixel size can be specified
for the rendering rather than a scale.

* I've tried to maintain API compatibility for libview/libdocument -
  things would be a bit cleaner and simpler if, e.g.,
  ev_job_thumbnail_new() were just changed rather than adding a new
  ev_job_thumbnail_new_with_target_size().

* For me, rotated rendering with the PS backend is completely broken
  before this patch (evince crashes when you rotate the page because 
  libspectre and the backend are disagreeing about what the dimensions
  are of the generated data.) I've just left it doing the same calculations
  as before.

* I didn't change _ev_view_transform_view_point_to_doc_point() and friends
  to do an exact computation using the page size - I left the old computation
  using using view->scale. The result will be less than 1 logical pixel off,
  but the exact computation wouldn't be more complicated. The reason I didn't
  do it is that the code seems to have some quirks - one direction tries to
  handle rotation, the other direction doesn't, it seems to handle the border
  wrong - and I didn't want to mix fixing that (or breaking if I misunderstand)
  with this patch. 

I've also attached a couple of small fixups to my last patches here.

======

Passing a scale to the backend meant that we were implicitly counting
on the backend code and front-end code to do exactly the same calculation
to get the rendered size of a page. Instead switch to passing a
target size in pixels to the backend.

This, among other things, allows us to make sure that we render at a size
that is integer device pixels in both X and Y directions.

ev-render-context.[ch]: Add ev_render_context_set_target_size() and
three helper functions:

 ev_render_context_compute_scaled_size(): get the pixel size to render at
 ev_render_context_compute_transformed_size(): size including effect of rotation
 ev_render_context_compute_scales(): get the corresponding xscale/yscale.

ev-jobs.[ch]: Add ev_job_thumbnail_new_with_target_size(), and pass the
target size for thumbnail and render jobs (which already had a target
size) to the backends.

backend/*: Use the helper functions rather than directly accessing
 render_context->scale.

ev-pixbuf-cache.c ev-view.c ev-view-presentation.c ev-sidebar-thumbnails.c
ev-window.c: Switch to passing target sizes rather than scales when
rendering.

(In reply to comment #24)
> Created an attachment (id=270845) [details] [review]
> Switch to specifying rendered output in pixels, not as a scale
> 
> Here's a patch that implements a variation of the idea of scale_x/scale_y -
> it switches things so that the a target pixel size can be specified
> for the rendering rather than a scale.

Sounds good.

> * I've tried to maintain API compatibility for libview/libdocument -
>   things would be a bit cleaner and simpler if, e.g.,
>   ev_job_thumbnail_new() were just changed rather than adding a new
>   ev_job_thumbnail_new_with_target_size().

I appreciate it.

> * For me, rotated rendering with the PS backend is completely broken
>   before this patch (evince crashes when you rotate the page because 
>   libspectre and the backend are disagreeing about what the dimensions
>   are of the generated data.) I've just left it doing the same calculations
>   as before.

Oops, I'll look at it.

> * I didn't change _ev_view_transform_view_point_to_doc_point() and friends
>   to do an exact computation using the page size - I left the old computation
>   using using view->scale. The result will be less than 1 logical pixel off,
>   but the exact computation wouldn't be more complicated. The reason I didn't
>   do it is that the code seems to have some quirks - one direction tries to
>   handle rotation, the other direction doesn't, it seems to handle the border
>   wrong - and I didn't want to mix fixing that (or breaking if I misunderstand)
>   with this patch. 

I committed some fixed related to that during the weekend.

> I've also attached a couple of small fixups to my last patches here.
> 
> ======
> 
> Passing a scale to the backend meant that we were implicitly counting
> on the backend code and front-end code to do exactly the same calculation
> to get the rendered size of a page. Instead switch to passing a
> target size in pixels to the backend.
> 
> This, among other things, allows us to make sure that we render at a size
> that is integer device pixels in both X and Y directions.
> 
> ev-render-context.[ch]: Add ev_render_context_set_target_size() and
> three helper functions:
> 
>  ev_render_context_compute_scaled_size(): get the pixel size to render at
>  ev_render_context_compute_transformed_size(): size including effect of
> rotation
>  ev_render_context_compute_scales(): get the corresponding xscale/yscale.
> 
> ev-jobs.[ch]: Add ev_job_thumbnail_new_with_target_size(), and pass the
> target size for thumbnail and render jobs (which already had a target
> size) to the backends.
> 
> backend/*: Use the helper functions rather than directly accessing
>  render_context->scale.
> 
> ev-pixbuf-cache.c ev-view.c ev-view-presentation.c ev-sidebar-thumbnails.c
> ev-window.c: Switch to passing target sizes rather than scales when
> rendering.

Looks good in general, I'll review the patches in more detail when I have some free time. Thanks!

*** Bug 726508 has been marked as a duplicate of this bug. ***

Comment on attachment 270528 [details] [review]
EvSidebarThumbnails: switch to using cairo_surface_t for images

Fixed a couple of issues in the gtk-docs and pushed to git master

Comment on attachment 270843 [details] [review]
libevdocument-sections.txt: Add new symbols

I've squashed this in the commit that added the symbols

(In reply to comment #18)
> Review of attachment 270528 [details] [review]:
> 
> Looks good to me, thanks. Please add the new symbols to
> libevdocument-sections.txt.
> 
> ::: shell/ev-sidebar-thumbnails.c
> @@ +829,1 @@
> +        thumbnail = gdk_cairo_surface_create_from_pixbuf (job->thumbnail,
> 
> Now we could avoid this conversion too, since the thumbnail job used pixbufs to
> avoid the conversion here. In the pdf backend, thumbnails are converted to
> pixbuf form the original cairo surface. But we can do this in a different
> commit.

I've also fix this in git master.

Comment on attachment 270845 [details] [review]
Switch to specifying rendered output in pixels, not as a scale

I've rebased this patch, and removed the device scale factor bits, since this is actually independent of the device scale factor. I prefer to do this before and then implement the hiDPI support on top of this. I've just pushed it to git master. Thanks!

(In reply to comment #25)
> (In reply to comment #24)
> > * For me, rotated rendering with the PS backend is completely broken
> >   before this patch (evince crashes when you rotate the page because 
> >   libspectre and the backend are disagreeing about what the dimensions
> >   are of the generated data.) I've just left it doing the same calculations
> >   as before.
> 
> Oops, I'll look at it.

It seems to work here with and without the patch. Does it happen to you with any PS document?
 
> > * I didn't change _ev_view_transform_view_point_to_doc_point() and friends
> >   to do an exact computation using the page size - I left the old computation
> >   using using view->scale. The result will be less than 1 logical pixel off,
> >   but the exact computation wouldn't be more complicated. The reason I didn't
> >   do it is that the code seems to have some quirks - one direction tries to
> >   handle rotation, the other direction doesn't, it seems to handle the border
> >   wrong - and I didn't want to mix fixing that (or breaking if I misunderstand)
> >   with this patch. 
> 
> I committed some fixed related to that during the weekend.

I'll see if we can update the transform methods to use the page size computation now.

Created attachment 276217 [details] [review]
EvPresentationView: Render correctly on hi-dpi displays

Rebased against master

Created attachment 276218 [details] [review]
EvView: render correctly on hi-dpi displays

Rebased against master

Created attachment 276219 [details] [review]
EvPixbufCache: Handle backends that don't render a selection

Rebased against master

Those patches work beautifully here with a PDF document on a "retina" display. The thumbnails seem to be blurry still though (I can attach a screenshot if needed).

Review of attachment 276218 [details] [review]:

Ok, addressed my own comments and pushed to git master. Thanks!

::: libview/ev-pixbuf-cache.c
@@ +353,3 @@
 	g_assert (job_info);
 
+	device_scale = get_device_scale (pixbuf_cache);

This could be moved after the early return below.

@@ +686,3 @@
 	job_info->job = ev_job_render_new (pixbuf_cache->document,
+					   page, rotation, scale * job_info->device_scale,
+					   width * job_info->device_scale, height * job_info->device_scale);

I think this is easier to read now using a new line per parameter.

::: libview/ev-view.c
@@ +206,3 @@
 static void       on_adjustment_value_changed                (GtkAdjustment      *adjustment,
 							      EvView             *view);
+static void	  on_notify_scale_factor		     (EvView		 *view,

We can define this before it's used.

@@ +6011,3 @@
 {
+	gdouble width, height;
+	gdouble x_device_scale, y_device_scale;

I prefer device_scale_x, device_scale_y for consistenty

@@ +6016,3 @@
+	cairo_surface_get_device_scale (surface, &x_device_scale, &y_device_scale);
+#else
+	x_device_scale = y_device_scale = 1;

We can initialize these to 1 when declared to get rid of the #else

@@ +6178,3 @@
 			scale_y = (gdouble)height / cairo_image_surface_get_height (page_surface);
+
+			cairo_surface_get_device_scale (page_surface, &device_scale_x, &device_scale_y);

We need a #ifdef HAVE_HIDPI_SUPPORT here as well

Review of attachment 276219 [details] [review]:

oh, this should have squashed in previous commit. Pushed to git master anyway.

Created attachment 281482 [details] [review]
EvSidebarThumbnails: render thumbnails correctly for hi-dpi displays

Updated this patch too, I think it's working now, please test it and confirm it works.

Review of attachment 276217 [details] [review]:

Pushed!

Review of attachment 281482 [details] [review]:

It works ok for me.

Comment on attachment 281482 [details] [review]
EvSidebarThumbnails: render thumbnails correctly for hi-dpi displays

Thanks for testing, pushed!

Done!

Works for me too.  Great, and thank you very much for the hard work!