[MEMORY] Reduce file descriptors memory footprint in the ImageCache

[bfraboni]

We would like to reduce the memory usage of file descriptors in the ImageCache as we observed a large amount of duplicated data. This is mainly due to image descriptors (ImageSpec ) being stored for each mip map (LevelInfo ) of each sub-image (SubImageInfo).

Clearing up some memory should be possible in that area ( LevelInfo / SubImageInfo ) but needs a bit a refactor, that I'm happy to start during DevDays with help from @lgritz.

More to come in the next comments about the approach to follow.

Tasks

Preview Give feedback

1. api: remove redundant ImageSpec from ImageCache (issue #4436) #4442
   Dev Days devdays24 help wanted texture / image cache +4
2. api: first draft for removing nativespec from ImageBuf #4482
   +0

[lgritz]

Yes, I agree that we want to store the ImageSpec per subimage, and the individual MIP levels really only need to know their resolutions and tile sizes, I think those are the only things that meaningfully differ from one MIP level to the next. They don't all need their own ImageSpec's at all.

[lgritz]

Additionally, even at the subimage level, we currently store both a "spec" and a "nativespec".

The original concept was that nativespec reflects what was in the file, whereas spec describes what's in memory in the cache. The main way they can differ is (a) in the cache, only a few pixel data types are allowed (uint8, uint16, half, float) and all channels must be the same data type, whereas in the file they can be other types and can differ among the channels; (b) in the cache, the image always appears "tiled", sometimes the real tiling of the file, sometimes looking like one big tile for the whole image, sometimes looking tiled even though it's not ("autotile"). But the real metadata will not change.

(In my defense, when this was designed, we saw a lot less metadata in files, certainly not big things like ICC profiles, thumbnails, or other big things. So having a second ImageSpec didn't seem like a big deal, but now it is.)

OK, so what I'm thinking is that we could store only ONE ImageSpec per subimage, and then only the couple of fields that explain how it differs between the file and the buffer.

I'm not yet of a strong opinion whether the one spec should be the file, with separate fields for the data format and tile size of the buffer? Or if the spec should be the buffer, with additional fields saying what the data formats and tiling was like in the file? (Probably the first option, but I'm not 100% sure.)

The question is what to do about the get_imagespec() and imagespec() methods, both of which take a native flag to say which it's selecting.

IC::get_imagespec makes a copy, so it could copy the one imagespec and then modify those few extra fields if they're asking for the other kind.

IC::imagespec() is a lot trickier because it returns a pointer. This is where I got stuck before. One thing we could do is decide that henceforth, the nativespec parameter is simply ignored, and the spec pointer that is returned is always the file one (? or the memory one?) and add a separate new API call to retrieve the in-memory data type or tile size.

It may also be instructive to see how in ImageSpec itself, there is a copy_dimensions() method, which is a very lightweight way of copying only a few important fields from one spec to another. Maybe there is something analogous that we need in ImageCache.

[bfraboni]

@jmertic PR #4442 is not enough to close this issue, we also need to rework the ImageCache backend and remove the actual source of high memory usage in LevelInfo. That will be the focus of a second PR that'll start soon.

[jmertic]

No worries - we are just trying to link things up for tracking. Feel free to tag other PRs

[lgritz]

Re-opening. The PR should not have been tagged as closing this issue. It was just one necessary step along the way.

[bfraboni]

Hey there, picking up on where this has been left back in October.

Problem

The internal ImageCache in imagecache_pvt.h stores up to two ImageSpec structures per mip map level descriptor (LevelInfo) which is very redundant, so we need to get rid of them to save memory.
This is especially visible when using ptex files which translate to one subimage per ptex face, resulting in milions of ImageSpec used.

For the quick history on the nativespec VS spec distinction see @lgritz comment here

Frontend work (done)

We changed the imagespec API to always reflect what is in the file at mip level 0, i.e. the "nativespec", and we introduced a new function get_cache_dimensions to override the dimensions of an ImageSpec with the ones of a specific mip level. The PR has been merged in OIIO 3.0.

Backend work (todo)

Now that the API has changed the remaining work consists in:

• removing m_spec and nativespec from LevelInfo
• storing the nativespec in ImageCacheFile or SubimageInfo. Note that if these are made pointers, we can reuse the same ImageSpec for every SubImage of a file if they share the same spec, e.g. for ptex faces/subimages.
• adding only the necessary overrides in LevelInfo to reflect what's in the mip level, e.g. with a minified spec structure like the wip version below:

    //! LevelSpec is a minified ImageSpec to describe a miplevel image in the ImageCache.
    //! This only hold fields that can differ from the ImageSpec of the associated subimage,
    //! or not constant across all mip levels.
    struct LevelSpec
    {   
        //! TODO: filter out any unnecessary fields:
        //! * extra_attribs     -> same as native spec
        //! * channelnames      -> same as native spec
        //! * channelformats    -> the image cache only allows one single format for all channels
        //! * alpha_channel     -> same as native spec
        //! * deep              -> same as native spec    
        //! * z_channel         -> same as native spec
        //! * n_channels        -> same as native spec, but maybe convenient to have locally
        
        //! * format            -> constant for all channels of all mip levels; should be stored in subimage once only 
        
        //! fields that changes for each miplevels
        int width;                ///< width of the pixel data window
        int height;               ///< height of the pixel data window
        int depth;                ///< depth of pixel data, >1 indicates a "volume"
        int tile_width;           ///< tile width (0 for a non-tiled image)
        int tile_height;          ///< tile height (0 for a non-tiled image)
        int tile_depth;           ///< tile depth (0 for a non-tiled image,
                                  //<             1 for a non-volume image)
        //! TODO: fields we are not sure about
        int x;                    ///< origin (upper left corner) of pixel data
        int y;                    ///< origin (upper left corner) of pixel data
        int z;                    ///< origin (upper left corner) of pixel data
        int full_x;               ///< origin of the full (display) window
        int full_y;               ///< origin of the full (display) window
        int full_z;               ///< origin of the full (display) window
        int full_width;           ///< width of the full (display) window
        int full_height;          ///< height of the full (display) window
        int full_depth;           ///< depth of the full (display) window
    };

• changing the logic of LevelInfo::spec() and SubImageInfo::spec() to reflect these changes
• make sure all unit tests pass