I originally thought I'd excise the version from the strings before comparison, but before going through the extra effort of doing so, I concluded that remaking the texture if the OIIO version changes is probably not the dumbest idea. It will occasionally remake the texture needlessly, but I think the main thing we're trying to accomplish is making sure that back-to-back (or nearly so, in the grand scheme of things) take a shortcut when it's very confident that it will give the exact same result. Since a change to the maketx internals could change the results (e.g., bug fixes), I think that's probably a time when it should remake the texture if asked.
> On Dec 6, 2015, at 1:12 PM, Thiago Ize <[email protected]> wrote: > > I agree that this more lax checking is probably good enough and is actually > what I originally intended (I didn't think of including the version string, > but I agree that seems even better). I went with the more exact fix since > you seemed concerned with users being surprised if the wrong thing happens. > Your patch seems to work for me in OIIO 1.5 after I replaced the > ImageInput::destroy(in) with in->close(); > > Thanks for the quick fix! > > On Sat, Dec 5, 2015 at 7:13 PM, Larry Gritz <[email protected] > <mailto:[email protected]>> wrote: > How about this one? It's very conservative about when it's safe to skip the > creation of the textures -- which I like now that I see it. Because the OIIO > version is embedded in the "Software" field as well, it means it'll also > rebuild the texture when the version changes, which I think is probably a > good thing, catching fixed bugs and whatnot. > > https://github.com/OpenImageIO/oiio/pull/1281 > <https://github.com/OpenImageIO/oiio/pull/1281> > > > >> On Dec 4, 2015, at 11:47 PM, Larry Gritz <[email protected] >> <mailto:[email protected]>> wrote: >> >> Golly, that looks a lot more complicated than I was going to suggest. >> >> maketx / make_texture_impl end up storing the maketx command line as the >> "Software" metadata >> >> For update mode, if the destination exists, I was merely going to quickly >> open the file as input, read its "Software" metadata, and compare it to the >> present one (excluding the version number, before the colon), and if they >> DON'T match exactly, then assume the texture has to be re-made, even if the >> input and output dates are identical. >> >> I grant that your method is more "exact", inasmuch as it wouldn't produce a >> false positive if you remake the texture with literally different but >> feature-equivalent arguments (for example, the same arguments but in a >> different order), whereas my approach would needlessly remake the texture in >> that case. But my hunch is that this is a rare case -- the command lines of >> a true intended repeat will be identical, and that remaking the texture for >> the odd corner case where this assumption is too stringent is a small price >> to pay for such simple implementation (maybe 10 lines versus ~400). >> >> I'll give it a stab tomorrow. >> >> >>> On Dec 4, 2015, at 7:37 PM, Thiago Ize <[email protected] >>> <mailto:[email protected]>> wrote: >>> >>> I had some free time this evening so I coded up the following quick patch >>> (on our OIIO 1.5). It needs some massage to put it in the OIIO coding >>> format (indentation is off, etc...) and I duplicated some functions which I >>> bet with some refactoring could be much improved. It also doesn't handle >>> checking for user-specified attributes (I have no idea what those are). >>> For my purposes, this is good enough, but I suspect you'll want to clean it >>> up if you end up committing it into OIIO. >>> >>> In argparse.cpp, there is a memory leak if a user called parse(string), and >>> then parse(xargc, xargv). I don't know what your rules are on c++11 for >>> OIIO 1.5, but this code would be way simpler if you could use >>> std::unique_ptr. If you can use an alternative such as from boost, that >>> would work too. >>> >>> Here's how it looks in action: >>> >>> 8:13% maketx ~/Desktop/IMG_1067.JPG --checknan --tile 32 128 -o "out of\ >>> file.tx" -u >>> maketx: no update required for "out of\ file.tx" >>> 8:13% maketx ~/Desktop/IMG_1067.JPG --checknan --tile 32 32 -o "out of\ >>> file.tx" -u >>> 8:13% maketx ~/Desktop/IMG_1067.JPG --checknan -o "out of\ file.tx" -u >>> 8:13% maketx ~/Desktop/IMG_1067.JPG --checknan -o "out file.tx" -u >>> 8:13% maketx ~/Desktop/IMG_1067.JPG --checknan -o "out file.tx" -u >>> maketx: no update required for "out file.tx" >>> 8:14% maketx ~/Desktop/IMG_1067.JPG -o "out file.tx" -u >>> 8:14% maketx ~/Desktop/IMG_1067.JPG -o "out file.tx" -u >>> maketx: no update required for "out file.tx" >>> 8:14% maketx ~/Desktop/IMG_1067.JPG -u >>> 8:14% maketx ~/Desktop/IMG_1067.JPG -u >>> maketx: no update required for "/Users/thiago/Desktop/IMG_1067.tx" >>> 8:14% maketx -u ~/Desktop/IMG_1067.JPG >>> maketx: no update required for "/Users/thiago/Desktop/IMG_1067.tx" >>> 8:14% maketx -u ~/Desktop/IMG_1067.JPG -v >>> maketx: no update required for "/Users/thiago/Desktop/IMG_1067.tx" >>> >>> Thiago >>> >>> On Tue, Dec 1, 2015 at 9:09 PM, Thiago Ize <[email protected] >>> <mailto:[email protected]>> wrote: >>> deal :) >>> >>> On Tue, Dec 1, 2015 at 6:06 PM, Larry Gritz <[email protected] >>> <mailto:[email protected]>> wrote: >>> I'm busy but it also sounds pretty simple. Mainly I didn't want us both to >>> do it. >>> >>> How about this: whichever one of us gets around to starting it first, send >>> the other an email so they know not to redundantly do it. >>> >>> >>> >>>> On Dec 1, 2015, at 4:28 PM, Thiago Ize <[email protected] >>>> <mailto:[email protected]>> wrote: >>>> >>>> I haven't gotten around yet to fixing this. Of course I won't complain if >>>> you (or someone else) wants to tackle this. If you're busy, just let me >>>> know and I'll give it a shot. >>>> >>>> On Tue, Dec 1, 2015 at 5:17 PM, Larry Gritz <[email protected] >>>> <mailto:[email protected]>> wrote: >>>> I think this totally makes sense. The dumb -u behavior is probably just an >>>> artifact of having predated our current use of the metadata and the >>>> growing richness of command-line options. >>>> >>>> So are you proposing to do it, or are you requesting that I do it? >>>> >>>> >>>> >>>>> On Dec 1, 2015, at 4:11 PM, Thiago Ize <[email protected] >>>>> <mailto:[email protected]>> wrote: >>>>> >>>>> That's exactly the solution I was imagining. >>>>> >>>>> I think it's less surprising to rebuild the .tx file if the arguments are >>>>> different. Right now users might be confused why their changes aren't >>>>> taking effect. After this, if users are trying to create the same file >>>>> it will likely early exit and if they are trying something new, it will >>>>> rebuild. The wrong case (what surprises users) shifts from the image >>>>> incorrectly still being outdated to the correct image being used through >>>>> extra redundant work. So trade incorrect image for correct but slower >>>>> images. >>>>> >>>>> On Tue, Dec 1, 2015 at 5:01 PM, Larry Gritz <[email protected] >>>>> <mailto:[email protected]>> wrote: >>>>> The original meaning of -u is "skip the work if the source image is older >>>>> than the existing texture output." Dumb and simple! Also, easy to >>>>> understand and not prone to people wondering why it did or didn't >>>>> succeed. But yes, I totally see your point. >>>>> >>>>> You are proposing to try to make it much smarter: "skip the work if you >>>>> are reasonably certain that you'll get the same image as last time." >>>>> >>>>> I think this should be possible. The command line arguments are stored in >>>>> the metadata of the texture file! So I suppose it could parse that and >>>>> compare to the present command line arguments. >>>>> >>>>> The question is, will this produce a more subtle behavior that >>>>> inadvertently causes people to be frequently surprised or not understand >>>>> what it's doing? >>>>> >>>>> >>>>>> On Dec 1, 2015, at 3:07 PM, Thiago Ize <[email protected] >>>>>> <mailto:[email protected]>> wrote: >>>>>> >>>>>> I think my question is best explained through an example: >>>>>> >>>>>> $ maketx --colorconvert linear sRGB foo.png -o foo.tx >>>>>> ... creates a foo.tx >>>>>> $ maketx -colorconvert linear linear foo.png -o foo.tx -u >>>>>> ... does nothing since timestamps are the same >>>>>> $ maketx -colorconvert linear linear foo.png -o foo.tx -u >>>>>> ... does nothing since timestamps are the same >>>>>> $ maketx foo.png -o foo.tx -u >>>>>> ... does nothing since timestamps are the same >>>>>> >>>>>> Wouldn't it be better if instead of just checking timestamps, it also >>>>>> checked if the arguments used to create the .tx file are different? >>>>>> Then you'd get something like: >>>>>> >>>>>> $ maketx --colorconvert linear sRGB foo.png -o foo.tx >>>>>> ... creates a foo.tx >>>>>> $ maketx -colorconvert linear linear foo.png -o foo.tx -u >>>>>> ... updates foo.tx >>>>>> $ maketx -colorconvert linear linear foo.png -o foo.tx -u >>>>>> ... does nothing since the resulting file would be the same >>>>>> $ maketx foo.png -o foo.tx -u >>>>>> ... updates file since arguments are different (let's not try to think >>>>>> too hard about whether linear to linear would have done the same thing >>>>>> or not). >>>>>> $ maketx foo.png -o foo.tx -u >>>>>> ... does nothing since the resulting file would be the same >>>>>> $ maketx foo.png -u >>>>>> ... ideally would do nothing, but I wouldn't be too upset if it updated. >>>>>> $ maketx -u foo.png >>>>>> ... do nothing. We should ignore ordering differences when applicable >>>>>> >>>>>> I can imagine trying to handle all cases is rather complicated (such as >>>>>> different arguments that produce the same image), but if we can at least >>>>>> get the easy cases, and if in doubt, always do an update, I imagine that >>>>>> would handle 99% of the use cases and would only have the drawback that >>>>>> very rarely maketx would end up doing redundant work, which is annoying >>>>>> but at least results in a correct file. >>>>>> >>>>>> Thiago >>>>>> _______________________________________________ >>>>>> Oiio-dev mailing list >>>>>> [email protected] <mailto:[email protected]> >>>>>> http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org >>>>>> <http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org> >>>>> >>>>> -- >>>>> Larry Gritz >>>>> [email protected] <mailto:[email protected]> >>>>> >>>>> >>>>> >>>>> _______________________________________________ >>>>> Oiio-dev mailing list >>>>> [email protected] <mailto:[email protected]> >>>>> http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org >>>>> <http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org> >>>>> >>>>> >>>>> _______________________________________________ >>>>> Oiio-dev mailing list >>>>> [email protected] <mailto:[email protected]> >>>>> http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org >>>>> <http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org> >>>> >>>> -- >>>> Larry Gritz >>>> [email protected] <mailto:[email protected]> >>>> >>>> >>>> >>>> _______________________________________________ >>>> Oiio-dev mailing list >>>> [email protected] <mailto:[email protected]> >>>> http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org >>>> <http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org> >>>> >>>> >>>> _______________________________________________ >>>> Oiio-dev mailing list >>>> [email protected] <mailto:[email protected]> >>>> http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org >>>> <http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org> >>> >>> -- >>> Larry Gritz >>> [email protected] <mailto:[email protected]> >>> >>> >>> >>> _______________________________________________ >>> Oiio-dev mailing list >>> [email protected] <mailto:[email protected]> >>> http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org >>> <http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org> >>> >>> >>> >>> <maketx-u.diff>_______________________________________________ >>> Oiio-dev mailing list >>> [email protected] <mailto:[email protected]> >>> http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org >>> <http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org> >> -- >> Larry Gritz >> [email protected] <mailto:[email protected]> >> >> >> _______________________________________________ >> Oiio-dev mailing list >> [email protected] <mailto:[email protected]> >> http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org >> <http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org> > > -- > Larry Gritz > [email protected] <mailto:[email protected]> > > > > _______________________________________________ > Oiio-dev mailing list > [email protected] <mailto:[email protected]> > http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org > <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
