Owned windows, keyboard hooking...issues and patches


#1

Hi everybody…my first post here :wink:

I’m working for a company that is developing audio plugins and recently we started testing JUCE for the GUI side of things (the JUCE audio plugin framework is not/will not be used) of our commercial product. So far I’ve come across several issues and tried to fix them myself.

The approximate list of changes (to the official 1.50 release) is as follows (the patches will be attached):

  • We needed owned window behaviour (in Win32 basically a window that is always above its owner in z-order) for desktop windows. For example the main plugin window opens another window (like settings, preset browser…) and wants it to exist outside its client area but to also “own” it. For this I added an ugly temporary “owner window handle” hack to the ComponentPeer class (and related Win32 implementation in juce_win32_Windowing).
  • As we are developing an audio plugin, the “keyboard takeover” done by many hosts was/is also a serious issue. For this I added a much more elaborate “workaround” through a new helper class called HostileKeyboardHooker in juce_win32_Windowing.cpp. It basically hooks keyboard messages (whenever there is a widget/component active that wants keyboard focus) and processes them before the host has a chance to “eat” them. It should therefor be generic and work across most hosts (except maybe for some of those that do not steal keyboard messages through hooking but through window subclassing…it should be relatively easy to modify it to work with those also…I just didn’t have the chance to work with one of those…so far tested only with Wavelab, Reaper and SoundForge). It would also help if all components would properly “reply” whether they used a key event or not (e.g. the TextEditor reports it consumes all keys but in effect obviously does not consume, for example, the F* keys).
  • As one user already requested (unfortunately I lost the link to the post) we also needed rounded and semi-transparent menus so I had to manually apply the “setOpaque( false )” patch into the PopupMenuWindow constructor in juce_PopupMenu.cpp.
  • I provided a .vcproj for MSVC++ 9.0 with tweaked compiler and linker options. That brings me to one more bug: using the MSVC SSE2 and /fp::fast optimization switches completely breaks JUCE (e.g. the JUCE demo window does come up but it is completely garbled and unuseable, no proper control is visible…only the window background and some “garbage”). That minor rounding errors/differences (or better, minor deviations from standard-prescribed rounding behaviour), that can occur with the above switches, cause such behaviour in a GUI library possibly points to serious bugs/“some hidden hackery somewhere”. IMO even using floating point for basic 2D GUI functionality (i.e. not drawing&co. but displaying standard widgets) is a performance bug.
  • Added JUCE_INCLUDE_GIF_SUPPORT and JUCE_INCLUDE_JPEG_SUPPORT configuration macros (in gui/graphics/imaging) to completely remove gif and jpeg code from my binary (preferably that whole code should be refactored to be less coupled so that only actually used functionality gets linked in, without the need to use macros…also it would be nice/welcomed to use gdi+ for jpeg and png support on windows to completly remove the libpng and libjpeg dependencies).
  • Changed LookAndFeel::drawFileBrowserRow() to not indent file names/file list entries that do not have an icon
  • Various other fixes: (hoards of) warnings related to custom operator new and delete chemistry in DLL builds (juce_Memory.h), MSVC “Secure CRT” related issues (juce_CharacterFunctions.cpp), non-Unicode builds related issues…

Hope this helps and gets (properly) included in a future JUCE release :wink:


#2

Ok, thanks… Sounds like there might be some interesting stuff in there, though I don’t really see how I can merge it into my (now very differently structured) tip build without spending hours going through each file by hand. Might have been better to post me each change as you did it so I could merge it gradually, rather than dumping the whole lot on me in one go!

And the tip has moved on quite a bit - some of your stuff might already be done. E.g. it already has macros to disable the jpeg/png code.

…erm, ok. Would love to hear you explain how to do that without putting half the library into templated header files! I am planning to use some platform-specific libraries for loading images, but since the current code works well, is consistent across platforms, and really only has a pretty small footprint, it’s not a big priority.

I’m intrigued about why you’d write your own code for adding a juce gui to a plugin? Having sweated for years over the damn thing, it seems like madness that anyone would want to reinvent such an absolute PITA piece of glue code.

