Am Donnerstag, 21. Mai 2015, 16:01:10 schrieb Taahir Ahmed: > Looks good. I've tested it on big- and little-endian images, and both > load fine. Some comments: > > 1) This was missing from my original commit, but the pfm export code > at src/imageio/format/pfm.c also needs a similar treatment. It > always exports as little-endian, which is fine, but it needs to > ensure that the exported data is actually little-endian.
It is. > 2) You use a variable 'cols' to describe the number of channels in the > image. Given that the number of columns in the image is actually > stored in img->width, it may be better to change the name of 'cols' > to something like 'chans'. That is just a variable name (and it was there before). No need to mess with the code for that. > 3) The branches for 3 channels and one channel are needlessly > different. If you pulled out the fread call, you could write it > like this and it would suffice for both branches: > > ret = fread(buf, chans*sizeof(float), (size_t)(img->width * > img->height), f); if(ret != img->width * img->height) > goto error_corrupt; > > This also has the advantage of reporting truncated files as > corrupt, whereas the current code will return an image with the > truncated region uninitialized. Yes, could be done. I don't think anyone cares though. Convincing Jo to add big endian support was hard enough. It's not like you download PFM files from the Internet. These are used in a workflow to move pixel data from one application to another. > 4) There is no need for the loop on line 67 to run backwards, nor for Yes, there is. If it ran forward we would overwrite our data. Read the code again and try to understand what it does. ***** Spoiler ahead ***** The image has 3 channels, darktable uses 4. > there to be two separate loops on lines 77 and 78. Sure, it can be turned into a single loop, but that would make the code inside harder to read, and chances are good that the compiler will take care of them. > Thanks for getting this support landed. > > Taahir Tobias
signature.asc
Description: This is a digitally signed message part.
------------------------------------------------------------------------------ One dashboard for servers and applications across Physical-Virtual-Cloud Widest out-of-the-box monitoring support with 50+ applications Performance metrics, stats and reports that give you Actionable Insights Deep dive visibility with transaction tracing using APM Insight. http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________ darktable-devel mailing list darktable-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/darktable-devel
