On 12/04/2011 9:31 AM, Vadim ProductEngine wrote: > This is an automatically generated e-mail. To reply, visit: > http://codereview.secondlife.com/r/255/ > > > On April 8th, 2011, 4:51 p.m., *Boroondas Gupte* wrote: > > indra/llimage/llimagedimensionsinfo.cpp > > <http://codereview.secondlife.com/r/255/diff/2/?file=1426#file1426line97> > (Diff revision 2) > > bool LLImageDimensionsInfo::load(const std::string& src_filename,U32 > codec) > > > > 97 > > mInfile.seek(APR_CUR, 16); > > Magic number. (I guess it's BMP header (14) + DIB header - current > position (File begin + 2)?) > > Yep, pretty obvious. If I start documenting every line, code will eventually > look even worse than without comments.
That is what the documentation is for. The code can always be checked against that. > > On April 8th, 2011, 4:51 p.m., *Boroondas Gupte* wrote: > > indra/llimage/llimagedimensionsinfo.cpp > > <http://codereview.secondlife.com/r/255/diff/2/?file=1426#file1426line147> > (Diff revision 2) > > bool LLImageDimensionsInfo::load(const std::string& src_filename,U32 > codec) > > > > 147 > > mInfile.seek(APR_CUR, 8 /* chunk length + chunk type */); > > ... until here. (Assuming it's the same 8.) Might be worth another > constant. > > Come on, Boroondas, it wasn't difficult to find! ;-) > > On April 8th, 2011, 4:51 p.m., *Boroondas Gupte* wrote: > > indra/llimage/llimagedimensionsinfo.cpp > > <http://codereview.secondlife.com/r/255/diff/2/?file=1426#file1426line217> > (Diff revision 2) > > > > bool LLImageDimensionsInfo::checkFileLength(S32 min_len) > > > > 217 > > char* buf = new char[min_len]; > > > > 218 > > int nread = mInfile.read(buf, min_len); > > > > 219 > > delete[] buf; > > Wouldn't a seek be a "cheaper" way to determine size than reading > into an actual buffer? (According to indra/llcommon/llapr.h, it returns -1 on > failure.) > > There's also a static method LLAPRFile::size, but that seems to > operate on not-yet-opened files given by filename. > > seek() may go beyond the file end, so we can't use it to check whether the > file contains the needed header. > Well, we could seek to the end of file, but accessing the whole file just to > find out that it's of wrong type looks like overkill. > I agree that reading into a dynamically allocated buffer doesn't look nice, > but I think we can live with it as long as we only read small chunks this way. > > - Vadim > > > On April 7th, 2011, 4:20 p.m., Vadim ProductEngine wrote: > > Review request for Viewer. > By Vadim ProductEngine. > > /Updated April 7, 2011, 4:20 p.m./ > > > Description > > * Added checks for image file contents not matching the file extension (e.g. > a bitmap named file.jpg). > * Added checks for abnormally short files to avoid crashes when parsing them. > > *Bugs: * STORM-1118 <http://jira.secondlife.com/browse/STORM-1118> > > > Diffs > > * indra/llimage/llimagedimensionsinfo.h (33ca961b0870) > * indra/llimage/llimagedimensionsinfo.cpp (33ca961b0870) > > View Diff <http://codereview.secondlife.com/r/255/diff/> > > > _______________________________________________ > Policies and (un)subscribe information available here: > http://wiki.secondlife.com/wiki/OpenSource-Dev > Please read the policies before posting to keep unmoderated posting privileges -- Tateru Nino http://dwellonit.taterunino.net/
_______________________________________________ Policies and (un)subscribe information available here: http://wiki.secondlife.com/wiki/OpenSource-Dev Please read the policies before posting to keep unmoderated posting privileges
