[poppler] Stream reset and close in ImageStream

[Albert Astals Cid]

A Diumenge, 10 de maig de 2009, Carlos Garcia Campos va escriure:
> Here is a new patch keeping the current API, it just adds
> ImageStream::close() for consistency. Ok to push?

This patch is nice :-)

Go Go Go!

Albert

>
> 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/S
> > > >WT-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