The change works well in my basic tests. Thanks Larry!
On Tue, Oct 30, 2012 at 5:22 PM, Larry Gritz <[email protected]> wrote: > Yeah, I think you want something like this: > > git checkout master # start with master > git checkout -b test # make a new branch > git pull https://github.com/lgritz/oiio.git lg-tga # pull my changes > > > > On Oct 30, 2012, at 5:17 PM, Nicolas Burtnyk wrote: > >> 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 > > -- > 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