I’m not going to add a VC9 project yet. Rather than maintaining umpteen different projects, my plan there is that when my new Jucer v2 is ready, it’ll be able to auto-generate projects for all the major compilers from a single master project.

Having never tried the fast floating point setting, that sounds like fun, I might give it a go when I get a moment.

I’m sorry, your post seems to have fallen through a time-warp from 1988… nowadays I think you’ll find most CPUs have built-in floating point support! :wink:


#3

[quote=“jules”]
…erm, ok. Would love to hear you explain how to do that without putting half the library into templated header files! I am planning to use some platform-specific libraries for loading images, but since the current code works well, is consistent across platforms, and really only has a pretty small footprint, it’s not a big priority. [/quote]

Already done that on mac quite a long time ago and posted on the forum.

http://www.littlejourney.net/Juce/juce_CoreImageLoader.cpp
http://www.littlejourney.net/Juce/juce_ImageFileFormat.cpp
http://www.littlejourney.net/Juce/juce_ImageFileFormat.h

Moreover it applies gamma settings and stuff like that so rendered image are just like the OSX Preview.

Hope this helps.


#4

Thanks, yes, I know you posted that, but it also needs to be able to save. There was no point in me adding the loader code if it still needed to have all the library code for saving.


#5

True…but unfortunately I did not have the time for that process (especially because we are still using svn)…I was going through literally dozens and dozens of GUI libraries “out there” trying to find one that would satisfy (or atleast suck least :mrgreen: and it looks we’ll stick with juce :mrgreen: :wink: ) and was therefor forced to use the “tracer bullet” strategy to find a workable solution as soon as possible.
I did however preserve the original folder structure in the patches archive while placing in it only the changed files along with their original counterparts (only renamed to .original.) so it should be trivial to see the actual changes using any decent file compare utility.

Your objection holds for my post itself aswell. Ideally/properly I should have separated individual/unrelated issues into separate topcis/threads but was, for the same reason as above (with the addition that I’m going away for three weeks on friday and had to post my current progress changes here as fast as possible so that the difference between my base, 1.50, and the tip does not grow further and perhaps some of the changes already get patched in when I come back :slight_smile: ), “forced” to merge it all into one post.
OTOH it also holds that it is reasonable to expect not to have to spend (considerable) time fixing/working on a library that we will eventually pay for. So I’ll use that justification to continue the above shameful practice of ‘dumping’ a few more issues here :mrgreen:

happy to hear that…looking forward to the next official release then :slight_smile:

I would have nothing against a (much) more templated library but for this issue it is not even necessary (atleast/especially with LTCG capable compilers):
Use of a base class with virtual functions is the main culprit here for why the compiler cannot see through the coupling and discard unused code. By briefly searching through JUCE code, ImageFileFormat seems to be used in ImagePreviewComponent, Drawable and in Image and ImageCache for loading from files/streams with the typical use case being something like:

ImageFileFormat* const format = ImageFileFormat::findImageFormatForStream ( someStream );
if (format != 0)
    Image * someImage = format->decodeImage ( someStream );

which seems redundant…
…all of the above mentioned ImageFileFormat ‘users’ are or should be optionally used classes/functionality so if we remove/change the use of virtual functions in the example snippet the compiler will be perfectly free to discard unused portions of the ImageFileFormat hierarchy (for code that does not call generic loadFrom()-like functionality but explicitly specifies the loader/image type). As “image file format loaders” are natural singletons (and should not be “constructed around freely”) this seems relatively easy to do…simply merge the static findImageFormatForStream() member function with the virtual decodeImage() member function into one static/global loadFrom()/decodeFrom()/decodeImage()/‘something along those lines’ that will simply ask all the statically registered decoders to decode and create the image (just like findImageFormatForStream() does now but with a non-virtual call to decodeImage())…because what real use actually exists for the abstract ImageFileFormat base class?

There is also one use of ImageFileFormat in juce_win32_Windowing.cpp:

static const unsigned char dragHandData[] = {...};
Image* const image = ImageFileFormat::loadFrom ((const char*) dragHandData, sizeof (dragHandData));
dragHandCursor = juce_createMouseCursorFromImage (*image, 8, 7);
delete image;

