EtheaDev / SVGIconImageList

Three engines to render SVG (Delphi Image32, Skia4Delphi, Direct2D wrapper) and four components to simplify use of SVG images (resize, fixedcolor, grayscale...)
Apache License 2.0
327 stars 96 forks source link

Blueish tint on Android and MacOS-X #229

Closed birbilis closed 2 years ago

birbilis commented 2 years ago

I see blueish tint on Android when using the latest TSVGIconImageList from GetIt Package Manager

Windows: TortoiseAndTheHare_EditMode_StructureView en

Android: Tortoise and the Hare en_StructureView_EditMode-Samsung Galaxy Tab S8-12 0

The app is at https://github.com/zoomicon/READCOM_App

Apart from using TSVGIconImage for the SVG images in the stories, I use TSVGIconImageList for the app icons. All those are tinted. However, stories that contain bitmap images show fine, so it's something with the SVG rendering

I think it uses Image32 for rendering now (judging from a little debugging). Is there any known issue with that? How can I tell your library to use Skia SVG renderer instead? (or is there other engine alternative on Android too?) Do I need to install Skia4Delphi too and right click my project and enabled Skia (to added needed lib to deployment)?

UPDATE: the same tint appears on MacOS-X (those are supposed to be English/Greek/Italian/Portuguese/Spanish flags in the image) image

Is it something related to RGB versus BGR? Is Image32 also being used by default on OS-X?

birbilis commented 2 years ago

Note that bitmap images (rendered via FMX) look fine image

carloBarazzetta commented 2 years ago

Please try to restore previous version of Img32.SVG.Reader that contains:

    if (fillColor = clCurrent) then
      {$IF Defined(MACOS) or Defined(MACOSX)}
      drawDat.fillColor := SwapRedBlue(thisElement.fReader.currentColor)
      {$ELSE}
      drawDat.fillColor := thisElement.fReader.currentColor
      {$IFEND}
    else if (fillColor <> clInvalid) then
      {$IF Defined(MACOS) or Defined(MACOSX)}
      drawDat.fillColor := SwapRedBlue(fillColor);
      {$ELSE}
      drawDat.fillColor := fillColor;
      {$IFEND}
    if fillOpacity <> InvalidD then
      drawDat.fillOpacity := fillOpacity;
    if (fillEl <> '') then
      drawDat.fillEl := fillEl;
    if (strokeColor = clCurrent) then
      {$IF Defined(MACOS) or Defined(MACOSX)}
      drawDat.strokeColor := SwapRedBlue(thisElement.fReader.currentColor)
      {$ELSE}
      drawDat.strokeColor := thisElement.fReader.currentColor
      {$IFEND}
    else if strokeColor <> clInvalid then
      {$IF Defined(MACOS) or Defined(MACOSX)}
      drawDat.strokeColor := SwapRedBlue(strokeColor);
      {$ELSE}
      drawDat.strokeColor := strokeColor;
      {$IFEND}

because it was lost during update to Image32 4.11 version. Then give me a feed-back if it works, so i can restore this fix and notify Angus Johnson, the author of Image32. Thanks.

birbilis commented 2 years ago

problem is I'm using the GetIt version (not sure if GetIt allows one to rollback to previous versions). Can I patch it by including my copy of that unit (I can get it from their version control) in the uses clause of the app? I haven't tried building your library, was afraid I'd lose time trying to pull in all dependencies

birbilis commented 2 years ago

another problem I see is that this code (which looks promising btw) check if it's targetting MACOS-X, but I don't see Android or iOS mentioned (at Android it's the same issue and I suspect it's on iOS too).

carloBarazzetta commented 2 years ago

Unfortunately, I don't have mobile devices on which to test. I advise you to install the library by cloning the repo from Git: the installation is simple, just open the package group do the build / install of the packages, as explained here: Now you can develop by compiling the sources directly and trying to make changes to fix the color problem on the different devices.

There is another place to adjust color that affect also ANDROID platform, into FMX.Image32SVG:

function AlphaToColor32(AlphaColor: TAlphaColor): TColor32;
var
  res: TARGB;
