search

jcelaya / hdrmerge

HDR exposure merging
http://jcelaya.github.io/hdrmerge/
Other
367 stars 78 forks source link

Output file has wrong STRIPOFFSETS value, image garbled; also: interpolation code is partly inoperative #58

Closed phries closed 9 years ago

phries commented 9 years ago

Corrupted image due to this bug

The top image is the output of hdrmerge before fixing this bug.

Bug

The image is garbled. Or rather, some part of it must be garbled, because RawTherapee can still produce a correct JPG from the file.

To reproduce the bug, use hdrmerge on Mac OS X to create a single HDR DMG image. Note: the bug may not be limited to Mac OS X. In the options, disable the writing of previews in order to eliminate that variable.

Code sequence causing this issue

1) mainIFD.addEntry(STRIPOFFSETS, IFD::LONG, 0); - create STRIPOFFSETS tag to hold a 32-bit number 2) Compute offset into output file where image data will start: dataOffset 3) file.seekp(dataOffset) 4) mainIFD.setValue(STRIPOFFSETS, file.tellp()); - set STRIPOFFSETS to hold a 64-bit number - BUG IS HERE 5) Other data is computed and written to file 6) TIFF header is written to the file and IFDs are serialized to it 7) file.close() 8) Exif::transfer(...) (calling exiv2) modifies the output file, obscuring the bug in step 4

The problem is that IFD::LONG and file.tellp() are 32 and 64 bits, respectively. setValue(..., file.tellp()) interprets the 64-bit value as a pointer to entry data greater than 32 bits, but we really just want to save a 32-bit numerical value to the entry itself, because 32 bits fit there.

The resulting behavior is non-deterministic because a random value is read from memory into STRIPOFFSETS. On my system, out of dozens of attempts, the bug occurred all except one time. By contrast, on Linux, I haven't seen the issue.

Diagnosis

I used a binary diff tool. At first, the difference between the correct file produced in Linux versus mine appeared to be the location of 1.4kb of data written by exiv2, past all of the headers. However, I realized I should remove the exiv2 step from hdrmerge because hdrmerge could have confused exiv2. I found the data does differ in the main IFDs of the files. Then I dumped the IFDs from the Linux and Mac OS X debuggers and diffed them to find that TIFF tag id 273(STRIPOFFSETS) was the point of difference. It was 1388 from Linux and 0 on my system.

STRIPOFFSETS gets set to a non-zero value at only one place in the code, and that is where the bug turned out to be. I analyzed some of the template code and it branches significantly based on sizeof(T) > 4 (where T is the value being set of arbitrary type).

Warnings

My pull request also contains several warnings fixed. These warnings came out of the Mac OS X compilation process only, and not Linux, but they're all legitimate, so I fixed them before making the above diagnosis.

The key change due to warnings is in the HDR interpolation code:

if (k > 1.0) k == 1.0;

should be

if (k > 1.0) k = 1.0;

Since this change affects the resulting image, I'm calling that out here.

-Philip

jcelaya commented 9 years ago

It should be fixed now, thank you