Menu

#89 FreeImage CompileTime Configuration Support

open
nobody
None
5
2015-01-09
2014-03-29
No

Hi Hervé,

as the FreeImage library gets bigger and bigger, some support for configuring what's actually in the build (and what not) seems to be useful.

I've implemented support for optional compile-time configuration of the FreeImage library. Currently, it is possible to configure, which plugins (aka formats) will be available in the library. The source code provided is based on the latest 3.16.0 release available in the CVS on SF.

Only some of FreeImage's source files are affected by that support. There are no changes needed in any of the third-party sources files, like these from libPNG or libTIFF, for example. Reducing the library's actual compiled size relies on the linker's capability, not to include any unreferenced object files (what actually seems not to work with MinGW, still not sure about GCC on Linux, but that is currently not the point).

Also, there aren't (or not yet) any ideas to include this configuration support into Makefile.srcs. That seems to be a hard task. The creation of Makefile.srcs by a Bash script makes things even harder. So, with Make, the only benefit seems to be a smaller library. Since all source files still get compiled, there is no out-of-the-box compile speed improvement. However, people could easily remove some of the source files from Makefile.srcs manually. If this matches what was configured to be available in the library, FreeImage still compiles happily.

Most of the changes are documented through comments in the modified source files. Search for the text @Hervé to find all remarks intended to explain the changes in more detail (the latter are intended to be removed again).

These are the steps required to make that work:

1. Register Plugins in any order (that was the biggest part)

We need to break the principle, that plugins must be added/registered in the order of the plugin ids defined in FREE_IMAGE_FORMAT. We want to keep the mapping between a plugin/format and its id stable and as is. For example, FIF_TIFF is now 18 and will ever be. However, the absence of one or more plugins creates holes in this list. Due to these holes, some of the actual code must be modified.

1.1 Method PluginList::AddNode

Currently, in PluginList:AddMode, m_plugin_map::size() is used to determine the registered plugin's id. That, of course, no longer works. I've added a new parameter FREE_IMAGE_FORMAT fif to method PluginList:AddMode, which explicitly specifies the plugin's id and is used as the its key in m_plugin_map.

If FIF_UNKNOW is passed to PluginList:AddMode, a so called unknown (in the meaning of not a built-in or well-known) plugin is to be registered. These calls come from FreeImage_Register[Local|External]Plugin or from the Win32 specific external plugin initialization block in FreeImage_Initialize. For these plugins, a new id is created from a new integer field m_next_unk_plugin_id.

1.2 Fast test on parameter FREE_IMAGE_FORMAT fif

The test