begin
  res.A := TAlphaColorRec(AlphaColor).A;
  res.R := TAlphaColorRec(AlphaColor).R;
  res.G := TAlphaColorRec(AlphaColor).G;
  res.B := TAlphaColorRec(AlphaColor).B;
  Result := res.Color;
{$IF Defined(ANDROID) or Defined(MACOS) or Defined(MACOSX)}
  Result := SwapRedBlue(Result);
{$IFEND}
end;
birbilis commented 2 years ago

thanks, will take a look

btw, an easy way to test is to upload an APK at https://www.browserstack.com/android-testing (note one needs to create a certificate from inside Delphi to sign the resulting .apk, else BrowserStack won't install it)

birbilis commented 2 years ago

Looking at that code snippets you shared, it seems they were loading the RGB as BGR (but only on OS-X) and then they were swapping again back to RGB or something (this time both on Android and OS-X).

So their original code (without the code that was removed from the SVG loader) would work on OS-X (had double inversion that was self-canceling), but on Android I guess. Now that they removed that they have it broken both on OS-X and Android (then probably on iOS it may work, will check on the iOS emulator)

birbilis commented 2 years ago

Started looking at it, one quick comment I have is that I feel the .groupproj should contain the demo projects too for convenience (so that one can easily test a change they did), or could add extra groupproj with everything (packages and demos), since the existing is called "SVGIconImageListGroupPackages". Not sure if GetIt needs that groupproj to only contain the packages or something, plus hope if extra is added with everything that GetIt isn't confused (e.g. losing time to build all demos or something)

birbilis commented 2 years ago

I can confirm that the issue occurs on Android with the SVGIconImageListDemoFMX project too (here I project via Miracast [wifi] from Android to Windows 11):

image

(I also notice some layout glitch at the FixedColor checkbox and the color dropdown [and its dropdown arrow] under it)

To build/run on Android, I had to right click the Libraries not and select "Revert System Files to Default":

image

birbilis commented 2 years ago

the issue isn't probably with loading the SVGs, but with rendering them, since if I play with the Fixed Color option at that demo, I see other color result (though I'm not sure how the Fixed Color functionality is implemented [if it "reloads" the SVG from a modified source or if it changes it in memory without invoking an SVG reader/parser again]):

image

birbilis commented 2 years ago

making AlphaToColor32 not call SwapRedBlue didn't work, but making SwapRedBlue not do anything (comment out the swapping commands) did work:

function SwapRedBlue(color: TColor32): TColor32;
var
  c: array[0..3] of byte absolute color;
  r: array[0..3] of byte absolute Result;
begin
  result := color;
  //r[0] := c[2];
  //r[2] := c[0];
end;

probably though that method is needed to be there (don't want to make it do nothing), so will look into it a bit more to see who calls it

birbilis commented 2 years ago

searching for the symbol with find in files gives:

opening one by one seems actual callers are these methods:

fixing the last one also does the trick (instead of making SwapRedBlue do nothing), but it has a conditional check for Android only, so have to check why MacOS-X also has the issue and if this fixes it for it too, or it needs fix also at other place

carloBarazzetta commented 2 years ago

I forwarded the issue to Angus Johnson, author of Image32...

birbilis commented 2 years ago

thanks, will post here any more findings since I'll try on OS-X too later on today

birbilis commented 2 years ago

note that just adding to SVGIconImageListDemoFMX project the patched Img32.SVG.Core.pas (with commented out SwapRedBlue call at line 1191) does the trick on Android, even with the GetIt version of SVGIconImageList

{$IFDEF ANDROID}
//color := SwapRedBlue(color); //TODO: this fixes Blueish SVG color tint (RGB-BGR substitution) on Android, but should try to fix OS-X too (https://github.com/EtheaDev/SVGIconImageList/issues/229) - haven't tried iOS yet
{$ENDIF}
AngusJohnson commented 2 years ago

forwarded the issue to Angus Johnson, author of Image32...

Thanks Carlo. I'm not doubting there's a problem, but I can't duplicate it, at least when just using Image32. I've attached a very simple demo FMX app that just uses Image32. And here are the Windows and Android screenshots:

fmx_svg_win

fmx_svg_android

fmx_svg_demo.zip

Edit: tidied up demo.

AngusJohnson commented 2 years ago

I'm now wondering if this problem is due to a conflict between Image32 and Carlo's FMX.Image32SVG unit.

As you've observed above, Image32 swaps the red and blue channels (when the precompiler directive ANDROID is defined). However, I note Carlo's AlphaToColor32 function (line 72) calls TAlphaColorRec which also switches these color channels depending on endianness. IOWs, I suspect the blue and red channels are being swapped twice.

Edit: Unrelated to the above problem, I can see it's much better to manage red-blue color swaps based on endianness rather than platform, so I'll need to change this in Image32.

Edit2: Scrap all that. The red-blue issue is unrelated to endianness.

birbilis commented 2 years ago

Hi, if you don't see the issue in Image32 demos (didn't notice any in the version included in SVGIconImageList repo), but only happening in SVGIconImageList FMX demos (and in my app that uses that library), then it's definitely something added there or some difference in the files they have.

