True, but reverting also disables passing the resolution to the rest of Gtk for e.g. selecting the correct sizes for icons.

I believe that the problem is that CoreText takes care of rescaling fonts for Retina (this is a purely Quartz issue so we should use Apple's term) and we should just tell Pango to use 72dpi. The place in Gtk+ to do it would be https://git.gnome.org/browse/gtk+/tree/gtk/gtkwidget.c#n9015 by having a GDK_WINDOWING_QUARTZ special case, but it would be more elegant to take care of it in pangocairo_coretextfont.c or pangocairo_coretextfontmap.c. I haven't yet found the right place...

> True, but reverting also disables passing the resolution to the rest of Gtk for e.g. selecting the correct sizes for icons.

Is there any place in gtk3-demo/widget-factory etc where I can see that problem?

I'm not at all sure that there is one, and I'm travelling with a non-Retina laptop so I can't test right now. The only user of gdk_screen_get_resolution() seems to be GtkCSS--gtkwidget's update_pango_context calls pango_cairo_context_set_resolution with GTK_CSS_PROPERTY_DPI. Most internal code seems to use either gdk_monitor_get_scale_factor or gdk_window_get_scale_factor, both of which call backingScaleFactor directly (it returns 1 for regular screens and 2 for Retina ones; [NSScreen deviceDescription][DisplayResolution] returns 72 and 144 respectively). That may mean that only themed graphic elements are affected by gdk_screen_get_resolution working as designed.

Hm, ok, thanks. Btw, I'm on non-retina as well, but you can enable the interface scaling using the Quartz Debug tool:

https://developer.apple.com/library/content/documentation/GraphicsAnimation/Conceptual/HighResolutionOSX/Testing/Testing.html

Created attachment 351992 [details] [review]
Don't scale CoreText fonts by screen resolution.

The root cause appears to be that pangocoretext_fontmap scales fonts based on the screen resolution, determined from the pangocairo_context. That doesn't seem to be necessary so removing it (patch attached) corrects the problem.

Will this work with gtk+2?

I didn't test it but I can't think of any reason it should matter.

Review of attachment 351992 [details] [review]:

::: pango/pangocoretext-fontmap.c
@@ +828,2 @@
   double size = pango_font_description_get_size (desc);
 

This doesn't look right to me.  What pango_font_description_get_size() returns in this case is in points.  And we want a pixelsize, for whatever "pixel" means.  The conversion needs to happen.

Now, it looks like, there's disagreement about what pixel is used in different places.  One happens to be device pixel, eg. pango_core_text_font_map_get_resolution() returns 144, but then the value is used to mean CSS pixel, which is twice bigger because of the retina scaling.

Whatever you do, you should align the meaning of pixel used.  This patch is definitely not the right way to do that.

To me, it looks like PangoCoreTextFontKey was blindly copied from PangoFcFontKey.  Reality is, CoreText happens to work with point sizes in the api.  Ie. in _pango_cairo_core_text_font_new(), this code:

  abs_size = pango_core_text_font_key_get_absolute_size (key); 
  size = pango_units_to_double (abs_size); 
 
  size /= pango_matrix_get_font_scale_factor (pango_core_text_font_key_get_matrix (key)); 
 
  ctdescriptor = pango_core_text_font_key_get_ctfontdescriptor (key); 
  font_ref = CTFontCreateWithFontDescriptor (ctdescriptor, size, NULL); 

is wrong because size here is in pixels, while CTFontCreateWithFontDescriptor() expects a point size.

So, correct fix is to change PangoCoreTextFontKey to remove pixelsize and resolution and add int size. Then in get_scaled_size, you do the opposite of what's happening now.  Ie. make sure returned value is in points, not pixel.  In reality, it will have the effect of this block never running.

Does that make sense?

Thanks for the detailed explanation. I think I understand, let's check:

All of the CoreText sizing is inverted, right? The font description returns a size in points, CTFontCreateWithFontDescriptor expects points. So while the code makes it look like the size is always absolute (i.e. pixels) it's really never absolute, it's always pixels. So I need to go through all of the CT code and make sure that that's clearly communicated; for example cafont->abs_size needs to be cafont->point_size because that's what it is. Then I need to make sure that only the layout code, the part that needs to know how big a glyph is in pixels, gets pixels and everything else gets points because that's what the CTFont/CTFontDescriptor API expects.

Do I need to worry about the transformation matrix? CTFont provides a CTFontGetMatrix but it's not used in Pango.

(In reply to John Ralls from comment #9)
> Thanks for the detailed explanation. I think I understand, let's check:
> 
> All of the CoreText sizing is inverted, right? The font description returns
> a size in points, CTFontCreateWithFontDescriptor expects points. So while
> the code makes it look like the size is always absolute (i.e. pixels) it's
> really never absolute, it's always pixels. So I need to go through all of
> the CT code and make sure that that's clearly communicated; for example
> cafont->abs_size needs to be cafont->point_size because that's what it is.

Sounds about right.

> Then I need to make sure that only the layout code, the part that needs to
> know how big a glyph is in pixels, gets pixels and everything else gets
> points because that's what the CTFont/CTFontDescriptor API expects.

Do we actually see real dpi at all?  Or is it fixed to 72 for non-retina no matter what actual dpi is?  It might be the case that they always use 72.

You have to decide what we want for "pixel" size during layout.  Should that be the device pixel (ie. 2x larger pixel size on retina), or some conceptual abstract pixel (similar to what CSS pixel is).  You then advertise layout results to be in that space.

I think whatever cairo Quartz backend uses should be what you use.  Or, whatever any cairo surface does.  And doesn't look like we have a non-cairo CoreText backend anyway.


> Do I need to worry about the transformation matrix? CTFont provides a
> CTFontGetMatrix but it's not used in Pango.

Well, probably don't worry about CTFontGetMatrix because we create the CTFont and we don't use it.  But you do need to worry about cairo / Pango transformation matrix.  That's already part of the code, so just don't break it.

I know I've broken pangoft2 with transformation many times.  It might even be broken right now, eg. kerning doesn't correctly scale when transformation matrix is present...

(In reply to Behdad Esfahbod from comment #10)
> (In reply to John Ralls from comment #9)
> 
> > Then I need to make sure that only the layout code, the part that needs to
> > know how big a glyph is in pixels, gets pixels and everything else gets
> > points because that's what the CTFont/CTFontDescriptor API expects.
> 
> Do we actually see real dpi at all?  Or is it fixed to 72 for non-retina no
> matter what actual dpi is?  It might be the case that they always use 72.

As I reported in comment 3, we get either 72 or 144 depending on whether MacOS thinks it's a Retina display or not, regardless of actual screen resolution or size.

> 
> You have to decide what we want for "pixel" size during layout.  Should that
> be the device pixel (ie. 2x larger pixel size on retina), or some conceptual
> abstract pixel (similar to what CSS pixel is).  You then advertise layout
> results to be in that space.
> 
> I think whatever cairo Quartz backend uses should be what you use.  Or,
> whatever any cairo surface does.  And doesn't look like we have a non-cairo
> CoreText backend anyway.

I think you mean a 4x smaller pixel for retina: A non-retina pixel is nominally the same physical size as a 2 x 2 cell of retina ones.

Yes, tailoring the result to what Cairo's expecting, both here and in GDK, makes perfect sense. Cairo is presently the only way we can draw on a Quartz screen so a non-Cairo CoreText backend wouldn't be very useful.

> 
> 
> > Do I need to worry about the transformation matrix? CTFont provides a
> > CTFontGetMatrix but it's not used in Pango.
> 
> Well, probably don't worry about CTFontGetMatrix because we create the
> CTFont and we don't use it.  But you do need to worry about cairo / Pango
> transformation matrix.  That's already part of the code, so just don't break
> it.

We don't use the CTFont to actually draw on the surface, but we do use it to get the metrics to directly draw the TT font, right? Isn't that the point of the CoreText backend?

(In reply to John Ralls from comment #11)

> We don't use the CTFont to actually draw on the surface, but we do use it to
> get the metrics to directly draw the TT font, right? Isn't that the point of
> the CoreText backend?

The main reason is for font discovery and enumeration.  Otherwise even metrics we get from cairo in pangocairo backend.  So, we just need to get hold of a CTFont or CGFont, pass it to cairo, and from there everything is handled through cairo.

Created attachment 352904 [details] [review]
Remove absolute size and dpi scaling from the CoreText backend.

After spending some more time studying the code I decided that cafont->abs_size can just go away. Nothing needs "pixel size", all of that is taken care of by CoreGraphics in real use. pango_core_text_font_key_get_absolute_size became pango_core_text_font_key get_size, key->pixelsize became key->pointsize, and pango_core_text_font_describe_absolute_size went away for the same reason. 

There was a block in pango_core_text_fontset_new that fiddled with the size of the "best_description" returned from find_best_match, but then best_description was just freed without being used for anything so I removed the block and made best_description internal to find_best_match.

I redirected the implementation of pango_font_describe_with_absolute_size to pango_core_text_font_describe because of the dire g_warning about it being a bug to leave it unimplemented. 

The layout test is a bit of an anomaly because it depends on a particular numerical result rather than a visually correct display--after all, one can't automatically test for the latter. The scale it depends on in other backends is 96 DPI and get_scaled_text () made the adjustment, applying a 4/3 (96/72) scaling factor. With that facility removed the test naturally failed. I compensated by replacing the font with a larger one when compiled for CoreText. I opted for a simple conditional compilation because it appears to me that's the default fontmap if it's available and testing for the actual fontmap type is a lot more complicated.

I've done a quick review of this, I don't think I can review it more without having a deeper understanding of CoreText.  The one part that bothers me is:


  /* There's no such thing as "absolute" with CoreText, so just return
   * the regular description.
   */
  font_class->describe_absolute = pango_core_text_font_describe;

I'm uneasy about describe_absolute returning a description without absolute size.  I'm not sure what can break, but I guess getting your patch in is more important than figuring out any possible side effect.

Thanks.

So maybe rather than saying "There's no such thing as absolute" I should say "In CoreText the device unit is points so describe_absoulute and describe are the same".

Does that make you more comfortable?

(In reply to John Ralls from comment #15)
> So maybe rather than saying "There's no such thing as absolute" I should say
> "In CoreText the device unit is points so describe_absoulute and describe
> are the same".
> 
> Does that make you more comfortable?

I think I'm willing to let this go in and deal with any breakage later.  Thanks. :)

(In reply to Behdad Esfahbod from comment #16)
> 
> I think I'm willing to let this go in and deal with any breakage later. 
> Thanks. :)

I take that as accepted-commit-now, so I did, with the comment change from comment 15.

This change broke gdk_screen_set_resolution on quartz backend - fonts are no longer scaled accordingly. I want to pass to gdk_screen_set_resolution a real DPI (computed from dimensions of screen and screen resolution in pixels) and have correct size of font (as it is on Linux). E.g. see your test - you should have just set DPI to 96 and not change font size - it would have resulted in test passing, instead you just hacked around the failure just so test would pass;))
As for original issue - Retina can be detected using backingScaleFactor and DPI corrected back to 72 that way (and in any case why detect it at all if it's always reported as 72/144??).

(In reply to parafin from comment #18)
> This change broke gdk_screen_set_resolution on quartz backend - fonts are no
> longer scaled accordingly. I want to pass to gdk_screen_set_resolution a
> real DPI (computed from dimensions of screen and screen resolution in
> pixels) and have correct size of font (as it is on Linux). E.g. see your
> test - you should have just set DPI to 96 and not change font size - it
> would have resulted in test passing, instead you just hacked around the
> failure just so test would pass;))
> As for original issue - Retina can be detected using backingScaleFactor and
> DPI corrected back to 72 that way (and in any case why detect it at all if
> it's always reported as 72/144??).

MacOS != Linux and Quartz != X11/Wayland. The 96DPI, in fact all of that scaling stuff, was lifted from the X11 backend without much thought. Quartz does it differently and in fact takes care of scaling the fonts for Retina at a lower level. There's no need to 

No, I didn't hack anything "so the test would pass". I changed gtk and pango together so that Gtk applications behave like native Mac applications, see 

Please see bug 787867 for more on this topic.