Interestingly, in the DPX spec (http://www.cineon.com/ff_draft.php, about 1/3rd of the way down), it actually turns out that DPX files *should* be able to support 1, 8, 10, 12 and 16 bit int, and 32 and 64 bit float.
Nuke doesn't support the float formats, so I think that the best solution here would maybe be not *not* make any changes to the code, and just force conversions to DPX to be the required data format. The only format that the DPX spec does allow that OIIO doesn't appear to support is 1-bit. Any thoughts on this? If the spec allows for 1-bit, float and double, should OIIO, even if the main tools that we use for dealing with DPXs don't support these formats... Hugh Macdonald nvizible – VISUAL EFFECTS [email protected] +44(0) 20 3167 3860 +44(0) 7773 764 708 www.nvizible.com On 4 Oct 2011, at 10:55, Hugh Macdonald wrote: > Hi Larry, > > I've sent that CLA through to you directly... > > Not making changes to the API sounds perfectly reasonable - I did something > similar in the data window pull request - the stash_script() internals could > very easily be pasted into each ImageOutput's open() function - because it > was the same code everywhere, and should rely on the same > supports('datawindow') as ImageBuf's decision whether or not to crop the data > window to fit the display window. > > And I certainly plan to be involved - I maybe won't submit sizeable changes > like that without discussion in future... > > > Hugh Macdonald > nvizible – VISUAL EFFECTS > > [email protected] > +44(0) 20 3167 3860 > +44(0) 7773 764 708 > > www.nvizible.com > > On 3 Oct 2011, at 18:30, Larry Gritz wrote: > >> 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 >
_______________________________________________ Oiio-dev mailing list [email protected] http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org
