GNOME Bugzilla – Bug 344813
GTK+ support for mediaLib
Last modified: 2010-07-10 04:08:35 UTC
The Sun mediaLib libraries provides hardware accelleration for x86, AMD, and UltraSparc processors: http://www.sun.com/processors/vis/ The source code for mediaLib has recently been released under the more open CDDL license. You can review the code here: http://dlc.sun.com/osol/devpro/downloads/current/ Here is the announcement of the code being released under the open license: http://www.opensolaris.org/jive/thread.jspa?threadID=9969&tstart=0 Would it be possible to add the attached patch that provides mediaLib support in GTK+ to speed up blending and compositing?
Created attachment 67295 [details] [review] patch to add mediaLib support
Brian, It appears Sun has has been using mediaLib with GTK+ for quite some time now (since GNOME 2.0, at least). Can you explain how the licensing issues were rationalised for this? Linking a GPL application to GTK+ linked with mediaLib (whether proprietary in the past, or CDDL now) raises a few licensing concerns. I'm not sure how this was rationalised in the past, and even with mediaLib shipping under the CDDL (which is not compatible with the GPL or LGPL), I'm not sure how this would be appropriate now. I'd recommend solving this by dual-licensing mediaLib under the CDDL and LGPL. Thanks, - Jeff
Created attachment 77920 [details] [review] updated patch that works against CVS head. Here is an updated patch. Jeff. mediaLib is shipped with the Solaris operating system, so is considered part of the system and can be linked against much in the same way that you can link against other system libraries like libc. Previously this patch was denied because mediaLib was not free software, but now that it is CDDL it should be okay legally to include support on systems where it is part of the system, such as Solaris and any other distro that wants to include it with the system. Making it dual licensed under CDDL and LGPL would only add value for people who wanted to ship gtk with mediaLib support on systems where mediaLib isn't installed as a part of the system. It'd probably be better if people who wanted to use this patch included mediaLib in the system if they wanted to take advantage of the speed benefits. If this is an issue with getting the patch into GTK, then I can talk with the mediaLib developers to see if they will consider dual licensing.
I talked with James Cheng (James.Cheng@sun.com) from the Sun mediaLib team and they seem agreeable to pursuing a dual license. James said the following to me: --- Given that Java has been GPL'd and OpenSolaris might add LGPL in the future, it's very possible for us to have mediaLib LGPL'd. I guess the real question is when. If OpenSolaris adds LGPL, we will follow the suit soon after, I guess. Otherwise, we will have to go through the legal review and management approval process alone, which might take longer time. --- I (or the mediaLib team) will keep this bug posted as things progress. Is it necessary for mediaLib to be dual-licensed before this patch can go in? I think in my above comment I highlight that on systems where mediaLib is a part of the system (as on Solaris) this patch should be acceptable now.
Getting mediaLib dual licensed would certainly be nice, but I don't see any principal reason to not include the patch. There are some small changes I would like to see in the patch though. I think it would be nicer to have a separate composite_0888_medialib function, and move the use-medialib check one level up, where we are juggling function pointers anyway. Also, it would be nicer to confine the medialib changes in gdk-pixbuf to the pixops directory
Thanks for reviewing the code. I agree that making composite_088_medialib would be a good idea, and an easy change to the patch. I could fix the gdk-pixbuf as you suggest, but the reason for the current logic is as follows: 1) Note the medialib functions require arguments that are not currently passed into the _pixops_scale function. Note that dest_y, dest_x, dest->pixels, offset_x, offset_y are not passed into _pixops_scale and _pixops_composite. Instead math is done in the calling function like this: _pixops_scale (dest->pixels + dest_y * dest->rowstride + dest_x * dest->n_channels, dest_x - offset_x, dest_y - offset_y, dest_x + dest_width - offset_x, dest_y + dest_height - offset_y, [...] Note the first few arguments where the function receives computed values rather than the arguments separately. You will notice similar math being done in _pixops_composite. The medialib function requires these values to be passed in separately, so if we rework the code to just pass the values into _pixops_scale/_pixops_composite and do the math in there, then this would be easier to make work. Is this what you want? 2) One nice thing about calling mediaLib function in gdk-pixbuf-scale instead of _pixops_scale/_pixops_composite is that we avoid a needless function call. Since the point of this patch is to improve performance, adding one extra function call to the stack would slow it down a bit. But if you think the value of encapsulating this code in pixops directory, then I can move this code. BTW, I've been talking with the mediaLib team about getting mediaLib dual licensed CDDL/LGPL, and they agree that this would be a good thing to do, and are exploring the work involved.
I will go ahead and update the patch as soon as I hear back about how you want me to update the pixops_scale code based on my last comment... For reference, I want to show the improvements that mediaLib provides for scaling/compositing. Running the timescale test program in the pixops directory, I see the following output with and without medialib. Note that the numbers are in Mpixels/sec, so a bigger number is faster than a smaller number. You can see the numbers are much better (about twice as fast) when using mediaLib. *** Without mediaLib SCALE ===== 3 4 4a 3 52.66 70.68 30.24 NEAREST 15.34 6.38 5.56 TILES 9.76 6.03 5.59 BILINEAR 2.13 1.98 2.12 HYPER 4 29.01 77.97 105.61 NEAREST 5.77 5.17 6.05 TILES 5.58 6.17 7.33 BILINEAR 1.91 2.21 1.88 HYPER 4a -1.00 -1.00 18.26 NEAREST -1.00 -1.00 3.73 TILES -1.00 -1.00 4.16 BILINEAR -1.00 -1.00 1.49 HYPER COMPOSITE ========= 3 4 4a 3 79.65 66.17 66.62 NEAREST 8.36 6.19 6.60 TILES 15.88 7.51 5.83 BILINEAR 2.11 2.12 1.88 HYPER 4 42.96 61.15 94.08 NEAREST 8.92 5.64 5.59 TILES 5.94 6.23 4.49 BILINEAR 2.01 2.10 1.96 HYPER 4a 8.10 6.93 6.83 NEAREST 4.29 6.71 3.21 TILES 4.91 6.10 3.00 BILINEAR 1.50 1.50 1.39 HYPER COMPOSITE_COLOR =============== 3 4 4a 3 68.46 35.44 64.75 NEAREST 14.97 5.63 7.65 TILES 8.28 5.46 5.88 BILINEAR 1.84 1.98 2.05 HYPER 4 71.24 82.30 65.90 NEAREST 5.71 4.04 5.63 TILES 7.54 5.58 8.15 BILINEAR 1.58 2.12 1.84 HYPER 4a 6.16 15.98 15.72 NEAREST 4.81 4.74 4.88 TILES 4.53 4.65 4.48 BILINEAR 1.55 1.47 1.45 HYPER *** With mediaLib SCALE ===== 3 4 4a 3 89.21 127.45 66.89 NEAREST 65.84 20.31 51.51 TILES 65.26 33.86 60.08 BILINEAR 25.95 21.56 34.51 HYPER 4 191.22 186.50 186.48 NEAREST 56.59 70.64 66.33 TILES 32.86 70.99 70.34 BILINEAR 19.93 45.89 30.67 HYPER 4a -1.00 -1.00 189.18 NEAREST -1.00 -1.00 34.29 TILES -1.00 -1.00 54.94 BILINEAR -1.00 -1.00 30.01 HYPER COMPOSITE ========= 3 4 4a 3 90.26 70.18 70.12 NEAREST 62.97 52.06 51.76 TILES 65.80 59.73 58.67 BILINEAR 14.47 36.55 35.45 HYPER 4 186.81 182.73 184.32 NEAREST 51.72 66.90 25.26 TILES 20.79 75.04 29.29 BILINEAR 20.36 45.28 16.85 HYPER 4a 30.59 27.80 60.25 NEAREST 38.42 36.50 12.71 TILES 37.31 24.95 39.88 BILINEAR 11.12 11.37 8.97 HYPER COMPOSITE_COLOR =============== 3 4 4a 3 47.55 24.96 44.65 NEAREST 7.66 5.90 5.81 TILES 16.61 6.60 6.08 BILINEAR 2.15 2.04 1.85 HYPER 4 77.09 89.98 89.76 NEAREST 5.59 5.77 6.69 TILES 5.37 6.04 6.59 BILINEAR 1.98 1.87 1.83 HYPER 4a 13.82 15.68 16.02 NEAREST 4.52 4.66 4.54 TILES 4.62 4.49 4.66 BILINEAR 1.56 1.56 1.58 HYPER
The above test was run on Solaris x86 with SSE2 architecture, by the way, running GNOME 2.17 with GTK+ version 2.10.6
Eventually the pixop could should be merged with pixman, and gdk-pixbuf should use cairo, right? Just checking.
Created attachment 79732 [details] [review] updated patch This patch, I think, addresses all the issues mentioned above. Now all USE_MEDIALIB references in the gdk-pixbuf code are in the pixops directory, and the composite_0888 function is separated into composite_0888_medialib for medialib use. I also cleaned up a number of issues (coding style, indentation, etc.) with the patch and think this makes things easier to read.
Note that Sun recently announced that Solaris (including mediaLib) will be released under the GPL license (I'm sure LGPL for mediaLib) after GPLv3 is finished. So, in the near future, the mediaLib code will be available for all users even if the library isn't "on the system". http://www.eweek.com/article2/0,1895,2084284,00.asp?kc=EWEWEMNL011507EP28A
Looks ok in general. Two more comments: a) Please add license and copyright information to the new source files b) Is there a specific reason to disable all or parts of the medialib support at runtime via envvars ? We don't normally add random toggles like that. If this was just for debugging, I'd prefer to strip it out before merging the patch. If there is a good reason for keeping these envvars, they need to be documented.
Created attachment 81501 [details] [review] updated patch Here is an updated patch that addresses the following: 1) Now gdk/medialib.[ch] has a proper header section 2) Now use DISABLE_MEDIALIB for disabiling mediaLib in all places rather than having separate environment variables for each use case. 3) configure.ac has been updated to check for mediaLib 2.2 and 2.5 4) if mediaLib 2.5 has been found, then we also tune gdk/gdkrgb.c function gdk_rgb_convert_0888 which also shows to be a function that consumes a lot of CPU in many normal use cases. In your last comment you ask if it makes sense to keep the environment variable that allows you to disable MEDIALIB. I think it adds some value since some users might not want to use it. Using performance turning may introduce off-by-one/rounding differences in some computations so some users may prefer using the slower non-tuned functions if they want pixel perfect results (even after updating to a new mediaLib library). Also, if a crasher problem is found with using mediaLib in some cases, the environment variable gives the user a way to fallback to the slower code. It's also useful for testing/comparing performance with and without mediaLib. I'm happy to update the GTK documentation to mention mediaLib. It probably should say something like: --- The gdk-pixbuf and GDK libraries can be built to make use of the mediaLib performance library, if available on the system (e.g. Solaris). With mediaLib support enabled, several common functions will make use of hardware acceleration. Accelerated functions include scaling, compositing, blending, and RGB conversion operations. mediaLib supports hardware acceleration for most popular hardware, including MMX/SSE/SSE2 (Intel) and VIS/VIS2 (Sparc). If desired, mediaLib support can be turned off by setting the DISABLE_MEDIALIB environment variable. When mediaLib is not used, the gdk-pixbuf library supports MMX assembly code which improves the performance of scaling and composite operations on systems which support MMX. --- However, I'm unsure where in the docs this should be mentioned. Since mediaLib is supported in both GDK and gdk-pixbuf libraries, I'm guesisng it should be mentioned in the gtk-docs, perhaps on a page that mentions some general information about the library. Any suggestions of where this should go? If you tell me where you think this documentation update should go, then I'll happily provide a patch for the file. Or feel free to paste the above couple of paragraphs into the appropriate place in the docs.
Created attachment 82440 [details] [review] updated patch Updated patch to fix a few issues: 1) In the GDK library we were calling the initialization function every time gdk_rgb_convert_0888_medialib or composite_0888_medialib was called, not just the first time. Now set the flags properly. This makes the performance much better. 2) The pixops and GDK mediaLib init functions were doing some work to verify that the mediaLib version is appropriate. This is not necessary since configure only builds mediaLib if the right version of mediaLib is present. This cruft was still in the patch because a long time ago mediaLib wasn't integrated into Solaris and users had to install it separately - so the orginal patch used dlopen/dlsym to only use mediaLib functions if the library were present. The dlopen/dlsym logic was removed long ago but the init functions were still checking the mediaLib version. This isn't a huge deal, but should make initialization a bit faster and removes this useless checking. 3) Made the comments in the init function a bit nicer and a bit of code cleanup there.
Created attachment 82510 [details] [review] updated patch Sorry for updating the patch again, but this version has a few fixes. I think I'm now pretty happy with this patch and I think it is ready to go upstream if the maintainers don't have further comments. Changes include: 1) Change the environment variable to disable mediaLib from DISABLE_MEDIALIB to GTK_DISABLE_MEDIALIB since this is a bit better, I think. Makes it clear that it only disables mediaLib for libraries built from the GTK module, rather than disabiling mediaLib overall. 2) I fixed gdkrgb.c and gdkdraw.c so that we check to see if mediaLib should be used just once when we set the function to use rather than each time the function is called. This makes the patch cleaner and faster.
Created attachment 82511 [details] patch with proposed documentation Earlier you said documentation would be needed before the patch can go in. Based on my earlier suggestion, I put the attached docs together. This is a separate patch since if you have comments about them, it will be easier for me to update this patch separately. Hopefully this now meets all the requirements, and can go upstream?
Prefixing the variable with GDK may be a slightly better match, since it is used in gdk and gdk-pixbuf, not in gtk. Regarding the documentation, I'm not sure it makes a lot of sense to speak about pixel-perfect there, the results of the mmx code are not perfect either.... It is a bit unfortunate that there is a large number of line-breaking/whitespace changes mixed in the patch, makes it much harder to see the actual changes.
Created attachment 82671 [details] [review] updated patch Here is an updated patch that uses GDK_DISABLE_PIXBUF. I also made another minor change: 1) Now we use mlib_ImageSetStruct instead of using mlib_ImageSet and mlib_ImageSetSubimage. This change was made because in reviewing the patch with the mediaLib team they highlighted that the previously used functions were undocumented and they recommended using the ImageSet function. This function was added to mediaLib 2.3 so the configure script now checks for this function (previously it was looking for a mediaLib 2.2 function). This is acceptable since the latest update release of Solaris 10 and Nevada releases use mediaLib 2.4 and these are the only platforms that mediaLib GTK+ integration will be supported. Now the mediaLib team gives their thumbs up for this patch and I've tested it and am happy with it. I'm sorry that the patch is big. I know I made some additional changes to gdk_pixbuf/pixops.c to make the spacing a bit more consistant and easier to read. This corresponds to lines 914-1134. But these chunks should not be so hard to review since it should be clear that I just added line breaks to avoid the lines being so long. Most of the changes in pixops.c are due to the fact that the _pixops_scale, _pixops_composite, and _pixops_composite_color functions changed their arguments and now call the *_real corresponding function...but this was needed in order to hide the MEDIALIB code in pixops.c as you requested earlier. Perhaps I didn't need to modify _pixops_composite_color arguments since we don't have mediaLib integration for this function - but it seems to make sense for the args for the three entry functions to be consistent. In a bit, I will add a docs patch to reflect your comments for the docs.
Created attachment 82673 [details] [review] updated docs patch This docs patch is updated to reference GDK_DISABLE_MEDIALIB and no longer mentions off-by-one issues.
Created attachment 83449 [details] [review] updated patch This patch has two minor changes caught by James Cheng of the mediaLib team. 1) The comment in configure.in now says "mediaLib" instead of "MediaLib" since "mediaLib" is the correct name. 2) There are a two places in the patch where you will see "#ifdef (__sun)". One of them was "#ifdef (sun)". Now fixed so both references say "#ifdef (__sun)" which is better to use.
(In reply to comment #11) > Note that Sun recently announced that Solaris (including mediaLib) will be > released under the GPL license (I'm sure LGPL for mediaLib) after GPLv3 is > finished. So, in the near future, the mediaLib code will be available for > all users even if the library isn't "on the system". > > http://www.eweek.com/article2/0,1895,2084284,00.asp?kc=EWEWEMNL011507EP28A > If only this were as exciting as it sounds :( The version of mediaLib that you (Brian) used to get the nice speedup is _not_ (yet) free software. The version of mediaLib that was released under the CDDL is the pure C version. Which, for GNOME and Cairo purposes, isn't really interesting as we already have some SIMD code that would probably outperform the pure C version of mediaLib. I hope I'm wrong. Or maybe Sun is just waiting a little before releasing the SIMD version?
Here is a comment from James Cheng of the mediaLib team. So you should be happy to know that it sounds like they are planning on releasing the VIS and SSE2 versions under CDDL sometime soon. Note mediaLib 2.5 should come out in the next month or so. It is true that we just released the C version mediaLib under CDDL last time. It was mainly for logistical reasons. For the SSE2 version, there was the issue of compiler (that the Sun C compiler before Mars couldn't handle SSE2 code), and most functions were not well tuned with SSE2 as they were with VIS. With the upcoming mediaLib 2.5 release, the disparity is going to be reduced or even reversed. It is definitely possible for us to release the VIS and SSE2 versions under CDDL at some point. It seems to me that some time after the mediaLib 2.5 release is a good time for doing so.
No comments on this bug report for about a month. I did quite a bit of work cleaning up the patch as requested. I can do more if you need, though I think the patch is in reasonable shape. I think the churn is over since we've now soaked the patch for some time and no more issues.
Yeah, sorry. Unfortunately, I'm about to vanish on a 10 day trip, so I have to ask for some more patience.
Ping - can this go upstream. I hope you had a great trip, but I assume you are back now...
Sorry for taking so long to get back to this. Almost ready to go in, I'd say. Some more small comments though: 1) I'm not really convinced of the usefulness of the envvar toggle, but I am willing to let this slip. 2) I really don't want to see #ifdef (__sun) or #ifdef (__linux__), this should be done by autoconf checks and HAVE_XXX_H macros. 3) Please use G_BEGIN/END_DECLS for +#ifdef __cplusplus +extern "C" { +#endif /* __cplusplus */ If you correct 2) and 3), the patch can go in.
Created attachment 88142 [details] [review] patch to address issues above This patch addresses the issues you asked me to fix. This patch applies against the previous patch. It seemed easier to do it this way since the original patch is large and this should also make it easier to review the changes I made. I moved the config.h include from gdkmedialib.h to gdkmedialib.c, so it could more easily find the new #ifdef's added. Also removed the unneeded "link.h" include. Aside from this, should be straightforward.
Thanks, looks ok to me now.
Okay, do you want me to commit, or will you?
Please do
Committed, thanks. Closing this bug as Fixed.
Note, refer to bug #442888 for a minor problem with this and a patch to fix it. I created a new bug report rather than reopening this one since this has too many comments and patch attachments already.