Enable Cyclic Refresh for VP9.

remoting/codec/video_encoder_vpx.cc

 // Copyright 2013 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 "remoting/codec/video_encoder_vpx.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/sys_info.h"
@@ skipping 21 matching lines @@
 const int kBytesPerRgbPixel = 4;
 
 // Defines the dimension of a macro block. This is used to compute the active
 // map for the encoder.
 const int kMacroBlockSize = 16;
 
 // Magic encoder profile numbers for I420 and I444 input formats.
 const int kVp9I420ProfileNumber = 0;
 const int kVp9I444ProfileNumber = 1;
 
+// Magic encoder constants for adaptive quantization strategy
+const int kVp9AqNone = 0;
+const int kVp9AqCyclicRefresh = 3;

[Wez, 2015/04/18 00:17:54]

nit: Suggest these start kVp9AqMode, for consistency with the codec-control enum
name (VP9E_SET_AQ_MODE)

[aconverse, 2015/04/20 18:32:38]

Done.

+
 void SetCommonCodecParameters(vpx_codec_enc_cfg_t* config,
                               const webrtc::DesktopSize& size) {
   // Use millisecond granularity time base.
   config->g_timebase.num = 1;
   config->g_timebase.den = 1000;
 
   config->g_w = size.width();
   config->g_h = size.height();
   config->g_pass = VPX_RC_ONE_PASS;
 
@@ skipping 75 matching lines @@
 
   // Use the lowest level of noise sensitivity so as to spend less time
   // on motion estimation and inter-prediction mode.
   ret = vpx_codec_control(codec, VP9E_SET_NOISE_SENSITIVITY, 0);
   DCHECK_EQ(VPX_CODEC_OK, ret) << "Failed to set noise sensitivity";
 
   // Configure the codec to tune it for screen media.
   ret = vpx_codec_control(
       codec, VP9E_SET_TUNE_CONTENT, VP9E_CONTENT_SCREEN);
   DCHECK_EQ(VPX_CODEC_OK, ret) << "Failed to set screen content mode";
+
+  // Set cyclic refresh for lossy encode

[Wez, 2015/04/18 00:17:54]

nit: missing .

Suggest "Enable cyclic refresh (aka "top-off") only for lossy encoding."

[aconverse, 2015/04/20 18:32:38]

Done.

+  int aq_mode = lossless_encode ? kVp9AqNone : kVp9AqCyclicRefresh;
+  ret = vpx_codec_control(codec, VP9E_SET_AQ_MODE, aq_mode);
+  DCHECK_EQ(VPX_CODEC_OK, ret) << "Failed to set aq mode";
 }
 
 void FreeImageIfMismatched(bool use_i444,
                            const webrtc::DesktopSize& size,
                            scoped_ptr<vpx_image_t>* out_image,
                            scoped_ptr<uint8[]>* out_image_buffer) {
   if (*out_image) {
     const vpx_img_fmt_t desired_fmt =
         use_i444 ? VPX_IMG_FMT_I444 : VPX_IMG_FMT_I420;
     if (!size.equals(webrtc::DesktopSize((*out_image)->w, (*out_image)->h)) ||
@@ skipping 136 matching lines @@
 
   // Do the actual encoding.
   int timestamp = (encode_start_time - timestamp_base_).InMilliseconds();
   vpx_codec_err_t ret = vpx_codec_encode(
       codec_.get(), image_.get(), timestamp, 1, 0, VPX_DL_REALTIME);
   DCHECK_EQ(ret, VPX_CODEC_OK)
       << "Encoding error: " << vpx_codec_err_to_string(ret) << "\n"
       << "Details: " << vpx_codec_error(codec_.get()) << "\n"
       << vpx_codec_error_detail(codec_.get());
 
+  if (use_vp9_) {
+    if (vpx_codec_control(codec_.get(), VP9E_GET_ACTIVEMAP, &act_map) ==
+        VPX_CODEC_OK) {

[Wez, 2015/04/18 00:17:54]

Is there any point calling this if we are already running w/ lossless encode?

[aconverse, 2015/04/20 18:32:39]

Done.

+      UpdateActiveMap(&updated_region);
+    } else {
+      LOG(ERROR) << "Failed to fetch active map";
+      updated_region.Clear();
+      updated_region.AddRect(webrtc::DesktopRect::MakeWH(image_->w, image_->h));

[Wez, 2015/04/18 00:17:54]

Under what circumstances can the get call fail? Should we just CHECK or
NOTREACHED here, instead?

[aconverse, 2015/04/20 18:32:39]

I was following the example of SET_ACTIVEMAP. Neither call should fail unless
the map size doesn't match the image size.

Changed.

[Wez, 2015/04/21 01:29:18]

Ah, OK; we should probably have SET_ACTIVEMAP errors be more fatal; not a
problem for this CL, though.

+    }
+  }
+
   // Read the encoded data.
   vpx_codec_iter_t iter = NULL;
   bool got_data = false;
 
   // TODO(hclam): Make sure we get exactly one frame from the packet.
   // TODO(hclam): We should provide the output buffer to avoid one copy.
   scoped_ptr<VideoPacket> packet(
       helper_.CreateVideoPacketWithUpdatedRegion(frame, updated_region));
   packet->mutable_format()->set_encoding(VideoPacketFormat::ENCODING_VP8);
 
@@ skipping 197 matching lines @@
 
     uint8* map = active_map_.get() + top * active_map_width_;
     for (int y = top; y <= bottom; ++y) {
       for (int x = left; x <= right; ++x)
         map[x] = 1;
       map += active_map_width_;
     }
   }
 }
 
+void VideoEncoderVpx::UpdateActiveMap(webrtc::DesktopRegion *updated_region) {
+  uint8* map = active_map_.get();
+  // Mark active areas updated.

[Wez, 2015/04/18 00:17:54]

nit: This comment doesn't fit this function; we're fetching the active areas,
not marking them. :)

[aconverse, 2015/04/20 18:32:38]

We've already fetched the areas. We are marking them in the |updated_region|.

This is the inverse of the previous comment "Mark updated areas active."

[Wez, 2015/04/21 01:29:18]

OK; suggest "Add areas marked active to |updated_region|", or just removing the
comment entirely, since it's already commented on the function declaration.

+  for (int r = 0; r < active_map_height_; ++r) {

[Wez, 2015/04/18 00:17:54]

Suggest r -> y

[aconverse, 2015/04/20 18:32:39]

Done.

+    int c1;

[Wez, 2015/04/18 00:17:54]

Move this inside the c0 loop?

[aconverse, 2015/04/20 18:32:38]

This is used in the iteration condition so that we are acting on lines of 16x16
blocks and not individual 16x16 blocks.

[Wez, 2015/04/21 01:29:18]

Gotcha; I think it might be more readable to declare x1 inside the for(x0) loop,
and initialize it to x0, then do the x0 = x1 + 1 assignment at the end of the
loop, after the if().

[aconverse, 2015/04/28 17:03:56]

Done. lmk if I misunderstood

+    for (int c0 = 0; c0 < active_map_height_; c0 = c1 + 1) {

[Wez, 2015/04/18 00:17:54]

Suggest c0 -> x0

[Wez, 2015/04/18 00:17:54]

Looks like this should be |active_map_width_|?

[aconverse, 2015/04/20 18:32:39]

Done.

[aconverse, 2015/04/20 18:32:39]

Done.

+      for (c1 = c0; c1 < active_map_height_; ++c1) {

[Wez, 2015/04/18 00:17:54]

Ditto.

[aconverse, 2015/04/20 18:32:38]

Done.

+        if (map[c1] == 0)

[aconverse, 2015/04/20 18:32:39]

Big oops on this line as well. [not taking row into account].

This whole function never should have worked at all as written. It appears that
the client is ignoring the updated region. (Demonstrated by clearing
|updated_region| and setting it to 300x300 and seeing changes across a whole
desktop.) Was that an intentional change? If so we can remove this whole
function.

[Wez, 2015/04/21 01:29:18]

Yes and no; no, we can't remove this, because we do want the updated-region to
be sent so that the client can optimize its rendering. But yes, the client
currently ignores it because we use the pp::VideoDecoder API and render them via
Graphics3D/GLES2, so there's no real need to optimize that via the
updated-region.

You can use the remoting.connectedView.enableDebugRegion(true) option to have
the client display the updated region, BTW.

[aconverse, 2015/04/28 17:03:56]

I should give this a try before we move forward. 

+          break;
+      }
+      if (c1 > c0) {
+        updated_region->AddRect(webrtc::DesktopRect::MakeLTRB(
+            kMacroBlockSize * c0,
+            kMacroBlockSize * r,
+            kMacroBlockSize * c1,
+            kMacroBlockSize * (r + 1)));
+      }
+    }
+  }
+  updated_region->IntersectWith(
+    webrtc::DesktopRect::MakeWH(image_->w, image_->h));
+}
+
 }  // namespace remoting