Problems with data_w,data_h, w, h of Fl_Image

[ZJUGKC]

data_w and data_h are added to Fl_Image from version 1.4. This causes some problems:

1. w() and h() may not equal to data_w() and data_h() after calling scale(), so only data_w() and data_h() would be used when accessing data() directly.
2. Three virtual functions, color_average, copy and desaturate will create new array and generate data from old array. So data_w() and data_h() should be used in these functions. But w() and h() are still used in these functions.
3. Derived classes, Fl_Pixmap, Fl_RGB_Image, Fl_SVG_Image, Fl_Tiled_Image, have above problems.

[ManoloFLTK]

@ZJUGKC : Good catch.
That should be fixed now in the github repo.
Do you see remaining problems ?

[Albrecht-S]

@ManoloFLTK Commit bfae813 breaks existing code.

I'm not sure what and how it can break code, but there's one specific thing I noticed: if you scale() an image and then copy(w, h) the image to another image, then the resulting image does not fulfill the expectations that w() == data_w() and h() == data_h(). This is a regression.

I'll post a test program that shows the issue soon.

[Albrecht-S]

Background: in FLTK 1.3 Fl_Image::copy(W, H) created an image img with img->w() == W and img->h() == H. This could be used to access the data() array in the requested size of the copied image. This is no longer true with the last commit if the image has been scaled before it is copied. The following output is from a demo program before the last commit:

Image tux1: original                    data = 0x55eb57241c10 (w, h) = (152, 180), (d, ld) = (  4,   0) (data_w, data_h) = (152, 180)
Image tux2: tux1->copy(), scale(W, H)   data = 0x55eb5725d290 (w, h) = (100, 100), (d, ld) = (  4,   0) (data_w, data_h) = (152, 180)
Image tux3: tux1->copy(W, H)            data = 0x55eb57277e20 (w, h) = (100, 100), (d, ld) = (  4,   0) (data_w, data_h) = (100, 100)
Image tux4: tux2->copy(W, H)            data = 0x55eb57281a70 (w, h) = (100, 100), (d, ld) = (  4,   0) (data_w, data_h) = (100, 100)

The last two lines show that w() == data_w() and h() == data_h() as expected (by me).
The following output is from the same demo program after the last commit:

Image tux1: original                    data = 0x55885dfa1c10 (w, h) = (152, 180), (d, ld) = (  4,   0) (data_w, data_h) = (152, 180)
Image tux2: tux1->copy(), scale(W, H)   data = 0x55885dfbd290 (w, h) = (100, 100), (d, ld) = (  4,   0) (data_w, data_h) = (152, 180)
Image tux3: tux1->copy(W, H)            data = 0x55885dfd7e20 (w, h) = (100, 100), (d, ld) = (  4,   0) (data_w, data_h) = (100, 100)
Image tux4: tux2->copy(W, H)            data = 0x55885dfe1a70 (w, h) = (100, 100), (d, ld) = (  4,   0) (data_w, data_h) = (152, 180)

The last line shows that w() != data_w() and h() != data_h() which is the mentioned regression.

More background: Fl_Image::copy(W, H) used to be a valid means to convert an image to another size and use its data() array to access the image data for instance to write a PNG file.

I used Fl_Image::copy(W, H) to get the data() array of the requested size of an image that (a) has another size and/or (b) has been scaled before to the correct size. This was my attempt (working but unfinished) to fix issue #205 and another issue where Fl_Window::cursor(Fl_RGB_Image, int, int) has a similar issue (doesn't work if the image has been scaled to the "correct" size).

What I described above can perhaps be resolved differently (how?) because it's internal stuff (caused by introducing Fl_Image::scale(), BTW) but the issue remains that user code might want to "convert" an image to another size to use its data array in that new size.

Demo program:
image-copy.cxx.txt

[ManoloFLTK]

None of this indicates a break of the API compatibility between FLTK 1.3 and 1.4, because Fl_Image::scale() appears with 1.4, so if an app uses only 1.3 means, Fl_Image::copy() produces in 1.4 the same results as in 1.3.

A consequence of using Fl_Image::scale() is that it's often but not always possible to ignore that images have 2 sizes
w()xh() and data_w()xdata_h() in 1.4 instead of one before. When an app wants to access the image data(), it must use the new "data" size, again if Fl_Image::scale() has been used.

It's true that the doc presently states "The values [w() and h()] are equal to data_w() and data_h() when the image is created". The recent commit makes Fl_Image::copy() violate this statement.

I see 2 ways out:

1. change in the doc the statement that the 2 sizes are always equal upon image creation adding they can dffer when an image was created by Fl_Image::copy() from a scaled image.
   or
2. change the code of Fl_Image::copy() (its overrides, really) to ensure its output is always unscaled. At present, the code sets the 2 sizes of the output image to the values they have in the source image, when Fl_Image::copy(W,H) is called with W==w() and H==h().

