* Switch runner type for bleeding edge, use gcc14 and python 3.12
* Various minor gcc-14 + C++20 + Python 3.12 fixes
Notable: gcc14 warns about std::atomic_load for share_ptr as deprecated,
since C++20 provides a true atomic shared pointer.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
This should address the following 2 issues when using MSVC:
- MSVC does not set defines like `__SSE2__` or `__SSE4_2__` etc. we must
do that ourselves
- MSVC uses the `/arch` flag differently than other compilers and it
should be set only once
The ramifications of not having those defines set basically means that
OIIO won't notice any SIMD is possible.
For MSVC the /arch flag only controls which instruction set the compiler
may use when generating code "on its own". For example, the x64 compiler
by default will use SSE2 whenever possible when its generating code but
when using `/arch:AVX` it will produce AVX code as it sees fit. NOTE:
case matters here too! "avx" does not work but "AVX" does.
However, if the source code uses intrinsics, MSVC will happily use those
instead. In fact it's possible to use `avx512` intrinsics without
specifying any /arch flag at all and it will happily generate
corresponding avx512 code. This is much different than say GCC where, in
order to use intrinsics at all, you must specify the corresponding flags
allowing you to do so for the arch in question.
Fixes#4265
Existing simd_test.cpp should be sufficient I think. In addition to
those, I've verified the below locally:
Currently: when using `-DUSE_SIMD="sse4.2,avx2"` to build, we get the
wrong `build:simd` flags:
`sse2`
After: using the same flags as above:
`sse2,sse3,ssse3,sse41,sse42,avx,avx2`
---------
Signed-off-by: Jesse Yurkovich <jesse.y@gmail.com>
We've been plagued on and off for a long time with spurious wrong
results in our SIMD versions of exp and log. It was really squirrelly,
came and went with changes to seemingly unrelated code, and had long
periods of time with no symptoms or ability to reproduce it. It just
started popping up again in CI in the dev-2.5 branch, consistently.
Today I think I finally tracked it down for real. I believe the problem
was actually in the SIMD bitcast_to_int and bitcast_to_float functions,
for the variety that was 8-wide when running on 4-wide hardware, and the
16-wide one when running on 8-wide (or less) hardware.
Background: When real 16-wide HW is not available, we define the 16-wide
vectors as an array of two 8-wide vectors. (Same for 8 vs 4, I'll spare
you explaining it all twice.) So most of the 16 wide functions boil down
to:
vfloat16 op (vfloat16 arg, ...)
{
#if 16 wide HW is available
return real_16_wide_intrinsic(arg);
#else /* recursively define in terms of 8-wide halves */
return vfloat16(op(arg.lo()), op(arg.hi()));
#endif
}
But for the bitcast operators, we did something different, a shade too
clever: we just did a pointer cast, dereference and return:
vint16 bitcast_to_int (const vfloat16& x)
{
#if OIIO_SIMD_AVX >= 512
return _mm512_castps_si512 (x.simd());
#else
return *(vint16 *)&x;
#endif
}
I believe that the problem is that this cast is undefined behavior for
the case where the vector x was broken into two halves, and by being in
the middle of a long code sequence of SIMD ops, the two halves happened
to be in registers at that moment. Basically, we are taking the address,
and it just doesn't seem to always understand that it needs to
materialize the whole thing in memory for the pointer cast to work
properly.
I removed the pointer cast and replaced with
return vfloat16(bitcast_to_float(x.lo()), bitcast_to_float(x.hi()));
basically falling back on the usual recursive definition of 16-wide
operations to being in terms of 8-wide operations when 16-wide HW is not
available. This totally clears up the problem since we're no longer
potentially relying on needing the address of something that has no
address at that moment.
A couple other spots (andnot) also had dubious casts that I fixed in the
same way.
N.B.: The 16-wide cast from bool to int had to do something slightly
different, because it's represented differently and isn't a true
bitcast.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Add proxy support for EXR multipart output by taking the setup code from
the other open() overload and calling the constructor accepting a stream
instead of a filename.
---------
Signed-off-by: Joachim Reichel <43646584+jreichel-nvidia@users.noreply.github.com>
Some weird combination of old libheif library gives a different
oiio:OriginalOrientation result. Need this ref because that's how it
behaves in that version (which I have to build against at work), but
it's not worth tracking down a "fix" because it's so old and works fine
on newer versions.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
This is just a code move. There no changes to the code logic, though in
the process, I do change old "local" uses of eval_as_bool to the new
Strutil one.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Background: Some formats that support multi-image files know as soon as
they are opened and read the header what the total subimage count is.
Others (I'm looking at you, TIFF) can't know without reading each
subimage in succession until you hit the end of the file. Format readers
who can easily figure it out open first opening are supposed to set
attribute "oiio:subimages" to the number of subimages, or 0 / unset if
it can't be cheaply or immediately determined.
ImageBuf has a `nsubimages()` method that you'd think would give you the
right count, and it did when all IBs were read through the ImageCache
(which fully inventories the subimages of any file it reads). But when
OIIO 2.5 changed IB behavior to only be backed by IC when explicitly
requested, we effectively lost this functionality most of the time, and
instead returned 0 ("don't know yet"). This was confusing. And also
silly, because it reported 0 even for files whose formats don't permit
multiple images (so you know the count is 1).
This patch does the following:
* Changes the behavior and documentation of `ImageBuf::nsubimages()` to
the following return values:
- 1, for formats that are always a single subimage. (Or for ImageBufs
that are not direct reads of a file.)
- n>=1 for files that permit multiple subimages and the count is known.
- 0 for files that permit multiple subimages, but the count is not known
because it's one of the formats where it can't easily be determined upon
opening the file.
* Add a new ImageInput::supports("multiimage") that says if the input
file format permits multiple images, and ensure that it returns true for
all formats where this is the case.
* Ensure that formats permitting multiimage and can easily know the
count up front set the "oiio:subimages" attribute (not all did).
* Document in the built-in format section of the docs the formats that
are expected to set this attribute.
Fixes#4225
Signed-off-by: Larry Gritz <lg@larrygritz.com>
This is a continuation of PR 4246, had one more spot where we needed to
disable warnings for unity builds involving clang18 and deprecated
unicode conversion functions.
There's some changes included to ci.yml and install_homebrew_deps.bash
that are part of work I did to make it easier for me to force it to use
clang-18 for certain tests while working on this.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
The nullptr at the end of default_font_name was left over from a time
that we iterated by going through array elements until we hit the
nullptr that signalled we had passed the last valid element. At some
point we switched to a range-for, which iterates over every element of
the array without needing an "end sentinel", and we should have
eliminated the nullptr but failed to do so.
Fixes#4248
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Only use the OIIO_LIBRARY_PATH environment variable by default.
Searching through such system paths has some risk loading plugins
installed for another purpose or that may be incompatible.
If the previous behavior is desired, applications can add these
environment variables to the plugin search path manually.
Signed-off-by: Brecht Van Lommel <brecht@blender.org>
clang/llvm 18 (and probably other compilers soon) has made noisy
warnings about the C++17-deprecated unicode conversions. Actually
replacing them enirely with something else is rather hairy, we will
tackle that later.
In the mean time, this patch seems to be the minimal change to disable
the warnings that should be totally easy to backport.
These seem to be the only changes necessary to make a clean build with
clang 18.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
We disable fmt execptions and force it to do one of our ASSERT's
instead. But fmt in their main branch recently added `[[noreturn]]`
annotations in a way that cause this to be compiler errors for our
overrides. Needed an extra std::terminate() call to guarantee the
noreturn promise.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
This change fixes out-of-bounds reads and incorrect scanline ordering
when OpenEXR's decreasingY lineOrder mode is used.
OpenEXR expects to process scanlines in decreasing order when the
decreasingY lineOrder option is enabled. This change fixes the code that
provides the chunks of scanlines to OpenEXR so that the proper scanlines
are available when it accesses the FrameBuffer OIIO created. The old
code was providing incorrect scanlines AND setting up the FrameBuffer
with incorrect pointers so out-of-bounds reads were occurring.
The key things to keep in mind are:
- Chunks of scanlines need to be sent to
OpenEXROutput::write_scanlines() from the bottom of the image to the top
when decreasingY is enabled. If the chunk's scanline range does not
contain the scanlines OpenEXR is expecting to fetch, then it can cause
out-of-bound reads because the wrong information is used to construct
the pointers for the Slices created when constructing a FrameBuffer.
- OpenEXROutput::write_scanlines() does its own chunking of scanlines
and this logic must also properly start from the bottom of the chunk
data passed into this function when decreasingY is enabled.
- The to_native_rectangle() call in OpenEXROutput::write_scanlines() may
return a pointer to m_scratch if a format conversion is needed. This
means the pointers passed to OpenEXR, in this situation, _WILL NOT_ be a
full frame buffer. This caused OpenEXR to do out-of-bounds reads because
the computations used to create the Slices were using the wrong scanline
number to adjust the pointer.
- The progress callback computation needed to be modified to handle
traversing scanlines in decreasing order.
I added an openexr-decreasingY test to verify that the code no longer
crashes because of the out-of-bound reads and it also verifies that the
decreasingY images match the increasingY images.
---------
Signed-off-by: Aaron Colwell <300262+acolwell@users.noreply.github.com>
Adds support for the following DXGI formats when using the DX10 extended
header on DDS files:
- Compressed BCn and its sRGB variants.
- BGRA8/BGRX8/RGBA8 and its sRGB variants.
- RGB10A2.
- R16.
On the sRGB formats, the attribute `oiio:ColorSpace` is also set to
`sRGB`.
Closes#4045.
Added new DDS test images to the OpenImageIO-images repo and updated the
testsuite expected output.
OpenImageIO-images PR:
https://github.com/AcademySoftwareFoundation/OpenImageIO-images/pull/7
Signed-off-by: Alejandro Aguirre <alexguirre.aa@gmail.com>
As mentioned in #4205, #3900 and
https://github.com/AcademySoftwareFoundation/OpenImageIO/discussions/3910#discussioncomment-8913737
OpenImageIO previously did not support reading of 16- and 32-bit files
beyond their merged image data section. I have now implemented this by:
1. Adding support for reading layer info from the "Lr16" and "Lr32"
Tagged Blocks as well as adding additional checks to the regular layer
info section
2. Implemented the Zip and ZipPrediction compression codec which is
required to read these files as that is photoshops preferred compression
codec for those bitdepths
Due to the way the Zip data is stored (per channel but not per scanline)
the current implementation reads the whole channel into a buffer which
is then memcpy'd from by calls to `read_native_scanline()`. The buffer
gets cleared on deletion of the `PSDInput` object
---------
Signed-off-by: EmilDohne <86836589+EmilDohne@users.noreply.github.com>
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Co-authored-by: Larry Gritz <lg@larrygritz.com>
Windows CI was failing because it couldn't get a proper libtiff from
VcPkg, which in turn is because lzma is a dependency of the libtiff
build in VcPkg, and for the time being, it looks like VcPgk is failing
to get lzma which is part of the big xz debacle of 2024.
So work around for now by just building our own libtiff rather than
relying on VcPkg. Seems to work fine.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
For OSL, we want to be able to decode a wrapmode directly from a
ustringhash without having to expensively convert it to ustring.
Signed-off-by: Chris Hellmuth <chellmuth@gmail.com>
... in the OCIO 1.x case where we relied on our own Find module rather
than an exported cmake config from OCIO itself. This is desired for
uniformity of naming with the exported configs of the later versions.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Base on the [OIIO slack channel
discussion](https://academysoftwarefdn.slack.com/archives/C05782U3806/p1689031780561469),
this PR implements the `copy_image` function for the OpenEXR format. The
main motivation of this PR was due to a recompression issue where
copying a lossy compressed EXR image (pixels are decoded and then
encoded again), which can cause the image quality to downgrade on each
iteration.
- `OpenEXROutput` is now a friend class of `OpenEXRInput`. Since
`OpenEXROutput::copy_image()` needs to access some of the private
members in `OpenEXRInput` (This approach is referencing the
implementation in `JpegInput` and `JpegOutput`).
- The `OpenEXRInputStream` and `OpenEXRInput` class have been moved from
`exrinput.cpp` to `exr_pvt.h`. As `OpenEXRInput` depends on
`OpenEXRInputStream`, so both have to be moved together. Such relocation
is necessary due to the above reason, i.e. `OpenEXROutput::copy_image()`
needs to access some of the private members in `OpenEXRInput`.
- And the implementation itself, `OpenEXROutput::copy_image()` overrides
the base class method just like `JpegOutput` does.
The `OpenEXROutput::copy_image()` implementation took the advantages of
the existing OpenEXR function `Imf::OutputFile::copyPixels`, quoting the
comment from the function:
> //--------------------------------------------------------------
// Shortcut to copy all pixels from an InputFile into this file,
// without uncompressing and then recompressing the pixel data.
// This file's header must be compatible with the InputFile's
// header: The two header's "dataWindow", "compression",
// "lineOrder" and "channels" attributes must be the same.
//--------------------------------------------------------------
IMF_EXPORT void copyPixels (InputFile &in);
When the output image is copied from an input image, the pixel data can
only be located in one of the input parts. So it checks if both input
and output part is initialized (non-null), then invoke the `copyPixels`
function in place.
All the error handling is done within the OpenEXR `copyPixels` call.
There is a try-catch block in OIIO side that emits an error message and
fallback to the default image copy routine, e.g.
`ImageOutput::copy_image`.
Note that currently, this doesn't do anything optimized for the
"core library" code path.
---------
Signed-off-by: Andy Chan <andychancse@gmail.com>
We inadvertently started using libheif functions that only became
available in their version 1.16.0. Guard against that and now we
recommend at least that version (but we aren't yet changing the minimum
requirement). This fixes a build break introduced in #4142 (but only
breaks for older libheif).
Signed-off-by: Larry Gritz <lg@larrygritz.com>
The operation it's supposed to be performing is to merge in the gathered
value where the mask is set, and leave mask=0 lanes unchanged. But for
our "no SIMD" fallback (i.e., no AVX2 for 4- and 8-wide), we incorrectly
implemented it as setting the unmasked lanes to 0!
This was quite wrong, and unfortunately masked by the fact that our test
of the function initialized a variable to 0. Change to a better number.
The best number.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
A variable was declared unconditionally but only used when Freetype is
enabled. Move its declaration to be within the right conditional, on
order to avoid compiler warnings about unused static variables.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Automatically recognize some additional color space name synonyms:
"srgb_texture", "lin_rec709" and "lin_ap1" are names that MaterialX and
USD use as canonical names.
Also add common permutation "srgb_tx" and "srgb texture" as additional
aliases for srgb.
Fixes#4165
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Added a new bit of pseudo-metadata that can be retrieved by oiiotool's
expression substitution: `{TOP.NONFINITE_COUNT}` will be replaced by the
count of inf and nan values present in the top image.
Also, the `-fixnan` command now has the side effect of setting a user
variable called `NONFINITE_COUNT` to the number of nonfinite values that
were detected or repaired.
Between the two of these, it's now easy to solve the following problem
that actually came up in production for us.
Sometimes, nans creep into images, and it's easier to repair them than
to fix the complicated process that produced them in the first place.
For some process in the studio, we had a script that was doing this
preemptively on every image it encountered:
oiiotool image.exr -fixnan black -o image.exr
It was very expensive to repair every image whether it needed it or not,
when actually it's only needed once every
many-thousands-of-images. Most of the expense was in outputting the
repaired file overtop the original. Now we can do this
oiiotool image.exr -if "{TOP.NONFINITE_COUNT}" -fixnan black -o
image.exr -endif
It still has to read the image and look for NaN values, but since there
are almost never any actual NaN's, now for the usual case, it can skip
writing the output -- which was the vast majority of time.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
It seems that unlike most image formats where we merely note the camera
rotation but don't rotate the pixels, the heic reader automatically
rotated.
The trick is that using the C++ API wrapper of libheif, there is no way
to pass the decoder options struct that is where you need to say not to
auto-transform. Needed to drop down to the lower level C API for this
one spot.
We want this format to behave like the others -- the orientation is
advisory, and it's up to the app how to deal with it. But we have back
compatibility to deal with.
If a rotated image is aecountered, OIIO 2.5 and earlier will by default
still auto-rotate to preserve compatibility, but OIIO 2.6+ will not
auto-rotate.
When not rotating, the Orientation metadata will reflect the desired
display orientation of the image, according to what was in the file.
When auto-rotating, Orientation will be 1 (canonical display
orientation), and a new attribute "heif:Orientation" will reflect what
was originally in the file.
When opening a file for input, the special configuration metadata hint
"heif:reorient" can express a preference for auto-rotation, overriding
the default. Thus, setting this hint to 0 for OIIO 2.5 will turn off
auto-rotation, and setting it to 1 for OIIO 2.6+ will make it
auto-rotate like it used to do.
I also added some additional tests for heif files.
Along the way I fixed a rather embarrassing documentation problem as
well, where all the orientation codes were "off by 1", oh boy.
Fixes#4123
---------
Signed-off-by: Larry Gritz <lg@larrygritz.com>
This PR is for allowing the TypeDesc type to be used directly in a C API
without indirection, and therefore also in a Rust API without
indirection.
---------
Signed-off-by: Scott Wilson <scott@propersquid.com>
It somehow escaped our notice, but not only does DCMTK have an exported
config, but also CMake itself has a FindDCMTK.cmake module. So get rid
of ours, and prefer the config if available. This is hoped to solve some
edge cases for finding and using this library properly.
Fixes#4146
(I hope?)
Signed-off-by: Larry Gritz <lg@larrygritz.com>
* Make sure clang variants are before clang and things that might
emulate gcc are before gcc.
* Use the right symbol for detecting ICC
* Capitalize `__linux__` correctly, oops.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
This is Update 7, the original PR is ongoing and we will continue to
update as additional people relicense.
Per PR #3905 (https://github.com/OpenImageIO/oiio/pull/3905), update the
RELICENSING document with the names of people and companies who have
signed on to the relicensing thus far, and update the SPDX license
notices on source files whose extant authorship (per 'git blame')
comprises only authors who have relicensed.
To the best of our estimation, we now have 99.85% of lines of code
relicensed, and 93.2% of all source files now consist of ONLY
Apache-2.0 licensed code with no traces of BSD remaining.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Add testsuite/filters that verifies that the filter shapes are correct
and don't change if we make future changes to the filter code.
Along the way, add a new global getattribute query 'filter_list', that
returns a semicolon-separated list of all the 2D filters it knows about.
---------
Signed-off-by: Larry Gritz <lg@larrygritz.com>
We did this separately each place we needed it. Do it just once, in a
public header, and give it a name that won't clash with other packages.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Remember that ImageBuf only stores one pixel data type per buffer, but
of course they may have differing-per-channel types in image files. So
how does oiiotool know what to write? There is a series of heuristics:
-d is for explicit user control, if not, then try to deduce it from the
input files.
A bedrock precept is that `oiiotool in.exr -o out.exr` ought to write
the same channel types that it read in.
The logic to implement this with the various image operations and
possible overrides can be hard to get right. Indeed, I did not get it
right. It made mistakes even in simple cases like the above. And the
code was complicated and hard to understand, as well.
This PR rewrites a couple functions that implement this chunk of this
logic, both correcting several errors as well as (I hope) being clearer
and better commented so that we can more easily reason about it and
modify it in the future.
I also added some additional tests to testsuite/perchannel to verify
some of the cases that were previously failing.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Primarily, recent changes (PR #4071) to vint4::store for the NEON case
appear to have some type mismatches, which apple clang on ARM-based Mac
(including our CI) seems ok with, but which is generating type errors on
other ARM Linux platforms.
I think the types were weird here, so I tightened it up to get the types
right for temporary variables in that function. That's the primary fix
here.
Secondarily, I modified simd.h and the CMake setup so that build option
USE_SIMD=0 will disable NEON in the same way that it disables SSE. (I
realized that USE_SIMD=0 was not disabling NEON, so there was no way for
a NEON platform to completely disable SIMD if they needed to.)
Fixes#4111
Signed-off-by: Larry Gritz <lg@larrygritz.com>
* jpeg output: Extra memory check
* TS: batch texture call fixed to handle any number of channels
* TS: assert to guard against possible null pointer dereference
* oiiotool printinfo: clarify assertion for the sake of static analysis
* jpeg2000 reader: assertion to ensure we don't dereference a null
pointer
Signed-off-by: Larry Gritz <lg@larrygritz.com>
No more need for the paid macos13-arm-py311 runner and only use it on
the ASWF repo selectively. An ARM-based MacOS-14 runner is now available
for open source projects, so can run free on every PR and on user forks.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
New OIIO global attributes (read only):
- "build:platform" retrieves the OS and CPU type that this build of OIIO
is fof (e.g., "Linux/x86_64").
- "build:compiler" retrieves the compiler name and version used to build
OIIO itself (e.g. "gcc 9.3").
- "build:dependencies" retrieves a semicolon-separated list of library
dependencies -- both for image format support (like libtiff) but also
general (boost, TBB, Python, fmt, OCIO).
- "build:simd" is a new (preferred) synonym for the SIMD and other
hardware capabilities selected at build time. The old name, "oiio:simd"
is hereby softly deprecated as confusing.
`oiiotool --buildinfo` is a new command that prints this information.
Example output:
```
$ oiiotool --buildinfo
OIIO 2.6.0.2spi | MacOS/x86_64
Build compiler: Apple clang 15.0 | C++17/201703
HW features enabled at build: sse2,sse3,ssse3,sse41,sse42
Dependencies: OpenEXR 3.2.1, LIBTIFF Version 4.6.0, jpeg-turbo
3.0.1/jp80, dcmtk 3.6.8, FFMpeg 6.0 (Lavf60.16.100), gif_lib 5.2.1,
libheif 1.17.6, OpenJpeg 2.5.0, null 1.0, OpenVDB 11.0.0abi11, libpng
1.6.40, Ptex 2.4, libraw 0.21.2-Release, Webp 1.3.2, Boost 1.83,
OpenColorIO 2.3.1, Python 3.11.7, TBB 2021.11.0, fmt 10.2.1
```
This is all a convenience utility to make it easy to ask users to run a
simple command that will tell us a lot of information relevant to
resolving their issues in cases where it's related to some of their
build-time compiler or dependencies.
Also cleaned up the "bug report issue template" to clarify the
instructions and specifically ask people to run this command and include
the information with any bug report.
---------
Signed-off-by: Larry Gritz <lg@larrygritz.com>
A better default for users is probably to assume a 2.2 value if the
GAMMA env variable is not set. If you want something different, set
GAMMA proactively or use a real color managed workflow.
Fixes#318
Signed-off-by: Larry Gritz <lg@larrygritz.com>
With GitHub runners with more cores becoming available, adjust our CI
scripts to do parallel builds with nprocs+2 instead of hard-coding. We
use slightly more than the number of available cores because these
builds and tests tend to be I/O bound, so they can be a little
over-threaded to get maximum utilization.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Upon reading, the subsampled Y/BY/RY(/A) channels of luminance-chroma
images are automatically converted to RGB(A) channels. These images will
set a metadata "openexr::luminancechroma" to 1 in the ImageSpec, to indicate
that the original image was luminance/chroma.
Subsampled channels are not supported with the exception of reading
luminance-chroma images with vertical and horizontal sampling rates of 2.
This limited support does not work when OpenEXR's C Core API in used, only
when OpenEXR's C++ API is used. Furthermore, it does not work in
combination with tiles, multiple subimages, mipmapping, or deep pixels.
The test images from the OpenEXR testsuite have been added and work with
the C++ variant of the test.
Fixes#4051.
---------
Signed-off-by: Joachim Reichel <43646584+jreichel-nvidia@users.noreply.github.com>
* pngoutput.cpp: Catch possible exceptions.
* printinfo.cpp: assertions to assure we don't dereference a null
pointer.
* testtex.cpp: make sure allocated chunk is initialized
* WriterInternal.h: Catch condition that could lead to buffer underflow.
* imagebufalgo_mad.cpp: Simplify pointless clause. The prior test was
redundant. Since a few lines above, if B was an image but A was not, we
swapped to ensure that A was always an image. So here, we can just test
A to know for sure that at least one of them is an image. That makes the
test here simpler, but it also makes it more clear to static analysis
that from this point forward, A can't be nullptr.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
The function signatures in the header didn't match those in the docs,
after some previous changes. It's probably been broken in the docs for
some time. Nobody noticed?
Signed-off-by: Larry Gritz <lg@larrygritz.com>
If you are looping i over range [0,width), and want index to go the
other direction, you need `index = width - 1 - i`, not `index = width - i`.
Identified by Sonar static analysis.
---------
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Bump our clang-format standard from llvm 12 to llvm 17, and our
C++ standard (for formatting) to 17.
Fixes/improvements along the way:
* Moved the clang-format CI test from an aswf container to bare Ubuntu
to make it run faster -- it doesn't need most of the dependencies,
so there's no point wasting 2 minutes or so downloading a container.
* build_llvm.bash: Make sure llvm installs when needed, even if not
using clang.
* gh-installdeps.bash: Setting SKIP_SYSTEM_DEPS_INSTALL=1 skips
installation of many non-essential dependencies.
There is some reformatting here and there as 12->17 changed some formatting
choices somewhat, but it's fairly minor. Most changes are improvements,
some are neutral, a few are inexplicable but not less readable than before.
We've long since accepted the tradeoff that using clang-format makes some
things formatted differently/worse than we would have preferred, but we deem
it worthy of the benefit of having it all 10% automatic and never fighting
over formatting in PRs.
---------
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Bleeding edge CI test failures due to more changes in fmt's trunk.
Solutions:
* Accommodate some recent header refactoring in fmt.
* Straighten up some const issues that worked before, but break under
the latest fmt. Making the custom formatter's 'format' method be const
fixes the latest, but breaks some of the oldest versions of fmt we
support, so we add the const only from fmt 9.0 on.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Remove duplicate dependecy for OpenImageIO_Util to fmt.
OpenImageIO target already depends on fmt through OpenImangeIO_Util
library.
Signed-off-by: Dominik Wójt <domin144@o2.pl>
fmt provides cmake config files since at least version 7.0.0, which is
the minimum version supported.
Fixed some variable names, so the provided config can be used.
The previous variable names were missing, if the library was installed
in a custom location,
like in case of manual build or provided by conan.
---------
Signed-off-by: Dominik Wójt <domin144@o2.pl>
We did `import OpenImageIO as oiio` then instead of using `oiio`, we
used `OpenImageIO`.
This was failing sometimes. Frankly, I'm not sure why it ever worked.
Maybe it works on some versions of Python but not others?
I dunno. But this fixes it everywhere and is clearly correct.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
We had an awkward situation of the simd matrix44::inverse() only being
defined for Intel SSE, and in all other cases falling back on
Imath::M44f::inverse. The problem with that is that it made simd.h
depend either directly on ImathMatrix.h, or assume that it would be
included prior to inclusion of simd.h. This was not good.
This patch rewrites inverse() to eliminate all of the direct SSE
intrinsics in terms of other simd wrapper functions, which means that
they will be defined one way or another on all architectures.
Along the way I added a new vfloat4 shuffle that combines two vectors
(corresponding to `_mm_shuffle_ps` in SSE land), and a vfloat4
constructor from 2 float*', where the first two elements come from the
first ptr and the second two elements come from the second ptr.
Ultimately, the final implementation I settled on for the new inverse
didn't use these (an earlier version did), but I want to keep them
around anyway in case they are useful in the future. I did add a test
for them.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Move a bunch of specific tests from testsuite/oiiotool to
testsuite/oiiotool-copy to group similar tests together, and also to
load balance better between the tests.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
This broke some time back, probably partly related to our change in
repository home. This seems to get it mostly working? The reports are
still not showing up, but I think the solution might be to make sure
this is checked into master, so we need to merge this before assessing
whether there is more work to be done in this area.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
For image with more than 4 channels, we were misallocating the OpenGL
buffer by using the total channels instead of the 4 we wanted to create
the texture out of. Also, make sure the calculation usees wide types to
avoid possible integer overflow.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
It's a little involved to explain why, but at work a combination of
libraries and some trickery with static linkage led me to really need a
way to inject another library into the mix, just for iv linkage.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
For the in-place case (where src and dst refer to the same IB), we were
doing an unnecessary full-image copy.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Unity builds (which we do for CI to speed it up) combine source files,
sometimes in ways that are a bit non-deterministic, and can run into
violations of the ODR that were not a problem when separate definitions
of the same thing (a macro in this case) were in different compilation
units but are in the same one as a unity build.
In this case, a private OIIO_DISPATCH_TYPES macro in printinfo.cpp
conflicted with a different macro of the same name in
imagebufalgo_util.h.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
## Description
Minimal change to update link to OpenEXR's new Test Images page. I poked
around a bit to find a "friendlier" variant of the link but could find
none.
Note that the section containing this link implies that folks need to
download these files themselves and should be "placed in a sibling
directory to oiio-testimages". The problem is that not even OpenEXR
lists the repository for the test images nor does the page linked above
offer a download.
Should we list out the openexr-images repo address too and say to just
clone that?
Fixes#4042
---------
Signed-off-by: Jesse Yurkovich <jesse.y@gmail.com>
Fix for #4084. Will now check if the given filename exists before
checking for REST parameters.
Before:
```
bin % ./oiiotool \?.png -o \?-output.png
oiiotool ERROR: read : "?.png": ImageInput::create() called with malformed filename
Full command line was:
> oiiotool ?.png -o ?-output.png
```
After:
```
bin % ./oiiotool \?.png -o \?-output.png
```
No output, `?-output.png` created.
Signed-off-by: AdamMainsTL <adam.mains@topazlabs.com>
Please look this over. If you are one of the duplicated names and I
haven't chosen your preferred canonical email (I made my best guess),
please comment here with corrections.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
I didn't have the condition right -- the `if` isn't respected in the
`pull_request` section, so this job was running on every PR instead of
just the ones with "macarm" in the branch name. Refactor to get the
conditional on the job instead.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
There was a flaw in PNGOutput::write_scanlines when the y range of
scanlines to write consisted of less than the whole image, and alpha
de-association was needed -- we used the whole-image size instead of the
y range of scanlines size.
Fixes#4044
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Without getting the version properly, we weren't disabling use of a
deprecated field for OpenJpeg >= 2.5, as we thought we were.
This fixes recently broken CI when building against the current openjpeg
master.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
In some environments, only one kind of the library could be available.
Choose proper fallback in such cases. Prefer static library, if
LINKSTATIC is ON and dynamic otherwise.
Signed-off-by: Dominik Wójt <domin144@o2.pl>
## simd.h improvements:
vint4::load from unsigned char pointer got pre-SSE4 code path. Testing
on Ryzen 5950X / VS2022 (with only SSE2 enabled in the build):
- vint4 load from unsigned char[]: 946.1 -> 4232.8 Mvals/sec
vint4::store to unsigned char pointer got simpler/faster SSE code path,
and a NEON code path. Additionally, it got test correctness coverage,
including what happens to values outside of unsigned char range (current
behavior just masks lowest byte, i.e. does not clamp the integer lanes).
- vint4 store to unsigned char[]: 3489.8 -> 3979.3 Mvals/sec
- vint8 store to unsigned char[]: 5516.9 -> 7325.3 Mvals/sec
NEON code path as tested on Mac M1 Max (clang 15):
- vint4 store to unsigned char[]: 4137.2 -> 6074.8 Mvals/sec
## Tests
vint4 store to unsigned char pointer got actual correctness checking
test, which seemingly was lacking before (only had a benchmark test).
Slightly different output to match for the combo of OCIO >= 2.2, but
simultaneously Python 2.7 -- which was not in our CI test matrix, so I
missed it, but ran into it at work.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Add a few more assertion checks and error messages in imagebufalgo_test.
I observe that if somebody runs the testsuite with `$OCIO` set to a
config that doesn't contain the required color space names for this
test, it will fail. As a backstop, use the default built-in config if
the initial request fails. That will always give a working processor
between these spaces if OCIO >= 2.2 is being used.
Also one more tiny SPI-specific enhancement to hacky identification of
nonconforming color space names in our old configs.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
- Fix perceptual test false positive by removing the initialization of
`cr` to avoid losing the information when leaving block scope
Included additional test with parameter `-fail 1`, which was giving a
false positive pass for a perceptual difference.
This was due to the fact that the result variable `cr` was getting out
of scope for the perceptual calculation and the first `if (cr.nfail <=
imagesize_t(allowfailures)) {` was testing for the result of the
absolute difference instead of the perceptual difference.
Before:
```
Comparing "img1.exr" and "img2.exr"
PASS
```
After:
```
Comparing "img1.exr" and "img2.exr"
64 x 64, 3 channels
Mean error = 0
RMS error = 0
Peak SNR = 0
Max error = 1 @ (5, 17, R) values are 0.1, 0.1, 0.1 vs 0.1, 0.6, 0.1
0 pixels (0%) over 0.008
121 pixels (2.95%) over 1
121 pixels (2.9541%) failed the perceptual test
FAILURE
```
Signed-off-by: Aura Munoz <aura.munoz@autodesk.com>
The important bug fix is in oiiotool image input when autocc is used,
where we reversed the sense of a test and did the autocc-upon-input if
the file's color space was equivalent to the scene_linear space
(pointlessly), and skipped the conversion if they were different (oh no,
big bug!).
Along the way:
* Add missing try/catch around OCIO call that should have had it.
* Some very ugly SPI-specific recognize-by-name color space heuristics.
I hate this, sorry, but it solves a really important problem for me.
* A bunch of additional debugging messages during color space inventory,
enabled only when debugging, so nobody should see these but me.
* A couple places where we canonicalize the spelling of
"oiio:ColorSpace".
Note that there is still an issue with the color space classification
using OCIO 2.3+'s identification of built-in equivalents by name. These
are shockingly expensive. I will return to this later.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Used to be no problem, but after a recent VSCode update, this seems to
have been causing errors when asking VSCode to format. No idea why this
recently became problematic.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
It's started failing on GHA and is old enough that it's not worth trying
to track down what's broken. We still have MacOS 12 and 13 tests in the
matrix.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Everything works, no code changes needed. But with some libtiff builds,
there is some variation in exactly which error messages bubble up when
reading certain corrupted files. This fixes some recently broken CI.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
This is Update 6, the original PR is ongoing and we will continue to
update as additional people relicense.
Per PR #3905 (https://github.com/OpenImageIO/oiio/pull/3905), update the
RELICENSING document with the names of people and companies who have
signed on to the relicensing thus far, and update the SPDX license
notices on source files whose extant authorship (per 'git blame')
comprises only authors who have relicensed.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
When we bumped to cmake 3.15 minimum for 2.5, we changed lots of
constructs like this:
if (VERBOSE)
message (STATUS "blah")
endif ()
to this:
message (VERBOSE "blah")
but I didn't realize that this was not enough, and to do it right
requires CMAKE_MESSAGE_LOG_LEVEL. So even when VERBOSE=ON, we weren't
actually getting all the verbose messages. Here, we fix it.
Also, when verbose, print the list of C++ compile features that cmake
discovers.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
To help with some other investigation I'm doing, amend imagebuf_test
with benchmarks to compare the relative speeds of traversing a big
ImageBuf with Iterator, ConstIterator, or just using pointers on an
equivalently large array.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
This change adds a menu item "Move to new window" under the menu "File" which
creates a new window and initialises it with the currently displayed
image.
I have tested the UI changes on Mac and linux, I don't have a Windows
machine currently.
No other functionality should be affected by this change.
This was one item from issue #2058
---------
Signed-off-by: Anton Dukhovnikov <antond@wetafx.co.nz>
Takes the make_texture section of the imagebufalgo doc and turns it into
a unit test in both python and c++.
This adds a make_texture section to the `docs-examples-cpp` test and the
`docs-examples-python` test.
Signed-off-by: Danny Greenstein <danny.gr@gmail.com>
- ``raw:threshold``
- float
- Libraw parameter for noise reduction through wavelet denoising..
The best threshold should be somewhere between 100 and 1000.
- ``raw:fbdd_noiserd``
- int
- Controls FBDD noise reduction before demosaic. 0 - do not use FBDD
noise reduction, 1 - light FBDD reduction, 2 (and more) - full FBDD reduction
---------
Signed-off-by: ssh4net <libalias@gmail.com>
Revision of PR #4023
I have since determined that although OpenVDB raised their own build
requirements to C++17 starting with 10.0, it wasn't until 10.1 that
C++17-isms crept into their header files and force downstream projects
to need C++17 to consume OpenVDB's headers. So we can go back to
allowing use of OpenVDB 10.0 even when we are building with C++14.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
There's a new GitHub runner in town!
Set up CI to run on Mac ARM. But not all the time -- it's too expensive!
This runner is only available for paid accounts, and it's 20x more
expensive than paying for a Linux runner. The ASWF can't afford to run
this job on every iteration of every PR, and again upon merge.
We split it into a separate workflow so we can direct it to run only for
tagged releases, branches with names containng "-alpha", "-beta",
"release", or contain the substring "macarm" (this last one is so we can
force a particular PR that we think is directly relevant to ARM to test,
even though it's not one of those other release-related cases). We also
run it once a month, on the 27th -- a few days before each monthly
release -- to try to catch any ARM problems we've accidentally
introduced by PRs that didn't test.
It's also rigged to run only on the ASWF repo, so you don't need to
worry that you will get charged for pushes to your own fork. (On the
other hand, it also means you aren't testing against ARM when you push
to your fork prior to making a PR.)
In the process of testing this, of course I found some minor things that
needed to be fixed on ARM -- a couple compile issues, and several
testsuite reference outputs that needed to be updated because they
differ ever so slightly in some math calculations on the different CPU
architecture.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Looks like OpenVDB >= 10.0 requires a minimum of C++17. Warn at build
time if trying to use an OpenVDB too new for the C++ compiler being
used.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
We were using parallel_convert_image but not realizing that asking for
the IB::localpixels() would give us nullptr for an IC-backed image.
Instead, "wrap" the cv::Mat with an IB and then use IBA::copy(). This
handles both in-memory and IC-backed ImageBuf's, and also has many other
advantages (such as format conversion) we may wish to take advantage of
in the future.
Fixes#3800
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Convert C++ and Python examples from the "imagebufalgo" chapter into
tests within the "docs-examples" testsuites (#3992).
Signed-off-by: Jeremy Retailleau <jeremy.retailleau@gmail.com>
Solves the issue "Load a directory: iv dirname ought to be the same as
specifying all the image files in that directory" from the Great Big iv
Wish List #2058
This PR addresses the request from the Master list of iv feature
requests to allow loading a directory as an argument, which in turn
loads all images within the specified directory. The code modifications
include checks to validate the file or directory existence, and if a
directory is provided, iterates through its files to load all valid
images into iv. This enhancement simplifies the process of viewing
multiple images collectively located in a single directory.
---------
Signed-off-by: Chaitanya Sharma <39400946+CheeksTheGeek@users.noreply.github.com>
Teach `idiff` to accept a directory as the 2nd argument.
Fixes#4009
## Tests
Functional tests for `idiff` don't currently exist. Manual testing
verifies it works as advertised.
---------
Signed-off-by: David Aguilar <davvid@gmail.com>
Slightly different set of issues, so I wanted to provide a full example
for anyone who wants to work on this chapter.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Our exr writer uses some special hints starting with "openexr:" to
control aspects of the writing, and the reader sets these to
communicate some things about the file it's reading.
But we identified a case where the actual exr file had metadata named
"openexr:levelmode" -- which is of no particular meaning to openexr
itself, but it is the name we use to communicate hints to our writer
by make_texture. We just need to make sure that if we don't set it
proactivesly, we make sure to clear it if there is a stray bit of
metadata with that same name coincidentally.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
In the ongoing effort to help newcomers to OIIO diagnose their own
problems, this uses a trick to print any global error messages that
never got retrieved by OIIO::geterror() at the time that the program
exits. (This is all similar to a change we made recently that adds an
analogous behavior to ImageBuf, but in this case it's for those error
messages that don't have an object like an ImageBuf to attach to.)
This should have no effect on anybody who diligently checks
OIIO::has_error() or OIIO::geterror() after any failed calls that would
send error messages to that global store. Also, it is possible to
disable the behavior by setting the new global attribute
"oiio:print_uncaught_errors" to 0.
But for users who don't know to check errors and are experiencing
baffling behavior with OIIO, maybe seing those error messages will
prompt them to understand what's going wrong before they feel the need
to file an issue against OIIO or need to ask us for help.
Doing this helped me to find two places where our own code in OIIO
failed to clear the global error at times that it should, and these are
also fixed as part of this PR.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
The following sequence would have a floating point exception:
// Readable IB backed by ImageCache
ImageBuf buf("file.exr"), 0, 0, ImageCache::create());
// Make a mutable iterator, even though it's an image file reference.
ImageBuf::Iterator<float> it(buf); <----- CRASHES INSIDE HERE!
The problem was in ImageBuf::IteratorBase::init_ib, several steps taken
if it's a mutable IB looking at a "read-only" IC-backed file, including
calling make_writable() on the buffer. But we neglected to set
m_localpixels to true.
Fixes#3770
---------
Signed-off-by: Larry Gritz <lg@larrygritz.com>
A static_assert was added recently as part of PR #3898, which assures
that the size of a TileID is a multiple of 8 bytes. But on architectures
with 32 bit pointers, this assertion fails. Which is not quite what we
want, because the code isn't wrong if the struct isn't a multiple of 8
bytes; it's just less efficient. And we don't want to fail for 32 bit
arch.
Bottom line: only do this check to ensure we're in the most efficient
case if we're 64 bits. Nobody's expecting highest performance on a 32
bit machine anyway.
Fixes#4001
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Add a GHA workflow that is triggered up on drafting a release, which
will echo a notice to the ASWF slack #release-announcements channel.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
This is the first release of 2.5 as the current supported production
release. The 2.4 family is now relegated to "obsolete" status with
only important bugs fixed and support level slowly reducing over the
next several months is adopted.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
First, some background: In the course of working toward OIO 2.5, we
added some heuristics that used OpenColorIO 2.2's new
isColorSpaceLinear, and also using its new "built-in config" to figure
out if an arbitrarily named color spaces in a user's config were
equivalent to some built-in ones that are commonly used.
All seemed well until... we recently discovered that it was moderately
expensive (~0.1s per color space!) on the OCIO side, which wasn't
particularly noticeable when I was testing, but can add up to several
seconds of needless startup time when inventorying large configs with
dozens and dozens of color spaces. And even worse, it was exceptionally
expensive for color spaces with LUT3Ds because of a bug on the OCIO side
that would invert the 3D LUT needlessly (taking >10s per LUT3D
inversion), leaving to MINUTES of overhead for some configs with many
such spaces.
So this PR attempts to work around all of this with several big changes
(which are amplified by the fact that I've really shuffled around the
code between methods and within the file, sorry):
1. Per suggestion from Doug Walker, there's a new heuristic to try to
detect if a color space from or to the reference space appears to
contain a 3D LUT, and if so, skip some of the more expensive heuristics
that appear to cause the LUT to be inverted. It's not perfect, but we're
making a bet that no LUT3D-involving color spaces are going to be one of
the few canonical spaces we're trying to detect. This fixes the "10s of
seconds of delays" problem for those particular color spaces.
2. When using OCIO 2.3+, use its newly introduced utility
IdentifyBuiltinColorSpace to have it tell us which color space in the
user's config matches some of the canonical ones we care about in the
built-in config.
3. Whew, switch to a kind of lazy evaluation of the propererties of each
color space. Whereas before, we fully inventoried the config and for
each cs we found in it, do all the heuristics, now we only do the
inexpensive tests up front (silly rules of thumb where we guess based on
the *names* of the color spaces). We save the potentially more expensive
but thorough ones (like transforming some sentinel colors around to see
if the behavior of the cs matches a color space we care about, and
asking OCIO what it thinks about isColorSpaceLinear) only if we are
asked to do certain things with *that* color space. In other words, a cs
in the config that OIIO doesn't need to *use* in some way will now not
undergo the expense of the trickier investigation about what it might be
equivalent to.
These add up to making the overhead for ordinary use be significantly
sub-second, no longer a problem. Nature is healing.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
It was possible for `bit_range_convert` to be called with a 0 for the
`FROM_BITS` parameter leading to an infinite loop in its processing.
Fixes#3993: OIIO library freezes when loading R16G16 uncompressed DDS files
The test image from the above bug is now able to be read correctly.
Signed-off-by: Jesse Yurkovich <jesse.y@gmail.com>
We found that PNG output (which is required to be unassociated alpha)
had precision problems with low alpha values because it quantized to the
final bit depth before doing the alpha deassociation.
Explanation:
PNG requires associated alpha to be stored in the file. So if you have
(unassociated) pixel value [0.00235, 0.00106, 0.00117, 0.0025], that
means that we need to divide the colors by alpha before storing in the
file, so that's [0.94, 0.424, 0.468, 0.0025] if you have full precision.
But you don't, because PNG only supports uint8 and uint16. For uint8,
this should end up as [240, 108, 119, 1].
But it turns out that our png writer does the integer conversion before
the un-premultiplication, so actually we first quantize from [0.00235,
0.00106, 0.00117, 0.0025] to [1 0 0 1], and THEN we un-premultiply,
giving us the [255, 0, 0, 1] that made you think "Why is that red value
clipped? And where did green and blue go?"
The solution, then, is to deassociate as floats first, then quantize to
the final integer data type.
Also add a test for this, as well as a test that verifies correctly
writing of 16 bit files that I had some problems with in the last png
PR.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
* Implement PNGOutput::write_scanlines, previously we only had
write_scanline. Sadly, being able to write multiple scanlines at once
reduces overhead by much less than I had hoped. But there's a bit of
savings, so I'll keep it.
* New compression recognized: "pngfast", which translates to the
Z_BEST_SPEED setting. I don't particularly recommend this, but it's
useful for benchmarking.
* New compression recognized: "none", which turns off compression. Also
not recommended, as it makes the files much larger. Mostly done for
benchmarking and other comparisons.
* Don't unconditionally copy user's data buffer to m_scratch. It's only
necessary if there's a data type conversion, stride shuffling, alpha
deassociation, or endian swapping. When none of those are needed, we can
avoid the extra allocatin and copy, and so now we do. (Sigh, in real
use, we're almost always doing the alpha deassociation, so this rarely
is a savings.)
* Better documentation on the custom output attribute hint
`"png:filter"` noting that a non-default value can make it write PNG
files dramatically faster, but with the tradeoff of having much larger
files.
---------
Signed-off-by: Larry Gritz <lg@larrygritz.com>
For the upcoming DevDays, I wanted to spruce up our main README to make
it easier to find info, and I wrote a new guide for how to locally build
the documentation.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
libjpeg-turbo has exported cmake configs since its version 2.1. Relying
on this simplifies the build and allows us to completely remove the
troublesome Findlibjpeg-turbo.cmake. This implies raising the floor for
libjpeg-turbo support to 2.1 (the current release is 3.0) which was
released in April 2021 (beta in Nov 2020).
That's cutting it a little close (we usually support dependencies going
back at least 3 years at a minimum, and often significantly longer). But
since this is an optional dependency anyway and simplifies the build,
I'm tempted to do so.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
The behavior of ImageBuf has always been that an IB referencing from a
file is backed by ImageCache unless you carefully use read() with the
"force" parameter set to true. The IC can be really helpful if you only
need *part* of the file, or if you have so many large IB's at once that
you want IC to manage the memory for you to keep the total reasonable.
Over the years, I've started to wonder if this was the right approach
for the default. It's much more typical that IBs aren't big enough or
numerous enough to gain much from the IC, and it's usually worse
performance to have it IC-backed. Plus, now the properties of the
ImageCache (size, behavior for nontiled images, etc.) can affect the
performane of ImageBuf without users understanding why.
So this patch tries on a different approach: ImageBuf's only use an
ImageCache underneath if explicitly passed a pointer to one in the IB
constructor or reset() call. Otherwise, it just uses local memory owned
by the IB. It's much simpler to reason about, and should be higher perf
for most common cases.
I did add a global OIIO option, "imagebuf:use_imagecache" that can
revert to the old behavior of having all IB's by default be backed by IC
when reading from a file.
Changed oiiotool to match: By default, IB's own their local memory.
Using the --cache option (which previously existed to set the size of
the cache) also enables use of the cache, so it's still possible to use
IC-backed IBs if the user needs it.
For the reader of this PR: What do you think? Is this a change you'd
like to see? Is it something we should try to squeeze into 2.5? Or are
we not sure enough about whether it's the right thing that we should not
be merging it close to a major release?
---------
Signed-off-by: Larry Gritz <lg@larrygritz.com>
I had previously prototyped an instance of this for the ImageOutput
chapter.
This patch makes empty placeholders for all of the other API chapters.
They don't contain any actual tests or generate any output now, but they
do build and run, so people can drop new examples into them without
having to worry about how to set up new files for each chapter.
---------
Signed-off-by: Larry Gritz <lg@larrygritz.com>
`oiiotool --mosaic` was always allocating a `float` buffer for the
result. Instead, make it use the "widest" data type of the input images.
So, for example, if you are making a mosaic of uint8 images, the result
buffer should be uint8, not have to allocate 4x bigger nor do
uint8->float conversions as it pastes the images.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
This is Update 5, the original PR is ongoing and we will continue to
update as additional people relicense.
Per PR #3905 (https://github.com/OpenImageIO/oiio/pull/3905), update the
RELICENSING document with the names of people and companies who have
signed on to the relicensing thus far, and update the SPDX license
notices on source files whose extant authorship (per 'git blame')
comprises only authors who have relicensed.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
This patch adds a new test to the CI matrix: verifying that we do not
break the ABI during releases.
Basically, in addition to building the current checkout, it also builds
a checkout of the designated "ABI standard" (generally, the first
non-beta release of a major version), then generates ABI dumps of both
versions and runs the ABI compliance checker using the tools from
https://github.com/lvc
Making two builds sounds expensive, but both builds are rigged to be
minimal, since only the main two libraries are needed for ABI checks (we
can turn off the binary tools, unit tests, python bindings, and support
for many formats), and of course the test suite need not run. So
actually, it's less expensive than most of the test matrix entries, and
it helps ensure that we don't accidentally break ABI when we don't mean
to.
I'm not sure how many false positives we'll have. We might need to
adjust the methodology slightly as we gain experience with this.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
When writing out EXIF headers in PNG files, the `tiff_version` field
wasn't handling endianness properly.
This lead to some tools like `exiv2` refusing to process the file at
all[1]
[1] https://github.com/Exiv2/exiv2/issues/2745
---------
Signed-off-by: Jesse Yurkovich <jesse.y@gmail.com>
New theory of code examples in the docs: every one should live in the
testsuite somewhere, and merely be included by reference in the docs.
This ensures that every code example works, stays current with the
evolution of the APIs, and never breaks. It also beefs up our test cases
for more thorough code overage in the testsuite.
I've done this with just two examples here, but hopefully it serves as
an example for others to -- bit by bit -- convert all the other code
examples in the docs into tests in a similar manner.
Doing this for any one example is a perfect "good first issue," since
it's bite sized and can easily be done in a day, it doesn't require any
knowledge of deep OIIO internals, and in the process it also teaches you
something about how both the docs and the testsuite are set up.
A few notes:
* I'm structuring it (for now?) as two testsuite entry for each chapter
of the documentation (one for C++ and one for Python).
* Note how I can have many code snippets in a test source file, and use
the Sphinx `:start-after:` and `:end-before:` modifiers of
`literalinclude` to clip particular subsections of the file, using
special comment markers in the code.
While I was there, I also fixed some typos I noticed in the text of the
chapter, oops.
---------
Signed-off-by: Larry Gritz <lg@larrygritz.com>
To my eyes, these two constructors look like they should be treated as
doing exactly the same thing:
MyType()
: mymember(...)
{ }
and
MyType()
{
mymember = ...;
}
But nvcc in Cuda mode says no, if the constructor is constexpr, the
former is okey dokey, but latter gives a warning. Fine, Cuda, whatver
you say.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
While testing out the 2.5 beta for use in Blender, I hit an odd build
error[1] apparently related to include order. Apparently `windows.h`
should be included before `shellapi.h`[2]. Swapping the order does fix
things, however, it turns out neither is used in this particular file so
just remove them both. Unsure why we didn't see it until now.
[1]
```
20>[9/146] Building CXX object src\libutil\CMakeFiles\OpenImageIO_Util.dir\strutil.cpp.obj
20>FAILED: src/libutil/CMakeFiles/OpenImageIO_Util.dir/strutil.cpp.obj
20>cl : Command line warning D9002: ignoring unknown option '/arch:sse2'
20>C:\Program Files (x86)\Windows Kits\10\Include\10.0.22621.0\um\shellapi.h(68): error C2146: syntax error: missing ';' before identifier 'DECLSPEC_IMPORT'
20>C:\Program Files (x86)\Windows Kits\10\Include\10.0.22621.0\um\shellapi.h(79): error C2065: 'HDROP': undeclared identifier
20>C:\Program Files (x86)\Windows Kits\10\Include\10.0.22621.0\um\shellapi.h(82): error C2086: 'int EXTERN_C': redefinition
20>C:\Program Files (x86)\Windows Kits\10\Include\10.0.22621.0\um\shellapi.h(68): note: see declaration of 'EXTERN_C'
```
[2]
https://stackoverflow.com/questions/2953704/vs2010-lots-of-errors-when-including-standard-librarieshttps://social.msdn.microsoft.com/forums/vstudio/en-us/ed93ae48-969a-4280-8fd5-ef57a332022b/problems-with-shellexecute-and-shellapih?forum=vcgeneral
## Checklist:
<!-- Put an 'x' in the boxes as you complete the checklist items -->
- [X] I have read the [contribution
guidelines](https://github.com/OpenImageIO/oiio/blob/master/CONTRIBUTING.md).
- [ ] I have updated the documentation, if applicable.
- [ ] I have ensured that the change is tested somewhere in the
testsuite
(adding new test cases if necessary).
- [ ] If I added or modified a C++ API call, I have also amended the
corresponding Python bindings (and if altering ImageBufAlgo functions,
also
exposed the new functionality as oiiotool options).
- [X] My code follows the prevailing code style of this project. If I
haven't
already run clang-format before submitting, I definitely will look at
the CI
test that runs clang-format and fix anything that it highlights as being
nonconforming.
Signed-off-by: Jesse Yurkovich <jesse.y@gmail.com>
Typo introduced in #3894 broke our ability to build against libheif <
1.13. A conditional compilation that should have kicked in for >=1.16
was mistakenly written as 1.6.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
This is Update 4, the original PR is ongoing and we will continue to
update as additional people relicense.
Per PR #3905 (https://github.com/OpenImageIO/oiio/pull/3905), update the
RELICENSING document with the names of people and companies who have
signed on to the relicensing thus far, and update the SPDX license
notices on source files whose extant authorship (per 'git blame')
comprises only authors who have relicensed.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Proposal from Thiago to add Jesse Yurkovich, representing Blender, to
the TSC.
Vote passed by majority of TSC members voting yes on the PR itself.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Fixes#3950
Fix the div-by-0 when handling unassociated alpha during a DXT4 DDS
load. The code mirrors what other formats do by first checking to see if
the alpha value is not 0 before continuing.
Added the 218-byte file from the bug to the local test suite.
---------
Signed-off-by: Jesse Yurkovich <jesse.y@gmail.com>
Enable set of Boost_NO_BOOST_CMAKE through make using
MY_CMAKE_FLAGS=-DBoost_NO_BOOST_CMAKE:BOOL=OFF.
Signed-off-by: Mikael Sundell <mikael.sundell@gmail.com>
The incoming length of the RLE data could be 0. In this case we would
attempt to take the address of the 1st element of an empty vector.
Added a new 1004-byte file to the local test suite.
Fixes#3951
---------
Signed-off-by: Jesse Yurkovich <jesse.y@gmail.com>
Fixes#3952
Several interrelated fixes to guard against corrupted files:
* Extend the `limits:imagesize_MB` sanity check to Targa files to
address #3952 specifically, and check in that failing example file into
the testsuite. (N.B.: Only a couple readers use this, TIFF and now
Targa. it should be extended to the others. Mackerel!)
* Improve the error message used for the above, both for Targa and TIFF.
* For low-memory situations, perhaps the default of 32GB is too large.
Make the default be the minimum of 32GB and 1/2 the size of physical
memory on the machine. (An app that legit expects humongous files is
expected to raise the limit appropriately.)
* The generic "Read error: hit end of file" we sometimes issued from
`ImageInput::ioread()` was hard to discern which reader it came from, in
cases where the file extension was absent or misleading and a succession
of files were tried. By changing this error message to say which format
reader triggered the error, it might make these errors easier to track
down in the future. This also changed the reference output of a few test
involving truncated or corrupted files.
---------
Signed-off-by: Larry Gritz <lg@larrygritz.com>
On overloaded VMs, the timer_test is ureliable and fails spuriously too
often.
Two strategies in this patch, hope some combination does the trick:
1. Increase the absolute error allowed for the timing tests when doing
CI runs.
2. Add just a bit more relative error (via a new unit test macro,
OIIO_CHECK_EQUAL_THRESH_REL).
Hopefully this will cut down on CI failures for unit_timer test case.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Disable GCC warnings around mismatched new/delete for shared_texturesys
creation
Signed-off-by: Shootfast <markboo99@gmail.com>
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Co-authored-by: Larry Gritz <lg@larrygritz.com>
* Add string literal operators for ustring and ustringhash so you can
write: ``` "foo"_us "bar"_ush ``` Note that on Cuda (but not on the
host), this ustringhash construction is constexpr.
* Add a new `ustringhash(const char*, size_t)` constructor, which is
also constexpr on the device side.
* Mark the existing `ustringhash(string_view)` constructor as constexpr
on the device side.
We can mark those things as constexpr on the device side because on the
Cuda side, the implementation only takes the hash (which is constexpr),
and does not actually add a ustring to the table (which cannot be
constexpr). To make such declarations easy, add a new macro to
platform.h, `OIIO_DEVICE_CONSTEXPR` which is constexpr for the device,
merely inline for the host.
---------
Signed-off-by: Larry Gritz <lg@larrygritz.com>
The most important fix is to remove links to the old CLAs and update
references to the mail list to the new one.
Also spruced up some language while I was there, hopefully to coax out
improved PRs and Issues.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
When building the code with Xcode ("cmake -G Xcode"), there are 1000+
warnings about implicit 64 to 32 bit conversion.
This change adds a compile option to suppress such warnings when
generating an Xcode project.
Signed-off-by: Anton Dukhovnikov <antond@wetafx.co.nz>
This is Update 3, the original PR is ongoing and we will continue to
update as additional people relicense.
Per PR #3905 (https://github.com/OpenImageIO/oiio/pull/3905), update the
RELICENSING document with the names of people and companies who have
signed on to the relicensing thus far, and update the SPDX license
notices on source files whose extant authorship (per 'git blame')
comprises only authors who have relicensed.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Text rendering doesn't work with utf-8...
I can't render chinese, arabic... or special chars as éèûîà...
It seems that the string function to convert utf-8 to unicode is quiet
simple.
Fixes#3933
[Note]: OpenImageIO is used in the [gpx2video
project](https://github.com/progweb/gpx2video).
---------
Signed-off-by: Nicolas VIVIEN <nicolas@vivien.fr>
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Co-authored-by: Larry Gritz <lg@larrygritz.com>
When an incoming 1-dimensional, buffer-like, object was used in Python,
the corresponding check inside OIIO would overflow if its length
exceeded a signed 32bit value.
An error similar to `Pixel data array error: Python array shape is
[2304000000] but expecting h=24000, w=24000, ch=4` would be raised.
I modified a few similar calculations and tried to follow the
predominant pattern already used.
Tested using the following snippet which would previously fail:
```
import OpenImageIO as oiio
import numpy as np
filename = "t:/foo.png"
xres = 24000
yres = 24000
channels = 4 # RGB
pixels = np.zeros(xres * yres * channels, dtype=np.uint8)
spec = oiio.ImageSpec(xres, yres, channels, "uint8")
image = oiio.ImageOutput.create(filename)
image.open(filename, spec)
if not image.write_image(pixels):
print("error = ", image.geterror())
image.close()
```
---------
Signed-off-by: Jesse Yurkovich <jesse.y@gmail.com>
Per PR #3905 (https://github.com/OpenImageIO/oiio/pull/3905), update the
RELICENSING document with the names of people and companies who have
signed on to the relicensing thus far, and update the SPDX license
notices on source files whose extant authorship (per 'git blame')
comprises only authors who have relicensed.
This is part 2, the original PR is ongoing and we will continue to
update as additional people relicense.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
---------
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Intel's servers have again updated the default icx that you get when you
install from their yum repository, and the new one seems incompatible
with our libstdc++ toolchain. So lock down on 2023.1.0 for now so that
our CI can pass.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
This updates ICOInput to use the IOProxy model so it can read from files
or the IOMemReader
Signed-off-by: Jason Baumeister <jason@jasonbaumeister.com>
During the CMYK to RGB conversion in PSDInput, the alpha channel was
never copied to the RGBA output. Converting a PSD to JPEG worked because
it ignored all the 0 alpha values, but converting to WebP would result
in a fully transparent image.
I added a cmyk-with-alpha.psd test to the psd-colormodes test to check
this
Signed-off-by: Jason Baumeister <jason@jasonbaumeister.com>
Clean up the `#if` tests that disable Intel SIMD when not on Intel
architectures, which incorrectly disabled ARM NEON when on ARM.
Fixes#3909
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Update the ImageInput documentation to not use the deprecated
`read_image` APIs.
Based on the discussion in #3902
---------
Signed-off-by: Jesse Yurkovich <jesse.y@gmail.com>
Per PR #3905 (https://github.com/OpenImageIO/oiio/pull/3905), update the
RELICENSING document with the names of people and companies who have
signed on to the relicensing thus far, and update the SPDX license
notices on source files whose extant authorship (per 'git blame')
comprises only authors who have relicensed.
This is part 1, the original PR is ongoing and we will continue to
update as additional people relicense.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Our own FindWebP.cmake was setting up targets that didn't
match the capitalization of the cmake configs that come with WebP itself.
Signed-off-by: Benjamin Buch <benjamin.buch@technoteam.de>
This batch is for files that LG (relicensed!) has contributed, in
addition to >2 other authors. Also add appropriate license notices to
some that had no posted license before due to oversight.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
* Add copyright/license notices to some mixed-authorship files that
lacked them before (all related to tests)
* Get rid of the pointless year in many copyright notices (change to the
current LF recommended wording)
---------
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Since LG has re-licensed all his old code under Apache-2.0, we can
change the license notice from BSD to Apache-2.0 for all files in which
LG is the only extant author. Also change all files that LG co-authored
with other people to specify "BSD-3-Clause and Apache-2.0".
Some files (mostly test scripts) had no license notice, so in those
cases, we added one.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
By majority vote of the TSC, we are changing OpenImageIO's main
incoming/outgoing open source license from BSD-3-Clause to Apache-2.0.
New code submitted from July 1, 2023 will be contributed under the
Apache 2.0 license.
Old code submitted prior to July 1 2023 is under the original BSD
3-clause license, unless relicensed per the process described in the new
RELICENSING.md file. We hope to get the vast majority of the code base
relicensed quickly and we will document which set of files contain a
mixture of licenses.
Instructions to everybody reading this:
* If you are contributing code under the authorization of an employer,
please let them know we are changing to Apache 2.0 and make sure that
you are authorized to make all future contributions to under that
license.
* If you made prior contributions to OpenImageIO over the years and are
willing to relicense your code under the Apache 2.0 license, please
submit a PR adding your name to the list in RELICENSING.md. If you were
working for a company that might own your IP output at the time you made
those contributions, please also ask somebody there (who has the
authority to do so) to submit a PR adding the company name as well,
indicating that they are re-submitting all of the old contributions
under the new license.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
By unanimous vote of the OpenImageIO TSC, as of July 1, 2023, we
adopt the technical charter, Code of Conduct, new CLAs, and GOVERNANCE
document.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
The underlying farmhash algorithm (which is the basis for
Strutil::strhash, and thus ustring's hash representation) turned out to
not be properly constexpr under certain circumstances and needed to be
expressed a different way to be fully constexpr on all platforms and for
all strings.
New implementation suggested by Alex Wells.
Also, make sure string_view ctr from char* can be constexpr, even for
C++14. This is related for the sake of the strhash calls that, when
taking string literals, actually choose the variety that accepts a
string_view.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
* Remove leftover logic from when we used TravisCI.
* Pick a default for LLVM_DISTRO_NAME so it works when the script is run
standalone when not on GitHub Actions CI. (But it's still really only
meant to run on x86 Ubuntu, and not designed for anything useful outside
of our CI.)
* Eliminate all the complexity with old LLVM versions -- in practice,
our CI only uses this to download fairly modern clang versions.
Fixes#3891
Signed-off-by: Larry Gritz <lg@larrygritz.com>
The strukturag repo for libheif recently upgraded the version it was
serving from 1.14 to 1.16, and that caused our tests to fail because (a)
some of the codec support has been moved to plugins that are loaded at
runtime and those are now distributed in packages we need to install
separately; (b) you must call heif_init() or it won't know about the
optional plugins.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
If colorconvert, ociolook, ociodisplay, ociofiletransform fail
because OCIO support is not enabled, make sure the error message
makes that clear.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Make sure we can input exif tags as strings containing numbers when
number are required.
E.G. if my input is "Orientation"=>"8", i would like the jpeg output to
contains the value 8 for the exif tag orientation.
PR #2147, addressing issue #1844, is related to an apparent OpenEXR bug
wherein single channel tiled images where the tile dimensions were not a
power of 2 and >= 16 would not work properly with dwa compression. We
work around the problem by automatically substituting zip compression in
those cases. But it was implemented with a subtle bug, forgetting that
tile_size == 0 for untiled images, and thus we were silently switching
from dwa to zip compression for all *untiled* single channel images of
any size (which presumably were never problematic). This patch fixes the
test so we are not changing the compression for the untiled files.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
J2K output was defaulting to UINT8 when asked to output an unsupported
type. It should have been UINT16 to try to minimize the amount of
precision loss.
Fixes#3876
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Everybody please go to https://lists.aswf.io/g/oiio-dev and sign up for
the new mail list as soon as possible. The old list is on its last legs
and having trouble, let's get this done.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
When reading a PSD file, if it contains a thumbnail, we read it from the
in-memory blob with a combination of an IOMemReader and an ImageBuf.
But... we always named it "thumbnail.jpg" without considering that
multiple simultaneous PSD files reading identically named images (but
which are in fact different) could cause a weird kind of clashing in any
underlying use of ImageCache!
The simple fix is to use ordinary ImageInput approach to reading the
thumbnail from the IOMemReader, but just store it in the ImageBuf.
Fixes#3824
Signed-off-by: Larry Gritz <lg@larrygritz.com>
A variety of minor documentation fixups:
- Use Python 3 style 'print' statements consistently everywhere (some
snippets had a mix of both)
- Correct the usage of `ImageSpec::channels` that should be
`ImageSpec::nchannels`
- Template syntax errors in two C++ snippets
- Document the new default for `max_memory_MB`
Various improvements to OpenCV support:
* IBA::from_OpenCV and to_OpenCV now set error messages for every type
of failure they encounter, instead of returning false with no
explanation. For from_OpenCV, the error message can be retrieved from
the ImageBuf returned; for to_OpenCV, the error message can be retrieved
from the global OIIO::geterror(). Several assertions were removed,
instead relying on error return codes and messages.
* Add global OIIO attribute "opencv_version" which allows you to find
out what version of OpenCV OIIO was built against, which was previously
impossible to know without seeing the build-time status messages. This
can be retrieved via
OIIO::get_int_attribute("opencv_version")
and also you can find out from the command line with
oiiotool --echo "{getattribute(opencv_version)}"
The return value is an integer encoded as `10000*major + 100*minor +
patch`, and it will return 0 if no OpenCV support was enabled when
OpenImageIO was built.
* For OpenCV >= 4.0, to_OpenCV can now correctly construct a 'half'
matrix. For OpenCV 3.x, we turn it into a float matrix as before, since
CV_16F was only supported starting in 4.0.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Just a routine update of these files as I proofread them in light of the
move to ASWF. This patch is addressing the "old" project, there will be
further ASWF-specific edits for changes that will take effect after the
project is fully moved.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
In `jpegoutput.cpp` when computing the density fields the relationship
between `XRes`, `YRes` and `aspect` is `aspect = XRes / YRes`.
However this relationship was not respected in one specific case, when
`aspect >= 1.0` and `XRes` and `YRes` are not set.
This PR solves this problem by enforcing the correct relationship.
Related to: https://github.com/OpenImageIO/oiio/issues/2822
The Intel yum repo renamed this package a couple weeks ago, and it broke
our CI. Update to the new name.
Adjust fmt use to solve icc warning.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
When reading a raw image, setting the `user_flip` to 1 meant ignoring
the `flip` value (to avoid buffer rotation from LibRaw).
However the value 1 for the `user_flip` is **not neutral**: values lie
between 0 and 7 (see
https://www.libraw.org/docs/API-datastruct-eng.html#libraw_image_sizes_t),
and 1 corresponds to a horizontal mirror (in my understanding of the
source code).
The proposed fix is to use 0 as `user_flip` value for ignoring the
`flip` value.
This modification invalidates the changes made in this PR:
https://github.com/OpenImageIO/oiio/pull/3847.
Therefore, flip to orientation conversion has been adapted to match
exactly LibRaw's documentation, which handles only rotations (+90, +180,
-90 degrees).
The previous PR was only tested on images of landscape and human faces,
hence the mirroring issue remained unnoticed.
When reading images containing text however the problem is clearly
visible.
This PR fixes this mirroring issue on the following 4 orientations: 0,
+90, +180 and -90 degrees rotation.
Other orientations could not be tested as they don't seem to ever appear
on raw images datasets.
When reading a raw image while having a `user_flip` set to 1, we set the
orientation Exif tag to LibRaw's flip value (so that LibRaw doesn't
rotate and/or mirror the buffer but the image still gets displayed
correctly).
However LibRaw's convention for flip values differ from Exif orientation
values: https://www.libraw.org/comment/5176#comment-5176.
In this PR we fix this issue by converting LibRaw flip values to Exif
orientation values. This conversion is based on:
- https://www.libraw.org/comment/5178#comment-5178 (conversion proposed
by LibRaw's maintainer)
-
https://github.com/LibRaw/LibRaw/blob/6fffd414bfda63dfef2276ae07f7ca36660b8888/src/write/file_write.cpp#L22
(LibRaw's function for applying the flip).
Tested on:
- CR2 images with flip values 1, 5 and 6
- ARW images with flip values 1 and 5
(unfortunately I could not find any image covering other flip values).
Since the conversion is `[1, 2, 3, 4, 5, 6, 7, 8] -> [1, 2, 4, 3, 5, 8,
6, 7]`, the only test that came out with different results before and
after the change is the one with flip value 6:
in that case, the image orientation got corrected by the fix.
Fixes#3837
oiiotool already attempted to trim extra channels when outputting to a
format that can't support it. It correctly handled the two common cases:
* More than 4 channels for formats that don't have
`supports("nchannels")` will drop after the first 4 channels.
* More than 3 channels for formats that don't have `supports("alpha")`
will drop after the first 3 channels.
But this omits an interesting (but rare) corner case: a 4 channel file
where the last channel is NOT alpha, and the output file supports an
alpha channel but not arbitrary channels. A concrete example might be an
input file that is exr with channels R, G, B, Z, writing to a PNG file
(which can support RGB or RGBA). We wouldn't want to write what appears
to be an RGBA file if that last channel isn't really alpha.
Also, noticed that oiiotool --chnames didn't properly clear the
alpha_channel and z_channel flags, which is important if the new channel
lists drop one of those channels.
`make_texture()` already took a configuration option "maketx:forcefloat"
(defaults to 1), which if set to 0 disables the conversion to a float
buffer for intermediate computation.
For `oiiotool -otex`, there was no way to specify this. This patch
allows it to be disabled with the new optional modifier
`-otex:forcefloat=0`.
Also, in make_texture, this option only controlled the initial
conversion of the whole buffer to float, but the downsized levels all
were float either way. This patch now uses the forcefloat config option
to also control the buffers used for downsizing.
Disabling forcefloat like this has a speed penalty (the downsizing math
on float buffers is faster than going in and out of float for every
indvidual value) and loses some precision in the process of repeatedly
downsizing to generate the MIP levels. But it can sure save a lot of
memory if you have a ginormous uint8 image and want to avod the 4x
balooning of memory. I don't recommend disabling forcefloat except for
images that can't fit into memory for the texture creation without it.
But when this is the right hammer to use, it can be the difference
between being able to create the texture, or not.
Some other improvements:
* `oiiotool -otex` wasn't outputting ANY status output during texture
output as maketx does, and for that matter, -v and -debug were not being
influencing the behavior of make_texture at all despite it having such
support. So fix to pass down the right flags. Now there are proper
verbose status and debug messages when requested.
* Eliminate useless call to is_monochrome by calling it only when the
previously-computed pixel stats indicate that the average value of all
the channels is the same (if they don't have the same average, there's
no way the pixel-by-pixel values will be the same in all channels for
all pixels). This saves an extra traversal of all the pixel memory
during texture creation.
* Use synchronized/flushed status messages so users can see them as they
are issued for very long maketx tasks. This also included some
conversion from old iostream stuff to fmt-based print.
* For verbose/debug, more fine-grained timing for make_texture, in
particular, print the time spent downsizing and writing each MIP level.
I'm honestly not sure why some GHA CI jobs suddenly started warning
about this in the past day or so. Something must have updated on their
end. But switching to the safer Strutil equivalents fixes it.
`ssize_t` is a posix type, msvc seemingly doesn't know about leading to
a build error with MSVC 2019
usage elsewhere in this files used the `py::ssize_t` type, which seems
to fix this
fmt 10.0 was recently released, and it breaks a lot for us because they
have fully removed legacy support for automatically being able to format
anything that has an iostream `<<` operator. This facility was deprecatd
in 8.1 but could be re-enabled with FMT_DEPRECATED_OSTREAM, but even
that was removed in 10.0.
So this means that we had to write several explicit format handlers for
some types for use with format() and print().
Also, they are even more strict with sprintf/printf which don't honor
the custom formatters as far as I can tell, so in several cases I also
switched those to the std::format/print style.
Fixes#3835
The PSD reader was written 12 years ago by a GSOC student and has
changed little since. The following is not a knock on that student, who
clearly did a good job if his code has survived mostly unomdified for
over a decade. In chasing down problems with handling extremely large
psd files, I've noticed some oddities and inefficiencies that need
fixing. It's not an emergency, so I'll handle it in pieces.
The item addressed by this patch is memory usage and thread concurrency.
It turns out that the reader allocates memory for the scanline reads as
it goes... and basically keeps it in the ImageInput until it's
destroyed, even though it will never actually be used again. I guess
it's just trying to save the next allocation if it reads the same
scanline again or on the next subimage? But this means that if you read
a really big psd image, it's basically doubling the memory use -- it
reads one copy into the user's buffer, but also holds on to this extra
copy for no reason. Also, a bunch of the decoding helper functions
implicitly use this memory, which means we lock a mutex for the duration
of read_native_scanline (which in theory should not be necessary since
all I/O is happening through thread-safe IOProxy calls).
So this patch does the following:
1. Remove both this retained scanline memory as well as some other
scratch memory from the ImageInput, and make it allocated and freed
local to read_native_scanline.
2. Change all the helper methods that used it implicitly so that they
are passed buffers to use. In many cases these have been changed to
`const` methods and in a few cases to `static`, making it much easier to
assure ourselves that they don't modify (or sometimes use at all)
anything in the PSDInput to do their task.
3. The result of these changes means that read_native_scanline doen't
need to modify any of the data methods of the class (though it does use,
without modifcation, some things set up in the course of open). Because
it's now non-mutable, we can also remove the mutex, now allowing
multiple threads to read scanlines from the same file simultaneously
without blocking each other.
4. Some other miscellaneous cleanup, including that some scratch memory
was allocated as std::string (!), whereas it should really be a
`vector<unsigned char>` or `unique_ptr<unsigned char>`, and also
changing some computations from double to float.
There is still some more work to do after this is merged. In particular,
I think now that scanline read is stateless / non-mutable with respect
to the PSDInput object, this is a candidate for adding a multi-scanline
read_native_scanlines method (replacing the default implementation of
looping and calling read_native_scanline serially for each scanline),
which should also be able to read multiple scanlines with some overhead
and allocations done once per block, and with blocks of scanlines
running in parallel using the thread pool. This should be a huge
efficiency gain when reading psd files. But it will wait until the next
patch.
The latest version of libfmt began using the `__float128` type but this
is [not supported in the NVPTX backend for
LLVM](https://discourse.llvm.org/t/compiling-cuda-code-fails/61240/13).
A way to detect this particular platform seems to be to check both
`__CUDA_ARCH__` and `__clang__`.
I am not disabling it entirely on any CUDA target because NVCC 10+
understands `_float128` and generates instructions for it when tested on
godbolt.
Following up from https://github.com/OpenImageIO/oiio/issues/3809.
Co-authored-by: Edoardo Dominici <edoardod@sy1wse0003.al.com.au>
Windows isn't smart about symbolic links. There were a few places in the
testsuite where we used symlinks to essentially de-duplicate test
reference images that were expected to be identical. But then the
resulting archive you get when you ask GitHub for a zip file of the
whole repo (which some people download to get OIIO source rather than
git clone) can't be unzipped correctly on Windows (at least not by
cmake's unzip command).
OK, so just change those links to full copies.
Fixes#3811
The `openexr-images` repo was never updated after the default branch was
renamed to `main`. This will fix the following silent errors in CI:
```
Cloning into '/__w/oiio/oiio/build/testsuite/openexr-images'...
warning: Could not find remote branch master to clone.
fatal: Remote branch master not found in upstream origin
```
Additionally, the cmake attempted to symbolically link local repos like
`oiio-images` if they were found. Unfortunately this type of link will
fail unless running as admin on Windows (which is rare) and the
COPY_ON_ERROR directive did _not_ work here (cmake 3.24; weird;
sigh...). The output will now look something like the following:
```
-- Linking oiio-images from C:/Users/jesse/source/oiio-images
-- Link result failed to create symbolic link 'C:/Users/jesse/source/oiio/build/testsuite/oiio-images': A required privilege is not held by the client.
-- Copying oiio-images from C:/Users/jesse/source/oiio-images
```
When decoding RLE data, some arguments to decompress_packbits() were
inexplicably declared uint16_t (they were passed uint32_t arguments, and
I honestly don't know why we didn't get a warning all along). This
prevented us from correctly reading PSD images whose width was more than
64k, which I suppose are rare enough that it's not hard to understand
why this went unreported for so long.
But anyway, just changing those parameters to uint32_t does the trick.
I ran into the problem described in #2929 and did a little digging to
see if I could find the source of the bad color conversions. I tracked
it down to `fast_exp2()`
https://github.com/OpenImageIO/oiio/blob/a77e36250fadfb4c391688eb1df4b3dd88d5e367/src/include/OpenImageIO/fmath.h#L2050-L2080
and in particular the return statement on L2071. In the process of
trying to figure out what part of that return statement was going awry,
I found that separating the bitcasts to separate variables as in this
PR, the problem went away. All testing was done with the `color_test`
added in #3023.
It also succeeded when adding various `std::cout` lines to check what
the values of each computation were, so it does sound like a compiler
optimization that does or does not get triggered is the problem.
Unfortunately I don't know why it's not failing on the CI builds, I
double checked to make sure I'm using the same settings and flags. I
also tested on two different machines and get the same failures, and
success with this patch, on both.
One strange wrinkle is that even without this patch, color conversions
worked fine when using AVX2 for the SIMD mode.
Summary :
Previous results (without this patch)
- USE_SIMD=0 - PASS
- USE_SIMD=SSE4.2 - FAIL
- USE_SIMD=AVX - FAIL
- USE_SIMD=AVX2 - PASS
With this patch
- USE_SIMD=0 - PASS
- USE_SIMD=SSE4.2 - PASS
- USE_SIMD=AVX - PASS
- USE_SIMD=AVX2 - PASS
Visual Studio 2019 versions 16.11.32228.343, 16.11.33328.57 and
16.11.33423.256(same as current Github build runners) in RelWithDebInfo
mode.
For cases of calls like `ImageOutput.write_image(array)`, this improves
the error messages when the array dimensions don't match the size of the
image we're writing, in two ways:
1. For the case of 2D images where the dims don't match, print what the
array shape actually is and what was expected. That's more helpful than
just saying "can't figure out array shape" and printing the number of
dimensions (but not the shape).
2. A mistake caused a number of error messages to be substituted by a
cryptic "can't handle numpy array with noncontiguous channels", but that
was only supposed to be for one particular case; fix that.
Currently, if you run oiiotool with --bumpslopes and --cdf at the same time (or -obump:cdf=1), the CDF is only produced for the first four (of six) channels. The techniques described in Burley (2019) actually do apply reasonably to the squared derivative channels, so it's useful to allow all six channels to have CDFs here.
maketx and oiiotool-maketx tests updated with new relevant tests.
* Tests for maketx -u, --attrib, --sattrib, --cdf
* maketx was documeted to take --cdf as a simple flag, but was
incorrectly implemented as requiring a (pointless) nonzero argument.
Fix it and hope nobody was depending on the undocumented behavior.
* maketx -v arg never had any effect. Add it back.
* Don't use deprecated ubuntu-18.04 runner. Switch to running on
ubuntu-20.04, but use docker containers that supply ubundu-18.04.
* Bump 'latest' version of openexr, webp, pugixml.
There turns out to be a way to get wedged into a mutual deadlock on
the file handle cache, with how get_texture_info on a udim pattern
checks to see if the attribute requested is present and equally valued
in all constituent files of the udim set. This hits the handle set
pretty hard, and if multiple threads are hammering the same sets and
rapidly cycling textures out of the cache, there can be trouble, it
seems.
Remedies in this patch:
* When it's trying to close files because the number of handles is too
high, we already had it so that if we only exceeded the max_files
limit by a little bit, we would simply try to get the lock, and
abandon the attempt if somebody else is holding it, but if we are
quite a bit over, then stay and wait for the lock to be released.
In this patch, we make the that default behavior -- if we are over
the limit and need to free some handles, simply try to get the lock,
and if somebody else is holding it, leave it all up to them and
return immediately. The pros of this are (a) snappier response by
never waiting at this point, and (b) removing the possibility of a
deadlock, because if you can't acquire the lock, you just move
on. But the con is that lots of threads adding new files may run
faster than the one thread who has the file and is trying to release
files, so it's possible that the number of handles will exceed the
limit by a wider margin now, but hopefully only momentarily. (But
note that this can be overridden with a new ImageCache attribute
"max_open_files_strict", which if set to nonzero revert to the old
behavior. At least for now, in case I am neglecting to understand
some bad consequence of this change.)
* The recursive_mutex controlling access to the ImageInput in each
tile cache record is changed to a recursive_timed_mutex, and when
closing a handle by calling release(), instead of an unconditional
lock, try acquiring a lock for 100ms. If it fails to do so, it may
be in a possible deadlock, so just give up and move on. The only
consequence is that maybe that handle isn't freed after all. It will
then move on to the next one, not the end of the world, but at least
it can't get stuck there.
* In find_file, eliminate the call to check_max_files -- it's not
needed here, since it's also called by open(), which is the only
time that new file handles might be opened. We just don't also need
it again simply because we're adding a file to the cache.
Setting the cmake option OIIO_DISABLE_BOOST_STACKTRACE to ON disables the use of
Boost stacktrace at compile time. This avoids dependencies on helper libraries
like dbgeng.dll.
* test: Add a corrupted test file
* fix(IC): upon tile read error, mark the file as broken
The idea is to keep fruitlessly reading from a corrupted file.
* fix: race condition in TIFF reader
TIFFInput::read_native_tiles had a very subtle race condition. There
is a section where it has a task_set of decompression tasks that are
sent to the thread queue. When the method ends, the task_set goes out
of scope, and its destructor will wait for all the remaining tasks to
finish. All fine, right? Except that other variables in the function
include unique_ptr's with allocated memory that the tasks need. So
what can happen is that if the compiler generates the code such that
the destructor of the unique_ptr happens before the destructor of the
task_set, the memory that the unique_ptr held may be deallocated
before some of the tasks still running in threads have finished.
In practice, we seem only to hit this race condition for particular
cases involving corrupted files, because the early `return` seems
quite likely to run those destructors before all the threads are done,
but it was possible to encounter it for valid files as well, if the
timing is just right. Shocking that we never had it reported.
The solution is just to have an explitit tasks.wait(), which will
block until all the tasks have finished before hitting the actual end
of the method and letting the other destructors to run. And for the
"early return" case, change it to a break so that we go through the
wait call at the end.
I believe that this fix addresses TALOS-2023-1709 / CVE-2023-24472.
There are two ways to fail the FITS magic number check: fail to read
the bytes, or the bytes don't match the magic number. It seemed
suspicious that we call close() for the latter but not the
former. Combine them into a single error response (they had the same
message, anyway).
Also, make sure FitsInput::init() fully clears some other data
structures. Oversight?
I don't know if any of these things are real bugs, but they look
sloppy and potentially problematic, so fixing them up.
* Make lookups of const unordered_maps thread-safe
* added back deleted end brace
* clang-format
---------
Co-authored-by: Larry Gritz <lg@larrygritz.com>
* Be more careful constructing strings from what we read in the header;
corrupted files may make them not be zero-terminated.
Fixes TALOS-2023-1707 / CVE-2023-24473
* Watch out for pixels with palette indices that exceed the size we
allocated for the palette.
Fixes TALOS-2023-1708 / CVE-2023-22845
* Check alpha_type for valid range.
Refactoring in #1641 incorrectly caused this to be set on the last layer
rather than the composite image as before. Now set it on the composite
image and every layer, as all of them use unassociated alpha.
* Better Typedesc testing of 'tostring' with fmt style formatting
commands, and fix a bug that uncovered.
* Refine 'make CODECOV=1' behavior
* Beef up python-imagecache test to test getpixels, more attribute,
invalidate.
* Better testing of python IB construction from numpy array.
* testtex: Use correct guard to avoid compiling unused code for
coverage analysis testtex: get rid of unused 3d warp code.
* Add texture-threadtimes test
* Eliminate redundant code in exroutput.c by implementing
write_scanline using write_scanlines.
* Add Exif write support to PNG output.
Stores any Exif data from the given ImageSpec into the PNG eXIf chunk.
* add test for writing Exif data in png
* add test reference output for libpng versions not supporting the eXIf chunk.
* Update testsuite/png/ref/out-libpng15.txt
Co-authored-by: Joris Nijs <joris.nijs@esko.com>
Sets color space metadata. While conceptually, the main job is to call
`attribute("oiio:ColorSpace",name)`, this is a central spot to add
logic that adjusts or removes other contradictory metadata.
The new 2023 one on the Intel site seems to require a glibc too new to
be compatible with the one on the ASWF CentOS-7 based containers, so
has been breaking our CI for a couple weeks.
We never had a test for `maketx --lightprobe`, so add it to
testsuite/maketx.
Immediately discovered that it only worked correctly for float images,
fails for half or other types. Fix.
* More extensive testing of texture3d: handle vs filename, subimages,
>4 channels, closest sample interpolation.
* Fixes for tex3d > 4 chan that turned out to be broken.
* Some rearrangement for the >4 channel case
* Refactor and simplify accum3d_sample_bilinear so multiple data types
all use the same templated code path.
* This all reduces the amount of untested code in OIIO by another 0.4%.
Would you believe we never had a testsuite test for environment
mapping? Well, it wasn't totally untested, in the sense that OSL
testsuite had one, and obviously it's used in production all the
time. But OIIO's testsuite didn't have any tests of
TextureSystem::environmemt(). We now remedy that.
* Add testsuite/texture-env, contains several environment tests,
including handles vs filenames, subimage selection, different interp
and mip modes, ask for >4 channels.
* New CMake build option OIIO_TEX_IMPLEMENT_VARYINGREF can be used for
testing to disable implementation of the old 1.x style of batch
texture calls. We don't use them, they are deprecated. This option
give us a way to exclude them from code analysis, because we really
don't care.
* Fix bug with >4 channels for environment batch shading!
* Avoid unnecessary spec lookup in TileID constructor.
* Avoid building testtex test_hash during code coverage -- it is an
extremely specialized test, not run in the testsuite, it shouldn't
really count againt good code coverage.
* testtex: better control over temp file creation
* Additional texture tests to improve coverage of udim generation,
and deadling with cache limits.
* Better ImageCache unit tests for perthread create/destroy and
individual tile retrieval. Fix a bug in get_tile for nonexistant
subimage or miplevel.
* Dead code removal: ImageCacheFile::udim_to_concrete() and
find_file_no_add(), which were no longer called anywhere.
* IC stats: fix messed up 'slowest' list ordering when there are broken files
* IC stats: when stats level is high, enable printing of some stats that
we previously skipped over when they were "uninteresting" (e.g., too
little time spent on something, no problematic files, etc).
* IC testing: test a variety of get_pixels error conditions
* IC testing: more tests of IC::imagespec() and get_imagespec(), incluing
error conditions.
* More get_texture_info query testing
* Minor bug fix for int-to-float data conversion in get_image_info
* Test IC query for resolution[3]
* Test IC::reset_stats()
* Fix ImageCache::reset_stats in cases where threads ended
* When checking code coverage, don't abort for failed tests
* Rejigger imagebufalgo_test a bit for better IBA test coverage
* Test zover
* Have testsuite/oiiotool-fixnan test with deep images
* Add to the extisting --fixnan tests a test for 'error' mode.
* [Excluded from backport to 2.4] Make testsuite/oiiotool-color test all colormap names
* Test 2D filters in filter_test
* Exclude python scripts in src/doc from coverage analysis
* Remove redundant attribute-seting code from the python internals
* Remove unused python_array_code function
* Exclude some imported code from coverage testing
* Re-enable and improve coverage testing from 'make CODECOV=1'
* Exclude unused/unexposed parts of xxhash.cpp from compilation
We didn't have thorough tests of pbm/pgm/ppm files, add some.
That immediately uncovered a problem with pbp files.
By spec, pbm (1 bit) files use 1 to mean black, 0 for white.
Confusing, because pgm (grey) and ppm (rgb) use 0 for black.
We had no testsuite exercise of --pdiff, add it.
Along the way, I saw tha we were getting the "you didn't have any
output" warning when using pdiff, so fix that.
And some other minor changes: formatted output update, some other
tightening up of things.
A little bit of jiggling the contents of XYZtoLAB_color to work around
what I think is a compiler error with icx -- it was generating NaNs
for reasons I was unable to understand or fix, but not if I rearranged
the code slightly. All the other compilers and versions had correct
behavior.
Seems like a reasonable assumption for BMP files. It's not strictly
correct -- there are header fields on V4 and V5 BMP files that
communicate color primaries and gamma, but honestly, I do not have any
idea if anybody uses them or whether they're more misleading than
helpful. For now, assuming that a BMP file is meant for output on
an sRGB display seems like it's likely to be true.
It should not have been set in platform.h (inside the cpuid function!)
in the first place. And apparently, though this was ok for Windows ARM,
on Mac ARM it's an error because that symbol is already defined.
So only redefine it if it's currently undefined. And move the whole
check and definition into simd.h where all the other ARM_NEON stuff
lives.
* Build: fmt external version flexibility
The current default behavior is to
1. Check if there's already a copy of fmt found.
2. If not (or if BUILD_FMT_FORCE is ON), download it locally.
3. Copy the relevant fmt .h files as part of the OIIO install in
OpenImageIO_ROOT/include/OpenImageIO/detail/fmt
This patch adds a new INTERNALIZE_FMT option, which defaults to ON,
whch is the old behavior. But if set to OFF at build time, the fmt
headers will NOT be copied into OIIO's install area (instead, it will
have dummy files that in turn include the real ones).
Set INTERNALIZE_FMT to OFF if you are really sure you want users of
OIIO to use the system install of fmt and not have OIIO install a new
copy.
When an externalized fmt is used, also make sure our exported cmake
config files cause the downstream user to find fmt and pass on the
dependency properly (necessary because strutil.h actually requires
properly including fmt headers).
Eliminate extraneous include_directories of fmt
Bump the auto-download version of fmtlib to 9.1
A few minor fixe to the term code, add a "term:filename" optional hint
to redirect output rather than go to stdout, and also change to the
new std::format notation for output.
Fixes#3721
Please read the comments in #3721. This is the "local" fix for the
build break due to the type mismatch. It is a band-aid. And it may be
the best solution for the 2.4 branch if we don't want to potentially
break ABIs by changing the definition of any public types.
Still pending is to examine the issue of whether it was a mistake to
define vbool4 storage for neon as uint32x4_t or if we should change it
to int32x4_t to better match the non-simd reference implementation.
After debating that (and identifying somebody with access to an
ARM-based machine to test the solution for us), we may return to
tackle this more fundamental change.
IOMemReader::pread catch out-of-range read positions.
Fixes#3711
Along the way, I noticed that the system pread & pwrite return a
ssize_t rather than a size_t as our IOProxy methods do, so I need to
catch the negative value returns that they use to indicate errors.
* There was lots of duplicated code between iinfo and `oiiotool -info`.
Refactor much of that into a new src/libOpenImageIO/printinfo.cpp.
These are internal functions, no change to public API.
* Test iinfo --hash, --stats, and with nonzero origin and full size
* Test iinfo and oiiotool --dumpdata of deep image
* Test iinfo for subimages, mips, -f, -s
* Some conversion to new print instead of printf
There is probably more such refactoring to do later, I did the easy
parts for now and that's a start.
Kai Pastor (dg0yt) made an important observation that the way I
recently made OpenImageIO_VERSION be a cache variable might lead to
unexpeted behavior by inadvertently retaining a version number upon
recompiles if the cache itself was not cleared, and this can happen
unexpectedly even when the user isn't trying to override the version.
This patch changs the behavior as follows:
You must use the special `-DOpenImageIO_VERSION_OVERRIDE=...` to
override the version.
* if you do so, you are playing with fire and get what you deserve
if you misuse it or change branches and don't remember to clear
the cache.
If you do a normal build (without _trying_ to override the version),
* you will get the real version number because it IS NOT cached
* if you switch branches, you'll always get the right version number
* ...but switching branches without clearing the cache can still, as
always, have other subtle behavior differences versus doing a
truly fresh build from scratch.
So in short, it still allows a way to override the version if you
really want to, but if you are not trying to do that, you can no
longer get yourself inadvertently into unexpected trouble.
When reading PSD file, if reading the thumbnail doesn't pass basic
sanity checks, we consider that a hard error reading the file. One
check that we do is to make sure that our computation of the byte
width of the uncompressed thumbnail (`bpp/8 * width`) matches what the
file says the `widthbytes` is.
BUT... it seems that it might round up to an even (or maybe 4-multiple?)
of columns, presumably for scanline alignment reasons. And so we were
considering whole PDF files unreadable if they fell into this case.
This PR primarily loosens the test to allow some padding up to a
multiple of 4.
Fixes#3658
* Maya IFF documentation says IFF file has max resolution of 8192, and
our implementation only allows writing RGB and RGBA, so check these
limits when an IFF file is opened for output.
* Make sure scratch space allocates enough for tile padding.
* Check for non-zero image origin coordinates (which are not allowed
in IFF files).
* Change some 16-bit loop variables to uint32_t to avoid possible
overflow.
Fixes TALOS-2022-1654, TALOS-2022-1655, TALOS-2022-1656
In open(), check for non-zero image offsets, which is not supported by
BMP, and issue an error if found. If the spec passed in has nonzero
spec.y, other code in the write functions would fail because they
assumed spec.y == 0 and could therefore access memory incorrectly.
More generally, also put checks in the write functions that issue
errors and take early outs if they find that they are called on a
BMPOutput that is not currently open, for example if open() fails but
the user doesn't check the return codes and proceeds to call the write
functions anyway.
This addresses TALOS-2022-1653 / CVE-2022-43594-CVE-2022-43595
There was a subtle error where we had a reference to an ImageSpec for
one of the subimages, confusingly called m_spec, which we altered, but
then didn't make the same change to the this->m_spec, and in a later
spot we referenced the `m_spec` meaning the one in `this`. This could
lead to a crash.
Fix, but not only that, also rename to avoid the confusion. `spec0` is the
alias we us for the spec for subimage 0, and `spec_s` is the alias we
use when referencing subimage S, and we really never use m_spec
anymore to avoid any confusion. This should make it easier to reason
about this code in the future and avoid these kinds of mistakes.
A second error resulted from the situation where a failure in open()
would return before one or both of m_buf or m_tilebuf were allocated.
If the return status of open() were ignored and the caller
subsequently attempted calls to write_scanline or write_tile, invalid
memory could be accessed. Fix this with a variety of guards not only
based on the allocation status of these buffers, but also issuing
errors (and exiting early) from write_scanline or write_tile if they
are called on an ImageInput that is not in fact open.
Addresses TALOS-2022-1651 / CVE-2022-43592 and
TALOS-2022-1652 / CVE-2022-43593
Fixes#3660
In investigating this issue, I also noticed that now that IOProxy
support can be relied on, the old code that in the psd reader that
directly fumbles with libjpeg is not needed and can be replaced by
just a couple of lines to make an ImageBuf read from an IOProxy.
And it fixes the bug, too. So I'm not even going to try to figure
out what was going wrong with the old code.
I uncovered a very subtle problem that can happen with the interaction
between ImageBuf, using an IOProxy, and the underlying ImageCache.
Remember that when using an IOProxy, it is critical that the lifetime
of the proxy outlasts any ImageInput, ImageBuf, or ImageCache that was
told about it.
Consider the following code:
IOProxy proxy(...); // temporary proxy in this local scope
ImageBuf A ("foo.exr", 0, 0, proxy);
We've set up an ImageBuf that will read from an IOProxy. Behind the
scenes, it made an ImageCache entry that remembers the IOProxy, as it
has to, since it needs to use that proxy for subsequent reads.
Now we read the image, and we use the `force=true` parameter to
force an immediate read of the file:
A.read (0, 0, true);
It reads from the proxy. But the IB and the underlying entry in the IC
still know about the proxy. But in the user's mind, they have fully
read the image, will never read from it again, so of course (they
think) it is safe to close or destroy the proxy, knowing that they
made it last past its use:
proxy.close();
But -- oops! -- the cache entry still has a pointer to the proxy, and
even though it won't need to read from it again, it still will access
it, close it, etc., but the user's proxy may by that point be out of
scope.
This is *technically* not a "bug" in the sense that it is the user who
violated the contract by closing or destroying the proxy before its
last use by anything they passed it to. But in context, it's very easy
to be confused by this -- the user thinks that the only observer of
the proxy is the ImageBuf, and they're done using the ImageBuf at that
point, but because they don't explicitly mention an ImageCache, they
don't realize that the ImageBuf passed the proxy reference along to
the underlying IC that could be backing any ImageBuf.
It's the user's fault in some sense, but let's just say that we have
given them an "attractive nuisance."
That's a long bit of background explanation, but I wanted to force
myself to explain it clearly and make sure it was spelled out in the
commit comments. Now here is the fix:
* When IB::read does an immediate forced read involving an IOProxy,
after the read is complete, tell the cache to invalidate the entry
for the file so that it no longer retains a pointer to the proxy.
* When an IB itself is destroyed or cleared, if it used an IOProxy,
also invalidate the IC entry for that file.
* Unrelated cleanup: avoid a redundant invalidate call in init_spec().
* Make it so that when we build against OCIO 2.2, it is possible to
specify the built-in configs. The logic had previously precluded
this, because at one point it checks to see if the passed config or
contents of env $OCIO was an existing file. Now you can set OCIO to
"ocio://default", for example, and it will be used by OIIO.
* Expose getAliases (I think that has been there for a while but we
didn't expose in in our ColorConfig wrapper).
* Expose isColorSpaceLinear (new to OCIO 2.2)
* New OIIO global getattribute query: "opencolorio_version" returns the
human-readable version string (such as "2.2.0").
* Some miscellaneous other cleanup in color_ocio.cpp
* Change in color methodology for test suite: Previously, for every
call to oiiotool in the testsuite, we passed a `--colorconfig`
argument to oiiotool pointing it to the sample colorconfig that we
ship in the testsuite. Here, I'm changing it so that the cmake setup
for tests sets the OCIO env variable for all tests, so no explicit
--colorconfig is necessary, and thus you won't see a path to the
config in any of the test output files if they need to echo the
command line arguments (and you'll see several test outputs altered
in this way). This is a prelude to later changes that will use the
built-in configs themselves for tests, when available.
More interesting changes coming in subsequent PRs.
When writing a zfile, anything that causes open() to fail will leave
m_tilebuffer at 0 size. Subsequent calls to write_tiles, and also
to close (which cleans up unwritten parts of the buffer at the end)
did not check for this condition and would access out of bounds memory.
While I was in there, I converted the zfile.cpp error messages to the
new formatting convention.
Fixes TALOS-2022-1657
Lately, it has been experiencing intermittent availability problems
that causes our CI to fail while setting up dependencies.
What happens if we don't use the problematic ubuntu-toolchain-r at all?
Nothing, it seems. So eliminate it.
I think that once upon a time, it was necessary to add this repo to
get certain new packages? But maybe they are mainstream enough now
that it's not necessary to add this repo? In any case, it doesn't seem
to hurt anything, it's one less moving part that can't fail if it's
not there.
We use this all the time in our internals, and I'm tired of having to
write Strutil::print. Too verbose. By putting it in the main OIIO
namespace, OIIO internals can reference as simply print(), and
external users now have a choice of saying Strutil::print() or
OIIO::print().
* Add casual tests of oiiotool with no args (print brief usage message)
and when an unknown command.
* Also changed the meaning of -q (quiet mode) to be more quiet -- in
particular, not to print the full help message for errors when quiet
mode is used, only print the error.
* oiiotool test sequence errors.
* Test (and fix) --selectmip and --subimage with out of range indices
Changes:
* Scorecards and analysis -- allow newly launched job to cancel
running old job from the same PR.
* Scorecard only runs on main repo, no need to run on forks.
* Nightly CI only runs on main repo, no need to run on forks.
* Test oiiotool control flow syntax mistakes
* Test oiiotool --set with array of strings
* oiiotool: better adjust_geometry unit testing
Helper utility additions:
* oiiotool --no-error-exit
`--no-error-exit` causes an error to not exit immediately, but
rather to try its best to continue execuing command line
operations. Use with caution! This is mainly for unit tests and
debugging.
`--unittest` also turns this on automatically, so that tests that
are probing failures don't cause an exit.
* unittest.h: If OIIO_UNIT_TEST_QUIET_SUCCESS is defined, don't print
anything extra for unit test success
Make extra sure to clear the oiio:ConstantColor, oiio:AverageColor,
and oio:SHA-1 metadata when writing textures, if those things are not
going to be set.
* Unit test adjust_geometry
* Tests of --set for various implicit and explicit types not
previously covered in the testsuite
* Comment out Oiiotool:get_position, which seems to be unused
* Exercise --iconfig for explicit types
* Add test for --origin and --originoffset
* Add test for --ociodisplay
* Add test for --unmip and --selectmip
It's similar to trim_whitespace, but returns the trimmed result, rather
than operating in-place on a `string_view&`.
Also add unit testing of trim_whitespace and trimmed_whitespace
Along the way, improve the runtest.py script by giving the
info_command() fuction the same silent, concat, and failureok
parameters that the oiiotool() function has. (This lets individual
tests assemble iinfo command lines with a little more flexibility.)
* In the case of DDS cubemap files where a cube face was not present,
we were filling m_buf with 0 bytes before allocating it.
* Add check for invalid bits per pixel.
* Add regression test for another bug that was perviously fixed in
2.4.3.
In one case, we actually had a check for this, but an assignment to an
int made the nonsensical offset appear negative, and we only tested
whether the necessary offset it was bigger than the buffer size.
Keeping it as (unsigned) size_t makes the test work as intended.
In another case, there were several places where we never checked that
we were staying within the exif block, and here we address this by
changing the utility decode_ifd so instead of passing it a pointer to
the ifd, it passes the offset (the pointer turned out to always be
inside the buffer) so it can check the extent for subsequent accesses.
Also some fixes related to squashing undefined behavior sanitizer
cases.
Also, change `operator<<` to output the string, not the hash itself.
These changes help push us more toward the original intent of
ustringhash being a drop-in replacement for ustring, any time it's
more desirable to be using around the hashes rather than the pointers.
* oiiotool tests for expressions var() and getattribute()
* Make sure we test oiiotool IMG[] expressions and several specific
special attributes to retrieve.
* Add coverage of oiiotool --keyword and --clear-keywords
When reading the image (which can have signals to jump around in the
array), we checked for out-of-bounds x coordinate, but not y, and some
corrupted files could overrun buffers as a result.
The existing syntax `TOP.name`, if the name wasn't found, produced
an error.
This PR adds another syntax: `TOP[name]` which is identical if the
named metadata is found. But if not found, this syntax just evaluates
to the empty string, with no error.
This test was taking almost 30 seconds to run, without a lot of images
being read or written. Turns out it was just a large number of separte
oiiotool invocations, and the overhead of all those shells and
programs launched adds up. Most of them were just firing up oiiotool
for the sake of doing an `--echo` to print things. A little bit of
restructuring to make fewer oiiotool invocations (each one doing a
bunch of echo commands, each testing a different thing) cuts the time
for the whole test by 50%.
I also made the runtest.py script allow certain test debugging by
letting a test set the `wrapper_cmd`, and if so, will prepend every
oiio utility launch with it. For example, a test could say
`wrapper_cmd='time'` to time every command.
Add a hidden --unittest command to oiiotool, only functional for Debug
builds, which enables additional testing of internal utility funcions
and other things that are difficult to fully exercise in the testsuite
from normal oiiotool commands (could be done, but would take tons of
additional tests to get into particular deep edge cases).
Prototype by adding unit tests for scan_box. More will be added later
now that the mechanism exists.
Simplify string_to_dataformat
Make sure to use --list-formats and --runstats during testing.
* We mis-named color_test, and that caused it to never get run!
* imagespeed_test is meant for benchmarking only, run by hand.
We don't make a testsuite test, so don't even build the source
(which would never be counted for code coverage because it doesn't
run) during CI.
* Add a test for cineon -- we never directly tested it before
* Don't compile code used only for writing. We don't supporting
writing cineon, so there is no point compiling the cineon module
used only for writing.
* And of course, once cineon was in the testsuite, we immediately
found a problem via the sanitizers! So fix buffer overflows when
reading FilmEdgeCode in Cineon. (And clean up the equally sloppy,
though not technically buggy, code in DPX as well.)
On any one platform, our testsuite was only testing the old C++ and
the new C core exr implementations.
Rig it to run the openexr-oriented tests twice -- once in each state
(when using OpenEXR new enough to support the core library).
Doing this cleanly required adding SUFFIX and ENVIRONMENT optional
parameters to the oiio_add_tests utility macro. Once that was added,
the texture batch testing could be implemented that way as well,
removing a bunch of hackier special purpose code.
* Protect against 0-sized allocation
* Exception safety in IffOutput destructor
* Using sonar.coverage.exclusions as a better way to exclude files
from coverage analysis
* Null pointer safety
I realized that the recently added ustringhash class is confusingly
similar to the (obscure, barely used, mostly forgotten) ustringHash
functor. But also ustringHash doesn't need to exist, because it's
equivalent to `std::hash<ustring>`, if we define it. So now we do define
it, and also `std::hash<ustringhash>`, and deprecate ustringHash, to
be removed entirely for 3.0.
Also, add a ustring ctr from ustringhash (explicit only for now),
and `operator<` for ustringhash.
And add from_hash(size_t) method to ustringhash, analogous to
ustring::from_hash().
Modernize a few places we still used `(void)` to just `()`.
Give ustring and ustringhash a `using rep_t =`
rep_t is the type of the underlying representation (const char* for
ustring, size_t for ustringhash).
Modernize some typedef into using.
Make `==` and `!=(ustringhash, const char*)` be constexpr for C++ >= 17
In particular, defining NOGDI before including <windows.h> achieves
several things amongh others:
- No longer causes ERROR symbol to be defined to 0. ERROR being defined
prevents using e.g. google/glog library, since it wants ERROR to mean
something else.
- No longer defines some possibly widely used names e.g. GetObject.
16 bit per pixel (e.g. 555, 565, 444) formats probably have never
worked. One part of the code was assuming 5 bits in the mask, another
was shifting by 4 bits, and the 16-bit pixel fetch code was reading just
one byte.
The code assumes "char" is "signed char" (as on x86), but on ARM "char" is
"unsigned char".
Co-authored-by: Joachim Reichel <joachim.reichel@posteo.de>
In some cases (at least on Windows, with static linking) the oiiotool
main returns zero, but the full process returns 3 error code due to
abort() being called. And that is called because a global shared
image cache is already destroyed, but the image references from another
global (Oiiotool itself) try to use it, get a call into a "pure virtual
function" stub that is put by MSVC C runtime into a destroyed object
vtable, and that ends up in an abort().
Work around that by making sure various member variables holding
ImageRecRef are cleared before main() finishes and global destructors
are called.
Most of the cost in HDR file reading was converting from RGBE to floats,
inside ldexpf(), which at least on Windows/VS2022 spends most of the
time inside _ctrlfp(). But we only have 256 possible exponents in total,
so just use a precomputed table.
On my PC (Windows 10, VS2022, Ryzen 5950X) this change gets file read
time for an 8x resolution .HDR image from 1.10s down to 0.27s
HDR code overhaul to support IOProxy (#3218) changed all reads into
a `pread()` with explicit file position tracking. Turns out, that is
quite a larger overhead compared to simple sequential reads; even more
so on Windows.
On my PC (Windows 10, VS2022, Ryzen 5950X) this change gets file read
time for an 8x resolution .HDR image (with RLE compression) from
8.28s back to 1.10s just like it was in OIIO 2.3.
On Windows the extra cost when using pread() is a bit more extra mutex
locks, but the real cost is in enventual ReadFile; looks like any seek
operations make it take some sort of "way slower" path with various
callbacks and whatnot inside the kernel.
---
Note repoduced from discussion #3588 from LG:
Switching from IOProxy::pread to using IOProxy::seek + IOProxy::read makes the IOProxy stateful and not thread-safe without an external lock (which this HdrInput has, so it's not going to break anything), precluding any future removal of the locks from read_native_scanline to make the whole ImageInput able to read concurrently. That's probably not important for this file format, but for others (especially crucial those usable as texture), it would be desirable to rely on those stateless pread calls and remove the lock from the ImageInput.
I think this patch is fine. We don't expect a lot of concurrent reads from hdr like we do for tiff and openexr that we use extensively for highly threaded on-demand reading of texture. It fixes the recent performance regression, and it doesn't really cost us concurrency because we already had the lock in HdrInput. But in general, we want to prefer eschewing read/seek and using pread instead, so we can expect maximum concurrency from multiple threads using an ImageInput (as we do in ImageCache/TextureSystem).
So I believe that a revised and more complete diagnosis is that the problem isn't the pread vs read per se, but that for RLE-compressed images we were calling pread separately for every 4 bytes! The calls to read are faster simply because they are buffered (in FILE) and require fewer OS system calls. But the real sin is that we call it way too many times.
I think that the rle case could go back to pread if we want, but do just one pread per scanline (rather than one per pixel value run), asking for the maximum amount that will be needed to read the scanline for the worst case rle representation (it's ok if it's the end of the file, pread will just read up to the end and return the true number of bytes read). In other words, we'd be doing the buffering ourselves.
But anyway, the real misdesign of this HDR reader is ancient, and consists of doing many many tiny reads. Even the buffered fread would be sped up a lot by not doing it separately for every 2-4 bytes of the file. I don't know if it's worth fixing for this format. Depends on whether anybody really needs to maximize its performance or expects multiple threads reading the same hdr file to be fully concurrent.
Add a new build-time cmake cache variable: FORTIFY_SOURCE
Default to empty, but can be set to 1, 2, or 3, corresponding to
setting the _FORTIFY_SOURCE macro available in recent versions of
clang and gcc. (I'm not sure exactly which minimum compiler version is
needed for each fortification level, except that for level 3, gcc must
be 12+.)
Fortification involves replacing several "unsafe" memory-related
functions such as memcpy, memset, strcpy, etc, with special versions
that do bounds checking, aided by some compiler smarts for
understanding the likely bounds of different buffers. If I understand
correctly, at level 2 it can figure out bounds of constant-sized
arrays, and at level 3 it can figure out certain dynamic cases as
well.
There are two use cases for this:
1. For our own CI, this is yet another bit of static and dynamic
anslysis to enable (currently, just in the gcc12 test) to possibly
catch bugs.
2. Users who are building OIIO to deploy it in security-sensitive
environment may wish to build with some fortification level enabled to
help prevent certain memory errors or security issues, understanding
that it may slightly impact performance.
* Refactor the clang-format job to be part of the aswf batch of jobs
rather than separate (reduces redundant logic in the workflow file).
* Give proper names to distinguish the two Mac jobs' artifacts for
broken builds and their caches.
* Revise scorecard workflow triggers: no need to run weekly, and run
on push only to the main repo, no reason to run on forks.
In addition to address and leak sanitizers, also turn on the undefined
behavior sanitizer in our CI run. We can do it in the same job as
asan/lsan.
Some things that come along for the ride, including fixes for things
that ubsan uncovered:
* src/build-scripts/ubsan-suppressions.txt can hold suppression list
(though we don't have any now, it just sets up the space for it for
future needs).
* Split the "targa" test so that the parts that directly need the OIIO
python module (which are all related to thumbnails) are now in
"targa-thumbnails" test. That has to be disabled for the sanitizers,
since we can't arrange for python to b linked to the sanitizer
libraries.
* Found vint4::store has a potentially unaligned integer store.
* Found Strutil::concat is not necessarily safe if either of the
strings are empty.
Also remove three raw test reference output files that we apparently
no longer use (corresponding to platform + libraw combinations that
aren't in our current set of CI configurations).
Something changed in libtiff master and it doesn't build shared libs
by default, breaking our "bleeding edge" CI test. So when we build it,
be sure to ask for it. It can be overridden in build_libtiff by
invoking it with env variable `LIBTIFF_BUILD_SHARED_LIBS=OFF`.
Comparing the time it takes to load DXT/BCn compressed DDS images via
OIIO, I noticed that it's several times slower than in another codebase
(Blender). So I started profiling, and fixing some low hanging fruit.
- After decoding a BCn block (always 4x4 pixels), it was written into
destination image using quite many branches in the innermost loop.
Now it still takes care of not writing outside of a destination image
that might not be multiple-of-4 in size, just with way fewer branches.
- BCn decompression is multi-threaded via parallel_for_chunked, using
at least 8 block rows per job (i.e. 32 pixel rows). This is
similar to internal multi-threading done by EXR or TIF readers.
- Avoid one case of std::vector resize (which clears memory) followed
by immediate overwriting of that buffer with bytes read from the file.
Switched to using unique_ptr for that array, similar to how other
input plugins do it.
Performance data: I'm timing how long it takes to do "iinfo --hash" on
24 DDS files, each 4096x4096 in size. Quite a lot of time there is taken
by just the SHA hash calculation which is not related to loading, so
also have timing info for just DDSInput::read_native_scanline where
all the actual DDS loading/decoding happens.
Timings on PC (Windows 10, VS2022 Release build), AMD Ryzen 5950X.
- BC1: total 3.68s -> 2.76s, read_native_scanline 1.54s -> 0.59s
- BC3: total 3.74s -> 2.84s, read_native_scanline 1.83s -> 0.66s
- BC4: total 1.26s -> 0.82s, read_native_scanline 0.74s -> 0.24s
- BC5: total 2.29s -> 1.57s, read_native_scanline 1.31s -> 0.46s
- BC6: total 7.50s -> 4.30s, read_native_scanline 4.67s -> 1.60s
- BC7: total 6.18s -> 3.09s, read_native_scanline 4.34s -> 0.84s
Possible future optimization: implement DDSInput::read_native_scanlines,
that could avoid one extra temporary memory buffer copy. This extra
buffer clear & eventual copy from it into final user destination pixels
is non-trivial cost from profiling. But it's also quite involved
to handle all the possible edge cases (e.g. user requests scanlines
not on 4-pixel row boundaries); the code would have to do similar
juggling as EXR reader does to handle "not on chunk boundaries" reads.
It could potentially also interplay with DDS cubemap/volume texture
reading ("tiled") code paths, for which there is no test coverage at all
right now. Maybe for some other day.
* DDS: fixes for A8, L8, A8L8 formats
While developing #3573 at one time I had them working properly, but then
while fixing the address sanitizer failure for 3-channel formats I
botched them again. Note to self: never again do a "yeah I'll add tests
later", since if I had all of them running all the time this would
not have happened. 🤦
* DDS: extend test coverage for more formats & channel size cases
With more test images recently added (https://github.com/OpenImageIO/oiio/pull/3459),
start using them for DDS tests. This covers now:
- Alpha, Luminance and Alpha+Luminance formats,
- RGB formats with rarer channel sizes (rgb10 a2, r3g3b2),
- Several "broken" DDS file cases
- Removed usage of sample-DXT1 since it is well covered by others.
Adapted from the similar document I wrote for OSL.
This serves both as a checklist for me to make sure I don't miss any
steps during a release, but also so that other people could take over
release procedures if necessary or desired.
I also rearranged the order of some lines in CMakeLists.txt for
clarity (but did not change the logic).
This patch allows ParamValue, ParamValueList, and ImageSpec to support
attribute/getattribute of metadata consisting of uint8[] arrays, which
is how we store certain binary blobs or raw byte arrays. We translate
this to Python as a numpy array of type uint8 (dtype='B').
This fixes a limitation where it was not possible to directly retrieve
or set the "ICCProfile" metadata (which is stored as a uint8 array)
from the Python APIs, as you could easily do from the C++ APIs.
In the process, I found similar bits of code in several different
places, in various states of incompleteness. I refactored to make a
single make_pyobject() utility and then used that call in place of
several other redundant pieces of functionality.
For a while, we have found embedded ICC profiles in TIFF, JPEG,
JPEG-2000, PNG, and PSD (PhotoShop) files and store them in the
metadata as a byte array with the name "ICCProfile".
With this patch, we further add the facility of parsing the binary ICC
profile data to extract a number of other fields that we add to the
ImageSpec metadata so that "oiiotool -info -v" or "iinfo 'v"
will print this info.
Important caveat: You can read this for informational purposes, but
changing it (e.g. with spec.attribute()) does not change the contents
of the ICCProfile byte array, which is still the canonical ground
truth of the profile. You have to replace the "ICCProfile" byte array
to actually change the profile.
* Add `Strutil::utf16_to_utf8(const std::u16string& utf16str)`.
This is subtly different from our existing utf16_to_utf8() that
takes a std::wstring, because wchar_t is not the same on all
platforms. If it really and truly is utf16-in-char16_t, you need
this and can't use the utf-in-wchar_t which may be 32 bit chars.
* Add `Strutil::utf8_to_utf16wstring()` (identical to the old
utr8_to_utf16) to make it more self-documenting as returning a
utf16-encoded wstring and NOT a u16string. Note that wchar_t is not
necessarily char16_t on all platforms (it is on Windows, but it's 32
bits on Linux).
* Deprecate the ambiguously named `utf8_to_utf16()` for OIIO >= 2.5 and
remove it for OIIO >= 3.0.
Rather than specify the version in the usual project() statement,
project (OpenImageIO VERSION 2.5.0.0 ...)
this patch decouples it
set (OpenImageIO_VERSION "2.5.0.0" CACHE STRING "Version")
project (OpenImageIO VERSION ${OpenImageIO_VERSION} ...)
thus allowing the possibility of overriding the version number at
build confuguration time (`cmake -DOpenImageIO_VERSION=3.1.4.1`).
This should be used with extreme caution. Probably never used by most
people. I don't need to describe all the ways that things can go
sideways if you make your build of x.y.z.w actually be arbitrarily
different from the x.y.z.w that everyone else uses and that is
documented.
That said, there are two scenarios I have in mind for which this
ability is very useful:
1. Sometimes, we add or deprecate API features "in advance" by having
an `#if` enable or disable code based on the version, essentially
putting in place changes that can't be enabled yet because they
would break compatibility rules, but we want to integrate the code
now and have it become active upon reaching a particular future
version in which that incompatibility will be allowed. The version
number override makes it easy to test this by quickly doing a build
"as if" we had already bumped the version number to see what will
happen when we actually do.
2. Every once in a while we need to do an internal build that, to
conform to version compatibility rules, needs to appear to still be
part of a past version family. This lets individual sites play some
games with version number overrides rather than have to create new
branches and tags for certain one-off internal releases.
--iccread reads a named file and directly adds its contents as the
"ICCProfile" metadata of the top image (as a byte array).
--iccwrite takes the "ICCProfile" metadata of the top image and
writes it to a named file.
Suggested by SideFX: explicitly shut down the OpenEXR thread pool
before exiting oiiotool, because sometimes on Windows it seems to hang
otherwise. A better solution is probably possible on the OpenEXR side,
but I don't know what it would be. (And in any case, we'd still need
to do this be be well behaved when using OpenEXR versions earlier than
where a fix might land.)
Solution tracked down by Edward Lam.
Oops, at some point, something about the way we set up the CI test
that runs the sanitizers, and ever since then (embarrassingly, perhaps
as much as a couple years ago), the "sanitizer" test has not in fact
been running the sanitizers. Here we fix that up, get sanitizers
running again.
* Minor png refactoring to fix some memory leaks for broken png files.
* Remove unused string variable from read_info() and simplify the
surrounding "if"
* Split png test into png and png-damaged, because I just can't seem
to squash some very minor memory leaks for broken png files, I think
it's not worth worrying about and I'll just exclude the broken case
from testing for the address sanitizer.
- If a DDS pixel format header contains "this is a normal map" flag
set, then handle that properly (#3525):
- For a DXT5, this means it's a "DXT5nm" format. Normal X & Y
components are in green and alpha channel respectively. Compute Z
component from those, and the result is a 3-channel tangent space
normal map.
- For ATI2/BC5 format that also has a "normal map" flag set, compute
the Z component automatically too, and result is a 3-channel image.
However a bunch of ATI2/BC5 normal maps out there in the wild do
not have that bit set, so also have a global "dds:bc5normal"
attribute to optionally turn this on.
- Fix support of R10G10B10A2 format - was producing garbage data due to
code assumption that truncation down to 8 bits never needs to happen.
- Fix bit expansion ("too dark") of very low-bit formats, e.g. R3G3B2.
Code was simply shifting the bits, instead of properly repeating
the high bits into new low bit positions (i.e. a 3-bit "111" needs
to become an 8-bit "11111111" and not a "11100000").
- Support "RXGB" (DXT5, with red & alpha swapped) as well as "BC4U"
and "BC5U" formats (the latter two are just BC4 and BC5, produced by
some older tools).
- Support alpha-only (e.g. A8) formats.
All this mostly prompted by doing DDS improvements in Blender, and
wanting to get Cycles renderer (which uses OIIO) to the same feature
parity. See https://developer.blender.org/T101405 and
https://developer.blender.org/D16087.
Reading uncompressed scanlines would copy 4 bytes from each pixel into
an int and then decode the bits, incrementing where it was reading
from by Bpp bytes each time. But when Bpp == 3, the last pixel of
every row would therefore read an extra byte past the end of the heap
storage. Allocation padding would in practice keep it from crashing,
and that meaningless extra byte was not used in the decode because it
knew it only needed the first 3 of the integer. But found by address
sanitizer!
This was an experiment that was added to master at one point but
removed before we branched for a release. But we forgot to remove a
few headers. Removing them now is not a compatibility break, since we
never shipped a supported release that contained the library that
implemented the functions declared in these headers.
This was part of a set of obsolete calls where we recently removed the
implementations, but for link compatibility, made matching shim calls
and wrap the modern versions. But I missed one case.
Changes since RC1:
- ImageCache/TextureSystem both have added a `getattributetype()`
method, which retrieves just the type of a named attribute. #3559
(2.4.4.0) NOTE: THIS IS AN ABI BREAKING CHANGE
- Python: Fix the ability to `getattribute()` of int64 and uint64
metadata or attributes. #3555 (2.4.4.0)
- Build: Improvements to the generated cmake config files when
building static libraries. #3552#3557 (2.4.4.0)
- Support for gcc 12.1. #3480 (2.4.2.1) #3551 (2.4.4.0)
- Support building with clang 15.0. #3563 (2.4.4.0)
- Fix cross-compiling on Android failing due to `-latomic` check. #3560
(2.4.4.0)
- Fix building on iOS. #3562 (2.4.4.0)
Note that we bumped the version to from 2.4.3.x 2.4.4.0 to reflect the
ABI break introduced by the last minute by the IC/TS addition of a new
virtual function. ABI will not change again for 2.4.
ImageSpec and ParamValueList have had (since 2.1) a getattributetype()
method, which simply returns the TypeDesc of the named attribute, or
TypeUnknown if no such attribute exits. This goes along well with the
related getattribute() and attribute() family of methods.
The getattribute() method was not present in ImageCache and
TextureSystem, despite their having very similar
getattribute()/attribute() interfaces.
A recent issue made me realize that in the Python bindings, the IC and
TS bindings for `getattribute(name)` did not work properly, always
returning None, and *required* you to use the full
`getattribute(name,type)` variety where you have to supply -- and
already know -- the type. This isn't especially Pythonic, where you
should just be able to ask by name and get a polymorphic result. And
when I went to look at how to repair this, it was going to be very
awkward without getattributetype(), but trivial with it.
So this patch sets up getattributetype() for both IC and TS, and
exposes them to both C++ and Python APIs, and also fully fixes the
getattribute() so that it works properly with the name only.
Unfortunately, because getattributename is a new virtual method, it
changes the vtable, breaking the ABI, and thus cannot be backported to
any supported release that makes ABI compatibility promises.
Currently when cross-compiling for Android build fails due to usage of
TRY_RUN(). In the particular case it tries to figure out whether atomics
can be used without the libatomic library.
Fortunately we don't need this check because on Android NDK atomics are
built-in and libatomic.so is not present. Only static dummy libatomic.a
exists for compatibility with -latomic flag.
Just a couple missing pieces were preventing getattribute() from
retrieving attributes that were stored as int64 or uint64. That
doesn't come up very often, but one place is retrieving certain
statistics that use 64 bit counters. (This comes up in ImageCache,
among other places.)
While we're at it, there was no existing test dedicated to the python
binding of ImageCache. So to add these getattribute calls, I made one.
There is an important caveat about ImageCache.getattribute(): In this
process, I discovered that the flavor of IC.getattribute(name) where
you only pass the name is BROKEN, and will always return None. You
MUST use the form where you pass the type, like
x = ic.getattribute("foo", "int")
This is unfortunate and not very Pythonic, but it is the workaround
for now. I will addess getting the name-only form working in a
separate PR.
Closes#3537
TBB::tbb is added as a library dependency via TARGET_NAME_IF_EXISTS.
Therefore if an executable links OpenImageIO static library without the
executable itself depending on TBB, then TBB will not be linked in and
result in link time errors.
This fixes builds in projects that compile OpenImageIO as static
libraries and don't themselves depend on the image libraries used by
OpenImageIO such as PNG, JPEG or TIFF.
When OpenImageIO is built as a static library its targets have e.g.
PNG::PNG among INTERFACE_LINK_LIBRARIES. If the including project does
not itself depend on PNG, the following error would be observed:
Target "<...>" links to target "PNG::PNG" but the target was not found.
Perhaps a find_package() call is missing for an IMPORTED target, or an
ALIAS target is missing?
@@ -15,7 +15,7 @@ This is a checklist for TSC's to review as part of the intake process. The TSC s
- Infrastructure/Assets
- [X] Technical Charter Approved
- [X] License scan completed and no issues found
- [] Code repository imported to ASWF GitHub organization or ownership of current GitHub organization given to `thelinuxfoundation` user
- [X] Code repository imported to ASWF GitHub organization or ownership of current GitHub organization given to `thelinuxfoundation` user
- [X] Developer Certificate of Origin past commit signoff done and DCO Probot enabled.
- [X] Issue/feature tracker established (JIRA, GitHub issues)
- [X] Mailing lists ( one of )
@@ -25,8 +25,8 @@ This is a checklist for TSC's to review as part of the intake process. The TSC s
- [ ] Website
- [X] CI/build environment
- [X] Trademarks/mark ownership rights ( complete 'LF Projects - Form of Trademark and Account Assignment' - create [issue on foundation repo](https://github.com/AcademySoftwareFoundation/foundation/issues/new) - only needed if project using existing name )
- [] Domain name ( create [issue with the LF IT staff](https://jira.linuxfoundation.org/plugins/servlet/theme/portal/2/group/19) to setup/transfer )
- [] Zoom account access ( create [issue on foundation repo](https://github.com/AcademySoftwareFoundation/foundation/issues/new) to get access to credentials )
- [X] Domain name ( create [issue with the LF IT staff](https://jira.linuxfoundation.org/plugins/servlet/theme/portal/2/group/19) to setup/transfer )
- [X] Zoom account access ( create [issue on foundation repo](https://github.com/AcademySoftwareFoundation/foundation/issues/new) to get access to credentials )
- [X] (N/A) Social media accounts or other project accounts ( create [issue on foundation repo](https://github.com/AcademySoftwareFoundation/foundation/issues/new) to transfer )
- [X] Logo(s) ( create [issue on artwork repo](https://github.com/AcademySoftwareFoundation/artwork/issues/new) to add in SVG and PNG format and color/black/white )
- New Project Governance
@@ -43,8 +43,8 @@ This is a checklist for TSC's to review as part of the intake process. The TSC s
- [X] Project added to ASWF website and ASWF landscape
- Incubation Stage requirements
- [X] Have achieved and maintained a [OpenSSF Best Practices Badge at the passing level](https://bestpractices.coreinfrastructure.org/en/criteria).
- [] Have had a successful license scan with any critical issues remedied.
- [] Have a defined project mission and scope
- [X] Have had a successful license scan with any critical issues remedied.
- [X] Have a defined project mission and scope
- [ ] An overview of the project’s architecture and features defined.
- [ ] A project roadmap defined, which should address the following questions.
- What use cases are possible now?
@@ -54,7 +54,7 @@ This is a checklist for TSC's to review as part of the intake process. The TSC s
- Demonstrate a sustained flow of commits / merged contributions
- A credible plan for developing a thriving user community, in particular expanding the number of committers and contributors?
- Outline of the plan for the project to complete the requirements for Adopted Stage
- [] Obtain an affirmative vote of the TAC.
- [X] Obtain an affirmative vote of the TAC.
- Adopted Stage graduation requirements
- [ ] Demonstrate a substantial ongoing flow of commits and merged contributions, authored by a healthy number of diverse contributors.
| master | :white_check_mark: :construction: ALL fixes immediately, but this is a branch under development with a frequently unstable ABI and occasionally unstable API. |
| 2.4.x | :white_check_mark: All fixes that can be backported without breaking ABI compatibility. New tagged releases monthly. |
| 2.3.x | :x: Only receives occasional critical fixes, upon request. |
| <= 2.2.x | :x: No longer receiving patches of any kind. |
| 2.5.x | :white_check_mark: All fixes that can be backported without breaking ABI compatibility. New tagged releases monthly. |
| 2.4.x | :warning: Only receives occasional critical fixes, upon request, only if they can be easily backported. |
| <= 2.3.x | :x: No longer receiving patches of any kind. |
## Reporting a Vulnerability
@@ -23,3 +23,30 @@ respond to vulnerability reports within 14 days.
Our policy is to address critical security vulnerabilities rapidly and post
patches as quickly as possible.
## Outstanding Security Issues
None known
## History of CVE Fixes
Most recent fixes listed first, more or less
- CVE-2023-42295: Fix signed integer overflow when computing total number of pixels while reading BMP files. [#3948](https://github.com/AcademySoftwareFoundation/OpenImageIO/pull/3948) (by xiaoxiaoafeifei) (Fixed in 2.5.3.0/2.6.0.1)
- CVE-2023-36183: Heap-buffer-overflow while reading ICO files [#3872](https://github.com/AcademySoftwareFoundation/OpenImageIO/pull/3872) (by xiaoxiaoafeifei)
- TALOS-2022-1627, CVE-2022-41977: Guard TIFF reads against corrupt files with buffer overflows. [#3628](https://github.com/AcademySoftwareFoundation/OpenImageIO/pull/3628) (2.4.5/2.5.0.0)
- TALOS-2022-1626 / CVE-2022-41794, TALOS-2022-1632 / CVE-2022-41684, TALOS-2022-1636 / CVE-2022-41837: Exif (all formats that support it, TIFF/JPEG/PSD) fix bugs where corrupted Exif blocks could overrun memory. [#3627](https://github.com/AcademySoftwareFoundation/OpenImageIO/pull/3627) (2.4.5/2.5.0.0)
- TALOS-2022-1634 / CVE-2022-41838, TALOS-2022-1635 / CVE-2022-41999: Fix DDS reading crashes for cubemap files when a cube face was not present, and check for invalid bits per pixel. [#3625](https://github.com/AcademySoftwareFoundation/OpenImageIO/pull/3625) (2.4.5/2.5.0.0) #3625 (2.4.5/2.5.0.0)
- [ ] libheif 1.11 (from 1.3), which supports many additional features of that format.
- [ ] WebP 1.2 (from 0.6) which lets us use their exported CMake configs and retire FindWebP.cmake.
- [ ] pybind11 2.6 or 2.7 (from 2.4).
- [ ] [#4156](https://github.com/AcademySoftwareFoundation/OpenImageIO/issues/4156) Eliminate the last few places where we use Boost and eliminate it as a dependency.
<br>
## API Modernization
- [ ] [#4159](https://github.com/AcademySoftwareFoundation/OpenImageIO/issues/4159) Using span and string_view instead of raw pointers
Scrub the APIs to find where we have API calls that take a pointer and size
(or worse -- just a pointer and an assumption about the size), and instead
use `span<T>` / `cspan<T>` for things that are like arrays, and
Using named keyword/value lists more extensively in imagebufalgo.h
- [ ] Deprecate as much as possible of the old printf-style string formatting we use internally, instead using the fmt/std::format style everywhere. This includes changing Strutil::format to alias to Strutil::fmt::format (currently it aliases to Strutil::old::format, which uses printf style).
- [ ] Remove deprecated API elements
Hunt for things marked DEPRECATED and try to get rid of them where possible.
If we can't remove them, at least make sure they are marked as
`OIIO_DEPRECATED` (or `[[deprecated]]` in modern C++).
## The rest
Other initiatives we hope to have completed by the time of this next
major release:
- [ ] [#4164](https://github.com/AcademySoftwareFoundation/OpenImageIO/issues/4164) Better color management
Regardless of OCIO availability, version, or contents of any configs, have universal support for the [canonical color spaces](https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/documents/Specification/MaterialX.Specification.md#color-spaces-and-color-management-systems) that seem to be the common consensus of ACES, MaterialX, and USD.
- [ ] Preliminary Rust bindings for most of the OpenImageIO APIs.
- [ ] Python wheel construction so `pip install openimageio` will be an easy way
for users to install the whole banana.
If there is something you think should be on the roadmap for the next major
release but is not, please open an issue or discussion to propose it, or
bring it up at a TSC meeting, or on the mail list or Slack channel.
## Parking
Here is where we will put things that definitely should be on the roadmap, but
that need not be completed in time for the fall 2024 release of OIIO 3.0.
message(WARNING"${ColorYellow}OpenVDB >= 10.1 (we found ${OpenVDB_VERSION}) can only be used when we build with C++17 or higher. Disabling OpenVDB support.${ColorReset}")
set(OpenVDB_FOUND0)
add_compile_definitions(DISABLE_OPENVDB=1)
endif()
checked_find_package(PtexPREFER_CONFIG)
if(NOTPtex_FOUNDORNOTPtex_VERSION)
@@ -246,12 +256,14 @@ if (NOT Ptex_FOUND OR NOT Ptex_VERSION)
endif()
checked_find_package(WebP)
# Note: When WebP 1.1 (released late 2019) is our minimum, we can use their
# exported configs and remove our FindWebP.cmake module.
# generate data in scanline[0..xres*channels-1] ...
scanline = ...
out.write_scanline (y, z, scanline)
}
out.close ()
The first two arguments to ``writescanline()`` specify which scanline is
The first two arguments to ``write_scanline()`` specify which scanline is
being written by its vertical (*y*) scanline number (beginning with 0)
and, for volume images, its slice (*z*) number (the slice number should
be 0 for 2D non-volume images). This is followed by a `TypeDesc`
@@ -212,7 +182,7 @@ All ``ImageOutput`` implementations will accept scanlines in strict order
any). See Section :ref:`sec-imageoutput-random-access-pixels` for details
on out-of-order or repeated scanlines.
The full description of the ``writescanline()`` function may be found
The full description of the ``write_scanline()`` function may be found
in Section :ref:`sec-imageoutput-class-reference`.
Writing individual tiles
@@ -220,7 +190,7 @@ Writing individual tiles
Not all image formats (and therefore not all ``ImageOutput``
implementations) support tiled images. If the format does not support
tiles, then ``writetile()`` will fail. An application using OpenImageIO
tiles, then ``write_tile()`` will fail. An application using OpenImageIO
should gracefully handle the case that tiled output is not available for
the chosen format.
@@ -297,7 +267,7 @@ individual plugin.
out.write_tile (x, y, z, tile)
out.close ()
The first three arguments to ``writetile()`` specify which tile is being
The first three arguments to ``write_tile()`` specify which tile is being
written by the pixel coordinates of any pixel contained in the tile: *x*
(column), *y* (scanline), and *z* (slice, which should always be 0 for 2D
non-volume images). This is followed by a `TypeDesc` describing the data
@@ -311,7 +281,7 @@ All ``ImageOutput`` implementations that support tiles will accept tiles in
strict order of increasing *y* rows, and within each row, increasing *x*
column, without missing any tiles. See
The full description of the ``writetile()`` function may be found
The full description of the ``write_tile()`` function may be found
in Section :ref:`sec-imageoutput-class-reference`.
Writing arbitrary rectangles
@@ -451,15 +421,15 @@ do not attempt to remap values, and do not clamp (except to their full
floating-point range).
It is not required that the pixel data passed to ``writeimage()``,
``writescanline()``, ``writetile()``, or ``write_rectangle()`` actually be
It is not required that the pixel data passed to ``write_image()``,
``write_scanline()``, ``write_tile()``, or ``write_rectangle()`` actually be
in the same data type as that requested as the native pixel data type of the
file. You can fully mix and match data you pass to the various "write"
routines and OpenImageIO will automatically convert from the internal format
to the native file format. For example, the following code will open a TIFF
file that stores pixel data as 16-bit unsigned integers (values ranging from
0 to 65535), compute internal pixel values as floating-point values, with
``writeimage()`` performing the conversion automatically:
``write_image()`` performing the conversion automatically:
..tabs::
@@ -484,7 +454,7 @@ file that stores pixel data as 16-bit unsigned integers (values ranging from
out.write_image (pixels)
Note that ``writescanline()``, ``writetile()``, and ``write_rectangle()``
Note that ``write_scanline()``, ``write_tile()``, and ``write_rectangle()``
have a parameter that works in a corresponding manner.
@@ -507,13 +477,13 @@ passed to the "write" functions are *contiguous*, that is:
* for 3D volumetric images, the first pixel of slice *z* immediately
follows the last pixel of of slice ``z-1``.
Please note that this implies that data passed to ``writetile()`` be
Please note that this implies that data passed to ``write_tile()`` be
contiguous in the shape of a single tile (not just an offset into a whole
image worth of pixels), and that data passed to ``write_rectangle()`` be
contiguous in the dimensions of the rectangle.
The ``writescanline()`` function takes an optional ``xstride`` argument, and
the ``writeimage()``, ``writetile()``, and ``write_rectangle()`` functions
The ``write_scanline()`` function takes an optional ``xstride`` argument, and
the ``write_image()``, ``write_tile()``, and ``write_rectangle()`` functions
take optional ``xstride``, ``ystride``, and ``zstride`` values that describe
the distance, in *bytes*, between successive pixel columns, rows, and
slices, respectively, of the data you are passing. For any of these values
@@ -1152,8 +1122,8 @@ specification of the subimages at the time you first open the file.
out.write_image (pixels[s])
out.close ()
In both of these examples, we have used ``writeimage()``, but of course
``writescanline()``, ``writetile()``, and ``write_rectangle()`` work as you
In both of these examples, we have used ``write_image()``, but of course
``write_scanline()``, ``write_tile()``, and ``write_rectangle()`` work as you
would expect, on the current subimage.
@@ -1278,8 +1248,8 @@ used for texture mapping):
out.close ()
In this example, we have used ``writeimage()``, but of course
``writescanline()``, ``writetile()``, and ``write_rectangle()`` work as you
In this example, we have used ``write_image()``, but of course
``write_scanline()``, ``write_tile()``, and ``write_rectangle()`` work as you
would expect, on the current MIP level.
Some files were not shown because too many files have changed in this diff
Show More
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Clifford Yapp