So, since there was another suspisious thing in that code, I think I found it.

My original fix showed ok: image

but not the FixedColor functionality of that demo, it was broken: image

so I went back to the code, first of all had checked the method you had mentioned and though it was being called a lot in the debugger (had breakpoint to confirm I had overriden it with my added changed unit in the demo project), it wasn't affecting the tint issue (whether I had my other fix added or not, it didn't affect something [apart maybe from the FixedColor I mention, didn't try that with it])

the correct (that solves both the tint and the FixedColor functionality) fix is here: image at the FMX.Image32SVG file of SVGIconImage repo

birbilis commented 2 years ago

to recap, ignore my originally proposed fix, the issue is in FMX.Image32SVG @carloBarazzetta please do this fix, tried on both Windows and Android (the R-B color channel reversal goes away and the FixedColor switch works fine too in both) - UPDATE: added pull request here: https://github.com/EtheaDev/SVGIconImageList/pull/231

Since I had observed the issue on OS-X, added OS-X too. However I have some issue with that demo to add OS-X target, fails to compile due to some file rights (can't write to the bin folder etc.). On my READCOM_App project OS-X target builds fine, so will check there too that the fix works on OS-X and let you know

Will also try on iOS Simulator soon (I hope) to see if the issue was ever there for that platform too

birbilis commented 2 years ago

since it's a major bug if you could release on GetIt a new version it would be great (just wait for me to check on iOS too)

AngusJohnson commented 2 years ago

image

Yes, that may be useful in Image32 too.

birbilis commented 2 years ago

I confirmed this fix is fine for my app on Android, but on Mac OS-X it isn't doing the job (I tried both RGBA and BGRA there with no difference which is strange)

The reason might be that some other code that also does reversals and has only check for Android, not Mac OS-X (or vice-versa). In fact there's a chance there are too many reversals happening that could be avoided.

PAServer is quite slow to deploy to remote Mac, so not easy to try alternatives (takes some time)

Plus had seen some Image32 code that was getting the TPixelFormat from BitmapSurface, not sure what that was about, but maybe one could avoid conditional defines and just get the PixelFormat from the device canvas (though I'm not sure if that is accessible early in the app startup)? e.g. would Android on Intel platforms be using other pixel format that Android on ARM? (not that Delphi can target Android on Intel currently, but Windows is moving onto ARM too so we might have issues with that too in the future if MS doesn't handle such things under the hood)

Another thing is that I don't see MACOSX symbol at https://docwiki.embarcadero.com/RADStudio/Alexandria/en/Conditional_compilation_(Delphi) is it for FreePascal or something? I only see a newer one called OSX, but I guess best is to use older MACOS one (and just that if only targetting Delphi). Just in case I tried the fix with OSX symbol (my taget is Mac 64-bit [Intel CPU, an older iMac]), does the same

birbilis commented 2 years ago

Just remembered at UTF8StringToColor32 at Img32.SVG.Core.pas(1191) it was only reversing for Android, probably it needed to reverse for OS-X too so that the Copy Image32 to Bitmap other fix will then work. Fingers crossed, will try

birbilis commented 2 years ago

Indeed, it worked with this extra fix (this time at Image32's Img32.SVG.Core), aka to have near the end of UTF8StringToColor32 do color reversal for both Android and MacOS-X

an issue is that Image32 was using {$IFDEF ANDROID}...{$ENDIF} instead of {$IF Defined(Android)}...{$IFEND} (note the IFEND btw)

I used this but not sure if it compiles on FreePascal/Lazarus, plus see my question on what MACOSX symbol is (that Carlo uses in his code): image

birbilis commented 2 years ago

[DONE] will update the pull request if still open to have the fixes for both Android and OS-X or else make new one

birbilis commented 2 years ago

btw, regarding the use of IFEND, it's required by older compilers if one use $IF (newers accept $ENDIF from what I see). If compiler has issues it's mentioned that one can add {$IF CompilerVersion >= 24.0 } {$LEGACYIFEND ON} {$IFEND} https://docwiki.embarcadero.com/RADStudio/Alexandria/en/Legacy_IFEND_(Delphi) https://stackoverflow.com/a/38052789/903783

to be consistent with existing code in SVGIconImageList I'll use {$IFEND} at the fix I had done in FMX.Image32SVG too

birbilis commented 2 years ago

also note I tested on "Android on ARM" and "MacOS-X on Intel" targets

AngusJohnson commented 2 years ago

an issue is that Image32 was using {$IFDEF ANDROID}...{$ENDIF} instead of {$IF Defined(Android)}...{$IFEND} (note the IFEND btw)

I used this but not sure if it compiles on FreePascal/Lazarus, plus see my question on what MACOSX symbol is (that Carlo uses in his code):

I've just fixed this but now I'm having problems uploading to Sourceforge.

AngusJohnson commented 2 years ago

I'm still having problems with SourceForge and have decided to move the repository over to GitHub. (This was something I'd been considering for a while but with the issues I'm currently having with SF has prompted me to move to GitHub immediately. Besides, I have other repositories there.) Anyhow, Image32's updated code can be found here.

birbilis commented 2 years ago

will probably need to use that LEGACYIFEND code block I mentioned above to stop emitting warning for the use of the legacy IFEND (unless one doesn't care about older compilers that needed $IFEND to match $IF), else will get:

birbilis commented 2 years ago

Speaking of older compilers (or FreePascal/Lazarus), I'm not even sure the "Defined(symbol)" is supported (to use with IF) and if they need IFDEF instead (the problem with IFDEF is you can't do OR/AND if I'm not mistaken, can only check one symbol - alternative most probably is for one to use nested IFNDEFs and reverse logic, but not always easy to do)

AngusJohnson commented 2 years ago

Speaking of older compilers (or FreePascal/Lazarus), I'm not even sure the "Defined(symbol)" is supported

I've tested ... {$IF DEFINED(ANDROID) OR DEFINED(MACOS) OR DEFINED(MACOSX)} {$IFEND} ... and this and it's OK in Delphi 7.

birbilis commented 2 years ago

nice, I also see at https://wiki.freepascal.org/$IF that FreePascal supports "$IF Defined

//But with the {$IF} you can check conditions together: {$if defined(SOMETHING) and defined(SOMETHINGELSE)}//simple and readabl instead of union {$IFDef}`s

also at https://wiki.freepascal.org/$IF/fr one sees $IFEND is supported (apart from $ENDIF that is used in the English page's example)

so if Delphi 7 supports matching an $IF with an $ENDIF guess one could use $ENDIF instead of $IFEND (and if you don't care for earlier Delphi). Can you check?

else will have to keep $IFEND (to match the $IF) and add the {$IF CompilerVersion >= 24.0 } {$LEGACYIFEND ON} {$IFEND}

to avoid Delphi 11 warnings about legacy $IFEND

judging though from that snippet (from https://docwiki.embarcadero.com/RADStudio/Alexandria/en/Legacy_IFEND_(Delphi)) checking for ver24 of the compiler and that being "Delphi XE3 / C++Builder XE3" at https://docwiki.embarcadero.com/RADStudio/Sydney/en/Compiler_Versions there's high chance Delphi 7 (if that's your min support target) needs the $IFEND

birbilis commented 2 years ago

(no need to check, I see "At the XE4 release, the Delphi compilers were changed to accept either $IFEND or $ENDIF to close $IF statements. Before XE4, only $IFEND could be used to close $IF statements." in the doc page on $LEGACYIFEND - so need to have that extra snippet, will add to my pull request (https://github.com/EtheaDev/SVGIconImageList/pull/231) since it's still open.

Not sure if one needs to have it per file, I suppose so. Else when it says it restores old behaviour to require $IFEND would mean it causing issues to other files in same project if one added the .pas files to their project directly

AngusJohnson commented 2 years ago

else will have to keep $IFEND (to match the $IF) and add the {$IF CompilerVersion >= 24.0 } {$LEGACYIFEND ON} {$IFEND}

I'm pretty sure this will be necessary.

birbilis commented 2 years ago

thing is I was seeing the warning but not seeing when I try to clean/rebuild. Not sure if only shown when one mixes $IF/$ENDIF and $IF/$IFEND in the same file

However I do see that SVGInterfaces.pas needs to be consistent too and use $IFEND to match $IF (uses $ENDIF). Speaking of consistency it also seems to mix $IF Defined and $IFDEF in that file

image

birbilis commented 2 years ago

@AngusJohnson please see https://github.com/EtheaDev/SVGIconImageList/issues/233

not sure why, but I had to undo the fix in Img32.SVG.Core.pas (aka only swap for Android, not OS-X) since blue tint reappeared on OS-X (and not Android this time) when trying latest SVGIconImageList.

I kept the RGBA that I had introduced for OS-X and Android at TFmxImage32SVG.PaintToBitmap (file FMX.Image32SVG.pas of SVGIconImageList repo) but also had to remove from AlphaToColor32 at same file the swap for OS-X this time (that was there from before)

I wonder if your comment at https://github.com/EtheaDev/SVGIconImageList/issues/229#issuecomment-1137069733 is related, aka if you added some RGBA similar logic somewhere in Image32 that caused this regression

as I said before, probably someone should look into those byte swaps and if they're actually needed or done multiple times (and just undo their actions)

I tried various alternatives again this time like not using RGBA but stick to BGRA that was originally used at that PaintToBitmap, but didn't help

AngusJohnson commented 2 years ago

I tried various alternatives again this time like not using RGBA but stick to BGRA that was originally used at that PaintToBitmap, but didn't help

I'm afraid I don't have any Mac devices to do testing myself so I'm dependent on feedback from those who do which obviously makes debugging more challenging (and much slower).

Anyhow, when confronted with these sorts of bugs, my first inclination is to identify the components and test each separately. So here I suggest testing Image32 independant of SVGIconImageList. To do that could you compile and run this simple demo (that I also linked to way above). If the color channels are reversed in the demo (as I suspect) then we've considerably narrowed down where to look for this bug. Cheers A.

carloBarazzetta commented 2 years ago

@birbilis, can you test the Image32 simple demo to understand at what level the error can be found? The pull request you asked me to approve, should it solve the problem? Thank you very much!

birbilis commented 2 years ago

yes it solves it, but just in case let me first check that Image32 standalone demo

birbilis commented 2 years ago

not sure you need to reopen this issue - I had added separate issue since the regression was for OS-X only: https://github.com/EtheaDev/SVGIconImageList/issues/233 (I have only screenshots in that issue after the new fix was tested in both Android and OS-X)

birbilis commented 2 years ago

Seems that demo zip is a subset (just some missing images) of Examples/FMX3 in Image32 distribution, so will look into that one with Android and OS-X

As a first step I did some fixes to Image32 (@AngusJohnson can see https://github.com/AngusJohnson/Image32/pull/3 )

Now I'll try adding more targets to the projects that are FMX based and check esp. the FMX3 one on Android and OS-X

Note I had to remove SVGIconImageList from GetIt to be able to build and install the design package of Image32 (since SVGIconImageList seems to register its own one for Image32)

birbilis commented 2 years ago

indeed, the sample project with latest Image32 has blueish tint on OS-X, removing the swap again for OS-X at UTF8StringToColor32 (probably something else was introduced that made it not needed anymore) fixes the issue as shown at https://github.com/AngusJohnson/Image32/issues/4

birbilis commented 2 years ago

of course for latest SVGIconImageList the rest of the changes I did at https://github.com/EtheaDev/SVGIconImageList/commit/853f5f6da35217335322196352dbcdfbad5bdffe (for https://github.com/EtheaDev/SVGIconImageList/issues/233) are needed too (think they were already pulled in)

birbilis commented 2 years ago

seems Image32 was also broken for Android regarding that tint. To fix it I just did https://github.com/AngusJohnson/Image32/commit/a91336d50962f058ee17940e7b5008974b39ac3e so taking this in mind I will need to do a new PR for SVGIconImageList too. My guess is I'll need to remove all swaps now (even for Android) from AlphaToColor32, will try ASAP

birbilis commented 2 years ago

It seems that by fixing the new Image32 to remove that swap, no swaps are needed anymore in AlphaToColor32, nor an RGBA instead of BGRA is needed at PaintToBitmap call, for either Android or OS-X. My guess is some recent change to Image32 is using that RGBA under the hood which made the older swaps obsolete

added https://github.com/EtheaDev/SVGIconImageList/pull/235 that should close this issue

AngusJohnson commented 2 years ago

@birbilis. Firstly, thank you again for the valuable feedback. Using Image32's sample app FMX3, I've just tried compiling the code in your latest commit and running it on my Android phone (Samsung Galaxy S20 FE) and I get the red-blue color channels inverted. But when I use my current Image32 source code (on the same phone) the colors are correctly displayed. Would you mind rechecking this on your Android device(s)?

birbilis commented 2 years ago

Did a pull from my repo (just in case) Uninstalled SVGIconImageList from GetIt and closed Delphi IDE Opened the Image32.groupproj on a newly started instance of Delphi 11.1 Built the packages at the top of the group project (just the Image32_FMX.bpl one should be enough though) Made the fmx_svg project Active (it is the one from FMX3/ folder) in the Projects pane Selected Android 64-bit Target Selected Development Configuration Right clicked Libraries and did "Revert System Files to Default" Did Project/Build Did Project/Deploy (have already generated/set a keystore and self-issued certificate to use with Android deployments) Uploaded .apk from Image32\Examples\FMX3\Android64\Release\fmx_svg\bin to BrowserStack's App Live Tried that APK with Samsung Galaxy S20 with Android 11

Resulting image is fine: image

Is there a chance your system is somehow mixing other versions of Image32 units coming from SVGIconImageList? On mine I'm uninstalling from GetIt since I only use Packages and don't set any paths for building


Just in case I cleaned and built again all the packages I have in the .groupproj and installed the VCL one, then cleaned the Android target again for the fmx_svg project and built, deployed and uploaded and tried on BrowserStack App Live again, this time on Samsung Galaxy S20 with Android 10 (instead of 11). Had the exact same result so my version shows fine.


To be extra sure I also then opened in Project view the Contains/../ node of Img32_FMX.bpl package project (one that I've added to Image32 to only contain the FMX-compatible units and reference [require] the FMX framework) and drag-dropped all the files (also separately did for the ones under the Clipper subfolder there) onto the fmx_svg project node in the Projects view (that is showing the Image32.groupproj). This added direct references to the Image32 units to that project, so I can be pretty sure it's not pulling some other unit version implicitly from elsewhere (that's the way I can also try patches when I work with Getit-installed stuff like SVGIconImageList, I add a direct unit reference to a demo project, and note this time I have SVGIconImageList uinstalled and also work directly on my fork of the Image32 repo). Again this time my version works fine on Google Pixel 6 with Android 12


Also note that I've tried the fixes with the SVGIconImageList repo and its copy of Image32 (which uses the old Clipper) separately and its SVGIconImageListFMXDemo works fine with my latest changes (on OS-X and Android). And my app too.

I'll see if I can try on another machine too, however ideally one should try on a Delphi installation where SVGIconImageList had never been installed (just grab my Image32 version, open the groupproj and do the above steps)

AngusJohnson commented 2 years ago

Hi again George. Here are the steps I've taken to test your modifications.

  1. Closed Delphi.
  2. Renamed my Image32 folder so any preset source and library folders in the IDE referencing Image32 won't be found.
  3. Downloaded and unzipped your latest Image32 commit to a temporary folder.
  4. Opened Delphi IDE again and opened the FMX3 sample in this temporary folder.
  5. Connected my phone to my PC using USB.
  6. Compiled, installed and executed the app in my phone (ie not using emulation) and you'll see the result below.

Screenshot_20220703-080903

img32_2

img32_3

birbilis commented 2 years ago

I don't think renaming is enough if Delphi is for some reason compiling units into some shared folder, or if you have another Image32 installed via a separate package like the one SVGIconImageList from GetIt seems to be installing

the safest way to try it is to add via drag-drop all the needed Image32 source files into the test project so that they're explicitly referenced. Note that I've tried that step too

I'll check in another machine too, but I don't have any machine where SVGIconImageList was never installed or other clean Delphi installation