[ManoloFLTK]

Solution 2) above is now committed. It's good to have with Fl_Image::copy() a means to set an image data size.

[Albrecht-S]

@ManoloFLTK Thanks for the update, this looks much better now and works well for me.

FTR, you wrote:

None of this indicates a break of the API compatibility between FLTK 1.3 and 1.4, because Fl_Image::scale() appears with 1.4, so if an app uses only 1.3 means, Fl_Image::copy() produces in 1.4 the same results as in 1.3.

Yes, this is strictly correct and I was aware of this. However, users tend to "interpolate" and use both features in their programs. The icon issue discussed in another thread was such an issue:

• The user used Fl_Image::scale() and used the scaled image in an old "icon" API
• The same user may use Fl_Image::copy(W, H) in the expectation to use the data() array with W, H even for previously scaled images
• I used Fl_Image::copy() internally in the FLTK library with the same expectation.

Conclusion: in FLTK 1.4 we have an ambiguity in the general feature Fl_Image::copy() if the image has been scaled because we did not exactly specify what the copy should produce:

1. the original data size with an image that is scaled to the new size
2. the new data size (W, H) with unscaled (i.e. converted) image data [EDIT:] if it is different than (data_w, data_h)

I think we need to document (specify!) this not only for users but also for developers of future changes and I think point (2) above is the correct implementation (as you did already in the update).

[Albrecht-S]

@ZJUGKC Manolo asked in #427 (comment) whether you see remaining problems. From my side the issue can be closed (documentation questions are now handled in issue #431).

Do you agree? If yes, please close this issue. TIA.

[ZJUGKC]

There are some remaining problems:

1. Fl_Shared_Image.cxx, line 193-194, w() and h() will set w_, data_w_, h_, data_h to the same values as image_->w() and image->h(), I think image_->data_w_ and image_->data_h_ should be set additionally. Other methods calling Fl_Shared_Image::update() should be checked again.
2. In Fl_Shared_Image.cxx, line 474, I think it should be temp->data_w() == W and temp->data_h() == H.
3. In Fl_Tiled_Image.cxx, line 95 and 110, I think image_->copy(image_->data_w(), image_->data_h()) should be used because image_->copy() is equal to image_->copy(->w(), ->h()).
4. In Fl_BMP_Image.H, line 33, default value -1 will cause the bitmap data in memory cannot be loaded. The document in Fl_BMP_Image.cxx, line 82-84, should be modified.
5. In Fl_SVG_Image.cxx, line 178-179, calling w() and h() is not correct. The same reason is shown as (1).

[ManoloFLTK]

@Albrecht-S Following point 4. above, do you agree for me to modify the 3-argument Fl_BMP_Image constructor as follows :

Fl_BMP_Image::Fl_BMP_Image(const char *imagename, const unsigned char *data, const long length)
: Fl_RGB_Image(0,0,0)
{
  Fl_Image_Reader rdr;
  int retval = (length < 0 ? rdr.open(imagename, data) : rdr.open(imagename, data, (size_t)length));
  if (retval == -1) {
    ld(ERR_FILE_ACCESS);
  } else {
    load_bmp_(rdr);
  }
}

[Albrecht-S]

@ManoloFLTK Yes, I agree, this looks correct. Thanks.

[ManoloFLTK]

@ZJUGKC Many thanks for your extremely valuable input. I believe all "remaining problems" you pointed are now fixed in the git repo. Nevertheless, I don't agree with your point 5. because Fl_Image::copy(W, H) is documented to produce an image where w() == data_w() == W and h() == data_h() == H. I therefore did not change the source code of class Fl_SVG_Image.

Please, close this issue if you agree.

[ZJUGKC]

Thanks for fixing 3 points. But I still have problems about point 2 and point 5.

1 In Fl_Shared_Image.cxx, line 474,

if ((temp->w() != W || (temp->h() != H) && W && H ) {
...

the temp object using in if statement may be returned by find method or new operator. In latter case, there has temp->w_==temp->data_w_ and temp->h_==temp->data_h_, so temp->w() and temp->h() can be compared with W and H. But if temp is returned by method find (its bsearch calling using data_w and data_h) directly, now temp->w_==temp->data_w_ and temp->h_==temp->data_h_ may be not true. So I think it should be temp->data_w() == W and temp->data_h() == H.

2. In Fl_SVG_Image.cxx, line 178-179 :

} else if (copy_source) {
  w(copy_source->w());
  h(copy_source->h());
}
...

These statements are in mthod init_ which is called by constructor and copy construtor. In constructor Fl_SVG_Image::Fl_SVG_Image(...), a new internal object will be created and setting w_, h_, data_w, data_h to the same new size is OK. But in copy constructor Fl_SVG_Image::Fl_SVG_Image(const FL_SVG_Image* source), copy_source->w() == copy->source->data_w() and copy_source->h() == copy->source->data_h() may not be true. So calling w(..) and h(...) is not correct here.

[Albrecht-S]

@ZJUGKC Point 1: Thank you very much for investigating and reporting this. I believe what you found is a very good point but I don't know what solution would be the best. Basically we have a conflict between the "old" (1.3.x) Fl_Shared_Image handling where another image with a different size could only be created by a copy() operation (and didn't have data_w() and data_h() as distinct values) whereas FLTK 1.4 has the scale() method. I doubt that using the scale() method on Fl_Shared_Image is correct by any means because it leads to conflicts like those you found. This is something that really needs investigation.

