From: Hezekiah M. C. <hc...@at...> - 2008-03-18 18:18:35
Attachments:
plimagefr_pltr_support_nofastimg.diff
|
Attached is a patch which adds coordinate transform support to plimagefr, similar to what is available in the plshade*, plcont, plmap and plmeridians functions. This patch is rather large, mainly because it removes a lot of code. - The dev_fastimg rendering path has been removed. This was only used [1] in the xwin driver, and does not work well with the custom coordinate transform support added to plimagefr. - The plimage functions have been restructured and hopefully simplified in this process. - An off-by-one bug was fixed during the code restructuring, which would sometimes lead to a missing row and/or column of (scaled) pixels in the plotted image. - api.xml has been updated (hopefully correctly) to include the addition of pltr and pltr_data parameters to plimagefr - The interface to plimage has remained the same, only plimagefr and related internal functions have been altered [1] - There one file which I have found which references dev_fastimg still: sys/win32/msdev/src/win3.cpp - I have not changed this as I have no way to test the result. It should just be a matter of removing the appropriate line. Similarly, examples/python/qplplot.py calls plplot.plNoBufferNoPixmap() which looks like it calls one of the removed functions from src/plimage.c. This function was removed as its only purpose, as far as I can tell, was part of the fastimg rendering path. This patch should apply cleanly against the latest PLplot SVN. It builds cleanly on my Debian Sid system building the C library and the Cairo rendering devices. C example 20 works as expected for xwin, xcairo and pngcairo. I realize that this is a large patch. I can break it down in to pieces if needed, but this is a complete patch as-is for the C portion of PLplot. None of the included language bindings have been altered. Thank you for your time. Hez -- Hezekiah M. Carty Graduate Research Assistant University of Maryland Department of Atmospheric and Oceanic Science |
From: Alan W. I. <ir...@be...> - 2008-03-18 20:32:23
|
On 2008-03-18 14:18-0400 Hezekiah M. Carty wrote: > Attached is a patch which adds coordinate transform support to > plimagefr, similar to what is available in the plshade*, plcont, plmap > and plmeridians functions. > > This patch is rather large, mainly because it removes a lot of code. > > - The dev_fastimg rendering path has been removed. This was only used > [1] in the xwin driver, and does not work well with the custom > coordinate transform support added to plimagefr. > - The plimage functions have been restructured and hopefully > simplified in this process. > - An off-by-one bug was fixed during the code restructuring, which > would sometimes lead to a missing row and/or column of (scaled) pixels > in the plotted image. > - api.xml has been updated (hopefully correctly) to include the > addition of pltr and pltr_data parameters to plimagefr > - The interface to plimage has remained the same, only plimagefr and > related internal functions have been altered > > [1] - There one file which I have found which references dev_fastimg > still: sys/win32/msdev/src/win3.cpp - I have not changed this as I > have no way to test the result. It should just be a matter of > removing the appropriate line. Similarly, examples/python/qplplot.py > calls plplot.plNoBufferNoPixmap() which looks like it calls one of the > removed functions from src/plimage.c. This function was removed as > its only purpose, as far as I can tell, was part of the fastimg > rendering path. > > This patch should apply cleanly against the latest PLplot SVN. It > builds cleanly on my Debian Sid system building the C library and the > Cairo rendering devices. C example 20 works as expected for xwin, > xcairo and pngcairo. > > I realize that this is a large patch. I can break it down in to > pieces if needed, but this is a complete patch as-is for the C portion > of PLplot. None of the included language bindings have been altered. > Thanks Hez. I am not competent enough with driver/core C code to comment on the specifics of your patch, but I would like to make some general comments. (1) sys/win32/msdev/src/win3.cpp is no longer maintained or used so you don't have to worry about it. To give you the historical background, it is part of svn for now so that people can study that historical windows driver code, but we no longer use it. In fact all of sys/win32 is excluded from our release tarballs since the new CMake build system (which works well on Windows) supersedes that old hand-crafted Windows-only build system, and there are a number of devices that work on Windows now, including the windows-only drivers/wingcc.c device driver which supersedes sys/win32/msdev/src/win3.cpp. (2) It's a fact (and not criticism) that your patch hits a substantial fraction of the areas of PLplot (e.g., drivers/xwin.c, src/plimage.c, examples/python/qplplot.py) which currently do not have much core developer support for a variety of reasons. Developers have lost some interest in drivers/xwin.c because the fonts currently look bad, and nobody has cared enough to fix that so far. src/plimage.c was donated by core developer Joao Cardoso who has not been heard from for several years now. examples/python/qplplot.py was donated by an external developer who did not maintain it afterwards. So I think we all appreciate your interest in plimage.c and want to encourage it. That said, I think you should not remove the dev_fastimg rendering path unless we find that rendering path really does not provide much of a speed increase. The reason I am concerned is Joao Cardoso introduced that rendering path (IIRC) because he was not satisfied with the speed of the example 20 results for our premier device at that time (early 2000's), xwin.c. Now, computers are much more powerful so speed is not as important an issue, but nevertheless, fast results are probably something we should not give up lightly and which we should positively encourage for the new devices which in other respects (such as font handling) are superseding xwin.c. Currently, you have stated above that that the dev_fastimg rendering path does not work well with the custom coordinate transform support added to plimagefr. I encourage you to fix that issue (assuming dev_fastimg rendering really is faster for xwin.c). I realize that is probably a lot of work, but it does make your patch much less obtrusive and prepares necessary infrastructure to propagate the dev_fastimg rendering to other devices. (3) I suggest you add a page or more to x20c.c to show an example (or examples) of how to use the new features you have added for plimagefr. The reason I suggest that change is new PLplot API that is not illustrated with one of the examples tends to go unused and untested by our developers and users. Of course, keep the old x20c.c pages as well which show plimage examples. Thanks again for your interest in improving PLplot. Alan __________________________ Alan W. Irwin Astronomical research affiliation with Department of Physics and Astronomy, University of Victoria (astrowww.phys.uvic.ca). Programming affiliations with the FreeEOS equation-of-state implementation for stellar interiors (freeeos.sf.net); PLplot scientific plotting software package (plplot.org); the libLASi project (unifont.org/lasi); the Loads of Linux Links project (loll.sf.net); and the Linux Brochure Project (lbproject.sf.net). __________________________ Linux-powered Science __________________________ |
From: Hezekiah M. C. <hc...@at...> - 2008-03-19 15:00:00
|
I forgot to CC the list... On Tue, Mar 18, 2008 at 4:32 PM, Alan W. Irwin <ir...@be...> wrote: > Thanks Hez. > > I am not competent enough with driver/core C code to comment on the > specifics of your patch, but I would like to make some general comments. Thanks for the feedback Alan. I am learning more and more about the PLplot internals as I go along, whether I want to or not :-) The changes I have submitted so far are important for my plotting needs, and I hope that they will be useful to others as well once they are complete. > (1) sys/win32/msdev/src/win3.cpp is no longer maintained or used so you > don't have to worry about it. To give you the historical background, it is > part of svn for now so that people can study that historical windows driver > code, but we no longer use it. In fact all of sys/win32 is excluded from > our release tarballs since the new CMake build system (which works well on > Windows) supersedes that old hand-crafted Windows-only build system, and > there are a number of devices that work on Windows now, including the > windows-only drivers/wingcc.c device driver which supersedes > sys/win32/msdev/src/win3.cpp. Sounds good - I will ignore that portion of the source tree for the purposes of this patch. > (2) It's a fact (and not criticism) that your patch hits a substantial > fraction of the areas of PLplot (e.g., drivers/xwin.c, src/plimage.c, > examples/python/qplplot.py) which currently do not have much core developer > support for a variety of reasons. Developers have lost some interest in > drivers/xwin.c because the fonts currently look bad, and nobody has cared > enough to fix that so far. src/plimage.c was donated by core developer Joao > Cardoso who has not been heard from for several years now. > examples/python/qplplot.py was donated by an external developer who did not > maintain it afterwards. So I think we all appreciate your interest in > plimage.c and want to encourage it. > > That said, I think you should not remove the dev_fastimg rendering path > unless we find that rendering path really does not provide much of a speed > increase. The reason I am concerned is Joao Cardoso introduced that > rendering path (IIRC) because he was not satisfied with the speed of the > example 20 results for our premier device at that time (early 2000's), > xwin.c. Now, computers are much more powerful so speed is not as important > an issue, but nevertheless, fast results are probably something we should > not give up lightly and which we should positively encourage for the new > devices which in other respects (such as font handling) are superseding > xwin.c. Currently, you have stated above that that the dev_fastimg > rendering path does not work well with the custom coordinate transform > support added to plimagefr. I encourage you to fix that issue (assuming > dev_fastimg rendering really is faster for xwin.c). I realize that is > probably a lot of work, but it does make your patch much less obtrusive and > prepares necessary infrastructure to propagate the dev_fastimg rendering to > other devices. dev_fastimg is definitely faster than the slow rendering path for the xwin driver. I tested example 20 with both 5.9.0 and plplot-svn+my patch, and (using xwin) 5.9.0 is certainly less CPU intensive. That said, from what I understand reading the xwin.c code, dev_fastimg assumes that the image is made up of homogeneously sized unrotated rectangular boxes. So while I think it could be adapted for non-transformed images, it would not work for images with most coordinate transforms. The patch I submitted does all of the drawing with plfill, which makes plimage much simpler and more flexible, and unfortunately noticeably slower. For the time being, would you accept a patch with the dev_fastimg rendering path code left in place, but unused? I would comment out the code in a few places, and leave it untouched but unused in others. The changes required to keep it in use for cases where it will be of use will take me a while as I will have to get to know the PLplot internals better. My initial move to use dev_fastimg would be for non-transformed images only. So calls to plimage would use the dev_fastimg path when available, but calling plimagefr directly would only use dev_fastimg if the pltr callback is NULL. Otherwise plfill would still be used for each transformed pixel. To sum up, I would like to submit patches in the follow steps: (1) Add coordinate transform to plimagefr and disable the dev_fastimg rendering path, but without removing the dev_fastimg code. (2) Update dev_fastimg to work with the updated plimagefr, but only use it for non-transformed images. (3) Update example 20 with some examples of what plimagefr can do, with pages to illustrate both fixed color ranges and coordinate transformations. Does this sound like a reasonable compromise? Taking this on over a few steps would be much easier for me since it breaks the work up in to smaller chunks, which in turn makes tracking plplot-svn simpler. Step (3) could come before step (2), and perhaps should for testing and verification purposes to make sure dev_fastimg works as expected. > (3) I suggest you add a page or more to x20c.c to show an example (or > examples) of how to use the new features you have added for plimagefr. The > reason I suggest that change is new PLplot API that is not illustrated with > one of the examples tends to go unused and untested by our developers and > users. Of course, keep the old x20c.c pages as well which show plimage > examples. I will certainly add one or more pages to example 20, though I would again prefer this as a separate step once at least the initial patch is in PLplot. I have some very simple test code that I use now, but it is mostly in OCaml to make sure I am keeping the interfaces in sync. > Thanks again for your interest in improving PLplot. > > Alan I am very happy to be able to be able to provide improvements where I am able. PLplot has been a pleasure to work with, particularly how open you (the core devs) have been to working with submitted patches and improvements. Hez -- Hezekiah M. Carty Graduate Research Assistant University of Maryland Department of Atmospheric and Oceanic Science |
From: Alan W. I. <ir...@be...> - 2008-03-19 17:15:24
|
On 2008-03-19 10:59-0400 Hezekiah M. Carty wrote: > To sum up, I would like to submit patches in the follow steps: > (1) Add coordinate transform to plimagefr and disable the dev_fastimg > rendering path, but without removing the dev_fastimg code. > (2) Update dev_fastimg to work with the updated plimagefr, but only > use it for non-transformed images. > (3) Update example 20 with some examples of what plimagefr can do, > with pages to illustrate both fixed color ranges and coordinate > transformations. > > Does this sound like a reasonable compromise? Hi Hez: Actually after sleeping on it, I am leaning toward saying do (1) (with code commentary where you do the disabling in plimage.c, xwin.c, etc., about why it was necessary) and leave (2) as a would-be-nice rather than a requirement since it sounds like it might be a lot of work which you could more productively spend on the OCaml bindings, for example. However, I don't feel right making this decision alone because I haven't used -dev xwin or the plimage capability for my own PLplot needs, and somebody who has more of a vested interest in those parts of PLplot may feel a lot stronger about their speed than I do. Thus, I am going to need advice/help from the other PLplot core developers on the decision about (1) and (2) so please step forward, guys, and comment. (3) sounds good! Alan __________________________ Alan W. Irwin Astronomical research affiliation with Department of Physics and Astronomy, University of Victoria (astrowww.phys.uvic.ca). Programming affiliations with the FreeEOS equation-of-state implementation for stellar interiors (freeeos.sf.net); PLplot scientific plotting software package (plplot.org); the libLASi project (unifont.org/lasi); the Loads of Linux Links project (loll.sf.net); and the Linux Brochure Project (lbproject.sf.net). __________________________ Linux-powered Science __________________________ |
From: Maurice L. <mj...@br...> - 2008-03-19 18:12:34
|
On Wednesday, March 19, 2008 at 10:14:30 (-0700) Alan W. Irwin writes: > On 2008-03-19 10:59-0400 Hezekiah M. Carty wrote: > > > To sum up, I would like to submit patches in the follow steps: > > (1) Add coordinate transform to plimagefr and disable the dev_fastimg > > rendering path, but without removing the dev_fastimg code. > > (2) Update dev_fastimg to work with the updated plimagefr, but only > > use it for non-transformed images. > > (3) Update example 20 with some examples of what plimagefr can do, > > with pages to illustrate both fixed color ranges and coordinate > > transformations. > > > > Does this sound like a reasonable compromise? > > Hi Hez: > > Actually after sleeping on it, I am leaning toward saying do (1) (with code > commentary where you do the disabling in plimage.c, xwin.c, etc., about why > it was necessary) and leave (2) as a would-be-nice rather than a requirement > since it sounds like it might be a lot of work which you could more > productively spend on the OCaml bindings, for example. > > However, I don't feel right making this decision alone because I haven't > used -dev xwin or the plimage capability for my own PLplot needs, and > somebody who has more of a vested interest in those parts of PLplot may feel > a lot stronger about their speed than I do. Thus, I am going to need > advice/help from the other PLplot core developers on the decision about (1) > and (2) so please step forward, guys, and comment. I use -dev xwin extensively (and its client, plframe) but not plimage. That said, doing (1) and leaving/documenting (2) as a nice-to-have sounds fine to me, unless someone can make a case otherwise. Ideally someone would play with dev_fastimg vs !dev_fastimg and see if there's a noticeable difference on mondern hardware. I'm sure many here have seen hardware advances make irrelevant some "optimizations" done previously, or at least mitigate performance concerns. For example, the X driver was first developed on 8-bit r/w color displays and sharing a single r/w colormap was the norm. If this didn't suffice for the application, a custom colormap could be used (which I never liked very much). Seems so quaint now. :) But when I went to 16 or 24 bit r/o colormaps on a Linux box years later, the performance degradation of swapping out colors really didn't seem to matter much. One of these days I'd like to give the xwin driver a bit of housecleaning, starting with chopping out the custom colormap support that was never really used anyway. -- Maurice LeBrun |
From: Andrew R. <and...@us...> - 2008-03-19 18:32:11
|
On Wed, Mar 19, 2008 at 01:12:24PM -0500, Maurice LeBrun wrote: > On Wednesday, March 19, 2008 at 10:14:30 (-0700) Alan W. Irwin writes: > > On 2008-03-19 10:59-0400 Hezekiah M. Carty wrote: > > > > > To sum up, I would like to submit patches in the follow steps: > > > (1) Add coordinate transform to plimagefr and disable the dev_fastimg > > > rendering path, but without removing the dev_fastimg code. > > > (2) Update dev_fastimg to work with the updated plimagefr, but only > > > use it for non-transformed images. > > > (3) Update example 20 with some examples of what plimagefr can do, > > > with pages to illustrate both fixed color ranges and coordinate > > > transformations. > > > > > > Does this sound like a reasonable compromise? > > > > Hi Hez: > > > > Actually after sleeping on it, I am leaning toward saying do (1) (with code > > commentary where you do the disabling in plimage.c, xwin.c, etc., about why > > it was necessary) and leave (2) as a would-be-nice rather than a requirement > > since it sounds like it might be a lot of work which you could more > > productively spend on the OCaml bindings, for example. > > > > However, I don't feel right making this decision alone because I haven't > > used -dev xwin or the plimage capability for my own PLplot needs, and > > somebody who has more of a vested interest in those parts of PLplot may feel > > a lot stronger about their speed than I do. Thus, I am going to need > > advice/help from the other PLplot core developers on the decision about (1) > > and (2) so please step forward, guys, and comment. > > I use -dev xwin extensively (and its client, plframe) but not plimage. That > said, doing (1) and leaving/documenting (2) as a nice-to-have sounds fine to > me, unless someone can make a case otherwise. Ditto. > Ideally someone would play with dev_fastimg vs !dev_fastimg and see if there's > a noticeable difference on mondern hardware. I'm sure many here have seen > hardware advances make irrelevant some "optimizations" done previously, or at > least mitigate performance concerns. I agree. If it turns out that dev_fastimg really is useful then I would encourage you to think about (2), i.e. still using dev_fastimg for the no-transform case at least. Without the tests it is hard to comment though. > For example, the X driver was first developed on 8-bit r/w color displays and > sharing a single r/w colormap was the norm. If this didn't suffice for the > application, a custom colormap could be used (which I never liked very much). > Seems so quaint now. :) But when I went to 16 or 24 bit r/o colormaps on a > Linux box years later, the performance degradation of swapping out colors > really didn't seem to matter much. One of these days I'd like to give the > xwin driver a bit of housecleaning, starting with chopping out the custom > colormap support that was never really used anyway. If you can find the time, it sounds like a good idea! I still extensively use the xwin driver. I prefer it over the more sophisticated gnome driver for example for many purposes because it is quick and clean. Andrew P.S. Hez, is the bug fix easy to tease out from the rest of your patch? We should at least apply that as we think about the rest it. Committing this separately also makes it clear what is bug fix and what is new feature. This can be useful when looking back through the commits. If the bug is only fixed because the broken code is replaced, then don't worry about it. |
From: Hezekiah M. C. <hc...@at...> - 2008-04-30 03:51:26
|
On Wed, Mar 19, 2008 at 2:31 PM, Andrew Ross <and...@us...> wrote: > > On Wed, Mar 19, 2008 at 01:12:24PM -0500, Maurice LeBrun wrote: > > On Wednesday, March 19, 2008 at 10:14:30 (-0700) Alan W. Irwin writes: > > > On 2008-03-19 10:59-0400 Hezekiah M. Carty wrote: > > > > > > > To sum up, I would like to submit patches in the follow steps: > > > > (1) Add coordinate transform to plimagefr and disable the dev_fastimg > > > > rendering path, but without removing the dev_fastimg code. > > > > (2) Update dev_fastimg to work with the updated plimagefr, but only > > > > use it for non-transformed images. > > > > (3) Update example 20 with some examples of what plimagefr can do, > > > > with pages to illustrate both fixed color ranges and coordinate > > > > transformations. > > > > > > > > Does this sound like a reasonable compromise? > > > > > > Hi Hez: > > > > > > Actually after sleeping on it, I am leaning toward saying do (1) (with code > > > commentary where you do the disabling in plimage.c, xwin.c, etc., about why > > > it was necessary) and leave (2) as a would-be-nice rather than a requirement > > > since it sounds like it might be a lot of work which you could more > > > productively spend on the OCaml bindings, for example. > > > > > > However, I don't feel right making this decision alone because I haven't > > > used -dev xwin or the plimage capability for my own PLplot needs, and > > > somebody who has more of a vested interest in those parts of PLplot may feel > > > a lot stronger about their speed than I do. Thus, I am going to need > > > advice/help from the other PLplot core developers on the decision about (1) > > > and (2) so please step forward, guys, and comment. > > > > I use -dev xwin extensively (and its client, plframe) but not plimage. That > > said, doing (1) and leaving/documenting (2) as a nice-to-have sounds fine to > > me, unless someone can make a case otherwise. > > Ditto. > > > > Ideally someone would play with dev_fastimg vs !dev_fastimg and see if there's > > a noticeable difference on mondern hardware. I'm sure many here have seen > > hardware advances make irrelevant some "optimizations" done previously, or at > > least mitigate performance concerns. > > I agree. If it turns out that dev_fastimg really is useful then I would > encourage you to think about (2), i.e. still using dev_fastimg for the > no-transform case at least. Without the tests it is hard to comment > though. > > > > For example, the X driver was first developed on 8-bit r/w color displays and > > sharing a single r/w colormap was the norm. If this didn't suffice for the > > application, a custom colormap could be used (which I never liked very much). > > Seems so quaint now. :) But when I went to 16 or 24 bit r/o colormaps on a > > Linux box years later, the performance degradation of swapping out colors > > really didn't seem to matter much. One of these days I'd like to give the > > xwin driver a bit of housecleaning, starting with chopping out the custom > > colormap support that was never really used anyway. > > If you can find the time, it sounds like a good idea! I still > extensively use the xwin driver. I prefer it over the more sophisticated > gnome driver for example for many purposes because it is quick and > clean. > > Andrew > > P.S. Hez, is the bug fix easy to tease out from the rest of your patch? > We should at least apply that as we think about the rest it. Committing > this separately also makes it clear what is bug fix and what is new > feature. This can be useful when looking back through the commits. If > the bug is only fixed because the broken code is replaced, then don't > worry about it. Several updates have been made to the PLplot Subversion source tree since I submitted my previous patches. Here is an updated version of my coordinate transform support in plimagefr patch which should apply cleanly against the latest PLplot SVN revision (8382 as of this writing). To recap, the changes are: - Comment out the dev_fastimg rendering path - Add coordinate transform support to plimagefr (same structure as in the plshade functions) - Update api.xml with the above changes Some code was hoisted out of plimagefr and back in to plimage to clean things up a bit. I also changed all of the comment to C style comments /* */ rather than C++ style // to match the rest of the PLplot code base. I ran some tests using example 20 compiled against PLplot 5.9.0 vs PLplot svn + this patch using the xwin display device to see what sort of difference the removal of the dev_fastimg rendering path makes. The difference in speed seems minimal if it exists at all. This is on a Pentium-M 1.7ghz laptop under Debian Sid. Please let me know if there are any concerns with these changes. Thanks, Hez -- Hezekiah M. Carty Graduate Research Assistant University of Maryland Department of Atmospheric and Oceanic Science |
From: Hezekiah M. C. <hc...@at...> - 2008-03-31 19:52:13
Attachments:
plimagefr_pltr_support2.diff
|
On Wed, Mar 19, 2008 at 2:31 PM, Andrew Ross <and...@us...> wrote: > > On Wed, Mar 19, 2008 at 01:12:24PM -0500, Maurice LeBrun wrote: > > On Wednesday, March 19, 2008 at 10:14:30 (-0700) Alan W. Irwin writes: > > > On 2008-03-19 10:59-0400 Hezekiah M. Carty wrote: > > > > > > > To sum up, I would like to submit patches in the follow steps: > > > > (1) Add coordinate transform to plimagefr and disable the dev_fastimg > > > > rendering path, but without removing the dev_fastimg code. > > > > (2) Update dev_fastimg to work with the updated plimagefr, but only > > > > use it for non-transformed images. > > > > (3) Update example 20 with some examples of what plimagefr can do, > > > > with pages to illustrate both fixed color ranges and coordinate > > > > transformations. > > > > > > > > Does this sound like a reasonable compromise? > > > > > > Hi Hez: > > > > > > Actually after sleeping on it, I am leaning toward saying do (1) (with code > > > commentary where you do the disabling in plimage.c, xwin.c, etc., about why > > > it was necessary) and leave (2) as a would-be-nice rather than a requirement > > > since it sounds like it might be a lot of work which you could more > > > productively spend on the OCaml bindings, for example. > > > > > > However, I don't feel right making this decision alone because I haven't > > > used -dev xwin or the plimage capability for my own PLplot needs, and > > > somebody who has more of a vested interest in those parts of PLplot may feel > > > a lot stronger about their speed than I do. Thus, I am going to need > > > advice/help from the other PLplot core developers on the decision about (1) > > > and (2) so please step forward, guys, and comment. > > > > I use -dev xwin extensively (and its client, plframe) but not plimage. That > > said, doing (1) and leaving/documenting (2) as a nice-to-have sounds fine to > > me, unless someone can make a case otherwise. > > Ditto. Thank you for all of the feedback, and sorry for the delayed response. As I am sure is the case for many of you, using PLplot for research got in the way of developing PLplot... The attached patch does the following: - Disables the dev_fastimg rendering path. This is done in a (hopefully) minimally invasive manner. No dev_fastimg code is deleted, just commented out when needed with a comment explaining why and that this is intended to be temporary. - Adds coordinate transformation (pltr callback) support to plimagefr - Removes the Dxmin, Dxmax, Dymin, Dymax arguments from plimagefr as they do not make sense when using a coordinate transform. The associated code/logic is moved in to plimage to keep its interface and function consistent with how it has worked in the past. - Should fix the (occasional) off-by-one bug in the number of image pixel rows/columns rendered when selecting a sub-image in plimage. - Updates api.xml to reflect the changes to plimagefr. Example 20 updates are not in place yet, but I will add them once we get this patch sorted out. This version of the patch works well for me. Some questions: - Should the name stay as plimagefr? I do not have any better ideas at this time, but with the addition of the coordinate transform support someone may have other naming suggestions. - What is the proper way to restore the initial drawing color when these functions end? Currently a call to plimage or plimagefr will leave the color set to whichever color palette 1 color was used for the last pixel. Ideally the color at entry would be saved at the start of the function and set again at the end of the function. I see examples elsewhere in the code which are specific to color palette 0 or color palette 1, but I would like to do this in a generic fashion, regardless of the color palette in use before the call to plimage/plimagefr. > > Ideally someone would play with dev_fastimg vs !dev_fastimg and see if there's > > a noticeable difference on mondern hardware. I'm sure many here have seen > > hardware advances make irrelevant some "optimizations" done previously, or at > > least mitigate performance concerns. > > I agree. If it turns out that dev_fastimg really is useful then I would > encourage you to think about (2), i.e. still using dev_fastimg for the > no-transform case at least. Without the tests it is hard to comment > though. There is a speed difference between the two - running example 20 using the xwin driver under PLplot 5.9.0 is quicker than with the attached patch. This only applies for the xwin driver though, as none of the other output drivers have dev_fastimg support. Adding dev_fastimg support back in should probably involve input from the output driver maintainers so that the interface is reasonably easily implemented and kept in sync with the currently popular and maintained drivers. That said, the speed difference is negligible for smaller images. The speed of example 20 is different but not very noticeable on my laptop (Pentium-M 1.7ghz) though this may add up to be significant if many images were plotted. I do not know what the speed difference would be for 1000x1000 and larger image arrays. I think the speed issue comes from using plfill to draw every pixel of the image. This is how the previous implementation of the slow rendering path works as well. The xwin dev_fastimg approach uses a custom rasterizer in xwin.c which this patch comments out. > > For example, the X driver was first developed on 8-bit r/w color displays and > > sharing a single r/w colormap was the norm. If this didn't suffice for the > > application, a custom colormap could be used (which I never liked very much). > > Seems so quaint now. :) But when I went to 16 or 24 bit r/o colormaps on a > > Linux box years later, the performance degradation of swapping out colors > > really didn't seem to matter much. One of these days I'd like to give the > > xwin driver a bit of housecleaning, starting with chopping out the custom > > colormap support that was never really used anyway. > > If you can find the time, it sounds like a good idea! I still > extensively use the xwin driver. I prefer it over the more sophisticated > gnome driver for example for many purposes because it is quick and > clean. I use the xwin driver quite often as well, largely because of the threading support. This makes it easier to quickly see results in an interactive environment. > P.S. Hez, is the bug fix easy to tease out from the rest of your patch? > We should at least apply that as we think about the rest it. Committing > this separately also makes it clear what is bug fix and what is new > feature. This can be useful when looking back through the commits. If > the bug is only fixed because the broken code is replaced, then don't > worry about it. The bug fix is something that I tackled during the rewrite, rather than as a separate issue. I think that the bug would be fixed by changing the following lines in plimage.c, function c_plimagefr, line 205: dx = (xmax - xmin) / (nx - 1); dy = (ymax - ymin) / (ny - 1); nnx = (Dxmax-Dxmin)/dx + 1; nny = (Dymax-Dymin)/dy + 1; to: dx = (xmax - xmin) / (PLFLT)nx; dy = (ymax - ymin) / (PLFLT)ny; nnx = ceil((Dxmax - Dxmin) / dx); nny = ceil((Dymax - Dymin) / dy); I have only tested this briefly, but I think it should do the trick. My apologies for not providing a separate patch - I am not sure that this will fix the issue, but I think it should. Thanks again for the feedback, Hez -- Hezekiah M. Carty Graduate Research Assistant University of Maryland Department of Atmospheric and Oceanic Science |
From: Hezekiah M. C. <hc...@at...> - 2008-04-01 20:16:35
Attachments:
plimagefr_pltr_support2-nan-fix.diff
|
On Mon, Mar 31, 2008 at 3:52 PM, Hezekiah M. Carty <hc...@at...> wrote: > > On Wed, Mar 19, 2008 at 2:31 PM, Andrew Ross > <and...@us...> wrote: > > > > On Wed, Mar 19, 2008 at 01:12:24PM -0500, Maurice LeBrun wrote: > > > On Wednesday, March 19, 2008 at 10:14:30 (-0700) Alan W. Irwin writes: > > > > On 2008-03-19 10:59-0400 Hezekiah M. Carty wrote: > > > > > > > > > To sum up, I would like to submit patches in the follow steps: > > > > > (1) Add coordinate transform to plimagefr and disable the dev_fastimg > > > > > rendering path, but without removing the dev_fastimg code. > > > > > (2) Update dev_fastimg to work with the updated plimagefr, but only > > > > > use it for non-transformed images. > > > > > (3) Update example 20 with some examples of what plimagefr can do, > > > > > with pages to illustrate both fixed color ranges and coordinate > > > > > transformations. > > > > > > > > > > Does this sound like a reasonable compromise? > > > > > > > > Hi Hez: > > > > > > > > Actually after sleeping on it, I am leaning toward saying do (1) (with code > > > > commentary where you do the disabling in plimage.c, xwin.c, etc., about why > > > > it was necessary) and leave (2) as a would-be-nice rather than a requirement > > > > since it sounds like it might be a lot of work which you could more > > > > productively spend on the OCaml bindings, for example. > > > > > > > > However, I don't feel right making this decision alone because I haven't > > > > used -dev xwin or the plimage capability for my own PLplot needs, and > > > > somebody who has more of a vested interest in those parts of PLplot may feel > > > > a lot stronger about their speed than I do. Thus, I am going to need > > > > advice/help from the other PLplot core developers on the decision about (1) > > > > and (2) so please step forward, guys, and comment. > > > > > > I use -dev xwin extensively (and its client, plframe) but not plimage. That > > > said, doing (1) and leaving/documenting (2) as a nice-to-have sounds fine to > > > me, unless someone can make a case otherwise. > > > > Ditto. > > Thank you for all of the feedback, and sorry for the delayed response. > As I am sure is the case for many of you, using PLplot for research > got in the way of developing PLplot... > > The attached patch does the following: > - Disables the dev_fastimg rendering path. This is done in a > (hopefully) minimally invasive manner. No dev_fastimg code is > deleted, just commented out when needed with a comment explaining why > and that this is intended to be temporary. > - Adds coordinate transformation (pltr callback) support to plimagefr > - Removes the Dxmin, Dxmax, Dymin, Dymax arguments from plimagefr as > they do not make sense when using a coordinate transform. The > associated code/logic is moved in to plimage to keep its interface and > function consistent with how it has worked in the past. > - Should fix the (occasional) off-by-one bug in the number of image > pixel rows/columns rendered when selecting a sub-image in plimage. > - Updates api.xml to reflect the changes to plimagefr. > > Example 20 updates are not in place yet, but I will add them once we > get this patch sorted out. > > This version of the patch works well for me. > > Some questions: > - Should the name stay as plimagefr? I do not have any better ideas > at this time, but with the addition of the coordinate transform > support someone may have other naming suggestions. > - What is the proper way to restore the initial drawing color when > these functions end? Currently a call to plimage or plimagefr will > leave the color set to whichever color palette 1 color was used for > the last pixel. Ideally the color at entry would be saved at the > start of the function and set again at the end of the function. I see > examples elsewhere in the code which are specific to color palette 0 > or color palette 1, but I would like to do this in a generic fashion, > regardless of the color palette in use before the call to > plimage/plimagefr. > > > > > Ideally someone would play with dev_fastimg vs !dev_fastimg and see if there's > > > a noticeable difference on mondern hardware. I'm sure many here have seen > > > hardware advances make irrelevant some "optimizations" done previously, or at > > > least mitigate performance concerns. > > > > I agree. If it turns out that dev_fastimg really is useful then I would > > encourage you to think about (2), i.e. still using dev_fastimg for the > > no-transform case at least. Without the tests it is hard to comment > > though. > > There is a speed difference between the two - running example 20 using > the xwin driver under PLplot 5.9.0 is quicker than with the attached > patch. This only applies for the xwin driver though, as none of the > other output drivers have dev_fastimg support. Adding dev_fastimg > support back in should probably involve input from the output driver > maintainers so that the interface is reasonably easily implemented and > kept in sync with the currently popular and maintained drivers. > > That said, the speed difference is negligible for smaller images. The > speed of example 20 is different but not very noticeable on my laptop > (Pentium-M 1.7ghz) though this may add up to be significant if many > images were plotted. I do not know what the speed difference would be > for 1000x1000 and larger image arrays. > > I think the speed issue comes from using plfill to draw every pixel of > the image. This is how the previous implementation of the slow > rendering path works as well. The xwin dev_fastimg approach uses a > custom rasterizer in xwin.c which this patch comments out. > > > > > For example, the X driver was first developed on 8-bit r/w color displays and > > > sharing a single r/w colormap was the norm. If this didn't suffice for the > > > application, a custom colormap could be used (which I never liked very much). > > > Seems so quaint now. :) But when I went to 16 or 24 bit r/o colormaps on a > > > Linux box years later, the performance degradation of swapping out colors > > > really didn't seem to matter much. One of these days I'd like to give the > > > xwin driver a bit of housecleaning, starting with chopping out the custom > > > colormap support that was never really used anyway. > > > > If you can find the time, it sounds like a good idea! I still > > extensively use the xwin driver. I prefer it over the more sophisticated > > gnome driver for example for many purposes because it is quick and > > clean. > > I use the xwin driver quite often as well, largely because of the > threading support. This makes it easier to quickly see results in an > interactive environment. > > > > P.S. Hez, is the bug fix easy to tease out from the rest of your patch? > > We should at least apply that as we think about the rest it. Committing > > this separately also makes it clear what is bug fix and what is new > > feature. This can be useful when looking back through the commits. If > > the bug is only fixed because the broken code is replaced, then don't > > worry about it. > > The bug fix is something that I tackled during the rewrite, rather > than as a separate issue. I think that the bug would be fixed by > changing the following lines in plimage.c, function c_plimagefr, line > 205: > > dx = (xmax - xmin) / (nx - 1); > dy = (ymax - ymin) / (ny - 1); > nnx = (Dxmax-Dxmin)/dx + 1; > nny = (Dymax-Dymin)/dy + 1; > > to: > > dx = (xmax - xmin) / (PLFLT)nx; > dy = (ymax - ymin) / (PLFLT)ny; > nnx = ceil((Dxmax - Dxmin) / dx); > nny = ceil((Dymax - Dymin) / dy); > > I have only tested this briefly, but I think it should do the trick. > My apologies for not providing a separate patch - I am not sure that > this will fix the issue, but I think it should. > > Thanks again for the feedback, Here is a small follow-up patch to properly handle the case when all values in an image are the same - this would lead to division by 0, nan being passed to plcol1 and segfaults under the xwin driver. This patch applies on top of plimagefr_pltr_support2.diff Hez -- Hezekiah M. Carty Graduate Research Assistant University of Maryland Department of Atmospheric and Oceanic Science |