> 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. > > Vadim ProductEngine wrote: > Come on, Boroondas, it wasn't difficult to find! ;-)
Ok, ok, I'll give you that one. > 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. > > Vadim ProductEngine wrote: > 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. There's no pointer equivalent of /dev/null that could be used here instead of an actual buffer, is there? > 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)?) > > Vadim ProductEngine wrote: > Yep, pretty obvious. If I start documenting every line, code will > eventually look even worse than without comments. Is it? Took me some while to figure this out, and even then I wasn't sure. An alternative to comments: Instead of a constant for DATA_LEN, have constants for its components, and re-use those constants here. - Boroondas ----------------------------------------------------------- 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
