[poppler] Poppler 0.12 release schedule

[Hal V. Engel]

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?

> > 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. 

>
> > > 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)

or setDocumentProfile().  And it appears that this is how it works although I 
am not sure I understand why this happens.

>
> > -) 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.

>
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: poppler-cm-2.patch
Type: text/x-patch
Size: 26556 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/poppler/attachments/20090514/d3ed37ec/attachment-0001.bin