By "invalid 32 bit float DPX", do you mean that the DPX standard doesn't
support 32 bit float, but the writer appeared to be trying to write that
anyway? If so, that's certainly a bug! (It would also be a bug, albeit a
different one, if DPX standard supports fp32 but our writer botched it somehow.)
As you saw in the bugs you submitted for ICO and Targa, the plugins are
*supposed* to gracefully handle unsupported formats by simply forcing a valid
data format. It's up to the format plugin to decide what the policy is for
choosing what to do for unsupported formats -- they could simply have a default
format that they use in those cases (perhaps the most common one or the one
most likely to be supported by an unknown reader), or try to pick the "closest"
supported format, perhaps one that will lose the least amount of precision when
converted to.
In other words, the advertised behavior of open() is that the app shouldn't
really need to know which data formats are supported by a plugin, and writer
should choose an appropriate one if the app's request cannot be satisfied.
If this worked properly for DPX, the DPX writer itself would be performing
pretty close to the logic you describe below, and there is no need for oiiotool
to go through the exercise of doing all those queries.
So first things first: Let's fix DPX to notice requests for unsupported formats
and do a reasonable substitution.
Next, let's take a look again at oiiotool -- if the DPX (and potentially
others) were fixed, would you still want to change oiiotool, and/or still want
the additional API calls?
Assuming we do want those API calls (I'm not taking that as a given quite yet),
I think we can make a less radical API change than you propose. I don't see
why we need supports_data_format. Can't we just use the existing supports(),
and have it recognize the text names of the formats ("uint8", "half", etc.) and
respond correctly? It's less clear how we can cram get_default_data_format()
into existing API calls, but -- if it's needed at all -- I'm more inclined to
add a generic introspection method, for which "defaultformat" would be one
possible query, similar to how supports() works as well as how ImageCache and
TextureSystem have getattribute() rather than separate get routines for every
attribute. The advantage to the generic approach (with queries as strings,
rather than separate method calls) is that we can expand the set of messages in
the future without breaking API compatibility.
Let's hash a few of these issues out on the mail list first, then as we reach
some consensus we can move back to the pull request and take a look at your
code.
-- lg
On Oct 3, 2011, at 8:15 AM, Hugh Macdonald wrote:
> The other thing that was bugging me last week was the way that, if oiiotool
> was given an EXR and told to write out a DPX, it would write out an invalid
> 32-bit float DPX.
>
> As such, I've added support for each of the ImageOutput classes to be able to
> specify which data formats they support, plus what they would consider their
> default image format.
>
> This is all in the pull request linked to Issue #180
>
> There are two new functions in each ImageOutput class:
>
> * bool supports_data_format(string format)
> * string get_default_data_format()
>
> In these, I am dealing with the data format as a string rather than a
> TypeDesc enum. This is because of data formats like uint10 and uint12, which
> are actually classified as TypeDesc::UINT16, but with an attribute of
> oiio:BitsPerSample set.
>
> Currently, oiiotool is the only thing that actually deals with these new
> function.
>
> It uses the following logic:
>
> * If -d <format> is an argument, check if this data format is supported. If
> not, print an error and exit
> * Check if the input data format is supported by the image output class.
> --> If it is supported, keep the same data type
> --> If it is not supported, ask the image output class for its default data
> format, and use that
>
>
> This allows for things like:
>
> $ oiiotool foo.exr -o bar.dpx
>
> Which will actually write 10-bit DPX files, rather than the 32-bit float DPXs.
>
>
> I've done my best to ensure that each ImageOutput subclass actually defines
> the correct data formats (doing this was where I came across the typos in
> icooutput.cpp and tgaoutput.cpp), but please do double check this for me.
> Also, I wasn't sure what would be considered the default data type for some
> formats. I guessed for a few. Please do confirm this.
>
> Thanks
>
> Hugh Macdonald
> nvizible – VISUAL EFFECTS
>
> [email protected]
> +44(0) 20 3167 3860
> +44(0) 7773 764 708
>
> www.nvizible.com
>
> _______________________________________________
> Oiio-dev mailing list
> [email protected]
> http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org
--
Larry Gritz
[email protected]
_______________________________________________
Oiio-dev mailing list
[email protected]
http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org
