Review: Field3D support

The OIIO APIs have always had support for "3d" (volumetric) images, though it was largely untested because we didn't actually have any format readers for volumetric files.

With this change, we add support for Field3D volume files (http://code.google.com/p/field3d/) and 3D texturing thereof.  It should continue to build just fine if it can't find the Field3D libraries, you just won't get support for that format.  

The field3d plugin itself is fairly straightforward.  The remainder is mostly fairly straightforward minor extensions to allow ImageCache and ImageBuf to hold volumetric tiles and images.  I had to modify the TextureSystem::texture3d() API call to account for the fact that I'd forgotten a dPdz before, and also some things in TextureOptions were renamed to reflect that the 3rd texture coordinate should have been named r, not z.  (Following from OpenGL's lead here.)  I modified 'testtex' to sample a slice through the middle of a volume, as a quick way to test 3d lookups without a volume renderer.  Since presumably nobody was using the non-functional 3D routines before, none of this should break compatibility with any of the existing 2D functionality in your apps.

OSL folks: the review for the corresponding OSL changes (yes, including a built-in texture3d() function) will be posted soon, hopefully today but certainly no later than Monday.  (Arnold developers: I'll also post the internal Arnold-side changes shortly.)

A few caveats to be aware of:

* The texture3d routines currently only sample/interpolate, they don't filter (which would be expensive since Field3D doesn't support multiresolution fields at present, and probably tricky to correctly filter anisotropically in 3D).  Also, I only trilinearly interpolate at the moment, but in a few days I'll fix it to do cubic lookups for nicer interpolation.

* It's possible that this will undergo some minor tweaking down the road, as production starts to play with it.  I promise we'll have it fully nailed down before we bless an OIIO 0.9 release branch.  (The break in APIs will NOT be back-ported to 0.8.)

* SPI people: I still don't have it building correctly with all the internal namespaced libraries.  I'll have that fixed by the time this is all reviewed.  Shouldn't affect anybody else or standalone use.

* Magnus tells me that in the next few days, he'll be pushing some minor Field3D library API changes to the public Field3D release.  I'll post an updated version of this patch when that happens.  It'll be fairly minor.

[lg_imageworks.com, 2010-10-30 00:54:28]

I should add that this first review only contains a reader for Field3D, I
haven't written the writer yet.

	-- lg


On Oct 29, 2010, at 4:36 PM, <larrygritz@gmail.com> <larrygritz@gmail.com>
wrote:

> 
> With this change, we add support for Field3D volume files
> (http://code.google.com/p/field3d/) and 3D texturing thereof.  It should
> continue to build just fine if it can't find the Field3D libraries, you
> just won't get support for that format.

--
Larry Gritz
lg@imageworks.com

[ckulla, 2010-11-02 17:39:38]

Looks pretty straightforward overall. I'm just a bit confused by the use of
max(depth,1). Does any code actually rely on depth == 0 to indicate a 2D image?

http://codereview.appspot.com/2798042/diff/1/src/include/imagebuf.h
File src/include/imagebuf.h (right):

http://codereview.appspot.com/2798042/diff/1/src/include/imagebuf.h#newcode326
src/include/imagebuf.h:326: int zmax () const { return spec().z +
std::max(spec().depth,1) - 1; }

Why is a max needed here, but not on width and height?

http://codereview.appspot.com/2798042/diff/1/src/libOpenImageIO/imagebuf.cpp
File src/libOpenImageIO/imagebuf.cpp (right):

http://codereview.appspot.com/2798042/diff/1/src/libOpenImageIO/imagebuf.cpp#...
src/libOpenImageIO/imagebuf.cpp:697: p += z * clamped_mult64
(m_spec.scanline_bytes(), (imagesize_t)spec().height);
Why use a clamped mult here? This method looks like it only works if the entire
image is in memory.

[larrygritz, 2010-11-02 21:40:06]

On 2010/11/02 17:39:38, ckulla wrote:
> Looks pretty straightforward overall. I'm just a bit confused by the use of
> max(depth,1). Does any code actually rely on depth == 0 to indicate a 2D
image?
...
> Why is a max needed here, but not on width and height?

Because width & height are always >= 0 for a valid image.  At some point in the
past, depth may have been 0 for 2D images.  I don't think this is still the
case.  I'm trying to sprinkle with DASSERT(depth>=1) and gradually eliminate the
max.


>
http://codereview.appspot.com/2798042/diff/1/src/libOpenImageIO/imagebuf.cpp#...
> src/libOpenImageIO/imagebuf.cpp:697: p += z * clamped_mult64
> (m_spec.scanline_bytes(), (imagesize_t)spec().height);
> Why use a clamped mult here? This method looks like it only works if the
entire
> image is in memory.

I was trying to replicate the math we use elsewhere for such things, which is
supposed to be safer if somebody compiles on 32 bit systems.

[larrygritz, 2010-11-02 21:41:13]

New patch -- actually include the field3d files; fixes for SPI Linux compiles

[ckulla, 2010-11-02 21:46:45]

> Because width & height are always >= 0 for a valid image.  At some point in
the
> past, depth may have been 0 for 2D images.  I don't think this is still the
> case.  I'm trying to sprinkle with DASSERT(depth>=1) and gradually eliminate
the
> max.

You may want to do another pass through the review. In most cases there's a max
and no DASSERT. In most cases I can think of, using 0 for depth would either
produce an obviously wrong result (that we would catch right away), or be
equivalent to the "right" math. So I would go ahead and keep the code simple and
get rid of the max's.


> 
> 
> >
>
http://codereview.appspot.com/2798042/diff/1/src/libOpenImageIO/imagebuf.cpp#...
> > src/libOpenImageIO/imagebuf.cpp:697: p += z * clamped_mult64
> > (m_spec.scanline_bytes(), (imagesize_t)spec().height);
> > Why use a clamped mult here? This method looks like it only works if the
> entire
> > image is in memory.
> 
> I was trying to replicate the math we use elsewhere for such things, which is
> supposed to be safer if somebody compiles on 32 bit systems.

Using a 64 bit multiply is fine if we want to support images larger than 4Gb,
but the clamped logic here isn't protecting you from much. If the 64 bit
multiplication overflows, that would imply the image is larger than the 64 bit
address space in which case you probably shouldn't be using this function.

[larrygritz, 2010-11-02 21:58:10]

On 2010/11/02 21:46:45, ckulla wrote:
> 
> You may want to do another pass through the review. In most cases there's a
max
> and no DASSERT.

I'm aware of that.  I'm hitting them as I see them.  I'll make a more concerted
effort after I check this in; I didn't want to subtly break anything for an
"unrelated" change.


> Using a 64 bit multiply is fine if we want to support images larger than 4Gb,
> but the clamped logic here isn't protecting you from much. If the 64 bit
> multiplication overflows, that would imply the image is larger than the 64 bit
> address space in which case you probably shouldn't be using this function.

I believe you're right.  The clamped math is helpful when computing pixel counts
and such, but once you're in code that's stepping through memory assuming the
whole thing is loaded, it's probably past the point of no return.  I'll address
this in the next round, though, if you don't mind.

[ckulla, 2010-11-02 21:58:49]

LGTM

[larrygritz, 2010-11-02 22:03:01]

On 2010/11/02 21:58:49, ckulla wrote:
> LGTM

Actually, I will change that multiplication line, to:

        p += z * m_spec.scanline_bytes() * spec().height;

I'll fix the depth max business in subsequent reviews, as I come across them.