Hi, Ernst Lippe <ernstl@xxxxxxxxx> writes: > > However our goal is to provide a way to add a preview to > > plug-ins basically w/o changing their code. > > It is an interesting idea, but when you look at the > requirements for the preview as they were discussed last > year (see http://refocus.sourceforge.com/preview/requirements.html) > it was not part of the list. I am pretty sure this point was mentioned. IIRC it has already been discussed on GimpCon 2000 and I think it's the basic and most important design requirement. > There are several point that you will virtually always have > to change when you want to add a preview to an existing > plug-in, and in some cases (some plug-ins have pretty messy > code) these changes have to be rather drastic. Without going into much details of your answer, it is clear that you completely missed my point. Of course there are some changes needed to the plug-in code. How large the changes are depends on the quality of the code, mainly in the user interface parts of it. When I said that only few changes should be needed when adding a preview, I was assuming an otherwise well-designed and well-written plug-in. The point I was trying to make is that the actual image manipulation algorithm should not have to be touched. For the code that operates on the actual pixels, there must not be a difference between rendering the result or rendering the preview. This means that this routine can always work on a GimpDrawable and doesn't need to care if this struct wraps a real drawable or just some preview data. The preview doesn't even need to be tile-based (I think you said it would have to be). Plug-ins usually don't see any tiles, they work on pixel regions and that's exactly the API that they should be using when rendering a preview. > > I don't see how core operations would be useful at all. The only > > use of the temporary drawable is to allow the preview to be > > rendered using the same GimpPixelRgn routines that are used to > > render the final result. > > Uh, are you saying here that we can simply ignore those plug-ins > that call any of the core tool operations or other plug-ins? Yes. I think this is a safe assumption for the general case. Not too many plug-ins call other plug-ins. In the GIMP source tree there is not a single one that would need a preview and calls other plug-ins. > > Basically any plug-in that works on pixel rows is less than > > optimal and should be rewritten at some point. Good plug-ins are > > supposed to work on tiles rather than rows. > > I am afraid that with this definition there are very few "good" > plug-ins. That is correct, but if we provide a preview API that encourages to write good plug-ins, this number will increase. If our preview API is modeled after the bad sort of plug-ins, we will never get a reasonable codebase in the plug-ins directory. Of course my proposal also allows for a row-based algorithm since GimpPixelRgn provides that as well. > Now obviously you have a problem with having to allocate temporary > drawables in the GIMP core just for the sake of previewing. > > But what is the problem? Is it just a performance issue? As far as I > have seen, it is not a serious issue, I have a pretty slow computer > and the performance of the preview is quite acceptable. In most cases > the drawables are pretty small, much smaller than the real images. In > general I am pretty skeptical about design decisions that sacrifice > simplicity for performance's sake, in the long term that is generally > a bad strategy (yes there are some cases where it is a correct > decision but you should always analyze them pretty carefully). Performance is not the primary concern here. The problem is that if we encourages plug-ins to allocate temporary drawables in the core, we would have to add a framework that makes sure that these drawables aren't leaked. One of the main reasons to keep plug-ins as separate processes is to ensure that a badly written plug-in doesn't make the GIMP core leak memory. I am pretty sure that temporary drawables created by plug-ins will eat up our tile-cache quite fast. In the long run we will have to improve how resources allocated by plug-ins are handled in the core. The core should be able to undo all operations in case a plug-in crashes and it should free all floating resources when a plug-in finishes it's job. If such a framework is in place, we can easily change the preview code to optionally allow a preview in the image window. The plug-in will then simply work on a core drawable and this can happen completely transparently. > Most existing plug-ins will write their output only to their input > drawable. But for the preview we don't want to modify the original > drawable, but the plug-in must somehow be coerced to write its output > to something that the preview could read. If you really don't want to > change the plug-ins algorithm at all the only solution is to somehow > intercept the modifications to the original drawable and route them to > the plug-in. This appears to be a different solution than you > proposed, and I think that it would be very messy, you would have to > change the functions that operate on the drawable to check if they > should operate on the real drawable or that they should send their > result to some other structure that could be used with the plug-in. > Then the plug-in would have to somehow set a switch that is associated > with this drawable to indicate if it is in "preview-mode" or not. This > entire approach looks pretty messy. What is messy about this approach? If we implement this in libgimp, the plug-in will not see much about it. It will ask for a preview drawable and either work on that or on the original drawable. Large parts of the plug-in will not even be able to tell the difference. To me this seems to be a rather elegant approach. > Another approach is to rewrite each plug-in so that it can use an > output data-structure that is different from the input drawable. The > most obvious solution is to give the plug-in a second output drawable > that could be different from the input drawable. No, that would mean making changes to the plug-in algorithm which is exactly what I don't want to see happening. I don't understand why an output drawable would be necessary at all. > For most plug-ins that I have seen this is a fairly easy > modification. I guess that you have also been thinking along these > lines. So here is were your proposal comes in. My first objection > against it, is that it appears to break the existing semantics of a > drawable because most operations that are valid for a drawable are > not valid for a temporary drawable. Also this makes the existing > code more complicated, the cache manager must also be able to handle > tiles that are never sent to the GIMP core, and there must be some > kind of error handling for unsupported operations on these temporary > drawables. I don't know how complicated this would be but it > certainly won't make the existing code more simple. The existing code in libgimp is darn simple. I am not afraid of making it a bit more complex. > A slightly different solution for the temporary drawable problem > would be to define a new object that can handle both rendering to > the tiles of a drawable as well as rendering to some local set of > tiles. Essentially this object would have an interface that is very > similar to the pixel-region interface. I have implemented something > similar for my refocus plug-in and I consider it more elegant that > changing the existing semantics of a drawable. After all, in cases > where you could successfully use temporary drawable, you are only > using a very limited part of the drawable functionality, effectively > you are assuming that a drawable is simply a collection of tiles. Of > course the plug-ins must be rewritten so that they use the new > function calls instead of the old pixel region functions, but it > seems that you could easily achieve most modifications with a simple > textual substitution. I don't see why this goal cannot be achieved using the existing pixel-region interface. > But I remain quite skeptical of your implicit suggestion that the > overhead due to using a real drawable is important. > When you look at the total sequence of events that is normally used > to generate a new content of the preview there are quite > a number of steps: > * X generates a mouse event. > * The mouse event gets routed to GTK (involves at least one process swap, > I am not completely certain if the window manager is involved > as well). > * GTK processes the event and routes it to the preview. > * Preview allocates a new drawable (actually it could > reuse the previous drawable when it has the same size). > * Preview calls the plug-in to render a new image. > * Preview draws the image on the pixbuf. > * GTK draws the pixbuf on the screen via X. > So when you look at the whole process I really doubt > that the drawable is in general a serious bottle-neck. > Of course, I can be convinced otherwise if you could > show me some good timing measurements. As I said before, I don't think performance is the problem. Sven
