Use correct device scale factors in Aura

Use correct device scale factors in Aura

The image transport surface now passes a scale factor. Set
the current_surface_ to use this scale factor instead of the
view's scale factor.

Fix a number of situations where the correct scale factor
from rendering was available, but the view's scale factor
was used instead.

BUG=132714
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202400

[ccameron, 2013-05-23 23:49:28]

This builds on three patches (that are slowly making their
way through the CQ).

I tested this manually by forcing renderers to (incorrectly)
use a 2x scale factor, but I'd like this scrutinized very
carefully, because my testing did not hit every path (esp
the software rendering paths).

Of note is that Aura isn't robust to non-integer scale factors
because it assumes that there are no rounding errors when
converting between DIP and pixels. This may be of concern in
the future.

https://codereview.chromium.org/15666007/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/15666007/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:1221:
ConvertSizeToPixel(this, dst_size),
This is the only remaining instance where we use the screen's scale factor (for
the destination surface). This seems reasonable, but might need more eyes.

https://codereview.chromium.org/15666007/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:1534:
ConvertRectToDIP(frame_device_scale_factor, damage_rect));
This looks to be an unrelated bug, but came up in my audit of ^Convert.*this$
instances.

[piman, 2013-05-24 01:03:41]

LGTM+nits. Thanks!

https://codereview.chromium.org/15666007/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/dip_util.h (right):

https://codereview.chromium.org/15666007/diff/1/content/browser/renderer_host...
content/browser/renderer_host/dip_util.h:28: const gfx::Size& size_in_pixel);
nit: the style guide eschews overloading... 
Maybe rename to ConvertSizeToDIPExplicit or something? Trying to find a good
name that's not too wordy.
Or maybe rename the old ones ConvertViewSizeToDIP etc? Turns out I think most of
them aren't used any more anyway, maybe we could simply  remove them?

https://codereview.chromium.org/15666007/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/15666007/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:1221:
ConvertSizeToPixel(this, dst_size),
On 2013/05/23 23:49:28, ccameron1 wrote:
> This is the only remaining instance where we use the screen's scale factor
(for
> the destination surface). This seems reasonable, but might need more eyes.

SGTM. The dst_size passed here is in DIP and is independent of the renderer
surface size.
Arguably it should probably be in pixels to CopyFromCompositingSurface but
that's another story.

https://codereview.chromium.org/15666007/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:1519:
gfx::ScaleSize(frame_size, 1.0f / frame_device_scale_factor));
nit: could use the new version of ConvertSizeToDIP here.

https://codereview.chromium.org/15666007/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:1534:
ConvertRectToDIP(frame_device_scale_factor, damage_rect));
On 2013/05/23 23:49:28, ccameron1 wrote:
> This looks to be an unrelated bug, but came up in my audit of ^Convert.*this$
> instances.

good catch.

[ccameron, 2013-05-24 09:45:03]

Thanks!!

https://codereview.chromium.org/15666007/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/dip_util.h (right):

https://codereview.chromium.org/15666007/diff/1/content/browser/renderer_host...
content/browser/renderer_host/dip_util.h:28: const gfx::Size& size_in_pixel);
On 2013/05/24 01:03:41, piman wrote:
> nit: the style guide eschews overloading... 
> Maybe rename to ConvertSizeToDIPExplicit or something? Trying to find a good
> name that's not too wordy.
> Or maybe rename the old ones ConvertViewSizeToDIP etc? Turns out I think most
of
> them aren't used any more anyway, maybe we could simply  remove them?

Good idea -- I pruned them down to the functions that are used and changed the
view ones to ConvertView*.

https://codereview.chromium.org/15666007/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/15666007/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:1519:
gfx::ScaleSize(frame_size, 1.0f / frame_device_scale_factor));
On 2013/05/24 01:03:41, piman wrote:
> nit: could use the new version of ConvertSizeToDIP here.

Done.

[piman, 2013-05-24 16:12:19]

LGTM, thanks!

