Let me explain something about quality control. When someone has spent many dozens of hours debugging both the performance and stability of a piece of code at the low level, then whenever you make sweeping changes to it like this, it is incumbent upon you to either work with the person who tested the code or to do the same testing yourself. Otherwise, you have, by definition, de-stabilized the code.
Without knowing why you were experiencing issues, what you've done below likely created more bugs than it fixed. This should have been re-tested with vncbenchtools prior to checking it in. On 10/18/11 9:27 AM, ossm...@users.sourceforge.net wrote: > Revision: 4731 > http://tigervnc.svn.sourceforge.net/tigervnc/?rev=4731&view=rev > Author: ossman_ > Date: 2011-10-18 14:27:07 +0000 (Tue, 18 Oct 2011) > Log Message: > ----------- > Another attempt at solving the compression level change problems. We were > still > not detecting the correct approach properly, and hence still getting crashes. > > Modified Paths: > -------------- > trunk/common/rdr/ZlibOutStream.cxx > trunk/common/rdr/ZlibOutStream.h > > Modified: trunk/common/rdr/ZlibOutStream.cxx > =================================================================== > --- trunk/common/rdr/ZlibOutStream.cxx 2011-10-14 21:35:40 UTC (rev > 4730) > +++ trunk/common/rdr/ZlibOutStream.cxx 2011-10-18 14:27:07 UTC (rev > 4731) > @@ -23,27 +23,28 @@ > > #include <zlib.h> > > +#undef ZLIBOUT_DEBUG > + > using namespace rdr; > > enum { DEFAULT_BUF_SIZE = 16384 }; > > ZlibOutStream::ZlibOutStream(OutStream* os, int bufSize_, int compressLevel) > : underlying(os), compressionLevel(compressLevel), newLevel(compressLevel), > - bufSize(bufSize_ ? bufSize_ : DEFAULT_BUF_SIZE), offset(0), > - newBehavior(false) > + bufSize(bufSize_ ? bufSize_ : DEFAULT_BUF_SIZE), offset(0) > { > zs = new z_stream; > zs->zalloc = Z_NULL; > zs->zfree = Z_NULL; > zs->opaque = Z_NULL; > + zs->next_in = Z_NULL; > + zs->avail_in = 0; > if (deflateInit(zs, compressLevel) != Z_OK) { > delete zs; > throw Exception("ZlibOutStream: deflateInit failed"); > } > ptr = start = new U8[bufSize]; > end = start + bufSize; > - const char *version = zlibVersion(); > - if (strcmp(version, "1.2.3") > 0) newBehavior = true; > } > > ZlibOutStream::~ZlibOutStream() > @@ -77,74 +78,39 @@ > > void ZlibOutStream::flush() > { > + checkCompressionLevel(); > + > zs->next_in = start; > zs->avail_in = ptr - start; > > -// fprintf(stderr,"zos flush: avail_in %d\n",zs->avail_in); > +#ifdef ZLIBOUT_DEBUG > + fprintf(stderr,"zos flush: avail_in %d\n",zs->avail_in); > +#endif > > - if (!underlying) > - throw Exception("ZlibOutStream: underlying OutStream has not been set"); > + // Force out everything from the zlib encoder > + deflate(Z_SYNC_FLUSH); > > - while (zs->avail_in != 0) { > - > - do { > - underlying->check(1); > - zs->next_out = underlying->getptr(); > - zs->avail_out = underlying->getend() - underlying->getptr(); > - > -// fprintf(stderr,"zos flush: calling deflate, avail_in %d, avail_out > %d\n", > -// zs->avail_in,zs->avail_out); > - checkCompressionLevel(); > - if (zs->avail_in != 0) { > - int rc = deflate(zs, Z_SYNC_FLUSH); > - if (rc != Z_OK) throw Exception("ZlibOutStream: deflate failed"); > - } > - > -// fprintf(stderr,"zos flush: after deflate: %d bytes\n", > -// zs->next_out-underlying->getptr()); > - > - underlying->setptr(zs->next_out); > - } while (zs->avail_out == 0); > - } > - > offset += ptr - start; > ptr = start; > } > > int ZlibOutStream::overrun(int itemSize, int nItems) > { > -// fprintf(stderr,"ZlibOutStream overrun\n"); > +#ifdef ZLIBOUT_DEBUG > + fprintf(stderr,"zos overrun\n"); > +#endif > > if (itemSize > bufSize) > throw Exception("ZlibOutStream overrun: max itemSize exceeded"); > > - if (!underlying) > - throw Exception("ZlibOutStream: underlying OutStream has not been set"); > + checkCompressionLevel(); > > while (end - ptr < itemSize) { > zs->next_in = start; > zs->avail_in = ptr - start; > > - do { > - underlying->check(1); > - zs->next_out = underlying->getptr(); > - zs->avail_out = underlying->getend() - underlying->getptr(); > + deflate(Z_NO_FLUSH); > > -// fprintf(stderr,"zos overrun: calling deflate, avail_in %d, > avail_out %d\n", > -// zs->avail_in,zs->avail_out); > - > - checkCompressionLevel(); > - if (zs->avail_in != 0) { > - int rc = deflate(zs, 0); > - if (rc != Z_OK) throw Exception("ZlibOutStream: deflate failed"); > - } > - > -// fprintf(stderr,"zos overrun: after deflate: %d bytes\n", > -// zs->next_out-underlying->getptr()); > - > - underlying->setptr(zs->next_out); > - } while (zs->avail_out == 0); > - > // output buffer not full > > if (zs->avail_in == 0) { > @@ -166,23 +132,69 @@ > return nItems; > } > > +void ZlibOutStream::deflate(int flush) > +{ > + int rc; > + > + if (!underlying) > + throw Exception("ZlibOutStream: underlying OutStream has not been set"); > + > + if ((flush == Z_NO_FLUSH) && (zs->avail_in == 0)) > + return; > + > + do { > + underlying->check(1); > + zs->next_out = underlying->getptr(); > + zs->avail_out = underlying->getend() - underlying->getptr(); > + > +#ifdef ZLIBOUT_DEBUG > + fprintf(stderr,"zos: calling deflate, avail_in %d, avail_out %d\n", > + zs->avail_in,zs->avail_out); > +#endif > + > + rc = ::deflate(zs, flush); > + if (rc != Z_OK) { > + // Silly zlib returns an error if you try to flush something twice > + if ((rc == Z_BUF_ERROR) && (flush != Z_NO_FLUSH)) > + break; > + > + throw Exception("ZlibOutStream: deflate failed"); > + } > + > +#ifdef ZLIBOUT_DEBUG > + fprintf(stderr,"zos: after deflate: %d bytes\n", > + zs->next_out-underlying->getptr()); > +#endif > + > + underlying->setptr(zs->next_out); > + } while (zs->avail_out == 0); > +} > + > void ZlibOutStream::checkCompressionLevel() > { > + int rc; > + > if (newLevel != compressionLevel) { > +#ifdef ZLIBOUT_DEBUG > + fprintf(stderr,"zos change: avail_in %d\n",zs->avail_in); > +#endif > > - // This is a horrible hack, but after many hours of trying, I couldn't > find > - // a better way to make this class work properly with both Zlib 1.2.3 and > - // 1.2.5. 1.2.3 does a Z_PARTIAL_FLUSH in the body of deflateParams() if > - // the compression level has changed, and 1.2.5 does a Z_BLOCK flush. > + // zlib is just horribly stupid. It does an implicit flush on > + // parameter changes, but the flush it does is not one that forces > + // out all the data. And since you cannot flush things again, we > + // cannot force out our data after the parameter change. Hence we > + // need to do a more proper flush here first. > + deflate(Z_SYNC_FLUSH); > > - if (newBehavior) { > - int rc = deflate(zs, Z_SYNC_FLUSH); > - if (rc != Z_OK) throw Exception("ZlibOutStream: deflate failed"); > + rc = deflateParams (zs, newLevel, Z_DEFAULT_STRATEGY); > + if (rc != Z_OK) { > + // The implicit flush can result in this error, caused by the > + // explicit flush we did above. It should be safe to ignore though > + // as the first flush should have left things in a stable state... > + if (rc != Z_BUF_ERROR) > + throw Exception("ZlibOutStream: deflateParams failed"); > } > > - if (deflateParams (zs, newLevel, Z_DEFAULT_STRATEGY) != Z_OK) { > - throw Exception("ZlibOutStream: deflateParams failed"); > - } > compressionLevel = newLevel; > } > } > > Modified: trunk/common/rdr/ZlibOutStream.h > =================================================================== > --- trunk/common/rdr/ZlibOutStream.h 2011-10-14 21:35:40 UTC (rev 4730) > +++ trunk/common/rdr/ZlibOutStream.h 2011-10-18 14:27:07 UTC (rev 4731) > @@ -46,6 +46,7 @@ > private: > > int overrun(int itemSize, int nItems); > + void deflate(int flush); > void checkCompressionLevel(); > > OutStream* underlying; > @@ -55,7 +56,6 @@ > int offset; > z_stream_s* zs; > U8* start; > - bool newBehavior; > }; > > } // end of namespace rdr > > This was sent by the SourceForge.net collaborative development platform, the > world's largest Open Source development site. > > > ------------------------------------------------------------------------------ > All the data continuously generated in your IT infrastructure contains a > definitive record of customers, application performance, security > threats, fraudulent activity and more. Splunk takes this data and makes > sense of it. Business sense. IT sense. Common sense. > http://p.sf.net/sfu/splunk-d2d-oct > _______________________________________________ > Tigervnc-commits mailing list > tigervnc-comm...@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/tigervnc-commits ------------------------------------------------------------------------------ All the data continuously generated in your IT infrastructure contains a definitive record of customers, application performance, security threats, fraudulent activity and more. Splunk takes this data and makes sense of it. Business sense. IT sense. Common sense. http://p.sf.net/sfu/splunk-d2d-oct _______________________________________________ Tigervnc-devel mailing list Tigervnc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
