From: D M G. <dm...@uv...> - 2009-10-23 18:07:33
|
Hi Kornel, why this patch? What is wrong with htons? I am reverting it. I don't see why to add a function that is already implemented (and reliable). If the problem is in Windows, then use the preprocessor to add an #ifdef that creates a macro that replaces htons only for windows (in sys_win.h), and place your function in sys_win.c, where windows specific code should go. Include sys_win.h in ColorBrightness.c. htons works as expected in OS X and Linux. I trust htons more than the libpano implementation. htons converts an 16 bit value. uint16_t htons(uint16_t hostshort); uint16_t ntohs(uint16_t netshort); your implementation converts a short. You might not know it, but the definition of short is implementation dependent. The standard says that a short should be shorter or equal to an int, which is shorter or equal to a long (it is possible to have a compiler than uses the same number of bits for all of them). See this: http://www.ibm.com/developerworks/aix/library/au-endianc/index.html Of course, this is a bit academic, as the current implementation of SHORTNUMBER in libpano has this problem too, but we should not write that adds unnecessary problems in the future. Jim, PTblender is broken. One of the 'memory leak' fixes created an extra free and it is crashing. --dmg ---------------------------------------------------------------------- trunk/libpano/ColourBrightness.c Modified: trunk/libpano/ColourBrightness.c =================================================================== --- trunk/libpano/ColourBrightness.c 2009-10-23 11:53:54 UTC (rev 1114) +++ trunk/libpano/ColourBrightness.c 2009-10-23 11:56:14 UTC (rev 1115) @@ -364,6 +364,14 @@ } +static short pano_htons(short y) +{ + short x; + char * px = (char *) &x; + SHORTNUMBER(y, px); + return(x); +} + int OutputPhotoshopCurve(FILE *output, int size, double *curve) { uint16_t shortValue; @@ -381,7 +389,7 @@ // The algorithm currently writes the first point, 12, and the last point -- -- Daniel M. German http://turingmachine.org/ http://silvernegative.com/ dmg (at) uvic (dot) ca replace (at) with @ and (dot) with . |
From: Kornel B. <Kor...@be...> - 2009-10-23 18:22:06
|
Am Freitag 23 Oktober 2009 schrieb D M German: > Hi Kornel, > > why this patch? What is wrong with htons? Nothing. > I am reverting it. I don't > see why to add a function that is already implemented (and > reliable). OK. > If the problem is in Windows, then use the preprocessor to add an #ifdef > that creates a macro that replaces htons only for windows (in > sys_win.h), and place your function in sys_win.c, where windows specific > code should go. > > Include sys_win.h in ColorBrightness.c. It looks like it were windows only. > htons works as expected in OS X and Linux. I trust htons more than the > libpano implementation. > > htons converts an 16 bit value. > > uint16_t htons(uint16_t hostshort); > > uint16_t ntohs(uint16_t netshort); > > your implementation converts a short. You might not know it, but the > definition of short is implementation dependent. The standard says that > a short should be shorter or equal to an int, which is shorter or equal > to a long (it is possible to have a compiler than uses the same number > of bits for all of them). You are right, I did not know. > See this: > > http://www.ibm.com/developerworks/aix/library/au-endianc/index.html Thanks for the pointer. > Of course, this is a bit academic, as the current implementation of > SHORTNUMBER in libpano has this problem too, but we should not write > that adds unnecessary problems in the future. Should I try to prepare appropriate patch (in sys_win.h) ? > Jim, > > PTblender is broken. One of the 'memory leak' fixes created an extra > free and it is crashing. > > > --dmg > Kornel -- Kornel Benko Kor...@be... |
From: D M G. <dm...@uv...> - 2009-10-23 18:40:36
|
Hi Kornel, Kornel> Am Freitag 23 Oktober 2009 schrieb D M German: >> Hi Kornel, >> >> why this patch? What is wrong with htons? Kornel> Nothing. >> I am reverting it. I don't >> see why to add a function that is already implemented (and >> reliable). Kornel> OK. [...] >> Of course, this is a bit academic, as the current implementation of >> SHORTNUMBER in libpano has this problem too, but we should not write >> that adds unnecessary problems in the future. Kornel> Should I try to prepare appropriate patch (in sys_win.h) ? Please do. I don't have any Windows computer. Jim has been working in the porting to windows (and now you). Just let me know in advance when you have problems and we'll find a common solution. Sorry for the wasted time doing this (fortunately it was a small patch). I don't mind reviewing your code, but ideally, let us know in advance what you are doing, and I can usually provide help/pointers, or at least we cna avoid these problems. I am not sure how windows compilation is detected these days. But just add something like #define htons(x) panoWinHtons(x) Please use camel style names with no _ in between then, and after pano, add a prefix that indicates the "submodule" where the function is to be located. this makes reading code easier. Panotools is very old code with many hands who have modified, but when possible I have been trying to add consistent names. ColourBrightness has such meaningless function names because it was reverse engineered from assembly, and it was impossible to know the purpose of the functions while doing it (the names have some relationships, but it is meaningless to the actual purpose of the functions). --dmg -- -- Daniel M. German http://turingmachine.org/ http://silvernegative.com/ dmg (at) uvic (dot) ca replace (at) with @ and (dot) with . |
From: D M G. <dm...@uv...> - 2009-10-23 19:17:24
|
hi Kornel, Kornel> Should I try to prepare appropriate patch (in sys_win.h) ? >> >> Please do. I don't have any Windows computer. Kornel> Unfortunatelly, I don't have either. Oh, I thought you were fixing the windows port _because_ you had windows. So you are fixing a bug you can't test? :) >> Jim has been working in >> the porting to windows (and now you). Just let me know in advance when >> you have problems and we'll find a common solution. Sorry for the wasted >> time doing this (fortunately it was a small patch). I don't mind >> reviewing your code, but ideally, let us know in advance what you are >> doing, and I can usually provide help/pointers, or at least we cna avoid >> these problems. Kornel> I did. And have not received any response. Where? I don't see any messages regarding this? >> I am not sure how windows compilation is detected these days. But just >> add something like >> >> #define htons(x) panoWinHtons(x) >> >> Please use camel style names with no _ in between then, and after pano, >> add a prefix that indicates the "submodule" where the function is to be >> located. this makes reading code easier. Kornel> OK. Kornel> Would something like the following be ok? Ideally yes, but if you want to reuse the implementation you created, that is fine with me. Please ask Jim to help you commit the code. Jim should run PTtiff2psd to create a file, and make sure Photoshop still reads the PSD file. He knows more about the Windows compilers than we can assume. -- Daniel M. German http://turingmachine.org/ http://silvernegative.com/ dmg (at) uvic (dot) ca replace (at) with @ and (dot) with . |
From: D M G. <dm...@uv...> - 2009-10-23 19:42:45
|
Kornel Benko twisted the bytes to say: >> Where? I don't see any messages regarding this? >> Kornel> http://www.mail-archive.com/hug...@go.../msg06696.html Kornel> http://www.mail-archive.com/hug...@go.../msg06730.html Kornel> And this was the mail from Jim, which directed me to use SHORTNUMBER() Kornel> http://www.mail-archive.com/hug...@go.../msg06608.html Kornel> ... Ah, in the hugin list... this should be discussed here, in the libpano mailing list. sorry, but i don't read every message in that list (and frequently just mark them all as read), and even if I did, this should be discussed in this mailing list. I do read every message in this list -dmg -- -- Daniel M. German http://turingmachine.org/ http://silvernegative.com/ dmg (at) uvic (dot) ca replace (at) with @ and (dot) with . |
From: Jim W. <jwa...@ph...> - 2009-10-23 21:33:02
|
D M German wrote: > Kornel Benko twisted the bytes to say: > > >> Where? I don't see any messages regarding this? > >> > Kornel> http://www.mail-archive.com/hug...@go.../msg06696.html > Kornel> http://www.mail-archive.com/hug...@go.../msg06730.html > > Kornel> And this was the mail from Jim, which directed me to use SHORTNUMBER() > Kornel> http://www.mail-archive.com/hug...@go.../msg06608.html > Kornel> ... > > Ah, in the hugin list... this should be discussed here, in the libpano > mailing list. > > sorry, but i don't read every message in that list (and frequently just > mark them all as read), and even if I did, this should be discussed in > this mailing list. > > I do read every message in this list > > -dmg And I don't always notice what list I am replying to. Daniel is right it should been discussed here. I was surprised to Daniel insisting to continue to use htons when Panotools already uses SHORTNUMBER, but I now understand why. I received you patch and will test it. As a Windows only patch it does belong in win.c/h files. -- Jim Watters jwa...@ph... http://photocreations.ca |
From: Kornel B. <Kor...@be...> - 2009-10-25 05:38:53
|
Am Freitag 23 Oktober 2009 schrieb Jim Watters: > D M German wrote: ... > > Kornel> ... > > > > Ah, in the hugin list... this should be discussed here, in the libpano > > mailing list. > > > > sorry, but i don't read every message in that list (and frequently just > > mark them all as read), and even if I did, this should be discussed in > > this mailing list. Ok, sorry for the confusion. I should have redirected the thread. > > I do read every message in this list > > > > -dmg > > And I don't always notice what list I am replying to. > Daniel is right it should been discussed here. > I was surprised to Daniel insisting to continue to use htons when > Panotools already uses SHORTNUMBER, but I now understand why. > I received you patch and will test it. Untested as it was, there was a small bug too. Copy and Paste error. ColourBrightness.c:54 => #if defined (__Win__) instead of "if" only > As a Windows only patch it does > belong in win.c/h files. The patch is done that way. Kornel -- Kornel Benko Kor...@be... |
From: Jim W. <jwa...@ph...> - 2009-10-24 21:40:03
|
D M German wrote: > Hi Kornel, > > why this patch? What is wrong with htons? I am reverting it. I don't > see why to add a function that is already implemented (and > reliable). > After reviewing everything I totally agree with Danial. Lets continue to use htons. The Windows build will need to include the necessary library. I have been using Ws2_32.lib, I notice that you have wsock32 in CMakeLists.txt. Maybe they do resolve to the same file but I don't think so. I don't have wsock32.lib on my Windows system. This lib should be added to ${_common_libs} CMakeLists.txt:68 set(_common_libs ${TIFF_LIBRARIES} ${ZLIB_LIBRARIES} ${JPEG_LIBRARIES} ${PNG_LIBRARIES}) if(WIN32) find_library(WXSOCK32 wsock32) if(NOT ${WXSOCK32} MATCHES "-NOTFOUND") list(APPEND _common_libs ${WXSOCK32}) endif() endif(WIN32) > If the problem is in Windows, then use the preprocessor to add an #ifdef > that creates a macro that replaces htons only for windows (in > sys_win.h), and place your function in sys_win.c, where windows specific > code should go. > Except the command line tools on Windows don't use sys_win it uses sys_ansi. Only the GUI tools use sys_wins. > See this: > > http://www.ibm.com/developerworks/aix/library/au-endianc/index.html > Thanks. That was a great link. > Jim, > > PTblender is broken. One of the 'memory leak' fixes created an extra > free and it is crashing. > I found the problem. Small fix coming soon. -- Jim Watters http://photocreations.ca |