Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#5246 closed enhancement (fixed)

[PATCH] Load/Save embedded ICC Profiles (TIFF/JPEG/PNG)

Reported by: cleo Owned by: warmerdam
Priority: normal Milestone: 1.11.0
Component: default Version: unspecified
Severity: normal Keywords:
Cc:

Description

In order to be able to do colour management, the ICC Profile or other colour information needs to be retrieved from the file.

TIFF has the following tags: TIFFTAG_ICCPROFILE TIFFTAG_PRIMARYCHROMATICITIES TIFFTAG_WHITEPOINT TIFFTAG_TRANSFERFUNCTION

JPEG has an APP2 marker with ID string of "ICC_PROFILE\0".

PNG also has ICC profiles. http://www.w3.org/TR/PNG/#11iCCP

Unfortunately, the original colour space is required to use colour profiles, yet there is still a ticket (#5097) that I can't move forward where JPEG YCCK returns inverted CMYK while reporting YCCK colour space. I provided a fix in that ticket.

BTW, what is the mechanism for approving changes and where is the repository? Can I add changes myself?

Thanks

Attachments (1)

profile.zip (17.2 KB) - added by cleo 3 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 4 years ago by Even Rouault

So your idea would be to expose ICC profiles as GDAL metadata ?

The standard way of having changes applied is to prepare a patch against the subversion repository ( http://svn.osgeo.org/gdal/trunk ) and attach it to a ticket for review. Direct access to the repository is for approved committers.

comment:2 Changed 4 years ago by cleo

Yes, exactly. Expose it as metadata.

For TIFF, it could use the same prefix as the other tags (they all start with TIFFTAG_), though it'd be nice if certain metadata were more consistent across formats. For example, the resolution metadata is different on every format. Would it be possible to try and have the ICC Profile itself under the same metadata name for all file formats? It's not critical. Just makes code easier to maintain.

If someone is willing to do this, that'd be great. Otherwise, I'll try and find time at some point in the future. The TIFF one doesn't look hard. But the JPEG one needs the APP blocks to be appended if I'm not mistaken. And I know nothing of PNG's internals.

comment:3 Changed 3 years ago by cleo

I've started working on this. For TIFFs I'm also loading the colorimetric data.

I've got the following metadata:

SOURCE_ICC_PROFILE

TIFFTAG_PRIMARIES_RED

TIFFTAG_PRIMARIES_GREEN

TIFFTAG_PRIMARIES_BLUE

TIFFTAG_WHITEPOINT

TIFFTAG_TRANSFERFUNCTION_RED

TIFFTAG_TRANSFERFUNCTION_GREEN

TIFFTAG_TRANSFERFUNCTION_BLUE

TIFFTAG_TRANSFERRANGE_BLACK

TIFFTAG_TRANSFERRANGE_WHITE

For PNG and JPG, I will only add SOURCE_ICC_PROFILE.

Does anyone have an issue with the name for the TIFF tags? Normally, there is a one to one match. But here, the primaries for example is actually only one tag. And the transfer function return three arrays. So I split them up this way. I think it's fairly straightforward.

As for the ICC profile, I'd like to use the same name across drivers. So this will be one tag that doesn't have the TIFFTAG prefix.

If anyone has objection to this or comments, let me know.

comment:4 Changed 3 years ago by cleo

Is there a style guide somewhere? Why is the code written like C in a C++ environment? Am I allowed to use STL for internal temporary stuff? I do know you can't use STL for anything public, but internally, it should be fine.

comment:5 Changed 3 years ago by Even Rouault

It would be good to have a common metadata prefix, for example ICC_ . Another possibility is to expose those metadata as a separate ICC metadata domain, as it of specialized use.

http://trac.osgeo.org/gdal/wiki/rfc8_devguide gives a few programming guide line. Otherwise the best is to look a bit at the existing code to see the common practices. As far as C++ is concerned, we tend to avoid too fancy C++ features and yes it does actually look like C with classes. Common C++ containers (vector, map, ...) are sometimes used.

comment:6 Changed 3 years ago by cleo

I'm currently testing these changes. JPEG has been tested. Will test the other soon and hopefully add some unit tests. JPEG, PNG and TIFF have support for read/write of Profile and colorimetric data. Note that if a profile exists or is output, colorimetric info is ignored as it's not necessary. The PNG specs seem to indicate that other info is recommended to be added for apps that don't support profiles, but I have no way of getting that info from the profile w/o adding a colour management library to GDAL and I see no point in that. The user could add this info, but again, I'd have to add a lot more code for not much gain.

I'm using the following tags because they relate to the original colour space, are found in multiple formats and relate to the original source data. The names have changed slightly.

SOURCE_ICC_PROFILE

SOURCE_ICC_PROFILE_NAME

SOURCE_PRIMARIES_RED

SOURCE_PRIMARIES_GREEN

SOURCE_PRIMARIES_BLUE

SOURCE_WHITEPOINT

The profile name is only used with PNG right now. But they have a tag for sRGB without the profile itself. So I need a way to check for that. PNG also has a name property for the profile. Basically, check if a profile exists. If it is not available, check if the profile name is 'sRGB'. That'll handle the PNG case.

The primaries and whitepoints are in "x, y, 1.0" format in xyY colours.

TIFFTAG_TRANSFERFUNCTION_RED

TIFFTAG_TRANSFERFUNCTION_GREEN

TIFFTAG_TRANSFERFUNCTION_BLUE

TIFFTAG_TRANSFERRANGE_BLACK

TIFFTAG_TRANSFERRANGE_WHITE

These are tiff specific, so I left them as tiff tags even though it's a single tag in the file (one for transfer function and one for transfer range).

For PNG, there is:

PNG_GAMMA

I'll update all driver docs.

I'm now getting an issue where the profile is being output to the pam xml file when I open the file. Anyone know how I can disable this? I haven't really looked into it, but thought I'd ask if there's an obvious answer first.

Hope this is good for everyone. I also put all of the tags in the "IMAGE_STRUCTURE" domain since that is where SOURCE_COLOR_SPACE was and this relates directly to the interpretation of the colour data. If anyone has suggestions, changes, comments, etc., please let me know.

comment:7 Changed 3 years ago by Even Rouault

Regarding metadata going to pam xml, I believe this must be due to the fact that you call SetMetadata? or SetMetadataItem? after the "Initialize any PAM information" section (that is often found at the end of the Open() method of drivers).

As far as the gist of your work is concerned, I'm afraid I'm not familiar enough with the topic to have an opinion without seeing a patch.

comment:8 Changed 3 years ago by cleo

Ah ok, thanks! I'll look for that. I'll post the patches when I'm done the unit tests and after I've had it code reviewed here.

comment:9 Changed 3 years ago by cleo

I'm trying to escape binary strings from python in one part of the unit test. I load the binary data from the file and it is the correct length. But when I call this, it's a little over half the original size. It appears to combine every two bytes as a single codepoint.

icc = gdal.EscapeString?(data, scheme=gdal.CPLES_BackslashQuotable)

Does anyone have an example how to get this working?

If I get this working, I think I'm done.

comment:10 Changed 3 years ago by cleo

I'll use base64 in Python and see if it gives identical results to CPLBase64Encode(). If so, I'll use that. It's more standard and base64 encoding is available on every development environment.

comment:11 Changed 3 years ago by cleo

I decided to use COLOR_PROFILE namespace for the new metadata. I didn't want to risk interfering with existing tags in the IMAGE_STRUCTURE namespace.

I attached a zip file containing the patch to be applied from within the gdal folder. There is also a autotest folder that should be decompressed into the folder of the same name. They are all new files and should be added via SVN (3 unit test files and 1 data file).

comment:12 Changed 3 years ago by Even Rouault

Summary: Feature request: Load/Save embedded ICC Profiles (TIFF/JPEG/PNG)[PATCH] Load/Save embedded ICC Profiles (TIFF/JPEG/PNG)

Quite substantial patch... It will take some time to review. Not sure when I'll be able to do so. Ping me or other committers if this stalls for too long. Just one note: this will definitely not make it into 1.10.2 as it is a new feature, but it will rather go into trunk ( 2.0 or 1.11 )

comment:13 Changed 3 years ago by cleo

OK, thanks. The docs in the zip will have to be updated with the correct version number of when these tags are being introduced.

As to the code itself, you'll find that the new code required very few changes from existing code and doesn't change any pre-existing variables (aside from getting/setting metadata). I tried to make it all fairly independent and self-contained from the rest of the code (low coupling). The length of it is mostly due to the various colorimetric tags that need to be parsed or converted, but should be very simple to follow.

Changed 3 years ago by cleo

Attachment: profile.zip added

comment:14 Changed 3 years ago by Even Rouault

Patch commited in trunk in r26537 with a few corrections: compilation fix for libpng < 1.5 (a few arguments have changed signature), and I've also fixed in geotiff.cpp the loop at line 6494 to be "for(int i = 0; ((i < 2) && bOutputTransferRange); i++)" (the maximum index was 3 which is wrong since pszTXRNames contains only 2 values). By the way, this part of reading/writing TIFFTAG_TRANSFERRANGE does not work (at least in my testing with internal libtiff) since the tag isn't declared in libtiff. You would likely need to declare a custom tiff field info like in GTiffTagExtender().

If you need revisions of your code, please make them on trunk now.

comment:15 Changed 3 years ago by Even Rouault

Ah, and I've fixed the .py scripts to be compatible with Python 2.X too

comment:16 Changed 3 years ago by Even Rouault

r26538 "JPEG: implement differed loading of ICC profiles (#5246)"

comment:17 Changed 3 years ago by Even Rouault

r26541 "PNG: implement differed loading of ICC profiles (#5246)"

comment:18 Changed 3 years ago by Even Rouault

r26542 "GTiff: implement differed loading of ICC profiles (#5246)"

comment:19 Changed 3 years ago by cleo

Thanks!

About transfer range, the tag is in the specs (and thanks for fixing the loop). However, I've only read of one kind of colorimetric info (ie. company) that would use it. I added it to be complete.

If the tests pass, then everything looks good to me.

BTW, I can't remember for certain, but if you are releasing an update, you may want to verify that szCreateOptions is large enough in GDALRegister_GTiff(). I seem to recall that someone had updated the string, but not the size of the string. I'd take a look at revision 24653 (I believe it was the alpha type changes). I took this into account when I updated the size in my changes. But for an update before 2.0, I'd verify the size. I was getting crashes when I was first testing the GTiff changes.

Thanks again for applying my patch and making the relevant changes!

I'll be double checking again in our apps that ICC profiles are working correctly. JPEG is done already. I'll post a quick comment when I'm done.

comment:20 Changed 3 years ago by cleo

I've applied your changes locally and tested reading and writing ICC profiles for JPEG, TIFF and PNG. Everything looks good.

comment:21 Changed 3 years ago by Even Rouault

Milestone: 2.0
Resolution: fixed
Status: newclosed

comment:22 Changed 3 years ago by Even Rouault

Milestone: 2.01.11.0
Note: See TracTickets for help on using tickets.