Blimp: add packet-level DEFLATE compression using zlib.

blimp/net/compressed_packet_writer.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "blimp/net/compressed_packet_writer.h"
+
+#include <vector>
+
+#include "base/logging.h"
+#include "base/sys_byteorder.h"
+#include "blimp/net/common.h"
+#include "net/base/io_buffer.h"
+#include "third_party/zlib/zlib.h"
+
+namespace blimp {
+namespace {
+
+// Allocate the maxmimum amount of memory to deflate (512KB) for higher
+// compression. (See zconf.h for details on memLevel semantics.)
+const int kZlibMemoryLevel = 9;
+
+}  // namespace
+
+CompressedPacketWriter::CompressedPacketWriter(scoped_ptr<PacketWriter> sink)
+    : sink_(std::move(sink)), compressed_buf_(new net::GrowableIOBuffer) {
+  DCHECK(sink_);
+
+  // MAX_WBITS means we are using the maximal window size for decompression;
+  // negating it means that we are ignoring headers and CRC checks.
+  int err = deflateInit2(&zlib_stream_, Z_BEST_COMPRESSION, Z_DEFLATED,

[Wez, 2016/03/29 21:38:38]

nit: zlib_error or init_error?

[Kevin M, 2016/03/29 23:48:24]

Done.

+                         -MAX_WBITS,  // Negative value = no headers or CRC.
+                         kZlibMemoryLevel, Z_DEFAULT_STRATEGY);
+  DCHECK_EQ(err, Z_OK);
+}
+
+CompressedPacketWriter::~CompressedPacketWriter() {
+  deflateEnd(&zlib_stream_);

[Wez, 2016/03/29 21:38:38]

You're calling deflateEnd() here but only DCHECK()ing the result of init, so a
release build may end up in a weird state? Should we just CHECK the OK result
from init?

[Kevin M, 2016/03/29 23:48:24]

Good point, done

+}
+
+void CompressedPacketWriter::WritePacket(
+    const scoped_refptr<net::DrainableIOBuffer>& buf,

[Wez, 2016/03/29 21:38:38]

nit: buf & cb should have style-guide-compliant names, please!

[Kevin M, 2016/03/29 23:48:23]

Done.

[Kevin M, 2016/03/29 23:48:24]

Done.

+    const net::CompletionCallback& cb) {
+  DCHECK(buf);
+  DCHECK(!cb.is_null());
+  size_t uncompressed_size = static_cast<size_t>(buf->BytesRemaining());

[Wez, 2016/03/29 21:38:38]

Use a checked cast here (see the header in base/).

[Kevin M, 2016/03/29 23:48:24]

Done.

+
+  if (uncompressed_size == 0) {
+    // Zero-length input => zero-length output.

[Wez, 2016/03/29 21:38:38]

nit: Move this outside the if block, and add a comment on the max-payload if
block below?

[Kevin M, 2016/03/29 23:48:24]

Done.

[Kevin M, 2016/03/29 23:48:24]

Done.

+    sink_->WritePacket(buf, cb);
+    return;
+  }
+  if (uncompressed_size > kMaxPacketPayloadSizeBytes) {
+    cb.Run(net::ERR_FILE_TOO_BIG);
+    return;
+  }
+
+  int compress_result = Compress(buf, compressed_buf_);
+  if (compress_result < 0) {
+    cb.Run(compress_result);
+    return;
+  }
+  uncompressed_size_total_ += uncompressed_size;
+  compressed_size_total_ += compress_result;

[Wez, 2016/03/29 21:38:38]

Do we log these in non-DVLOG enabled builds? If not then suggest wrapping code
touching them in #if !defined(NDEBUG)

[Kevin M, 2016/03/29 23:48:24]

Done.

[Kevin M, 2016/03/29 23:48:24]

Done.

+
+  scoped_refptr<net::DrainableIOBuffer> compressed_outbuf(
+      new net::DrainableIOBuffer(compressed_buf_.get(), compress_result));
+  sink_->WritePacket(compressed_outbuf, cb);
+  DVLOG(4) << "deflate packet: " << uncompressed_size << " in, "
+           << compress_result << " out.";
+  DVLOG(3) << "deflate total: " << uncompressed_size_total_ << " in, "
+           << compressed_size_total_ << " out.";
+}
+
+int CompressedPacketWriter::Compress(
+    const scoped_refptr<net::DrainableIOBuffer>& src_buf,
+    const scoped_refptr<net::GrowableIOBuffer>& dest_buf) {
+  DCHECK_EQ(dest_buf->offset(), 0);

[Wez, 2016/03/29 21:38:38]

nit: Prefer DCHECK_EQ(expected, actual)

nit: Follow pre-condition DCHECK w/ a blank line to make preconditions vs
actually does-work code separation obvious.

[Kevin M, 2016/03/29 23:48:24]

Done.

+  const int zlib_output_ubound =
+      deflateBound(&zlib_stream_, src_buf->BytesRemaining());
+  if (dest_buf->capacity() < zlib_output_ubound) {
+    dest_buf->SetCapacity(zlib_output_ubound);
+  }
+
+  zlib_stream_.next_in = reinterpret_cast<uint8_t*>(src_buf->data());
+  zlib_stream_.avail_in = static_cast<unsigned>(src_buf->BytesRemaining());
+  zlib_stream_.next_out = reinterpret_cast<uint8_t*>(dest_buf->data());
+  zlib_stream_.avail_out = static_cast<unsigned>(zlib_output_ubound);
+  int err = deflate(&zlib_stream_, Z_SYNC_FLUSH);

[Wez, 2016/03/29 21:38:38]

See request above re naming.

[Kevin M, 2016/03/29 23:48:23]

Done.

+
+  if (err != Z_OK) {
+    DLOG(FATAL) << "Unexpected deflate() return value: " << err;
+    return net::ERR_UNEXPECTED;
+  }
+  if (zlib_stream_.avail_in > 0) {
+    DLOG(ERROR) << "deflate() did not consume all data, remainder: "
+                << zlib_stream_.avail_in << " bytes.";
+    return net::ERR_UNEXPECTED;
+  }
+
+  return zlib_output_ubound - zlib_stream_.avail_out;
+}
+
+}  // namespace blimp