STR #3308: Inconsistent interpretation of ld() in image handling.

STR #3308

Application:	FLTK Library
Status:	1 - Closed w/Resolution
Priority:	4 - High, e.g. key functionality not working
Scope:	3 - Applies to all machines and operating systems
Subsystem:	Image Support
Summary:	Inconsistent interpretation of ld() in image handling.
Version:	1.3.3
Created By:	AlbrechtS
Assigned To:	AlbrechtS
Fix Version:	1.3.4 (SVN: v12029)
Update Notification:	Receive EMails Don't Receive EMails

Trouble Report Files:

#1	AlbrechtS 17:25 Oct 01, 2016

patch_v1.diff
5k

Trouble Report Comments:

#1	AlbrechtS 08:51 May 30, 2016

As discussed in STR #3296 the documentation and usage of the ld() value of images is inconsistent and probably wrong in the docs and at least in src/Fl_win32.cxx, line #2247 in method static HICON image_to_icon(). This is pretty new code and it is likely that this usage is wrong, although consistent with the docs of Fl_Image. Most of the image handling code interprets ld() as the delta in bytes between two pixels of the same column in adjacent lines. Note that ld() can be negative to flip images vertically. documentation/drawing.dox, line #821 explicitly states: "\p D is the delta to add to the pointer between pixels, it may be any value greater or equal to \p 3, or it can be negative to flip the image horizontally. \p L is the delta to add to the pointer between lines (if 0 is passed it uses \p W*D). and may be larger than \p W*D to crop data, or negative to flip the image vertically." I consider this the authoritative documentation, and most of the image handling code seems to adhere to this. Docs and code need a review...

#2	chris 09:16 May 30, 2016

There may be other candidates too: Fl_File_Icon2.cxx line 358 Fl_x.cxx line 2748, line 2882

#3	AlbrechtS 13:20 May 30, 2016

Thanks again, both cases confirmed.

#4	AlbrechtS 17:25 Oct 01, 2016

Working on this STR now. I believe that I got everything mentioned in this STR, but this needs some testing. Please check and, if possible, test attached patch_v1.diff (apply with patch -p1). Did I get everything, or is there anything still missing? Help to test and to look for missing pieces would be very much appreciated.

#5	chris 23:18 Oct 01, 2016

The patch looks good for me, but I didn't test it (see note below). However I found two more files that maybe use ld() wrongly: - src/Fl_cocoa.mm (is this used?) #3794 - fluid/Fluid_Image.cxx #126 #128 Note: I tried to test with test/file_chooser, but I can't see any file icons displayed here with Ubuntu 14.04 (has nothing to do with your patch!). This is already so with FLTK 1.3.2. Looking into Fl::load_system_icons() in Fl_File_Chooser2.cxx, the logic loading of icons is hard to grasp, but when I comment out everything but the loading of default icons (lines #780-784) the icons are displayed with test/file_chooser.

#6	AlbrechtS 02:23 Oct 02, 2016

Thanks for testing and finding the other questionable issues. src/Fl_cocoa.mm is used for macOS (aka OS X). I can also test under Ubuntu 14.04, but this will take some time, please don't hold your breath. Thanks for your test description. Anyway, my spare time is currently rare, so any help in testing would be appreciated.

#7	manolo 04:33 Oct 04, 2016

The MacOS X-specific use of Fl_Image::ld() has been fixed at r.12008 and r.12009 for branches 1.3 and 1.3-porting.

#8	AlbrechtS 09:43 Oct 14, 2016

Fixed in Subversion repository. Fixed in branch-1.3 (svn r 12028) and branch-1.3-porting (r12029). @Manolo: thanks for fixing the MacOS part. Thanks also to Chris for your support. Regarding comment #5 (no display of icons): I can't confirm this on my system, and since this is OT in this STR, please feel free to file another STR if the problem persists. I consider this STR resolved unless someone finds bugs, and I'll close this STR in a few days or so.

#9	AlbrechtS 10:00 Oct 14, 2016

Notes (FTR): There was new code with issues as mentioned in comment #1, but there was also older code (e.g. in fluid) that didn't handle ld() correctly. However, the code in fluid was very likely only used with loaded image files, hence ld() should always be 0, and ld() = 0 works fine. The documentation of Fl_Image/Fl_RGB_Image specified that ld() was the size of extra data per line (aka padding), i.e. additional data after w()*d() bytes of image data. This turned out to be wrong: in Fl_Image and subclasses ld() must be 0 or >= w()*d(), otherwise some methods like copy(), desaturate(), etc. would potentially allocate arrays with negative sizes. w(), h(), and d() must also be > 0, with 1 <= d() <= 4 to avoid malfunctions in image copy() and other methods. [1] [2] The citation in comment #1 that states that d() and ld() can be negative is from the docs of fl_draw_image() and friends. These drawing functions are less restricted than Fl_Image and subclasses and do indeed allow negative values of d() and ld(). [1] In some cases negative d() and ld() values for Fl_(RGB_)Image's _may_ work, but this is not guaranteed for copy() and other mentioned functions. [2] I found some related STR's and commits: svn r5411, r5427, STR #1412: Fl_RGB_Image::copy not working for ld()!=w() svn r5825, STR #1673: [scaled?] Fl_RGB_Image::copy() broken with ld != 0 svn r8611, STR #2606: Fix for alpha blending under X11 when using the line data size

#10	chris 22:35 Oct 14, 2016

I opened http://www.fltk.org/str.php?L3341 for my note in #5.

#11	AlbrechtS 05:27 Oct 16, 2016

Thanks for opening the new STR. Closing this one now.