Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

invalidate() on a cached entry doesn't seem to reset its m_configspec? #1632

Closed
devernay opened this issue Mar 5, 2017 · 6 comments
Closed

Comments

@devernay
Copy link
Contributor

devernay commented Mar 5, 2017

Here's my use case, but I'm not sure if I'm doing something wrong, or OIIO does:

  • I want to open a file using the OIIO cache, passing extra config parameters (for RAW files), so I'm using add_file() before get_imagespecs()
  • at some point (a decoding parameter changed), I invalidate the file in the cache, and re-call add_file() with the new config spec, gefore the next get_imagespec()
  • OIIO doesn't take into account the new the config spec, probably because invalidate_spec() doesn't reset m_configpec

Am I doing something wrong?
Is there a way to reopen a file with a new config spec?

@devernay
Copy link
Contributor Author

devernay commented Mar 5, 2017

btw ImageCacheFile::m_configspec is a memory leak (allocated in the constructor, never deleted)

@devernay
Copy link
Contributor Author

devernay commented Mar 5, 2017

since m_configspec is set in the ImageCacheFile constructor, the only clean way to do it would be that invalidate() actually deletes the ImageCacheFile, but it merely seems to keep it in an invalid state, so that the next add_file() does nothing.

@lgritz
Copy link
Collaborator

lgritz commented Mar 6, 2017

I think you are basically correct about all this.

There are performance and safety reasons why we don't want to delete the whole ImageCacheFile upon invalidation.

But, I think that the real problem is that add_file, if the file already exists, just does nothing (and in particular does nothing with the config passed to it). I think a better behavior is that add_file of a file that already exists in the cache ought to invalidate it and use the passed configuration as a replacement for whatever config it may have had before.

I'll see if I can prepare a patch to address this, and also the leak that you point out.

@devernay
Copy link
Contributor Author

devernay commented Mar 6, 2017

I think add_file should do nothing if the file already exists in the cache and is valid, but should load the new config if the file was invalidated.

If add_file always invalidates the file, then we need a way in the API to know if a file is present and valid in the cache, because we don't want to invalidate the cache every time we load a file (I do add_file every time I open a file, because I have no way to know if it is already in the cache and valid)

@lgritz
Copy link
Collaborator

lgritz commented Mar 7, 2017

OK, that makes sense to me: add_file should not invalidate, but if the file it mentions is already invalidated, it can replace the config with the new one. Working on it.

@lgritz
Copy link
Collaborator

lgritz commented Dec 29, 2018

I think that this old issue was addressed by the fairly recent PR #2021. That change (which can be found in OIIO 2.0 as well as master) added a replace parameter to add_file(), which controls what happens if the file being added is already in the cache -- if replace == false (the default), the add_file is just a no-op if the file is found, whereas if replace == true, a found file will be invalidated and replaced according to the new creator and configuration spec.

So I'm going to close this. Please re-file if the new feature is inadequate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants