On Thu, 19 Feb 2004 16:00:08 +0100 Sven Neumann <[EMAIL PROTECTED]> wrote:
> Hi, > > Ernst Lippe <[EMAIL PROTECTED]> writes: > > > The decision if the plug-in "needs" the temporary drawable > > is made by its designer and it is certainly not forced by > > the preview. It is perfectly possible to draw a row of > > pixels on the preview (indeed for a very long period that > > has been the only way you could draw on the preview, support > > for drawables is a recent addition). If a plug-in designer > > writes a plug-in in such a way that it can only output to > > a drawable, then it will always have to allocate a temporary > > drawable (unless you want to mess around with the original > > input drawable, which sound pretty horrible). > > But when the internal datastructures for the algorithm > > can be used to construct a single row for the preview > > there is no reason to use a temporary drawable. > > So you are seeing the GimpPreview as just a widget that plug-ins can > draw on. Basically, yes. > 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. If you would have proposed it as a requirement at that stage I would have rejected it, because I simply think that it is infeasible as it stands. 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. A few points: * How do you detect that parameters have been changed? Users will expect that the preview gets updated when they change some parameters. Many plug-ins do not need to detect this at the moment because the values only become relevant when the user presses the OK button. When rewriting such a plug-in you will have to add modification detection code, and you will also have to modify the code that collects the current values of the parameters and runs the underlying algorithm with these parameters. Some plug-ins completely mix their business logic with their GUI and in these cases it might require quite a bit of work to fix this. * Current plug-ins assume that they can write the results back to the input drawable. So you will either need to coerce to write back their results somewhere else or you will have to mess around with the original drawable (probably some hidden global "preview-mode" switch). * Current plug-ins are not very flexible in the area that they render. Some always generate the entire drawable, and others generate the area from gimp_drawable_mask_bounds. For the latter category you could of course think of a hack that changes the output of gimp_drawable_mask_bounds based on some hidden global "preview-mode" switch, but then you will also have to consider the consequence of this decision for all other places where this function may be used, not to mention the fact that the whole idea of global mode switches is inherently ugly. So I don't think that should try to find some solution without rewriting, but to find some framework that could easily fit a large set of the current plug-ins. > The change should be > limited to the plug-in dialog and a few hooks here and there. Unless there have been some major changes to the GIMP plug-ins since I last looked at them, I believe that this idea is not very realistic. > Your > preview widget could be part of this infrastructure since it provides > a way to draw pixels to the screen, but in its current state, it > certainly doesn't meet the goals I outlined above. > > > One of the design requirements of the preview is to provide a > > > convenient way for plug-ins to preview their results. Convenient means > > > that it should be possible to reuse the existing pixel manipulations > > > routines without or with small changes. Allocating temporary drawables > > > in the core is in my opinion not a viable solution for this problem. > > > > > > It should however be possible to keep the temporary drawables > > > completely on the plug-in side. To achieve this, the GimpDrawable > > > wrapper would have to know whether it needs to get the tiles from the > > > core (via shared memory or over the pipe) or allocate them in the > > > plug-in process. I think this change would be rather straight-forward. > > > > But unless I am mistaken, this temporary drawable has only > > a limited functionality, because you cannot use any of > > the "core" operations on it. > > So I am not really certain if it would be worth the effort. > > 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? > > Furthermore, this functionality would only be needed > > for those plug-in algorithms where it is difficult > > to compute an entire row of pixels, and actually > > I cannot remember that I have ever seen one. > > So if you have some good examples, perhaps the problem > > could also be solved by changing the interface of the > > preview, e.g. adding support for drawing tiles on the > > preview. > > 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. > If our preview widget encourages the > plug-in author to work on a row-by-row basis, then we will never get > plug-ins that operate on the pixels in the most efficient way. Like I said, I don't object to adding a tile-based interface as well, it is just that I have not yet seen any plug-in that needed it. 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). So what is the simplest solution for the plug-ins? 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, and it can't see how it could work for plug-ins that invoke any of the core painting tools or other plug-ins. So on the face of it this approach does not look very promising. 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. 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. Another problem for which I don't see an easy solution is how many tiles should be cached for the drawable. After all when you are worried about performance you are probably not very happy when the temporary drawable has the same number of tiles as the original image. A possible solution is to allocate only a single fixed tile for the entire temporary drawable, but in that case this solution is only usable for plug-in algorithms that can compute individual tiles in a single pass. I believe that such plug-ins are only a very small fraction of the total set of existing plug-ins. So, unless you consider it OK to keep all tiles for the temporary drawable in core, it seems that there must be some additional function call to "commit" a certain tile. At this moment I don't see an easy method that the cache manager could use to determine this, a possible solution would be to use gimp_tile_unref, but then you would have to be very careful not to call this function inside the other pixel region routines, e.g. during a gimp_pixel_rgn_get_row. 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. Still I am not very convinced that it is really worth the effort. There are only very few tile-based plug-ins for which "temporary drawable" might be the best approach. A large number of the plug-ins generate data on a row basis and for these plug-ins it might be both simpler and faster (no tile overhead) to simply write the generated rows to the preview. So you are completely correct when you say that preview plug-in does not solve all of your plug-in problems. 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. Ernst Lippe _______________________________________________ Gimp-developer mailing list [EMAIL PROTECTED] http://lists.xcf.berkeley.edu/mailman/listinfo/gimp-developer
