Page MenuHome

ImBuf: add support for detecting the file format from memory
ClosedPublic

Authored by Campbell Barton (campbellbarton) on Nov 10 2020, 2:02 AM.

Details

Summary

Add IMB_ispic_type_from_memory so we can detect the format of a packed file.

This removes is_a_filepath callback and uses a magic check for photoshop files
that's compatible with OIIO.

This means we can know the file format without having the file on-disk (packed files) for e.g.

Even though OIIO doesn't support packed images, this doesn't have to prevent us from being able to check the file-type of in-memory data.


To cover points from https://wiki.blender.org/wiki/Process/Contributing_Code#Ingredients_of_a_Patch

  • Description of the problem that is addressed in the patch.

    Currently we can't know the format of a packed image.
  • Description of the proposed solution, and a motivation as to why this is the best solution.

    Use is_a callbacks for all image types (this was only missing for photoshop files)
  • List of alternative solutions, and a motivation as to why these are undesirable.

    D9500: Add file extension to unpacked image could be fixed differently, for example the file-path of generated image files could always be set on packing, but this only solves the issue for newly packed files.
  • Limitations of the proposed solution.

    None, it's using existing API's in a more consistent way.

Diff Detail

Repository
rB Blender
Branch
TEMP-IMB_ispic_type_from_memory (branched from master)
Build Status
Buildable 11199
Build 11199: arc lint + arc unit

Event Timeline

Campbell Barton (campbellbarton) requested review of this revision.Nov 10 2020, 2:02 AM
Campbell Barton (campbellbarton) created this revision.
Campbell Barton (campbellbarton) retitled this revision from Add support for detecting the file format from memory to ImBuf: add support for detecting the file format from memory.Nov 10 2020, 2:10 AM
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)
Dalai Felinto (dfelinto) added 1 blocking reviewer(s): Sergey Sharybin (sergey).

Assuming it works, the code seems fine.

Sergey Sharybin (sergey) requested changes to this revision.Nov 10 2020, 10:52 AM

Are there any changes on user level with the current state of the patch?

source/blender/imbuf/intern/oiio/openimageio_api.cpp
168

Once the size is known, check it to be at least sizeof(magic).

source/blender/imbuf/intern/oiio/openimageio_api.h
34

Such functions are to receive usable size of memory together with its pointer:

int imb_is_a_photoshop(const unsigned char *mem, const size_t mem_size);

Also should be bool.

This revision now requires changes to proceed.Nov 10 2020, 10:52 AM

No user level changes.

The other changes you suggest would require changes to all is_a callbacks, I don't think that's needed for this patch since it's in keeping with existing code.

Although agree it's quite bad to pass in memory and not the size.

User benefit is performance unpacking images.

For clarity of communication with artists and other developers, this better to be explained a bit more like.

User benefit is performance unpacking images. The actual timing will depend on specific configuration of file: if there are only handful of packed files the performance improvement is unlikely to be noticeable. In something more extreme case (like renderfarm which packs all images prior to distribution) speedup can be 10x.

The other changes you suggest would require changes to all is_a callbacks, I don't think that's needed for this patch since it's in keeping with existing code.

It is not immediately clear that the signature of the new function you are adding is following that. Good thing to do is to mention in the description that the new code you are adding is following an existing code.

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)

Thanks for more detailed description. Now it is more clear how it all fits into the bigger picture.

source/blender/imbuf/intern/util.c
196–197

You can reduce the scope by for (const ImFileType *type =.

This revision is now accepted and ready to land.Nov 10 2020, 12:12 PM

On a related note, can (in a separate work) ispic be renamed to something else? I still don't know what it means and it got me to misread the return functions even.