Skip to content
This repository has been archived by the owner on Mar 30, 2022. It is now read-only.

(WIP) Attempt at adding metadata to PNG output #66

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ammgws
Copy link
Contributor

@ammgws ammgws commented Dec 16, 2019

An attempt at the PNG part for #6.

Brought over relevant code from cairo's cairo-png.c and added png_set_pHYs call.

At the moment it compiles but I've probably done something silly as only an empty file is generated. Opening WIP (Work in progress) PR now in case someone has time to look over the code in the meantime.

P.S.
What tool should I be using to format the files in order to honour the styling of this project? e.g. line length, braces, etc

@emersion
Copy link
Owner

Example libpng code, if that helps: https://github.com/swaywm/wlroots/blob/master/examples/screencopy.c#L171

@ammgws
Copy link
Contributor Author

ammgws commented Dec 20, 2019

Cheers, I rewrote based on that and can at least get a file written to disk now, and have confirmed the scale metadata is there too.

Resolution reported in "Image properties" in GIMP:

  • Before this PR: 72 × 72 ppi
  • After after this PR: 11.811 × 11.811 pixels/mm

Now I just have to figure out how to get libpng to write to a buffer instead of to a file and this PR should be done.

PNG_FILTER_TYPE_DEFAULT);

#ifdef PNG_pHYs_SUPPORTED
cairo_surface_get_fallback_resolution(sfc, &dpix, &dpiy);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should get the px/mm value from the monitor's physical size: https://github.com/emersion/grim/blob/master/main.c#L113

Copy link
Contributor Author

@ammgws ammgws Dec 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Righto. For the case of a single output it would just be something like pixels_x/physical_width, but now I'm wondering how this should work for when the screenshot contains multiple outputs.

something like get_output_layout_extents / (sum of all physical sizes in the layout)?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, except the screenshot region could partially span over multiple outputs. In general it may be even more complicated because different outputs could cover the same region.

I wonder if there's a good way to handle this.

@emersion
Copy link
Owner

General style note: we don't align, we use tabs

@emersion
Copy link
Owner

emersion commented Oct 8, 2021

#102 adds manual libpng calls.

@ammgws
Copy link
Contributor Author

ammgws commented Oct 8, 2021

Nice, that solves the issue I was having tryng to get cairo to output to stdout (probably something silly on my part).

That just leaves the discussion about what to do with screenshots spanning multiple monitors.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants