DecodePixelData incorrectly y flips the image when loading "Decreasing Y" files

[MarkCallow]

Describe the issue
I think the title is a clear and concise description.

To Reproduce
Steps to reproduce the behavior:

1. Use the simple test file, decreasing.exr, in testexrorder.zip attached below.
2. Call LoadEXRImageFromMemory to load the file.
3. Observe that the pixel on the first line is 1 (white/red). It should be black.

testexrorder.zip contains increasing and decreasing versions of the same simple 3 line image. See AcademySoftwareFoundation/openexr#1946 (comment) for a detailed description of the test case.

The same issue arises using the only decreasing Y file I found in the Academy's EXR test files: https://raw.githubusercontent.com/AcademySoftwareFoundation/openexr-images/main/ScanLines/Blobbies.jpg.

Expected behavior
I got this result:

I expected this

Ignore that it is red (the sole channel in the file is labelled "R" which my software follows) and the lighter area (this is from a 3D scene with illumination).

The images in "decreasing.exr" and "increasing.exr" are identical so the result should be identical.

** My Analysis **

The source data_ptr passed to DecodePixelData is acquired from offsets[y_idx] where y_idx essentially becomes the line_no parameter also passed to DecodePixelData. So data_ptr is pointing at the correct source for, e.g. line 0 so it just needs to be copied to the first scanline in the output image.

But in DecodePixelData we see lines like

tinyexr/tinyexr.h

Lines 4100 to 4105 in 5fcb4dc

	if (line_order == 0) {
	outLine += (size_t(y) + v) * size_t(x_stride);
	} else {
	outLine +=
	(size_t(height) - 1 - (size_t(y) + v)) * size_t(x_stride);
	}

The above is taken from the uncompressed case as being the simplest example.

We can see that for decreasing line order it sets the output pointer to the last scanline of the output and proceeds to write scanline 0 there.

I can workaround this by setting line_order = 0 in the header I pass to LoadEXRImageFromMemory. But I am worried this might not work for, lets say, more complex cases. In both the examples I've referenced DecodePixelData is called once for each scanline with num_lines == 1. I do not know enough about EXR or TinyEXR to judge what cases, if any, would be broken by simply removing the if (line_order == 0) clauses and always performing the line_order == 0 case. num_lines > 1 looks iffy to me.

Environment

• OS: macOS Sequoia
• Compiler Apple clang version 16.0.0 (clang-1600.0.26.6)

[syoyo]

Thank you for the detailed report.

Let me give some time to investigate the issue(and the spec of EXR lineOrder)

[MarkCallow]

I think that outline should always be set with

outLine += (size_t(y) + v) * size_t(x_stride);

because regardless of the order of the scanlines in the file, the scanline for y is the scanline for y. That is, the scanline for coord y is found in the file from entry y in the scanline offsets table. And the scanline for y will always be labelled as such.

For the case where num_lines > 1 I think the source pointer, data_ptr needs to be decremented for files with Decreasing Y scanline order (line_order == 1) and incremented for Increasing (line_order == 0).

If DecodePixelData is ever called with line_order == 2 (random) and num_lines > 1 the function will not work. Whether such a case is possible, I have no idea.

[syoyo]

Firstly, lineOrder=RANDOM_Y is only applicable to Tiled image, and TinyEXR does not support RANDOM_Y yet, so we'll discuss the issue for a scanline image with increasing_y and decreasing_y.

Dumping line_no info at here:

tinyexr/tinyexr.h

Line 5318 in 756f7d3

for Blobbies.exr gives the following result.

y 0, offsets[y] 6091998, line_no -20
y 1, offsets[y] 6066206, line_no -4
y 2, offsets[y] 6024978, line_no 12
y 3, offsets[y] 5969465, line_no 28
y 4, offsets[y] 5915415, line_no 44
y 5, offsets[y] 5855750, line_no 60
y 6, offsets[y] 5789599, line_no 76
y 7, offsets[y] 5716930, line_no 92
y 8, offsets[y] 5640285, line_no 108
y 9, offsets[y] 5554768, line_no 124
y 10, offsets[y] 5463955, line_no 140
y 11, offsets[y] 5369044, line_no 156
y 12, offsets[y] 5268097, line_no 172
y 13, offsets[y] 5162941, line_no 188
y 14, offsets[y] 5053789, line_no 204
y 15, offsets[y] 4940378, line_no 220
y 16, offsets[y] 4820515, line_no 236
y 17, offsets[y] 4700124, line_no 252
y 18, offsets[y] 4580597, line_no 268
y 19, offsets[y] 4465203, line_no 284
y 20, offsets[y] 4354738, line_no 300
y 21, offsets[y] 4242952, line_no 316
y 22, offsets[y] 4131968, line_no 332
y 23, offsets[y] 4027004, line_no 348
y 24, offsets[y] 3927168, line_no 364
y 25, offsets[y] 3828424, line_no 380
y 26, offsets[y] 3726571, line_no 396
y 27, offsets[y] 3620766, line_no 412
y 28, offsets[y] 3517113, line_no 428
y 29, offsets[y] 3413746, line_no 444
y 30, offsets[y] 3309347, line_no 460
y 31, offsets[y] 3201101, line_no 476
y 32, offsets[y] 3089596, line_no 492
y 33, offsets[y] 2982063, line_no 508
y 34, offsets[y] 2871086, line_no 524
y 35, offsets[y] 2750491, line_no 540
y 36, offsets[y] 2622890, line_no 556
y 37, offsets[y] 2494044, line_no 572
y 38, offsets[y] 2360190, line_no 588
y 39, offsets[y] 2226897, line_no 604
y 40, offsets[y] 2095490, line_no 620
y 41, offsets[y] 1972979, line_no 636
y 42, offsets[y] 1856220, line_no 652
y 43, offsets[y] 1745940, line_no 668
y 44, offsets[y] 1643646, line_no 684
y 45, offsets[y] 1543684, line_no 700
y 46, offsets[y] 1449368, line_no 716
y 47, offsets[y] 1358068, line_no 732
y 48, offsets[y] 1273503, line_no 748
y 49, offsets[y] 1199181, line_no 764
y 50, offsets[y] 1129002, line_no 780
y 51, offsets[y] 1062595, line_no 796
y 52, offsets[y] 994564, line_no 812
y 53, offsets[y] 925704, line_no 828
y 54, offsets[y] 850506, line_no 844
y 55, offsets[y] 768778, line_no 860
y 56, offsets[y] 682739, line_no 876
y 57, offsets[y] 593117, line_no 892
y 58, offsets[y] 503680, line_no 908
y 59, offsets[y] 415161, line_no 924
y 60, offsets[y] 332265, line_no 940
y 61, offsets[y] 250517, line_no 956
y 62, offsets[y] 172401, line_no 972
y 63, offsets[y] 103039, line_no 988
y 64, offsets[y] 41110, line_no 1004

NOTE: ZIP compression(used on Blobbies.exr) uses 16 scanlines(num_lines) per block.

So, i'th block(scanline) corresponds to i'th block(scanline) position(no change to the ordering of scanline) but offset(memory address) is descending order for decreasing Y.

Also, scanline data itself in the memory seems not flipped when num_lines > 1, so we probably don't need to consider line_order in DecodePixels as you pointed out!

[syoyo]

@MarkCallow Could you please prepare synthetic EXR image with decreasing Y, with compression none(num_lines = 1) and ZIP(num_lines=16) using OpenEXR lib?

e.g. height 256 image, whose y'th scanline value has 'y'.

[MarkCallow]

What is line_no in the table you list @syoyo? A negative line_nois making my head spin.

testexrorder.zip, which I uploaded in the initial description, has a file with compression none, exactly as you request except it has only 3 lines. It also includes the program used to create the file. It should be a simple matter to modify the program to write 256 lines and probably not much more difficult to make it zip compressed. I have never used the OpenEXR lib so I would have a severe learning curve. Please make the files yourself using the content of testexrorder.zip.

[syoyo]

You should read the comment here:

tinyexr/tinyexr.h

Line 5338 in 756f7d3

// line_no may be negative.

Blobbies.exr has negative value in dataWindow

dataWindow (type box2i): (-20 -20) - (1019 1019)

I have never used the OpenEXR lib

? testexrorder.zip has C++ code with OpenEXR lib.

[MarkCallow]

Blobbies.exr has negative value in dataWindow

Thanks. I'll have to study the data window. I'm new to EXR.

I have never used the OpenEXR lib

? testexrorder.zip has C++ code with OpenEXR lib.

I did not write that. It was written by @peterhillman and provided by him in AcademySoftwareFoundation/openexr#1946 (comment) to help explain decreasing scanline order to me.

[syoyo]

You should not post someone's code.

There is no meaningful info about lineOrder attribute in the OpenEXR spec, so you need to check the source code of OpenEXR.

After some code investigation, lineOrder handling when reading the image changed from openexr 2.0(TinyEXR is based on) and openexr 3.0.

For example, there is no lineOrder consideration anymore in recent ImfScanLineInputFile.cpp

https://github.com/AcademySoftwareFoundation/openexr/blob/main/src/lib/OpenEXR/ImfScanLineInputFile.cpp

Whereas lineOrder is considered in openexr 2.0

https://github.com/mitsuba-renderer/openexr/blob/dbabb6f9500ee628c1faba21bb8add2649cc32a6/OpenEXR/IlmImf/ImfScanLineInputFile.cpp#L571

So flipping may be an expected behavior in openexr 2.0.

You need to prepare reproducible code with openexr 2.0 and openexr 3.0, and check if how it behaves.

[peterhillman]

As far as I can tell, all versions of the OpenEXR C++ library load the image the same way up. I've tested back to OpenEXR-1.6.1, and all load the Blobbies.exr image correctly, consistent with Blobbies.jpg

I agree that the OpenEXR documentation isn't very clear what lineOrder does. It does say (here) that the scanline block starts with the y coordinate of the top scanline within a block, which suggests the topmost scanline is always the one with the smallest y coordinate value in its block header. It also says the scanlines are stored from top to bottom within each chunk - it doesn't say that's dependent on line order either. This is consistent with how the library behaves.

The logic you see in OpenEXR-2.0 and earlier was an optimization, and doesn't change the layout of the data as read. Relevant comments are in the code here and here. When calling readPixels(startLine,endLine) to read multiple scanlines in a single call, it was more efficient to decode them in the same order they appear in the file, startLine to endLine for INCREASING_Y order, and endLine to startLine for DECREASING_Y order, because that would avoid having to seek to a new place in the file before reading each scanline block.

The latest OpenEXR library (3.3 onwards) uses the C OpenEXRCore library to do much of the file reader, which explains why much of the code has disappeared from the C++ library classes which read files. That hasn't changed behavior of the lineOrder, though. The Core library uses pread() to access data from the file offset specified in the chunk table, so that optimization doesn't apply.

To be clear, with all versions of the OpenEXR C++ library, calling readPixels(dataWindow.min.y) will read the topmost scanline in the file into the beginning of the provided data buffer, regardless of lineOrder. It seems tinyEXR puts that scanline at the end of the provided buffer if the lineOrder is set to DECREASING_Y.

[MarkCallow]

Thank you for your comment @peterhillman. I apologize if you were bothered by my including your testexrorder code in this issue. I had no intention to claim it as my own.

It seems to me that the following are true of the EXR format:

• The logically top scanline of an image is given a y coordinate of 0.
• A file contains a scanline offsets table which is indexed by the y coordinate which provides the offset of scanline[y] in the file regardless of the scanline order given in the file header.
• Scanlines are labelled with their y coordinate.

I doubt these fundamental things have changed with different versions of the format.

Therefore I think that whether a library

1. Always returns the decoded image in memory with the first bytes being those of the top (y = 0) scanline.
2. Returns increasing Y images with the first memory bytes being those of top scanline and decreasing Y images with the first memory bytes being the bottom scanline, thus maintaining the distinction (this is what TinyEXR currently does, as far as I can see) or
3. Provides a parameter to the decode function to allow the application to select the behavior

is entirely up to the library and independent of the format.

I do not know the reasons for supporting decreasing Y in EXR but behavior no. 1 seems to make it pointless. If it is to be useful to some programs it must be visible to them so the library should do no. 2 or no. 3. Whatever it does MUST be documented. Perhaps documentation is all it takes to resolve this issue.

I have no particular preference.

[peterhillman]

Thanks @MarkCallow. I have no concerns about sharing the code. I should have added a copyright line and license identifier to the code, but I had intended it to be shared as a minimal example.

I would suggest the third option could be the best: have a parameter which specifies which order the image is decoded into, regardless of the arrangement of the file. DECREASING_Y lineOrder files are somewhat unusual, so It would be better if the library handled them internally, rather than relying on calling code to handle them correctly, and for authors to find suitable test cases to verify their code. In other words, code using a library should get the same data buffer regardless of the lineOrder of the source file.

For backwards compatibility tinyexr might continue to default to matching decoded data order to file order, but provide an option to fix the decoded data order, and strongly encourage setting it to the required order. That said, I suspect many people using the library assume the first bytes are always the top of the image, so changing the default behavior is likely to be a helpful change. For reading images into OpenGL textures, where y=0 is at the bottom, software could ask for the image to be flipped by tinyexr for all images, as it currently does for DECREASING_Y images.

The statement of the file structure is a good summary. For accuracy, it is worth clarifying that the topmost scanline is dataWindow.min.y which is often y=0 but is not always. The DisplayWindow examples in openexr-images have cases where this isn't true. Also scanlines are usually compressed in groups. The group size depends on the compression scheme. Each group is labelled with its smallest y coordinate, and the offset table contains one entry per group.

DECREASING_Y lineOrder is a feature provided for writing files. It is possible to create an OpenEXR image that is very much larger than the amount of memory available to a process by generating a few scanlines at once in a memory buffer, writing them to the file, then reusing the same memory to generate the next scanlines. Software doing that must write the scanlines to file in the order specified by lineOrder. So, if it needs to start at the bottom of the image (largest y coordinate), and work upwards, it must use DECREASING_Y lineOrder, so the first scanlines it generates can be written to the file first.