tannerhelland / PhotoDemon

A free portable photo editor focused on pro-grade features, high performance, and maximum usability.
https://photodemon.org
Other
1.45k stars 204 forks source link

PhotoDemon and twinBASIC #556

Open tannerhelland opened 7 months ago

tannerhelland commented 7 months ago

(This issue was first prompted by discussion in #555 .)

Thank you to @wqweto, @fafalone, @WaynePhillipsEA, @mansellan for catching me up on the latest twinBASIC developments. I am heartened to hear about all the progress twinBASIC has made in the past few years! My sincerest congratulations and awe to everyone involved.

I would love to be useful in getting PhotoDemon up-and-running in twinBASIC.

Short version: how can I be useful to the twinBASIC team?

Long version: I know PhotoDemon is a very complex project. It probably contains lines of code that first originated in VB4 😟 . I do think it is useful for "stress testing" any VB6 successor since PD relies on relatively little non-VB code (for example, it uses very few thunks - one from @wqweto's ZipArchive class, and another to enable run-time activation of some Windows 7 interfaces in a way that doesn't break XP compatibility... and that might be it?)

PhotoDemon still runs well all the back to Windows XP and is reasonably performant against other software in its class, particularly given the age of the VB6 compiler. It uses no standard VB6 UI elements except menus and forms (it's usercontrols all the way down), which makes it a good-if-complicated testbed for usercontrol quirks. PD does lean a lot on nonstandard hackery for everything from window management to file I/O, but it's fully unicode-enabled across all UI and I/O features (AFAIK). This, plus generous volunteer localizations have led to a large international userbase, so it's actively used across more locales and OS configurations than the average VB6 project.

It does use a frightening amount of subclassing and hooking and this can make it unpleasant to debug in the traditional VB6 IDE. Worse still, the unique performance constraints of photo editing mean that a lot of PhotoDemon's features aren't usable in the IDE at all (they're way too slow) so compiling to native code has always been the only way to develop most of its core features. This makes it an especially challenging codebase to debug, and that's if you ignore the cruft and sketchy design decisions that have compounded over 25 years of intermittent development! (Like using naked Longs as pointers in roughly fifty bazillion places - that's gonna be fun to solve for a 64-bit transition 🀦 )

That said, PhotoDemon's a surprisingly popular little app and I am grateful that many people find it useful. I think it could provide a unique showcase for what twinBASIC can offer, and I'd love to help with the effort.

Of course, I wish I had the time to improve roughly a million aspects of PhotoDemon so it could better do twinBASIC justice (I'm embarrassed by a lot of half-assed and underbaked things in PD), but as you can see from my commit history, I unfortunately don't have a lot of time to spare. I apologize for that. I am lucky if I can push a few PhotoDemon commits a week, and responsibilities sometimes pull me away from the project for weeks at a time. I am also not great at replying to emails (I have messages that are years old in my Inbox - I'm so sorry if anyone reading this sent one. Please don't take it personally, I'm just unbelievably bad at tracking what I have and haven't responded to, and my backlog grows much faster than my willpower to deal with it.)

That said, I would love to help where and how I can. I'm not in a position to test every twinBASIC beta or to spend much time over at Discord, but I'm happy to start working on making PhotoDemon a dual VB6/twinBASIC project. (I'll also continue plugging away at PD itself, of course, and maybe reorient my efforts to focus on a lot of the papercuts that could undermine it as a proper twinBASIC demonstration.)

I'd much prefer working on a massive task like 64-bit compatibility slowly, over time, so I can regression-test everything as I go. Because I am so slow at producing new stable releases of PD, many users rely on nightly builds in their production workflows, so I try to be really careful about "breaking" nightly builds with massive commits. If anyone has already started fixing issues in the codebase to make it more twinBASIC compatible, I'm happy to merge pull requests. (Just please be patient with me if it takes me time to get to them.)

Also, if anyone can share best-practices on how they're migrating their own projects to dual-VB6/twinBASIC compatibility, I'd be very grateful to learn from them. I have no knowledge on twinBASIC's progress since 2022 nor do I fully understand what it plans to add (or has already added) atop VB6, so I'm coming into this pretty blind.

Thank you in advance for any guidance πŸ™

fafalone commented 7 months ago

Apologies in advance for the length, this is all very exciting to me so I had a lot more to say than I thought 🀣

First and foremost, to be clear: For PhotoDemon to run as a 32bit app in twinBASIC, you need to do nothing to the codebase. The expectation is that everything will run, unmodified-- the only exceptions to that are hacks involving VB6 deep internals, like class self-subclassing assembly thunks that rely on the precise undocumented memory layouts/contents or calls to obscure runtime functions like CreateIExprSrv for multithreading*, but I while I haven't looked exhaustively, so far I haven't seen any of that in many of the places where one would traditionally find it; and the other is bad code relying on undefined behavior that might seem to work in VB, but eventually causes problems (e.g. use-after-free), but PD's code seems extremely high quality and professional.

The current state of PD is you can import it from an untouched download and hit 'Run' (or Build then Run exe) with no changes at all, and most things already work; you can load and save photos and use most editing features. The color wheel bug I mentioned was already fixed in the latest beta. I have faith Wayne will take care of the few remaining major bugs in some specific features quickly.

As you can imagine from this, it's progressed quite far since 2022; it also runs lots of other complex apps, like Carles PV's perfect replica of Lemmings, NES emulators, highly complex UCs like VBFlexGrid/VBCCR and my ucShellBrowse. But it's still in Beta, a work in progress, so there's some missing features (for instance none of the Printer stuff is available yet, so PDs printing functionality won't work). So now's the time to start planning and provisioning for the stable commercial phase IMO, but I wouldn't for instance deprecate PD's VB6 version and start using tB-only features besides compiling existing code as 64bit.

* - (twinBASIC has internal implementations of the common ones, GetMemX/PutMemX, vbaObjSet[AddRef], vbaCopyBytesZero, vbaMoveAry; msvbvm.dll declares for these are automatically rerouted)

tB doesn't have a p-code interpreter, so when run from the IDE it's running native code. I don't recall the performance; compiled, tB unoptimized performs better than VB6 p-code but not as good as VB6 native. Optimized LLVM compilation is partially implemented (subscribers only); where that can be used, tB absolutely crushes VB6, in some cases by a factor of 10 to 1000+. \ This might actually be worth talking about before 64bit; I see a lot of math code that tB would support optimizing; right now, it can't handle strings, interfaces, variants, or classes, but can handle "the basic integral and floating point types (...), arrays, global variables, UDTs, standard module function calls, and all control-flow statements".


Now, 64bit conversion. I've converted all my major open source projects to 64bit, including complex apps and UserControls like my kernel logger event trace app, my ucShellBrowse control, and my popular cTaskDialog class. I did the original conversion of Krool's VBFlexGrid (that and VBCCR have a 64bit tB package now and work with very few, minor issues), and Krool did the rest. wqweto also has some 64bit compatible stuff. So we've got several community members who can potentially help out; 95% of the work is simply tedious, once you've learned all the arcane edge cases beyond simple Long->LongPtr stuff.

The APIs are fairly easy; I don't know if you've heard of it, but I maintain a package for twinBASIC covering 1200+ Windows COM intefaces and 6500+ of the most common APIs, you add it to your project and it's like working in C++ with #include windows.h and a number of other common headers. For this I've gone back to the SDK itself and re-done it all from scratch to preserve the 64bit types. So I expect over 99% of the Windows APIs PD uses there's already 64bit compat defs ready to go. The challenge will be the 3rd party libs; it will be a major undertaking, difficult for me in that many don't use standard Windows types so figuring out what's a pointer/other bitness-dependent size type requires more effort.

What you could do to help out; we need to recompile the 3rd party components PD uses as well to target 64, so I don't know-- are the specific versions and compile settings you've used well documented? And do you have an existing setup where you could just run them to compile for x64? I see a lot of compiled DLLs but none of the source packages. That's what I see as the biggest source of 'information we need' as opposed to 'work to be done' that we can do without taking you away from regular enhancements to PD.

Everyone in the community is very friendly and supportive, I'm sure everyone will respect your limited time and won't ask for huge time commitments, just help like that in the couple areas where some additional info is required. Once it's all done it won't be much work to add a 64bit distro; the way I think we'd all agree on approaching it is dual compatibility; use conditional compilation so VB6 builds from the same codebase, unmodified.

It's a little too early to be talking about PRs, but work towards that point can now finally enter its initial steps :)

WaynePhillipsEA commented 7 months ago

Firstly, congratulations to you on such an amazing VB6 project! My hat goes off to you. It doesn't feel like any typical VB6 app that I've ever come across before - it's very professional, modern, and surprisingly nippy!

Also thanks for your support and offer of help to get PD fully working with twinBASIC and 64-bit, it is very much appreciated.

how can I be useful to the twinBASIC team?

Initially, I think it would be great to just have you start doing general testing and reporting problem areas/concerns. It seems that most of us on the core tB team are not overly familiar with PD, and so getting someone that is more familiar with it to check things over would be most useful at this stage. There will, of course, be issues that we haven't yet identified. I can certainly appreciate that your time is limited, and so please don't feel any pressure from us - we can work at your pace.

Some general comments in response to your OP:

Assembly thunks should generally work the same in tB (32-bit) provided they don't use undocumented internals of VB. Since tB-PD seems to be running pretty well, one can probably assume there's no major issue in this regard. When we start work on getting PD 64-bit compatible, we can look at these more closely. One thing to mention is that assembly thunks are often unnecessary in tB since we have support for class-instance AddressOf, which should hopefully make things easier with the transition to x64.

twinBASIC will also be targeting WinXP/NT2000 on-wards, although I believe current tB builds are using a declare or two that are not available in these environments, but that will be fixed soon and certainly before the v1 release.

I've been aware of PD for quite some time, but it is only very recently that UserControl support has matured in tB, and so it was a pleasant surprise to see PD working without too much effort in recent builds.

You mentioned that debugging hooks etc is difficult in VB6 IDE, and one good thing about tB is that the IDE runs in a separate process to the debugging session, and so debugging subclassing and hooking code is generally much easier, without having to fight the IDE.

The new feature list for tB is massive, but realistically we need to stick to fully back-compatible changes with VB6 on this, and so as @fafalone mentions, conditional compilation is going to be the key to making this project x64 bit compatible whilst keeping the existing VB6 codebase working.

I haven't yet thoroughly reviewed the PD codebase, but from what I've seen so far, I agree with @fafalone in that the first issue to start with will be trying to obtain 64-bit builds of the dependent DLLs. If you could assist in this regard, it will be most helpful.

I am very much looking forward to seeing PD x64 edition sometime in the future :)

tannerhelland commented 7 months ago

Thank you for the replies, @fafalone and @WaynePhillipsEA . I'm taking a little time to digest everything, but your messages have been read and appreciated! I've also started taking a look at recent twinBASIC betas and cleaning up some low-hanging fruit in PD to make it better work with tB.

Re: 3rd-party libraries in 64-bit format. Whenever possible, I try to use existing libraries as-is because it makes my life much easier. But sometimes it's inevitable to have to recompile libraries, typically because their interfaces can't be made VB6-compatible. (For example, resvg, which I use for SVG support - it's written in Rust, and some of its APIs would require passing UDTs by value. I obviously have to manually rewrite these as taking pointers instead to get them working with VB.)

The lack of calling convention drama in 64-bit builds makes it much easier for me to manually build 64-bit builds of libraries if I need to. I can put together a collection of 64-bit third-party libraries for PhotoDemon "soon" - it's mostly just a matter of tracking down existing ones where I can, and firing up various toolchains to get the manual ones built.

I'll report back when I have functional 64-bit libraries ready. Thank you πŸ™

fafalone commented 7 months ago

Unfortunately 64bit doesn't solve the ByVal UDT problem. tB has plans to support that, but of course VB6 still wouldn't. Just FYI; you don't actually need to rewrite the lib to take pointers; it makes things easier certainly, but I deal with this in the Windows API a lot; can't just recompile Windows components.

In 32bit, ByVal UDTs can be passed as a sequence of 4-byte ByVals; i.e. ByVal POINT can be declared as ByVal x As Long, ByVal y As Long. 64bit is on 8-byte alignment though, so you'd need ByVal xy As LongLong, or ByVal xy As Currency for VB compat. That also works on 32bit, so I write single definitions for both even though it's a little more work if you're doing 32bit only. For x64, only UDTs larger than 8 bytes are automatically converted to ByRef.

So it gets more complicated for e.g. a ByVal GUID; you need separate 32bit and 64bit defs, since in 32bit you need 4xByVal Long, and in 64bit one ByRef GUID. I recently encountered some functions taking ByVal UDTs of obnoxiously large sizes, that was fun...

HRESULT WinBioGetCredentialState(
  [in]  WINBIO_IDENTITY         Identity,
  [in]  WINBIO_CREDENTIAL_TYPE  Type,
  [out] WINBIO_CREDENTIAL_STATE *CredentialState
);
#If Win64 Then
Public Declare PtrSafe Function WinBioGetCredentialState Lib "Winbio.dll" (Identity As WINBIO_IDENTITY, ByVal Type As WINBIO_CREDENTIAL_TYPE, CredentialState As WINBIO_CREDENTIAL_STATE) As Long
#Else
Public Declare PtrSafe Function WinBioGetCredentialState Lib "Winbio.dll" (ByVal i1 As Long, ByVal i2 As Long, ByVal i3 As Long, ByVal i4 As Long, ByVal i5 As Long, ByVal i6 As Long, ByVal i7 As Long, ByVal i8 As Long, ByVal i9 As Long, ByVal i10 As Long, ByVal i11 As Long, ByVal i12 As Long, ByVal i13 As Long, ByVal i14 As Long, ByVal i15 As Long, ByVal i16 As Long, ByVal i17 As Long, ByVal i18 As Long, ByVal i19 As Long, ByVal Type As WINBIO_CREDENTIAL_TYPE, CredentialState As WINBIO_CREDENTIAL_STATE) As Long
#End If

LOL.

So anyway, point is: Don't worrying about rewriting signatures for tB. We can work with whatever they are.

mansellan commented 7 months ago

Random thought, but might it be easier to treat a twinBASIC version of PD as a fork, rather than using conditional compilation in a single codebase? The tB fork could treat the VB repo as an upstream.

Would probably need to keep to VB6-compatible syntax where possible to avoid descending into merge-hell, but it could maybe keep the codebases a little cleaner?

Might be a crazy idea, it's not something I've ever attempted before.

fafalone commented 7 months ago

There's very, very little conditional compilation outside the Declare statements needed for 64bit updates, especially when we don't need an isolated module and can add LongPtr globally.

ByVal UDTs > 8 bytes are rare generally (<= to 8 bytes, the callsite can be identical); I haven't even checked if PD has any. Tanner's comment indicates he rewrote them for the 3rd party libs.

There will be some changes to support 64bit, like rewriting pointer math, but for that we generally just change it to using LenB(some LongPtr) or a module-level (or even project level) PTR_SIZE constant.

The codebase absolutely won't be a mess of conditional compilation everywhere; just the API declare blocks, assembly thunks, ...and that's really it. And above procedures we want to enable LLVM for, just a quick #If tB Then [CompilerOptions...]. The only other place I use it is for UserControls that should be self-contained and not expose a public LongPtr declaration, in that case public methods with LongPtr arguments are written conditionally; but again, n/a to PD.

In the future, when tB is complete and stable enough, and it makes sense to start using tB-only features to enhance? Then it might be worth considering if the condtionals get out of control; like we'd probably need quite a bit of it to put in mulithreading.

tannerhelland commented 7 months ago

Passing ByVal UDTs was a one-off problem for SVG support via resvg, and since I do other work in rust, manually compiling resvg wasn't a problem. Almost all my other manual library changes are more about stdcall rewriting (sometimes; I mostly use DispCallFunc wrappers now unless I have to make callbacks work), or rebuilding manually for XP support since many libraries ship with Win7+ targets only.

The great thing about PD as a testbed for twinBASIC, IMO, is that 99% of its features are actually written in pure VB6, or rely only on built-in Windows libraries. I've only leaned on 3rd-party libraries where I absolutely have to for performance reasons (real-time zstd compression ain't happening in VB6) or sheer complexity of a given file format (I nearly lost my mind manually writing a Photoshop PSD parser, so for PDFs I just use Google's pdfium).

Anyway, here's a zip with fresh 64-bit builds of (almost) all PD's 3rd-party libraries!

x64.zip

I'm locally storing these in an /App/PhotoDemon/Plugins/x64 subfolder. I probably don't want to sync them to PD's main repo until they're actually supported, since binary files inflate repo size quite a bit (and I may need to rebuild these if anything's broken).

PD loads all external libraries manually via LoadLibrary calls. Switching to these DLLs in a 64-bit build should be as simple as taking the few relevant LoadLibrary calls and prepending "x64\" to the filename (plus declare changes for addresses as necessary, obviously).

The only two libraries I don't have 64-bit builds of are EZTwain (only ever available as 32-bit; 64-bit should probably use native WIA drivers anyway). And pspihost, which is a 3rd-party library for interfacing with Photoshop 8bf plugins. I need to figure out why my copy of Adobe's SDK isn't working with it but that feature's not essential, so I'm gonna postpone solving it until later. I also haven't worried about .exe plugins that are shelled and communicated with over pipes (like ExifTool) because they're out-of-process so 64-bit shouldn't change anything. (PD already uses some 64-bit apps this way, like libraries for AVIF and JPEG-XL files, which are only ever available in 64-bit.)

Thank you again @fafalone for your kind replies and ongoing information. πŸ™ I am very grateful.

mansellan commented 7 months ago

tl;dr yes, it was a crazy thought. Carry on.

fafalone commented 7 months ago

It's a legitimate concern, don't feel bad asking. πŸ‘

As mentioned once we're talking about tB-specific features like multithreading, it's very much a real concern.