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

Fix ImageBuf vs ImageCache out-of-date image problem #2696

Merged
merged 1 commit into from
Sep 14, 2020

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Sep 3, 2020

ImageBuf uses ImageCache underneath. But this can lead to the following
unexpected behavior if you don't understand this:

  1. Make an ImageBuf for an image, read it (even just header info).
  2. Done with it, destroy the ImageBuf.
  3. Some other part of the program, or even a different process, changes
    that file on disk.
  4. Make another ImageBuf with the same filename, thinking you're getting
    the updated image, but in fact it's relying on the cached representation
    which doesn't contain the changes made in step 3.

So these tiny changes try to make this much harder to shoot yourself in the
foot:

  • New ImageBufs automatically call cache->invalidate on the filename
    before they start reading, to ensure that if the file has changed on
    disk, anything in the cache is invalidated.
  • When you destroy or clear an ImageBuf, it calls cache->close() on the
    file, to release the handle (especially helpful on Windows, where an
    open file handle can lock the file to other processes).

There is a chance that some image operations will have degraded
performance, if you are mixing direct ImageCache use (including
TextureSystem) and ImageBuf reads of the same files -- since creating
and destroying the IB's will flush tiles from the cache and close cached
file handles. But I think that this should not be a problem in practice,
since most apps that are heavy users of ImageBuf (like oiiotool) aren't
heavey users of IC/TS, and vice versa. And eliminating this confusing
behavior is probably more important than occasional over-eager
invalidation of the cache.

Also clarify in the docs (and imagebuf_test) that if you use the
variety of ImageBuf ctr where you give it a custom ImageCache, it's
important that the ImageBuf outlives the ImageCache, as it is a
non-owning reference.

Fixes #1987

Signed-off-by: Larry Gritz lg@larrygritz.com

ImageBuf uses ImageCache underneath. But this can lead to the following
unexpected behavior if you don't understand this:

1. Make an ImageBuf for an image, read it (even just header info).
2. Done with it, destroy the ImageBuf.
3. Some other part of the program, or even a different process, changes
   that file on disk.
4. Make another ImageBuf with the same filename, thinking you're getting
   the updated image, but in fact it's relying on the cached representation
   which doesn't contain the changes made in step 3.

So these tiny changes try to make this much harder to shoot yourself in the
foot:

* New ImageBufs automatically call cache->invalidate on the filename
  before they start reading, to ensure that if the file has changed on
  disk, anything in the cache is invalidated.
* When you destroy or clear an ImageBuf, it calls cache->close() on the
  file, to release the handle (especially helpful on Windows, where an
  open file handle can lock the file to other processes).

There is a chance that some image operations will have degraded
performance, if you are mixing direct ImageCache use (including
TextureSystem) and ImageBuf reads of the same files -- since creating
and destroying the IB's will flush tiles from the cache and close cached
file handles. But I think that this should not be a problem in practice,
since most apps that are heavy users of ImageBuf (like oiiotool) aren't
heavey users of IC/TS, and vice versa. And eliminating this confusing
behavior is probably more important than occasional over-eager
invalidation of the cache.

Also clarify in the docs (and imagebuf_test) that if you use the
variety of ImageBuf ctr where you give it a custom ImageCache, it's
important that the ImageBuf outlives the ImageCache, as it is a
non-owning reference.

Fixes 1987

Signed-off-by: Larry Gritz <lg@larrygritz.com>
@lgritz
Copy link
Collaborator Author

lgritz commented Sep 10, 2020

Does anybody think this is a bad idea?

@lgritz lgritz merged commit eb2818d into AcademySoftwareFoundation:master Sep 14, 2020
@lgritz lgritz deleted the lg-cache branch September 14, 2020 01:29
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Oct 3, 2020
…Foundation#2696)

ImageBuf uses ImageCache underneath. But this can lead to the following
unexpected behavior if you don't understand this:

1. Make an ImageBuf for an image, read it (even just header info).
2. Done with it, destroy the ImageBuf.
3. Some other part of the program, or even a different process, changes
   that file on disk.
4. Make another ImageBuf with the same filename, thinking you're getting
   the updated image, but in fact it's relying on the cached representation
   which doesn't contain the changes made in step 3.

So these tiny changes try to make this much harder to shoot yourself in the
foot:

* New ImageBufs automatically call cache->invalidate on the filename
  before they start reading, to ensure that if the file has changed on
  disk, anything in the cache is invalidated.
* When you destroy or clear an ImageBuf, it calls cache->close() on the
  file, to release the handle (especially helpful on Windows, where an
  open file handle can lock the file to other processes).

There is a chance that some image operations will have degraded
performance, if you are mixing direct ImageCache use (including
TextureSystem) and ImageBuf reads of the same files -- since creating
and destroying the IB's will flush tiles from the cache and close cached
file handles. But I think that this should not be a problem in practice,
since most apps that are heavy users of ImageBuf (like oiiotool) aren't
heavey users of IC/TS, and vice versa. And eliminating this confusing
behavior is probably more important than occasional over-eager
invalidation of the cache.

Also clarify in the docs (and imagebuf_test) that if you use the
variety of ImageBuf ctr where you give it a custom ImageCache, it's
important that the ImageBuf outlives the ImageCache, as it is a
non-owning reference.

Fixes 1987

Signed-off-by: Larry Gritz <lg@larrygritz.com>
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

Successfully merging this pull request may close these issues.

OIIO python api is not properly overwriting an image
1 participant