feat: global attribute to enable/disable formats at runtime

[Rahul-Baradol]

Description

A solve for the issue #4759

Added OIIO attributes to enable/disable formats at runtime

Tests

• Proving input image formats can be restricted at runtime
• Proving output image formats can be restricted at runtime

Checklist:

• I have read the contribution guidelines.
• I have updated the documentation, if applicable. (Check if there is no
  need to update the documentation, for example if this is a bug fix that
  doesn't change the API.)
• I have ensured that the change is tested somewhere in the testsuite
  (adding new test cases if necessary).
• If I added or modified a C++ API call, I have also amended the
  corresponding Python bindings (and if altering ImageBufAlgo functions, also
  exposed the new functionality as oiiotool options).
• My code follows the prevailing code style of this project. If I haven't
  already run clang-format before submitting, I definitely will look at the CI
  test that runs clang-format and fix anything that it highlights as being
  nonconforming.

[jarias-lfx]

/easycla

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: Rahul-Baradol / name: Rahul Baradol (fd31293, 9a79c85)

[lgritz]

Definitely needs a test, which I think can be straightforward: maybe in testsuite/oiiotool, one test that does something like

oiiotool -oiioattrib allowed_input_format_list tiff -info black.tif 

and it should output the correct error message about not reading that format that it would ordinarily be able to read.

[lgritz]

As I read it, I'm not sure this works. How did you test it?

So what I think this is doing is: when it goes to open a file, it checks the attribute that holds the set of allowed formats, and if the format you're looking for is not in the list, it issues an error and refuses to create the ImageInput or ImageOutput.

But... if the user doesn't set the attribute in the first place, NOTHING is on the list, right? So all requests will be rejected?

Also, I think that as implemented, you're essentially looking up based on the name of the file extension, but that's not the name of the format. (You can pass create() either the name of a format, OR the name of a file, from which it guesses the format based on the file's extension.)

Honestly, I think that what you need to do here, before you work on any more code, is come up for a design for how this feature is supposed to work. Let's iterate on the design first, and then when we all agree that it's the right behavior, it should be straightforward to finish the code implementation.

When I speak of the design, I mean write down, really step by step, what you think the behavior should be. It doesn't need to be long, but it does need to be complete, or else you won't know what you're really trying to implement or how to judge if it's correct. It should be clear from reading this design how one should reason about the feature. Like, it should be easy to answer these questions:

• What are the names being specified -- format names? file extensions? both? (I think it should be format names, since the real use case here is to limit which plugins' code might execute.)
• What is the default behavior if an app/user never sets the attribute? What's its default value?
• How do you ask that only one format is accepted?
• How do you ask for all formats except one be accepted? That is, to disable just one format? Is there a convenient way to do that without needing to first know all the build-time supported formats and enumerate all but what you want?
• Since the usual case is that the vast majority of apps will not need to alter the format list compared to the support enabled at build time, how can we make that common case as close to zero cost to performance as possible?
• Is there a reason to specify it separately for input and output files? What's the use case for allowing a format to be written but not read, or vice versa? Why limit outputs at all, can't that be accomplished by just not trying to write that kind of file?

[Rahul-Baradol]

Alright!

Design 1 (Current implementation's design)

Has two attributes allowed_input_formats and allowed_output_formats.

We call getattribute to get the attribute values.

• If return value is 1, we get the list
• If the return value is 0, we know that attribute was not passed, and hence continue AS IS

And in ImageInput::create() and ImageOutput::create() , what I understand is format could be either the format name or the extension

And yes, the user has to pass file extension and format name, which I realized would be not convenient

After the format is set, we check if it is part of the list passed by the user (via attribute)
If it is not, we return.

And we do this check in both ImageInput::create() and ImageOutput::create()

Design 2

I observed there is a map, which contains mappings of

• format to ImageInput::Creator
• extension to ImageInput::Creator

And declare_imageio_format_locked populates the map by taking format names, input and output extensions, and also the input and output creators.

We can have two attributes allowed_formats and disallowed_formats
We can add a check at the start of declare_imageio_format_locked,

• if allowed_formats is passed, we make sure format name is part of the allowed_formats, if not, we do not add this to the map
• if disallowed_formats is passed, we make sure format name is NOT part of the disallowed_formats, otherwise we do not add this to the map
• if the both attributes are passed, we can maybe show a error message, asking to pass one format
• if none are passed, getattribute() for both attributes would return 0, thereby continuing AS IS

This would be convenient, as the user would have to pass the format names in the attribute list
And that, this would be applied to both input and output files.

We make sure that certain plugins' code is not touched, making it seem OpenImageIO is unaware of the format.

What do you say?

• Should we tweak design 1?
• Go with design 2?
• Or something even more simple that you think of ?

What are your thoughts?

[lgritz]

We call getattribute to get the attribute values.

* If return value is 1, we get the list

* If the return value is 0, we know that attribute was not passed, and hence continue AS IS

I see. Was it like that originally and I misunderstood all along, or did you fix that section?

Yes, now if the string is empty, it returns 0.

Here's what the docs say that getattribute does:

/// Get the named global attribute of OpenImageIO, store it in `*val`.
/// Return `true` if found and it was compatible with the type specified,
/// otherwise return `false` and do not modify the contents of `*val`. 

I think that this is an interesting question and would like others' opinions. I admit that in this case, the wording is ambiguous about exactly what "if found" means. I usually would consider "failed" here to mean either that the attribute does not exist (like, maybe they misspelled it and asked for an unknown name), or that they asked for a particular type and it's not the right kind of type to ask for this attribute. I never really considered that it should "fail" and return false for an attribute that the system knows about, but that the user did not previously proactively set. I don't think there is any other string attribute that works in this manner.

To help us reason about this situation, let's consider what happen if the user code does this:

attribute("allowed_output_formats", "");

what does that do? I think most people will believe it means "do not accept any formats". But either way, we agree that the user HAS set the attribute and that it exists, right?

So then, subsequently, you see:

ustring formats;
bool success = getattribute("allowed_output_formats", TypeString, &formats);

Should success be true or false at this point?

[Rahul-Baradol]

Yeah, I modified the section to handle the attribute that way.

Looking at how getattribute is defined, looks like it returns 0 if the attribute (which is being accessed) is invalid

Now to answer your question using the following snippet,

if (name == "allowed_output_formats" && type == TypeString) {
    if (allowed_output_format_list.empty()) {
        return false;
    }
    *(ustring*)val = allowed_output_format_list;
    return true;
}

success would be false (0)

Even if the user doesn't pass/set the attribute, getattribute would return false, because the default value of ustring is empty string.

Now we can either,

• check for empty strings in getattribute, and return false if the string is empty. If the attribute is not passed, then default value of attribute would be empty string, leading to return false
• make getattribute give us the attribute value AS IS. And we do the checking at the caller side.

We can go for option (2) to avoid confusion in the community, regarding how getattribute works

Regardless of where we handle empty strings, we need to decide to either

• continue silently if the attribute is not set, or if the attribute is set and value is empty string
• continue silently if the attribute is not set, if the attribute is set and value is empty string let the user/app know that the list cannot be empty

[Rahul-Baradol]

How can we proceed here?

[lgritz]

How can we proceed here?

Sorry, got caught up in other things. Will try to look this over today.

[lgritz]

I think this is basically sound, but having getattribute fail just because the (existing, valid!) attribute is the empty string doesn't match how it behaves for everything else. Let's succeed (return true), but then just interpret the empty string as "allow anything". I also gave a suggestion for how to change the retrieval and loop to avoid all the extra string allocation and copying by using a version of split that return string_view's.

[lgritz]

I still believe that we should not return false (which usually indicates that it's an invalid query -- like an attribute name that doesn't exist) for an attribute that exists but happens to be the empty string.

[lgritz]

To solve the "empty list" problem and also eliminate all these string allocations and copies, I would suggest this change here:

Suggested change

	ustring comma_sep_allowed_output_formats;
	if (OIIO::getattribute("allowed_output_formats", TypeString,
	&comma_sep_allowed_output_formats)) {
	std::vector<std::string> allowed_output_formats;
	Strutil::split(comma_sep_allowed_output_formats, allowed_output_formats,
	",", -1);
	bool isValidFormat = 0;
	for (std::string allowed_format : allowed_output_formats) {
	auto allowed_output_formats = Strutil::splitsv(OIIO::get_string_attribute("allowed_output_formats"), ",", -1);
	if (allowed_output_formats.size()) {
	bool isValidFormat = 0;
	for (auto allowed_format : allowed_output_formats) {

[lgritz]

Wait, I think there is still another problem here:

If I'm not mistaken, it's really comparing the file extension to the list of formats, which are not always the same. I think we definitely want the attribute to be the name of formats. So we'll need a step where you use extension_to_format_map to convert the extension to the canonical format name.

[lgritz]

I think, also, that when this attribute is used, we may need to alter the behavior where if the format suggested by the filename extension fails, it tries all the other formats. To see what I mean, let's use an example. Since this is sort of a security feature, consider the case where an application has disabled TIFF files but allows OpenEXR files. Somebody could maliciously name a TIFF file "bad.exr", and then this code as it currently is will think "sure, exr is fine" and proceed, but after failing to open it as an exr file, it will try all formats and happily open it as tiff even though that's supposed to be prohibited. So I think we'll need to add a check there in that "try all formats" logic as well.