which is exactly how not to do it…you should know at compile time which format you saved the dragHandData[] with and use that decoder specifically. More importantly the snippet shows one pattern/style that is unfortunately prevalent accross the library…unjudicious/inefficient and (exception) unsafe use of dynamic memory allocation…there is apsolutely no need to allocate the temporary image on the heap there and if juce_createMouseCursorFromImage() throws (as it can, despite it being incorrectly marked with throw()) the memory will be leaked…you should atleast use the readily and freely available std::auto_ptr<> for such uses…

ps. Marking functions with throw() is a pessimization with standard-conforming compilers (use __declspec( nothrow ) equivalents where available).

pps. “Injudicious use of new” brings me to injudicious use of dynamic_cast. Besides being plain inefficient it also forces the use of RTTI which significantly bloats my binary as I am using template metaprograming to automatically generate various GUI elements which coupled with mangled names (caused by templates), virtual functions (imposed by inheriting from JUCE classes) and RTTI (imposed by the virtual functions) create a lot of large (completely useless) RTTI records. I would therefor very much like it if you would replace dynamic_cast usage with the visitor pattern or plain virtual functions.

A “small” footprint is obviously a subjective/relative meassure…but I would agree with/accept the priority assesment if atleast the disabling macros are there (for all the loaders).

Well…

  • the main functionality was already mostly done before the decision was made to try and go with JUCE for the GUI
  • my plugin framework is template/CRTP based, uses zero vritual functions, zero memory allocations (of its own), zero macros and zero threads (of its own) and actually worked from day one with Wavelab (with VSTGUI, Qt, wxWidgets and JUCE used for the GUI)…(although it still only has one backend, a reimplementation of the VST 2.4 32bit :mrgreen: )
  • the “least common denominator” target of the JUCE plugin framework seemed possibly limiting
  • the plugin framework is one of the main components of a plugin and it does not seem rewarding depending for it on one developer that also decided to “render the STL obsoloete” :shock: and reimplement/‘reinvent the wheel’ and maintain a few other ‘universes’ (from mutexes, strings and filesystems to playing svgs and audio files, even if many of those are already provided by boost and the standard library itself and usually better) because of the attitude that “using more than one library is painfull” (no ofense but IMNHO that stance/view is just plain wrong…although it is, unfortunately, followed by most of the major GUI libraries out there)…
  • the previous point also points to a contradiction: why would you go and reinvent the PITA glue code associated with the functionality mentioned in the above point?
  • when writing plugins knowing your target and your platform is the key to a succesful job…so it can sometimes even be beneficial to go from the beginning yourself to really learn “your buisness”
    :wink:

I did have to copy/paste the “attach to the native handle” code from the JUCE VST wrapper to be able to use JUCE in a plugin without the JUCE plugin framework. IMO this should be redesigned so that this sort of code gets moved into the JUCE GUI library so that it has the power to work in plugin like environments (where JUCE is not the “global application wrapper” like wxWidgets and Qt also want to force upon you) on its own. A plugin framework should not be coupled with GUI functionality any more than it absolutely needs to.

…and if I am allowed a counter question…why did you go down the Qt path of “reinventing”/reimplementing the windowing system yourself instead of using what is already provided by the OS (like wxWidgets where atleast one widget maps to one HWND)? Perhaps many/some of the issues would not exist if you used as much as native functionality as possible (especially in environments like plugins where you hook into ‘someone else’s window system’ through native handles)…not to mention more efficient (time and especially binary size wise)…

You should check out CMake for that (they’ve already implemented that). :wink:

[quote]
I’m sorry, your post seems to have fallen through a time-warp from 1988… nowadays I think you’ll find most CPUs have built-in floating point support! :wink:[/quote]

Most do (many ARMs still do not) but that does not change the fact that some operations are still faster with integers (e.g. addition) and more importantly if the underlying OS uses integer coordinates you incurr constant conversions which are always slow and simply bloat the library (because the conversion code is silently inlined allover in small pieces). IMO it is wrong for a library to make such assumptions about its targets and user preferences (if it does not have to). There is also the now somewhat obscure issue of forcing the use of _mm_empty() in MMX code…

…gotta run now… :mrgreen: :wink:


#6