GNOME Bugzilla – Bug 498721
Add TIFF image density metadata support
Last modified: 2014-10-30 20:33:51 UTC
Some pictures, like those produced by hylafax, have different resolutions for the x and y directions. If you want to see them with a correct aspect ratio, you need to scale them in one of the directions. However, gdk-pixbuf only takes into account the number of pixels, which means the picture is displayed completely distorted in all applications using gdk-pixbuf directly, while e.g. the gimp displays them fine.
Created attachment 99429 [details] Sample file produced by hylafax
Created attachment 101967 [details] EOG, 2.20.3
Created attachment 101968 [details] Evince, 2.20.2
I fail to see how this is a gdk-pixbuf bug. What change do you propose? Would it be sufficient, if gdk-pixbuf would provide access to the physical resolution of each picture it loaded? If you really need scaling: When should that scaling be applied? Directly after loading? Just when drawing? Wouldn't that be a incompatible change in gdk-pixbuf's behaviour? Shouldn't just EOG be changed to correctly handle this case?
I think the first thing to do is indeed to provide access to the physical resolution. But it would also be nice to provide functions to generate a scaled pixbuf that would render correctly on screen given the physical resolution difference, avoiding to put this logic in each and every application.
I agree. I reported this bug as an EOG bug only. It works well on Evince. But Evince has other bug. Try to load the `received.tiff' sample on EOG and rotate it. Wrong AR but It's OK. When the same is done on Evince the sample is so buggy. I'll attach a sample.
Created attachment 102023 [details] Evince, 2.20.2; received.tif rotated 90°.
Bug 701622 has patches about adding resolution information to images, so I'll make this older bug a duplicate of that newer one. (In reply to comment #5) > I think the first thing to do is indeed to provide access to the physical > resolution. That would be done in the above bug. > But it would also be nice to provide functions to generate a scaled > pixbuf that would render correctly on screen given the physical resolution > difference, avoiding to put this logic in each and every application. It's purely a rendering problem. I don't think that GdkPixbuf would be doing someone any favours by scaling the image to have square pixels, especially when we can handle non-square ratios better in the front-end (totem has been handling non-square pixels for longer than this very old bug has existed ;) *** This bug has been marked as a duplicate of bug 701622 ***
bug 701622 is about JPEGs, not TIFFs...
Created attachment 289153 [details] [review] Add TIFF image density metadata support
Review of attachment 289153 [details] [review]: The gdk_pixbuf__tiff_image_save_to_callback() changes will likely clash with that from bug 578876 which contains support for many more metadata. ::: gdk-pixbuf/io-png.c @@ -27,3 @@ #include <string.h> #include <png.h> -#include <math.h> That change probably needs to be made in a separate commit.
Created attachment 289162 [details] [review] Add TIFF image density metadata support Remove removal of duplicate math.h include
Regarding bug 578876 - do you want me to do anything about that?
(In reply to comment #13) > Regarding bug 578876 - do you want me to do anything about that? It would be great if you had time to update the patch from bug 578876, so it can be landed before this one.
(In reply to comment #12) > Created an attachment (id=289162) [details] [review] > Add TIFF image density metadata support > > Remove removal of duplicate math.h include I meant the adding it somewhere else as well, seems to be a bug that it's not in the place where the math functions are used (where round() is).
> > Remove removal of duplicate math.h include > > I meant the adding it somewhere else as well, seems to be a bug that it's not > in the place where the math functions are used (where round() is). I added into gdk-pixbuf-private.h so it was no longer needed in io-png.h anymore (it was only used for the conversion macros which are now common).
Created attachment 289187 [details] [review] Add TIFF image density metadata support Rebased on master. For some reason it the TIFF tests are now failing for me with "SKIP format not supported" but I can't work out what might be causing that.
First, you'll need to add "tiff" to test-common.c. I would also add (in a separate commit) a: g_assert_nonnull (pixbuf); after the gdk_pixbuf_new_from_file() call, to make sure that the image got actually loaded, and did not fail without an error. Once that's done, I get: $ GDK_PIXBUF_MODULE_FILE=../gdk-pixbuf/loaders.cache ./pixbuf-dpi /pixbuf/dpi/png: OK /pixbuf/dpi/jpeg: OK /pixbuf/dpi/tiff: ** ERROR:pixbuf-dpi.c:92:test_nonincremental: assertion failed: (strcmp (x_dpi, "300") == 0) You should be using g_assert_cmpstr() which shows the value expected, and the value we got. Which shows: ERROR:pixbuf-dpi.c:92:test_nonincremental: assertion failed (x_dpi == "300"): ("-2147483648" == "300") Which shows where the problem might be.
Apparently it's a float, not a uint32. That fixes the test suite. Yay tests :)
Created attachment 289622 [details] [review] Add TIFF image density metadata support Fixed up types used with TIFFGetField / TIFFSetField. Fixed tests.
Created attachment 289623 [details] [review] Add TIFF image density metadata support Removed some temp debugging code
Review of attachment 289623 [details] [review]: Looks good otherwise. ::: gdk-pixbuf/io-tiff.c @@ +897,3 @@ + if (x_dpi != NULL && y_dpi != NULL) { + char *endptr = NULL; + uint16 resolution_unit = RESUNIT_INCH; Can't you directly use it in the SetField function below?
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.
> + uint16 resolution_unit = RESUNIT_INCH; > > Can't you directly use it in the SetField function below? I think you can but I decided to be explicit in that is was a uint16, not a general integer.