[poppler] Stream reset and close in ImageStream

[Carlos Garcia Campos]

El mié, 06-05-2009 a las 19:54 +0200, Albert Astals Cid escribió:
> A Dimarts, 5 de maig de 2009, Carlos Garcia Campos va escriure:
> > Hi all,
> >
> > we have received a bug report in the evince list today about this file:
> >
> > http://www.thomsongrassvalley.com/docs/DataSheets/switchers/kayenne/SWT-302
> >5D.pdf
> >
> > at a first glance it seemed like a corrupted PDF file, since there were
> > a lot of parser and lexer errors on stderr:
> >
> > Error (19684): Illegal character <4b> in hex string
> > Error (19684): Illegal character <29> in hex string
> > Error (19684): Illegal character <28> in hex string
> > Error (19684): Illegal character <79> in hex string
> > Error (19684): Illegal character <29> in hex string
> > Error (19686): Illegal character <70> in hex string
> > .......
> >
> > However, the problem is not reproducible with the splash backend ....
> > quite strange because gfx, parser, lexer and so on is shared code that
> > has nothing to do with cairo or splash. After a lot of debugging, I've
> > finally figured out what the problem is, There was a missing
> > str->close() in CairoOutputDev::drawSoftMaskedImage().
> >
> > The FileStream API is very confusing, reset() saves the current file
> > position and seeks to the begining, and close() doesn't close the file,
> > but restores the position to the previously saved position when reset
> > was called. I think it makes more sense to call them reset() - restore()
> > or save() - reset() - restore().
> >
> > ImageStream has a reset() method wich is actually a wrapper to reset its
> > stream, but it doesn't provide a close() method. Every time we use
> > ImageStream we repeat this pattern:
> >
> > imgStr = new ImageStream(str, ...);
> > imgStr->reset();
> > .......
> > str->close();
> > delete imgStr;
> >
> > With this API is quite easy to forget the str->close(), and I've noticed
> > it's missing in other places too, indeed. We need to know that
> > ImageStream::reset() calls str->reset() in order to know that we have to
> > call str->close() to restore the file position. Confusing.
> >
> > So, I propose to remove the reset() method from the ImageStream class
> > and call str->reset() in the constructor and str->close() in the
> > destructor. See the attached patch.
> >
> > What do you think?
> 
> My problem with this is that we separate ourselves even more from xpdf and so 
> people using xpdf/poppler core headers will have headaches when switching from 
> one to the other because API is similar but behaves differently. 

hmm, I thought we didn't recommend anybody using the poppler core
headers, but anyway, I propose then adding ImageStream::close() that
calls str->close() analogous to ImageStream::reset(), so that we call
reset() close() on the same object which is more natural to me. 

> I think i like more fixing the "buggy" places and adding a comment to Stream.h 
> explaining the problem.
> 
> Does the "bug" happen only in CairoOutputDev but on other places as well?

I have only noticed it with CairoOutputDev and that particular document.
I think it might fix other bugs, but I'm not sure. 

> Albert
> 
> _______________________________________________
> poppler mailing list
> poppler at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/poppler
-- 
Carlos Garcia Campos
   elkalmail at yahoo.es
   carlosgc at gnome.org
   http://carlosgc.linups.org
PGP key: http://pgp.mit.edu:11371/pks/lookup?op=get&search=0x523E6462
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Esta parte del mensaje =?ISO-8859-1?Q?est=E1?= firmada
 digitalmente
Url : http://lists.freedesktop.org/archives/poppler/attachments/20090507/722c8d73/attachment.pgp