On 3 Oct 2011, at 17:23, Larry Gritz wrote:
> 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.)
I would need to re-check the spec, but Nuke, for example, only supports 8, 10,
12 and 16-bit DPXs.
Taking a further look into dpxoutput.cpp, there is a bit where it checks
oiio:BitsPerSample, but it never seems to do the right check with m_spec.format.
> 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.
I think that that makes perfect sense... As you said, most of the writers do
support this already. I'm sure that there were one or two others that I
couldn't find anything in already, so there may be other issues out there, but
DPX is the one I came across issues with.
> 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?
I think probably not - doing it all in the individual open() calls would pretty
much negate, in my mind, the need for either of the new functions. (It was a
good way to get my head around certain parts of OIIO, though, so not wasted
time on my part...)
> 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.
Sure - sounds good. I'm happy to take all of this a different direction - as
the DPX writer needs fixing, I can take a look into that (as it's the one that
we had the issue with ourselves...)
Cheers
Hugh Macdonald
nvizible – VISUAL EFFECTS
[email protected]
+44(0) 20 3167 3860
+44(0) 7773 764 708
www.nvizible.com
>
> 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
_______________________________________________
Oiio-dev mailing list
[email protected]
http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org
