exiv2 icon indicating copy to clipboard operation
exiv2 copied to clipboard

Test coverage for EPS files

Open hassec opened this issue 3 years ago • 10 comments

Describe the bug

I was trying to improve the test coverage of the code in epsimage.cpp by adding an eps file to the test suite.
The file I chose is the eps test image from exiftool but exiv2 fails when reading the file:

Warning: Nested document at invalid position: 257
Exiv2 exception in print action for file PostScript.eps:
Failed to read image data
To Reproduce

Steps to reproduce the behavior:

  1. File is linked above
  2. Simple exiv2 PostScript.eps will trigger the error
  3. Same behavior on 0.27-maintenance and main
Expected behavior

exiftool handles this file without problem, including the XMP and IPTC data.
I'm not familiar with the details of the EPS format, so am not sure how simple or complicated this test file is, but I assume it's not unreasonable to expect that exiv2 should also be able to handle this file?

Additional context

EPS was first deprecated and then re-added again to the main branch. See discussion in #1557

If we want to have EPS support on main and thus in a future v1.0, I think we should at least have some test coverage.
Let me add @vog and @rentapacs, as you are the authors and from the discussion in #1557 it seemed like you work with exiv2 + EPS.

Two questions for you :wink:

  1. It seems like exiv2 works for the EPS files you handle, any chance you can provide 1 or 2 that trigger a large chunk of the code in epsimage.cpp ?
  2. Any chance one of you might have time to look into the problem described above?

hassec avatar Dec 29 '21 08:12 hassec

There are test files for EPS. svn://svn.exiv2.org/svn/testdata/trunk

I put them (and the video test files) there to reduce the size of the code base download. In Makefile, you do $ make teste and it sniffs for the eps test files and downloads them if necessary. Volker is a very careful person and I think you'll find the coverage is quite comprehensive.

You'll have to get a fairly elderly release of Exiv2 (say 0.24) to do the archaeology. https://www.exiv2.org/archive.html

I don't recall anybody every reporting an issue with the EPS code and so I wrongly concluded that nobody uses it. Volker does use it and Luis restored the EPS code in 'main'. On the 0.27-maintenance branch, you will get deprecation warnings when you compile the EPS code.

clanmills avatar Dec 29 '21 09:12 clanmills

@clanmills thanks for the hint. But in this case, I don't think we actually execute these tests anymore.
It seems that what you describe above is only part of the old bash based test suite, which is deprecated.
Thus, this should probably be ported to new python test suite...

hassec avatar Dec 29 '21 09:12 hassec

That's correct. All that stuff is written in bash. It's probably straightforward to port it into tests/bash_tests. However before we start to port it, we should see if it's good for your purposes. If your intention is to increase coverage, this is probably a good place to start.

I'll have a look at the PostScript code and give you an update. I was the Engineering Lead for Adobe PostScript. It's been a while (20 years) since I looked at anything like that, but it's etched in my soul. My wife refused to let me have the Adobe Logo tattooed under my shorts when I retired: Unknown !

clanmills avatar Dec 29 '21 11:12 clanmills

I think the issue with the file is that it is an EPS file, with an embedded EPS file! The "recursive" EPS starts at byte 257 (0x0101)

510 rmills@rmillsm1:~/Downloads $ exiv2 ~/temp/foo.eps 
Warning: Nested document at invalid position: 257
Exiv2 exception in print action for file /Users/rmills/temp/foo.eps:
Failed to read image data

There's something at byte 257 (0x0101) that's upset him:

511 rmills@rmillsm1:~/Downloads $ dmpf ~/temp/foo.eps | head -10
       0        0: %!PS-Adobe-3.0 EPSF-3.0.%%Creato  ->  25 21 50 53 2d 41 64 6f 62 65 2d 33 2e 30 20 45 50 53 46 2d 33 2e 30 0a 25 25 43 72 65 61 74 6f
    0x20       32: r: Adobe Photoshop Version 7.0.1  ->  72 3a 20 41 64 6f 62 65 20 50 68 6f 74 6f 73 68 6f 70 20 56 65 72 73 69 6f 6e 20 37 2e 30 2e 31
    0x40       64: .%%Title: c.eps.%%CreationDate:   ->  0a 25 25 54 69 74 6c 65 3a 20 63 2e 65 70 73 0a 25 25 43 72 65 61 74 69 6f 6e 44 61 74 65 3a 20
    0x60       96: 7/18/05 2:33 PM.%%BoundingBox: 0  ->  37 2f 31 38 2f 30 35 20 32 3a 33 33 20 50 4d 0a 25 25 42 6f 75 6e 64 69 6e 67 42 6f 78 3a 20 30
    0x80      128:  0 8 8.%%HiResBoundingBox: 0 0 8  ->  20 30 20 38 20 38 0a 25 25 48 69 52 65 73 42 6f 75 6e 64 69 6e 67 42 6f 78 3a 20 30 20 30 20 38
    0xa0      160:  8.%%SuppressDotGainCompensation  ->  20 38 0a 25 25 53 75 70 70 72 65 73 73 44 6f 74 47 61 69 6e 43 6f 6d 70 65 6e 73 61 74 69 6f 6e
    0xc0      192: .%%EndComments.%%BeginProlog.%%E  ->  0a 25 25 45 6e 64 43 6f 6d 6d 65 6e 74 73 0a 25 25 42 65 67 69 6e 50 72 6f 6c 6f 67 0a 25 25 45
    0xe0      224: ndProlog.%%BeginSetup.%%EndSetup  ->  6e 64 50 72 6f 6c 6f 67 0a 25 25 42 65 67 69 6e 53 65 74 75 70 0a 25 25 45 6e 64 53 65 74 75 70
   0x100      256: .%%BeginDocument: (Test-Doc1.eps  ->  0a 25 25 42 65 67 69 6e 44 6f 63 75 6d 65 6e 74 3a 20 28 54 65 73 74 2d 44 6f 63 31 2e 65 70 73
   0x120      288: ).%!PS-Adobe-3.0 EPSF-3.0.%%Crea  ->  29 0a 25 21 50 53 2d 41 64 6f 62 65 2d 33 2e 30 20 45 50 53 46 2d 33 2e 30 0a 25 25 43 72 65 61