@ZJUGKC, @ManoloFLTK: I believe that there are probably more issues WRT w() vs data_w() and h() vs data_h(). I want to investigate this further and I created issue #188 for this. This comment will automatically be linked to in #188 so we don't forget this aspect.

[ManoloFLTK]

The code at Fl_SVG_Image.cxx, line 178-179 is correct because it's used only by the private Fl_SVG_Image constructor with a const Fl_SVG_Image * argument. This constructor is itself only used by Fl_SVG_Image::copy(int W, int H) that finishes with svg2->w(W); svg2->h(H);. These 2 statements overwrite whatever values of w_, data_w_, h_, and data_h_ have been set by the private constructor, and set w_ == data_w_ and h_ == data_h_ for the resulting, copied SVG image.

[ZJUGKC]

@ManoloFLTK OK, you are right, this is not a problem. So the remaining problem is POINT 1 (Fl_Shared_Image).

[Albrecht-S]

@ZJUGKC @ManoloFLTK If everything but point 1 (the issue of Fl_Shared_Image) is resolved now, then I suggest to close this issue. The remaining Fl_Shared_Image issue has been referred to in issue #188 and will be resolved in this context.

Closing this issue will avoid having two issues open for "the same thing".

@ZJUGKC If you agree, please close.

[ManoloFLTK]

About the "remaining issue" in class Fl_Shared_Image :

I believe the current FLTK source code is correct for the reasons detailed below.

The instruction under discussion
if ((temp->w() != W || (temp->h() != H) && W && H ) {

corresponds to this part of the doc of the get() member function :

If the image exists, but only with another size, then a new copy with the
requested size (width \p W and height \p H) will be created as a resized
copy of the original image.

I believe the size mentionned in the doc is the drawing size, that is, the get() member function wants to produce a shared image whose drawing size is WxH. If the find() member function has found one with this drawing size, it's not necessary to do the copy() operation to produce a new shared image with the required drawing size : we already have it. Therefore, the instruction under discussion is correct to compare W and H with the drawing size (w() and h()) rather than with the data size (data_w() and data_h()).

That is visible, I believe, slightly modifying FLTK's test/pixmap_browser program as follows:
add this

  if (img2) {
    img2->scale(100,150,0,1);
    img2 = Fl_Shared_Image::get(n, 100, 150);
  }

after line 52 :
Fl_Shared_Image *img2 = Fl_Shared_Image::get(n);

and also comment out the call to the scale() member function below:
//img->scale(b->w(), b->h());

The modified code produces with the added scale() call a shared image with drawing size 100x150 and with data size whatever is the size of the read image file. The following get() call calls the instruction under discussion, and the test made is negative so the program flow skips the copy() call, because it's not needed. If the source code were modified as suggested by the OP, a copy of the image would be done and the copy would be memorized, although these tasks are not necessary.

[Albrecht-S]

@ManoloFLTK and others: please avoid using #NNN codes (hash + numeric values) in GitHub comments for anything else but issues and PR's because this creates "mention messages" in the issue or PR with number NNN (see above, line [#]52, edited by me). Unfortunately editing the message doesn't revert the "mention" entry. That's GitHub :-(

[Albrecht-S]

I believe the size mentionned in the doc is the drawing size, that is, the get() member function wants to produce a shared image whose drawing size is WxH.

Yes, that's basically true, but ...

@ManoloFLTK This documentation was written when only one size existed, and all Fl_Shared_Image's had to be copied (not scaled) to create another size. In current FLTK 1.4 this is ambiguous and that's why issue #188 exists. I doubt that the current implementation works as it should [1], and based on this, analyzing existing code is mute. But anyway, if your analysis is correct for whatever code exists now, then this is another reason to close this issue - and investigate this further when evaluating issue #188.

[1] I have some ideas what to test to verify or falsify my gut feeling but this would need more time than I can afford now. Sorry.

[ZJUGKC]

@ManoloFLTK If we consider W and H as drawing size, what you said is right. But many images need to be copied with different sizes.

This difficult position is caused by no following a principle of separation between data-storage and data-presentation. If a new drawing class containing w and h members is separated from Fl_Image which containing data_w and data_h, these issues can be solved gracefully.

OK, I agree to close this issue.

[Albrecht-S]

@ZJUGKC Thanks.