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 > > > [email protected] > > > http://lists.freedesktop.org/mailman/listinfo/poppler > > > > _______________________________________________ > > poppler mailing list > > [email protected] > > http://lists.freedesktop.org/mailman/listinfo/poppler _______________________________________________ poppler mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/poppler