Exiftool is happy:

512 rmills@rmillsm1:~/Downloads $ exiftool ~/temp/foo.eps 
ExifTool Version Number         : 12.03
File Name                       : foo.eps
Directory                       : /Users/rmills/temp
File Size                       : 11 kB
File Modification Date/Time     : 2021:12:29 11:12:38+00:00
...

For sure, we can ask Volker to look at this. However before we drag him into this, can we agree on our objective here? Are we concerned about this single file (which smells OK to me), or to boost the % coverage in CodeCov, or another objective? Volker actively uses the exiv2 EPS code in an application. He's a nice guy and I'm sure he'll "Step Up" if we ask for his help.

clanmills avatar Dec 29 '21 11:12 clanmills

My main objective was only to get better test coverage to make sure any future refactoring doesn't break anything.
I'm surprised that this file isn't handled. But given that we haven't had anyone complain, I'd argue making the current code handle this file might be a secondary objective... ?

I've had a look at the test files you linked and did a bit of forensic digging, and it seems that the EPS code used to have quite extensive testing, but that has unfortunately been removed at some point.

hassec avatar Dec 29 '21 11:12 hassec

@hassec I sort-of "let this go" because I thought it was going to die. As Volker would like the EPS code to continue in Exiv2, I think we should ask him to look at this. Increasing % CodeCover sounds like a good objective and more important than this single file.

@vok do you have time to restore your EPS tests to a healthy working state? The test framework has been rewritten in python. If you have time to work on this, I'll help you with the python related code.

clanmills avatar Dec 29 '21 11:12 clanmills

I put them (and the video test files) there to reduce the size of the code base download. In Makefile, you do $ make teste and it sniffs for the eps test files and downloads them if necessary. Volker is a very careful person and I think you'll find the coverage is quite comprehensive.

I can confirm that I provided all test cases to you that I had. My goal was indeed something like 90%-100% test coverage, as the test cases were added specifically to check each new condition and twist which I had to add to the EPS code.

While I do have some more test files, those are all proprietary files that originally triggered some issues. However, I was careful to recreate each relevant issue by manually adjusting some free image file.

The file I chose is the eps test image from exiftool but exiv2 fails when reading the file

I have no explanation for this. It would indeed be great if @clanmills could have a look at this. He also assisted @rentapacs and me with tons of very helpful advice while developing the EPS code.

@vog do you have time to restore your EPS tests to a healthy working state? The test framework has been rewritten in python. If you have time to work on this, I'll help you with the python related code.

I'll try to have a look at this, but won't be able to do much before January.

vog avatar Dec 29 '21 12:12 vog

@vog. Thank You for your reply. Best Wishes for 2022. Let's hope the covid crisis fades and we get our lives back soon.

There's no reason to hurry with the EPS test coverage. At the moment, the code is not being actively tested. Let's change that in 2022.

I looked at the ExifTool test file. It's an EPS with an embedded EPS! I don't know if that is a common EPS scenario. Better to give focus to getting the test suite to use your "old" files and increase % CodeCov

clanmills avatar Dec 29 '21 12:12 clanmills

The ExifTool sample is actually an EPS file with 3 embedded EPS files, and the second one also has an EPS file embedded within it. One level of embedded EPS happens commonly when EPS-format figures are included in a document. Two levels may not be so common.

boardhead avatar Dec 29 '21 18:12 boardhead

Happy New Year, @boardhead Phil. Best Wishes for 2022. Thank you for the insight concerning Russian Doll EPS files.

clanmills avatar Dec 29 '21 20:12 clanmills