| | [ Return to Bugs & Features | Post Text | Post File | SVN ⇄ GIT | Prev | Next ]
STR #2583
| Application: | FLTK Library |
|---|
| Status: | 5 - New |
|---|
| Priority: | 1 - Request for Enhancement, e.g. asking for a feature |
|---|
| Scope: | 3 - Applies to all machines and operating systems |
|---|
| Subsystem: | Core Library |
|---|
| Summary: | transparency for fl_draw_image() |
|---|
| Version: | 1.4-feature |
|---|
| Created By: | ossman |
|---|
| Assigned To: | Unassigned |
|---|
| Fix Version: | Unassigned |
|---|
| Update Notification: | |
|---|
Trouble Report Files:
[ Post File ]
Trouble Report Comments:
[ Post Text ]
| |
| #1 | ossman
04:10 Mar 02, 2011 |
| I've unfortunately hit the problem that fl_draw_image() on OS X behaves very different from X11 or Windows. On the latter two the data is assumed to be RGB (as documented) and the delta argument is just a memory layout issue. On OS X though, it is assumed that the data is RGBA if the delta is 4. This means that code that works fine on X11 and Windows breaks in fun and intersting ways on OS X.
I suggest changing the OS X code so that it is always assumed that there is no alpha channel. This is how the other backends behave and is also how fl_draw_image() is documented. The only place that mentions alpha is Fl_RGB_Image, and that gives you consistent alpha behaviour for all backends. | |
| |
| #2 | AlbrechtS
05:57 Mar 02, 2011 |
| Yes, this is a known issue of different behavior on different platforms in FLTK 1.3. I remember that Matt added the alpha channel support on Mac OS only, with a comment like "I hope this doesn't break anything".
A workaround is to use Fl_RGB_Image for drawing images with alpha channel, because this works on all platforms. I used this in the demo program test/unittest_images.cxx to make it platform-independent (intentionally showing the difference, and I also added a comment).
I really don't know what the best way is: add alpha channel support on all platforms to fl_draw_image(), or remove it on Mac OS, as you suggest. At least it should be documented correctly, but for the future I'd like to have alpha support in fl_draw_image() on all platforms.
Thanks for the heads-up ! | |
| |
| #3 | AlbrechtS
06:01 Mar 02, 2011 |
| Amending my previous message: do you mean that you have image data with d=4, but with RGB data only (w/o alpha channel?). What is the contents of the 4th byte? Random data, or a specific value? | |
| |
| #4 | ossman
07:31 Mar 02, 2011 |
| It is random data, so I'd really like to have to option to disable alpha processing. It is evidently possible on all three platforms. | |
| |
| #5 | AlbrechtS
09:33 Mar 02, 2011 |
| Yes, it is possible to do it in either way on all platforms, but the code is (... well ... let's say ...) grown over a long time, and there are different strategies (subfunctions) on some platforms. IMHO this needs some redesign to be manageable again... (I took a look at it when I fixed a bug)
That said, maybe one more (optional) function argument (say: int alpha) might help to switch alpha support on or off, and leave d to only specify the memory layout (as you suggested). However, that would then be different than in Fl_RGB_Image, where d *also* decides whether to use alpha or not.
Opinions and ideas welcome... | |
| |
| #6 | manolo
10:34 Mar 02, 2011 |
| This STR shows us that situations arise where
an image is made of series of 4 bytes, only 3 of which have
meaningful (RGB) values. Thus, the present semantic of
fl_draw_image() on the Mac platform where the 4th byte, if present,
must be an alpha channel is not adequate.
My suggestion would be:
1) unify the fl_draw_image() semantic on all platforms to ignore
the 4th byte if it exists;
2) reserve a future API that would allow to draw RGBA images
with this function:
void fl_draw_image(uchar*, X,Y,W,H, D, L, bool alpha=false);
where transparency would require an explicit true last argument.
The code change, in fl_draw_image_mac.cxx, is just to replace:
//lut, delta&1?kCGImageAlphaNone:kCGImageAlphaNoneSkipLast,
lut, delta&1?kCGImageAlphaNone:kCGImageAlphaLast,
by:
lut, delta&1?kCGImageAlphaNone:kCGImageAlphaNoneSkipLast,
//lut, delta&1?kCGImageAlphaNone:kCGImageAlphaLast, | |
| |
| #7 | ossman
00:51 Mar 03, 2011 |
| I agree with Manolo. FLTK is a platform abstraction layer. Therefore the most important thing should be consistent behaviour across platforms (to the extent possible).
Removing the alpha support from Mac is a lot easier than adding it to the others, so it should be the preferred choice. There is also the fact that Fl_RGB_Image handles alpha, meaning that fl_draw_image() probably shouldn't so that you have the choice. If both mandate alpha then you have to add a lot of wasteful CPU cycles making sure your input data has 0xff in the fourth byte.
I can verify that the patch solves the issue here.
(Nitpick: kCGImageAlphaNone and kCGImageAlphaNoneSkipLast mean the same thing, so the test there could be removed. :)) | |
| |
| #8 | AlbrechtS
02:39 Mar 03, 2011 |
| Agreed.
+1 for removing alpha channel support from fl_draw_image() on Mac OS in favor of an additional function parameter to enable the alpha channel explicitly (as suggested above).
I propose that we use an additional parameter (enum [image_]data_format) for future expansion, with the following values as a start:
FL_IMAGE_DATA_RGB = 0
FL_IMAGE_DATA_RGBA = 1
(the names are subject for discussion).
The documentation should state that all other values lead to undefined behavior and are reserved for future expansion (e.g. FL_IMAGE_DATA_ABGR or whatever, maybe also other bit sizes).
Whatever we do now, we *should* re-enable the alpha channel for Mac OS by using this parameter accordingly. This ought to be acceptable since the old version has never been released officially.
Then we *may* also add the alpha channel support for all other platforms *or* document this as a /current/ restriction.
Opinions and votes, please... | |
| |
| #9 | manolo
03:51 Mar 03, 2011 |
| +1 for removing alpha channel support from fl_draw_image() on Mac OS
+1 also for declaring
fl_draw_image(const uchar *buf,int X,int Y,int W,int H,int D,int L,
enum image_data_format f = 0)
and stating that f=FL_IMAGE_DATA_RGBA is currently implemented
only on the Mac platform. | |
| |
| #10 | ossman
03:51 Mar 09, 2011 |
| Do we have a consensus? Anyone implementing this? :) | |
| |
| #11 | AlbrechtS
08:44 Mar 09, 2011 |
| Yes, I think we have consensus, with the additional optional argument (enum, as proposed). Nobody objected during the last 6 days.
Manolo, if you like, please go ahead and do the Mac part, or propose a patch, or... . Otherwise I can maybe find a few free hours next Friday or Sunday.
I can probably take a look and add alpha support to Windows, and maybe also X11. | |
| |
| #12 | ossman
23:55 Mar 09, 2011 |
| I have one semi-related issue that you might want to consider whilst looking at this. If we get the option of different pixel formats, FLTK should be able to inform the application of the most efficient one. That way we can avoid having a pointless conversion stage when not needed.
Right now for my application, it gets converted from BGRx to RGBx (what FLTK requires) and then back to BGRx (which the X server wants). One big massive waste of CPU cycles there. :) | |
| |
| #13 | manolo
12:14 Mar 10, 2011 |
| The attached fl_draw_image.patch is proposed to add alpha channel
optional support under Mac OS. I have defined another enum value,
FL_IMAGE_DATA_LA, when the image is luminance + alpha, because
using the name FL_IMAGE_DATA_RGBA for this case would be
misleading.
With this patch, the undocumented FL_IMAGE_WITH_ALPHA that was
used to support Fl_RGB_Image::draw() under MSWindows is removed from
file FL/Enumerations.H
Functions fl_draw_image{_mono}(Fl_Draw_Image_Cb,...) under WIN32
were programmed to accept the FL_IMAGE_WITH_ALPHA flag.
They are never called with this flag in the present code base.
Thus, the patch removes support for the alpha flag for these
functions with the assumption that no user application would use
this undocumentated flag. | |
| |
| #14 | AlbrechtS
07:25 Mar 11, 2011 |
| Manolo, thanks for the patch, this is a good starting point, but IMO there are a few points that don't work as they should.
(1) We can't introduce FL_IMAGE_DATA_LA and distinguish it from FL_IMAGE_DATA_RGBA, because this is very likely to break existing code. We have only one /default/ value that we can set for the optional format value. I also didn't anticipate this problem, but it is real. So we must have some flag for *automatic* detection, depending on the d() or delta value, as it was before. Thus, I propose to change the enum to have:
FL_IMAGE_DATA_AUTO = 0 // automatic, depending on d, w/o alpha
FL_IMAGE_DATA_ALPHA = 1 // automatic, depending on d, with alpha
The default is 0, which means:
1 <= abs(d) <= 2 = gray scale (luminance) w/o alpha
abs(d) > 2 = RGB (w/o alpha)
Then we can add:
FL_IMAGE_DATA_L = 2 // luminance (gray scale)
FL_IMAGE_DATA_LA = 3 // luminance (gray scale) with alpha
FL_IMAGE_RGB = 4 // RGB
FL_IMAGE_DATA_RGBA = 5 // RGBA (RGB with alpha)
We should interpret the enum as a bit mask, with the following
encoding:
Bit 0 (0x0001) = alpha channel (can be combined with others)
Bit 1 (0x0002) = gray only (luminance) => L (or LA)
Bit 2 (0x0004) = RGB or RGBA (with alpha)
later, e.g.
Bit 3 (0x0008) = BGR (inv. colors) or ABGR (with alpha)
... or even other encodings, such as 16-bit color channels.
Hence, FL_IMAGE_DATA_RGBA == FL_IMAGE_DATA_RGB | FL_IMAGE_DATA_ALPHA
etc.
Second point: fl_draw_image_mono() should IMHO *not* have the optional format argument. It is designed and documented so that you can pick one of n channels out of, for instance, RGB(A) data and draw this as a gray scale image. This wouldn't work with an alpha channel at a different offset. Thus, fl_draw_image_mono() should simply call fl_draw_image() with format=FL_IMAGE_DATA_L (explicitly), and this should do it. Or am I missing a point here?
I assume that fl_draw_image_mono() was only needed because of the auto-detection of color formats implied by the old function. Thus, we should deprecate fl_draw_image_mono() once the new implementation is complete, in favor of using the additional argument.
[OT: another useful optional argument for fl_draw_image_mono() could maybe be a color value, either R, G, or B, so that one could draw the R channel in red, the G channel in green, etc.]
Third point (just a note): FL_IMAGE_WITH_ALPHA *has* been used in src/Fl_Image.cxx at line #503 (in the "present code base", before your patch), but this can surely be replaced.
Last point: the fourth image (Gray+Alpha) in test/unittests doesn't draw correctly on Windows, but this is probably a side effect of the different data format settings. This is probably moot, when the data format enum is changed, because the function must be called differently anyway.
I'm going to modify the patch and propose an update, but this will probably not be before Sunday afternoon.
Meanwhile, all comments and suggestions would be welcome. Thanks. | |
| |
| #15 | manolo
05:52 Mar 12, 2011 |
| > Second point: fl_draw_image_mono() should IMHO *not* have the optional > format argument. It is designed and documented so that you can pick
> one of n channels out of, for instance, RGB(A) data and draw this as
> gray scale image. This wouldn't work with an alpha channel at a
> different offset. Thus, fl_draw_image_mono() should simply call
> fl_draw_image() with format=FL_IMAGE_DATA_L (explicitly), and this
> should do it. Or am I missing a point here?
I had the impression that this optional format argument was necessary
to support what is seen in unittests_images.cxx where
fl_draw_image_mono() is called uder Mac OS to draw a transparent
monochrome image. | |
| |
| #16 | AlbrechtS
07:46 Mar 14, 2011 |
| Currently the __APPLE__ code in unittest_images.cxx calls fl_draw_image() for all image types, because it just works. This can be replaced by fl_draw_image(..., format) with format = { FL_IMAGE_DATA_RGBA | FL_IMAGE_DATA_LA }, resp. for those calls (image formats) that have an alpha channel, whereas the formats w/o alpha can call the old version w/o format for compatibility reasons. Eventually this should work for all platforms...
fl_draw_image_mono() was only necessary if delta>2, but was not defined to use the alpha channel. This can always be replaced with an appropriate value of the format argument (once everything is done).
FYI: I have a working Windows version now, but this needs some more cleanup and maybe a little redesign. I also believe that I found some incompatibilities in the printing device versions. This needs more investigation. I'll post my patch ASAP.
BTW: I agree that the undocumented flag FL_IMAGE_WITH_ALPHA should be removed, and I hope that this doesn't break existing code. At least there will be another way to achieve the same effect with the format argument. | |
| |
| #17 | ossman
05:35 Apr 11, 2011 |
| Any updates to this issue? It would be nice to get committed as things won't work for me on Mac without this. | |
| |
| #18 | manolo
13:51 Apr 11, 2011 |
| I believe I could commit the change that makes the Mac platform
behave as others, that is, fl_draw_image() does not expect
alpha data, even if d is 4.
The implementation of transparency would come later, when time permits.
Would that be useful for you, Evan ? | |
| |
| #19 | manolo
13:53 Apr 11, 2011 |
| Sorry, I meant Ossman. | |
| |
| #20 | AlbrechtS
01:34 Apr 12, 2011 |
| Manolo, I'd appreciate if you committed the Mac part (removing alpha channel from the default call), so that we have consistency across platforms.
I tried to implement transparency on Windows, but it's more complicated than I thought (Fl_RGB_Image caches some bitmaps that wouldn't be available for fl_draw_image()), and I got stuck. I won't have much time this week and maybe the next one, too, thus I think that it would be better to do one step now and add the optional argument later. | |
| |
| #21 | manolo
05:06 Apr 12, 2011 |
| Committed in r.8581.
Now, fl_draw_image() ignores any 4th pixel byte on all 3 platforms. | |
| |
| #22 | manolo
05:22 Apr 14, 2011 |
| The initial scope of this STR was inconsistency between the Mac and
the other platforms. This is now fixed.
This STR has then become RFE-like with the project to add support
for transparency to the fl_draw_image() function.
It has appeared that this is not a simple change.
My suggestion is now to close this STR, and create a new RFE for 1.4,
or to reclassify it as an RFE, so it does not prevent 1.3 to be released.
Albrecht: what do you think ? | |
| |
| #23 | ossman
04:01 Apr 15, 2011 |
| The initial bug is now properly fixed in todays snapshot. Many thanks. :) | |
| |
| #24 | manolo
02:57 Apr 17, 2011 |
| Reclassified as an RFE because the initial Mac OS inconsistency
has been fixed, as noted by the OP.
The enhancement would be to add transparency support to the
fl_draw_image() function on all platforms. | |
[ Return to Bugs & Features | Post Text | Post File ]
|
| |
