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]> 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 > > > > On Dec 4, 2015, at 11:47 PM, Larry Gritz <[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]> 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]> wrote: > >> deal :) >> >> On Tue, Dec 1, 2015 at 6:06 PM, Larry Gritz <[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]> 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]> 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]> 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]> 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]> 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] >>>>> 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 >>> >>> >> > <maketx-u.diff>_______________________________________________ > 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 > > > -- > 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
