I think that the general resolution will be "no change" then, for now.
Personally, I don't have any particular use for 1-bit DPXs, and there are some things that I'd like to fix in OCIO as a higher priority. I may come back and re-visit this at a later date, but, for us, DPXs have a very specific use, which is as 10-bit log storage containers for ingest/delivery. We do everything internally using EXRs. Cheers 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 15:25, Larry Gritz wrote: > If DPX spec allows float, and it seems to be useful (which I think it > undeniably is), we should support it. > > Glancing at src/dpx.imageio/dpxoutput, it looks to me like the format logic > is already correct -- it accepts uint8, uint16, float, and double; if asked > for half it uses float; and everything else it outputs uint16 as a failsafe. > (I'm presuming for the moment that we have supported it correctly.) > > So the problem all along was that Nuke doesn't support the full DPX spec. > Clearly the solution is that Nuke ought to be using OIIO! (I'm only half > joking; they are on this list, use OIIO in other products, and for all I know > have already fixed this problem for future releases in the way I suggest. > Some other important internal tools at SPI also switched to using OIIO > specifically because its DPX support was more complete, go Leszek!) > > Frankly, I'm also inclined to throw much of the responsibility for this in > the lap of the oiiotool user, and simply say that if you're outputting dpx > and you know some tool in your pipeline doesn't accept float, you need to use > the -d flag appropriately for your output. > > I vote for no API changes at this time. If we want to augment supports() > with format names, I'd be ok with that, I can see how it might be useful for > an app to know which data formats are supported natively. The more I think > about default_format, the less I like it -- I'm not sure how it's useful or > even exactly what it means for all plugins (e.g., how do you express a > complex behavior like if you ask for uint32 it would default to uint16, but > if you ask for half it would default to float?). > > Whether or not we support 1 bit DPX output depends on whether anybody thinks > it's useful. Feel free to submit a patch! > > -- lg > > > On Oct 4, 2011, at 6:58 AM, Hugh Macdonald wrote: > >> 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 > > -- > 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
