Enable Cyclic Refresh for VP9.

remoting/codec/video_encoder_vpx.cc

Index: remoting/codec/video_encoder_vpx.cc
diff --git a/remoting/codec/video_encoder_vpx.cc b/remoting/codec/video_encoder_vpx.cc
index b9a04e76469dd9ce469ef46ab22b7ddf24a76daa..6e4ea693727ec1b39e99aeab2111208acd3866e4 100644
--- a/remoting/codec/video_encoder_vpx.cc
+++ b/remoting/codec/video_encoder_vpx.cc
@@ -39,6 +39,10 @@ const int kMacroBlockSize = 16;
 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.
@@ -134,6 +138,11 @@ void SetVp9CodecOptions(vpx_codec_ctx_t* codec, bool lossless_encode) {
   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,
@@ -290,6 +299,17 @@ scoped_ptr<VideoPacket> VideoEncoderVpx::Encode(
       << "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;
@@ -507,4 +527,27 @@ void VideoEncoderVpx::PrepareActiveMap(
   }
 }
 
+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