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.
* 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.
* Make lookups of const unordered_maps thread-safe
* added back deleted end brace
* clang-format
---------
Co-authored-by: Larry Gritz <lg@larrygritz.com>
For non-0-origin images, we were checking the resolution range
incorrectly (`>=` vs `>`).
Also were printing the error message with the wrong notation, should
have shown a half-open range as `[beg,end)` and not `(beg,end]`.
When we recently added IOProxy support to the Maya iff file reader in
PR 3647, something broke for certain (but not all) iff files, and I
have productions stumbling over this.
It's proving tricky to track down and I have shows needing a fix
immediatly, so this change sets up a CMake variable
ENABLE_RLA_IOPROXY, default to ON, but when set to OFF, it switches
back to the old "no proxy" versions of the iff reader.
Disabling this will get me out of my current emergency -- at the cost
of not having IOProxy for iff, which I don't need at this moment
anyway -- and give me some breathing room to fix the problem for real,
at which point I will re-remove the old code.
There's a recurring idiom spread all over the python bindings for
ImageBufAlgo, which I encapsulate in a new local helper template
vecresize(). This ensures that the logic is identical in all the
places it's used, and also improves our coverage analysis (because it
was tricky to make tests that exercise the uninitialized-handling
cases separately for each of the many different places the idiom was
used).
Tweak the heuristics for trying to determine which arbitrary-named
color spaces are equivalent to certain known common names.
For OCIO 2.2+, some of this is easier and more reliable by using the
new built-in color spaces -- for example, we can transform a set of
sentinel colors from an arbitrary cs into the known built-in acescg,
or lin_srgb, etc., and if the values don't change, we know that cs
*is* the canonical one, even if named something weird.
But one problem is that for older OCIO, the built-in config is not
present, so if you have a color space named, I dunno, "linear", it's
hard to know what that is. Is it what we now call lin_srgb? Is it
ACEScg? Something else? So the hack is: first, we try to identify a
space that we think is sRGB (almost all configs have something called
"srgb", because everybody needs it, and we take it at face value that
if they call a cs "srgb", it's the real "srgb", fools who use the name
for something else get what they deserve). We transform the sentinel
colors from the srgb into the cs we are interrogating, and if they
match what we expect to see from an srgb->lin_srgb transformation,
then guess what -- the unknown color space must be lin_srgb!
We may, in the future, expand this to deduce other spaces. But this is
a start.
There are other changes I made here to make this whole process much
less expensive than it had been getting, there was some redundant
work, and some work that only was useful for 2.2 or only useful for <=
2.1, so I only do those in the right circumstances now.
Also along for the ride: improve test coverage for maketx features of
colorconvert, unpremult, attrib, attrib.
Examining some timings and function call counts, I saw that a typical
oiiotool command was spending a surprising amount of time in
ImageBufAlgo::zero(), not just more runtime overall, but also calling
it many more times than I thought it should. For example,
oiiotool -create 4096x4096 3 -colorconvert srgb acescg
called ImageBufAlgo::zero()... 4 times, totaling as much time as the
colorconvert operation.
The idiom used in `IBA::op(dst, src, ...)` which initializes the `dst`
IB if it's not already initialized will call the IB constructor that
zeros out its contents (so you don't have every new IB full of garbage
pixel values). But for IBA functions, this is almost never necessary,
since the IBA function itself will tend to overwrite all pixel values.
Not always, of course -- since IBA functions let you restrict the
spatial area or channel range -- but in the cases where the
destination starts out uninitialized and is allocated to exactly mtch
the ROI, I *think* you can count on the calling IBA function filling
in all those values.
So I changed IBAPrep() to call the IB constructor that says not to
zero out the pixels.
Just in case we ever need the ability, I made a new flag value that
lets you call IBAPrep and tell it, yes, do zero out any newly
allocated buffers. But we pass the testsuite without needing that flag
anywhere, so I think we're ok.
Also, the --create command itself would make new buffers (with the
implied zeroing), and then explicitly zero it again!
So after these changes, the oiiotool example command line above only
calls IBA::zero() ONCE (correctly, in the initial new image --create
makes), eliminating three redundant calls.
I also hypothesized that each call to zero was taking too much time,
because underneath it was calling IBA::fill which laboriously walked
through the IB with iterators and copying a fixed pixel value that
happened to contain 0 values. So I rewrote a special case where if the
buffer had contiguous strides, it would just use memset instead.
Surprisingly, it was not appreciably faster, and I'm guessing that the
memory bandwidth necessary to touch every pixel in a 4k buffer is the
limiting factor, and the other pixel-by-pixel iterator diddling was
inconsequential compared to waiting for the memory bus. I'm still
going to leave this "optimization" in, even though it was barely
measurable for the case I was trying.
Timer:
* Fix goof where m_printdtr was a bool but was tested for equality to
an enum. It happened to work like that, but only coincidentally.
* Add another enum value that allows a debugging message to be printed
upon construction of the timer, in addition to its destruction.
* Switch from printf style to std::print style formatting of the
debugging messages.
LoggedTimer:
* Modify to allow modifying the call count. This turns out to be
helpful for times when one logged function calls another logged
function (like if a special case is broken out) and they both log to
the same timer -- it makes it look like just one invocation instead
of two.
Currently, --resize takes a single argument giving an "image geometry"
(like "640x480") that designates the size of the result image, and the
rescaling specifically maps the "full size" (a.k.a. display window) of
the input image to the output.
This experimental change extends --resize to accept additional new
optional modifiers:
- ":from=<geom>" Overrides the default of the full window of the
source.
- ":to=<geom>" Overrides the default of the full window of the
destination.
- ":offset=+x+y" Adds an additional offset transformation to the
destination.
- ":edgeclamp=<0|1>" Turns on and off some special edge sample
clamping to try to reduce certain ringing artifacts. (Experimental.)
Unlike the output resolution, which like all image sizes must be an
integer resolution, the overrides on the source and destination
windows and the offset is allowed to be floating point, that is,
allowing for fractional pixel sizes or offsets.
The from and to windows do not restrict the boundaries of pixels
computed, but rather, simply define the transformational mapping
between the two images: the transformation will be chosen such that
the float-valued coordinates of the source image correspond exactly to
the float-valued coordintes within the destination image. In both
cases, coordinates refer to the true continuous coordinates in the
image plane, with (0.0, 0.0) being the "upper left" corner, and pixel
centers being at half-unit positions such as (ipixel+0.5, jpixel+0.5).
For example,
oiiotool in.exr -resize:from=100x50.3+50+20 1000x503 -o out.exr
creates a 1000x503 pixel image where the full output image is a
resized version of the 100x50.3 pixel window starting at coordinates
(50,20) in the input image.
This is mostly implemented (when it doesn't end up equivalent to the
old style resize) in terms of the existing IBA::warp() function. I needed
to add a new variety of warp() to avoid some artifacts, but I do this
tentatively and so currently do not expose it as part of the public API.
I want to gain some experience with the oiiotool usage by the users who
requested this, then will return later to exactly how it should be
exposed directly as IBA functions.
Some background: TIFF allows for pixel values to be encoded in some
alternative color spaces, like YCbCr, CIELAB, etc., and this is
indicated by the TIFF PhotometricInterpretation tag. Since in practice
we almost never encounter these encodings in the wild, and they
require a lot of custom code to handle (for example, YCbCr also comes
along with the ability to subsample the chroma, and options for
different placements of the subsampled positions, yuck), for most of
these cases we just lean on libtiff's ability to convert the whole
thing to RGBA for us, and so in that case, we return to the user the
pixels as RGBA, and set the "tiff:ColorSpace" metadata just as an
advisory to indicate how it had been encoded in the file. But we don't
support writing any of those. (One exception is that we do allow
reading and writing of CMYK files.)
OK, but now what about a case where the app already has the data in
one of these spaces (such as YCrCb; and let's ignore the whole
subsampling miasma and assume we don't want to support that), and all
we want to do is write that data to a TIFF file -- without any kind of
data conversion -- but to set the TIFF tags properly to indicate that
the data in the file is what we know it to be?
A related special case of this is: what if we have a TIFF file that
has YCbCr data but neglected to mark it as such (I'm staring daggers
at a certain widely used compositing package right now), and to fix
this, we want to use OIIO to just copy the file and set the right tags?
It turns out that in the status quo, it is surprisingly difficult to
do this. There is just nothing in our existing TIFF output code that
will write any of those photometric tags for those spaces, and no way
to communicate that this is what we desire.
Whew. Now here's what this patch does:
* If the "oiio:ColorSpace" attribute (our usual way to indicate
presumed color space of the pixel data) says that the pixels are
already in one of those color spaces (specifically, "YCbCr",
"CIELAB", "ICCLAB", "ITULAB", "LOGL", "LOGLUV"), then we set the
PhotometricInterpretation tag as such in the TIFF file that we
write.
* We still DO NOT interpret that as a request to convert pixel data
from any other space into this one. In other words, the TIFF output
does not convert color spaces (except for RGB <-> CMYK); this merely
lets us output a file that correctly says the data are what the app
thinks they are.
* A bunch of minor refactoring was necessary because of some unobvious
coupling and order dependencies. For example, the sgilog compression
techniques MUST be used with the LOGLUV color space, and vice versa.
You have to know how both should be set before checking that they
are compatible, and if they are not, adjust something that it's not
too late to change.
So the punchline at the end is that if you had a TIFF file that has
YCbCr encoded pixels, but for some reason it's marked as ordinary RGB,
you can fix it as follows:
oiiotool ycbcr_disguised_as_rgb.tif --iscolorspace YCbCb -o real_ycbcr.tif
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.
* 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.
* 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.
Note that these intrinsics don't seem to be available for gcc.
They are available on clang, but are defined in terms of the memcpy
trick. But in Intel, they seem to actually help.
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.
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.
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.
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.
* 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.
* 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
Turns out the clang/gcc intrinsics themselves compile to the same
memcpy idiom that I was using for the general case. So there is no
need for the clutter of the specialization.
libtiff 4.5 was just released, so confirm that we build against it,
update the "latest versions" CI test to use 4.5.
More careful sorting out of libtiff version numbers for conditional
compilation.
Get rid of some ancient tiff version guards that are related to
libtiff versions that are older than we currently support.
When building against libiff 4.5, use the new open calls that allow
per-file error handlers, which is better than the old global error
handlers for thread safety of multiple threads reading from the same
ImageInput if there are errors.
Convert error messages to std::format form.
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.
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.
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.
* OpenColorIO improvements, especially for 2.2
* When OIIO builds against OCIO >= 2.2, in the event that there is no
config supplied (e.g., specified by env `$OCIO`), use he new
build-in "cg" config to supply a basic set of color spaces and
transformations. Yay for OCIO 2.2 and its built-in configs!
* New OIIO-understood aliases (regardless of whether they were in the
config or not): "scene_linear", "srgb", "lin_srgb", and "ACEScg".
It will do its best to know which, if any, supported color spaces
are equivalent to these, even if arbitrarily named. For OCIO >= 2.2,
some trickery is employed to use the new features that can convert
to built-in config color spaces, transforming several colors in this
manner and seeing if it gets what would be expected for various
canonical color spaces.
* `oiiotool --help` now a much abbreviated set of color information
(just the OCIO version, and the config, and a hint about how to get
more info).
* A new command `oiiotool --colorconfiginfo` prints the FULL
inventory of color management information, which now has been
expanded to reveal the aliases of each color space, print the roles,
and generally spruce up the output to be much more readable (though
longer) for large configs.
* New methods for our ColorConfig class:
- `resolve(name)` turns the name, which could be a color space, an
alias, a role, or an OIIO-understood universal name (like "sRGB")
into a canonical color space name.
- `equivalent(name1,name2)` returns whether or not the two names
refer, ultimately, to equivalent color spaces (to the best of its
ability to figure out).
* Some minor changes (including additional `const` qualifiers)
on the ColorConfig class.
* Make testsuite tests use the built-in OCIO config if OCIO 2.2+ is
found.
* Build sonar against ocio2.2 so we are primarily testing the new
code paths.
* For OCIO >= 2.2, because we can count on the built-in configs,
conditionally compile a bunch of hard-coded color transforms. These
will eventually disappear completely, someday, when OCIO 2.2 is the
minimum supported version.
I think the behavior should all be largely unchanged when running
OIIO in a facility that has a proper OCIO config pointed to by the
`$OCIO` env variable and using color space names contained therein. So
studio users will (I hope) see no changes. For users that have no
OCIO config, or that are writing software that they want to work
reasonably well for common cases no matter what config they encounter
at runtime, this should give more helpful and consistent behavior when
OCIO 2.2+ is used (which I now strongly recommend for anybody not
constrained to 2.1 for compatibility reasons.
But because of the risk and the possible behavioral changes, this is
all currently envisioned as OIIO 2.5 features, and I am not planning
to backport it to the 2.4 release branch. Additionally, a lot has
changed and I expect some additional tweaking and identification of
edge cases I may have missed.
* docs, etc
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.
Remove the xxhash stuff from hashes.cpp -- it long ago moved to
xxhash.cpp. Also, comment out the bjhash implementations that OIIO
itself doesn't use or expose publicly, since there's no point
compiling it (or having it count against us in coverage analysis).
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.
* 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%.
* 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
Many ImageOutput::open implementations start out with various validity
checks -- such as whether the ImageSpec requests a resolution, number
of channels, or other features simply not allowed by that file format.
This is not only repetitive, but which checks are done by each file
type is somewhat haphazard, error-prone, hard to get full test
coverage (must we separately test each possible failure case for every
file format?), and the error messages are not always consistent across
formats. Frankly, most of the format writers assume that the caller
has a full understanding of supports() and will never send requests
for features that are unsupported.
This PR proposes adding a utility method ImageOutput::check_open(),
which performs all validity checks on the ImageSpec that can be
deduced from supports(). The various ImageOutput::open()
implementations may call this function and avoid having to replicate
(or forget) all the checks done in other output formats. This is a
protected method that is used by ImageOutput implementations
internally, but has no use for client-side code.
I've implemented check_open, and also used it here for all the output
formats. If this seems ok to everyone, I also plan to do an analogous
helper for ImageInput common validity checks (which will be an even
bigger reduction of redundant code and greatly increase the safety
against corrupted or nonsensical input files).
Along the way I also instituted a new supports() query: "noimage".
That is to indicate a file for which it is permitted to have a pixel
data window consisting of 0 pixels (i.e. an "image" that is only a
container for metadata). Currently, FITS is the only format we support
that allows this, though it's something we occasionally talk about for
OpenEXR. Allowing an explicit way for formats to say they allow no
pixels is what allows check_open to issue error messages for all the
other formats if it is passed zero-sized width, height, depth, or
nchannels.
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.
When creating a texture, the old logic was to set the file metadata
"DateTime" to the current time, unless update mode was used, in which
case it was set to the file's write timestamp of the input file.
But that made it impossible to explicitly set that metadata, and also
a big PITA for testing because it meant the metadata of any written
texture would be the time of its writing, and so appear different
every time.
This change alters the logic: update mode still sets DateTime to the
last update time of the input file, as before; other cases will only
set DateTime if it isn't already set, but not alter it if the input
carries that metadata already.
* 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.
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.
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
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.
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().
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.
* 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
* 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
Add IOProxy support.
Fixes#3623
Along the way, noticed an important bug at line 209 of this change --
the header "imagename" field should be fully zeroed out even if there
is no ImageDescription metadata!
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
It surprised me a little to realize that --printinfo prints the
current spec of the top image (I mean, duh), but that may not match
what you'd get with `oiiotool -info -v`, which prints the data about
each file as it's read from disk, inasmuch as the "current" spec
reflects the in-memory representation. So, for example, it might say
the image is float, because oiiotool tends to store everything
in-memory as float, rather than uint8 of the disk file.
So these changes help fix this oddity and other ambiguities I realized
follow:
* Give --printinfo an optional modifier `:native=1` that will print
the native (from the original file) info, if that's what you need,
rather than the up-to-the-moment in-memory data.
* Also add --printinfo optional modifiers `:verbose=` and `:stats=`.
* Expression substitution gets some new names it knows: "nativeformat"
is the data type in the file, versus "format" which I now see would
return the in-memory version; METANATIVE and METANDATIVEBRIEF, which
are the native versions of META and METABRIEF.
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.
* Add OIIO::getattribute() queries for "font_list", "font_file_list",
and "font_dir_list", which are, respectively, the lists of fonts
that were found, the list of font files (full paths), and the list
of directories it searched. All return a single string that is a
semicolon-separated list of the items.
* As a consequence of the new attributes, if you are curious to see
the full inventory of fonts it can find, you can just query the
attribute:
oiiotool --echo "{getattribute(font_list)}"
* Some refactoring of the font inventory process (in
imagebufalgo_draw.cpp) was necessary to achieve this.
* We now search for ".ttc" (TrueType collection) files, which we didn't
notice before.
* We now search explicitly in share/fonts/OpenImageIO, which we
neglected to do before, but found those fonts because we searched
/share/fonts recursively. In exchange, we now do not search the
directories recursively, because that seems unwise for performance
reasons.
* We pass the full testsuite, but it's possible that this
change may fail to find fonts that were previously found because of
the recusion. If that is the case, you should fix it by explicitly
adding the deep directories with fonts to the search path specified
in the environment variable `$OPENIMAGEIO_FONTS`.
* 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
* Add an optional size parameter to read_text_file to limit the
maximum size that will be allocated and read (default to a limit of
16MB).
* New read_text_from_command, similar use to read_text_file, but reads
from the console output of a shell command.
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.
ustring::hash() and the representation of ustringhash were size_t,
which might be 32 bits on some platforms. But we really would prefer
ustrings hash to 64 bits, to greatly reduce the chance of hash
collisions.
* Add strhash64() which is like strhash() but guaranteed to be a 64
bit value (returning uint64_t instead of size_t -- though note that
these are the same on 64 bit CPUs).
* ustring::hash() and ustringhash::rep_t are now uint64_t.
* Note that std::hash(ustring) and std::hash(ustringhash) are still
size_t, per the C++ definition of std::hash. So ustring::hash() may
have more bits than std::hash(ustring) on 32 bit platforms.
This will NOT be backported to the release branch, since it changes
ABI on 32 bit platforms.
* 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
Including windows.h leads to a lot of possibly common names being
"taken", mostly by preprocessor defines. Some of these are possible
to turn off (like "max" and "min" by defining "NOMINMAX" before
including <windows.h>). But many others are not possible to turn off,
e.g. "near" and "far" just get defined to nothing, preventing from using
variables named like that.
Additionally, includes have a cost, and while <windows.h>, being a C
header does not have expensive language constructs that would slow down
compilation a lot, it's still something close to 100k lines of
preprocessed code to wade through.
Now, only the OIIO implementation .cpp files that actually need
<windows.h> include it (all of them under "libutil").
There were two exceptions:
1) thread.h pause() implementation uses YieldProcessor() macro when on
a Windows system. Replaced it with manually "re-implemented"
things that YieldProcessor expands to. Right now made the
implementation for all 4 platforms that Windows normally supports
(x64, x86, arm, arm64); the latter two OIIO does not support though
when on Windows so it's untested.
2) timer.h Timer::now() inline function used QueryPerformanceCounter
and LARGE_INTEGER. These are quite hard to somehow "forward-declare"
or similar, without resulting in duplicate type definitions. So on
Windows I moved the now() function to be non-inline.
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.
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.
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.
* 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.
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.
- 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.
* 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.
Cmake-generated visual studio project files were using just one CPU core for all the compilation, which is not terribly fast. Add the `/MP` option so that it compiles separate .cpp files in parallel. On my (16 core, 32 thread) machine this makes a Release build of OpenImageIO.lib go from 136 sec down to 30 sec.
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).
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.
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.
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.
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).
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.
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.
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.
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
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.
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?
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.
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.
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!
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.
Just like we had done for the other libraries recently. And need
to make sure that the imath libs are added explicitly to the python
module's needs, since for hidden static libraries, they will not get
picked up merely by linking to libOpenImageIO.
Add an OIIO build-time option for forcing our exported
OpenImageIOConfig.cmake to skip the find_dependency of Imath and
OpenEXR.
This is helpful if you're at a site that is trying to build OIIO
against statically compiled Imath/OpenEXR, and want to be super sure
the downstream projects are definitely not going to encounter that
find_dependency(Imath) when the read the OIIO config files.
(They could do their own `-DOPENIMAGEIO_CONFIG_DO_NOT_FIND_IMATH`,
but I want even that to be unnecessary.)
There was that one pesky place where we were still including Imath
headers, from simd.h in the case where OIIO_SIMD==0 and we wanted to
fall back on some Imath::Matrix methods.
Ugh, but this backfired because when including this file from Cuda,
OIIO_SIMD==0 and it included the header. But what happens if a
downstream app (a) includes this file from Cuda, and (b) uses a
DIFFERENT Imath (so names clash) or doesn't want to use Imath at all?
So one more change:
* Remove the include of Imath.h from simd.h entirely.
* Most places where we fell back on the Imath::Matrix methods, just
replace it with scalar code, it's not complicated. (For some technical
reasons, this required some minor surgery on the matrix44 class, but
not anything that changes anything from a caller's point of view.)
* The one complicated case is matrix44::inverse(). Punt! Just let that
only be implemented if either (a) there is SIMD available, or (b)
there is Imath available (i.e., included by the downstream code
prior to this header). That's it. If you want to invert a 4x4 matrix
without SIMD, you need to arrange inclusion of the correct
ImathMatrix.h yourself before including this header.
For vector displacement maps or normal maps that are in tangent space,
it is heplful to be able to record whether the data are intended to use
a local left-handed or right-handed coordinate system, and for that to
be querably from the shader accessing the map.
The `maketx --handed` option, or `oiiotool -attrib -otex:handed=...`,
or adding "handed" metadata to the configuration ImageSpec being
passed to `IBA::make_texture()` does this. It's easy for texture
formats like openexr that allow arbitrary metadata. For TIFF textures,
we embed it in the ImageDescription tag (as we do for several other
texture-specific hints).
Skip 3 bytes between "software" and "background (key) colour" for the "software
version" (was simply missing). Avoids that m_alpha_type is overwritten with
wrong values.
The very latest fmt master changes a behavior: fmt functions no longer
automatically know how to format any type that can output to an
ostream, and instead need custom formtter objects. This breaks A LOT.
Fix by defining the preprocessor symbol `FMT_DEPRECATED_OSTREAM`,
which restores the old behavior (for now, at least).
While I was at it, I moved the fmt inclusion logic out of strutil.h
and into a new separate header, detail/fmt.h. This is in turn included
from strutil.h, so nothing in user code needs to change.
It turns out that this can crop up all the time, sometimes from
malformed metadata inside image files. We weren't noticing it because
in many cases, things ended up being turned into ustrings twice, and
the sequence string-with-nuls -> ustring() -> c_str() -> ustring()
will launder it. As I was tracking down and eliminating some of the
double ustring constructions, the metadata strings that were malformed
all along started becoming symptomatic.
Bad paradoxes follow from a ustring contain embedded NUL characters,
including ustring.c_str() and ustring.string() being different
character sequences, as well as ustring.hash() not being equal to
strhash(ustring.c_str()). One should definitely be able to count on
ustring(cstring) == ustring(ustring(cstring).c_str()).
So this patch checks for embedded NUL characters at the appropriate
time of ustring construction, and truncates the character sequence at
the point of any internal NUL character.
* `OPENIMAGEIO_IMATH_DEPENDENCY_VISIBILITY` is a new CMake cache
variable at OIIO build time that controls whether the Imath library
dependencies will be declared as PUBLIC (default) or PRIVATE target
dependencies of libOpenImageIO and libOpenImageIO_Util.
* For *downstream projects* that consume OIIO's exported cmake config
files, setting CMake variable `OPENIMAGEIO_CONFIG_DO_NOT_FIND_IMATH`
to true will skip the find_depencency() calls for Imath and OpenEXR.
To clarify, this is not a variable that has any effect when building
OIIO, it's something set in the downstream project itself.
* Add a `OIIO_IMATH_H_INCLUDED` header guard in OpenImageIO/Imath.h,
which allows something downstream to pre-define it to prevent OIIO
headers it needs to read from using the contents of that header.
Most builders of OIIO won't care and shouldn't change the defaults or
use any of these. But here's the specific case it addresses: If you
are building OIIO against static libraries of Imath & OpenEXR, but the
libOpenImageIO will be consumed by a downstream project that wants to
use a different Imath version, this can help you avoid some build-time
conflicts in the downstream project.
PPM by specification is Rec709! Make it so.
PNG files were getting marked as sRGB or Gamma if certain header
attributes were found in the file, but if they were absent, we didn't
designate a color space at all. Change so that the default assumes
sRGB, in the absence of any metadata telling us for sure.
Fixes#3084
One new method that should have been inline and non-virtual (so that
this whole set of changes does not change ABI by shifting the vtable
around) was accidentally marked virtual -- cut and paste error. This
patch fixes it to restore ABI compatibility.
IPTC data blocks seems buggy, and I don't know why. Our code?
Something in libtiff? Since nobody is depending on it and none of our
testsuite appears to fail when disabled, only write the IPTC block if
the user explicitly asks for it. Also, when reading them, don't
overwrite other metadata of the same name.
We wrote our string_view before C++17 std::string_view was finalized,
and the std one doesn't have a c_str() method. For conformance with the
standard, I want to remove this over time from ours.
Part 1: There were some places we did not need to call c_str() at all,
and some places where it was much more appropriate to just use a
std::string.
As Doug Rogers points out in Issue 3304, it is customary for Windows
applications to use wstring extensively for Unicode support. But it's
especially critical for unicode filenames, because unlike Unix-like
OS's where fopen() and similar functions support UTF-8, on Windows
those functions are strictly ASCII, and Windows users are used to
calling _wfopen() and other nonstandard functions that expect
filenames to be std::wstring if they want to support Unicode.
This patch:
* More clearly documents through all of our classes when parameters that
represent filenames or searchpaths are assumed to support UTF-8.
* Augments ImageInput, ImageOutput, and ImageBuf so that their methods
that take file names or path lists have additional flavors that take
wstring (always implemented as an inline shim that translates the
UTF-16 wstring to a std::string with UTF-8 encoding to pass on to the
usual method).
* Augments ImageCache and TextureSystem in just a couple places, the
functions where a filename is turned into a handle. All the many
other functions that take ustring for function names were left alone,
because they are called very frequently and are performance-sensitive,
so we don't want any translation from wstring to string happening on
every call.
We do not attempt to add wstring-based versions of functions that take
strings for other purposes -- that is, that are not intended to
represent filenames.
C isspace() takes an int, but on Windows when building in debug mode,
the implementation of isspace() can hit an assert if the value is < 0,
which of course it can be if you're looking at a character in a string
whose high bit is set (because char is signed).
Easiest solution is to make our own Strutil::isspace that does exactly
what we want and is safe.
Fixes#3300
This makes sure that two new preprocessor symbols are defined in
oiioversion.h: `OIIO_USING_IMATH_VERSION_MAJOR` and
`OIIO_USING_IMATH_VERSION_MINOR`, revealing the version of Imath being
used by the build of OIIO (which may not be the same as the version
found or being used by the app that's calling OIIO). They also get
defined as CMake variables in our exported cmake configs, allowng
downstream cmake build systems to be able to easily test whether the
Imath they found is compatible with the version that we use in our
APIs.
This patch sets out to minimize the extent to which Imath is a
necessary source dependency for using OIIO C++ header files and
APIs. The major changes are:
* Remove the unconditional `#include <OpenImageIO/Imath.h>` from
fmath.h and simd.h. Those forced Imath headers on nearly all all
code that uses OIIO's public APIs, whether or not they needed the
very few calls where Imath types are used as parameters.
* Make sure all public OIIO non-virtual methods and freestanding
functions that have Imath types as parameters are enclosed in header
guards like `#ifdef INCLUDE_IMATHVECTOR_H`. Downstream client code
needs to include Imath headers before including OIIO headers, if
they want to call those few particular functions.
This means that client applications who don't need those particular
functions, but call other parts of the OIIO APIs, don't need to
`#include` any Imath headers (or pay the extra compilation time for
doing so), and don't need any knowledge of which specific version of
Imath is used by the OIIO they are using.
The small number of public API calls that will no longer be visible
downstream unless the client code also include Imath headers (before
the OIIO headers) are:
- color.h: ColorConfig::createMatrixTransform (needs ImathColor.h)
- fmath.h: type conversions involving `half` (needs half.h).
- imagebufalgo.h: warp(), colormatrixtransform() (needs ImathMatrix.h)
- simd.h: conversions between our SIMD types and Imath types (Vec3,
Matrix33, Matrix44) and vtransform() calls involving those types
(requires ImathVec.h and ImathMatrix.h), and SIMD load/store
involving `half` data (requires half.h).
These changes necessitated adding including of Imath.h in a number of
internal .cpp files, the ones where those types are actually used.
The following header files do still unconditionally reference Imath
types or headers:
- texture.h: Some virtual methods of TextureSystem use Imath types
as parameters.
- imagebufalgo_util.h: For `half` support. We use this a lot
internally, but presume most client code never uses this header.
- simd.h, ONLY IF USE_SIMD=0 (which is generally only used for
development debugging and if porting to obscure platforms that are
neither x86 nor ARM).
The bottom line is that the vast majority of downstream callers of OIIO
APIs who need ImageInput/ImageOutput, ImageBuf, ImageBufAlgo, and
generally everything in the OpenImageIO_Util library, do not need to
use Imath types, include Imath headers, or even be aware of which Imath
release OIIO is using.
Why would we do all this?
Because this paves the way for optionally building OIIO to link against
static libraries for OpenEXR and Imath, essentially embedding and
completely hiding an OpenEXR/Imath inside libOpenImageIO.
That allows libOpenImageIO to use the very latest Imath (3.1, say,
with the new faster core API), even though that libOpenImageIO is
being consumed by an application that for whatever reason is forced to
use an entirely different (older) OpenEXR or Imath library. In other
words, it lets OIIO use OpenEXR/Imath fully embedded without
transitively passing that OpenEXR version as a downstream dependency
(except for the few use cases that need to call the tiny number of
OIIO API methods that require Imath types.
There is a small chance that client code that use Imath types and
SHOULD have been includeing Imath headers themselves actually omitted
those includes and dependd on the fact that certain OIIO headers
included them. Now that they don't, it may require the client-side
code to add those includes. I'm going to claim that this is a "bug" in
the client code (they didn't include a header that defined a type they
used), and not a breaking incompatibility in OIIO's public calls
themselves. Thus, I feel comfortable with these changes being in OIIO
2.4 (today's master) and not necessitating a 3.0 release (in which
arbitrary loss of backward compatibility would be allowed). So watch
out for the occasional need for OIIO-calling code to need to add an
`#include` for Imath types the code was always using, but neglected
to explicitly include.
Our Strutil utilities for utf8 <-> utf16 conversion were based on
Windows specific calls that we had added even before C++11 was our
minimum. Replace the Windows-specific code with std calls, and
expose them in strutil.h for all platforms (why not?).
We were passing along, in the exported cmake config files, a demand
for downstream clients to use a C++ standard version equal to that
which we used to build OpenImageIO itself. But that's not
necessary. We may choose to build with C++17 (for example), but all of
our public APIs are safe to use for C++14.
Split the build-time options to CMAKE_CXX_STANDARD for the standard we
build this project with, and DOWNSTREAM_CXX_STANDARD for the standard
we are declaring is the minimum to *use* the library.
The main event:
* Assume that for a TGA 1.0 file (too old to have the header field
that says if alpha is associated, unassociated, or meaningless), if
the alpha is zero everywhere, it means that it's meaningless, not
unassociated, so in that case we should skip the
auto-premultiplication. (See issue 3277)
Public-ish stuff:
* Be more consistent with Targa-specific attributes all being called
"targa:foo" (most were, but one was "tga:alpha_type").
* Document all the targa-specific attributes (a few were not).
* Add "targa:version" to reveal whether the file was a TGA 1.0 or
2.0 version of the format.
Internal stuff, opportunistic:
* Some more minor conversions of string fomatting to fmt style.
* Convert some std::cerr debugging code (commented out) to the DBG macro
we use anywhere.
* The `std::unique_ptr<uint8_t> m_buf` should always have been
`unique_ptr<uint8_t[]>` because it's not a pointer to a single value.
I realized recently that if you said something like
oiiotool foo.exr --ch R,G,B,A -o out.exr
and there was no "A" channel... it would just silently fill the channel
with 0 in the output.
This patch adds a warning if --ch is told to copy a channel that doesn't
exist in the input.
Ugh, there's a situation where PhotoShop is misinterpreting an alpha
channel for certain OIIO-written TIFF files, but it's fine with some that
came from elsewhere, and the only thing that appears to be different is
that the non-OIIO one entirely lacks the EXTRASAMPLES TIFF tag.
So I'm adding an option (use with extreme caution) allowing you to
tell the TIFF output to not output this tag, even though clearly it
should be.
Some rogue app out there is writing 4-channel TIFF files without the
ExtraSamples tag in the header that explains that the data is a valid
alpha channel.
We were using TIFFGetFieldDefaulted to retrieve this tag, and guess what?
The defaulted behavior changed between libtiff 4.0 and 4.3!
Changing to TIFFGetField actually turns out to fall back to the right
logic on our side, which boils down to assuming a 4th channel is alpha
if there's no other information. That seams reasonable -- RGB + mystery
channel is almost certainly RGBA.
* Bugs fixed in build_opencolorio.bash, it botched some of the directory
choices when customizing where to place and build it. Also, switch to
the more standard BUILD_SHARED_LIBS cmake variable to control whether
to build dynamic or static libraries.
* Correctly handle the cmake variable VISIBILITY_MAP_FILE being set to
"" on the command line as a way to disable using the visibility map file
entirely.
* FindOpenColorIO.cmake changes:
- Prefer and use an exported cmake config for OCIO if found (is should
be present for OCIO 2.1+), unless caller set OPENCOLORIO_NO_CONFIG=ON.
- New hint OPENCOLORIO_INTERFACE_LINK_LIBRARIES lets caller force a
list of interface link libraries. This is helpful when purposely using
a static OCIO library, which itself has static dependencies, and you
need to name those specific library filepaths.
* Simplify some of the logic for setting extra link flag arguments for the
two main libraries.
* On Linux, also set link flags `--exclude-libs ALL` to hide symbols from
any embedded static library dependencies, to prevent clashes with other
versions of those libraries that may be compiled into a DCC app.
I realized that a call to make_texture() with the "maketx:ignore_unassoc"
attribute set did not properly set the configuration attribute up when
opening a new ImageBuf.
```include/OpenImageIO/typedesc.h:620:10: warning: 'const' type qualifier on return type has no effect [-Wignored-qualifiers]
auto parse(format_parse_context &ctx) -> decltype(ctx.begin()) const // c++11
```
Co-authored-by: Thiago Ize <thiago@solidangle.com>
The docs said that passing a tuple for min or max param of the clamp()
function gave per-channel values, but passing a single float gave one
value for all channels.
In fact, the implementation used the float to clamp only the first
channel. This patch fixes the behavior.
Fixes#3264
In addition to C++ code examples for each IBA function, work toward
also having equivalent Python code. Also, where applicable, show the
equivalent operation using oiiotool.
We had forgotten to mark --colorcount and --rangecheck as commands
that intentionally outputs to the console. This omission meant that an
oiiotool command consisting only of a those would warn about no
output. But we didn't really want it to.
How do you craft an oiiotool command that will examine a file and add
an alpha=1 channel if it's a 3-channel file, but leave the existing
alpha alone if it's already a 4 channel file?
This patch adds control flow, so you could express the above like this:
oiiotool in.exr --if "{TOP.nchannels < 4}" --ch R,G,B,A=1 --endif -o out.exr
The following slate of new fetures is added:
* `--set name value` sets a "user variable".
* Expression evaluation will expand identifiers that are the names of
user variables. For example, `{i}` is replaced by the value of user
variable `i`, and `{i+3}` returns 3 more than the current numerical
value of `i`.
* Expression evaluation has been extended to support the following
numerical comparison operators: `<` `>` `<=` `>=` `==` `!=` and
`<=>` (the last one being the 3-way comparison, like in C++20).
Also logical operators `&&` and `||`. All return 1 for true, 0 for
false. Note that the comprison and logical operators have the same
precedence as `+` and `-`, so use parentheses liberally to
disambiguate complex mixed expressions.
* New functions recognized within expressions:
- the value held by a named user variable: `var(name)`
- logical negation: `!`, `not()`
- string comparison: `eq()`, `neq()`
* `--if condition commands... [--else commands...] --endif`
* `--while condition commands... --endwhile`
* `--for variable range commands... --endfor` where the range may be,
like Python, any of `end` (implied begin=0, step=1), `begin,end`
(implied step=1), or `begin,end,step`.
Also needed to modify ArgParse so that it has the notion of whether
it's "running" -- actually calling the actions for each argument, or
if it's merely parsing but taking no action -- and to be able to set
and retrieve which command line argument index an action is responding
to.
This little change makes this work:
ImageBufAlgo::add (buf, { 0.2f, 0.2f, 0.2f });
instead of the more awkward
ImageBufAlgo::add (buf, cspan<float>({ 0.2f, 0.2f, 0.2f }));
and for anyplace tht has Image_or_Const parameter types.
New OpenColorIO and fmt versions are out.
* Bump "latest releases" test to fmt 8.1.0, and OCIO 2.1.1.
* Re-adjust CI test fmt versions across many tests to give a better
spread over the supported releases.
* Bump the default version to download and use if none is found to 8.0.0.
We had --line (wrapping IBA::render_line) and --box (wrapping
IBA::render_box).
This adds the obvious --point, wrapping the existing IBA::render_point.
The Python bindings for ParamValueList and ImageSpec allow a "dict"
style (associative array) interface:
imagespec["key"] = value
val = imagespec["key"]
This patch makes some minor improvements:
* Clarify in the docs that like a regular Python dict, the associative
retrieval will raise a KeyError if the key is not found.
* Add `__contains__` python method as a synonym for contains(), thus
implementing `key in dict`.
* Add `__delitem__` python method as a synonym for erase(), thus
implementing `del dict[key]`.
* Add an `ImageSpec.get()` method that is a synonym for getattribute
(which retrieves the item or returns None or a default if not
found), mirroring the behavior of Python's `dict.get()`.
Somehow I never implemented a copy() method for ROI, which of course is
super useful if you want to make an actual copy of an ROI and modify
the copy.
I also realized that ImageSpec.copy() unnecessarily/incorrectly set
return_value_policy to reference_internal, but actually it should be a
real copy, which it has to be anyway because the implementation
creates a separate copy.
* Docs: we previously only documented the "width=" optional modifier.
Add height, color1, color2.
* Change behavior ever so slightly so that if the "height=" modifier is
not supplied, the height defaults to the "width".
* Use the new pybind11 v2.9.0 for 2022 and "latest" tests.
* Bump the default version built by build_pybind11.bash to 2.8.1.
* Adjust the pybind11 version used in many of the other CI tests, to
give an even spread across versions, and to make each of the
"vfx platform" CI tests use the pybind11 version that was available
near the start of its repsective year.
For each ImageSpec ctr that takes a TypeDesc, add another one
that's the same but takes a string_view (which is just a simple
pass-through of the TypeDesc-from-string_view).
Also tagged Strutil and ustring::fmtformat as 'nodiscard', to produce
warnings if people call it and ignore the results. (As was the case in
my own bug that I tracked down in the course of this patch.)
When doing operations with filters that have negative lobes, you can
sometimes get visible ringing artifacts in very high contrast regions
of HDR images (you tend not to see these artifacts in images that have
range [0,1] because the ringing is very low amplitude unless the input
is very large).
You can do "highlight compensation" to reduce the artifacts when they
occur by doing --rangecompress, which does a log transform on the
input, then the filtered op, then --rangeexpand to transform back to a
linear space. But that makes for kind of verbose and clumsy command
lines, and makes it easy to err by doing the transforms in the wrong
sequence or omitting one.
The -otex command takes a highlightcomp=1 modifier, which does this
bracketing automatically, foolproof, with a compact notation.
This patch adds support for optional highlightcomp=1 modifier for all
of the oiiotool commands that let you specify a filter= option:
--rotate, --warp, --resize, --fit, and --pixelaspect.
* Clean up intra-document section references.
* New explanations about input and output configuration hints.
* More code examples in both C++ and Python.
* Fix some formatting flubs.
This was an obsolete experiment in adding a C binding. We abandoned
this effort (a replacement will happen eventually), but this one file
was left behind when we intended to remove it all.
Errors encountered while reading tiles were getting lost as we used
ImageBuf::Iterator to iterate over an image. Have the iterator keep a
flag for whether it encountered an error, set to true if the
ImageBuf::retile indicates an error occurred during the read.
Also, some ImageCache errors might get lost on the way to
TextureSystem calls, fix that as well.
TextureSystem is designed primarily for tiled images (typically 64x64
pixels each), but to handle untiled images, it just treats the whole
image as one big tile. That means for very large untiled images, you
might end up with one VERY large tile, and if the image is large
enough, the size and/or offset computations therein may overflow a
32 bit int.
This patch attempts to make all the offset computations in the texture
system be 64-bit safe.
After setting up the new IOProxy helper methods for ImageInput and
ImageOutput, here we go back to the readers/writers that had used
proxies for a long time, and switch them to using the new helpers.
Add two new global OIIO attributes: "limits:channels" and
"limits:imagesize_MB" which are imposed limits for validating a
reasonable number of channels and total uncompressed image size,
respectively. We presume that an image file header describing an image
that purports to exceed these is most likely corrupted or even
maliciously crafted. The limits, therefore, are designed to avoid
crashes, absurd memory allocations, or other cascading errors that
would result from taking such a claim at face value (for example,
a file that appears to have tens of thousands of color channels).
The limits default currently to a maximum of 1024 color channels, and
a maximum uncompressed data size of a single 2D image to 32GB. These
seem reasonable today, but if such large sizes become likely to be
seen in the wild in legit files in the future, we may change the
defaults. They may be adjusted now simply by setting the attributes,
of course, as should be done by applications that legitimately expect
to see input that exceeds them. (Applications also may make the
validation limits even smaller if they wish.)
I've modified the TIFF reader to check these limits as a first step.
We'll expand to doing such validation checks on other readers over time,
as we have opportunities to visit them.
This string utility computes the "edit distance" between two strings.
Currently it only uses the Levenshtein metric, but it has room for
future expansion by taking a parameter that can specify which metric
to use. Levenshtein is an algorithm for computing how many single
character insertions or deletions are necessary to turn one string
into the other.
I have a few ideas for where this can be useful, but as one
demonstration, I also modify the ArgParse class internals so that if a
command is not found, it will print a suggestion naming the valid
command option that has the closest edit distance (but only if one
choice is <= 2 edis away, and only if it's not a single-character
option, in order to reduce the number of useless suggestions). Here's
its behavior in action:
$ iinfo --stat foo.exr
iinfo error: Invalid option "--stat" (did you mean --stats?)
...
* The main event here is that we're now telling OpenJPEG to use
threading when compressing/decompressing JPEG-2000 files. This
improves the speed of JPEG-2000 reading on my laptop by 2-3x! The
threading feature was added to OpenJPEG 2.2 for decode, and 2.4 for
encode.
* CI: Build OpenJPEG 2.4 for the "latest releases" test, and build
their master for our "bleeding edge" test.
* Fixes to deal with the in-progress 2.5 deprecating a struct field.
* Generally clean up our detection of what OpenJPEG version we are
using.
* Some clean-up of the FindOpenJPEG.cmake file, use PATH_SUFFIXES.
Also fix longstanding bugs in PNM input:
* We weren't correctly emulating random scanline access, as is expected
(but in practice rarely used) of all ImageInput implementations.
* We weren't marking the color space in any way, now we do mark as
"Gamma2.2", which is the correct/preferred interpretation according
to the PNM spec.
* Add full IOProxy support to both input and output.
* This necessitated a major refactor, including sucking the I/O specific
code from rgbe.{cpp,h} into hdr{input,output}.cpp and getting rid of
the old files.
* Add a testsuite entry for hdr (shockingly, there was none before)
As well,
* Add a DDS test, apparently we never had one.
* Remove DDS setting of "texturetype", we don't ever use that, it's only
"textureformat" that we use. Not sure how that crept in there years ago.
Helper functions ioproxy_opened() and iotell() should have been const
all along, since they just return back information and should not
change state of the proxy, and also ioproxy() should have both const
and non-const versions.
* On Mac CI, use openvdb from Homebrew now that it's OpenVDB 9, which
properly supports OpenEXR/Imath 3.1.
* Update our FindOpenVDB.cmake target names to match the names used in
OpenVDB's own FindOpenVDB.cmake (even though we don't use their module,
may as well stick to their convention, especially if they eventually have
a properly exported config file).
For some reason, in the early days I got in the habit of having OIIO
input readers call their color space "GammaCorrected2.2" (for example)
when they appeared to be a straight-up gamma. (Generally only used for
certain fairly old formats that date from before sRGB existed.)
But there appears to be a widely used convention of naming OpenColorIO
spaces "GammaX.Y", so I'm switching OIIO to conform that
convention. It's more straightforward and increases the chances that
it will match the names of color spaces that already exist in people's
OCIO configs.
As we move toward fully equipping all image format implementations
with full IOProxy support, it's clear that a lot of repeated code is
error-prone. I first tried to collect this in a mixin class, but then
re-implemented after realizing that we can use the existing PIMPL
ImageInput::Impl and ImageInput::Impl classes, without changing any
ABI or making new headers.
This patch add a couple data members to the Impl data -- an
`IOProxy*`, (the current proxy, used for both passed-in and created
proxies) and also a `unique_ptr<IOProxy>` used when we have to create
our own and want to make sure it's destroyed with the ImageInput or
ImageOutput.
Then we add a number of protected member functions to ImageInput and
ImageOutput, not meant to be ever called by "users" of the OIIO APIs,
but used internally by II's and IO's to encapsulate several common
tasks related to managing IOProxy's. This coalesces redundant code
that would need to appear separately in most of the plugins that
support IOProxy, helps to reduce cut-and-paste errors from having to
make these changes separately in many places, and centralizes the
logic so that as our ideas of how to implement IOProxy evolve, we can
easily change it in just one place and have it apply to all plugins.
GIF revise with new IOProxy utility methods
JPEG simplify with new IOProxy utilities.
To try to combat long template instantiation times, did some refactoring
on the ImageBuf iterators:
* Pushed more functionality from the templated Iterator and
ConstIterator to the non-templated base class IteratorBase. This
should reduce the amount of code that is separately instantiated for
every data type, in every module that uses iterators.
* Pushed many implementations from inline defined in imagebuf.h to
separately compiled in imagebuf.cpp. The most important methods that
are called on every pixel are still inline, but we are switching to
separate compilation for all the methods that are only called upon
construction (once per image being iterated over) and most of the
code that is called only when we wrap to the next scanline.
* Some other miscellaneous refactor to remove redundant computation in
those classes.
Maybe somebody else is building this as a subproject or has a varible
called "CI"?
* For our CI runs, set an env variable "OpenImageIO_CI" (not just
generic "CI").
* Make sure STOP_ON_WARNING is true for our CI runs, even for release
branches -- but now check OpenImageIO_CI, not CI, in case somebody
else has set that.
This was deleted a few months back, but I think I did not realize
it still served a purpose. We don't use JasPer directly, but LibRaw
does, so if we are using static LibRaw, we still need it here.
Fixes#3209
When the make_texture option monochrome_detect is used to collapse 3
identical channels to 1, it was leaving the first channel name in
place (for example, "R"). That was confusing, because at this point,
we're not trying to represent a red image, we're trying to represent a
grayscale image. A strategic call to `default_channel_names()` clears
it up, imposing the standard "Y" we use for a single channel luminance
image.
Also amended the documentation of IBA::channels(). It turned out not
to be the way I called channels() that was the problem here, but by
the time I figured that out, I had already seen that its docs could be
improved.
Fixes#3204
* --onlyformat or env IMAGEINOUT_TEST_ONLYFORMAT to test only one format.
* --no-delete to preserve the temporary files for inspection
* other debugging checks
* Add a DDS test, apparently we never had one.
* Remove DDS setting of "texturetype", we don't ever use that, it's only
"textureformat" that we use. Not sure how that crept in there years ago.
The previously existing max and min take 2 images and compute the
pixel-by-pixel, channel-by-channel max or min of the two images.
The new maxchan/minchan take one multi-channel image and produce a
single-channel image where each pixel contains the max or min of the
*channels* of the corresponding pixel of the original image.
It is equivalent to this (for an RGB image):
oiiotool in.exr -ch R in.exr -ch G in.exr -ch B -max -max -o max.exr
Except, of course, that --maxchan is a single command, is much more
efficient, and doesn't require you to know the number or names of the
channels of the input.
This was a specific user request.
Note: in the course of making the tests, I noticed that we did not
have documentation or testsuite coverage of the python bindings for
IBA.min() an max(), although they were implemented. So I added that as
well.
* Make PROJECT_VERSION_RELEASE_TYPE be a cache string, so individual
build configs or sites can override it.
* Change OpenImageIO_IS_SUBPROJECT to PROJECT_IS_TOP_LEVEL (and with
the opposite sense), to match CMake 21+ behavior. And no need to set
it ourselves if using cmake 21+.
ImageBufAlgo has both premult and repremult (as well as unpremult).
But oiiotool only exposed unpremult and premult, but not repremult.
This little patch rectifies that oversight.
Just to remind everybody of the difference:
`premult` just multipies the color channels by alpha, like it says.
Note that for a pixel that has Alpha = 0 but nonzero color, this will
crush the color to black.
That's fine if you have an *original* image that's unpremultiplied and
you want to do a one-time conversion to premultiplied.
But there's another use case, which is when you start with a
premultiplied image, you need to temporarily unpremultiply it (like
for a color space conversion), and then re-premultiply it. In that
case, you may have those glowing pixels in the original image, the
unpremult will preserve them, and you DO NOT want to crush them to
black with the subsequent premult. So there is a "repremult"
operation that's like premult, but preserves the color values when
alpha == 0. This is the preferred operation when you are doing the
sequence of "unpremult, do something, re-premult" so that the glowing
pixels make it through the round trip.
When I implemented `oiiotool --invert`, I knew that the default should
be to only invert the first three channels (presumably R,G,B) and not
anything beyond, because we wouldn't want to invert the Alpha channel.
But I botched it, and instead what happened is that if given an RGBA
image, it would just leave the A black in the result image.
So this patch has two improvements:
1. If there are more channels in the source image than we are
inverting, make sure to copy the other channels unmodified, don't
just leave them blank.
2. Allow optional modifiers chbegin= and chend=, so users can change
the default if they want a different channel set to be inverted.
This new IBA/oiiotool command adjusts the saturation of a color image,
with a scale factor. Scale = 0 makes the image gray scale of
equivalent perceptual luminance, scale = 1 leaves color unchanged,
scale > 1 increases color saturation.
This adds a new cmake variable in our build system, TIME_COMMANDS,
that when set ON will adjust the compile and link rule launch to
include a time command. This is probably only useful when also setting
`CMAKE_BUILD_PARALLEL_LEVEL=1` so that build are happening serially.
The use case is to investigate how long each module takes to compile,
for optimizing the build.
* Add a test for MacOS 11
* Updated supported versions of dependencies
* Move linux clang test to clang13 on vfx2022 container
* Switch clang-format test to use clang12
In the early days of OIIO, it was 100MB. Ten years ago, we bumped it
to 256MB. I've heard a rumor that computer memories have grown in the
last 10 years, and so have the size of images some people deal
with. So it seems reasonable to raise it to 1GB.
`sscanf()` still has locale-dependent behavior that we wish to avoid
because it can lead to subtle bugs for people whose machines are
configured for certain locales.
This patch removes from the OIIO codebase almost every remaining
sscanf use (and definitely every one that could harbor locale bugs).
Instead, we use the rich set of parsing functions we've added to
Strutil over the years, which are locale-independent, and in most
cases are also faster than the standard C/C++ equivalent.
The official names we use are "box2" and "box3", but users of Imath
know these as box2f/box3f, so accept these as synonyms when
constructing a TypeDesc from a string.
* Typo in webp section -- said "gif:LoopCount" when we meant
"webp:LoopCount".
* In the code, did not actually set the "oiio:LoopCount" metadata
that the docs said would be set.
* Opportunistic fix: TIFF docs said we only suported IOProxy for input,
but we have supported it for output for a while now.
* `parse_values()` and `scan_values()` both can parse a series of
values in a string, with optional prefix, suffix, and separator. The
scan versions just figure out the values, the parse versions
optionally can update a string_view& like the other parsing
functions in Strutil.
* `scan_datetime` parses the int components of a string that looks like
"YYYY:MM:DD hh:mm:ss" (and a few minor variants).
* Fix stoui -- it was previously casting stoi results, which is not
correct.
* Add `from_string<>` specialties for `double`, `int64_t`, `uint64_t`.
When outputting to formats that don't support negative pixel origins
(such as JPEG), we crop away the negative areas. But we did not
consider the case where the entire data window was in the negative
region!
TIFF has an absolute limit of 4GB file size due to use of uint32
offsets within the file. If you try to write a big enough TIFF image,
it will fail. This is not a software problem, it's a limitation of the
original TIFF file specification itself.
The underlying libtiff supports a nonstandard "bigtiff" extension that
uses 64 bit offsets. This patch switches to this automatically for
images whose uncompressed size would get too close to (or exceed) the
4GB limit. Any such files can be read by OIIO-based software, as well
as anything else using a relatively modern version of libtiff, but may
not be readable by independent TIFF reading implementations.
This is a mixed bag, though. On one hand, this at least lets us write
files that would have been an error before. But note that some files
that would have worked as classic TIFF before -- because their
*compressed* size was still under the 4GB limit, even if their
uncompressed pixel size was not -- will now be written as bigtiff
because we have to commit at the time we open the file, before we've
seen the pixels and know the final compressed size. We just can't
know for sure, for an image with > 4GB of pixels, if the file will get
lucky and be below the limit by the time the scanlines or tiles are
compressed. So for those files, not all apps will be able to read
them (though anything using libtiff >= 4.0 or OIIO will be fine).
Fixes 3156
When ImageInput::create() or open() is called, it first uses the
extension of the filename to guess which reader plugin to try (for
example, "foo.jpg" will try the jpeg reader). If that fails, however,
the default behavior is to then try EVERY reader to see if any will
open the file, in case it guessed wrong, or the extension is used by
more than one format, or the file was simply given the wrong
name. Thus, ultimately, OIIO will never fail to open a valid image
file in a known format, even if the file is wildly misnamed. However,
it's a lot faster to open a correctly named file, because it will try
that one first, before trying a long succession of readers in the hope
that eventually one will work.
There are three cases where this is not the desired behavior:
1. You are confident that files are named with the correct extensions,
and you want a corrupt or invalid file to fail QUICKLY, without trying
every format reader.
2. If the file is corrupt, rather than see a generic "Could not find
any format reader that will open the file" because they all failed,
you want to see the more specific error message for why the (presumed)
correct reader could not succeed.
3. When doing regression tests to make sure that sample corrupted
files are correctly reporting the right errors, we really do want to
see error messages from that one reader we are testing.
This PR exposes a global OIIO attribute, "try_all_readers", which
defaults to 1, but when set to 0, changes the behavior so that ONLY
the one format implied by the file extension is tried, and OIIO does
not subsequently try every known reader.
This new oiiotool command lets you set global OIIO attributes, i.e.,
making calls to OIIO::attribute(). It allows oiiotool command lines to
set some additional OIIO-wide options for which no specific oiiotool
command flags could otherwise access.
When building against OpenEXR >= 3.1.3, exr output now allows zip/zips
compression to take a quality metric (such as compression
"zip:4"). The range is from 1-9, 4 is the default, and it's a control
over speed of compression vs compression ratio (higher number is more
expensive but may result in smaller files). This only affects
compressed output -- it's lossless and will be read to the same pixel
values upon input.
This is also incentive to upgrade to OpenEXR >= 3.1.3, because its new
default (and ours) is 4, versus the old (implied, not directly
controlled) 6. This change was made because compared to 6, it's
significantly faster to write exr files with zip compression 4 (tens
of % faster), while only increasing file size by 1-2%, so that seemed
like the right tradeoff. Lower numbers make much larger files for
little speed gain, and larger numbers than 6 make for much slower
writes of exr files for very tiny compression improvements. But YMMV
and you may want to experiment to find the right trade-off for your
use cases.
OIIO 2.3 marked it as deprecated. This new change is for master (2.4+)
only -- removing all support for Field3D.
F3d is effectively unmaintained, is known to be incompatible with
the latest versions of OpenEXR/Imath, and we haven't found anyone
prepared to speak up as a user of it via OpenImageIO support.
One more boost extraction: tokenizer.
The only place we used it is Filesystem::searchpath_split, which can
easily be rewritten in terms of the Strutil parsing functions.
While I'm at it, add a new version that returns the vector instead
of needing to pass in a ref.
Suggested by Luke Emrose, the idea here is that we introduce two new
MIP interpolation modes: StochasticTrilinear and StochasticAniso.
These work similarly to Trilinear and Aniso, respectively, except that
instead of sampling both of the bracketing MIP levels and blending
between them, in stochastic mode we use a random variate to select ONE
of the two MIP levels with probability proportional to the computed
blending weight.
If these mip modes are used, the caller needs to pass a
well-stratified value in the new "rnd" field of TextureOpt. Also note
that the method only really makes sense in the context of a renderer
that is using many samples per pixel. As a single texture lookup, it
looks horrible and noisy, as you can see by the testshade-based tests
we do of this, which are 1spp and looks pretty rough.
My benchmarks (in a hard stress test of the texture system using
`testtex --threadtimes 8`) show that this approach has speedups of
35-40% when using between 1-4 threads, dropping to more like 25-30%
speedup as threads are in the 12-32 range (that makes sense, it's
doing the same amount of total I/O).
In this first implementation, in order to preserve ABI compatibility
if we decide to backport this feature to the release branch,
TextureOpt::rnd is added as a union co-member with the unused "bias"
field. For TextureOptBatch, we had to add it outright, but it's
guarded by an `#if` and will not be backported to 2.3. (We may change
our minds and not backport it at all, I just wanted to leave the
option open with this initial patch. Also, there will probably be
additional further ABI-breaking changes coming later that will
definitely not be backported.)
In this first phase, we only use the stochastic component to turn the
2-MIP-level lookup into one level lookup. It has been further
suggested that we could apply stochastic techniques to anisotropic
filtering (instead of many samples spaced across the major axis of the
filter footprint, take one sample distributed across the axis), and
even for the ordinary bilinear or bicubic within each level (instead
of 4 or 16 weighted texels, just retrieve one texel chosen with the
weights as probabilities). But these deserve to be benchmarked and
have their quality evaluated separtely and deliberately, and so will
be relegated to a later investigation and implementation.
We will have a C API eventually, but it will be implemented
differently, and this cruft will probably get in the way. If the new
code needs any of this (particularly the test cases), it's easy enough
to revive it from the project history).
When requesting that raw image be returned as "ProPhoto" color space,
we were using the wrong gamma. Also, at user request, add
"ProPhoto-linear", which is the same but with linear response. (This
is analogous to the existing "sRGB" and "sRGB-linear".)
Fixes#3152
Thiago Ize points out that TextureSystemImpl::fill_gray_channels
assumes that the `float*` addresses it is passed are big enough for
a vfloat4, and also properly aligned as such. Not true.
This is an edge case anyway, only called in particular cases where
we are doing 3/4 channel lookups on 1/2 channel files with the option
turned on to expand single channel gray to RGB gray, and also doesn't
do enough SIMD to have a perf impact. So just recode as scalar and
have no more alignment mismatches to worry about.
Also make sure at least one of the Windows CI tests is explicitly
turning SSE on.
Fixes#3128
Field3D is essentially unmaintained, and is currently incompatible with
OpenEXR/Imath >= 3.
In this first stage (which will apply to OIIO 2.3):
* By default, Field3D support is disabled. To enable it, you must
invoke CMake with `-DENABLE_FIELD3D=1`. No warning or other messages
are printed by the build configure step when not enabled.
* If requested, but an incompatible OpenEXR is found, it will be disabled
with a clear error message.
* If requested and enabled, a warning will be printed indicating that
Field3D support will be removed from future versions (2.4 and beyond).
* Add a new test, texture-texture3d, that provides the same testing that
texture-field3d did, but with openvdb.
We have options that allow adding random dither to images before
output, which when converting float images to 8 bit or less, can
greatly reduce the appearance of banding artifacts. This PR makes a
number of improvements:
* When the option is used, it should apply dither any time you are
reducing an image with > 8 bits/channel to an output with <= 8
bits/channel. Not just from float/half, but also from uint16, for
example. (When outputting > 8 bits per channel, there are enough
gradations that you tend not to see banding artifacts, so dithering
is not necessary for 16 bit output, even if coming from a float data
source.)
* Change the actual dither we use to blue noise. The old dither just
used a hash to add uniformly distributed random numbers to each
pixel value before quantization, completely uncorrelated from pixel
to pixel. Blue noise is specially constructed to still have a
uniform distribution of the output range, but it ensures that
spatially, similar values are distributed as far away from each
other as possible, rather than having similar values sometimes
clumped together. The result is a MUCH NICER appearance.
* Of course, this means the we needed to store a blue noise table in
the library, and some access routines for it. We incorporate a
public domain blue noise texture as a table.
* ImageBufAlgo::noise() and `oiiotool --noise` now take "blue" as a
noise name to return blue noise. Also, make "white" the (preferred)
synonym for "uniform" (to disambiguate the fact that both white and
blue noise are uniformly distributed; the difference is that white
is independently random for each pixel, whereas blue shapes the
spectrum to favor high frequencies, making it better suited for
dithering and sampling purposes.
* ImageBufAlgo::bluenoise_image() returns a reference to a stored
periodic blue noise image.
* oiiotool -d would let you say "uint10" or "uint12" to indicate 10 or
12 bit output (for formats that support it), but had no way to ask
for less than 8 bit output (for example, TIFF supports that). So we
add "uint6", "uint4", "uint2", "uint1" as recognized names for the
`-d` option in those rare cases when you specifically want to
produce a low bit depth file. And furthermore, if you ask for
`--dither` when using lower bit depths, the dither amplitude is
adjusted to be right sized for the bit depth you are using.
* Many format output modules that support 8 bit output honor the
option to dither the output, but in the "built-in plugins" chapter
of the docs, most did not list this option in the section that
describes which special output options it knows. So now we've added
this to the docs for all the output plugins that did support it.
Turns out that heif files that contain alpha can have either
associated or unassociated alpha. We had been assuming
associated. With LibHeif 1.12 and beyond, you can ask, so now we
do. Thanks Frédéric Devernay for pointing this out.
Similar to other formats that optionally have unassociated alpha:
* The default behavior is to associate/premultiply automatically when
reading, but set metadata "heif:UnassociatedAlpha" to indicate that
it was unassociated in the file.
* Open configuration hint "oiio:UnassociatedAlpha", when nonzero,
means to please keep the data unassociated and not premultiply. In
that case, metadata "oiio:UnassociatedAlpha" will be set in the
ImageSpec to indicate that the returned image (not just the file)
consists of unassociated values.
I did a couple other minor docs touch-ups while I was at it.
Fixes#3129
New ":C=name" modifier for --dumpdata will cause the dumped image data
to be formatted with the syntax of a C array, that can be dropped right
into source code.
$ oiiotool --dumpdata:C=foo image.exr
// image.exr : 256 x 256, 4 channel, float openexr
float foo[256][256][4] =
{
{ /* (0, 0): */ { 0.517036676, 0.261921108, 0.017822538, 0.912108004 },
/* (1, 0): */ { 0.653315008, 0.527794302, 0.359594107, 0.277836263 },
...
};
Without this modifier, --dumpdata will keep the same formatting it
always had.
This works pretty well for ordinary boring flat, single subimage,
non-MIPmap images. The option should probably not be used (or at
least is very unpolished) for multi-subimage, MIP-mapped, deep, or
other oddball cases (but you probably don't want to shove those into
a single C array anyway).
We had long ago established that the one in fmath was a slightly
faster formulation, as well as having the property of squashing
NaNs. There were a few remaining places where we used a different
clamp so fix them.
The DSLR raw format reader has the ability to pass hints to the
underlying LibRaw requesting that the pixel data be returned to us in
particular color spaces.
* Add new supported color spaces "DCI-P3" and "Rec2020" when using a
LibRaw new enough to understand them -- which means the upcoming 0.21.
* A new name to request the data be returned using sRGB color
primaries but a linear response curve: "sRGB-linear".
* The old name we used for sRGB-linear, "linear", is now considered
deprecated since it's ambiguous -- since its first use, new color
spaces "ACES", "DCI-P3", and "Rec2020" have been added, all of which
are also linear response.
When reading tga files, make sure that every seek and every read
checks results and issues error messages. Also double check that
large allocations succeed.
Also improve error propagation in several spot of iinfo, oiiotool
Owing to our C++11 focus in previous years, we had a number of places
were constants were declared as `static constexpr`. When done in
headers, this potentially causes separate copies of those constants to
be instantiated in each module that includes the header. It's not
really harmful the way we're using it for small constand objects, but
it's wasteful to potentially have dozens of copies of each those
constants by having it repeated in each module.
C++17 has a new idiom, `inline constexpr` that is more like we want --
makes it a true compiler constant that doesn't get instantiated in the
memory of each module.
We still have to cater to C++14, and in platform.h we already had a
C++-standard-dependent definition of OIIO_INLINE_CONSTEXPR that was
almost correct. Fix it (it wasn't previously used, but I must have
seen this coming at some point) so that it expands to
`inline constexpr` for C++17 and beyond, but stays as
`static constexpr` for C++14, and then use it for these constants.
Fixes#3117
If think that our inventory of UDIM tile files won't find names with
wildly out-of-range tile indices, you haven't been working in a
production studio! When those indices imply tile numbers < 0, we were
running off the beginning of the vector when filling in the udim
table, to hilarious results.
Oiiotool::extract_options was kind of fishy and brittle before --
splitting the string at ':' characters, then split each piece (which
should form a `name=value` pattern) at the first '=' character. There
are probably many malformed option strings that would have had weird
results with the prior naive parsing method. And it has been noted
that it can't handle a value that is intended to be a Windows filename
(like "C:/foo"), nor a searchpath (like "/usr/bin:/usr/local/bin").
Rewrite to parse it more carefully as name=value pairs delimited by
':'. In the process, we now have the ability to have the value be
enclosed by a single or double quoted string, in which case the
contents of the quote may themselves contain a colon, for example, the
following command and modifier is now no longer a problem:
`--foo:filename='C:/blah':count=3:`
We were already passing this on to libtiff when it handles the
compression. But for the case where we were calling zlib ourselves
(to facilitate multi-thread compression, which libtiff doesn't do),
we were just using the default compression.
This is inspired by a great [blog post](https://aras-p.info/blog/2021/08/05/EXR-Zip-compression-levels/)
by Aras Pranckevičius in which he made a similar change to OpenEXR
and benchmarked the results. He reported approximately a slightly more
than 2x write speed increase, while getting files that were only around
2% larger.
I tried replicating his experiment for TIFF, and while I don't quite
get results as good as he did, it still is really interesting. I
tested with 100 images randomly selected from the [Animal Logic Lab
USD scene](https://animallogic.com/usd-alab/) (they are almost all
4k^2, a mix of 1- and 3-channel images), converted from half exr to
uint16 TIFF images. I ran the tests for both scanline and tiled
images.
Compared to the default zip level 6, using level 4 resulted in 3%
bigger files, and around 40% faster write times. That seems like it
is a potentially very helpful tradeoff.
I tested the same set of images also with zip levels 8 and 2. For
level 8, the files were only another 2% smaller than the default level
6, but the write speed was 45-50% slower, so there is little value to
asking it to work harder on compression -- it just burns up time. For
level 2, it went somewhat faster (maybe another 10% faster than level
4) but the files started getting bigger, level 2 was 12% bigger than
level 6, that's definitely getting into territory where people might
mind the larger files.
So I want to add the control now, though I'm deferring a decision on
changing the default value until I can do some more extensive
tests. In particular, I would like to assess how it behaves with 8 bit
files (I was testing uint16, specifically), how well it works with
rendered images and with natural/photographic images (I was testing a
set of textures, which may not have the same characteristics), and
perhaps also see if results are better or worse for lower resolution
images.
The old libOpenEXR/libIlmImf (default) implementation handled
multithreading on the exr side of things, inside the readPixels method.
The new (experimental, enabled only by option) OpenEXRCore
implementation leaves the threading to the caller, and we weren't
doing it, so the performance was unsurprisingly awful.
This patch turns the implementations of the exrcore-based
read_native_scanlines and read_native_tiles (and also the deep
versions thereof) to use parallel-for loops, restoring parallel
processing to the exr reader when using OpenEXRCore.
Reading of whole scanline and tiled images now is on par or slightly
faster (a few %) than when using the main OpenEXR library. Reading of whole
scanline and tiled deep images is substantially faster (2-3x!) than
before.
I did some extra refactoring while I was there, simplifying and
eliminating some redundant error handling and using a new
DecoderDestroyer utility class.
STILL TO DO:
* See the long comment at the end of read_native_scanlines -- the error
message strategy implemented here is temporary, I'd like to improve it,
will probably involve both some work here as well as on the openexr
side.
If set to ON, will include the testtex program in the install.
The default is OFF, which builds it for tests, but doesn't install it
with the real command line utilities.
In forward looking code (current master means OIIO 2.4, which won't
be released until mid-2022), get rid of some stray logic catering to
OpenEXR releases 2.2 and older.
Background: Old style CMake was to use FindFoo.cmake modules, which
set FOO_INCLUDES and FOO_LIBRARIES variables. Best new cmake practices
are for the upstream packages themselves to export a FooConfig.cmake
and friends, which set up cmake targets. Of course, many packages don't
provide exported configs, so we use the old Find modules for many of
our dependencies.
I've slowly been migrating our Find modules to set up targets in addition
to the variables, and favoring use of the targets over the variables.
But I was wrong about this. It's more than just a choice of style.
When targets are used, use of transitive linkage (especially for static
library versions of dependencies) make the dependency targets show up in
our own exported OpenImageIOConfig.cmake. But to a downstream package,
now our config talks about a target like Libsquish::Libsquish, but it
doesn't know what that is, and it doesn't have the FindLibsquish.cmake
module that we used (internally) when we found that dependency.
This was causing particular problems for Libsquish on Windows with
vcpkg, but I see now that this could be a problem elsewhere as well:
field3d, libheif, openvdb, and webp.
So this patch:
* Reverts from the ill-advised targets back to the old style
variables, for the dependencies where we use *custom* Find modules.
(We presume that a dependency's exported configs, as well as any
Find modules that are distributed as part of CMake itself and define
targets, are done correctly, and will be available to downstream
packages, and so we continue to use targets in those cases.)
* Beefs up our FindLibsquish.cmake module to correctly find debug
versions of the library, to use in debug builds.
* Alters build_webp.bash to allow WEBP_BUILD_TYPE to override the build
type, which had previously been hard-coded to Release.
* Removes search for HDF5, which as far as I can tell was unused
directly by OIIO.
Instead of a build-time option for enabling (and always using) the
experimental OpenEXR "core" library...
* Build support for it by default any time we're using a new enough
OpenEXR version (>= 3.1). You can still hard-disable it being compiled
in at all with `-DOIIO_USE_EXR_C_API=OFF`.
* Select whether it's actually used with the global OIIO attribute
"openexr:core" (default to 0, disabled, for now).
This easily lets an app select support at runtime, and also may be
selected by user with environment variable
`OPENIMAGEIO_OPTIONS="openexr:core=1".
This all is intended to make it easier to benchmark use of this new
feature, switching back and forth without a rebuild.
* Honor env $OPENIMAGEIO_FONTS as an additinal font searchpath.
* Global attribute "font_searchpath" as an additional font searchpath.
* Refactor and simplify the code that assembles the list of directories
to search.
When $OCIO was not set at all, the call to OCIO::GetCurrentConfig()
was checking it again and writing the info message that none was found
to stdout, which we really don't want. And as Patrick pointed out
recently, asking for the "current" config is suspect, since who knows
exatly what the app has done.
So we're making the following executive decision: At least from OIIO's
viewpoint, if you want an OCIO color config, you either have to name it
expliticly, or have it determined by the `$OCIO` env variable. That makes
cleaner logic, and also rids us of the info messages.
I also now have the exported cmake config set
`OpenImageIO_HAS_OpenColorIO`. This makes it easier for downstream
consumers of our exported OpenImageIOConfig.cmake to know if the OIIO
build they have found has build-in support of OpenColorIO.
When it comes time to read_image() into the ImageBuf's internal
memory, we should have said to convert to `m_spec.format`, which
describes the buffer we allocated. Instead, we asked for `convert`,
which is the user request, which at that time can still be UNKNOWN,
which indicates to preserve the file's data format, which in fact does
not necessarily match the internal buffer.
This becomes especially troublesome for images with mixed per-channel
data types, which will obviously never match the internal buffer since
ImageBuf only internally stores one data format for all channels.
This is a very low priority change, but every little bit helps when it
comes to limiting boost's footprint as thing we used to require it for
become available in the standard library.
Add tests to be sure we have reasonable error reporting for at least
two important cases: a specified OCIO config file is not found at all,
and an OCIO config is present but cannot be read (in particular, if
the config is newer than the library). Because the exact error spewage
is different for each OCIO version, and apparently also depending on
the exact version of the yaml library that OCIO used, we have a few
different possible output references.
* oiiotool --nostderr (new hidden command)
This hidden flag causes oiiotool errors to go to stdout instead of the
default stderr. It only does this for oiiotool's own error and warning
calls; if there are other places within library functions that
directly output to stderr, they will not be redirected by this
command.
The main purpose of this is for testsuite and debugging. I don't
expect ordinary users to have any use for this, and so it is currently
hidden from the oiiotool --help explanation. If it seems useful for
something other than testsuite, I'll expose it in oiiotool --help.
It seems that for the case of OCIOv1 reading a v2 config, OCIO itself spews a bunch of warnings to stderr. And apparently the order is nondeterministic, because I just could not get any set of reference error output to match stably from run to run in CI. But the problem is, if I don't capture stderr, I also don't get the oiiotool error messages, which are the very thing I'm trying to test!
So what I did is add a new flag to oiiotool, --nostderr, which causes oiiotool's error and warnings to get sent to stdout rather than stderr. This lets me capture the usual output and the error messages from oiiotool, for this test, while allowing OCIO's stray stderr output to go into the ether where we don't care about its order.
I'm not sure if there's any non-CI, non-debugging application for oiiotool --nostderr, but maybe it'll turn out to be generally useful.
When OIIO that was built against OCIO 1.x, but encounters an OCIO 2.x
config file, OCIO could throw an exception that was not caught by
OIIO. The problem was the call to OCIO::GetCurrentConfig(), it should
have been inside the try/catch block.
Calling close() in open will forget too much state. All we want to do
here is make sure the TIFF file itself is closed at that point (so it
can be re-opened in append mode to add a subsequent subimage).
* CI test for VP2022, fix revealed python binding bugs
* CI test against ASWF's latest VFXPlatform-2022 docker container,
including Python 3.9.
* Pesky crash resulted in testsuite/python-imagebufalgo, laboriously
traced to IBA_color_range_check releasing the GIL too liberally. The
scoped release cannot contain the tuple construction!
* Audited the rest of py_imagebufalgo.cpp and found the same mistake in
only one other place: IBA_histogram. Was asymptomatic so far, but fixed
now.
* Presumably we just got lucky and these bugs were never symptomatic
until we started building against Python 3.9.
* Noticed that the recent change in build tree layout where we now
copy the test images into the build/testsuite area, resulted in HUGE
GHA build artifacts for failed tests. Modify ci.yml use of
upload-artifact action to exclude those directories.
* more tweaks
oiiotool --runstats tries to report how much time was taken by each
operation. But it has always had a major flaw: Because we try to read
input files lazily and use ImageCache underneath, only in certain
circumstances do the true cost of reading the files show up in the
entry for "-i"; often the cost gets wrongly credited to the other
command (like --add or -o) whose operations triggered the I/O.
This patch addresses the problem, and I think correctly credits I/O
time where it belongs.
The workhorse here is a new helper class within oiiotool,
OTScopedTimer, which does some tricks to try to account for both
direct read time and ImageCache I/O time, subtracting those from the
op it was instructed to time and instead adding it to the counter for
"-i". Because the destructor apportions time to the right commands,
this eliminates a whole bunch of spots where we did that by hand.
Also fixed a few places where ops directly called ImageRec::read()
instead of Oiiotool::read(), which would also result in bypassing some
timers we relied on. I made the ImageRec one be private, so harder to
make that mistake in the future.
Use a cmake idiom so that you need an explicit `-DCMAKE_INSTALL_PREFIX=`
in order to write over /usr/local or any potentially sensitive area.
In the absence of this being set explicitly, it will configure the
cmake build so that an "install" ends up in ./dist (which is what the
Makefile wrapper did all along).
Before doing all the work of copying the image, first see if the
`--ch` is actually specifying any change versus the image as it
stands. If not, skip all the copying and just stick to the image we
have.
The main benefit here is if you have a --ch primarily for "conformance",
e.g. you want to reduce to a known subset of channels and remove any
extra that may be unanticipated in the input, but it's possible that
the input will come to you already with only the desired channels. As
an example,
oiiotool in.exr --ch R,G,B ...more ops... -o out.exr
Previously, if in.exr aleady had channels R,G,B (and no other), there
would be some unneceessary expense of creating (in memory) a new image
for the output of --ch copying data to fill it in. With this patch,
the --ch would very nearly turn into a no-op. In both cases, if
in.exr's channels were something other than R,G,B, channel copies
would need to be done identically in both old and new code.
I realized after this was merged that I often share and copy a lot of
these little dependency build scripts across multiple projects, so I
wanted to remove the OIIO-specific naming.
To help make testsuite pass even with nonstandard relative path to the
build area, slightly change how we reference the OCIO config that is
in the testsuite.
Fixes python error about OIIO_TESTSUITE_ROOT not defined in the case
where the testsuite dir wasn't found in the standard places. This can
happen if you put the build directory in an unusual location that's
not right in the main project area.
* Ensure that "CMAKE" built "OpenCV" libraries can be found.
* Fix incorrect url to "openexr-images" repository.
* Fix typo in "FindTBB.cmake" module.
Co-authored-by: Thomas Mansencal <tmansencal@wetafx.co.nz>
* New SETVARIABLES option sets named cmake variabes to true if the
package is found.
* DEBUG should print variables even if not found. Duh, failing to find
the package is the most important debugging situation.
For historical reasons, our wrapper makefiles assume that we build in
`build/ARCH[.variant]`, so maybe build/linux64, build/linux64.debug, etc.
I think this is a throwback to literally decades ago when I would
build multiple architectures from different machines that viewed a
common shared directory of code, this allowing them to build
simultaneously and/or erase one without needing to rebuild the others,
etc.
But now I find that:
* It's contrary to modern cmake practice, and there are many benefits
to having our build directory structure identical to the vast majority
of other open source projects.
* I never want or need those simultaneous builds in practice, haven't
for a long, long time.
* ccache means that there is no longer an onerous cost to, for example,
having to blow away a release build and do a debug build and then
blow that away and do release again. It's just a few seconds to do a
ccache-based full rebuild of a recently-built tree.
So this patch switches all our assumptions to having a single level
hierarchy to the build and dist areas.
Also, I'm changing the assumption that the oiio-images (and other data
sets used by the testsuite) live "up a level", but instead added code
that either copies it from there (or another area settable with the
cmake variable OIIO_LOCAL_TESTDATA_ROOT) or downloads it, into the
build/testsuite directory itself. This simplifies a great number of things
in the test scripts as well as the test reference output (a lot of which
changes slightly to accommodate the new paths to test files).
* raw: Fixed output when raw:Demosaic == "none"
* raw: Include metadata regarding filter pattern
* raw: Added missing call to set unpacked status
* raw: Implement "balance_clamped" mode to help HDRI processing
* raw: Implement raw:apply_scene_linear_scale feature
* Fix up formatting with clang-format
* Update testsuite reference output
* raw: Fixed error message format handling
* raw: Updated documentation to include new raw features
* Ensure that all the `build_*.bash` scripts don't download anything if the packages appear to already be there.
* Env variable `OIIO_DEP_DOWNLOAD_ONLY` can be used to do the download only (if not present) and skip the actual build.
This combination allow the scripts to be run twice: once to do all the network I/O to download the packages we'll need, and a second time to trigger the builds.
The existing --info and --stats seem a little confusing and
inflexible. Unlike the majority of oiiotool arguments which are
evaluated in order, these merely set flags which apply to the entire
oiiotool command (specifically, they cause all image inputs to print
the metadata or image statistics at the time that the input are read
from disk). There isn't a way to make it apply to a single image, nor
to print the metadata or stats of any *computed* image, leading some
people to awkwardly save a computed image and re-read it merely to
print information about it.
So in this patch, we add new commands
* `--printinfo` which is like `-info -v` but is done immediately for
the "current" image on the top of the stack (even if it's a computed
image and does not correspond to any disk file).
* `--printstats` which is like `--stats` but is done immediately for
the current image on top of the stack.
Furthermore, `--printstats` takes an optional modifier `:window=`
which can be given a rectangular region in either of the notations
we use ("WxH+X+Y" or "xmin,xmax,ymin,ymax"), for example:
oiiotool blah.exr --printstats:window=30x30+1000+500
prints the statistics of the 30x30 region whose upper left corner
is pixel (1000,500).
The code to implement this is actually pretty short and straightforward,
but this patch turns out to touch a lot more. I needed to plumb
some of the info-printing internals for this support and did some
refactoring along the way.
Also, upon adding these to the docs, I realized that it would be
better to reorganize a bit to separate descriptions of the "flags"
from the "actions", and so many of the oiiotool arguments got shuffled
between different subsections, and in a few cases the descriptions
themselves got rewritten for clarity.
To help applications that need to deal with UDIM textures, the
TextureSystem now exposes the following public API calls:
* is_udim() : is a filename (or a `TextureHandle*`) referring to a
UDIM pattern / virtual file?
* resolve_udim() : given a filename (or handle) and s,t coordinates,
return the handle for the concrete file corresponding to those
coordinates.
* inventory_udim(): retrieve the full list of concrete filenames, as a
vector indexed by `(utile + vtile * nvtiles)`, and the nutiles,
nvtiles counts. Empty filenames in the vector indicate UDIM tiles
that are not populated.
Some extra functionality falls out naturally from these and existing
ingredients:
* inventory_udim + get_texture_handle together can get you the list of
handles corresponding to each found udim tile.
* resolve_udim + get_texture_info together can retrieve the attributes
of the udim tile corresponding to a particular texture coordinate.
Additionally, I have done some refactoring and performance
improvements of the internals:
* The internal UdimInfo that's stored inside the ImageCacheFile record
is now a vector, not a flat_map, so conversion of texture
coordinates to a handle is now O(1) rather than O(log n).
* Per Luke Emrose's suggestion, use `rcontains()` when searching for
UDIM pattern markers, since they will almost always occur close to the
end of the full pathname.
* Added TextureSystem and ImageCache methods that lets you find out
the filename that corresponds to a handle. (That's bound to be handy
for somebody.)
* Beefed up the udim tests to be sure to test every udim notation that
we support.
* At the suggestion of Luke Emrose, add rcontains() and ircontains(),
which are like contains/icontains, but start the search at the end
of the string rather than the beginning. This is measurably faster
than contains() for the case when you are reasonably certain that if
the search pattern is found at all, it is highly likely to be close
to the end of the string. (They're approximately the same speed when
the pattern is not found.)
* To verify this, I added a bunch of benchmarking to strutil_test.
* The benchmarks also revealed that contains was about 2x slower than
calling find! It should be the same. I don't know what the underlying
boost::algorithm::contains is doing differently than find, but whatever.
Changing our implementation of Strutil::contains to simply call find
doubles its speed.
* The new implementation of contains has an extremely minor behavior
change for the degenerate case of `contains("", "")`, which used to
return true and now returns false. Since the right behavior here is
debatable at best (is it false because the first empty string
contains nothing? or is it true because containts(a,b) should return
true if a==b?), I'm going to accept this behavior change as well
worth the 2x speed improvement (besides, "contains(a,b) should have
the same result as find(a,b)!=npos" is also a reasonable
expectation).
* The benchmarks also reveal that the case-insensitive functions (like
ifind) are not just a little bit slower than their case-sensitive
siblings (like find), they are 20x or more slower! I don't think
there is anything to do about that, but I did note it in the
documentation of those functions so that we are disuaded from using
them except when a case-insensitive test is really necessary.
The primary issue being addressed here was that get_texture_info() on a
udim pattern filename (like "foo_<UDIM>.exr") was unable to retrieve
info, except for an extremely narrow range of queries, because of course,
different UDIM "tiles" might have different attributes or be missing.
So in consultation with Lee Kerley, we decided that get_texture_info should
work as follows:
* If the attribute exists and has the same value for all of the
matching UDIM file files, then succeed and return the value.
* If any of the tile files does not contain the attribute, or if the
attribute doesn't have the same value in all found files, then it's
a failure and the value cannot be retrieved.
This necessitated a more rebust accounting of all the tile files
comprising a UDIM pattern, by doing a full inventory the first time
the UDIM pattern is encountered, rather than (ick) iterating through
all possible tile files every time we retrieve an attribute, as we
were apparently doing before. And so the following improvements were
also made:
* get_texture_info now works for more queries -- in fact, any query
that would be possible to ask about on an individual file, without
restriction. (Though, as I said, the query will fail if the value
isn't identical across all the found tile files.)
* The info queries (even the few that were previously supported) will
be MUCH FASTER than before, because we are doing the tile file
inventory once per udim pattern, rather than on every query.
* That inventory will also pre-build the association between u,v tiles
and the concrete filename (and texture handle!), so that each
individual texture call does a simple map lookup to get all the way
to the texture handle, without any string manipulation to first
construct a concrete filename, then look that new filename up in the
cache to get a handle, etc. I'm expecting this to reduce some of the
overhead of using UDIM files with the TextureSystem.
Note: I added an ImageCacheImpl::find_file_no_add at some point in
this work, eventually not needing it by the time I got to the final
version of my implementation of this PR, but I'm going to keep it
because I can forsee its usefulness and I don't want to delete it only
to write it again some day.
We now think that this C API should be removed, and replaced by a
different one coming soon.
Merely commented out the option and other lines in the top-level
CMakeLists.txt, since almost the exact thing will be needed eventually.
Also kept libOpenImageIO-C/CMakeLists.txt since we'll eventually need
one much like it. But deleted the .cpp and .h files, since those will
be totally replaced.
For the upcoming OCIO 2.1, the library names will incorporate the
major/minor version numbers, so our Find module needs to take that into
consideration.
Work towards eventually using OCIO exported configs (though still not
100% working now, may need fixes on their end).
Change CI to use the OCIO's that come in the aswf containers instead
of building from scratch in many cases.
Sometimes we use a filename, or pieces of it, as a regex.
But filenames can have characters that are interpreted differently when
part of a regex pattern (like `.`, which in a filename is literal, but
in a regex means to match any character!).
So this handy utility inserts the right backslashes so the filename will
end up as a literal match when part of a regex.
Furthermore, there's an optional parameter `simple_glob`, which when
true, will also substitute `?` with `.?` and `*` with `.*`.
We then use this to fix a latent bug in scan_for_matching_filenames():
it could have incorrectly matched files during the search.
I noticed when working on an ImageCache thing that even the smallest
change to imagecache.h was causing a substantial amount of the
codebase to needlessly recompile even though very few places really
need to use the IC APIs directly.
It took a little bit of juggling of some declarations, but this patch
allows imagebuf.h to be able to treat ImageCache and ImageCacheTile as
opaque, forward-declared types, without the need for every file that
needs imagebuf.h to also have to parse imagecache.h and everything it
might pull in.
This speeds up my development cycle substantially in many cases, and
it also speeds up compilation of user code (by a small amount) by
avoiding parsing of imagecache.h in circumstances where the user code
only needed to know about ImageBuf but not ImageCache.
There's a downside to this -- it's possible that user code that truly
needs both imagebuf.h and imagecache.h only includes the former
(inadvertently relying on the fact that the latter was transitively
included). These apps may need `#include <OpenImageIO/imagecache.h>`
to be added after upgrading to OIIO 2.3. Is that really really bad?
Added this handy function that figures out if the first string contains
any character present in the second string.
Also rewrite the comments describing parse_until and parse_while for
better clarity and correctness.
Some of the OpenEXR sample files had an attribute "version".
All along, I assumed this was telling me the version of the OpenEXR file
itself, so I was passing it along and also renaming "openexr:version".
Turns out I was wrong! This is just arbitrary metadata. Who knows why
it's in some of these files? Maybe nobody. But it's not meant to indicate
the file format version, and indeed it does not correctly reflect it.
In fact, there is no simple API call in the OpenEXR API that tells you
the file format version, you have to infer it from the features
present (multi-part or deep or long attribute name indicate changes
that came along with OpenEXR 2.x, but there is no version number in
the file per se).
So I'm just going to stop doing this rename, let "version" be "version",
it's just some user's metadata, I guess.
A few image formats (but not most) support "thumbnails" -- low-res preview
images.
Traditionally, we have shoved them in the ImageSpec metadata as a byte
array (and ints for the width, height, nchannels). This is less than
ideal: It makes the metadata very bloated, taking more memory and
being more expensive to copy metadata, and potentially wastes time
doing I/O on this data that most applications don't want or
need. (Remember that any alterations to the image will make the
thumbnail "wrong", also. So it's rarely useful except for applications
that specifically want a preview image.)
Discussion in Issue 2902 led us to think that thumbnails should be
treated explicitly by the API, rather than shoved into the metadata in
this way. This patch is the attempt to overhaul thumbnail handling:
* ImageInput has a new `get_thumbnail()` method that can retrieve the
thumbnail (if it exists) into an ImageBuf. The base class / default
implementation does nothing; the image formats that support
thumbnails should have their readers override this method. They are
encouraged to read the thumbnails "lazily" if possible -- only
if/when this method is called, and avoid the cost, memory, and I/O
if not requested.
* ImageOutput likewise has a new `set_thumbnail` method.
* These are implemented for Targa and PSD, the only two formats for
which we previously had implemented thumbnail functionality. (There
are probably other formats that support thumbnails, but for which we
never added OIIO support. Leave that for another day.)
* ImageBuf has new `has_thumbnail`, `get_thumbnail`, `set_thumbnail`,
and `clear_thumbnail` methods.
* ImageCache has a new `get_thumbnail` method.
* oiiotool and iconvert attempt to preserve a thumbnail from input to
output (but of course, only if both file types support it).
* ImageBufAlgo::IBAPrep clears the thumbnail in the destination image,
on the theory that pretty much everything IBA would ever do will
likely invalidate any preview image that it started with. This gets
us to the point with oiiotool where a straight copy (like a format
conversion) `oiiotool in.tga -o out.tga` will preserve the thumbnail,
but just about anything else `oiiotool in.tga --stuff -o out.tga`
will not transmit the original thumbnail to the output (which would
almost certainly be wrong).
There is more work to do in the future. Some file formats that support
thumbnails don't yet have OIIO support for it. I'm not sure we ever
really want (except in the case of a truly straight copy) for oiiotool
to keep any input thumbnails; maybe if a thumbnail is wanted for
output, it should always be created anew (by downscaling) so that it
definitely matches the main image? Maybe also oiiotool needs a command
to explicitly extract and save the thumbnail (is that needed for any
legit use? or only as a debugging tool?).
Imagecache repeatedly has to determine whether a filename is a type
of file that's "procedural" and does it expensively -- every time, it
creates the type of ImageInput in question just to ask it if it
supports("procedural").
We can ask this of each plugin type once, when we first register it.
Then just look it up when we need it.
This is strictly internal, has nothing to do with public APIs.
OpenCue-launched apps are encouraged to honor the setting of
CUE_THREADS to limit the number of threads they use. This change makes
oiiotool (and other OIIO-based tools and scripts) better citizens when run
as part of an OpenCue job.
CUE_THREADS has lower precedence than the more OIIO-specific
`OPENIMAGEIO_THREADS`.
For ordinary command line usage, this should not change behavior, since
you wouldn't expect CUE_THREADS to be set.
(Also fix docs which incorrectly named `OPENIMAGEIO_TEXTURE_OPTIONS`.)
And make sure to switch all of our internal uses of those to the
preferred new equivalents!
Also, hide some deprecated things from the documentation system so that
new people won't be tempted to use them.
These are all things that have been deprecated starting with OIIO 2.0
(or in some cases, even earlier). Multiple releases (2.0, 2.1, 2.2)
have gone by with them commented or documented as deprecated, but
without our adding the warnings. It's about time. We're still not
taking them away entirely, I usually don't do that until we've had at
least one release where it's been marked as OIIO_DEPRECATED, and also
only do true removals of API at major releases. But we're laying the
groundwork for being able to totally remove these when we eventually
have a 3.0 release.
This patch modifies the build system to make it possible to control
whether OIIO should link Boost in the form of static or dynamic
libraries, independently from the LINKSTATIC setting, without
having to use the fully custom route (BOOST_CUSTOM).
This is done by preserving the value of the Boost_USE_STATIC_LIBS
variable, if it has been defined by the user. If the value
of Boost_USE_STATIC_LIBS has NOT been explicitly defined, it is
set to match the value of the LINKSTATIC variable, which should
result in Boost being linked in the same way as other dependencies.
Slightly amends PR 3012
* Add docs for this recently-added feature.
* Change the make_texture configuration hint to "maketx:uvslopes_scale"
for symmetry with all the other texture conversion configuration names.
Everything works as before, including setting "uvslopes_scale"
metadata in the file. (But still accept the name without the
prefix as a synonym.)
* Change it to a float for a bit more flexibility.
Ordinarily, --mosaic takes the N images you have, arranges them into a
rectangular array of "cells" where the cell size is simply the biggest
image of the collection.
This adds the optional modifier `:fit=WxH` that lets you specify a
cell size expliticly. Images will be sized to fit the dimensions in
the same way as the `--fit` works (maximally filling the dimensions
while preserving aspect ratio of the original image).
This is particularly helpful for creating a contact sheet from full-size
frames, without having to do any resizing as a separate step.
The existing semantics of `oiiotool -info`, `-info -v` and `-stats`
are that these options set flags so that each time a file is opened
for the first time, it will print the metadata and/or stats. But these
do not allow you to create new images and print their metadata or stats
(at least, not without writing to disk and reading those files).
This patch extends the expression substition syntax to recognize the
following new metadata notations:
* `METABRIEF` : a string containing the brief one-line description that
would be printed with `oiiotool -info`.
* `META` : a multi-line string containing the full metadata that would
be printed with `oiiotool -info -v`.
* `STATS` : a multi-line string containing the image statistics that would
be printed with `oiiotool -stats`.
Some interesting use cases are illustrated with these examples:
# Roughly the same behavior as `oiiotool -info img.exr`:
oiiotool img.exr -echo "{TOP.filename} : {TOP.METABRIEF}"
# Perform computations on an image and print statistics about
# the computed image (not the original):
oiiotool img.exr -blur 3x3 -echo "blurred stats: {TOP.STATS}"
# Print statistics on intermediate results... and keep going with
# more operations:
oiiotool fg.exr bg.exr -over -echo "{TOP.STATS}" -o comp.exr
# Write metdata as "burn-in" on an image:
oiiotool in.exr -text:x=10:y=20 "{TOP.META}" -o burned.exr
The implementation in the business logic of oiiotool.cpp was very
simple. The more extensive change behind the scenes to make this
possible was a massive refactoring of printinfo.cpp, which previously
just wrote everything to stdout in a totally undisciplined fashion,
and transforming it to very carefully write to output streams instead,
which could be passed a stringstream to render the metadata listing or
stats output as a string. Also needed some refactoring to allow it to
take either the name of a file to read anew (as it did before), or an
existing ImageBuf/ImageRec (to support the new feature).
Internally, ImageBuf has for some time now supported strided or
non-contiguous buffers (for IBs that reside in memory, rather than in
an ImageCache). It was originally anticipated to accommodate cases
where we would want pixels or scanlines to have particular alignment
or spacing, such as to be even multiples of the SIMD width.
But we never allowed the kind of IB that "wraps" an application's
memory buffer to specify the strides, in case the app buffer was in
some layout that was not contiguous. This patch adds that ability, by
amending that constructor with stride values (which are optional and
default to AutoStride, which indicates that contiguity should be
assumed). This is 100% back compatible for source code.
This lets you do some cool things with in-memory buffers:
* More flexible about the kinds and layouts of buffers that you can wrap
to look like an IB.
* Can easily make an IB that wraps an interior rectangular subset of
another buffer (including another ImageBuf).
* To a limited extent, makes it possible to have an IB that is a "view"
of another IB (if the second IB is in memory), including strided pixels
or scanlines -- and even if the strides are negative (this lets you make
an IB view of a buffer where copying the buffer can reverse left/right
or top/bottom, however you can't use it to change the channel order or
layout within each pixel).
* I think this also lets you make a 2D IB that wraps a 2D slice along any
axis of a 3D volumetric IB.
Some other changes that came along for the ride:
* Added `ImageBuf::contiguous()`, which tells you if the memory layout is
contiguous (will return false if it's not an in-memory buffer of some
kind, so ImageCache-backed IBs are NOT considered contiguous).
* Renamed some internal types and fields, in particular, changes like
`size_t ImageBufImpl::m_pixel_bytes` to `stride_t m_xstride` to make it
clear that the values are strides and can be negative.
There are several alpha choices. We were assuming that color values
were unassociated (need to be premultiplied) unless it was marked as
premultiplied alpha. But it seems that, actually, we should only
premultiply if it's "useful alpha". [Indeed, their terminology is
the worst.]
Rename the m_alpha member to m_alpha_type to clarifywhat it holds.
Also add the alpha type to the metadata as "tga:alpha_type" to make it
easier to inspect the value in tga files.
I don't think issues have been reported, so maybe this wasn't a
problem in practice for reasons I've long forgotten about, but doing
some other work in ImageBuf, I noticed what I think was a thread
safety problem.
ImageBuf reads are done lazily, a lot of operations call
validate_spec() or validate_pixels() which can trigger a call to
init_spec() or read() if they have not yet been done, and those had a
spin mutex protecting them from being called at the same time. But the
underlying read() and init_spec() themselves did not have any lock,
which made me worried about what would happen if one user thread was
explicitly calling read() while another was calling it implicitly
through validate.
Also, spin_lock is probably inappropriate for something that could
trigger an entire image read from disk -- it's meant specifically for
situtions where the lock only will be held for a few instructions.
So, this patch:
* Renames ImageBufImpl::m_valid_mutex to m_mutex, reflecting that it's
not merely guarding access to `m_spec_valid` and `m_pixels_valid`,
but to potentially anything internal to the ImageBufImpl that needs
thread safety.
* Changes that mutex from a spin_mutex to a std::recursive_mutex, to
reflect that it *may* block longer than a few cycles, so blocked
threads should yield rather than spin. And recursive to simplify the
hand-off logic between validate_spec, validate_pixels, init_spec,
and read, the former two may call the latter two.
* Add locks to read() and init_spec(), which previously lacked them (I
think in error), in addition to the existing ones in the validate
methods.
* For some locking ImageBufImpl methods, add a param that lets the
caller (which will always be internal code) specify that the lock is
already held.
* Add a timing test of recursive_mutex to spinlock_test, just to verify
that I'm not doing anything to totally botch performance. On my Intel
Mac laptop, an uncontested lock acquisition/release is 6.4ns for a
spin_mutex, 16ns for a std::mutex, and 27ns for std::recursive_mutex.
So this is definitely a more expensive lock, but 27ns is still very
reasonable, and *most* operations don't need to acquire the lock at
all (first it checks the m_spec_valid or m_pixels_valid, once they are
set there is no more locking).
* Improve ImageBuf docs, adding more explanation including some guideines
about which methods are considered thread-safe.
* strutil.h already had a symbol called OIIO_FORMAT_IS_FMT defined to
0 or 1 that reveals (to clients of OIIO) whether Strutil::format()
behaves like printf (the old and current way) or like std::format
(the future way). Now we adjust so that the caller may pre-set the
value to force one implementation or the other, at least for the
"inline" functions of the headers.
* As an aid to downstream packages trying to ensure that they aren't
using the function slated for eventual deprecation or behavior
change, you can now define OIIO_HIDE_FORMAT to cause a deprecation
warning now on those functions that will eventually change or be
deprecated. This can help to identify any places where you call the
future-deprecated functions and change them to a "safe" variety.
ImageInput and ImageOutput have ways for the caller to say that there
should be no "fan-out" in the internals of what the reader or writer
needs to do. But TIFF wasn't honoring this.
This patch provides an additional but optional parameter for --bumpslopes option to mormalize the derivatives in UV space and scale them by a user defined constant. The scale factor is used to prevent half floating point overflow at time of the UV normalization. A value of 256 is recommended to prevent floating point overflow with texture resolutions up to 32k. The constant value is stored as an extra attribute in the output file for reverting the scaling when using the texture. Note that this options is only meaningful with height maps.
Co-authored-by: Pascal Lecocq <plecocq@imageworks.com>
* Add TypeDesc support for metadata that is a 2D or 3D bounding box,
represented as an array of 2 VEC2 aggregates (for 2d) or VEC3
aggregates (for 3d), having the BOX semantic. The basetype
determines whether it's an integer-based (generally for pixel
ranges) or float-based (generally for 3D spatial) bounds. The new
text representations are "box2" and "box3" (for float-based), and
"box2i" and "box3i" (for int-based).
* Somehow we never had a C++ test thoroughly exercising all the corners
of TypeDesc. Now we do: typedesc_test.cpp
* The test revealed a number of edge cases we don't handle well, now
shored up.
* New fmt 8.0.0 is out -- make sure we test against both that and their
current master.
* Starting with fmt 8.0, and in accordance with C++20 std::format's
final incarnation, the format string needs to be constant, for the
sake of compile time checks. To use a nonconstant format string, as
we do for these wrapper functions, we need to lean on vformat.
* For Strutil::sprintf, just `using ::fmt::sprintf` and eliminate the
wrapper entirely. (Though we express the wrapper inside
`#ifdef OIIO_OXYGEN_ONLY` so that it fully appears in any docs we
generate.)
Add custom UDIM support and Houdini UDIM convention support
For our (Animal Logic) in-house renderer, we need support for an additional UDIM format of `/path/to/file/textureName_u##v##.ext`
Added second line to ImageCacheImpl::resolve_udim to make this work.
Additionally add support for Houdini UDIM specification
* New Ptex (since 2.3.1) has exported cmake configs. Use those if
available and seem to work.
* Rename FindPTex.cmake -> FindPtex.cmake to match the capitalization
in the Ptex project itself, thus avoiding warnings from CMake.
* Fix our FindPtex.cmake to set up Ptex::Ptex_dynamic target. Use
that instead of the old style variables. That makes a seamless future
transition between the old Find module and future exported configs.
* Our FindPtex.cmake got the version number all wrong. Fix.
* gh-installdeps.bash tried to install ptex-base. That's not Ptex! So
stop doing that.
* New build_Ptex.bash does a simple clone and build of Ptex. Do so for
a few of the CI test cases so that we are more thoroughly testing Ptex
in CI.
LibRaw 0.19 and earlier use auto_ptr in public headers, which was
deprecated in C++11 and removed in C++17.
When this combo is detected, issue a warning message from CMake.
When using filesystem::directory_iterator, I was using operator++,
which can throw exceptions for OS errors. I should have used
increment(ec) instead. Throw in a surrounding try/catch just in case,
since even increment is not marked as noexcept.
* Use g++11 on the bleeding edge CI test
* Remove OIIO_CONSTEXPR20 decls from string_view -- gcc 11, even in
C++20 mode, doesn't have std::string fully constexpr plumbed, it's
not ready for this.
* Suppress gcc11 warnings about arithmetic with enums that occur in the
OpenCV headers.
* Suppress gcc11 warning (correct) in the spot where we were trying to
force a crash for testing purposes.
We were double-printing error messages. If maketx/oiiotool is itself
printing errors returned by IBA::make_texture, it is not necessary to
pass std::cout to make_texture so that it can echo the message to the
console itself.
It's meaningless to try to use make_texture on input images that are
"deep". We never even thought to check for this, which led to a
cascade of problems resulting in a segfault.
There's something wrong with the behind the scenes tricks we're using
to doctor the internal string representation. Is it because of big
endian? The "alternate" string rep? Something else different on
Apple's libc++ on M1?
Fix for now by just disabling that clause. The only harm is that
ustring internals will take more memory on that platform. That's
better than crashing!
When I have more hands-on access to an M1 (or when GitHub Actions
supports running CI tests on M1), I'll return to this for a proper
fix.
A few varieties of encoded videos with bit depth 12 and having alpha
were not marked as such, and falling to the default case that reported
them as 8 bit RGB.
The docs were ambiguous about what would happen for many-channel
image, and the behavor in fact was to copy and color transform only up
to the first 4 channels, leaving additional channels in the
destination undefined (though in practice they turned out black).
Clarify now that for colorconvert, colormatrixtransform, ociolook,
ociodisplay, and ociofiletransform, the first 4 channels are assumed
to be RGBA to undergo the transformation, and any additional channels
will be copied unaltered from source to destination (not set to black).
In ImageInput::read_scanlines and read_image, if somehow the image has
a spec with 0 size, it will lead to a divide by zero error. Check this
and fail safely.
For BMP in particular, do some sanity checks to catch corrupted headers.
(This is in response to Issue 2982)
For TBB 2021, headers moved around, and our FindTBB.cmake was failing.
But actually, starting with TBB 2021, TBB exports a proper cmake config!
So take advantage of that by preferring to load TBB from a config file.
Fixes 2963
Both global and per-package controls have been added to our
checked_find_package cmake function, to cause it to first check for
an exported config for the dependency, and only if not found, revert
to potentially using a FindPackage.cmake module.
This reflects our general leaning toward assuming that a package that
exports a cmake config probably does a better job of it than we would
do writing a Find module for it.
Rename FindOpenJpeg -> FindOpenJPEG to match the conventions used by
that project, in particular for its exported CMake config files.
(We don't yet rely on their exported configs, I need to do some
homework to see if they exist and are reliable as far back as the
oldest OpenJPEG release that we support.)
Add helper script to build OpenJPEG
We still had a few stray localtime() calls (which is not reentrant,
because it depends on a global buffer), even though all along we had a
"safe" wrapper in Sysutil::get_local_time, which internally calls safe
versions localtime_r or localtime_s (depending on the OS).
* Writing: Write single channel BMP as 8 bit palette images (not 24/32
bit images). This fixes a problem I discovered related to the recent
change that reads 8 bit palette images as if they were single channel.
The lack of symmetry there made problems for round-tripping an image
and having it end with the same apparent number of channels as it
started with.
* Reading: respect a configuration hint "bmp:monochrome_detect" that if
set to 0, will disable the detection of monochrome palettes and spoofing
to look like a single-channel image. The default is 1.
* Read RLE-compressed images correcty now! Both 4 and 8 bpp varieties.
* Beef up the testsuite/bmp tests, now test against a wider set of sample
BMP images from bmpsuite (including rle compressed images).
OpenImageIO exported config has a dependency on Imath/OpenEXR. For
Imath 3, I accidentally used OpenEXR_VERSION when I should have said
Imath_VERSION. Makes no differece if they're in sync. But should
somebody downstream have a separately compiled Imath and OpenEXR that
just might be different patch versions, it could break builds by
rejecting if the Imath version appears to be behind the OpenEXR
version. There's no guarantee that Imath and OpenEXR will always be
released exactly in sync.
Found some files that encountered the problem described here:
https://prepression.blogspot.com/2017/06/metadata-bloat-photoshopdocumentancestors.htmlhttps://feedback.photoshop.com/conversations/photoshop/photoshop-corrupt-ancestors-tag-in-xmp-causing-giant-file-sizes/5f5f45f74b561a3d426ba97f
The pathologically long lists of "photoshop:DocumentAncestors" tags (a
4MB PNG of which 3MB consisting of XMP) was tripping us up with an
O(n^2) handling of appending to lists for those attributes that could take
minutes per file.
It seems like this was a widely acknowledged problem that has been fixed
in Photoshop 20.0.2 (January 2019) and later.
Solution:
* Just skip the problem tag; I can discern no real use for preserving
it in any OIIO context.
* More generally, be on the lookout for pathological or corrupt XMP by
keeping track of the length of lists and just cutting them off from
further appending when they seem senselessly long. I can imagine XMP
with lots of attributes, but it's hard to imagine that any single
attribute having thousands of list entries is intentional and useful.
* Fix some places where we were needlessly generating and then tearing
down std::string's.
This makes several minor formatting changes, mostly related to how
clang-format 11 has improved ability to track the alignment of = in
successive statements, even when one of those statements is split
across two lines.
This started out as just trying to fix our CI on the "bleeding edge"
building against OCIO master and complaining about a recently committed
OCIO deprecation of Config::parseColorSpaceFromString.
But it turned out that the new OCIO v2 method, getColorSpaceFromFilepath,
didn't work quite the same way and broke our tests. So I ended up having
to implement a bit more of a complicated set of functionality:
* Config::parseColorSpaceFromString now wraps the OCIO version if it's
available, but implements the equivalent logic if OCIO is new enough
that the function is deprecated.
* As a bonus, our pCSFS now works (didn't before) in cases where lack of
OCIO support or missing config meant the OCIO functionality wasn't
available. That is, it works on our meager set of default color spaces
that are present even without OCIO.
* New Config::getColorSpaceFromFilepath wraps the OCIO version if OCIO
is new enough, but if it isn't, or if there's no OCIO, or if OCIO ends
up not figuring it out and returning the default rule, we fall back
to Config::parseColorSpaceFromString.
* Add OCIO to VFX Platform CI builds -- because it's not in the aswf-osl
docker images, we weren't testing OCIO in as many versions as we had
previously thought.
* Bump default OCIO version built by build-scripts/build_opencolorio.bash
to 2.0 (from 1.1.1).
the temporary image buffer is created with only 3 channels, so the get_pixels call at the end of the
read_unmipped method must account for that when creating the ROI passed to it.
This also bumps compiler minimums to gcc 6.1, clang 3.4, MSVS 2017,
and icc 17 (all because those are the first versions to fully support
C++14).
This has been coming and has been socialized for a long time, with
nobody voicing serious objection. So here it is.
This WILL NOT be backported to release branches. The new minimums only
affect OIIO 2.3 and later.
When last discussed on the oiio-dev mail list, I said I would aim to
introduce this change on May 1. I will not merge this PR earlier than
that, and I will tag a developer preview release before that happens.
The new `--missingfile` argument (taking a value of either `error`,
`black`, or `checker`) specifies the policy for what oiiotool does
for input files that are entirely missing (not present but broken).
In all cases, an error messages is issued, but for the cases of
black and checker, an image is substituted and computation continues,
rather than immediately ending the attempt to execute the command
sequence.
On a personal project a while back, I wasted a full day chasing down
inexplicable behavior that turned out to stem from TWO implementations
of ustring being in the same application, a result of inadvertently
linking against both libOpenImageIO and libOpenImageIO_Util. This led
to two ustrings that contain the same character sequence, but with
different pointers because they resided in different ustring tables.
There is no way that anybody without very deep understanding of ustring
and a lot of patience could have tracked down the underlying issue.
This patch eliminates the possibility of duplicated modules in
libOpenImageIO and libOpenImageIO_Util. Now libOpenImageIO depends on
and links to the latter. This should all be automatic for users -- as
before, link against libOpenImageIO if you want everything, link
against libOpenImageIO_Util if you just want the util helper classes
but don't need any of the image I/O functionality. But now, if you
accidentally specify both... well, that's just the same thing as
asking for libOpenImageIO.
Fully separating libOpenImageIO from libOpenImageIO_Util requires new
OIIO_UTIL_API
Make whether namespace includes the patch number be dependent on
whether it's a dev or release branch. We generally want the patch
in the namespace for master, but not release.
This teaches oiiotool's command line expression substitution (`{...}`)
syntax to understand that
getattribute(name)
should be replaced by the value that OIIO::getattribute(name,...) would
retrieve. The `name` may optionally be single or double quoted, but doesn't
have to be if it's a simple identifier.
Some examples:
# Which SIMD features does this OIIO use?
oiiotool --echo "{getattribute('oiio:simd')}"
# Which dependencies are used for each format?
oiiotool --echo "{getattribute(library_list)}"
# Which file extensions are associated with each format?
oiiotool --echo "{getattribute(extension_list)}"
The cv::Mat::create method does the same thing as the constructor but "Allocates new array data if needed." And when not needed, which is my use case when all the images I process are the same size, create does nothing. This is more efficient than the implementation where a new mat is constructed and then moved to the destination. This is a minor optimization, but it does avoid allocating an image size block of pixels when it is not necessary.
Sorry this has taken so much tinkering to get right for downstream
projects. It's tricky business.
First, we start by adding testsuite/cmake-consumer -- a test that
consumes OIIO and its exported cmake modules, and is independent of
the rest of OIIO's build system, more accurately simulating whether
things will work for a downstream client.
Adjust our exported module, it needs to break out the OpenEXR 3 vs 2
cases separately, as well as whether it found an OpenEXR/Imath config
file or not. I think I have all the cases covered now. I also needed
to specify find_dependency of IlmBase and Threads, for the sake of
openexr 2.4 with config, and add the INCLUDES directory lists in the
case of no config files found.
Some adjustment of our library target_link_libraries -- better PRIVATE
hiding of things not needed in the interface by downstream builds.
In particular, the OpenEXR components and OpenCV should be private, not
public.
Add m_allgray field and color_table_is_all_gray method to detect
that the color table entries have the same r, g, b values and indicate
a single channel gray scale image rather than a 3 channel rgb image.
Temporary allocations were made and freed for each scanline read or
written. Do it once per image and reuse. When reading, eliminate one
temp buffer by writing data into its final location, also eliminating
a needless extra copy of each scanline.
We broke things subtly for older OpenEXR when we added the 3.x
support. Without this fix, our export CMake config files weren't
properly exporting the OpenEXR include path as the interface includes
for OIIO libraries. This doesn't affect the OIIO build itself, but it
can affect downstream libraries that use OIIO as a dependency and
rely on our exported cmake config files.
We also remove the need for OpenEXR/OpenEXRConfig.h entirely through
header configuration.
Ever since adding OpenEXR 3.0 support, we had some cases where
installations that built the 2.x IlmBase and OpenEXR portions
separately and installing to separate areas.
Also make some minor changes to ci.yml to make sure we are documenting
the right openexr versions used for each test.
It seems that for CMake, --config should only be used in conjunction
with --build. Who knew? Things are breaking right and left now, especially
on GitHub Actions CI. But also on my laptop.
Hypothesis: maybe this was silently ignored in older cmake versions, but
now with CMake 3.20 is a hard error?
This does not (YET) change the minimum to C++14. For now, you can still
build for C++11 -- and with compilers that only support C++11 -- by using
CMAKE_CXX_STANDARD=11.
By changing the default, we hope to more thoroughly test everybody's
ability to compile C++14, ahead of the actual cut-off when we make that
the true miimum.
Also, the latest OpenVDB 8.0 does has C++14 as a minimum, so compiling
in C++11 mode will automatically disable OpenVDB support if OpenVDB >=
8 is what is found. We expect an increasing number of our dependencies
to drop support for C++11, so we want our default compilation mode to
be one that doesn't silently drop format support without
users/builders being aware of it.
In many cases, this informative debugging code was left over from when
parts of this code was strictly a command line utility. It's not
appropriate for a library call, especially on Windows.
Make sure every last output to std::cerr is guarded by some
preprocessor symbol that would only be enabled when one of the
developers is trying to do some "printf debugging". And similarly,
std::cout should only be used for standalone command line
utilities that are by design supposed to output to the console.
We were treating some recoverable warnings from jpeglib as fatal errors.
Removed testsuite/jpeg-corrupt-header since now we can read it just
fine despite the very minor corruption! Also, turns out the image was
inappropriate, sorry, I had never actually seen it before because
OIIO wouldn't read and display it and I assumed it was just garbage.
Apparently all the recent farmhash changes are fine with C++14 mode,
and fine with C++11 mode on gcc 4.8, but gets some errors when
usig in newer compilers (gcc6+) using C++11 mode.
Add a CI test with gcc10 but with C++11 compatibility to verify this fix.
We will remove this test once we retire C++11 compatibility.
For the case of applying a color transformation to just one color, we
were sloppy and set the strides to 0 (just one color means we are not
iterating from point to point using the strides).
OpenColorIO 1.x didn't care, but OpenColorIO 2.0 throws an exception if
the strides don't look valid.
Thanks to Thomas Metais for the fix suggestion.
Change imageio.cpp convert_from_float to use the convert_type function
from fmath.h, rather than defining redundant functionality in that
module.
This has the added benefit that the ones in fmath.h have some interesting
specializations to use SIMD ops and other optimizations that the one-offs
in imageio.cpp lacked.
Ordinarily, oiiotool will exit upon encountering an error (such as an
input file not being found). When operating on a frame range or with
frame number wildcards, that means that it will exit without attempting
the rest of the frame range.
The new `--skip-bad-frames` command line argument means that an error
will skip the rest of that iteration, but not exit, instead starting
fresh on the next frame in the sequence. (But if any frame in the
sequence encounters an error, the exit code of the oiiotool command
will still be EXIT_FAILURE.)
GitHub Actions in its wisdom has removed pre-installed Boost from the Windows
images, causing our CI to fail.
Instead, get boost from vcpkg, just the pieces we need.
Also needed some JPEG adjustments for Windows CI build.
Added utility function OIIO::get_extension_map() which returns a
`std::map<std::string, std::vector<std::string>>` listing all
image file formats and their presumed file extensions.
`oiiotool --list-formats` helps to test this and is useful in its own
right.
Fix reading tiled tiff files when vertical read size is less than tile size, just for the code path where we use TIFFReadRGBATile (used for certain rarely encountered types of TIFF files). TIFFReadRGBATile returns the image data in bottom-to-top order, and we had been correctly reversing it all along, but neglected to account for the issue of a partially-filled tile ending up with the padding at the beginning of the buffer rather than the end.
All of our prior tga tests had valid thumbnails. The code skipped the
thumbnail if the "offset to the thumbnail" was 0. But if the offset was
nonzero but the thumbnail record at that spot had a 0x0 size, we just
ran merrily along, and on Windows in debug mode even it an assert.
This patch makes it understand that a 0x0 thumbnail size means no
thumbnail.
I added an image from Doug Rogers, the reporter of the problem, to the
oiio-images repo in the targa directory, and updated the OIIO testsuite
to also check that image. (So testsuite will fail for people who don't
update their oiio-images repo.)
Also changed some stray refs to &vector[0] to vector.data().
Fixes#2900
The very old OS2_V1 variety of BMP files -- which dates from the OS/2
and Windows 2.0 circa 1987 -- could have undefined results by relying
on the cpalete header field that didn't exist in that early
version. Our failure to initialize it led to expecting color palettes
of undefined size (which could either cause a read failure or even a
crash if it vastly over-allocated).
Some other minor touch-ups to the bmp code while we're at it:
* Zero-initialize all the other fields that don't exist in the OS2_V1
version of the header.
* Don't set the Resolution metadata if the values in the file aren't
positive (including for the V1 header that lacks it entirely).
* Fix reading V1 header resolution on big endian architectures (I just
happened to spot that potential bug).
* Fix longstanding error (that I only just noticed) in reading 4 bpp
images with odd horizontal resolutions, wherein the very last column
was not filled in correctly (that's the change at lines 188 and
193).
* Beef up one of the error messages invoked for failures while reading
the color palette.
* Rig the BMP reader to know the "dib" is another extension often
associated with BMP files.
* Add a regression test case for this format variant.
* Make sure CI tests bmp -- the GHA CI did not download the bmpsuite and
so was not actually running the testsuite/bmp tests.
* Add "bmp:bitsperpixel" (when not 24 or 32) and "bmp:version" metadata.
This is primarily for debugging and understanding which encodings the
BMP file is using.
* Recognize nonsensical values for the header's field for number of entries
in the palette. Add a test for that.
This patch ensures that two ustrings will never have the same hash value.
The basic strategy is that the usual 64 bit hash of the characters provides
an initial guess. If no other string has already used that hash (as is
almost always the case), we use it. However, if a different string already
took that hash value, we rehash, and repeat until we find an unused hash.
We maintain a "reverse hash map" that relates hashes to ustrings to
help figure this out efficiently. (Note: This adds at least another 16
bytes per unique ustring to maintain the reverse map. Do we care?
Probably few applications are in the business of creating more than a
few million distinct strings.)
Because we are recording the collisions as they happen, this also
greatly simplifies (and reduces to trivially low cost) the
recently-added ustring::hash_collisions() function, because there is
no complicated search when it is called.
When making a texture out of float values, but writing ultimately to a
`half` file, it's not too hard to end up with valid but large float
values that are outside the range representable by a `half` (fp16)
value and therefore will get converted to an `Inf`, which nobody wants
in their texture.
This patch prevents Inf from showing up in the output of make_texture
by clamping output buffers to the range of `half` when the source data
are float and the destination file is half.
Fix minor warnings and deprecations of C++20.
Bump to more modern ubuntu images for recent-year tests and make some
related adjustments. Also make sure that the "latest" and "bleeding edge"
tests are using python3, they were inadvertently still using python2.
A recent commit in the OpenEXR master changed which headers are
transitively included by other OpenEXR headers, and that broke our
build until this patch adds the missing headers explicitly.
The change may or may not be reverted on the OpenEXR side, but this
patch to OIIO at least unbreaks our CI in the mean time, and has
no down-side.
Update old calls to Strutil::format to new fmt conventions, convert
some sprintf-like calls to std::format like calls.
This is not meant to be comprehensive; there is more work to do.
It's just an opportunistic bite at getting some of it done.
The oiiotool --colorconvert operator (and the other color conversion ops)
have an optional modifier ":unpremult=1" that will bracket the color
transformation with an unpremultiply/premltiply. But the implied
colorconvert that happens upon input and output with --autocc has no such
option, which is an oversight. This patch adds the ability to control
with autocc transformations happen in an unpremultiplied state (which I
believe they SHOULD, generally speaking).
Also beef up the docs and help messages in some areas where I
coincidentally realized that -i and -o explanations omitted some of
the possible options.
There were some flaws in the logic of IB::set_write_format that could
cause a crash when used in conjunction with a file format that doesn't
support per-channel data types (even if the set_write_format call
specified a single type rather than per-channel types!).
There are basically two problems I fixed here.
First, in ImageBuf::write, where we are dealing with the fact that
set_write_format previously registered a request, be extra careful
that the "default" data format is the "biggest" type out of the
per-channel choices, and also make sure that if all the channels are
identical, we just use the default type and clear the channelformats
vector.
Secondly -- and more importantly to the crash itself -- in
ImageOutput::to_native_rectangle, if the output file format does not
suport per-channel data types, just use the main format. Do NOT
consider this case a hard error, which previously took an early out by
returning NULL for the location where the native data was copied. It
was that NULL that, when subsequently copied from (not noticing that
it was NULL indicating an error), actually produced the segfault that
caused the crash. In other words, don't worry, be happy -- just do
your best by treating the unsatisfiable request as the next best
thing, as if it were the same type for all channels.
Homebrew just upgraded to Qt6, and that breaks us. Peg to Qt5 as a
temporary measure to let our CI pass, until somebody figures out what
needs to be fixed for full Qt6 compatibility.
I have submitted a PR to Imath that adds "interopability" constructors
and assignment, that use some templates to allow seamless
construction, assignment, and passing of "foreign" vector types to
Imath vectors. This makes it easier library A that uses Imath vector
types in its API to interact with app or library B that has a custom
internal vector type, without a ton of ugly casts and copies.
(See https://github.com/AcademySoftwareFoundation/Imath/pull/91)
These created a few minor conflicts with OIIO's internal SIMD classes
and their construction and casting from Imath types. This patch cleans
up the ambiguities:
* Make Imath::V4f -> simd::vfloat4 and Imath::M44f -> simd::matrix44
constructor `explicit`.
* Make the simd types `simd()` method (returning the underlying raw SIMD
type) also have a variety that returns a ref to a non-const vector,
to ensure proper working of the _MM_TRANSPOSE4_PS macro.
Now I am going to commit a sin. This MUST get backported to OIIO 2.2
in order for it to not break against the upcoming Imath 3.0. This
doesn't break ABI compatibility, nor change OIIO's primary advertised
public APIs, but it does change the technically/informally public APIs
of simd.h in ways that introduce the possibility that an app using
those simd vector classes *might* need minor source code editing if
they relied on those implicit constructors. Such software might need
certain `simd::vector4 foo = imath_bar` to become `foo = vector4(bar)`
and the same for certain matrix44 assignments. It is usually a big
no-no to backport any change that could require editing of source
code. But since the alternative is not being able to use the new
Imath, I think this is the lesser evil, and maybe nobody out there is
doing the thing that would need to be changed?
Need some include fixes to more gracefully handle the case where
Imath_ROOT points to a custom build (say, of Imath 3.x), but there is
a system installation of a different version (say, OpenEXR 2.x in
/usr/include, which could easily end up earlier in the path list).
Also update some reference output due to recent bug fixes in OpenEXR,
where error messages changed.
* Better handling of lists/sequences -- append into attribute arrays
instead of allowing each value to overwrite the previous one.
* Better inference of unknown attributes that look like int, float, or
rational, rather than cramming into strings.
* Fixed what I now think were bugs in handling rational values.
Turns out it doesn't -- we're still seeing timeouts on the Windows CI
tests, even with the OpenEXR that we thought fixed the threadpool
problem.
But let's keep this change anyway.
Background: In our GitHub Actions CI, we were doing environment setup,
dependency setup, OIIO build, and testsuite -- all in one action
"step" because each stage is its own shell process, so I couldn't
figure out how to communicate between them (for example, a dependency
setup might build and install a dep package, but then the later build
stage needs the CMake Pkg_ROOT variable set to that location).
Doing it all as one "step" was clumsy, not best GHA practice, and it
made it hard to understand which stages were taking different amounts
of time and how to optimize them.
The trick, it turns out, is that $GITHUB_ENV is the name of a file
that will be executed at the start of each step. So you put commands
into that location that restore whatever env variable you need to
restore between steps, or any other setup you need.
So in this patch, we:
* Elevate env setting to be for all steps.
* Split build-and-test.bash into separate build and test scripts.
* Separate setup, dependency installs, build, and test into separate
GHA steps.
* Each step's script, at its very end, calls new script save-env.bash,
which puts commands to restore the environment into the file named
by $GITHUB_ENV, which will be read at the beginning of the subsequent
step.
* Merge gh-installdeps and gh-centos-installdeps scripts -- there was a
lot of redundancy between them anyway.
The net result of this is that when we examine the test results, it's
much easier to see which stage a failed test had trouble on, and
especially how much time each stage took. That's really handy with the
recent changes to add caching, and helps us verify that the caching is
working as intended, since it is expected to take a big bite out of
the dependency and build stages, but not the test stage.
This speeds up CI builds by 1/2 - 2/3 of total time, for PRs that are
minor changes by reusing the compiled results from previous runs.
Most of the remaining time cannot be squeezed down -- it's the
instantiation of docker containers and running the testsuite
itself. But the build (including of dependencies we build from
scratch) is dramatically faster.
Currently, this depends on ccache, so is for Linux/Mac only. I'm also
working on doing something similar for Windows, but it's not ready for
review yet.
Do some adjustment/simplification of the macros that set up the right
decorators on the new inline farmhash.
Some additional constexpr's, including Strutil::strhash().
Add to hash_test verification of the actual results of each of the
hashes, so we know that we don't break them over time, that they give
the same result on all platforms, and most immediately, to 100% verify
that the new constexpr inline version of the hash precisely matches
the old procedure call one.
Augment the existing benchmarking of the hashes to include
farmhash::inline. It tries a variety of string sizes. I was worried
that the constexpr/inline version would be a performance hit because
it can't use any of the SSE intrinsics (which, ugh, aren't declared
constexpr in the headers). But it turns out that the inline (but
SSE-free0 version is a little faster on some tests, a little slower on
other tests, overall a wash and never differing by more than
10%. Maybe that means that the inlining/constexpr makes up for the
lack of SSE? Maybe it means that the compiler optimization is adding
SSE back in well enough that losing the intrinsics isn't as bad as we
would have thought? In any case, the upshot is that this doesn't
represent a performance it, so I may consider a future change to just
go strictly with the inline constexpr versions and eliminating the
separately compiled version altogether. Will mull that over.
* cmake: Make LibRaw Jasper and LCMS2 detection optional, add LIBRAW_NODLL
* cmake: Fix OpenColorIO defines on Windows to use correct cmake mechanism
* cmake: Add missing LibRaw defines (needed when linking statically)
Some fields have disappeared from certain structures, so we need to make
adjustments to the various conditional compilation guards.
This patch just makes OIIO build against the new libraw, and is intended
to be backported to the release branch.
This patch does not incorporate any new fields added to libraw
structs, representing newly retrieved metadata. A separate PR patch
will address the newly added capabilities (TBD whether or not that
ends up safe to backport and when).
Bump CI "current releases" test to OCIO v2, it's released!
Oops, we used the wrong name for the env variable, and that CI test
wasn't getting the OCIOv2 RC all along!
Add OIIO::ColorConfig::OpenColorIO_version_hex().
Have oiiotool --help print the OCIO version.
Also Bump the pybind11 version for the "latest" test, and update
INSTALL.md a bit to document the latest versions known to work of
several dependencies.
When we wrote our `span<>`, we based it off the draft for std::span,
which at that time specified span::size() as ptrdiff_t, which is
signed. By the time std::span got finalized in C++20, it was changed
to size_t, which is unsigned. Ick. Switching to size_t will be an API
break, so we can't do that until we're at a version where it's ok to
make breaking changes.
But we can stage a few changes that will make that future transition
easier.
* Add std::size() and std::ssize() wrappers for both span and
string_view. This gives downstream apps the opportunity to avoid
span::size(), instead using std::size(the_span), which is guaranteed
to be unsigned, and std::ssize(the_span), which is guaranteed to be
signed, and both of these will work now as well as after we make the
other breaking changes.
* Doing this properly required adding a couple 'constexpr' in places
where they should have been all along.
* Define macros OIIO_SPAN_HAS_STD_SIZE and OIIO_STRING_VIEW_HAS_STD_SIZE
making it easy to know if std::size and std::ssize are defined for
span and string_view
* OIIO_SPAN_SIZE_IS_UNSIGNED will be defined when OIIO::span switches
to having an unsigned size like std::span (making it easy for client
software to know if span::size is signed or unsigned).
Turn this on automatically for OIIO >= 3.0 so we don't forget.
If client code predefines OIIO_SPAN_SIZE_IS_UNSIGNED before
including span.h, that forces span::size to unsigned now (good for
experimentation and compatibility testing, but I don't recommend
anybody ship software in that state unless they really know what
they are doing and don't mind potential incompatibility with the
out-of-the-box OIIO).
These SHOULD NOT break any existing code or ABIs that uses our
existing span and string_view classes or their size() methods. It only
adds options that will help bridge compatibility.
A variety of small problems were fixed:
* Symbols in libOpenImageIO-C that were not marked as OIIOC_API.
* MSVS not being happy with array length determined by a variable
* Issues with Windows paths
This is a weird thing we're doing in this very artificial test,
removing a file when the IB reading it is still alive. That's safe on
Unix, where a file with an open file handle still be safe to read even
if it's removed. But it's not kosher on Windows. Fix by simply making
sure that in this test, we aren't doing the unadvised thing of having
the IB still active on the file we're deleting. This should fix the
unit_imagebuf test that is failing our Windows CI.
* Try to address the OpenEXR thread pool hanging when we call exit().
* Strive to remove all exit() calls after any file reads happen,
in oiiotool, iconvert, and idiff.
* This DOES NOT stop all hanging. It's still a bit of a mystery whether
there is anything we can do to fully prevent it without a fix on the
OpenEXR side.
* Temp workaround: for the Windows CI tests, use ctest commands to
allow each test to timeout 3 times before giving up. To be clear,
this does not solve the problem, it only masks it. But it helps
us "pass" those tests, exposing any other problems occurring with
them, and we will return later to find the source of any residual
timeouts.
* Update ref output for several tests where the differ slightly under Windows.
* Make runtest.py convert text output line endings cr-lf -> lf before
comparing to reference output. (This eliminates unnecessary diffs.)
* Update many tests' run.py file to use double quotes instead of single
quotes for arguments containing some characters -- looks like the Windows
shell invoked by Python system() does not handle the single quotes quite
the same way as Unix shells.
* oiiotool: when printing the command line after errors, genericize
identifiable filenames so they render the same on Windows and POSIX,
and strip directory name from the oiiotool filepath. This makes
error output comparison more equal between Windows and POSIX, making
several more tests pass.
In total, this seems to get us down to failing only 7 tests (not counting
spurious timeouts that cause failures).
Similar to C++17 std::filesystem::path::generic_string(), but for a
filepath sent as string_view.
While I was there, I noticed that a few Filesystem functions that I
declared as `noexcept` actually call things that can throw, so I put a
few try/catch in stratgic spots. Nobody had reported a problem with
this, and maybe it would be fine, but it's the right thing to do.
* Change to finding a consistent Python version (3.7)
* Install numpy
* On Windows, convert crlf to lf before diffing reference output, so we
don't get mismatches just because of LF vs CRLF.
* Update ref output on testsuite/python-imageinput
* Fix windows detection in runtest.py oiio_relpath
* Fix some semicolon problems that were making bash command lines conused
Along for the ride:
* Have ci-build-and-test.bash send OIIO_CTEST_FLAGS as extra args to ctest
* Remove some needless verbosity from the dependency building scripts.
With some recent heic related packages, we lost the ability to build
against some older libheif versions that we purport to support. We may
eventually want to raise the minimum supported libheif, but in the
mean time, this restores the prior build abilities.
Use pybind11 functions to set up python module, it's got superior logic
to what we were doing.
Bump pybind11 version default in build_pybind11.bash
Suppress clang warnings about python register use
* Re-enable vcpkg installation of freetype on windows
* Add TESTSUITE-README.md file
We'll use this file to document how the testsuite works and how users
should update it when necessary.
* Re-enable windows tests
Fix a few issues parsing rational values with large denominator in OpenEXR header.
Incorrect array index can cause divide by zero crash (observed consistently on Windows with repro image).
Conditional comparison was assigning 1 or 0 to int f declaration instead of gcd value.
Changed gcd type parameter to long long to avoid unsigned int->long int overflow on windows (long int is 4-bytes on windows).
Co-authored-by: Chris Crosetto <ccrosetto@gmail.com>
* fixes to make vs2019 CI run. Not passing yet as some tests fail
* added explicit ctest timeout (180sec) + misc fixes to run CI local
* disable Windows VS2019 tests again as they are not passing yet
Co-authored-by: Dieter De Baets <dieter.baets@unity3d.com>
OSL has some concerns about how ustring hash collisions could impact
its strategy for strings on GPU.
This patch adds some instrumentation to ustring to detect hash
collisions. Added static ustring methods total_ustrings() and
hash_collisions(). The latter tells you the number of collision pairs,
including optionally enumerating them into a vector of ustrings. Also,
`getstats(verbose=true)`, in Debug builds only, will print all of the
colliding pairs. These are both somewhat expensive if there are lots
of strings (my laptop takes about 4 seconds to find and print all
collisions for 10M ustrings). So if you are concerned about
performance in debug builds, call getstats(false) instead if you have
a truly enormous ustring table.
This addresses slight ringing at image edges when using a combination of
black wrap mode and a filter with negative lobes. We need to be sure that
we really clamp the filter at the source image edges.
But we only want to do the clamping selectively. Works great for --fit,
not so much for more general warp and rotate. So add it as an internal
parameter to the underlying warp we're doing.
Needed to update a bunch of the fit-related output reference.
There are several ways to make an image fit a new size while preserving
aspect ratio. Originally, fit() would do what we now call "letterbox"
mode, which tries to maximally fill the new size, retain the original
aspect ratio of the visible image, exactly fill one of the dimensions,
but not lose any visible image data -- this means that a dimension that
cannot be filled will be padded with black pixels. This is still the
default behavior.
But additional modes are not possible: "width" means to exactly fill
the horizontal dimension (the vertical may be letterboxed, or may be
cropped, depending on if it's too big or too small), "height" means to
exactly fill the vertical dimension (the horizontal may be letterboxed
or cropped).
* Add `fillmode` parameter to IBA::fit() in both C++ and Python.
* Old fit() without this parameter are still present for
back-compatibility, but are deprecated and will eventually (when
we're at an ABI-breaking release in the future) be removed.
* oiiotool: Add optional `:fillmode=` modifier to the `--fit` command.
* More comprehensive tests in testsuite/oiiotool-xform, testing every
combination of portrait, landscape, shrinking, growing, each fill mode.
* Update docs
The WebPData::size is a size_t, on 32 bit systems it will balk at our
putting a uint64_t in it. Downcast explicitly so the warning doesn't
flag us.
Also some changes to ensure adequate testing of webp in CI. (Even though
we only CI test 64 bit systems, so this was not related to the specific
build break being fixed... but an excuse to make sure we are hitting it
a reasonable amount in CI.)
Filesystem::IOFile constructor now upon an error opening the file will
set the error message appropriately.
Move IOProxy::error() implementation from inline to be in
filesystem.cpp to hide its details, use a static mutex to make error
set and retrieval be thread-safe (we presume that the rare chance that
multiple threads will be setting or getting IOProxy errors
simultaneously are rare enough that it's not a thread performance
issue, and if the user is getting file I/O errors, a bit of minor
locking is the least of their worries).
Better checking of these errors in OpenEXR input and output open
operations. Hopefully this means that exr file I/O errors will
percolate up more helpful error messages.
TL;DR: This could crash during openexr output, turned out to be a subtle
race condition.
Long explanation:
Metadata gets prefixed with `<formatname>:` in cases where it's only
meant to be understood by the reader or writer of one format. Formats
that support arbitrary metadata filter out any items that are prefixed
by the name of a different format (so you don't, for example, when
converting file formats, incorrectly copy a TIFF-specific metadata
hint into an OpenEXR file as literal metadata).
The specific code we used in the OpenEXR writer tried to do this but
got the threading locks wrong, and this could lead to very rare, subtle
crashes. It could have been fixed by a 3-line very local fix, but...
I also noticed that the same code was replicated in Field3D output (and
quite incorrectly -- in the cut and paste from the openexr code, it was
still looking for "openexr:" prefix instead of the correcd field3d one!).
Since this idiom was happening (wrong) in two different places, and
seemed likely to crop up again some time, instead of fixing locally I
added a new API call, `is_imageio_format_name()`, to do this centrally
and correctly. So most of this patch is implementing that and then
using it instead of the incorrect code elsewhere.
Also add python bindings and docs for the new API function
Upon reading cmake docs mre carefully, it's clear that when you pass a
version to find_package (which does much of the heavy lifting inside
checked_find_package), it DOESN'T MEAN you want that version or
later. It means you want to find a version that is "compatible" with
the named version. The meaning of "compatible" is up to the package.
Some packages define it as being >= the requested version. Others
define it as being the same major release, or same major AND minor
release, or exact.
But generally speaking, we only care about the minimum version (e.g.,
"I want FooBar 8.0 or later"). And it's not guaranteed that is what
the package's compatibility will do. In fact, FooBar may be using
compatibility mode "SameMajorVersion", which will make our find of
FooBar succeed for 8.0, 8.1, 8.2, but if a user encounters FooBar 9.0,
our build will reject it, even though it's probably fine.
So I augmented our checked_find_package wrapper to take optional
VERSION_MIN to give the real minimum version, when that's the semantic
we really want (in which case we would omit the ordinary version
number used for the compatibility check and let VERSION_MIN do all the
work). There's also an optional (and usually omitted) VERSION_MAX that
you can use if you are sure that we must be less than a certain
version (the max is exclusive, you must be <, not <=, whereas the min
is inclusive >=).
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Some permissions on GH changed due to security concerns, and that broke
the setup-nuget action we were using... which turned out to be obsolete,
anyway, and switching to the new recommended replacement fixes everything.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Inspired by OpenColorIO's beautiful docs, I'm adding the "sphinx_tabs"
extension to our docs, and testing them out in one little spot in
ImageBufAlgo to see if we like the effect.
Please look at https://openimageio.readthedocs.io/en/rtd/imagebufalgo.html
look at the first couple screenfuls to see the example of how the
tabs look. Opinions are welcome. Should we do this kind of thing more
extensively?
Signed-off-by: Larry Gritz <lg@larrygritz.com>
* Detect if OIIO is being built as a subproject, and if so, set
`OpenImageIO_IS_SUBPROJECT` (a.k.a. `${PROJECT_NAME}_IS_SUBPROJECT`).
* Move all the testing logic out of the main CMakeLists.txt (and a few
testing-related macros from oiio_macros.cmake) and into a new
src/cmake/testing.cmake.
* Move all of the packaging logic out of the main CMakeLists.txt and
into a new src/cmake/packaging.cmake.
* Make all the testing and packaging inactive if this is being built
as a subproject. Also disable the clang-format target for subprojects.
* Move fancy_add_executable and add_oiio_plugin from oiio_macros.cmake
into their own files (leaving oiio_macros.cmake empty and
eliminating it).
Signed-off-by: Larry Gritz <lg@larrygritz.com>
As OpenEXR works toward their 3.0 release, which includes splitting
Imath (the vector, matrix, math, and 'half' stuff) out into a separate
repo, their top of tree master finally breaks our "bleeding edge"
build.
This is a little tricky, because now our support of this dependency is
attempting to span 3 eras: 3.0 with the full split of openexr from
imath, recent openexr+ilmbase 2.x that supplies reliable exported
cmake configs, and older 2.x that predates cmake configs. Eventually
we'll phase out older versions and clean up.
Fixes in this patch generally include:
* Add a new CI test of VFX2021 + openexr/imath master. (This will go away
when 3.0 is released and that's what we use for the main 2021 case.)
* For new enough versions of OpenEXR, use the exported config file and
targets, rather than the old style OPENEXR_LIBRARIES style variables.
We have kind of a mixed idiom use because we're still supporting older
versions that predate use of the config files.
* Move imath includes to new OpenImageIO/Imath.h Deal with the fact
that the Imath headers now live inside `<Imath/ImathBlah.h>` rather
than `<OpenEXR/ImathBlah.h>`. But of course, you need to start
including things just to find out the version. It's tricky. The
logic is to look at `<OpenEXR/OpenEXRConfig.h>` to determine the
version we're dealing with, then include Imath stuff from one or the
other depending on what we found. I rearranged quite a few includes
to ensure that all this logic only happens in a couple spots and the
rest gets included transitively. As it turns out, quite a few of our
`#include`s of Imath headers were redundant.
* OpenImageIO-C depended on OpenImageIO publicly, but actually it
should depend on OpenImageIO privately, but export its include
needs.
* OIIO's exported config needs to look for OpenEXR.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Use __builtin_bswap when available. These seem to be 8-15x faster than
the std::swap-based implementation when multiple data values are
being swapped. (It's not dramatically better when swapping a single
value, either way it's pretty fast.)
Also add correctness unit test and benchmark for swap_endian.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
The core ImageInput and ImageOutput (and auxiliary classes they use, such as ROI, ImageSpec, DeepData) now have C99 interfaces that wrap the native C++ classes. In addition to being good for using OIIO with C, this is an important step along the way to simplifying the implementation of Rust bindings (and other languages that have a simple ffi interface to C but are tricky to wrap C++).
Still to do: ImageBuf, ImageBufAlgo, etc.
* Bump breathe release to the latest (we'e been holding back because of
errors that we fix in the other elements of this patch, below)
* Changing declaration formatting, particularly in the ImageBuf and
ImageBufAlgo chapters, eliminates function ambiguity warnings and even
python exceptions we were getting in Breathe before.
* Document missing texture option fields and improve formatting.
* Instruct Doxygen to not catalog deprecated ImageBufAlgo functions.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
We were running some CI tests only for PRs or for certain branches.
This may save some time, but I found that I was, more often than not,
naming my branches to trigger all tests to run. That's silly. Just run
all the tests every time, it's not that expensive.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
OCIO master recently renamed the symbol in its headers that we used to
detect its version, breaking OIIO's build against the current OCIO
top of tree master.
This change makes our FindOpenColorIO.cmake work with both old and new
OCIO.
Also, cut out other useless OCIO building (for our purposes) by having
our build_opencolorio.cmake disable buildig of GPU tests (which,
recently, became controlled by a different option than is used to
disable building all the other tests).
Signed-off-by: Larry Gritz <lg@larrygritz.com>
ImageBuf methods subimage(), nsubimages(), miplevel(), nmiplevels(),
and file_format_name(), which are concerned with the file to which the
ImageBuf refers, only make sense for an ImageBuf that directly refers
to a file -- either by being constructed/reset from a filename, or in
some cases, being a direct unmodified copy of such an imageBuf. All
other cases, including pixel-copies of other ImageBufs (even those
that came from files), it is merely an in-memory image and doesn't
really refer to a file.
Clarify the docs (and fix the implementation) to ensure that for
any ImageBuf that is not a simple reference to a file, these methods
return a suitale default value.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
* Accommodate changes for 0.20.2 that Homebrew now has.
* Mistake in variable naming meant we weren't testing libraw master as
we previously thought.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
There is a big bug lurking in how we read TIFF files with "separate"
planarconfig (RRRGGGBBB, versus "contig" RGBRGBRGB). I knew that
libtiff when reading scanline files doesn't support true "random
access" to scanline for most compression methods -- because you can't
seek to the start of a scanline in the middle of a compressed "strip,"
you have to read whole strips, or read individual scanlines
consecutively. But what somehow I've misunderstood for years (if not
decades) is that for "separate" planarconfig, this means that you
can't jump from scanline i in plane 0 to scanline i in plane 1! You
have to read all the scanlines (at least within one strip) in a single
plane before moving to the next plane.
This means that we get all sorts of read failures when reading TIFF
files meeting all of these conditions:
* "separate" planarconfig (these are rare in general)
* Scanline-oriented (tiles are fine, because they're read whole)
* More than one channel (with one channel, separate and contig are
basically the same)
* Certain compression codecs (turns out the separate+scanline combo
tests in the libtiff example suite are uncompressed, which is how
our testsuite failed to catch this).
* rowsperstrip > 1 (if rps == 1, every scanline is a new strip, and
that worked)
* written by an app that doesn't use OpenImageIO (because OIIO TIFF
output forced rps=1 for separate planarconfig)
Things were very broken. There is a comment in tiffoutput.cpp that says
// I can only seem to make separate planarconfig work when
// rowsperstrip is 1.
and forces rps=1 for separate planarconfig output. So at some point
many years ago I realized that something odd was happening here, but I
apparently I thought it was a bug in libtiff when writing with rps > 1.
Probably we were writing correctly all along, and the real bug was in
the reading. This also explains why we had mostly not seen this
problem before, which is because all OIIO-written files avoided
producing tiff files that fell into the failures case.
This patch mostly fixes it and also makes a number of performance
enhancements:
* Fix misunderstanding in read_scanlines where it was loose with thread
safety in retrieving the rows per strip.
* In read_scanlines when deciding the "chunk size" to limit temp
buffer size, and in read_image when deciding how many scanlines to
read at a time via read_scanlines, these computations weren't the
same, leaving slivers to be read alone instead of staying aligned
with strips. Make those both compute the same way, so from read_image
on down, we end up reading chunks that are always aligned with
strips.
* TIFF read_scanlines used to try to read whole strips, except for
reading individual scanlines that didn't comprise a whole strip
length. Modify it to recognize that the one exception is the last
strip in the file, in which the strip is shorter than usual if the
total number of scanlines is not an even multiple of rps. This
ensures that the last strip is read as a strip, not as individual
scanlines.
* Imagecache autotile behavior is modified to force the pseudo-tile
size to be a multiple of the strip length. Again, that keeps the
resulting reads to always be in units of strips, which is much more
efficient and is the safe way to seek and read separate
planarconfig. Also, always turn on autotile for any tiff file with
rowsperstrip > 1, because it's already organized that way in the
file. This is not only more efficient generally, but also makes it
so that ImageCache will only do reads aligned safely to strip
boundaries.
This is not a complete solution, in the sense that an app making
individual read_scanline calls for solo scanlines, or read_scanlines()
that is not aligned to strips, can still have read failures for TIFF
files that are just the right combination of separate planarconfig +
certain compressions + scanline order.
However, with this patch, the following common cases will ALWAYS work
correctly now: (a) read_image or read_scanlines of the whole image
size; (b) any reads mediated by ImageCache or TextureSystem; (c) all
images read by ImageBuf (which always use either (a) or (b)); (d) all
operations of oiiotool (which use ImageBuf).
After this patch is merged, solving the common cases, I'll return to
the topic to tackle the remaining issue of broken reads for these
files when doing individual ImageInput::read_scanline reads or
read_scanlines with ranges not aligned to strips. To make those work,
I'll need some kind of ugly internal strip buffering.
I'm going to leave the "force rps = 1 when writing planar separate tiff"
as it is, because even after this set of fixes, software using old
versions of OIIO will not be able to correctly read rps>1 planar files,
so we wouldn't want to be inadvertently writing them, at least not
until all the pre-fix OIIO releases are unlikely to be used by
anyone. Not writing planar files with rps>1 is not much of a
sacrifice, considering how rarely planar files are used at all.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Now that we use Linux and the ASWF docker containers for the
"clang-format" test, there's no reason to have logic in this setup
script for Mac to choose a more restrictive set of package for that
case.
Same for clang-tidy -- we don't have a test like that anyway, and
if we add one, we will surely do it with one of the Linux containers.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
There was no formal error setting! Errors were echoed to the optional
output stream, if supplied. But that's not good enough.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Although we were careful to make sure that the new versions of
ImageInput `read_scanlines`/`read_tiles`/`read_image` methods were
thread-safe when called concurrently, one thing that slipped through
the cracks was error reporting. A failed read (or other) call would
return an error flag and use error() to set the error message, which
could be retrieved via geterror()/has_error().
Except the flaw was that the error itself was a single std::string in
the class. It was guarded by a mutex, so at least the threads wouldn't
clobber each other over access to the string. But it means that the
following call sequence would do the wrong thing:
thread 1: read_tile A --> fails
thread 2: read_tile B --> fails
thread 1: geterror() --> gets both errors appended, oops!
thread 2: geterror() --> gets an empty error message, oops!
Why hadn't we noticed until now? Because read failures of individual
scanlines or tiles are rare -- usually the failure or discovery of a
missing or corrupt file is in open(). So I guess it's not often that
two concurrent reads would both have just the right timing to have the
error retrieval problem described above.
There is no easy fix, so a bit of a bigger, ABI-breaking, change was
necessary to make this work properly once and for all:
* ImageInput and ImageOutput error messages are now thread-specific. That
is, geterror() retrieves errors generated by method calls made by the
same thread.
* ImageInput and ImgageOutput now use a PIMPL idiom to store some data
behind an opaque pointer. This is good -- it means that the details
are hidden from the public API, and can use classes we don't wish to
expose (which we will do here with boost::thread_specific_pointer) as
well as allow us to change what's behind the pointer without breaking
the ABI.
* The error message uses a thread_specific_pointer.
* We therefore removed the m_errormessage member that was unfortunately
in the public class. Also removed the mutex and the m_threads, which
can now be safely tucked behind the PIMPL.
* With the mutex hidden from view, we now re-jigger references to it
to use the lock/unlock methods we had already. In fact, instead of
using lock_guard<mutex>, we can use lock_guard<ImageInput> because
the presence of lock/unlock methods make ImageInput (and ImageOutput)
themselves compliant with the BasicLockable concept.
Note that this changes the data layout of ImageInput and ImageOutput,
and therefore is an ABI break. So this is a change meant for master
only (future OIIO 2.3+), and will not be backported to the 2.2 release
branch.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
* WebP input overhaul (including animated webp and metadata)
Huge overhaul of webp reading. Since webp support was first added in
2011 (with no major changes since, other than a couple minor bug
fixes), we have been using the basic/simplified webp API, which had
many limitations. This patch rewrites it to use the more complete
API. This results in the following changes and new capabilities:
* RGB images are kept as 3-channel instead of forced to be RGBA upon
input.
* Skip alpha premultiplication for RGB files. And for RGBA, skip the
dubious color space transformation when doing premultiplication of
RGBA files. I think this has been wrong all along.
* We can now read animated webp images (akin to animated GIFs, they look
like multi-subimage files).
* Much more efficient to read just the header, the pixels are read and
decoded lazily.
* Switch to the WebPDecode*Into API calls, which lets us preallocate
and manage the decoded image buffer (as a unique_ptr). Also switch
the encoded buffer from a std::vector to std::unique_ptr, which
avoids an unnecessary zeroing out of the buffer.
* Close the file handle immediately after we're done reading, rather
than leaving the handle open until the client is done with the
ImageInput.
* Read Exif metadata properly from webp files.
* To use the more sophisticated API, we need to find and use the
libwebpdemux library.
* Since there is a "LoopCount" metadata here, similarly to with GIF, I
decided to make an OIIO metadata "oiio:LoopCount" that will work
with both. GIF saves with this, too, and "gif:LoopCount" is now
considered a deprecated synonym.
Fixes#2721
N.B. This only covers webp input, not output. A similar overhaul should be
done eventually on the output side.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Cuda 10.1 seems to have trouble because of the `__attribute__` used in
`OPENIMAGEIO_PRINTF_ARGS`. Declan Russell suggests modifying the `#if`
to avoid using it for `__CUDACC__`.
LG further observes that `OPENIMAGEIO_PRINTF_ARGS` is only used for
the declaration of two functions, it seems like a nice decluttering to
just directly use the short attribute exactly where it is needed
instead of having a lengthy explanation and possibly defining
`__attribute__` everywhere as a side effect.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Co-authored-by: Declan Russell <declan@solidangle.com>
For IC::add_tile() and IC::get_tile(), the documentation advertised that
passing chend < chbegin meant "all channels". But the implementation was
setting chend=nchannels but not setting chbegin to 0.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Different classes and modules had minor inconsistencies on how errors
were reported and retrieved. Let's fix these up.
* geterror() should always take a `clear` parameter that allows the
caller to control whether or not retrieving the error clears the
internal error message (default is true, clear it after retrieving).
* Any class with a geterror() should also have a has_error() that merely
reports whether there is an error message pending retrieval.
* geterror() will always *append* to an existing error message that
has not yet been retrieved, with a newline separating error
messages. Previously, some of our geterror implementations appended
and others replaced.
* geterror() will trim a single trailing newline from the message it
is passed, thus storing error messages that are not
newline-terminated and making it so that callers of geterror() don't
need to wonder whether or not they should terminate their error
messages with a newline or what happens if you make the wrong
choice.
* Usage guidance is therefore that individual errorf/errorfmt() calls
not include a trailing newline (though if one is present, it will
be harmlessly stripped), and that whoever retrieves with geterror()
should, after outputting it, add a newline (which they seem to
invariably do already).
Signed-off-by: Larry Gritz <lg@larrygritz.com>
One of our CI matrix entries -- building with clang -- has
mysteriously broken with compile errors complaining about reallocarray
being inconsistently declared in stdlib.h and gif_lib.h. It just broke
a few days ago, unrelated to any changes on our part. I have no idea
if it was the compiler or gif library that got updated and then broke.
Whatever. Insulate ourselves from random changes on GHCI by converting
this test to use one of the ASWF containers that we can count on to
have stable dependencies installed.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
* Bring back the idea of a Findpugixml.cmake file because it turns out
that (for pugixml <= 1.10) the exported config files are not reliable.
* Fix CI scripts to build PugiXML (with a few different versions). We
previously were relying on the embedded version for all CI tests, which
masked problems associated with using an external PugiXML.
* Fix deprecation warnings about xml_document.load(), the new call is
xml_document::load_string().
Signed-off-by: Larry Gritz <lg@larrygritz.com>
It was setting it, but later, too late for the particular case where
constant_color_detect was used and found a constant color and also the
config spec came in without any tile preference (still 0's). So move
the default initialization up to the top.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
This is good for optimization, allowing "de-virtualization" of function
calls in cases when the compiler can be sure that there can be no further
subclassing.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
This used to work, was inadvertently broken in #2398, in Nov 2019, and
no release of OIIO 2.1 ever had it working! (I guess people don't
come across 16 bit iff files very often?)
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Boost date_time, though never explicitly called in OIIO code, can be transitively pulled in by boost threads. This seems to be a problem on Windows when you build a partial boost containing only the libraries you think you need.
The solution seems to be to set BOOST_ALL_NO_LIB on MSVC, even for the dynamic linking case.
What data format does oiiotool write for its outputs? The ones
specified by -d, if supplied, but otherwise it will make a guess based
on the types of inputs it has seen.
This patch makes several improvements:
* Multi-subimage files were getting very confused because different
subimages may want different data formats, and it was at best
remembering data formats overall and by channel name, but not taking
subimages into consideration. Make this much more specific by
remembering both raw channel names (for all subimages), as well as
specific channels of named subimages.
* The way the input data formats were remembered had repeated (and
non-matching) code depending on whether the input was read
immediately or lazily. That is now cleared up, with the logic
refactored and expressed only once.
* The -d directive can specify a type (for all channels), a type for a
particular channel name (`channel=type`), and -- this part is new --
a type for all channels of a named subimage (specified as
`subimage.*=type`) or just one channel of a specific subimage
(`subimage.channel=type`).
Signed-off-by: Larry Gritz <lg@larrygritz.com>
This can cause cache re-reads unnecessarily because it thinks you're
asking for potentially different configuration hints every time you
set the ImageBuf to a different subimage.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
A bug (issue 2713) revealed that an alloca in IBA::colorconvert could
cause a stack overflow on an image that had an unusually wide number of
columns per scanline.
This patch takes a number of measures to help root out these problems
in the future:
* The OIIO_ALLOCA macro is amended to assert (in DEBUG builds only)
upon any attempt to alloca more than 1MB, which is surely a sign that
we have used alloca in an inappropriate place.
* A new OIIO_ALLOCATE_STACK_OR_HEAP macro is used to encapsulate an
idiom I have used before, to alloca small things but new[] big things,
and ensure in both cases that the memory is freed when the function
exists scope. I choose arbitrarily for "big thing" to mean <= 64 KB,
which is big enough for one 64x64 tile or one scanline of a 4k image
if it has 4 float channels, that means most of the time we'll safely
allocate on the stack, but for anything larger, it will use heap.
* Use OIIO_ALLOCATE_STACK_OR_HEAP in those problematic spots of
colorconvert.
* Use OIIO_ALLOCATE_STACK_OR_HEAP in a few other places that seem
prudent.
* Grepped through the code for all instances of OIIO_ALLOCA and I think
that all the potentiallly "big" ones are now using the safe idiom.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
For "mip-mapped" TIFF files where we use the multiple images in the file
to emulate MIP-maps rather than what we ordinarily consider subimages,
a call to `ImageInput::spec(subimage,miplevel)` was not properly
recognizing invalid subimage values and returning an empty ImageSpec
as advertised. Basically, we assumed that they would only ask for a
valid subimage and miplevel and didn't properly handle out of range
values.
Same problem for spec_dimensions().
Signed-off-by: Larry Gritz <lg@larrygritz.com>
For both, allow an optional modifier ":n=" to specify the number of
images from the stack to be combined by having their channels or
subimages appended (versus the default of combining just the top 2
images).
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Fix cause of a subtle bug: splitting an empty string returned one
string piece (also empty) instead of 0 pieces.
Beef up the Strutil unit tests in this area.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
This was subtle. Empty subimage include/exclude specifications would
confuse compute_subimages(), which in turn would MUCH later do a
mis-allocation and lead to a situation in --resize where it would
iterate over subimages that had not need allocated.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Switch from old cmake macros that were marked obsolete in cmake 3.12 to
the new FindPython.cmake module.
Instead of defaulting to python 2.7, default to whatever is found.
But user can still set PYTHON_VERSION variable to request a specific
one in case there are multiple.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
ImageBuf uses ImageCache underneath. But this can lead to the following
unexpected behavior if you don't understand this:
1. Make an ImageBuf for an image, read it (even just header info).
2. Done with it, destroy the ImageBuf.
3. Some other part of the program, or even a different process, changes
that file on disk.
4. Make another ImageBuf with the same filename, thinking you're getting
the updated image, but in fact it's relying on the cached representation
which doesn't contain the changes made in step 3.
So these tiny changes try to make this much harder to shoot yourself in the
foot:
* New ImageBufs automatically call cache->invalidate on the filename
before they start reading, to ensure that if the file has changed on
disk, anything in the cache is invalidated.
* When you destroy or clear an ImageBuf, it calls cache->close() on the
file, to release the handle (especially helpful on Windows, where an
open file handle can lock the file to other processes).
There is a chance that some image operations will have degraded
performance, if you are mixing direct ImageCache use (including
TextureSystem) and ImageBuf reads of the same files -- since creating
and destroying the IB's will flush tiles from the cache and close cached
file handles. But I think that this should not be a problem in practice,
since most apps that are heavy users of ImageBuf (like oiiotool) aren't
heavey users of IC/TS, and vice versa. And eliminating this confusing
behavior is probably more important than occasional over-eager
invalidation of the cache.
Also clarify in the docs (and imagebuf_test) that if you use the
variety of ImageBuf ctr where you give it a custom ImageCache, it's
important that the ImageBuf outlives the ImageCache, as it is a
non-owning reference.
Fixes 1987
Signed-off-by: Larry Gritz <lg@larrygritz.com>
This new command takes two images as arguments, and appends all the
metadata (only) from the first image onto the second image's pixels
and metadata, producing a combined image.
This can be useful for users who have some (either non-OIIO-based or
just badly implemented) application that operates on image files but
fails to propagate the metadata, by having a simple way to transfer
metadata from one file to another:
bad-app in.exr -o out.exr
# oops, out.exr no longer has the crucial metadata from in.exr
oiiotool in.exr out.exr --pastemeta -o out2.exr
# Now out2.exr has the pixels of out.exr, but with all metadata
# from in.exr added to or overwriting that found in out.exr.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
We have many classes that have methods
.attribute(name, intval)
.attribute(name, floatval)
.attribute(name, stringval)
.attribute(name, TypeDesc, anyval)
For ImageSpec.attribute() and OIIO.attribute(), the anyval was
restricted to only accept tuples. Fix these to bring them in line with
many other classes' attribute calls in which the 'anyval' really can
be just about anything -- tuples, lists, numpy arrays or other buffer
objects, or scalar values.
Fixes#2674
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Seems that cmake's FindGIF.cmake isn't good at discerning minor
releases (prior to GIF 5.x including more info in its headers), so
saying "find_package(GIF 4.1)" doesn't work the way we'd like, and
seems to fail to find a legit 4.1 installation. Go back to
find_package(GIF 4).
We seem to have broken this in PR 2681, and I'm worried that people
with gif_lib 4.1 will find themselves mysteriously lacking GIF support
in their OIIO builds. I may do an emergency tweak release of 2.2 just
to address this.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
When transferring blocks of pixels (for example, by ImageInput.read_image
or ImageOutput.write_image), "half" pixels ended up mangled into uint16,
the only type of the right bit width.
But NumPy has a numpy.float16 type! A little bit of extra hinting to
pybind11 makes this work as users probably expect.
Fixes#2689
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Don't copy from the data pointer if it's null, that indicates an error.
Related to issue 2688, but doesn't fully solve it.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
For a few rare edge cases of photometric interpretation, we fall back
on libtiff's ability to auto-translate to an RGBA image for us. It
only does this for the whole image at once, so we do it for the first
scanline we read (storing the whole image in a buffer, skipping the
conversion for subsequent scanlines -- and it can tell it's the
"first" scanline because the buffer isn't yet allocated).
The bug was that for these cases (such as a photometric YCbCr), if the
file was *multi-subimage*, we were not clearing the rgba buffer, so
upon reading the second subimage, it thought its buffer was already
filled, so it wasn't actually doing the read and convert, just using
whatever was already living in the buffer. Oops.
Also, I noticed that we should clear m_use_rgba_interface for each
subimage -- it was previously just setting it when it found those edge
cases. We never saw a symptom from this, but I realized it would be
problematic for a file where the first subimage was YCbCr and the
second subimage was regular RGB, it wouldn't know to fall back to our
usual code path.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
It's been endless trouble, because the nature of the test is malformed
images that fail in inconsistent ways. I can never seem to make a
stable set of reference output for it, especially for OpenEXR
development master, which is always adding new fixes or tinkering with
exceptions and error messages.
So just don't run that test in the automated CI.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Plain old bug in all cases:
--croptofull botched the subimage count by reserving space for results
based on the subimages in the input image, but then only producing
results for the first one if -a (or :allsubimages=1) was not used.
Output specific bugs:
There are some operations that happen ephemerally upon output, mostly
to account for output file formats that don't support some feature of
the top image (like action_croptofull if the output format doesn't
support differing display and data windows, or action_channels if
there are more channels than are supported by the format, or
action_colorconvert when --autocc is used).
In these cases, we were forgetting that those operations may tend to
drop all but the first subimages, if no -a or :allsubimages=1 is not
used. But that's NOT the behavior we want in this case, because for
output specifically, we never want to drop subimages. This is because
we want a simple format conversion like
oiiotool in.xyz -o out.pdq
to faithfully copy, not "narrow" to one subimage, even if some other
operations on their own will tend to drop all but the first subimage if
not instructed to preserve them all.
So the solution is that we need to add ":allsubimages=1" modifier to
these ephemerally called adjustments.
Periodic compare of analogous build scripts in OIIO & OSL and synchronize
to ensure that my current most favored idioms are in both.
The biggest change is that I split the checked_find_package into a separate
cmake file, for easier copying between projects.
Also fix some minor typos.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
HEIF is a lossy format, and it seems libheif 1.8 changed internally
in such a way that the bit-for-bit comparsions of the write-read
cycle we do in imageinout_test started failing. (Bummer on our Mac
CI, where the homebrew version of libheif updated automatically and
then started failing.)
The easy solution is that just for this test, write zeros to the image,
not 1.0 values (which were coming out 254, not the expected 255).
Signed-off-by: Larry Gritz <lg@larrygritz.com>
In the case of a dependency not being found, we look for
src/build-scripts/build_{package}.bash and if it exists, suggest using
it. But the scripts were all lower case, and the packages were not, so
it was often not giving the hint! Also, in some cases, the package
might be named 'TIFF' but the script was 'build_libtiff.bash'.
So now, look for a build script with the package name straight, and
also all-upper and all-lower-case versions, and "lib<lower>".
Rename build_ocio.bash to build_opencolorio.bash, because it's the one
case where the script didn't match any of those obvious
transformations.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
This new file contains the hashes of commits whose only function was
to reformat large amounts of code.
Ordinarily, this would make for confusing 'git blame' output, by
making old code written by author A appear to have a new modification
date and authorship by B, when in fact we know that B just did some
reformatting. This can make it especially hard to discern code history
after after commits that establish the use of "clang-format".
A new git feature (requiring git >= 2.23) allows pointing to a file
containing hashes of commits to be ignored by git blame:
git blame --ignore-revs-file .git-blame-ignore-revs
To avoid having to remember this all the time, consider adding this to
your git config:
git config blame.ignoreRevsFile .git-blame-ignore-revs
Note that git supports this feature but does not dictate the name of
the file. I'm just following the convention I'm seeing pop up in other
projects of calling it ".git-blame-ignore-revs".
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Sometimes the true minimum version of a dependency that we are able to
build against is lower than the minimum *recommended* version (perhaps
because some important feature was added in a subsequent release that
we would like to expose if available).
This patch allows checked_find_package to take two new optional
arguments: RECOMMEND_MIN is the minimum *recommended* version (but not
the true rejection minimum) and RECOMMEND_MIN_REASON is some text to
add to the error message if there is a succinct explanation for why we
recommend a higher version than the true minimum.
As an example, we will (currently) build against any OpenEXR 2.x. But
if it's not at least 2.2, we can't support DWAA and DWAB compression.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
These were so out of date and unhelpful, it has to be better to
have no instructions at all. Tear it down and make room for somebody
to do it right.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Oops, ParamValue.type ought to be a TypeDesc, just like in C++. In
the Python binding overhaul of 2.0, it seems that I made a mistake and
unintentionally changed it to appear to Python as a string (e.g.,
'int' rather than TypeDesc('int')).
Fixing it to be the way it was originally intended would involve a
breaking change and not appropriate until such things are allowed by a
2.x -> 3.0 transition, so we'll just have to live with it.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
The thing about IOProxy is, once closed, it can't be reopened. So it's
important that if an IOProxy-based file is in the IC, its ImageInput
should not be reclaimed by closing it (and that might not help the
file handle count anyway, since the main use of an IOProxy under those
circumstances is probably to read an image from memory).
Signed-off-by: Larry Gritz <lg@larrygritz.com>
It's important not to mess with a passed-in IOProxy* in Input::close(),
because that proxy may not still exist.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
* Add support for reading DPX files from any IOProxy
* Replace memcpy_s function with memcpy
* Fix IOProxy bug (not just for DPX) seeking from end
Signed-off-by: ra-mt <radu.arjocu@masstech.com>
I just like this better than the prior habit of putting it one PAST
the column width, which made the backslashes go in column 81, ick.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Last week, Vcpkg apparently had some flaw that recently caused it to
fail when building/installing libpng (among other things). It's been
fixed in the vcpkg main branch, but that hasn't propagated to the
vcpkg that's installed on the GitHub Actions CI runners, and that's
causing our Windows CI tests to fail.
Rather than wait out the fix, I'm just taking it into my own hands by
building all our dependencies from source on Windows. This means adding
bash scripts to download and build zlib, libpng, and libjpeg-turbo (and
maybe those will be helpful to others as well).
**Late breaking news** They beat me to it, vcpkg has been updated and
the GitHub CI Windows tests are passing again.
But I had already done this work! So I still would like to switch to
this methodology because:
* Sometimes it's faster to build ourselves rather than have vcpkg
build, since vcpkg always builds both release and debug, but we only
need the release builds of the dependencies.
* This makes it easier to customize the specific version of a
dependency, or particular build options, which are hard or
impossible to change for a vcpkg build.
* These same dependency-building scripts could be useful for people who
are not on Windows or not interested in using vcpkg.
Also some minor changes to the build_libtiff and build_openexr scripts
to synchronize some of the idioms I use in these scripts.
Rename workflow.yml to ci.yml to adhere to convention and also make it
so we can have multiple workflows in the future.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Fixes a compilation error, that appears on alpine linux.
/builds/lmarz/aports/testing/openimageio/src/oiio-Release-2.1.18.0/src/libOpenImageIO/exif.cpp: In function 'void OpenImageIO_v2_1::read_exif_tag(OpenImageIO_v2_1::ImageSpec&, const TIFFDirEntry*, OpenImageIO_v2_1::cspan<unsigned char>, bool, int, std::set<unsigned int>&, const OpenImageIO_v2_1::pvt::TagMap&)':
/builds/lmarz/aports/testing/openimageio/src/oiio-Release-2.1.18.0/src/libOpenImageIO/exif.cpp:789:20: error: comparison of integer expressions of different signedness: 'unsigned int' and 'OpenImageIO_v2_1::span<const unsigned char, -1>::index_type' {aka 'int'} [-Werror=sign-compare]
789 | if (offset >= buf.size()) {
|
When the texture cache reads scanlines in random order, the same pixels could
be read many times over. Now the file offset for scanlines is cached for quick
random access.
* OpenImageIO/imagebufalgo.h: remove OIIO_API from enums
* OpenImageIO/strutil.h: remove OIIO_API from inlined lower and upper functions
* libutil/plugin.cpp: cast return value of GetProcAddress to void*
* libutil/sysutil.cpp: define _POSIX_C_SOURCE for localtime_r declaration
* libutil/thread.cpp: cast t->native_handle() to HANDLE type
* ptex.imageio/ptexinput.cpp: use perr.c_str() for %s string format
* Fix some incorrect error calls in ImageBuf internals.
* Simplify because we know that ImageBuf::error() and warning(), with
just one string argument, doesn't need a format command.
* Make sure that if no message bubbles up, we say "unknown error".
Signed-off-by: Larry Gritz <lg@larrygritz.com>
We weren't setting up the proper handler callbacks for libpng internal
errors to bubble up to the caller.
Tell libpng to turn off sRGB color profile check. There are many
false positives where embedded ICC profiles that don't match its
expectations (but that other people and apps consider fine) trigger
errors in libpng.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
# "Vendor" is only used in copyright notices, so we use the same thing that
# the rest of the copyright notices say.
set(CPACK_PACKAGE_VENDOR${PROJECT_AUTHORS})
set(CPACK_PACKAGE_DESCRIPTION_SUMMARY"OpenImageIO is an open source library for reading and writing image file formats, a nice format-agnostic image viewer, and other image-related classes and utilities.")
* For a full list of supported build features: `.\vcpkg search openimageio`
*`vcpkg install openimageio [tools]`
* 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`
@@ -139,7 +162,7 @@ file format (jpeg, fits, png, etc.). This works both as a CMake variable and
also as an environment variable.
`ENABLE_PkgName=0` : Disables use of an *optional* dependency (such as
FFmpeg, Field3D, Webp, etc.) -- even if the dependency is found on the
FFmpeg, OpenVDB, Webp, etc.) -- even if the dependency is found on the
system. This will obviously disable any functionality that requires the
dependency. This works both as a CMake variable and
also as an environment variable.
@@ -215,7 +238,7 @@ Additionally, a few helpful modifiers alter some build-time options:
| make USE_PYTHON=0 ... | Don't build the Python binding |
| make BUILD_SHARED_LIBS=0 | Build static library instead of shared |
| make LINKSTATIC=1 ... | Link with static external libraries when possible |
| make SOVERSION=nn ... | Include the specifed major version number in the shared object metadata |
| make SOVERSION=nn ... | Include the specified major version number in the shared object metadata |
| make NAMESPACE=name | Wrap everything in another namespace |
The command 'make help' will list all possible options.
@@ -243,91 +266,94 @@ or, if you are using CMake directly,
Building on Windows
-------------------
**Method 1 - Default**
**Method 1 - from source**
1. Check out the trunk or a branch of your choice. The remainder of
these instructions assume that you checked out the trunk to the
D:\OIIO\trunk directory.
You will need to have Git, CMake and Visual Studio installed.
2. Download the package with precompiled external libraries from
http://www.openimageio.org/external.zip
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.
Next, unpack it. The directory with downloaded code from the repository
and the directory with unpacked libraries should be siblings. For example,
* 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.3.x | :white_check_mark: All fixes that can be backported without breaking ABI compatibility. New tagged releases monthly. |
| 2.2.x | :x: Only receives occasional critical fixes, upon request. |
| <= 2.1.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 administrator
has 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
message(STATUS"${ColorYellow}WARNING When building for C++17, LibRaw should be 0.20 or higher (found ${LibRaw_VERSION}). You may get errors, depending on the compiler.${ColorReset}")
# Currently, we issue the above warning and let them take their chances.
# If we wish to disable the LibRaw<0.20/C++17 combination that may fail,
# just uncomment the following two lines.
# set (LibRaw_FOUND 0)
# set (LIBRAW_FOUND 0)
endif()
checked_find_package(OpenJPEGVERSION_MIN2.0
RECOMMEND_MIN2.2
RECOMMEND_MIN_REASON"for multithreading support")
# Note: Recent OpenJPEG versions have exported cmake configs, but we don't
# find them reliable at all, so we stick to our FindOpenJPEG.cmake module.
checked_find_package(OpenVDB
VERSION_MIN5.0
DEPSTBB
DEFINITIONS-DUSE_OPENVDB=1)
checked_find_package(PtexPREFER_CONFIG)
if(NOTPtex_FOUNDORNOTPtex_VERSION)
# Fallback for inadequate Ptex exported configs. This will eventually
# disappear when we can 100% trust Ptex's exports.
set(ILMBASE_LIBRARIES${OPENEXR_IMATH_LIBRARY}${OPENEXR_IEX_LIBRARY}${OPENEXR_HALF_LIBRARY}${OPENEXR_ILMTHREAD_LIBRARY}${ILMBASE_PTHREADS}CACHESTRING"The libraries needed to use IlmBase")
set(OPENEXR_LIBRARIES${OPENEXR_ILMIMF_LIBRARY}${ILMBASE_LIBRARIES}${ZLIB_LIBRARIES}CACHESTRING"The libraries needed to use OpenEXR")
set(FINDOPENEXR_FALLBACKTRUE)
endif()
mark_as_advanced(
@@ -153,3 +264,5 @@ mark_as_advanced(
# Restore the original CMAKE_FIND_LIBRARY_SUFFIXES
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