> On April 8, 2011, 4:51 p.m., Boroondas Gupte wrote: > > indra/llimage/llimagedimensionsinfo.cpp, line 97 > > <http://codereview.secondlife.com/r/255/diff/2/?file=1426#file1426line97> > > > > 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. > On April 8, 2011, 4:51 p.m., Boroondas Gupte wrote: > > indra/llimage/llimagedimensionsinfo.cpp, line 147 > > <http://codereview.secondlife.com/r/255/diff/2/?file=1426#file1426line147> > > > > ... until here. (Assuming it's the same 8.) Might be worth another > > constant. Come on, Boroondas, it wasn't difficult to find! ;-) > On April 8, 2011, 4:51 p.m., Boroondas Gupte wrote: > > indra/llimage/llimagedimensionsinfo.cpp, lines 217-219 > > <http://codereview.secondlife.com/r/255/diff/2/?file=1426#file1426line217> > > > > 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 ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/255/#review580 ----------------------------------------------------------- On April 7, 2011, 4:20 p.m., Vadim ProductEngine wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://codereview.secondlife.com/r/255/ > ----------------------------------------------------------- > > (Updated April 7, 2011, 4:20 p.m.) > > > Review request for Viewer. > > > Summary > ------- > > * 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. > > > This addresses bug STORM-1118. > http://jira.secondlife.com/browse/STORM-1118 > > > Diffs > ----- > > indra/llimage/llimagedimensionsinfo.h 33ca961b0870 > indra/llimage/llimagedimensionsinfo.cpp 33ca961b0870 > > Diff: http://codereview.secondlife.com/r/255/diff > > > Testing > ------- > > > Thanks, > > Vadim > >
_______________________________________________ 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
