[PATCH app/xdpyinfo v2] Use XRANDR 1.2 extension for reporting dimensions and resolution per output

[Pali Rohár]

Hi!

On Wednesday 04 April 2018 09:47:59 Giuseppe Bilotta wrote:
> Hello Pali,
> 
> I like the idea of this patch, wasn't aware of its existence, thanks
> for pinging me about it. A few comments below:
> 
> On Tue, Apr 3, 2018 at 10:15 PM, Pali Rohár <pali.rohar at gmail.com> wrote:
> >> @@ -455,27 +462,75 @@ print_screen_info(Display *dpy, int scr)
> >>      double xres, yres;
> >>      int ndepths = 0, *depths = NULL;
> >>      unsigned int width, height;
> >> -
> >> -    /*
> >> -     * there are 2.54 centimeters to an inch; so there are 25.4 millimeters.
> >> -     *
> >> -     *     dpi = N pixels / (M millimeters / (25.4 millimeters / 1 inch))
> >> -     *         = N pixels / (M inch / 25.4)
> >> -     *         = N * 25.4 pixels / M inch
> >> -     */
> 
> You may want to keep this comment here, rather than in the else block below,
> since the formula applies to any conversion, regardless if it's core
> or per-output DPI.

Ok, no problem.

> >> -    xres = ((((double) DisplayWidth(dpy,scr)) * 25.4) /
> >> -         ((double) DisplayWidthMM(dpy,scr)));
> >> -    yres = ((((double) DisplayHeight(dpy,scr)) * 25.4) /
> >> -         ((double) DisplayHeightMM(dpy,scr)));
> >> +#ifdef XRANDR
> >> +    int event_base, error_base;
> >> +    int major, minor;
> >> +    XRRScreenResources *res = NULL;
> >> +    XRROutputInfo *output;
> >> +    XRRCrtcInfo *crtc;
> >> +#endif
> >>
> >>      printf ("\n");
> >>      printf ("screen #%d:\n", scr);
> >> -    printf ("  dimensions:    %dx%d pixels (%dx%d millimeters)\n",
> >> -         XDisplayWidth (dpy, scr),  XDisplayHeight (dpy, scr),
> >> -         XDisplayWidthMM(dpy, scr), XDisplayHeightMM (dpy, scr));
> >> -    printf ("  resolution:    %dx%d dots per inch\n",
> >> -         (int) (xres + 0.5), (int) (yres + 0.5));
> 
> I would unconditionally show the core output, regardless of RANDR
> state. Even if it's fictitious when RANDR is enabled,
> it can still be useful to spot inconsistencies. It also ensures that
> the xdpyinfo output is "less" broken by this patch (search for
> xdpyinfo grep resolution to get an idea of how used this is as a debug tool).

XRANDR code below provides that 'grep resolution' support correctly and
in the same format. It is there for all those scripts which expects
resolution and dimensions to be correct.

> >> +
> >> +#ifdef XRANDR
> >> +    if (XRRQueryExtension (dpy, &event_base, &error_base) &&
> >> +        XRRQueryVersion (dpy, &major, &minor) &&
> >> +        (major > 1 || (major == 1 && minor >= 2)) &&
> >> +        (res = XRRGetScreenResources (dpy, RootWindow (dpy, scr))))
> >> +    {
> 
> I'm pondering whether it would be a good idea to print the RANDR
> version here, something like:
> printf("  RANDR (%d.%d) outputs:\n", major, minor);
> and then nesting the per-output data even more.

I as a user, would expect to find XRANDR version in "xrandr" utility
output, not in xdpyinfo output.

But if you really think that it would make sense to have it also in
xdpyinfo, it can be included somewhere...

> >> +        for (i = 0; i < res->noutput; ++i) {
> >> +            output = XRRGetOutputInfo (dpy, res, res->outputs[i]);
> >> +            if (!output || !output->crtc || output->connection != RR_Connected)
> >> +                continue;
> >> +
> >> +            crtc = XRRGetCrtcInfo (dpy, res, output->crtc);
> >> +            if (!crtc) {
> >> +                XRRFreeOutputInfo (output);
> >> +                continue;
> >> +            }
> >> +
> >> +            printf ("  output: %s\n", output->name);
> >> +            printf ("    dimensions:    %ux%u pixels (%lux%lu millimeters)\n",
> >> +                    crtc->width, crtc->height, output->mm_width, output->mm_height);
> >> +
> >> +            if (output->mm_width && output->mm_height) {
> >> +                xres = ((((double) crtc->width) * 25.4) / ((double) output->mm_width));
> >> +                yres = ((((double) crtc->height) * 25.4) / ((double) output->mm_height));
> >> +            } else {
> >> +                xres = 0;
> >> +                yres = 0;
> >> +            }
> >> +            printf ("    resolution:    %dx%d dots per inch\n",
> >> +                    (int) (xres + 0.5), (int) (yres + 0.5));
> 
> This doesn't work correctly when the display is rotated, since the
> width/height in pixel will follow the orientation,
> but the physical sizes won't. You should probably take that into
> account, and possibly show the output orientation (e.g. in the
> dimensions line, or in the name line if you do add the "RANDR
> <version> outputs" header).

Hm... I have not thinking about it. If this is a real problem I can look
at it and try to fix it.

> Also (please don't hate me), for RANDR versions 1.5 or higher we
> should include RANDR monitors too (possibly in place of individual
> outputs, I'm not sure about this actually, it depends on how detailed
> we want to be; I would go with both, but then again I'm an information
> junkie).

I know that I RANDR 1.5+ supports "monitors" which is needed for display
port outputs, but unfortunately I do not have such configuration for
testing. And I do not like idea to write and send code without real
testing.

This patch is just to fix long standing bug, that scripts which parse
xdpyinfo output and users who looking at it too just get wrong
information about dimensions and DPI.

-- 
Pali Rohár
pali.rohar at gmail.com