OK, I look forward to getting a patch from you to fix the DPX issue.
Then we can move along to other problems that remain, if any. I'm not strictly
against any of your other deeper structural suggestions, but because API
function signature changes mean a break in compatibility, I always want to make
API changes only when it's clear that something cannot be accomplished cleanly
with the existing API, and only after some discussion on the mail list giving
the opportunity for people to weigh in.
By the way, thanks for the patches, and it's great to see you getting involved!
If you're anticipating contributing more code than the occasional 2-line bug
fix (which clearly you are!), please send me a CLA, and also one from your
employers, if applicable (if you are doing any of the OIIO work "on the job",
or even off the job if you lack an employment agreement that makes it clear
that what you do on your own time doesn't belong to anybody but you). You can
find copies of the CLAs in the "docs/" directory, just sign, scan, and email to
me privately.
-- lg
On Oct 3, 2011, at 9:46 AM, Hugh Macdonald wrote:
> 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
--
Larry Gritz
[email protected]
_______________________________________________
Oiio-dev mailing list
[email protected]
http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org