[commit-bot: I haz the power, 2013-05-27 08:13:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ccameron@chromium.org/15666007/6001

[commit-bot: I haz the power, 2013-05-27 08:13:22]

Failed to apply patch for
content/browser/renderer_host/render_widget_host_view_aura.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/renderer_host/render_widget_host_view_aura.cc
  Hunk #2 succeeded at 1199 (offset 3 lines).
  Hunk #3 succeeded at 1221 (offset 3 lines).
  Hunk #4 succeeded at 1322 (offset 3 lines).
  Hunk #5 succeeded at 1339 (offset 3 lines).
  Hunk #6 succeeded at 1349 (offset 3 lines).
  Hunk #7 succeeded at 1362 (offset 3 lines).
  Hunk #8 succeeded at 1388 (offset 3 lines).
  Hunk #9 succeeded at 1441 (offset 3 lines).
  Hunk #10 succeeded at 1510 (offset 3 lines).
  Hunk #11 succeeded at 1518 (offset 3 lines).
  Hunk #12 succeeded at 1533 (offset 3 lines).
  Hunk #13 FAILED at 1580.
  Hunk #14 succeeded at 1612 (offset 13 lines).
  Hunk #15 succeeded at 1637 (offset 13 lines).
  Hunk #16 succeeded at 1672 (offset 13 lines).
  Hunk #17 succeeded at 2255 (offset 13 lines).
  1 out of 17 hunks FAILED -- saving rejects to file
content/browser/renderer_host/render_widget_host_view_aura.cc.rej

Patch:       content/browser/renderer_host/render_widget_host_view_aura.cc
Index: content/browser/renderer_host/render_widget_host_view_aura.cc
diff --git a/content/browser/renderer_host/render_widget_host_view_aura.cc
b/content/browser/renderer_host/render_widget_host_view_aura.cc
index
0da585350faa3e1e3fd09dbd9c5df1c4a8cde9d5..31af7a03ca7726a00384d18e3a4427ed7d9823f7
100644
--- a/content/browser/renderer_host/render_widget_host_view_aura.cc
+++ b/content/browser/renderer_host/render_widget_host_view_aura.cc
@@ -632,6 +632,7 @@
RenderWidgetHostViewAura::RenderWidgetHostViewAura(RenderWidgetHost* host)
       text_input_type_(ui::TEXT_INPUT_TYPE_NONE),
       can_compose_inline_(true),
       has_composition_text_(false),
+      last_swapped_surface_scale_factor_(1.f),
       paint_canvas_(NULL),
       synthetic_move_sent_(false),
       accelerated_compositing_state_changed_(false),
@@ -1195,7 +1196,9 @@ void
RenderWidgetHostViewAura::CopyFromCompositingSurfaceHelper(
   gfx::Rect src_subrect_in_gl = src_subrect;
   src_subrect_in_gl.set_y(GetViewBounds().height() - src_subrect.bottom());
 
-  gfx::Rect src_subrect_in_pixel = ConvertRectToPixel(this, src_subrect_in_gl);
+  gfx::Rect src_subrect_in_pixel =
+      ConvertRectToPixel(current_surface_->device_scale_factor(),
+                         src_subrect_in_gl);
   gl_helper->CropScaleReadbackAndCleanTexture(
       current_surface_->PrepareTexture(),
       current_surface_->size(),
@@ -1215,7 +1218,7 @@ void RenderWidgetHostViewAura::CopyFromCompositingSurface(
   }
 
   CopyFromCompositingSurfaceHelper(src_subrect,
-                                   ConvertSizeToPixel(this, dst_size),
+                                   ConvertViewSizeToPixel(this, dst_size),
                                    callback);
 }
 
@@ -1316,11 +1319,13 @@ void RenderWidgetHostViewAura::UpdateExternalTexture() {
   bool is_compositing_active = host_->is_accelerated_compositing_active();
   if (is_compositing_active && current_surface_) {
     window_->SetExternalTexture(current_surface_.get());
-    current_frame_size_ = ConvertSizeToDIP(this, current_surface_->size());
+    current_frame_size_ = ConvertSizeToDIP(
+        current_surface_->device_scale_factor(), current_surface_->size());
     CheckResizeLock();
   } else if (is_compositing_active && current_dib_) {
     window_->SetExternalTexture(NULL);
-    current_frame_size_ = ConvertSizeToDIP(this, last_swapped_surface_size_);
+    current_frame_size_ = ConvertSizeToDIP(last_swapped_surface_scale_factor_,
+                                           last_swapped_surface_size_);
     CheckResizeLock();
   } else {
     window_->SetExternalTexture(NULL);
@@ -1331,6 +1336,7 @@ void RenderWidgetHostViewAura::UpdateExternalTexture() {
 
 bool RenderWidgetHostViewAura::SwapBuffersPrepare(
     const gfx::Rect& surface_rect,
+    float surface_scale_factor,
     const gfx::Rect& damage_rect,
     const std::string& mailbox_name,
     const BufferPresentedCallback& ack_callback) {
@@ -1340,9 +1346,11 @@ bool RenderWidgetHostViewAura::SwapBuffersPrepare(
     DLOG_IF(ERROR, damage_rect != surface_rect) << "Expected full damage rect";
     skipped_damage_.setEmpty();
     last_swapped_surface_size_ = surface_rect.size();
+    last_swapped_surface_scale_factor_ = surface_scale_factor;
   }
 
-  if (ShouldSkipFrame(ConvertSizeToDIP(this, surface_rect.size())) ||
+  if (ShouldSkipFrame(ConvertSizeToDIP(surface_scale_factor,
+                                       surface_rect.size())) ||
       mailbox_name.empty()) {
     skipped_damage_.op(RectToSkIRect(damage_rect), SkRegion::kUnion_Op);
     ack_callback.Run(true, scoped_refptr<ui::Texture>());
@@ -1351,7 +1359,7 @@ bool RenderWidgetHostViewAura::SwapBuffersPrepare(
 
   ImageTransportFactory* factory = ImageTransportFactory::GetInstance();
   current_surface_ =
-      factory->CreateTransportClient(current_device_scale_factor_);
+      factory->CreateTransportClient(surface_scale_factor);
   if (!current_surface_) {
     LOG(ERROR) << "Failed to create ImageTransport texture";
     ack_callback.Run(true, scoped_refptr<ui::Texture>());
@@ -1377,7 +1385,8 @@ void RenderWidgetHostViewAura::SwapBuffersCompleted(
     if (frame_subscriber()->ShouldCaptureFrame(present_time,
                                                &frame, &callback)) {
       CopyFromCompositingSurfaceToVideoFrame(
-          gfx::Rect(ConvertSizeToDIP(this, current_surface_->size())),
+          gfx::Rect(ConvertSizeToDIP(current_surface_->device_scale_factor(),
+                                     current_surface_->size())),
           frame,
           base::Bind(callback, present_time));
     }
@@ -1429,7 +1438,8 @@ void
RenderWidgetHostViewAura::AcceleratedSurfaceBuffersSwapped(
       gpu_host_id,
       params_in_pixel.mailbox_name);
   BuffersSwapped(
-      params_in_pixel.size, params_in_pixel.mailbox_name, ack_callback);
+      params_in_pixel.size, params_in_pixel.scale_factor,
+      params_in_pixel.mailbox_name, ack_callback);
 }
 
 void RenderWidgetHostViewAura::SwapDelegatedFrame(
@@ -1497,6 +1507,7 @@ void RenderWidgetHostViewAura::SwapSoftwareFrame(
   current_dib_.reset(dib.release());
   current_dib_id_ = dib_id;
   last_swapped_surface_size_ = frame_size;
+  last_swapped_surface_scale_factor_ = frame_device_scale_factor;
 
   ui::Compositor* compositor = GetCompositor();
   if (!compositor) {
@@ -1504,8 +1515,8 @@ void RenderWidgetHostViewAura::SwapSoftwareFrame(
     return;
   }
 
-  gfx::Size frame_size_in_dip = gfx::ToFlooredSize(
-      gfx::ScaleSize(frame_size, 1.0f / frame_device_scale_factor));
+  gfx::Size frame_size_in_dip =
+      ConvertSizeToDIP(frame_device_scale_factor, frame_size);
   if (ShouldSkipFrame(frame_size_in_dip)) {
     can_lock_compositor_ = NO_PENDING_COMMIT;
     SendSoftwareFrameAck(last_dib_id);
@@ -1519,7 +1530,8 @@ void RenderWidgetHostViewAura::SwapSoftwareFrame(
   CheckResizeLock();
   released_front_lock_ = NULL;
   window_->SetExternalTexture(NULL);
-  window_->SchedulePaintInRect(ConvertRectToDIP(this, damage_rect));
+  window_->SchedulePaintInRect(
+      ConvertRectToDIP(frame_device_scale_factor, damage_rect));
 
   if (paint_observer_)
     paint_observer_->OnUpdateCompositorContent();
@@ -1568,17 +1580,19 @@ void RenderWidgetHostViewAura::OnSwapCompositorFrame(
       reinterpret_cast<const char*>(frame->gl_frame_data->mailbox.name),
       sizeof(frame->gl_frame_data->mailbox.name));
   BuffersSwapped(
-      frame->gl_frame_data->size, mailbox_name, ack_callback);
+      frame->gl_frame_data->size, frame->metadata.device_scale_factor,
+      mailbox_name, ack_callback);
 }
 
 void RenderWidgetHostViewAura::BuffersSwapped(
     const gfx::Size& size,
+    float surface_scale_factor,
     const std::string& mailbox_name,
     const BufferPresentedCallback& ack_callback) {
   scoped_refptr<ui::Texture> texture_to_return(current_surface_);
   const gfx::Rect surface_rect = gfx::Rect(size);
-  if (!SwapBuffersPrepare(
-      surface_rect, surface_rect, mailbox_name, ack_callback)) {
+  if (!SwapBuffersPrepare(surface_rect, surface_scale_factor, surface_rect,
+                          mailbox_name, ack_callback)) {
     return;
   }
 
@@ -1587,7 +1601,7 @@ void RenderWidgetHostViewAura::BuffersSwapped(
 
   ui::Compositor* compositor = GetCompositor();
   if (compositor) {
-    gfx::Size surface_size = ConvertSizeToDIP(this, size);
+    gfx::Size surface_size = ConvertSizeToDIP(surface_scale_factor, size);
     window_->SchedulePaintInRect(gfx::Rect(surface_size));
   }
 
@@ -1612,7 +1626,8 @@ void
RenderWidgetHostViewAura::AcceleratedSurfacePostSubBuffer(
       params_in_pixel.mailbox_name);
 
   if (!SwapBuffersPrepare(
-      surface_rect, damage_rect, params_in_pixel.mailbox_name, ack_callback)) {
+      surface_rect, params_in_pixel.surface_scale_factor, damage_rect,
+      params_in_pixel.mailbox_name, ack_callback)) {
     return;
   }
 
@@ -1646,12 +1661,13 @@ void
RenderWidgetHostViewAura::AcceleratedSurfacePostSubBuffer(
   if (compositor) {
     // Co-ordinates come in OpenGL co-ordinate space.
     // We need to convert to layer space.
-    gfx::Rect rect_to_paint = ConvertRectToDIP(this, gfx::Rect(
-        params_in_pixel.x,
-        surface_size_in_pixel.height() - params_in_pixel.y -
-        params_in_pixel.height,
-        params_in_pixel.width,
-        params_in_pixel.height));
+    gfx::Rect rect_to_paint = ConvertRectToDIP(
+        params_in_pixel.surface_scale_factor,
+        gfx::Rect(params_in_pixel.x,
+                  surface_size_in_pixel.height() - params_in_pixel.y -
+                      params_in_pixel.height,
+                  params_in_pixel.width,
+                  params_in_pixel.height));
 
     // Damage may not have been DIP aligned, so inflate damage to compensate
     // for any round-off error.
@@ -2228,7 +224…
(message too large)

[commit-bot: I haz the power, 2013-05-27 08:44:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ccameron@chromium.org/15666007/21001

[commit-bot: I haz the power, 2013-05-27 08:59:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ccameron@chromium.org/15666007/33002

[commit-bot: I haz the power, 2013-05-27 12:52:50]

Change committed as 202400