Review: MIPmap vs subimage, take 2

More fleshing out of the API changes that will tease apart subimages versus MIPmap levels.  This more or less implements the API changes (including those necessary to ImageCache, TextureSystem, and ImageBuf), the required changes to the format plugins, some of the utility programs.  Tests almost all pass, there are a few things left to mop up.

It's not done yet, but I thought it was becoming big enough to review in stages.  That's also why I'm doing it via codereview.appspot.com -- so that I can upload subsequent takes and you can just diff against earlier patches without having to wonder what has changed.

Still to do: ImageCache/TextureSystem has lots of FIXME notes, especially regarding how to figure out and report lack of tiling or mipmapping (example: what do you do with a multi-image texture that has some faces mipmapped but not others?); docs; iv is still confused about subimages versus mip levels; I don't think idiff and iconvert are as robust as we want quite yet.

But I think I'm 80% of the way there.

Note that this will be a non-back-compatible change, so please think about what this will mean for your apps and get your feedback in before anything is committed.  In any case, this will be a 0.9 feature and will never change 0.8 or other release branches.

Note that this is a key enabling change for eventually supporting ptex and certain other proposed multi-face/multi-view formats.  The support for those particular formats will come later, at this stage I just want the APIs to be in good shape to accommodate them.

[larrygritz, 2010-10-12 21:38:40]

PS. If reviewing, start with imageio.h, then the contents of src/libtexture.  It
bugs me that Reitveld only does reviews in filesystem order, rather than letting
you specify the order in a way that makes it easier to "tell the story" of the
patch.

[larrygritz, 2010-10-14 06:51:53]

Next round -- I think everything works now

[larrygritz, 2010-10-14 06:54:30]

On 2010/10/14 06:51:53, larrygritz wrote:
> Next round -- I think everything works now

If you read the initial patch, you can just look at the differences between this
new one and the first round.

I'm now passing all tests (well, those that passed before) and I think I've
beefed up the utility programs (iinfo, iconvert, idiff, igrep, iv) to fully
support both mipmaps and subimages (and both).

I'm not saying there is nothing left -- most prominently, the IC/TS has some
FIXMEs mostly related to warnings and statistics.  But all major functionality
is there.

Next I'll actually try to write a Ptex reader and see if I've left anything out.

[larrygritz, 2010-10-16 23:11:35]

OK, ptex files read, get meta, display in iv!

[larrygritz, 2010-10-16 23:11:49]

OK, ptex files read, get meta, display in iv!

[larrygritz, 2010-10-16 23:17:30]

On 2010/10/16 23:11:49, larrygritz wrote:
> OK, ptex files read, get meta, display in iv!

To elaborate: this is the "real" review.  

The APIs have been expanded to accept the new Ptex concepts (specifically, that
a file may have subimage AND mip levels per subimage, and that the
TextureOptions may specify a face/subimage).  These changes, though conceptually
simple, reverberate all through TS, IC, and IB.  I think that everything that
used to work is working properly now.

Also, I've added a ptex plugin.  I can view each ptex face and MIP level in iv
just fine, so it clearly is mostly working.  No writer yet.  And I am currently
NOT filtering correctly when using it as a texture.  I want to get what I have
already reviewed and checked in before I start messing with that, and also I'm
trying to figure out if I can use any of Disney's ptex library to do the
filtering, or need to do it all myself.  Having seen their filtering code, I'm
not very afraid of rewriting it, it's actually not all that extensive.

[larrygritz, 2010-10-21 04:22:23]

Any takers?

Pros or cons?

Anybody want to argue for checking into the trunk, or not?  And if not, what
would need to be done to make it ok to commit?

It's not at all that there's a rush on this, although I would like to avoid
divergence of this from anything else going in to the trunk that will make a
later merge more painful, as well as to move onto the next phase of the ptex
work.

    -- lg

[Chris Foster, 2010-10-21 13:33:46]

This pretty much LGTM Larry, with a few minor comments.

I didn't read all the ImageCache changes in full detail, but it seemed like
there wasn't much in the way of logic changes, mostly just moving code around? 
Also, I just glanced at the ptex input, didn't read it in detail because I'm
assuming it's pretty routine.

I like the compromise you reached with TIFF mipmap emulation.

http://codereview.appspot.com/2405043/diff/22001/src/include/imageio.h
File src/include/imageio.h (right):

http://codereview.appspot.com/2405043/diff/22001/src/include/imageio.h#newcod...
src/include/imageio.h:684: /// respectively.
Docs need to be updated for OpenMode here

http://codereview.appspot.com/2405043/diff/22001/src/include/imageio.h#newcod...
src/include/imageio.h:692: bool append_subimage) { return false; }
Should this disappear before committing, or is it still necessary?

http://codereview.appspot.com/2405043/diff/22001/src/include/texture.h
File src/include/texture.h (right):

http://codereview.appspot.com/2405043/diff/22001/src/include/texture.h#newcod...
src/include/texture.h:340: TypeDesc datatype, void *data) { return false; }
I'm unclear about why this version of get_texture_info is still here.  Don't we
want a compile error if people use the old version?

http://codereview.appspot.com/2405043/diff/22001/src/libtexture/imagecache_pvt.h
File src/libtexture/imagecache_pvt.h (right):

http://codereview.appspot.com/2405043/diff/22001/src/libtexture/imagecache_pv...
src/libtexture/imagecache_pvt.h:371: m_file.filename().hash() * 769;
You might find boost::hash_combine interesting for this kind of thing, it makes
the process of combining multiple numbers into a hash pretty simple.

http://codereview.appspot.com/2405043/diff/22001/src/maketx/maketx.cpp
File src/maketx/maketx.cpp (right):

http://codereview.appspot.com/2405043/diff/22001/src/maketx/maketx.cpp#newcod...
src/maketx/maketx.cpp:672: ImageOutput::AppendMIPLevel :
ImageOutput::AppendSubimage;
This kind of usage certainly seems to indicate that having OpenMode rather than
two bool flags is more convenient :)

http://codereview.appspot.com/2405043/diff/22001/src/tiff.imageio/tiffinput.cpp
File src/tiff.imageio/tiffinput.cpp (right):

http://codereview.appspot.com/2405043/diff/22001/src/tiff.imageio/tiffinput.c...
src/tiff.imageio/tiffinput.cpp:76: // the subimage.  It is indeed a tangled web
of deceit we weave.
Tangled maybe, but I think this is a good compromise :)

[larrygritz, 2010-10-21 15:56:39]

On 2010/10/21 13:33:46, Chris Foster wrote:
> This pretty much LGTM Larry, with a few minor comments.

Thanks.  I will remove the couple of obsolete functions and fix the comments, as
suggested by your review, before committing.

Also, I'll fix the spec/docs before commit.

I'll give everybody 24 hours more to comment before committing, in case there
are any further concerns or objections.  You have been warned!

[larrygritz, 2010-10-21 23:14:30]

Added docs mods, and a couple small changes I realized were needed when writing
the docs

[Chris Foster, 2010-10-21 23:32:25]

LGTM.  I noticed a FIXME you can remove.

http://codereview.appspot.com/2405043/diff/83001/src/ptex.imageio/ptexinput.cpp
File src/ptex.imageio/ptexinput.cpp (right):

http://codereview.appspot.com/2405043/diff/83001/src/ptex.imageio/ptexinput.c...
src/ptex.imageio/ptexinput.cpp:181: std::string wrapmode;
I guess that means you can remove this FIXME comment?