[poppler] Stream reset and close in ImageStream

[Carlos Garcia Campos]

Here is a new patch keeping the current API, it just adds
ImageStream::close() for consistency. Ok to push?

El jue, 07-05-2009 a las 10:32 +0200, Carlos Garcia Campos escribió:
> 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
> _______________________________________________
> 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: 0001-Make-sure-ImageStream-close-is-called-after-Image.patch
Type: text/x-patch
Size: 7097 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/poppler/attachments/20090510/ab69de5d/attachment.bin 
-------------- 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/20090510/ab69de5d/attachment.pgp