fix(ImageBuf): IB::nmiplevels() returns 0 for non-IC ImageBuf

[luna-y-kim]

The ImageBuf constructor path without an ImageCache ends in init_spec(). There, the non-IC branch only reset m_nmiplevels = 0 and never assigned a real value, so nmiplevels() always returned 0.

Now init_spec() sets m_nmiplevels = 1 if the format doesn't support mipmaps, or assigns the number of MIP levels if it can be known up front. If a format does support mipmaps but the count requires walking through the levels, then it's counted by iteration, lazily on the nmiplevels() call. The iteration calls spec_dimensions() repeatedly until it returns an empty ImageSpec.

This improves EXR, DDS, and Ptex to set the "oiio:miplevels" attribute when read (Ptex previously set it only for MIP-mapped files), so that init_spec() can get the attribute's value and save it to ImageBufImpl::m_nmiplevels. For TIFF, it only sets the attribute to 1 when a file has no texture format tag, because an iteration would be needed to get the number of MIP levels otherwise. Setting the attribute to 1 up front avoids an unnecessary open for non-mipmapped TIFF files.

Additionally, an ImageBuf that doesn't have a direct reference to a file returns 1 as documented.

A test for IB::nmiplevels() is added to the python-imagebuf testsuite. Also test reference files affected by the new "oiio:miplevels" attribute are all updated accordingly.

Fixes #5409

[lgritz]

I'm worried that this is quite expensive, since seek_subimage will read/decode all the MIP image metadata. It will make opening a file with an ImageBuf bear the expense of seek_subimage calls even if no mip levels will ever be needed.

I'm wondering a few things:

• Do we have a definitive list of the file formats we support that can have MIP levels at all, and for those that do, whether the number can be known up front or must be iterated over all levels to find out?
• I think certainly we don't want to do the iteration unless the format supports MIP-maps, the image IS a mip map, AND we can't tell the number of mip levels up front.
• I wonder if seek_subimage is a lot less efficient than calling spec_dimensions, for two reasons: (a) spec_dimensions doesn't reset the "current" MIP level, and can be thread-concurrent (no locking needed), and generally doesn't need to re-seek back to the original level again when it's done; (b) because spec_dimensions only returns a partial spec giving the "size", it either is (hopefully) or could be made (future) less expensive by knowing it can skip work related to retrieval of the full metadata for that MIP level.

Or, another possibility is that we don't do it as part of the read() at all, but instead use the separate ImageBuf::nmiplevels() method for this. Right now it returns m_nmiplevels, which obviously might not know. But it could instead figure it out by doing the seeks or spec_dimensions or whatever, if it doesn't already set it.

In other words, we split the responsibilities to figure this out lazily:

• The ImageBuf, upon init_spec(), knows the number of MIP levels, or that there are MIP levels but it doesn't know the exact number yet (some file formats have an inexpensive way to know for sure, others don't, and that's why it could be in either state).
• ImageBuf::nmiplevels() returns the cached number if it was known for sure from init_spec(), or if it has already been computed. Otherwise, it does the potentially expensive thing of seeking or spec_dimensions'ing, saving the result and returning it.

So that would prevent/postpone the expensive kind of search until the occasion that the user actually needs to know the answer, conveyed by calling nmiplevels().

Thoughts?

[luna-y-kim]

Oops, it was an oversight. Here are my thoughts after some investigation:

• Currently mipmap supporting formats are: DDS, OpenEXR, Ptex, and TIFF.

  For DDS, OpenEXR, and Ptex, the number of mip levels can be known up front.

  • DDS: it is held privately in the ImageInput subclass and thrown away currently.
  • OpenEXR: commented out spec attribute setup for "oiio:miplevels" exists (comment says unit tests fail).

    OpenImageIO/src/openexr.imageio/exrinput.cpp

    Lines 1148 to 1151 in 1992123

    	//! Add the number of miplevels as an attribute for the first miplevel.
    	//! TOFIX: adding the following attribute breaks unit tests
    	// if (m_miplevel == 0 && part.nmiplevels > 1)
    	// m_spec.attribute("oiio:miplevels", part.nmiplevels);

  • Ptex: already sets the spec attribute for "oiio:miplevels".
    

    OpenImageIO/src/ptex.imageio/ptexinput.cpp

    Lines 445 to 447 in 1992123

    	// Add the number of miplevels as an attribute for the first miplevel.
    	if (miplevel == 0 && nmiplevels > 1)
    	m_spec.attribute("oiio:miplevels", nmiplevels);

  For TIFF, an iteration would be needed.

• I didn't realize I could use spec_dimensions() like that, but after looking at it, I think it will work great for the iteration case.

• Having a lazy nmiplevels() makes sense for iteration-required formats. And for others, grabbing the values during init_spec() would be cheap.

So I think the next steps are:

1. Re-enable exr's spec attribute setup + add new DDS spec attribute setup (oiio:miplevels).
2. In init_spec(), replace the seek_subimage() iteration with the following: (1) check supports("mipmap"), set the count to 1 if not, (2) check spec.get_int_attribute("oiio:miplevels"), save if it exists.
3. In IB::nmiplevels(), (1) validate_spec() (which calls init_spec() if m_spec_valid is false) (2) check if the value is already set (3) otherwise compute it by iteration with spec_dimensions().

Am I missing something?.....

Or would checking spec.get_int_attribute in IB::nmiplevels() instead be better?

[luna-y-kim]

The test reference python-imagebuf/ref/out-alt.txt is left untouched, since PR #5452 removes it.

[luna-y-kim]

Oh, #5449 replaces builtinplugins.rst. @lgritz, are you planning to merge 5449 first? If so, I'll wait a bit and adjust the doc changes after.

[lgritz]

Yeah, this is kind of an icky one-time switch and I'm hoping to get it merged soon. Just want to give people a chance to air any concerns. But inspecting the results, it looks like everything translated just fine, and I challenge anyone to find a visible difference compared to the docs in rst. (Of course, it cheats a little by lapsing into rst for short sections within the md files when it needs to.)