[poppler] Stream reset and close in ImageStream

[Albert Astals Cid]

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. 

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?

Albert