Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass IFDs to libtiff as TIFF_LONG8 #8529

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

radarhere
Copy link
Member

Resolves #8522

#7053 found that "EXIFIFDOffset" tag 34665 was being unexpectedly treated as a TIFF_LONG8 by libtiff. To fix this, it cast many TIFF_LONGs to 64-bits.

This reverts that change. Instead, I found https://gitlab.com/libtiff/libtiff/-/blob/master/libtiff/tif_dirinfo.c#L152-164 explaining that this is special behaviour for the IFD tags.

/*--: EXIFIFD and GPSIFD specified as TIFF_LONG by Aware-Systems and not TIFF_IFD8 as in original LibTiff. However, for IFD-like tags,
 * libtiff uses the data type TIFF_IFD8 in tiffFields[]-tag definition combined with a special handling procedure in order to write either
 * a 32-bit value and the TIFF_IFD type-id into ClassicTIFF files or a 64-bit value and the TIFF_IFD8 type-id into BigTIFF files. */
{TIFFTAG_EXIFIFD, 1, 1, TIFF_IFD8, 0, TIFF_SETGET_IFD8, TIFF_SETGET_UNDEFINED, FIELD_CUSTOM, 1, 0, "EXIFIFDOffset", (TIFFFieldArray *)&exifFieldArray},
{TIFFTAG_ICCPROFILE, -3, -3, TIFF_UNDEFINED, 0, TIFF_SETGET_C32_UINT8, TIFF_SETGET_UNDEFINED, FIELD_CUSTOM, 1, 1, "ICC Profile", NULL},
{TIFFTAG_GPSIFD, 1, 1, TIFF_IFD8, 0, TIFF_SETGET_IFD8, TIFF_SETGET_UNDEFINED, FIELD_CUSTOM, 1, 0, "GPSIFDOffset", (TIFFFieldArray *)&gpsFieldArray},
{TIFFTAG_FAXRECVPARAMS, 1, 1, TIFF_LONG, 0, TIFF_SETGET_UINT32, TIFF_SETGET_UINT32, FIELD_CUSTOM, TRUE, FALSE, "FaxRecvParams", NULL},
{TIFFTAG_FAXSUBADDRESS, -1, -1, TIFF_ASCII, 0, TIFF_SETGET_ASCII, TIFF_SETGET_ASCII, FIELD_CUSTOM, TRUE, FALSE, "FaxSubAddress", NULL},
{TIFFTAG_FAXRECVTIME, 1, 1, TIFF_LONG, 0, TIFF_SETGET_UINT32, TIFF_SETGET_UINT32, FIELD_CUSTOM, TRUE, FALSE, "FaxRecvTime", NULL},
{TIFFTAG_FAXDCS, -1, -1, TIFF_ASCII, 0, TIFF_SETGET_ASCII, TIFF_SETGET_ASCII, FIELD_CUSTOM, TRUE, FALSE, "FaxDcs", NULL},
{TIFFTAG_STONITS, 1, 1, TIFF_DOUBLE, 0, TIFF_SETGET_DOUBLE, TIFF_SETGET_UNDEFINED, FIELD_CUSTOM, 0, 0, "StoNits", NULL},
{TIFFTAG_IMAGESOURCEDATA, -3, -3, TIFF_UNDEFINED, 0, TIFF_SETGET_C32_UINT8, TIFF_SETGET_UNDEFINED, FIELD_CUSTOM, 1, 1, "Adobe Photoshop Document Data Block", NULL},
{TIFFTAG_INTEROPERABILITYIFD, 1, 1, TIFF_IFD8, 0, TIFF_SETGET_IFD8, TIFF_SETGET_UNDEFINED, FIELD_CUSTOM, 0, 0, "InteroperabilityIFDOffset", NULL},

This PR passes the EXIF, GPSInfo and Interop IFDs to libtiff as TIFF_LONG8.

@mgorny
Copy link
Contributor

mgorny commented Nov 4, 2024

Thanks. It fixes the regression, but it doesn't seem to get IFD right still, at least on PowerPC:

____________________________________________________ TestFileLibTiff.test_exif_ifd ____________________________________________________

self = <Tests.test_file_libtiff.TestFileLibTiff object at 0xf60a0c90>

    def test_exif_ifd(self) -> None:
        out = io.BytesIO()
        with Image.open("Tests/images/tiff_adobe_deflate.tif") as im:
            assert im.tag_v2[34665] == 125456
            im.save(out, "TIFF")
    
            with Image.open(out) as reloaded:
                assert 34665 not in reloaded.tag_v2
    
>           im.save(out, "TIFF", tiffinfo={34665: 125456})

Tests/test_file_libtiff.py:709: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
src/PIL/Image.py:2605: in save
    save_handler(self, fp, filename)
src/PIL/TiffImagePlugin.py:1954: in _save
    encoder = Image._getencoder(im.mode, "libtiff", a, encoderconfig)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

mode = 'RGB', encoder_name = 'libtiff'
args = ('RGB', 'tiff_adobe_deflate', 0, '', [(254, 0), (256, 278), (257, 374), (258, 8), (259, 8), (262, 2), ...], {254: 4, 273: 4, 279: 4, 305: 2, ...})
extra = ()

    def _getencoder(
        mode: str, encoder_name: str, args: Any, extra: tuple[Any, ...] = ()
    ) -> core.ImagingEncoder | ImageFile.PyEncoder:
        # tweak arguments
        if args is None:
            args = ()
        elif not isinstance(args, tuple):
            args = (args,)
    
        try:
            encoder = ENCODERS[encoder_name]
        except KeyError:
            pass
        else:
            return encoder(mode, *args + extra)
    
        try:
            # get encoder
            encoder = getattr(core, f"{encoder_name}_encoder")
        except AttributeError as e:
            msg = f"encoder {encoder_name} not available"
            raise OSError(msg) from e
>       return encoder(mode, *args + extra)
E       RuntimeError: Error setting from dictionary

src/PIL/Image.py:467: RuntimeError
-------------------------------------------------------- Captured stderr call ---------------------------------------------------------
_TIFFVSetField: : Bad LONG8 or IFD8 value 538833550496960 for "EXIFIFDOffset" tag 34665 in ClassicTIFF. Tag won't be written to file.

@mgorny
Copy link
Contributor

mgorny commented Nov 4, 2024

Hmm:

125456 = 0x1EA10
538833550496960 = 0x1EA10F65ED4C0

So it's still getting some junk?

@radarhere
Copy link
Member Author

Yes, I jumped the gun slightly.

I've updated the commit. Please try again.

@mgorny
Copy link
Contributor

mgorny commented Nov 4, 2024

Thanks! With this change, all tests pass for me now.

src/encode.c Outdated
@@ -959,6 +959,10 @@ PyImaging_LibTiffEncoderNew(PyObject *self, PyObject *args) {
status = ImagingLibTiffSetField(
&encoder->state, (ttag_t)key_int, (FLOAT64)PyFloat_AsDouble(value)
);
} else if (type == TIFF_LONG8) {
status = ImagingLibTiffSetField(
&encoder->state, (ttag_t)key_int, PyLong_AsLongLong(value)
Copy link

Choose a reason for hiding this comment

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

perhaps the return value of PyLong_AsLongLong(value) should also be force-casted like (almost) all the others, to make it obvious what type is expected (and make it easier to find wrong modifications of the code in the future).

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a commit to cast to uint64_t

Copy link

Choose a reason for hiding this comment

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

Thanks, looks good.

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

Successfully merging this pull request may close these issues.

Regressions in TIFF support on 32-bit PowerPC (from "Corrected passing TIFF_LONG to libtiff")
3 participants