Add DesktopFrame rotation functions

webrtc/modules/desktop_capture/desktop_frame_rotator.h

+/*
+ *  Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#ifndef WEBRTC_MODULES_DESKTOP_CAPTURE_DESKTOP_FRAME_ROTATOR_H_

[Sergey Ulanov, 2016/11/17 02:07:39]

suggest renaming it to "desktop_frame_rotation.h"

[Hzj_jie, 2016/11/17 07:14:55]

Done.

+#define WEBRTC_MODULES_DESKTOP_CAPTURE_DESKTOP_FRAME_ROTATOR_H_
+
+#include "webrtc/modules/desktop_capture/desktop_frame.h"
+#include "webrtc/modules/desktop_capture/desktop_geometry.h"
+
+namespace webrtc {
+
+// Represents the rotation of a DesktopFrame.
+enum class Rotation {
+  CLOCK_WISE_0,
+  CLOCK_WISE_90,
+  CLOCK_WISE_180,
+  CLOCK_WISE_270,
+};
+
+// Rotates input DesktopFrame |source|, copies pixel in a rotated rectangle
+// |rect| into another DesktopFrame |target|, and adds |rect| into
+// |target|->mutable_updated_region().
+// |rotation| specifies |source| to |target| rotation. |rect| is in |target|
+// coordinate.

[Sergey Ulanov, 2016/11/17 02:07:39]

I think it would be cleaner, easier to understand the interface if the first
version of that function had rect in source coordinates. That's what I would
expect looking at the function definition. Also it would be best to swap rect
and rotation parameter when rect is in target coordinates, i.e. (source,
rotation, rect, target). That would keep the |rect| argument closer to the frame
argument it refers to.

[Hzj_jie, 2016/11/17 07:14:55]

If the |rect| is in source coordinate, but indeed the DXGI APIs return it in
target coordinate, we will need to provide a DerotateRect() function instead of
RotateRect(). So I prefer to swap the |rotation| and |rect| to avoid
DerotateRect().

[Sergey Ulanov, 2016/11/19 02:11:40]

I don't think you really need to DerotateRect(). Just exposing ReverseRotation()
function here would be enough. That way this file will provide all useful
primitives to do rect rotation, and then the calling code would be easier to
understand without reading these comments.

Rect source_rect = 
  RotateRect(target_rect, ReverseRotation(rotation), source->size());
RotateDesktopRect(source, source_rect, rotation, target);

Yes, that makes the calling code slightly longer, but it's much easier to
understand it.

[Hzj_jie, 2016/11/19 05:10:59]

Done.

But I still do not think exporting RotateRect() and ReverseRotation() functions
are correct. As I have explained in another comment, consumers do not need to
randomly rotate rectangle or rotation.

+// This function triggers check failure if |target| is null, returns false if
+// |target| does not cover the |rect|.
+bool CopyRotatedRectTo(const DesktopFrame& source,

[Sergey Ulanov, 2016/11/17 02:07:39]

Maybe call it RotateDesktopFrame()?

[Hzj_jie, 2016/11/17 07:14:55]

Done.

+                       DesktopRect rect,
+                       Rotation rotation,
+                       DesktopFrame* target);
+
+// Rotates input DesktopFrame |source|, copies pixel in a unrotated rectangle
+// |rect| into another DesktopFrame |target|, and adds rotated |rect| into
+// |target|->mutable_updated_region().
+// |rotation| specifies |source| to |target| rotation. |rect| is in |source|
+// coordinate.
+// This function triggers check failure if |target| is null, returns false if

[Sergey Ulanov, 2016/11/17 02:07:39]

Don't need to mention target==null here. It's obvious that target is not
nullable here.

[Hzj_jie, 2016/11/17 07:14:55]

Done.

+// |target| does not cover the |rect|.
+bool CopyUnrotatedRectTo(const DesktopFrame& source,

[Sergey Ulanov, 2016/11/17 02:07:39]

The reason I suggested to expose RotateRect() function here is because it hard
to name these two functions to make the difference between them clear. Having
just one rotation function would avoid this problem. If you do keep two
functions then I think this one needs better name, but I don't have any
suggestions :(

[Hzj_jie, 2016/11/17 07:14:55]

IMO, these two functions are more like RotateAndCopyRect() and
CopyRectAndRotate(), which show 'rotating', 'copying' and their order, meanwhile
imply the coordinate of 'rect' -- whether it needs to be rotated or not.
More details, RotateAndCopyRect means "rotate a 'DesktopFrame' by 'Rotation',
then copy 'DesktopRect' to the 'target'", while CopyRectAndRotate means "copy a
'DesktopRect' from 'DesktopFrame', then rotate the copied area by 'Rotation',
paste it to 'target'".
As we have discussed before, in functionality level, providing these two
functions equals to providing one function with a rotate rect function.
Providing two functions looks natural to me, as the DesktopRect should be in
either source coordinate or target coordinate, there is no third choice. So
consumers do not really need to randomly rotate a DesktopRect. But I do not
really see a very huge difference. So I applied your suggestion.

+                         DesktopRect rect,
+                         Rotation rotation,
+                         DesktopFrame* target);
+
+}  // namespace webrtc
+
+#endif  // WEBRTC_MODULES_DESKTOP_CAPTURE_DESKTOP_FRAME_ROTATOR_H_