Excellent! I can give it a spin. Off-topic Git n00b question: is there an easy was to grab just the changes in that pull request into my fork of oiio?
On Tue, Oct 30, 2012 at 5:15 PM, Larry Gritz <[email protected]> wrote: > Pull request updated. I think there were a lot of little bugs in the writing > of the extensions area in the file. > I believe I've got it all fixed now. > > > On Oct 30, 2012, at 4:17 PM, Larry Gritz wrote: > >> Yeah, there are a few more things that look very sketchy. Let me take >> another look... >> >> >> >> On Oct 30, 2012, at 3:04 PM, Nicolas Burtnyk wrote: >> >>> Hi Larry, >>> >>> I took a peek at your pull request and although it looks like this >>> change will cleanly close tga files on write errors, it still seems >>> like we're bailing too early in the case of empty author or document >>> name fields. >>> >>> For example, in the block of code below, if the image spec doesn't >>> have an "Artist" attribute (and hence tmpstr is empty), we early >>> return false form write_tga20_data_fields skipping all other fields. >>> >>> if (!fwrite (tmpstr.c_str(), std::min (tmpstr.length (), size_t(40)))) >>> return false; >>> >>> Is that the intended behavior? >>> >>> I guess the core of the problem is that the fwrite helper function is >>> returning false not only on write errors, but also when it is asked to >>> write zero bytes and no attempt is made to distinguish bewteen those 2 >>> cases. >>> >>> Perhaps the solution is to have the helper fwrite return true when >>> itemsize or nitems is zero? >>> >>> Also, I noticed the block of code that fills with zeros after writing >>> the "job ID" field (taken from the "DocumentName" attribute) looks >>> kind of sketchy: >>> >>> // job ID >>> tmpstr = m_spec.get_string_attribute ("DocumentName", ""); >>> if (!fwrite (tmpstr.c_str(), std::min (tmpstr.length (), size_t(40)))) >>> return false; >>> >>> // fill the rest with zeros >>> for (int i = 41 - std::min (tmpstr.length (), size_t(40)); i > 0; i--) { >>> if (!fwrite (&tmpint) || >>> !fwrite (tmpstr.c_str(), std::min (tmpstr.length (), size_t(40)))) >>> return false; >>> } >>> >>> Why is it trying to write the DocumentName string again every time it >>> successfully writes 1 zero? >>> >>> >>> -Nicolas >>> >>> >>> On Tue, Oct 30, 2012 at 12:44 PM, Larry Gritz <[email protected]> wrote: >>>> https://github.com/OpenImageIO/oiio/pull/448 >>>> >>>> >>>> On Oct 30, 2012, at 10:53 AM, Larry Gritz wrote: >>>> >>>>> Thanks for pointing that out. >>>>> >>>>> Yes, in fact there are also several places in TGAOutput::open() where the >>>>> file handle is not properly closed. >>>>> >>>>> Sounds easy to fix. Stay tuned for a pull request from me. >>>>> >>>>> >>>>> >>>>> On Oct 30, 2012, at 10:39 AM, Nicolas Burtnyk wrote: >>>>> >>>>>> While testing OIIO, I noticed that TGAs that were being written to >>>>>> disk were not being closed despite calling ImageOutput::close(). >>>>>> Indeed fclose is not called in TGAOutput::close when the "Artist" >>>>>> attribute is empty. >>>>>> The problematic block of code is: >>>>>> >>>>>> if (!fwrite (tmpstr.c_str(), std::min (tmpstr.length (), size_t(40)))) { >>>>>> return false; >>>>>> } >>>>>> >>>>>> Since tmpstr is empty, the itemsize parameter of fwrite is 0 and >>>>>> fwrite returns false, which in turn makes TGAOutput::close early >>>>>> return false (skipping the call to fclose). >>>>>> >>>>>> Writing of other string attributes are similarly problematic when the >>>>>> attribute is empty. >>>>>> >>>>>> -Nicolas >>>> >>>> -- >>>> 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 > > -- > 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
