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

Lets copy XMP metadata #3050

Merged
merged 2 commits into from
Oct 12, 2020
Merged

Conversation

jjimenezshaw
Copy link
Contributor

Add feature to copy XMP metadata in CreateCopy, in particular for GTiff driver (COG indirectly), and all drivers that use DefaultCreateCopy method.

gdal_translate adds the option -noxmp to not copy this data when desired

Tasklist

  • Add test case(s)
  • Add documentation
  • Review
  • Adjust for comments
  • All CI builds and checks have passed

 Implements in GDALDrive::DefaultCreateCopy and in GTiff driver CreateCopy (indirectly also available in COG driver)
 gdal_translate has a new option '-noxmp' to not copy the XMP
@@ -18010,6 +18010,15 @@ GTiffDataset::CreateCopy( const char * pszFilename, GDALDataset *poSrcDS,
}
}

/* -------------------------------------------------------------------- */
Copy link
Member

Choose a reason for hiding this comment

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

This will likely result in a suboptimal file. This will cause likely the TIFF directory to be rewritten not at the beginning of the file. You could check that with tiffdump. If things are written optimally you should see "Directory 0: offset 8" towards the beginning of the output.
I believe you'd need to manually write the TIFF tag before the /* Cleanup */ of line 17796 with
something like

    char **papszXMP = poSrcDS->GetMetadata("xml:XMP");
    if( papszXMP != nullptr && *papszXMP != nullptr )
    {
        int nTagSize = static_cast<int>(strlen(*papszXMP));
        TIFFSetField( m_hTIFF, TIFFTAG_XMLPACKET, nTagSize, *papszXMP );
    }

Copy link
Member

Choose a reason for hiding this comment

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

You could add a check for that in autotest with:

assert int(ds.GetRasterBand(1).GetMetadataItem('IFD_OFFSET', 'TIFF')) == 8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @rouault

Before fixing the code I added the assert you mentioned... and it does not fail.

This is the output of tiffdump

> tiffdump ./gcore/tmp/test_181_copy.tif
./gcore/tmp/test_181_copy.tif:
Magic: 0x4949 <little-endian> Version: 0x2a <ClassicTIFF>
Directory 0: offset 758 (0x2f6) next 0 (0)
ImageWidth (256) SHORT (3) 1<20>
ImageLength (257) SHORT (3) 1<20>
BitsPerSample (258) SHORT (3) 1<8>
Compression (259) SHORT (3) 1<1>
Photometric (262) SHORT (3) 1<1>
StripOffsets (273) LONG (4) 1<358>
SamplesPerPixel (277) SHORT (3) 1<1>
RowsPerStrip (278) SHORT (3) 1<20>
StripByteCounts (279) LONG (4) 1<400>
PlanarConfig (284) SHORT (3) 1<1>
SampleFormat (339) SHORT (3) 1<1>
700 (0x2bc) BYTE (1) 671<0x3c 0x3f 0x78 0x70 0x61 0x63 0x6b 0x65 0x74 0x20 0x62 0x65 0x67 0x69 0x6e 0x3d 0x22 0xef 0xbb 0xbf 0x22 0x20 0x69 0x64 ...>
33550 (0x830e) DOUBLE (12) 3<60 60 0>
33922 (0x8482) DOUBLE (12) 6<0 0 0 440720 3.75132e+06 0>
34735 (0x87af) SHORT (3) 32<1 1 0 7 1024 0 1 1 1025 0 1 1 1026 34737 21 0 2049 34737 6 21 2054 0 1 9102 ...>
34737 (0x87b1) ASCII (2) 28<NAD27 / UTM zone 11N|NAD ...>

What is wrong with the assert?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I found the reason. I have to close and open the file to let the code to dump all the data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or calling FlushCache(). Any preference?

Copy link
Member

Choose a reason for hiding this comment

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

Any preference?

after close & reopen. The semantic of FlushCache() is that it should push 'dirty' blocks from the block cache to disk, but there's no guarantee that other 'structural' elements of the file format will be flushed as well. Only close() guarantees that the dataset is properly finalized

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added the assert, and _check_cog in cog.py where the ghost area makes the size different. (and some other minor improvements)

Check that directory offset of first band is 8 bytes in normal GTiff
Improve tests, including COG test
@jjimenezshaw jjimenezshaw marked this pull request as ready for review October 11, 2020 21:59
@jjimenezshaw
Copy link
Contributor Author

There are two failing tests in Travis CI. They seem to be not related with my changes. What should I do?

@rouault
Copy link
Member

rouault commented Oct 12, 2020

There are two failing tests in Travis CI

I've restarted them

@rouault rouault merged commit d0a8c50 into OSGeo:master Oct 12, 2020
@rouault rouault added this to the 3.2.0 milestone Oct 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants