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 IB::nmiplevels() always returned 0.

Now IB::nmiplevels() returns 1 for non-MIP-mapped images, or the number of MIP levels if it can be known up front using the oiio:miplevels spec attribute. If an image is MIP-mapped but the count requires walking through the levels, then it is counted by iteration lazily. The iteration calls spec_dimensions() repeatedly until it returns an empty ImageSpec, which is less expensive than seek_subimage().

A return value of 0 for IB::nmiplevels() indicates failures such as a failure in validate_spec(), a file open failure, or a seek_subimage() failure inside spec_dimensions().

This improves ImageInput for EXR and DDS to set the "oiio:miplevels" attribute (Ptex is already setting it), so that IB::nmiplevels() can get the attribute's value and save it to ImageBufImpl::m_nmiplevels. For TIFF, it only sets the attribute to 0 if the image is MIP-mapped, which triggers the iteration count in IB::nmiplevels().

A test for IB::nmiplevels() is added to the python-imagebuf testsuite.
Documentation for the oiio:miplevels attribute is also updated.

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.)

[luna-y-kim]

All conflicts resolved!

[lgritz]

Sorry it took me a while to get back to this, busy with releases.

I have to say, I'm not crazy about all the ref outputs being cluttered with "oiio:miplevels" set to 1. I'd like to avoid that if possible, so I want to propose a slightly different approach:

• ImageInput sets "oiio:miplevels" to
  • a value >= 1 if the file is mip-mapped and the number of levels can be known cheaply
  • 0 if the file is be MIPmapped but you can't tell how many levels without doing something expensive
  • not set at all in all other cases (which are the vast majority of cases, including non-MIP-mapped files and file formats that don't support MIP-mapping)
• The IB retrieves it with get_int_attribute("oiio:miplevels", 1) meaning that you assume 1 if no other signal is present.
• Only if you get 0 from the above retrieval do you do the expensive traversal, so you have a construct that looks like

    int IB::nmiplevels() {
        if (m_miplevels == 0) {
            m_miplevels = spec.get_int_attribute("oiio:miplevels", 1);
            if (m_miplevels <= 0)
                m_mipevels = ...the expensive walk...
        }
        return m_miplevels;
    }
  

I think that this is very close overall to what you have implemented, but has the advantage that most of the reference outputs will not need to change, since the vast majority of cases, the "oiio:miplevels" will not be present, rather than be 1 needlessly.

(If it bugs people to have "0" mean "mipmapped, but not known", then we could use -1 there? Then the nonzeroness means "mipmapped" and the negativity means "but we don't know how many levels yet.)

[luna-y-kim]

That makes sense. Updated. I used the value 0 for oiio:miplevels (MIP-mapped, count unknown) for consistency because oiio:subimages uses 0 for the same purpose.
I feel like now all those oiio:miplevels: 0 for tiff are a bit noisy, but I think at least it now clearly indicates that the file is MIP-mapped.

[lgritz]

I think we're still not there yet. I definitely don't want the output to say "oiio:miplevels" 0. Output like you'd get from "oiiotool --info" or "iinfo" should not include this, I think it's confusing. But I'm not sure how to fix this yet, let me sleep on it and think about it over the weekend.

Remind me -- is TIFF the only format where it might be MIP-mapped, but we can't tell up front how many MIP levels there are?

[luna-y-kim]

Remind me -- is TIFF the only format where it might be MIP-mapped, but we can't tell up front how many MIP levels there are?

Yeah. Currently TIFF is the only format where we can't know the MIP level count up front (among the formats supporting mipmaps).

[lgritz]

TIFF is special. It doesn't really support MIP-maps, they are emulated. It only supports subimages, but when the TIFFTAG_PIXAR_TEXTUREFORMAT tag is present in the header, we choose to interpret / pretend that the multiple images in the file are MIP levels, and we also set the "textureformat" attribute in the ImageSpec when that happens.

I'm wondering if maybe ImageInput should only set "oiio:miplevels" if it knows the number of levels for sure (i.e., always >= 1 and only set for true MIP mapped files) and not set it for the case where it doesn't know yet. Then ImageBuf can use the combination of the presence of "textureformat" and the absence of "oiio:miplevels" to understand that this is the sole case where it's MIP-mapped but it doesn't know how many levels yet, and that if IB::miplevels() is called, it will have to do the possibly-expensive inventory.

[luna-y-kim]

Oh! I used the m_emulate_mipmap member (derived from the TIFFTAG_PIXAR_TEXTUREFORMAT tag) to check if it is MIP-mapped (although it's not really MIP-mapped), but didn't notice the textureformat attribute. I took a quick look, and the combination of "textureformat" being present and "oiio:miplevels" being absent sounds promising. I'll see if that works!

[jinhgkim]

Hello, just throwing out another idea while following along this thread:

Can we just simply check the m_emulate_mipmap flag and read TIFFNumberOfDirectories to populate oiio:miplevels? My understanding of TIFF is that it does not natively support mipmap but we use subimage space to emulate it when the flag is set. And libtiff provides TIFFNumberOfDirectories, which gives you the number of images (which will act as mipmaps in our case) in tiff cheaply.

So, something like:

if (m_emulate_mipmap) {
    // treat every directory as a MIP level
    int nmiplevels = int(TIFFNumberOfDirectories(m_tif));
    if (nmiplevels > 1)
        m_spec.attribute("oiio:miplevels", nmiplevels);
}

[lgritz]

Ooh, Jinnie FTW! I hadn't thought of that! Verify with a couple MIP-mapped TIFF files, but it sure sounds like it might work.