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>
Rig CI to properly test "latest" and "bleeding edge" fmt. It turns out
we had not yet been testing 10.1 or keeping up with their master.
In the process, I discovered that the combo of fmt >= 10.1 and gcc >= 11
results in a mangled AVX math heisenbug, symptomatic in our simd_test. I
struggled with this on and off for many days, having great difficulty
reproducing (though it does fail every time and deterministically, I
just can't seem to narrow it to a smaller example). It only happens with
gcc, only gcc >= 11, only when we use FMT_EXCEPTIONS=0 to disable true
exceptions in fmt. The change happens specifically at fmt commit 9a034b0
(midway between 10.0 and 10.1), which changes the definition of
FMT_THROW when exceptions are disabled. Why this should affect SIMD math
is a total mystery, and currently I am suspecting either a gcc compiler
error or that it's exposing undefined behavior. I found that redefining
FMT_THROW on our side to something innocuous is a good workaround. If I
no longer disable fmt exceptions, that also makes it work, but I don't
know what that might break for us or downstream.
My report of this to the fmt project is
[here](https://github.com/fmtlib/fmt/issues/3620), maybe they'll uncover
the true cause.
Signed-off-by: Larry Gritz <lg@larrygritz.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?
When enforcing the max open files limit, there's a heuristic where we
only "try" to get the lock, and if we can't, rather than block, we
just defer the cleanup step until the next time around. So what if we
go slightly over the limit temporarily?
Interesting edge case: what if max_open_files is relatively low, but
the number of threads is very high and you have a high contention rate?
Then you can end up *significantly* over the limit, because almost all
the threads say "somebody else has the lock, I'll leave it to them to
clean up" and blithely open new handles.
So this patch modifies the logic so we do the defer trick only if
we're only a little over the limit. If we've exceeded the limit by
16 handles, grab the lock unconditionally and do the cleanup.
* ArgParse::add_version(str) tells ArgParse the version string, and
automatically adds an option --version that prints the version string
and exits.
* Add this to oiiotool, iinfo, igrep, iv, maketx.
* OIIO::getattribute("version") retrieves the version string.
JFIF/JPEG files only support 1 and 3 channels. For > 3 channels, we
had just been discarding the extra. For 2 channels, we had been
discarding the second, but maybe that's not always right. Try to
distinguish between files whose names appear to be YA -- in which case
output the Y as grayscale (JFIF has no way to represent alpha anyway),
and other cases which we now promote to RGB and just make B be black.
* For 2-channel DDS files, label them as Y,A if the flags indicate
luminance and/or alpha, otherwise label them as R,G.
* Do not set "oiio:BitsPerSample" for cases where the dds.fmt.bpp field
is not assumed to be valid. MS docs say it's valid only if the flags
field indicates RGB, LUMINANCE, or YUV types.
The implementation of boost::filesystem::path::extension() and the
equivalent for std::filesystem disagreed about the result of
extension(".foo"). C++17 correctly considers ".foo" to have no
extension.
This was envisioned to be useful in the very earliest days of OIIO,
but we never really finished it or used it in any helpful way.
In addition to being essentially dead and untested code, it ties us
to boost.asio, which we use nowhere else.
So just get rid of it this vestigial piece.
* Any use of `-i:ch=...` would result in a crash because of
dereferencing an empty shared pointer.
* In the case of `-i:ch=` specifying missing channels, improve the
warning message to make it clear which channels were actually present.
Extend ImageBufAlgo::compare() to take relative metrics, which if
nonzero check the symmetric mean relative error, in addition to the
current absolute error. To be formal, this means that a value
comparison will fail if
abs(A-B) > failthresh && abs(A-B)/((abs(A)+abs(B))/2) > failrelative
Setting the failthresh to 0 is equivalent to only checking relative
error, and setting failrelative to 0 is equivalent to the (previous
behavior) of checking only absolute error.
The idiff utility is extended to take new `-failrelative` and
`-warnrelative` command line arguments.
The hope is that there are various kinds of image comparisons within
tolerances -- like we frequenty use for unit tests, shader
verification, etc. -- where this metric is more helpful than absolute
error.
Set up SonarQube to run weekly (seems unnecessary to do on every PR,
it's a LONG run). Also required some build system fixes to enable code
coverage analysis.
This is handled by the "analysis.yml" workflow (into which I also
merged the scorecard analysis we were doing). This workflow runs once
weekly or on only those pushes/PRs with specially named branches.
There's no reason to run this expensive analysis on every last PR or
push.
A number of fixes come along related to things found by Sonar as I was
setting this up:
* Fix buffer warnings
* Address sonar warning: was reading wrong size into buffer
* warning reduction in dpxoutput, simplify string ops
* Clean up dubious uses of sprintf
* Add assertions
* Save a needless string construction in ParamValue::get_string_indexed
* Fix string safety warnings
* iptc: simplify encoding string data and improving safety
Stranely, this made slightly numerically different texture test
results on icc. Switching its math mode to precise brings it back in
line with the other compilers. The differences aren't visible, so this
isn't an issue in practice, but for tests it made it hard to compare
to reference images.
Hide nearly all of the implementation of the parallel_for family of
methods rather than expose them as inline. This gives us more freedom
to change the implementation in the future without breaking ABI.
Deprecate the parallel methods that take task functions whose first
parameter is the thread ID. The only reason they existed is because
our cobbled together thread_pool used that in its inner workings. But
there are good reasons why we should not expose that:
* We never used it anyway.
* It was not very useful, since sometimes it was a real thread ID, but
other times it was -1 in cases where the calling thread executed the
task directly.
* It is inconsistent with other thread pool implementations that we may
wish to try in the future.
So better just to not expose those thread IDs at all. Mark those
versions of the parallel loops as deprecated and schedule them for
removal in OIIO 3.0.
If TBB is detected at build time, support will be built in that allows
either the old OIIO built-in pool, or TBB, depending on the setting of
a new global OIIO attribute, "use_tbb" (default 0), if set to nonzero
will use the TBB thread pool.
In my experiments, the TBB thread pool seems slightly better -- I
think because there is less overhead, and maybe the clever
work-stealing it does elps to load balance. It's not used by default,
set the attribute if you want to use it (assuming the build
included. After we've had a chance to test more thoroughly, we may
change to use TBB by default. I'm interested to hear people's thoughts
on the matter.
One case where you almost certainly will want to use the TBB thread
pool is if you're using libOpenImageIO from within an application that
uses TBB for its own threding -- that way you're using one pool for
everything, rather than have two separate thread pools that don't know
about each other (and probably twice as many threads as you have
cores)..
Incorrect use of reserve(), then writing past the end of the vector.
It should have been resize(). But it can be cleaned up even further by
using assign().
If udimfile is null, it would get dereferenced anyway. Was missing
a `return` in that case.
While I was there, I realized that ImageCacheImpl::inventory_udim and
resolve_udim had no reason to be virtual, so I removed that
qualifier. (It was probably a cut/paste error, as the
identically-named methods in TextureSystem are virtual, on
purpose. But they don't need to be for this class.)
All read_native_* methods of ImageInput are required to be thread-safe
with any other read_native_* call or with the varieties of read_tiles()
that take an explicit subimage and miplevel parameter.
JpgInput::read_native_scanline() does a seek_subimage() call internally,
so it's obviously not thread safe without additional locking.
The current description of IOProxy::pread() and IOProxy::pwrite() is a
little ambiguous. This commit expands the description to explicitly tell
what the functions are and are not required to do.
Should not have passed an argument. Went undetected for a long time
because I guess we didn't previously call the erase() method. (Until
I did when working on something unrelated.)
Virtual destructors don't significantly differ from regular virtual
functions, so it is customary to add override specifier when virtual
destructor is overridden.
Current code assumes that CMAKE_INSTALL_INCLUDEDIR and
CMAKE_INSTALL_LIBDIR has exactly one filesystem component. This is not
the case on e.g. Debian, where standard paths are as follows:
CMAKE_INSTALL_INCLUDEDIR=include
CMAKE_INSTALL_LIBDIR=lib/x86_64-linux-gnu
Fortunately the Config.cmake file is installed into a path that is
itself relative to CMAKE_INSTALL_LIBDIR, so we can get
CMAKE_INSTALL_LIBDIR from CMAKE_CURRENT_LIST_DIR and then get
CMAKE_INSTALL_INCLUDEDIR by applying relative path from
CMAKE_INSTALL_LIBDIR to CMAKE_INSTALL_INCLUDEDIR on top of that.
Fixes: c8dfbd7066
The current limit has been already hit when AMD released
second-generation Epyc processors (codename Rome) back in 2019. Top
models have 64 cores per CPU, or 128 threads. This means 2 socket
configuration has 256 threads.
Fourth generation Epyc processors (codename Genoa) are rumored to have
96 cores per CPU, or 192 threads. This means 2 socket configuration has
384 threads. While the CPUs have not been announced yet, it makes sense
to prepare for them in advance.
Revealed by clang-tidy. Use length-safe versions.
Also get rid of some dead code by disabling some memory-clearing
code that we don't need (we're not using SHA-1 for cryptographic
or security reasons, just for comparing images).
Python 3.8 stopped loading DLLs from PATH on Windows, meaning anything that relied
on that environment for shared library resolution breaks. This PR works around that
behviour by manually adding PATH back to DLL resolution using os.add_dll_directory()
in a new __init__.py which just re-exports everything in the current .pyd.
The new behaviour is enabled by default, but is easily disabled at runtime if the
environment variable OIIO_LOAD_DLLS_FROM_PATH is equal to "1".
Found this coincidentally during some address sanitizer runs.
Some tiff files out in the wild that have RichTIFFIPTC tags
inexplicably mark them as if the data was LONG instead of BYTE or
NOTYPE. I guess early in our history, we came across one of those and
thought it was correct, because we were always treating it as if it
came in the form of longs. But that's not always true, and in
particular, the size in the tag I think is always expressed in
bytes. So this could make for buffer overruns (generally harmless, but
caught by the sanitizer!).
So this patch cleans this all up.
And also we update a few sample images with bad RichTIFFIPTC tags to
strip out the in the incorrect form of the tag.
Recently, we added the ability to do stochastic sampling in texture
lookups, by choosing the MIP level stochastically rather than reading
two MIP levels and mixing them.
This PR adds the ability to also stochastically sample the anisotropic
filter footprint -- instead of multiple anisotropic samples, choose
just one sample well distributed over the footprint axis. On real
scenes, this seems to speed up overall rendering (not just texture) by
up to 2-3%, for scenes where average anisotropy is around 3-4.
It's much less dramatic a speedup than the stochastic MIP selection,
which sped up texturing by around 40% and in practice sped up overall
rendering by around 15% for us. Why doesn't cutting the number of
texture samples by another factor of 3 only speed up by 3% instead of
another 20%? Basically, it's because the stochastic MIP mapping *by
definition* means that each texture call touches half the number of
tiles as before. In contrast, those multiple probes along the
anisotropic axis are almost always within the same tile, and
frequently touch a very nearby pixel within the tile, so not only are
we not needing to look up a new tile from the tile cache, but also the
very memory we need to read is highly likely to be in L1$. But still,
we'll take 2-3% rendering gain any time we can get it, that's
equivalent to a lot of money if you have a really large compute farm.
Some other changes that come along for the ride:
Add a --bluenoise option to testtex (makes for prettier 1spp results),
and add tests for the aniso sampling.
Some internals changes to precompute min w,h of mip levels so there is
less per-texture-call rifling through the ImageSpec of each MIP level.
Sadly, this did not produce any consistently detectable speedup in
overall rendering.
And I changed the interface for stochastic texturing a bit. The old
way involved asking for new MIP modes, StochasticAniso or
StochasticTrilinear, in addition to being sure to set TextureOpt::rnd
to the stratified random number you are using for sampling. Now we are
deprecating StochasticAniso and StochasticTrilinear. Just use the same
old mip levels you always did. So the new interface can be summarized
as follows:
* On each texture call, setting opt.rnd >= 0.0f is itself a request for
stochastic texturing on that call (opt.rnd will default to -1.0, and
any negative value does the old fashioned non-stochastic filtering).
* A ShaderSystem-wide option "stochastic" (int) indicates which things
should be sampled stochastically, for those lookups that have rnd >= 0.
It's a bit field: 0 means no stochastic sampling (even if rnd >= 0),
1 means to do stochastic MIP level selection 2 means to do stochastic
anisotropic sampling (so 3 means to do both, and higher bits are reserved
for future extensions). The default is 0, so renderers need to enable
it to get any stochastic texturing.
Finally, let me give a reminder that you ONLY want to do stochastic
texturing for shading that involves multiple stochastic samples per
pixel, like orinary surface shades. For displacement (shade once per
mesh vertex) or "imager" (shade once per pixel), you just have a
single shade that you want perfectly filtered, so you probably don't
want to set opt.rnd >= 0 in those cases.
Previous code was doing ioread one pixel at a time. Change it to
read one scanline at a time, similar to e.g. how BMP reader does it.
On a 8192x8192 rgba8 image (256MB), on Apple M1 Max "iinfo --hash"
time goes from 2.19s down to 0.65s, and is now similar to the time
it takes on an uncompressed TIF or BMP image.
When viewing this .md file in Github UI, everything seems to work. But when creating an actual PR, the links to CONTRIBUTING and CLAs point to nothing that would exist. Looks like they try to be relative links from the "create a new PR" page. Workaround seems to be to use absolute links instead.
Implements support for BC4, BC5, BC6H (unsigned and signed) and BC7
GPU texture compressed formats in DDS reader (#2338).
- BC4: loaded into 1 channel uint8,
- BC5: loaded into 2 channel uint8,
- BC6H: loaded into 3 channel half,
- BC7: loaded into 4 channel uint8.
This is implemented by replacing whole of libsquish (~4700 lines of
code) with bcdec.h (https://github.com/iOrange/bcdec, ~1300 lines of
code). bcdec.h has an advantage of being faster, smaller and supports
all BCn formats for decoding.
bcdec.h license is MIT or Unlicense (aka public domain), updated all
the relevant third party / license files that I could find.
* Add a SECURITY.md detailing our security policy and the private
email address to contact to report vulnerabilities.
* Create scorecards.yml, to run OSSF scorecard to check supply chain
security issues. It has some false positives, but it's only advisory.
* Make sure actions we use don't get write perms on our repo.
* Use exact SHA's for actions versions to guard against hijacked
actions repos that might try to substutite something else at the
version tag we use.
* Tried to use harden-runner action to ensure no actions on the runner
overwrite source code or have unauthorized outgoing net connections
during the GHA run, but currently leaving it disabled because of too
many false positives -- it flags every dependency download as a case
of altering source code. Which it technically is, but it doesn't
matter for any jobs that aren't producing any artifacts that will be
consumed or relied on by users.
Learned about this from a security talk.
On Linux, we'd been using gettimeofday, which gives results with usec
resolution. Switch to clock_gettime, which has nsec results.
In practice, the round trip time of a pair of "get time" system calls
is about 30-50ns in either case. But the new call at least nominally
returns ns resolution values. Another way of saying that is: the
answers we get should be more accurate with this call, but the call
itself is the same expense as before, so we should still be just as
cautious about not inserting timers around events that are so brief
that the timers themselves are going to be a significant addition to
the cost.
This only effects Linux (and maybe BSD?). On MacOS, we use
mach_absolute_time(), and on Windows we use QueryPeformanceCounter(),
so neither of those platforms is affected by this patch.
Oops, calling it "m_max_tile_channels" (the name of the member
variable) instead of "max_tile_channels" (the name of the attribute)
actually prevented properly setting or retrieving the attribute by
name.
ustring is a unique string (that lives in an internal hash table) whose
representation is the pointer to the characters that live in the table.
ustring hash is its alter ego -- it also refers to a ustring in the
internal hash table, but the representation is just the hash itself.
It presents an interface that is a subset of that of ustring, and in
many ways they can be used interchangeably. For ustringhash,
accessing the characters requires using the hash to find the string in
the table -- which is not hard, but more expensive than accessing the
charcters with a ustring, where the representation is the pointer
directly to the characters.
For some uses where you don't need access to the characters in any
performance-critical paths, this may be a more convenient
representation. In particular, it's well suited to a GPU that doesn't
have access to the character memory. Another interesting difference is
that from run to run, a ustring may have a different literal value,
since there's no reason to expect that the pointer to a string like
"foo" will refer to the same memory location every time the program
executes, but in contrast, the hash is guaranteed to be identical
from run to run.
In the process of adding unit tests for ustringhash, I ended up
overhauling all of ustring_test.cpp, as I saw that the ustring tests
were extremely cursory. Did a tiny bit of (non-breaking) touch-ups
on a few ustring methods as well.
If the BMP image we're reading declares a certain sized palette, but
then has pixels that contain values outside the range of that palette
size, we would read past the end of arrays.
With a 1x1, 2bit image of a Targa 1.0 file, the entire image was only
22 bytes, so the seek to 26 bytes before the end to look for the
Targa 2.0 data... obviously failed.
Don't look for the Targa 2.0 block if the file itself is not long
enough to hold both the header (18 bytes) and the block (26 bytes).
This tiny change is a quality-of-life fix for me, addressing the fact
that there are two different simd.h files in our code base. I never
would have done that on purpose, but one of them is part of the
imported "squish" source code that we use for dds encoding.
For years I've been frustrated that reflexively typing `CMD-P simd.h`
in the editor has a 50/50 chance of pulling in this file, when in fact
there is literally a 100% chance of my wanting
src/include/OpenImageIO/simd.h.
So finally put my editing brain out of its misery by renaming the file
that I never want to edit.
When doing thread "wedge" tests, most of the time spent is for the
1-thread case. Who cares about 1 thread anyway? So we make two
improvements with new command line options:
* --minthreads sets the minimum number of threads that will be
used. For example, `--minthreads 2` will skip the single-thread case
and start the wedge with the 2 thread case.
* --lowtrials is an optional maximum number of trials just for the 1
or 2-thread case (overriding --trials just for those thread counts,
if it is a lower value).
By default, the old behavior is still the default. But these allow a
bit of customization in the tradeoff of how much time to invest in the
very low thread counts in a wedge of thread timings.
You'd think there would be no way that basic failure like this could
occur after 14 years, right?
But the TIFF reader code was a little mixed up about the fact that the
internal m_next_scanline field is relative to the starting scanline of
the image, not an absolute value. But in the case of the code that has
to deal with handling the stray scanlines at the end of a strip-read
file neglected to set it properly and the end of the stripped part.
Obviously, for TIFF files with a first scanline of 0 (the vast, vast
majority), failing to adjust by this offset doesn't matter. And also
as long as the reading continued to be strictly sequential, it also
was asymptomatic. But in the presence of certain multithreaded random
access patterns, it makes a difference. Oops.
The udim_lookup_mutex_pool is not necessary. Just using an atomic
pointer for udiminfo.icfile is enough.
I'm seeing about a 5-8% overall speed improvement in testtex-based
intense udim benchmarks under heavy threading, a ~2% overall
improvement in a real rendered scene that uses a bunch of udim
textures (I think there are bigger bottlenecks elsewhere that I'll
move on to next, but every little bit helps).
Some testtex enhancements coming along for the ride:
* UDIM-oriented tests (automatically decuded if the test textures
contain `<UDIM>` in their name.
* --maketests will construct random test textures and use them (handy
if you need lots of textures for a big test, but don't want to store
them). The tests are based on a file pattern using std::format-like
replacement with an index. Also, if the pattern contains `<UDIM>` in
the name, it's understood to be a udim pattern and many individual
udim piece files will be created.
An example of these two items in action is:
```
testtex --maketests 200 "tmp/foo{:04}_<UDIM>.exr" --threadtimes 8 -iters 100000000 -runstats -v --handle -maxfiles 1000 -trials 5 -wedge
```
fmt master seems to have just deprecated the ability for
fmt::sprintf("%s") to default to printing anything that allows stream
output. This breaks use of Strutil::sprintf formatting ustrings.
This isn't yet in any tagged release of fmtlib, but I'm trying to get
ahead of it.
The quick fix is to just change those handful of places to use
std::format style calls, which we are slowly converting to anyway.
Also throw in a fmt::formater for ustrings for good measure.
It's a lot of data, you don't need it if you don't want to run the
full testsuite, and a lot of places and distros have policies against
live downloads happening in the build step. Also, downstream projects
that need to build OIIO for their CI or other reasons don't want to
accidentally download a lot of test data they won't need.
So change the default of OIIO_DOWNLOAD_MISSING_TESTDATA to OFF.
Build with -DOIIO_DOWNLOAD_MISSING_TESTDATA=ON to turn on auto
download of the data images, or else run
src/build-scripts/install_test_images.
Now, this does have the following danger: That people who build OIIO
and do want to run the tests -- and don't know to turn this option
back on -- will end up with very few tests able to run.
We largely address this by observing that quite a lot of our tests
that depended on oiio-images actually depended only on one file
(grid.tif, which is not especially large). So by making a copy of that
file in testsuite/common, quite a few additional tests can run even
without oiio-images.
Though, to be clear, there are still many tests that want oiio-images,
particularly tests that validate support for various image file
formats, which often depend on large collections of images that are
stored in oiio-images. So remember, if you are developing OIIO, the
best policy is to run the whole testsuite. But on the other hand, if
you submit a PR, they'll all be run in CI as well.
For failed CI builds, save the cmake caches in a safe place, don't zip
them up in situ, because then when I unpack them in my build area
locally, they overwrite my working cache.
The Intel OneAPI LLVM-based C++ compiler, as we download it from Intel
for our CI, bumped from 2022.0.0 to 2022.1.0.
New warning popped up -- newest Intel LLVM-based compiler warns about
overriding certain floating point math flags.
Also had to adjust thresholds on a couple texture tests that have a
few pixels change slightly due to numerical differences.
Another round to hide Imath externally to OIIO public headers as well
as speed up our own builds.
* Remove the external reference to Imath::M44f from color.h, and instead
use M44fParam.
* The `<OpenImageIO/Imath.h>` includes the most commonly used Imath
headers including half.h. But this is unnecessarily expensive (and
presumptuous about wanting other Imath types as well as fmt.h) if
you just need to know about `half`. So we create a new
`<OpenImageIO/half.h>` that only includes what you need for `half`
(as before, with logic to handle the changing location of the half.h
header depending on whether you're using Imath 3.x or OpenEXR 2.x).
* Several files that included Imath.h, but in reality only needed half
all along, change to including OpenImageIO/half.h.
* simd.h leans more on vecparam.h and other template trickery to
reduce exposure of methods that only work in the presence of Imath
headers, and make fewer use cases that would care whether the Imath
headers are included before or after simd.h (which is less
controllable for unity builds).
* Vec3Param gains a `cast<V>()` templated method that allows easy
casting to foreign vector types even if Imath headers are not
included prior to vecparam.h being included (which is less
controllable for unity builds).
We have a template OIIO::bit_cast which has served us for a while, but
it turns out that C++20 has std::bit_cast which operates ALMOST the
same -- unfortunately, the tempalte arguments are reversed from the
way we did it!
So here we:
* Set up a new `bitcast<>` that works exactly like C++20 std::bit_cast.
* Implement our old bit_cast in terms of the new one, for now.
* Set up `#if` trick so that the confusing OIIO::bit_cast will be
marked as deprecated beginning with OIIO 2.5, and will be removed
entirely from OIIO 3.0. (Farther out than that, we may choose to
re-introduce bit_cast as truly being identical semantics to
std::bit_cast. But before that is possible, we need to completely
ween downstream users off the old one.)
* Took the opportunity to expand the set of compilers for which
certain bitcast overloads use Intel intrinsics. Previously, we only
did this for the Intel compiler, but it seems to work for modern
versions of clang and gcc as well. Do a little bitcast testing in
fmath_test.cpp to verify that the use of these intrinsics doesn't
break for any of the compilers we use for CI.
Over the span of several individual PRs, we've made the code base safe
for "unity" (a.k.a. "jumbo") builds -- a feature of CMake that
automatically combines multiple .cpp files into a single compilation
unit (basically making temporary .cpp that `#include`s each original
cpp) in order to, allegedly, speed up compilation by not re-parsing
headers multiple times, and other sources of per-module overhead.
We discovered early on that on a modern computer where modules are
built in parallel (another automatic feature of CMake), jumbo builds
are simply not an overall performance improvement for OIIO. Basically,
the efficiency gain from removing redundant parsing is mostly offset
by the loss of parallel efficiency -- fewer and larger modules means
there are periods where all cores are not able to find work to
do. (Reductive example to illustrate the basic problem: If you have 8
independent modules to compile, and each takes 1 second, 8 cores can
compile the entire collection in parallel in 1 second. But a unity
build that batches those files in groups of 4 and achieves a 2x
speedup, thus compiling each set of 4 modules in 2 seconds instead of
4, only can do two of these compilations in parallel, so it takes 2
seconds. We've cut the total work in half, and yet it takes twice as
long in real time to complete, and 6/8 of your cores are idle.
OK, but that's when you have several cores. If you had only one core
(or maybe two) so you weren't using much parallelism in the first
place, then the unity build would look like a win, since you'd really
see the improvement from the amortized overhead of the combined
modules.
Most of us don't have single core machines on our desks, or laps, or
even in our pockets (or on our wrists?).
But we do have single core machines (or close to it) in our CI runners!
We get 1.5-2 cores, roughly, in the slice of VMs we get to run CI on
GitHub Actions. So maybe this is the one place where a unity build would
be of benefit.
That's the theory anyway. Does it pan out? I tested it, and the answer
is basically yes. I can speed up the "build" step of our CI by
somewhere in the vacinity of 30% (varies a LOT from run to run). I
tried a whole bunch of different approaches, summarized in the table
below. I picked the best basic strategies revealed by those tests and
set up the CI to use them. (I didn't preserve all the other
permutations.)
| Name | try 1 build | try 2 | try 3 | try 4 |
|:----------------------- | ----------- | -------------------- | -------------- | -------- |
| **linux gcc9 BASELINE** | 14:19 | 11:46 | 11:56 | 13:32 |
| linux gcc9 group | 9:39 | 8:06 | 9:58 | **7:58** |
| linux gcc9 batch 8 | 8:06 | **7:17** | **8:02** | **7:59** |
| linux gcc9 batch 16 | 8:29 | 7:32 | **7:56** | 9:35 |
| linux gcc9 batch 4 | 8:39 | 8:38 | 8:52 | 10:31 |
| linux gcc9 batch 2 | 13:34 | 12:43 | 11:22 | 9:32 |
| linux gcc9 batch mix 4 | 7:22 | 9:14 | 9:01 | 9:39 |
| linux gcc9 batch mix 2 | 9:11 | 7:45 | 10:55 | 9:36 |
| **linux icx BASELINE** | | 14:39 | 15:07 | 15:06 |
| linux icx group | | 11:59 | **10:36** | 10:26 |
| linux icx batch 8 | | **9:40** | 11:31 | **9:14** |
| linux icx batch mix 4 | | 10:06 | 11:58 | 11:16 |
| **macos11 BASELINE** | 25:41 | 27:53 | 26:30 | 28:32 |
| macos11 group | 10:56 | 20:24 | 19:28 | 19:24 |
| macos11 batch 8 | 16:43 | 17:38 | **18:34** | **17:42** |
| macos11 batch mix 4 | | **9:22** | 19:12 | 18:26 |
| **windows BASELINE** | 20:17 | 23:27 | 28:43 | 19:13 |
| windows group | 23:05 | 15:57 (tests broken) | 21:57 (broken) | 15:33 |
| windows batch 8 | 21:16 | 15:57 | **17:38** | **15:08** |
| windows batch mix 4 | | **15:30** | 23:19 | 15:16 |
Notes:
* Timings vary wildly from run to run. You never know which VMs you'll
get or how overloaded they are. But over 3 trials, the trends were
clear.
* I rigged it to clear the ccache, so these are FULL uncached builds.
* This is only "build" time. The CI run also includes time to set up the
VM, download docker images, build or install dependencies, and of course
run the testsuite. So the overall speedup of the entire CI run will not
be as improved overall as these numbers indicate for the build step.
But still, saving several minutes per job means the CI tests turn
around faster, and that's what we want.
For C++17 and beyond (and maybe earlier in some compilers), we already
used std::gcd. Now for C++14 compilers without std::gcd, we roll our
own simple gcd implementation instead of using boost::gcd. Maybe it's
slightly slower, but this is nowhere near a performance-critical
path. It's worth it to take one more step at eliminating boost.
Reminder: When opening a file by name, first it tries to open assuming
that the file's extension indicates the format. If that doesn't work,
it tries all known format readers to see if one will work.
This patch includes several improvements to this process and surrounding
code in imageioplugin.cpp:
* When trying all formats because the extension was wrong, instead of
traversing the input_formats map in alphabetical order of the list
of all format names + extensions, try the three most common formats
we encounter (openexr, tiff, jpeg) FIRST, then try the other formats
in alphabetical order. We do this by traversing the
format_list_vector, which contains all format names in the order
they were declared, and ensure that those common formats are
declared first. Basically, this means that if the mislabeled file is
one of those most common formats, the correct reader is found after
only a couple tries, instead of having to try a dozen or more bad
guesses of rarely encountered formats.
* Debug mode messages that make it easier to understand which readers
are being tried for which files.
* Comments clarify that input_formats and output_formats hold not just
the map from format names to creators, but also map extensions to
creators. (That confused me while I was debugging this, so I figure
the clarification is needed.)
* Establish a new extension_to_format_map that lets you get the format
name if you know the extension.
* Get rid of input_extensions and output_extensions maps, which weren't
ever used.
* Some other minor refactoring.
If you know the hash of a ustring, this tells you the ustring.
(Caveat: in the very unlikely chance of two ustrings having the same
hash, this will return the first one it finds in the table.)
I realized that --attrib (and several related actions) only set the
attribute as directed for the first subimage if it's a multi-image
file (such as a multi-part exr), or all of them if the -a flag is
used. But there was no ability to set it for a particular (non-first)
subimage, like many other oiiotool actions have the ability to do.
So in this patch, I refactor the following commands to use the
OiiotoolOp and related macros, which automatically make it responsive
to the `:subimages=` optional modifiers:
--attrib
--sattrib
--eraseattrib
--caption
--orientation
--clear-keywords
--iscolorspace
As a bonus aside, one thing this lets you do is set a different
compression method per-subimage using, e.g.,
`--attrib compression:subimages=3 dwaa:100`.
But if you set compression with `--attrib compression`, do not also
use `--compression`, which will override it and also apply to all
subimages.
When this symbol is defined, this marks as deprecated the ErrorHandler
class methods that use sprintf formatting notation. It's not enabled
by default, but a downstream project can define the symbol in order to
help root out any uses of the old style formatting functions, to aid
identifying and changing those spots to the new style, long before
they are truly deprecated or removed from OIIO.
This change also sets up an OIIO_INTERNAL symbol that will be present
when OIIO itself is compiling, but not a downstream project using the
OIIO headers.
This is the first tiny step to fixing all the little things that break
in a unity build. This one is pretty small fry -- the oiiotool set of
modules only needed some minor rearrangement of header file include
order.
* Initial implementation of `ImageBufAlgo::st_warp`
* Simplify default filter logic, and share it with `ImageBufAlgo::warp`
* Add st_warp to oiiotool
* Add tests for bespoke `st_warp` validations that are done before IBAprep
* Add st_warp test to oiiotool-xform testsuite
* Address review notes
- Remove ST buffer datatype guard
- Simplify additional ROI prep
- Remove unnecessary buffer reallocation
* Use sample accumulator type for source/dest iterator USERT
* Add `--st_warp` section to `oiiotool.rst`
Cut/paste error inadvertently deleted a critical cmake line that I
wasn't intending to alter or remove, that resulted in the command line
utilities not being tagged for part of the install step. Oops!
Oh boy, never leave anything unbenchmarked.
Turns out the boost::algorithm functions we were relying on underneath
many Strutil "case-insensitive" comparisons were ridiculously slow.
We thought they were good enough because they allowed specification of
locale, so we could just pass the static classic locale, and so they
would be inexpensive because they didn't have to query the current
locale. But this is wrong, they were still ghastly.
So here I rewrite Strutil::iequals, iless, starts_with, istarts_with,
ends_with, iends_with in terms of a new (internal) Strutil::strcasecmp
and strncasecmp (which underneath handle differences in platform, and
use the locale-independent versions). The net result is that most of
those case-independent comparisons speed up by a factor of
50-100x. Wow.
I still need to tackle the family of 'ifind' related functions. They
are a bit trickier. But I'll leave them for another time, because I
need to roll this present fix out right away to fix a real production
bottleneck.
(Worth noting: iequals is instrumental when you're searching a
ParamValueList for a particular name, which is what happens when you
look up attributes from an ImageSpec, which is what happens when you
call get_texture_info(), which is what underlies OSL gettextureinfo()
calls in the cases that they cannot be constant-folded during runtime
optimization. So this came to my attention when analyzing a slow scene
whose shaders had a pathological explosion of gettextureinfo calls that
couldn't be optimized away.)
Reminder: isConstantColor() finds out if an ImageBuf is constant
color. It uses the thread pool to check several areas in
parallel. Once any task finds that its subregion is not constant, it
sets a flag so that as other subregions are pulled off the task queue,
they will take an early out if one of their siblings already knows the
image is not constant.
Turns out this isn't much of a big savings in practice, because lots
of regions may be in flight by the time one region realizes it's
non-constant, and once a region task starts, it doesn't check the flag
again.
This patch speeds up isConstantColor by a pretty big factor for
non-constant images (it can't really help for images that ARE
constant) in the following ways:
1. Before doing anything, it checks to see if the FIRST TWO pixels of
the image are unequal. If that's the case -- and it's a fairly
common case for non-constant images -- then it immediately knows
the whole image is not a constant and doesn't need to look at
anything else or launch any of the threads at all. (If it is a
constant image, or if the first two pixels are just coincidentally
identical, this quick check of only two pixels didn't really cost
us any masurable time.)
2. In addition to checking the result flag at the start of each
parallel job, now we also check if the flag has been changed as we
iterate to each pixel. So pretty much as soon as one thread finds
unequal pixels, the other threads can spot this and stop their
tasks early, no need to finish looking at their entire region. (To
make the more frequent checks of the flag inexpensive, I changed it
from an atomic to just a regular bool. It doesn't need to be
atomic, because the strict ordering is not important at all here
and it doesn't change the result if the threads' access to the flag
is unsynchronized.)
When outputting OpenEXR files, in a case where the "compression"
request was for "dwaa" (no compression specified in the main name,
like "dwaa:100") we accidentally forced a default quality level of 4,
which is the default we wanted for zip compression. But that's not a
good default for dwaa!
As I learned recently, a "unity" (aka "jumbo") build is where multiple
.cpp files are combined into one translation unit -- imagine a unity.cpp
that simply has
#include "file1.cpp"
#include "file2.cpp"
...
#include "fileN.cpp"
and so you compile unity.cpp instead of the separate file?.cpp files
individually.
Turns out that CMake understands this concept and can do it for you
automatically!
The benefit of a unity build is that file1...fileN probably include
most of the same headers, expand the same templates, etc., so a bunch
of the per-file work of the compiler can be done once rather than
redundantly for each file.
There are two potential downsides, however:
1. It may not be safe to concatenate your cpp files! For example, if
both file1.cpp and file2.cpp each contain a `static int foo;`, that
may have been safe when compiled separately, but is not allowed to
happen twice in one compilation unit. Similarly, if you have headers
that don't have proper guards against multiple includes, etc. So one
should expect a whole lot of little fix-ups are needed for this to
work properly. (We'll come back to that topic.)
2. Combining source file into these "jumbo" modules can make heavily
parallelized builds on many-core machine not be able to load balance
or keep all the cores busy. (Simplified examples: if you have 16 .cpp
files on a 16 core machine, each core can compile one cpp file in
parallel with the others. But if you mash the modules into just one
huge cpp file, give that to one core, and your other 15 cores are
idle, so the full build probably takes much longer.)
I tried this out, including the many fixups implied by (1) above, and
at first the unity builds were slower on both my laptop (8 cores) and
workstation at work (32 cores), because of downside (2) explained
earlier -- harder to take advantage of parallel builds when there are
fewer, bigger, compilation units. Some tweaking of strategy got me to
the point where I could always get the unity builds to go faster, but
not by a whole lot when many core were available. Slighty faster, but
not worth the trouble. A bit disappointing, nearly abandoned the whole
idea.
HOWEVER, in situation where you are limited to very few cores (like in our
CI, which allocates 2 cores, and it sure seems more like 1-1.5 for the
Windows and Mac CI runners), the unity builds are substantially faster --
there's already not much parallelism to exploit, so you come out ahead
with the savings of that redundant per-file work we talked about.
So my current thinking is to go ahead and make the changes that allow
unity builds. I don't particularly recommend them when highly parallel
builds are available to you, but it might help to cut our CI
turnaround time down on the GitHub runners. And maybe it will help in
other situations for other people.
Ok, then. The present patch introduces these concepts and makes the
CMake and other build system changes to allow unity builds. (N.B. It
won't WORK yet, so don't try it!) After we get that out of the way, in
subsequent PRs I'll show all the changs to the code that were
necessary to fix all the little things that went wrong when source
files got combined.
* Fix warning from duplicate define
* Fix warnings about bool/int expressions
* Fix discarded result warning in PSD input
* Fix codepage warning from unicode symbol in string constant
* Ensure the term object is initialized with the stream we intend to output to
* Fix pointer casts in paramlist test
Signed-off-by: Chris Kulla <ckulla@gmail.com>
Co-authored-by: Larry Gritz <lg@larrygritz.com>
In FFMpegInput::close(), some of the tear-down calls would happen
regardless of if their pointers were already null. Seemed safe, but
maybe not always? So check the pointers.
Fixes#3371
When trying to break filenames into the base name and any "RESTful"
arguments (such as "/foo/bar.exr?name=value&dog=cat"), we were
just looking for presence of the "?" to determine if it was a RESTful
thingie and using that as the split position.
But this is thrown off by an odd Windows notation where names that
start with (get this) "\\?\" somehow designate long filepaths. See
See: https://docs.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation
So the solution is to change Strutil::get_rest_arguments to explicitly
check for this pattern at the start of the string.
In OpenEXROutput::write_deep_scanlines and write_deep_tiles, don't
just pass the DeepData along -- handle the case of where the DD (the
buffer the caller is presenting) does not match the data types that
you said the file is going to hold. In this case, we need to make a
local temporary DD that holds a copy of the passed-in data, but with
the formats transcoded.
Some changes to DeepData were done to support this.
GHA is retiring the windows-2016 image, we won't be able to use it soon.
Also bump openexr versions in a couple places.
There's some in-progress work on a VS2022 test, but ignore that for now,
it's commnted out.
I'm a total convert to fmt (and/or std::format, std::print) and slowly
converting all prior use of sprintf and iostream to it.
If I print an Imath::V3f or a simd::vfloat3,
fmt::print ("v = {} {} {}\n", myvec[0], myvec[1], myvec[2]);
That's ok, I guess, but it's clunky and a big pain to put the format
specification for every element separately if it were a vfloat16,
say. I would like to be able to do this:
fmt::print ("v = {}\n", myvec);
and in fact, it does print all elements (thanks to some nice behavior
by fmt)! But this does not work as expected:
fmt::print ("v = {:.3f}\n", myvec);
fmt's tricks will print all elements, but only using the default format
spec, and the spec above applied so the "whole thing", not each element,
if you see the distinction.
So this patch sets up the custom formatter infrastructure to be able
to print array-like things (and in particular, the Imath and
OIIO::simd vector and matrix classes) by specifying the format spec
used for the elements.
The important elements to note:
* detail/fmt.h adds custom formatter helpers for (a) any class with
operator[] and size() ("index_formatter"); (b) any class that lacks
these but nonetheless has an array-like memory layout and a known
length ("array_formatter"). I also added a nonstandard extension to
the format spec for these, which optionally lets you use a comma
separator rather than just the space between elements.
* These building blocks are used to specify custom formatters for
the commonly-used Imath types (in our Imath.h) and our internal
SIMD vector types (in simd.h).
* simd.h also adds a few other goodies, including an `is_simd<>`
template, and `size()` constexpr methods to each vector type. These
make certain template metaprogramming tasks easier for these types.
* A new "type_traits.h" header for collecting a bunch of little utility
templates, such as `has_subscript<>` and `has_size_method<>`.
JPEG expresses pixel aspect ratio implicitly as header fields
"xdensity" and "ydensity" (which we call "XResolution" and
"YResolution" in our metadata after the TIFF names). We also have
"PixelAspectRatio", so the three of them, if they are all used, form
an over-constrained system. Plus, they may come both in the JPEG
header and also (not necessarily having the same values) in any Exif
block.
And on top of that, the way Nuke and PhotoShop infer the aspect ratio
is BACKWARDS! (If pixelaspect = width/height of a displayed pixel,
then it's also = ydensity/xdensity. But Nuke is the other way
around. Only for JPEG, it gets it right for TIFF and OpenEXR.)
Anyhoo, there were some edge cases in which we were still getting
confused (specifically, that we were making some files that Nuke was
not properly recognizing as anamorphic). This patch tightens up
these cases.
Create a small shim class template called
Vec3Param (and its specialization, V3fParam) that does nothing but
implicitly convert from and to things that looks like 3-vectors
(thanks to some template magic I developed a while back for the Imath
project). And similar for template MatrixParam (and specializations
M33fParam and M44fParam) to pass a square matrix.
Basically the idea is that we can replace the public API function
parameters where we take an Imath::V3f and instead use a
V3fParam. User code that passes an Imath::V3f still works because
V3fParam accepts and converts it. Implementation code can implicitly
convert the V3fParam to an Imath::V3f, so it doesn't need to be
changed, either. Only the function declaration itself. (Same basic
situation for MatrixParam and Imath::M33f/M44f.)
So in the process, now we have laundered our public API so that it
doesn't reference the Imath types and headers any longer. This confers
two big advantages:
1. It allows OIIO itself and any downstream client code calling OIIO
to use *different* versions of Imath if they want.
2. It allows client code to not need Imath at all, for example if they
have their own proprietary vector type that they prefer to use
internally.
I'm not necessarily 100% sold on this approach, but I think it's got
promise and am seeking feedback. It seems to work, and I was able
to compile OSL against this altered OIIO with *no changes*.
The use of ends_with instead of iends_with made it so that
capitizalized files (especially on case-independent filesystems like
Windows or some Mac filesystems) were rejected.
(And yes, I did grep the whole code base for ends_with to be sure that
this was the only place we made this mistake.)
Fixes#3362
icx is the *new* Intel LLVM-based compiler, and it behaves a lot like
clang (versus icc, which is the "classic" Intel compiler).
* An additional CI test in ci.yml for icx.
* Tweaks in compiler.cmake and platform.h to detect and set options for
this compiler.
* A variety of warning fixes encountered for this compiler.
* Add a new cmake build-time option `ENABLE_FAST_MATH` (default: OFF)
for enabling fast-math optimizations that trade IEEE compliance for
speed. The Intel compiler enables it by default (!), so this build
option makes icx behave like the other compilers (not fast math
unless explicitly asked for), which makes the CI tests and other
behavior more consisent across compilers and platforms.
* Note: The bug that prevents some versions of fmtlib from working
with icc does not appear to be a problem for icx.
I finally learned how the "strategy" stuff works and refactored our
CI workflow based on it.
In short, we previously treated each CI test as its own universe, with
a huge amount of repeated build logic. These changes cut down on
redundancy by, for each set of related tests, collapsing them into
just one recitation of the build script logic and a handful of
variables that create the different test variations.
Currently, the categories are (a) all tests based on ASWF containers and
corresponding to VFX Platform specs, (b) "off-year" combinations based
on the native Ubuntu installations, (c) Mac tests, (d) Windows tests,
(e) the clang-format checker.
This refactor cuts the size of the ci.yml file by more than 50%, and
makes it much easier to add or change individual test configurations
in the future.
Strutil::print wraps fmt::format to render the formatted output into a
std::string, then sends the string to Strutil::sync_output(), which
ensures that every string printed (or sent to a FILE or stream) is
done so atomically, using an internal mutex, and with a stream flush
after each string. But this makes it a lot more expensive than it
needs to be, when you don't need such care -- either because buffered
output is ok, or because you don't need the thread safety.
This patch:
* Adds an optional `flush` parameter to sync_output(), defaulting to
true, which controls whether sync_output flushes or not.
* Introduces Strutil::sync::print, which is the same fully
synchronous, thread-safe, non-buffering behavior that Strutil::print
has traditionally had. Use this when you need those features.
* Redefines Strutil::print to be an alias for fmt::print, which means
- There is one fewer layer of template expansion and generated code,
and possibly fewer internal string copies, so that should make for
both faster compilation and faster runtime performance.
- Results are buffered, no sync after every output, so much faster
when you don't need that.
- No internal mutex.
On the platforms we use -- and with the current implementation of
fmtlib -- fmtlib's underlying use of stream and FILE output appears to
be adequately thread-safe. But we can't 100% guarantee this on all
platforms or all future versions of fmtlib, so users who need the
stronger guarantees (and are willing to sacrifice performance) can
simply use sync::print.
* New CI test downloads the Intel icc compiler and builds with it.
* A variety of minor fixes to address warnings from icc that were
not apparent from the other compilers.
* Some minor changes to just a few tests and reference output where
the icc-built compiler produces slightly different (usually LSB)
results.
These are cases where we declared a template, then later gave its full
definition, and we marked the declaration of the OIIO_FORCEINLINE but
omitted it from the later definition.
This addresses some compiler warnings from icc.
Slight reworking of -Werror logic to let the default behavior be
changed by setting a cmake variable. In particular, this allows us to
make a CI build disable -Werror when we need to.
Previously, having the "OpenmageIO_CI" env variable set trumped all
else and forced -Werror. Now it just contributes to the default behavior,
but setting `-DSTOP_ON_WARNING=OFF` and cmake config time can still
disable it.
Seemed like a good idea when we wrote it, but subsequently,
std::string_view was accepted into C++17 without a str() method. So
for the sake of future full interchangeability with std::string_view,
stop using this method. We will eventually deprecate it.
Special case some methods of ParamValue, ParamList, and
ImageSpec::attribute for ustring and string_view data, to reduce some
redundant copies and/or ustring conversions.
The internal method init_noclear() is augmented with a flag that allows
the caller to specify that any string data it might point to is already
ustrings, and thus does avoid the costly default behavior of converting
strings in the data block to ustrings. (This is not publicly exposed, so
users can't screw it up.)
So the new ParamValue constructor directly from a ustring can use this
flag to avoid passing the already-ustring through the string-to-ustring
conversion a needless second time. But also, the copy constructor, move
constructor, and assignment can use the flag and avoid the extra
conversion, since by definition they are copying from the contents of an
existing ParamValue which is guaranteed to be already ustring-ified.
While I was at it, I turned the optional arguments of ParamValue
constructors into "strongly typed" flags, deprecating the ones that
take a raw bool, which might be confusing.
c_str() was cute, but it's a little dicey and it's not present in
the C++20 std::string_view. We didn't use it in a ton of places,
so replace with other idioms so that in the future we are better
able to interchange with std::string_view or replace our own entirely
once we have flying cars and C++20 minimum.
* Allow ustring and OIIO::string_view to auto-cast to fmt::string_view
(helps some fmt formattting of types without custom formatters).
* When std::string_view (C++17) and/or std::experimental::string_view
(some compilers pre-C++17) are available, add OIIO::string_view
auto-construction from and auto-casting to those types.
...and stop using them internally as well.
For some of the IBA functions, we were unable to fully mark them as
OIIO_DEPRECATED (so instead that hint is commented out) because the
function signatures of the deprecated and new ones can be
ambiguous. We're just going to have to delete those when we are at a
compatibility break point, without going through a period where they
are present but marked as deprecated.
OpenImageIO has been accepted to be an ASWF-administered project, by vote of
the ASWF Technical Advisory Council (TAC) on May 3, 2023.
At this time, the ASWF subdirectory contains *proposed* (and sometimes TBD/WIP
placeholders) project organization and policy documents that will be formally
adopted as the OpenImageIO project is formally turned over to ASWF and moved
to the AcademySoftwareFoundation GitHub account. Please file issues or PRs
or review comments against these documents to make any suggested changes while
we are revising the drafts.
Summary of the current state of documents:
| File | Status | Notes |
| ---- | ------ | ----- |
| CLA-corporate.md | Final | Standard ASWF corporate CLA, approved by the TSC. Will take effect when the repo moves. |
| CLA-individual.md | Final | Standard ASWF individual CLA, approved by the TSC. Will take effect when the repo moves. |
| CODE_OF_CONDUCT.md | Final | Links to the standard LF CoC, approved by the TSC and now in effect. |
| CONTRIBUTING.md | Preliminary | Modified version of our existing CONTRIBUTING guidelines, lightly amended to reflect the changes that will come along with moving the repo, the new CLA/DCO rules, etc.
| GOVERNANCE.md | Final | Approved by the TSC and now in effect. |
| project_intake.md | Ongoing | This is the latest ASWF project intake checklist. We will check off items as we get them done. |
| Technical-Charter.md | Final | Approved by the TSC. |
This is a checklist for TSC's to review as part of the intake process. The TSC should review this entire list during the kickoff meeting. For anything outstanding, create an [issue](../issues) to track and link to it in the list
- Existing Project Governance
- [X] A [LICENSE.md](../LICENSE.md) file in every code repository, with the license chosen an OSI-approved license.
- [X] Any third-party components/dependencies included are listed along with thier licenses ( [THIRD-PARTY.md](../THIRD-PARTY.md) )
- [X] A [README.md](../README.md) file welcoming new community members to the project and explaining why the project is useful and how to get started.
- [X] A [CONTRIBUTING.md](../CONTRIBUTING.md) file explaining to other developers and your community of users how to contribute to the project. The file should explain what types of contributions are needed and how the process works, along with how to disclose security issues responsibly ( may also point to a [SECURITY.md](../SECURITY.md) file ).
- [ ] A [CODEOWNERS](../CODEOWNERS) or [COMMITTERS](../COMMITERS.csv) file to define individuals or teams that are responsible for code in a repository; document current project owners and current and emeritus committers.
- [X] A [CODE_OF_CONDUCT.md](../CODE_OF_CONDUCT.md) file that sets the ground rules for participants’ behavior associated and helps to facilitate a friendly, welcoming environment. By default, projects should leverage the Linux Foundation Code of Conduct unless an alternate Code of Conduct was previously approved.
- [X] A [RELEASING.md](../RELEASING.md) file that provides documentation on the release methodology, cadence, criteria, etc.
- [X] A [GOVERNANCE.md](../GOVERNANCE.md) file that documents the project’s technical governance.
- [ ] A [SUPPORT.md](../SUPPORT.md) file to let users and developers know about ways to get help with your project.
- Infrastructure/Assets
- [X] Technical Charter Approved
- [ ]**(Awaiting scan from LF)** 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] 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 )
- [X] Setup new lists ( -dev@ and -tsc@ ) on [ASWF groups.io](https://lists.aswf.io) ( create [issue on foundation repo](https://github.com/AcademySoftwareFoundation/foundation/issues/new) to setup )
- [X] (In progress) Move to [ASWF groups.io](https://lists.aswf.io) ( create [issue on foundation repo](https://github.com/AcademySoftwareFoundation/foundation/issues/new) to transfer )
- [X] Slack ( create [issue on foundation repo](https://github.com/AcademySoftwareFoundation/foundation/issues/new) to setup project channel on [ASWF Slack](https://slack.aswf.io) )
- [ ] 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] (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
- [X] TSC members identified
- [X] First TSC meeting held
- [X] TSC meeting cadence set and added to project calendar
- [X] CLA Approved ( if used ) ( [CCLA](CLA-corporate.md) and [ICLA](CLA-individual.md) )
- Project charter ( [Technical-Charter.md](Technical-Charter.md) )
- [X] [Core Infrastructure Initiative Best Practices Badge](https://bestpractices.coreinfrastructure.org/) achieved as the 'Passing' level.
- Outreach
- [ ] New project announcement done ( create [issue on foundation repo](https://github.com/AcademySoftwareFoundation/foundation/issues/new) to trigger )
- [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
- [ ] 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?
- What does the next year look like in terms of additional features and use cases covered?
- [ ] Community and contributor growth assessment
- The current number of contributors and committers, and the number of different organizations contributing to the project.
- 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.
- Adopted Stage graduation requirements
- [ ] Demonstrate a substantial ongoing flow of commits and merged contributions, authored by a healthy number of diverse contributors.
- [ ] Demonstrable roadmap progress.
- [ ] A healthy number of public adopters that are identified within the project ( using an ADOPTERS file or showcased on the project’s website ).
- [ ] Have achieved and maintained an [OpenSSF Best Practices Badge at the gold level](https://bestpractices.coreinfrastructure.org/en/criteria/2).
- [ ] Have a technical lead appointed for voting representation of the project to the TAC.
- [ ] Be deemed by the TAC to add value to the mission of ASWF.
- [ ] 2/3 supermajority vote of the TAC
- [ ] Affirmative majority vote of the Governing Board
* For a full list of supported build features: `vcpkg search openimageio`
* Instructions for building a Python wheel on Windows:
https://github.com/Correct-Syntax/py-oiio
* homebrew (https://github.com/Homebrew/brew)
* https://formulae.brew.sh/formula/openimageio
* `brew install openimageio`
@@ -265,8 +270,92 @@ Building on Windows
**Method 1 - from source**
I really need someone to write correct, modern docs about how to build
from source on Windows.
You will need to have Git, CMake and Visual Studio installed.
The minimal set of dependencies for OIIO is: Boost, zlib, libTIFF, OpenEXR, and libjpeg or libjpeg-turbo. If you have them built somewhere then you skip
the section below, and will only have to point OIIO build process so their locations.
* Boost: get the boost source archive, extract into `{BOOST_ROOT}`.
```
cd {BOOST_ROOT}
bootstrap
b2
```
* zlib: this will build it, and then delete the non-static library, so they don't get picked up:
* OpenEXR: you'll have to point it to your `{ZLIB_ROOT}` location from the above. If copy-pasting the multi-line command (with lines ending in `^`) into
cmd.exe prompt, make sure to copy all the lines at once.
| 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. |
## Reporting a Vulnerability
If you think you've found a potential vulnerability in OpenImageIO, please
report it by emailing security@openimageio.org. Only the project administrators
have access to these messages. Include detailed steps to reproduce the issue,
and any other information that could aid an investigation. Our policy is to
respond to vulnerability reports within 14 days.
Our policy is to address critical security vulnerabilities rapidly and post
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.
Larry Gritz
