A Divendres, 15 de maig de 2009, Hal V. Engel va escriure: > On Thursday 14 May 2009 11:22:14 am Albert Astals Cid wrote: > > A Dijous, 14 de maig de 2009, Pino Toscano va escriure: > > > Alle martedì 12 maggio 2009, Hal V. Engel ha scritto: > > > > Here it is. This patch applies and builds on git head as of this > > > > afternoon. > > > > > > Nice! > > > Just as a general comment: there are many whitespace-only changes (like > > > a tab that becomes four spaces); if you could avoid them, and produce a > > > patch in unified format (`git diff -U` should be enough), it would be > > > slightly more readable. > > Thanks for the tip. I have not worked with git before this. So I am on a > learning curve. > > > > Now some comments about the Qt4 related parts: > > > > 1. I am not a cmake expert so I had to hack on qt4/src/CMakeLists.txt > > > > so that USE_CMS was defined for the qt4 stuff. Someone with more > > > > cmake expertise almost for sure needs to fix my hack. > > > > > > I don't like much the "have/don't-have LCMS" exposed in the public API. > > Meaning poppler-qt4.h or are you saying that this should also apply to > GfxState.h?
I mean poppler-qt4.h, GFxState is not really public API even some people use it. > > > > Especially that including lcms.h would break all the poppler-qt4 > > > library users which don't have LCMS available. > > Which is almost no one now days but it is still a valid point. > > > > The only solution I have in mind is the following: given the only LCMS > > > bit exposed is cmsHPROFILE, which internally is a void*, it could be > > > passed as Qt::HANDLE, like this: > > > void setColorProfile(Qt::HANDLE cmsProfile); > > > void setColorProfileName(const QString &name); > > > Qt::HANDLE colorRGBProfile() const; > > > Qt::HANDLE colorProfile() const; > > > and then doing 'cmsHPROFILE profile = (cmsHPROFILE)cmsProfile;' to cast > > > the profile back and forth. Of course the apidox for the function would > > > need to specify that the Qt::HANDLE refers to a cmsHPROFILE, and in > > > case there's no color management that those handles are NULL. > > > > If we are going to pass void * just pass void *, it's as ugly as a > > Qt::HANDLE and shows what you're doing more clearly imho. > > I agree that using void* is more general and it would make it easier to > abstract the code into a general purpose library for use by all tool sets > rather than just Qt. > > I am thinking of pulling the functions to get the system supplied monitor > profile(s) into a separate library. I think I can find a place to host > this code so that it is available to a wide range of projects. But it will > take some time to do that. In the mean time working out the details in the > demo viewer seems like the way to go. > > > > Also, the function would always be compiled, and the actual > > > implementations making nothing when HAVE_LCMS is not defined. For this, > > > adding a static bool canManageColorProfiles(); > > > would help users to know whether they can actually expect the function > > > to do something. > > > > Agreed. > > USE_CMS or HAVE_LCMS? At some point it might be desirable to abstract the > CMS support so that other CMS's like ColorSysc (OS/X), the Windows CMS, > ArgyllCMS, AdobeAce ... can be used instead. The more general solution is > to use USE_CMS for this so as not to tie this to a specific CMS. > > I now have the CMS related functions in the poppler-qt4 library and the > demo viewer setup so that LCMS specifics are not visible in the header > files and these functions will always be built. But if USE_CMS is not > defined they will simply return NULL or do nothing. I will look at doing > the same type of thing in GfxState.* but perhaps Koji should have a look at > this. > > I added > > bool GfxColorSpace::cmsAvailable() and bool Document::cmsAvailable() > > to make this test available for user apps. > > On the other hand for poppler to meet the current PDF specification it > really needs to have CMS support. So in general not using a CMS should be > discouraged and in the longer term it should become a very rare situation > for users to not have a CMS installed since most users with up to date > distros will have lcms installed by default because may other apps now > require it. Agreed. > > > > > 4. Someone with Windows and/or Mac expertise needs to write versions > > > > of qt4/demos/viewer.cpp cmsHPROFILE PdfViewer::getProfileAtom () for > > > > those other OS's for this to work on Windows and on the Mac. It > > > > might also be useful to move these functions to a support library so > > > > that they are available to anyone who uses the library. > > > > Yeah, but i don't really think that library belongs into the poppler > > scope, so let's just use that code until someone creates that lib :D > > This is really more in the scope of OpenICC. So I will look into it. > > > > > 5. I am not a gkt programmer so I made no attempt to update any of > > > > the gtk stuff to support CM. But the Qt4 code should provide enough > > > > clues for a gtk programmer to do the same basic stuff as I did with > > > > the Qt4 support library and the demo viewer. > > > > > > Then the X11 specific code should be grouped inside #ifdef Q_WS_X11 > > > blocks. > > Yes I now have code blocks for X11, Windows and OS/X for this OS specific > code. Each OS does this in completely different ways with APIs that are > very different. So it is necessary to abstract this in a non-OS specific > API. I pulled most of the Windows and OS/X code I am using from stuff I > had in LProf for dealing with video gamma tables since the logic involved > in getting the handle for the specific monitor is the same as needed here. > I also found the rest of the Windows code in a note that had been sent to > the lcms email list by Marti Maria (the author of lcms). So most of the > code has been tested in other apps but I am still trying to understand some > details in the OS/X API and I have not located the documentation for this > yet. > > > > Other notes: > > > > > > -) API: not sure whether it is necessary to expose 'int > > > setupDocumentColorProfiles()', it seems more worth just making sure it > > > is properly intialized when needed, either in the poppler core or when > > > creating a new SplashOutputDev? > > > > In my opinion it's not needed at all, should be automatic on creation and > > on calls that need to change something (like i guess > > setDocumentProfileName) Documentation on poppler-qt4.h is weird, mentions functions that don't exist and we also have a commented function, can you clean it up? It would be good if you kept doxygen style comments, that is one comment per function. Also config.h is not needed there anymore. > or setDocumentProfile(). And it appears that this is how it works although > I am not sure I understand why this happens. It works if you call the functions before GfxColorSpace::setupColorProfiles() is called. Also i think void GfxColorSpace::setDisplayProfile(void *displayProfileA) void GfxColorSpace::setDisplayProfileName(GooString *name) should take care of deleting old contents of the variables they assign to, what do you think? > > > > -) API: in Document::setDocumentProfileName(), QStringToGooString() > > > creates a new GooString, which should be delete'd. > > Done. But I don't think it is necessary since this is a local variable > that is created on the stack and it is gone as soon as the call returns. No, that means you get a leak each time because a newed pointer goes out of scope. The rest seems oka-ish to me. Albert > > > About naming, should setDocumentProfileName be setOutputProfileName? I > > mean should i set there the profile of my monitor (output) or of the > > document (input) > > This is a good question. This is really a wrapper function for > GfxColorSpace::setOutputProfileName(). But it might make things clearer if > these methods were overloaded and my code now does this. > > > Albert > > > > > -) Qt4 demo: you don't need to create and hold a new QDesktopWidget, > > > QApplication::desktop() will return the global one. > > Done > > > > -) Qt4 demo: getProfileAtom() should be just colorProfile(), returning > > > a Qt::HANDLE and being always defined. > > Actually a more meaningful name would be getMonitorProfile(...) since this > is actually what it does. I have changed this in my current code. Also > Windows and OS/X require additional information to get the correct screen > handle so I have set this up to allow that data to be passed in. In X11 > with Qt these are just dummy parms that don't do anything. But it will > make the API the same in every OS. I should add that the reason that these > same parms are not needed is that it can be handled via a > QX11info::display() call so doing this in a generic X11 way (IE. not using > Qt calls) will likely also require the same parms as Windows and OS/X. Qt > does not have a similar API for Windows or OS/X. > > > > -) Qt4 demo: the general coding style there is much like > > > http://qt.gitorious.org/qt/pages/QtCodingStyle, so please follow it, > > > whenever possible. > > I had a look and my code is now in the correct style at least for the most > part. > > I have created a new patch with these changes and it is attached here. The > getMonitorProfile() code for Windows and OS/X is completely untested and I > don't even know if it compiles at this point. But it is mostly correct and > there is perhaps a 50% chance that the Windows code is OK or at most only > needs minor tweaks to work. > > Hal _______________________________________________ poppler mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/poppler