if ((fif >= 0) && (fif < FreeImage_GetFIFCount())) {

actually present in functions FreeImage_[LoadFrom|SaveTo]Handle does no longer work properly. I've just replaced this with

if (s_plugins != NULL) {

and just let PluginList::FindNodeFromFIF do the work. This may be slightly slower, but, of course, does not find a plugin, that has not been registered.

1.3 Iterating over plugins with a simple for-loop

Of course, we can no longer iterate over all plugins with a simple counting loop like

for (int i = 0; i < FreeImage_GetFIFCount(); ++i) {
    PluginNode *node = s_plugins->FindNodeFromFIF(i);

since a FreeImage library with only plugins JPEG and TIFF available returns 2 for FreeImage_GetFIFCount, but plugin's ids are 2 and 18, respectively.

I've found these in function FreeImage_GetFIFFromFilename (Plugin.cpp) and in function FreeImage_GetFileTypeFromHandle (GetType.cpp).

I decided to add two new methods to class PluginList:

PluginNode *FindNodeFromExtension(const char *extension);
PluginNode *FindNodeByHandle(FreeImageIO *io, fi_handle handle);

which internally both use the C++ map's real iterator (since m_plugin_map along with its iterator is private, all this must happen from inside the PluginList instance).

Basically, PluginList::FindNodeFromExtension contains the code previously found in function FreeImage_GetFIFFromFilename inside the iterator loop and PluginList::FindNodeByHandle contains the code of function FreeImage_Validate.

Likely this solution is even faster than calling PluginList::FindNodeFromFIF constantly from inside a for loop.

2. New public API functions

There are two new publicly available API functions present:

int FreeImage_IsPluginSupported(int fif);
const FREE_IMAGE_FORMAT *FreeImage_GetSupportedPlugins();

The term supported is used throughout the configuration system. If a feature (currently only a plugin, but maybe other aspects in the future like the toolkit or metadata routines) is supported it is compiled into the binary and available in the library.

Function FreeImage_IsPluginSupported works much like function FreeImage_IsPluginEnabled, but already returns TRUE, if PluginList::FindNodeFromFIF actually found a node.

Function FreeImage_GetSupportedPlugins returns a pointer to a FIF_UNKNOWN terminated array of FREE_IMAGE_FORMAT entries. The array contains one entry for each supported plugin (plus one last terminating FIF_UNKNOWN entry).

This array is created by the new method PluginList::GetSupportedPlugins() (using the iterator of m_plugin_map) in funciton FreeImage_Initialize, after the well-known, built-in plugins have been registered. The pointer returned by PluginList::GetSupportedPlugins() is then stored in a static variable s_supported_plugins, which in turn is returned by function FreeImage_GetSupportedPlugins.

Before FreeImage_Initialize has been called (when FreeImage is used as a static library), s_supported_plugins points to a single FIF_UNKNOWN entry (which is also a valid FIF_UNKNOWN terminated array) so, FreeImage_GetSupportedPlugins never returns NULL.

However, in contrast to FreeImage_IsPluginSupported, which also works for self-registered plugins, FreeImage_GetSupportedPlugins only returns an array of supported/available built-in plugins.

3. Feature/Plugin configuration

Basically, there is a new header file Config.h, which contains #defines SUPPORT_xxx, one for each xxx to be configured IN or OUT (supported or not). Defining to 1 supports/enables a feature. Not defining it or defining it to any other value than 1 (0 is recommended), drops the feature from the library.

All options present in Config.h are actually well documented. The following example actually prevents format FIF_WEBP from being included into the build:

// Defines, whether the Google WebP image
// format is supported (1) or not (0).
//
// Formats:        FIF_WEBP (35)
#define SUPPORT_PLUGIN_WEBP 0

There are two macros used to determine, whether a feature is supported (shall be included) or not. FI_SUPPORT(x) and FI_IDFRS_VISIBLE(x) (forget about the latter for the moment). These take the "name" of the feature to test for. The name of the feature is the part following the SUPPORT_ in the above example). Note, that this name itself is never defined explicitly.

Now, some parts of the code are just surrounded by a preprocessor conditional, using the FI_SUPPORT macro. As an example, this is a code fragment from function FreeImage_Initialize:

[...]
#if FI_SUPPORTS(PLUGIN_WEBP)
            s_plugins->AddNode(FIF_WEBP, InitWEBP);
#endif
[...]

As said before, the whole configuration system is optional. That means, Config.h is included into FreeImage.h only, if the new preprocessor option HAVE_CONFIG_H is defined. If its not, in FreeImage.h both macros FI_SUPPORTS(x) and FI_IDFRS_VISIBLE(x) are defined to 1, making the #if conditional always evaluate to true. So, if HAVE_CONFIG_H is undefined, which is the default, every feature is included into the library.

Currently, HAVE_CONFIG_H is defined in FreeImage.h (in my new version). Not sure whether we should leave it there as a comment. Basically, this should be an externally provided option like FREEIMAGE_EXPORTS and FREEIMAGE_LIB.

Macro FI_IDFRS_VISIBLE(x) is actually an abbreviation of FI_PUBLIC_IDENTIFIERS_VISIBLE(x). There is an extra general option in file Config.h

#define PUBLIC_IDENTIFIERS_VISIBLE 0 // or 1

This option controls, whether so called public identifiers of even unsupported features are always visible/available in FreeImage.h. This option only has an effect, when FreeImage.h is used by a client/consumer. It has not much relevance for building the library.

The idea behind this is, that (for my mind) the FREE_IMAGE_FORMAT enumeration should only contain these formats, that are actually available. Additionally, the loading and saving flags for a certain plugin should not be defined, if the plugin is not supported. That could help users of the library not to use things, that are not present.

However, defining PUBLIC_IDENTIFIERS_VISIBLE to 1 makes all these public identifiers available, even if the corresponding plugin has been dropped from the library.

So, the members of FREE_IMAGE_FORMAT are surrounded by:

[...]
#if FI_IDFRS_VISIBLE(PLUGIN_WEBP)
    FIF_WEBP = 35,
#endif
[...]

as well as are the loading/saving flags:

[...]
#if FI_IDFRS_VISIBLE(PLUGIN_WEBP)
#define WEBP_DEFAULT    0       //! save with good quality (75:1)
#define WEBP_LOSSLESS   0x100   //! save in lossless mode
#endif
[...]

Now, of course, FI_IDFRS_VISIBLE(x) evaluates to true, if the plugin is
supported or PUBLIC_IDENTIFIERS_VISIBLE is defined to 1 (simple as that).

Additionally, there is always one conditional after including FreeImage.h in all of the PluginXYZ.cpp files and some more in some of the helper files (like MNGHelper.cpp for example):

#include "FreeImage.h"

#if FI_SUPPORTS(PLUGIN_XYZ)

[...]

#endif // FI_SUPPORTS(PLUGIN_XYZ)

Basically, there are two reasons for that. First, the PluginXYZ.cpp will likely use these public identifiers, which actually may not be defined in FreeImage.h. Second, the file compiles faster, if it contains less code...

4. Plugin dependencies and restrictions

Some features in FreeImage require some of the plugins to be present. Also, some plugins heavily rely on other plugins (as FIF_JNG relies on FIF_JPEG, for example).

These general dependencies have been grouped into two groups: Dependencies and Restrictions. This distinction is important, since conflicts are solved differently for both dependencies and restrictions.

4.1 Dependencies

Currently, there are only two plugins, that other plugins have dependencies on. These are PLUGIN_JPEG and PLUGIN_PNG.

Plugins PLUGIN_JNG and Plugin PLUGIN_PICT have dependencies on PLUGIN_JPEG. Since JNG is basically a multi-page JPEG and PICT may only contain a JPEG encoded image, using FIF_JNG and FIF_PICT without having the JPEG code in place is pointless. You just cannot have JNG without JPEG.

The same is true for PLUGIN_ICO and PLUGIN_MNG, since MNG is just a multi-page PNG and (at least modern Vista) icons are actually PNG files, these both have a dependency on PLUGIN_PNG.

The nature of a Dependency is that you just can't use one without the other.

There are two solutions for that. We could disable PLUGIN_JNG and PLUGIN_PICT, if PLUGIN_JPEG is not supported or we could auto-include PLUGIN_JPEG into the build if any other supported plugin requires it. The latter solution is currently implemented.

There is a simple and hard-coded #if #else #endif block at the end of Config.h which enables/supports PLUGIN_JPEG if PLUGIN_JNG and PLUGIN_PICT are supported. Additionally, there is another similar block, for PLUGIN_PNG.

4.2 Restrictions

Restrictions are limitations introduced by the absence of a certain plugin, one could live with easily.

As an example, PluginTIFF.cpp may (as a last resort solution) load an image's thumbnail in PSD format. Now, if PLUGIN_PSD is not supported, this will not work, of course. So, that is a restriction, clearly documented for option SUPPORT_PLUGIN_PSD.

To make this work, the load thumbnail in PSD format stuff in PluginTIFF.cpp is properly surrounded by an #if conditional:

// ... or read Photoshop thumbnail (only if plugin PSD is supported)
#if FI_SUPPORTS(PLUGIN_PSD)
     if(!thumbnail) {
          uint32 ps_size = 0;
          void *ps_data = NULL;
[...]
#endif // FI_SUPPORTS(PLUGIN_PSD)

In addition to the lack of PSD thumbnail support for TIFF, currently, there are only two more restrictions present.

One are the JPEG Lossless Transformation routines, which are not available until PLUGIN_JPEG is supported. Here, the proper preprocessor conditionals are present in FreeImage.h and in JPEGTransform.cpp.

Second is the capability to load Exif thumbnail images, which are JPEG encoded. If PLUGIN_JPEG is not supported, the thumbnail loading code at the end of function jpeg_read_exif_dir in file Exif.cpp is removed by preprocessor conditionals.

5. Conclusion

I recommend to implement this configuration support. With that in place, it is easy for people to build custom libraries with fine grained control of the available features. Likely I myself will be one of the first users of this feature.

Although, the complexity of all this is not zero regarding the dependencies and restrictions, I think it is enough to have this described and commented solely in Config.h (which is already done :-)). Since this is a compile-time feature, we do not need much documentation in the PDF.

The only thing to be pulled into the PDF is some words about the two new API functions. As you will see, the documentation for FreeImage_GetSupportedPlugins is already present in the source file. Function FreeImage_IsPluginSupported seems to be easy to document.

So, the only thing to consider is, that newly developed plugins should integrate well into this configuration system. So, likely there is some (but not much) more work to be done for upcoming plugins.

Carsten

1 Attachments

Discussion

  • Floris van den Berg

    Interesting thought. Though i no longer develop FreeImage (but i read the forums now and then), I feel the need to say something about this because it delves into stuff I designed. Your idea brings the plugin system closer to an independent plugin system, which I like. On the other hand, IMHO it's a fix for a problem that isn't there. My two cents:

    • Is it really necessary to cut out pieces of FreeImage? The library is not that big in terms of megabytes and most computers have terabytes of storage. Even smartphones have several gigs.

    • Just a wild idea: Wouldn't it be an option to ditch the whole set of predefined FREE_IMAGE_FORMATs and rely exclusively on FreeImage_GetFIFFromFormat and such? It would be less work, it would solve the gaps in the FIF's, but obviously it would break compatibility. I personally am not so fond about functions like FreeImage_IsPluginSupported, because they are not so elegant and are only there to bridge an old system and a new system.

    Ofcourse i'll let Hervé decide, but I had to go out and so my saying. Cheers.

    Floris

     
  • Floris van den Berg

    Another thought. We could make a new order of FIF's, say from 1000 and onwards, and make the current ones aliases of the new ones. So the node would have the new plugin id, and a predefined alias. The node functions would then search both the plugin id and the alias.

     

    Last edit: Floris van den Berg 2014-03-29
  • Carsten Klein

    Carsten Klein - 2014-03-30

    Hi Floris,

    I see your ideas and concerns. However, the latest FreeImage DLL is about 3.2 MB in size. Of course, considering today's storages, that is not much. However, I prefer storing huge amounts of data on these hard disks and process them with much smaller pieces of software.

    To be more clear, I do think, that a shared library performs much better, if it is smaller. Tat is due to loading an relocation times. Additionally, it makes me cringe, if I need to deploy a small application, let's say, an 280 KB executable, along with a 3.2 MB library just for loading/saving one or two images. Don't you feel this, too?

    Furthermore, there have been users requesting this along with putting in, that many of the supported formats are not really of any importance today. So, there are several recipes out there in the forums, how to manually strip FreeImage to someone's needs. Actually, I just packed these into one official configuration system.

    The work needed to get that in place is not an issue to me, since everything is already done. The files that I've provided are actually drop-in replacements, based on version 3.16.0. So, Hervé only needs to re-apply the fixes, that have come up since the last release.

    Also, the config system is optional. It's intended only for those users, that are able to build the library (actually only for those, that were using the "how to strip FreeImage" recipes). I'm sure, those users know what they are doing and also have a certain reason for that.

    About the FREE_IMAGE_FORMAT enum and your ideas about reordering FIFs and so on. At first glance, this is and should stay an issue, that is independent from the config system. My challenge was to make it configurable.

    Of course, there are things in the plugin system, that could be improved. Most of these you were already talking about. There is a fix in FreeImage_GetFileTypeFromHandle (GetType.cpp), that tries to detect a type of RAW, even if the camera is stating it got a TIFF image. This has to do with the order, the plugins test the image (RAW should better come before TIFF).

    Now, we can add the plugins in any order. However, since the C++ map's iterator still uses the key's natural order, plugins are still iterated in ascending FIF order. Of course, there are solutions for this (using an Object key with an extra class, give this an additional int with the 'insertion order' and use a custom comparator for the map). However, at first, that was not my focus.

    However, If you (and Hervé, of course) like to make some improvements to the library as a whole (even breaking backwards compatibility and moving to version 4), I'm in (if you like).

    Carsten

     
  • Anonymous

    Anonymous - 2014-03-30

    Hi Carsten,

    As you said it, 3.2MB is not much in relation to today's storages. I don't agree that a small library is automatically more efficient and I personally don't mind to include a 3MB library, even if the application is smaller, but I can empathize with the sentiment that it can 'feel wrong'. I'm not at all against compiletime configuration. However, I feel that it should be done with either as limited as possible impact on the FreeImage API or a complete rework and bump to 4.0.

    About the FREE_IMAGE_FORMAT enum and your ideas about reordering FIFs and so on. At first glance, this is and should stay an issue, that is independent from the config system. My challenge was to make it configurable.

    It's not independent from the config system, because you propose changes to the plugin system to make the config system work. I'm not against changing the plugin system per se, but here additions are made that seem unnecessary because existing functions could handle them.

    • FreeImage_GetFIFFromFormat could do the same job as FindNodeFromExtension
    • FreeImage_FIFSupportsReading could do the same job as FreeImage_IsPluginSupported.

    Of course, we can no longer iterate over all plugins with a simple counting loop

    This is only true if we keep relying on the predefined FREE_IMAGE_FORMATS. If we force people to use the already built plugin-find functions, alias the existing built-in plugins and let FreeImage_FIFSupportsReading return false and load and save NULL for non-existing plugins, we are practically there already. Even plugins referencing other plugins can be dealt with by using the plugin-find functions.

    I like FindNodeByHandle btw.

    Cheers,
    Floris

     

    Last edit: Floris van den Berg 2014-03-30
  • Floris van den Berg

    To clarify. The plugin system was designed so that every plugin every time can have a completely different FIF. When a plugin is initialized the plugin system tells the plugin what its FIF is. The built in plugins have predefined FIFs because of ease of use and because i never envisioned that anyone would want to rip plugins out. Call it a lack of insight.

    FreeImage_GetFIFFromFormat, FreeImage_GetFIFFromFilename, etc. all deal with finding plugins and would already adapt to new plugin ids. So to make compiletime configuration possible all that really is needed is to stop using the FREE_IMAGE_FORMATs alltogether or to make an alias to be able to reference the new plugin id (if any).

     

    Last edit: Floris van den Berg 2014-03-30
  • Carsten Klein

    Carsten Klein - 2014-03-31

    Hi Floris,

    so actually you are bothered by the fact, that the FREE_IMAGE_FORMAT is passed to PluginList::AddNode? Of, course, this clearly violates the idea, that the FIF is provided by the plugin system.

    Nevertheless, my current implementation works even with the FREE_IMAGE_FORMAT enum around. As I said before, my goal was not to change the whole thing but only make it configurable (basically by unifying what's out there as recipes for making the DLL smaller).

    To make that clear, too, I did not want to blame you or your code. My only intension was to implement this feature (primarily due to my own needs) and give that work back to the community (for me, that's how open source works).

    I was just not able to recognize your initial intension, that the plugin system should work without predefined FIFs. Of course, for the plugin system it's best, if it does not relay on any externally defined ids.

    However, in today's practice, the FREE_IMAGE_ENUM is omnipresent. Some users may relay on it and even the various plugins use these FIF_xxx constants internally. For sure, these well-defined constants make coding easier and faster as you do not need to constantly lookup the plugin's internal id by plugin or format name.

    In any case, if we follow your idea of creating internal ids (starting with 1000 for example), we should provide a proper replacement for the FREE_IMAGE_FORMAT enum. Relying on the plugin's name is one option, but involves the lookup calls mentioned above. Relying in the formats file extension is not possible, since these are not necessarily unique.

    So for my mind, your idea of aliases seems to be the first choice. You could add another member int m_alias_id to the PluginNode structure and either change PluginList::FindNodeFromFIF or the various functions passing a FIF as a parameter. Basically, FindNodeFromFIF should accept a real FIF (> 1000) or an alias number. The various FreeImage_GetFIFFromXXX functions should likely return the new internal FIF (> 1000) only.

    The m_alias_id could be provided by a new setter method

    PluginList::SetAliasId(int alias_id);
    

    externally (externally from the plugin system, from FreeImage_Initialize for example). Also, the aliases could as well be managed by an additional map, which also may be located out of PluginList.

    With the aliases in place, we could easily reorder the plugins. We could then skip the additional test in FreeImage_GetFileTypeFromHandle, if we register plugin RAW before plugin TIFF, for example.

    However, I don't think that there are many benefits for the users of FreeImage. I'm sure, they will keep using the well-known FREE_IMAGE_FORMAT constants (which are aliases then). Using the aliases is slightly slower, since additional lookups are needed. So, we need to ask ourselves whether that much effort, required to change this, is really worth it. Do you see any more advantages for the users?

    Of course, having a clearly defined plugin system is a good idea in any case so, the idea itself is a striking argument for it.

    After all, since we were talking about compile-time configuration, do you think that my patches are no longer needed after having the FREE_IMAGE_FORMAT enum removed (supposed that compile-time configuration is a required feature at all)?

    Actually, I don't see that configuration support is automatically available (for free), if we reorder the plugins with new internally generated FIFs (> 1000). The preprocessor conditionals in FreImage_Initialize are still needed. So are these in the PluginXYZ.cpp and various other files. The aliases will likely be defined as enumeration (like FREE_IMAGE_FORMAT, if we not just leave this enum as is) so, even the preprocessor conditionals in FreeImage.h should remain there.

    So, the only thing that I've changed is passing the FREE_IMAGE_FORMAT to PluginList::AddNode, which, of course, is a step into the wrong direction, if we focus on the plugin system as an independent structure only. As far as I do understand now, the only question is, whether we go this step anyway for a fast working config solution, or if we settle for new internally provided FIFs along with alias FIFs and rework many more parts of the project.

    Carsten

     
  • Carsten Klein

    Carsten Klein - 2014-03-31

    Hi Floris,

    sorry, I haven't got an email about your next to last post. SF is really not reliable with these tracker forums... :-(

    So, you stated clearly what you like and what not. Let me please address that in more detail.

    Function FreeImage_GetFIFFromFormat (PluginList::FindNodeFromFormat) only considers the format name, which is only the primary extension in most cases. PluginList::FindNodeFromExtension was required to support FreeImage_GetFIFFromFilename, which wasn't my invention. This function was already present. These two look for the format name as well as for the extension list. Of course, if we had the plugin system reworked already, we would not need a new method in PluginList.

    Of course, FreeImage_IsPluginSupported is not absolutely needed and could be replaced by a call to FreeImage_FIFSupportsReading (and another one to FreeImage_FIFSupportsWriting, if we'll allow write-only plugins). My only intension was, that there is a properly named function for a properly defined task (e.g. getting known, whether a plugin is there or not). In fact, most of the FreeImage_GetFIFFromXXX functions would do the job as well. As function FreeImage_GetSupportedPlugins, which is also not absolutely essential, this is kind of a convenience function.

    So, we could just drop these two functions (although I think, these are no real changes to the FreeImage API, these are just extensions, not breaking anything already established, so who is hurt by these convenience functions?) and so, have applied absolutely no changes to the (at least the public) API.

    Yes, simple counting loops would again be possible, if we had the plugin id system reworked. Nevertheless, I'm not sure whether these loops are really better than using the map's iterator.

    for (int i = 0; i < FreeImage_GetFIFCount(); ++i) {
        PluginNode *node = FindNodeFromFIF(i);
        [...]
    }
    

    Here, you constantly search the map m_plugin_map for the next plugin id. I'm sure, that the actual iterator of the map is much faster. (Actually, I'm not really talking about performance here. The map is a balanced tree with only 36 entries so, of course, it is fast either way. Maybe the iterator approach is a bit more elegant. However, I'm aware, that we may need (even more in the future?) specialized methods like FindNodeFromExtension. Having new internal plugin ids (without "holes", your approach) or somehow accessing the map's iterator more generically from external are solutions for that.

    I'm not against reworking the plugin id concept and making FREE_IMAGE_FORMAT just aliases. As you've seen in my last post, that would help also in other places (RAW/TIFF format fix on GetType.cpp, for example). However, there are decisions to make, that are far beyond from what I've done so far with the config system. I'm sure, Hervé will/wants to say something to this topic as well. Actually, we both will not make these decisions on our own. Also, someone has to implement these changes. I just feel, that we are still far from actually making these changes.

    However, I'd love to have the config system in place independently from these new ideas (which I agree with, of course). As I was writing in my recent post, we should also keep in mind, what benefit the FreeImage users would have from these massive changes and whether it's really worth implementing it.

    Carsten

     
  • Floris van den Berg

    Hi Carsten,

    I don't feel that my code is being blamed :-) I just acknowledge that I didn't foresee that people use FreeImage differently as I intended (which is fine). This hasn't been a problem until we tried to yank internal plugins out. Also I would like to mention that I really appreciate all the work you've done.

    I'm not so much bothered by the fact that a FIF is being passed to AddNode. I would do this too. Actually what you've done is not so far from what I suggest at all. You renumber the plugins too, except for those that have predefined FIFs. This indeed saves a redirection, which is good for speed (albeit the increase is very minor). My main concern is the introduction of new functions, that to me seem unneccesary.

    What I suggest is this (which is exactly as you put it, but I thought it's convenient to restate):

    • Keep the FREE_IMAGE_FORMAT exactly as it is
    • Renumber all plugins (starting with 100, 1000, or any number that's higher than the last predefined FIF)
    • Send the FIF to AddNode and let the Node store the FIF as an alias int m_alias_id
    • Let all FreeImage_GetFIFFromXXX return the new id
    • Let FindNodeFromFIF search plugin id's and aliases
    • Remove FreeImage_IsPluginSupported and FreeImage_GetSupportedPlugins.

    Do you see any more advantages for the users?

    There are none. I'm only concerned about the elegance of the code.

    Also, I would like to restate that I'm really happy with the work you, Hervé and all other contributors have done. Many things in FreeImage are now beyond my capability level, which is a good thing. The lib wouldn't have been so awesome if you guys didn't pick it up.

    Cheers,
    Floris

     

    Last edit: Floris van den Berg 2014-03-31
  • Floris van den Berg

    Of course, FreeImage_IsPluginSupported is not absolutely needed and could be replaced by a call to FreeImage_FIFSupportsReading (and another one to FreeImage_FIFSupportsWriting, if we'll allow write-only plugins). My only intension was, that there is a properly named function for a properly defined task (e.g. getting known, whether a plugin is there or not).

    I see, what you are saying. My concern with this is that in my mind every plugin should be supported. One could be ommited, but if it's there it's supported. If it would be renamed to FreeImage_IsFIFValid() it probably wouldn't bother me so much.

    Here, you constantly search the map m_plugin_map for the next plugin id. I'm sure, that the actual iterator of the map is much faster.

    Agree with this.

    However, there are decisions to make, that are far beyond from what I've done so far with the config system. I'm sure, Hervé will/wants to say something to this topic as well.

    I'm sure about this too.

    Actually, we both will not make these decisions on our own.

    Agreed. That's the nature of open source.

    I just feel, that we are still far from actually making these changes.

    Agreed.

    As I was writing in my recent post, we should also keep in mind, what benefit the FreeImage users would have from these massive changes and whether it's really worth implementing it.

    From a user standpoint there probably is no advantage per se. As long as it works, it's fine. But elegance of code should be a concern.

     

    Last edit: Floris van den Berg 2014-03-31
  • Mihail Naydenov

    Mihail Naydenov - 2014-07-30

    +1 for the ability to exclude Plugins, I also wrote some patch for this, but cannot find it atm.

    As for the patch, I personally think FI needs massive, oldfashioned refactor before implementing new major functionality.
    The code is not easy to follow, definitions and declarations are all over the place, the mix c and c++ is awkward.

    It's also worth considering adding the possibility to split the lib not in terms of plugs, but in terms of I/O vs. image manipulation.
    I assume FI is mostly used for I/O, and is quite capable (robustness, file formats), but is weak in image manipulations, and there are much better alternatives (OpenCV)

     

Anonymous
Anonymous

Add attachments
Cancel