Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2500883004:
Add DesktopFrame rotation functions (Closed)
DescriptionAdd DesktopFrame rotation functions
This change adds RotateDesktopFrame(), RotateRect(), RotateSize(),
ReverseRotate() functions, so an implementation can use these free functions to
rotate and copy pixels from one DesktopFrame to another at the same time.
This is the first part of the change to support rotation in DirectX capturer. In
a coming change, these functions will be used in DxgiOutputDuplicator to do the
rotation and copying.
Background,
DirectX APIs always return unrotated data buffer, so we need to rotate it to
match the user-selected rotation. What worse is except for the data buffer,
other variables return by these APIs are all rotated, e.g. output size, monitor
position. So we will eventually not be able to capture the rotated monitors,
because we cannot set their position and size correctly. Though
DXGI_OUTDUPL_DESC provides a DXGI_MODE_ROTATION enumeration to indicate the
output rotation, it does not provide a simple way to rotate an IDXGIResource,
which is the only thing we can get from duplication APIs. A typical user case
here is to use a matrix to transform the IDXGIResource and render it to a
surface. But since we do not render the IDXGIResource at all, we need to
manually rotate it.
BUG=314516
Committed: https://crrev.com/90ea7362fc81db6a677d129a9161ee3d49b77ec1
Cr-Commit-Position: refs/heads/master@{#15205}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Resolve review comments #
Total comments: 32
Patch Set 3 : Resolve review comments #
Total comments: 32
Patch Set 4 : Resolve review comments #
Total comments: 10
Patch Set 5 : Resolve review comments #
Total comments: 2
Patch Set 6 : Resolve review comments #
Total comments: 6
Patch Set 7 : Resolve review comments #Patch Set 8 : Sync latest changes #Messages
Total messages: 97 (77 generated)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
Description was changed from ========== Lint error Lint error DesktopFrameRotator Correct change DirectX capturer flickers on the second monitor BUG= ========== to ========== Add DesktopFrame rotator functions This change adds a CopyRotatedRectTo() and CopyUnrotatedRectTo() function, so an implementation can use these two free functions to rotate and copy pixels from one DesktopFrame to another at the same time. I cannot tell whether the CopyRotatedRectTo() or CopyUnrotatedRectTo() should be used in DirectX capturer, i.e. whether the updated_region returns from DetectUpdatedRegion() is rotated or not. So I added both, though one is only a combination of the other with several annoymous functions. Background, DirectX APIs always return unrotated data buffer, so we need to rotate it to the user-selected rotation. What worse is except for the data buffer, other variables return by these APIs are all rotated, e.g. output size. So we will eventually not be able to capture the rotated monitors, because we cannot set their position and size correctly. Though DXGI_OUTDUPL_DESC provides a DXGI_MODE_ROTATION enumeration to indicate the output rotation, it does not provide a simple way to rotate an IDXGIResource, which is the only thing we can get from duplication APIs. A typical user case here is to use a matrix to transform the IDXGIResource and render it to a surface. But since we do not render the IDXGIResource at all, we need to manually rotate it. BUG=314516 ==========
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_mips_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_mips_db...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Add DesktopFrame rotator functions This change adds a CopyRotatedRectTo() and CopyUnrotatedRectTo() function, so an implementation can use these two free functions to rotate and copy pixels from one DesktopFrame to another at the same time. I cannot tell whether the CopyRotatedRectTo() or CopyUnrotatedRectTo() should be used in DirectX capturer, i.e. whether the updated_region returns from DetectUpdatedRegion() is rotated or not. So I added both, though one is only a combination of the other with several annoymous functions. Background, DirectX APIs always return unrotated data buffer, so we need to rotate it to the user-selected rotation. What worse is except for the data buffer, other variables return by these APIs are all rotated, e.g. output size. So we will eventually not be able to capture the rotated monitors, because we cannot set their position and size correctly. Though DXGI_OUTDUPL_DESC provides a DXGI_MODE_ROTATION enumeration to indicate the output rotation, it does not provide a simple way to rotate an IDXGIResource, which is the only thing we can get from duplication APIs. A typical user case here is to use a matrix to transform the IDXGIResource and render it to a surface. But since we do not render the IDXGIResource at all, we need to manually rotate it. BUG=314516 ========== to ========== Add DesktopFrame rotator functions This change adds a CopyRotatedRectTo() and CopyUnrotatedRectTo() function, so an implementation can use these two free functions to rotate and copy pixels from one DesktopFrame to another at the same time. I cannot tell whether the CopyRotatedRectTo() or CopyUnrotatedRectTo() should be used in DirectX capturer, i.e. whether the updated_region returns from DetectUpdatedRegion() is rotated or not. So I added both, though one is only a combination of the other with several annoymous functions. Background, DirectX APIs always return unrotated data buffer, so we need to rotate it to match the user-selected rotation. What worse is except for the data buffer, other variables return by these APIs are all rotated, e.g. output size. So we will eventually not be able to capture the rotated monitors, because we cannot set their position and size correctly. Though DXGI_OUTDUPL_DESC provides a DXGI_MODE_ROTATION enumeration to indicate the output rotation, it does not provide a simple way to rotate an IDXGIResource, which is the only thing we can get from duplication APIs. A typical user case here is to use a matrix to transform the IDXGIResource and render it to a surface. But since we do not render the IDXGIResource at all, we need to manually rotate it. BUG=314516 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add DesktopFrame rotator functions This change adds a CopyRotatedRectTo() and CopyUnrotatedRectTo() function, so an implementation can use these two free functions to rotate and copy pixels from one DesktopFrame to another at the same time. I cannot tell whether the CopyRotatedRectTo() or CopyUnrotatedRectTo() should be used in DirectX capturer, i.e. whether the updated_region returns from DetectUpdatedRegion() is rotated or not. So I added both, though one is only a combination of the other with several annoymous functions. Background, DirectX APIs always return unrotated data buffer, so we need to rotate it to match the user-selected rotation. What worse is except for the data buffer, other variables return by these APIs are all rotated, e.g. output size. So we will eventually not be able to capture the rotated monitors, because we cannot set their position and size correctly. Though DXGI_OUTDUPL_DESC provides a DXGI_MODE_ROTATION enumeration to indicate the output rotation, it does not provide a simple way to rotate an IDXGIResource, which is the only thing we can get from duplication APIs. A typical user case here is to use a matrix to transform the IDXGIResource and render it to a surface. But since we do not render the IDXGIResource at all, we need to manually rotate it. BUG=314516 ========== to ========== Add DesktopFrame rotator functions This change adds a CopyRotatedRectTo() and CopyUnrotatedRectTo() function, so an implementation can use these two free functions to rotate and copy pixels from one DesktopFrame to another at the same time. I cannot tell whether the CopyRotatedRectTo() or CopyUnrotatedRectTo() should be used in DirectX capturer, i.e. whether the updated_region returns from DetectUpdatedRegion() is rotated or not. So I added both, though one is only a combination of the other with several annoymous functions. This is the first part of the change to support rotation in DirectX capturer. In a coming change, one of these two functions will be used in DxgiOutputDuplicator to do the rotation and copying. Background, DirectX APIs always return unrotated data buffer, so we need to rotate it to match the user-selected rotation. What worse is except for the data buffer, other variables return by these APIs are all rotated, e.g. output size. So we will eventually not be able to capture the rotated monitors, because we cannot set their position and size correctly. Though DXGI_OUTDUPL_DESC provides a DXGI_MODE_ROTATION enumeration to indicate the output rotation, it does not provide a simple way to rotate an IDXGIResource, which is the only thing we can get from duplication APIs. A typical user case here is to use a matrix to transform the IDXGIResource and render it to a surface. But since we do not render the IDXGIResource at all, we need to manually rotate it. BUG=314516 ==========
zijiehe@chromium.org changed reviewers: + jamiewalch@chromium.org, sergeyu@chromium.org
Do not be nervous, less than 250 lines of this change are real logic change, others are for test cases or test support functions.
https://codereview.webrtc.org/2500883004/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/desktop_frame.h (right): https://codereview.webrtc.org/2500883004/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/desktop_frame.h:81: void Paint(DesktopVector pos, RgbaColor color); Do we really need this function here? RgbaColor is currently marked as tests-only (see the comments in the header) and I'd prefer to keep it restricted to tests. If you use this function only in tests then maybe add make it a static function in a separate test_utils file. Same applies for DataEquals() and Clear(). https://codereview.webrtc.org/2500883004/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/desktop_frame_rotator.h (right): https://codereview.webrtc.org/2500883004/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/desktop_frame_rotator.h:31: bool CopyRotatedRectTo(const DesktopFrame& src, It's better to use libyuv::ARGBRotate() instead of reimplementing that functionality here. libyuv is optimized with SSE and so it should be significanlty faster.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #2 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2500883004/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/desktop_frame.h (right): https://codereview.webrtc.org/2500883004/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/desktop_frame.h:81: void Paint(DesktopVector pos, RgbaColor color); On 2016/11/15 02:28:10, Sergey Ulanov wrote: > Do we really need this function here? RgbaColor is currently marked as > tests-only (see the comments in the header) and I'd prefer to keep it restricted > to tests. If you use this function only in tests then maybe add make it a static > function in a separate test_utils file. Same applies for DataEquals() and > Clear(). Sure. Updated. https://codereview.webrtc.org/2500883004/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/desktop_frame_rotator.h (right): https://codereview.webrtc.org/2500883004/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/desktop_frame_rotator.h:31: bool CopyRotatedRectTo(const DesktopFrame& src, On 2016/11/15 02:28:10, Sergey Ulanov wrote: > It's better to use libyuv::ARGBRotate() instead of reimplementing that > functionality here. libyuv is optimized with SSE and so it should be > significanlty faster. That's too bad, I have not found this function. I will test its performance. Yes, libyuv::ARGBRotate() is 1x faster than the original C version. So I use it in the implementation.
Patchset #2 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/desktop_frame_rotator.cc (right): https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:25: DesktopRect Rotate(DesktopRect rect, DesktopSize size, Rotation rotation) { Maybe call it RotateRect()? https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:33: return DesktopRect::MakeLTRB( nit: this would be slightly simpler with MakeXYWH() - it would make it clear that rect size doesn't change and the math for the last two parameters would be simpler. https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:39: default: Don't need default case. Maybe add NOTREACHED after switch. https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:56: default: don't need default case. https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:64: DesktopRect Derotate(DesktopRect rect, DesktopSize size, Rotation rotation) { I don't think you need this function, given that it's simple and is used in only one place. https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:69: DesktopSize Derotate(DesktopSize size, Rotation rotation) { GetRotatedSize()? https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:83: libyuv::RotationMode ToYuvRotationMode(Rotation rotation) { ToLibyuvRotationMode https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:105: if (!target) { Do you really need this? Maybe replace with a DCHECK? https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:111: if (rect.is_empty()) { Do we need this special case? I think CopyPixelsFrom() and libyuv::ARGBRotate() should handle zero size rect correctly. https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:118: // DesktopFrame::CopyPixelsFrom() for a better performance. Does it actually perform better? I would guess libyuv::ARGBRotate() should already have optimizations for this case, so this special case isn't necessary. https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/desktop_frame_rotator.h (right): https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.h:31: bool CopyRotatedRectTo(const DesktopFrame& src, s/src/source/ https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.h:36: // Rotates input DesktopFrame |src|, copies pixel in a unrotated rectangle I'm not sure I understand the difference between these two functions. Is is correct interpretation that |rotation| always specifies src->target rotation, and |rect| is rotated for the first function but not the second one, and that's the only difference between them? It would be useful to say "|rect| is in |src| coordinates" or "|rect| is in |target|" coordinates. Do we need both functions? As alternative we could have one function to rotate a rect and RotateRect() to help with the reverse operation. https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/desktop_frame_rotator_unittest.cc (right): https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator_unittest.cc:96: PaintDesktopFrame(&expected, DesktopVector(0, 0), RgbaColor(11)); This code would be much simpler with a simple memcpy() from a static uint32_t array. https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator_unittest.cc:132: PaintDesktopFrame(&expected, DesktopVector(0, 0), RgbaColor(3)); Instead of initializing expected frame, maybe just memcmp() the data() with a static array, e.g. uint32_t expected_values[] { 3, 7, 11, // 2, 6, 10, // 1, 5, 9, // 0, 4, 8, // } EXPECT_EQ(0, memcpy(target->data(), expected_values, sizeof(expected_values))); This is a much simpler code and you wouldn't need to comment that shows the matrix. https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator_unittest.cc:514: // rotating one 4096 x 3072 frame uses around 67ms. Is that the number you get with libyuv? https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/test_utils.cc (right): https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/test_utils.cc:50: if (memcmp(my_this_line, right_this_line, DesktopFrame::kBytesPerPixel) != Why not compare the whole line, instead of comparing each pixel separately?
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_dbg/builds/8327)
https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/desktop_frame_rotator.cc (right): https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:25: DesktopRect Rotate(DesktopRect rect, DesktopSize size, Rotation rotation) { On 2016/11/15 20:50:22, Sergey Ulanov wrote: > Maybe call it RotateRect()? Done. https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:33: return DesktopRect::MakeLTRB( On 2016/11/15 20:50:22, Sergey Ulanov wrote: > nit: this would be slightly simpler with MakeXYWH() - it would make it clear > that rect size doesn't change and the math for the last two parameters would be > simpler. Done. https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:39: default: On 2016/11/15 20:50:22, Sergey Ulanov wrote: > Don't need default case. Maybe add NOTREACHED after switch. Done. https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:56: default: On 2016/11/15 20:50:22, Sergey Ulanov wrote: > don't need default case. Done. https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:64: DesktopRect Derotate(DesktopRect rect, DesktopSize size, Rotation rotation) { On 2016/11/15 20:50:22, Sergey Ulanov wrote: > I don't think you need this function, given that it's simple and is used in only > one place. Done. https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:69: DesktopSize Derotate(DesktopSize size, Rotation rotation) { On 2016/11/15 20:50:22, Sergey Ulanov wrote: > GetRotatedSize()? Done. https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:83: libyuv::RotationMode ToYuvRotationMode(Rotation rotation) { On 2016/11/15 20:50:22, Sergey Ulanov wrote: > ToLibyuvRotationMode Done. https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:105: if (!target) { On 2016/11/15 20:50:22, Sergey Ulanov wrote: > Do you really need this? Maybe replace with a DCHECK? Done. https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:111: if (rect.is_empty()) { On 2016/11/15 20:50:22, Sergey Ulanov wrote: > Do we need this special case? I think CopyPixelsFrom() and libyuv::ARGBRotate() > should handle zero size rect correctly. When the input rectangle is empty, ARGBRotate will return -1 (error), which is a little bit different than our expectation. https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:118: // DesktopFrame::CopyPixelsFrom() for a better performance. On 2016/11/15 20:50:22, Sergey Ulanov wrote: > Does it actually perform better? I would guess libyuv::ARGBRotate() should > already have optimizations for this case, so this special case isn't necessary. Done. No, it's for old version, and should be removed. https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/desktop_frame_rotator.h (right): https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.h:31: bool CopyRotatedRectTo(const DesktopFrame& src, On 2016/11/15 20:50:22, Sergey Ulanov wrote: > s/src/source/ Done. https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.h:36: // Rotates input DesktopFrame |src|, copies pixel in a unrotated rectangle On 2016/11/15 20:50:22, Sergey Ulanov wrote: > I'm not sure I understand the difference between these two functions. Is is > correct interpretation that |rotation| always specifies src->target rotation, > and |rect| is rotated for the first function but not the second one, and that's > the only difference between them? It would be useful to say "|rect| is in |src| > coordinates" or "|rect| is in |target|" coordinates. > Do we need both functions? As alternative we could have one function to rotate a > rect and RotateRect() to help with the reverse operation. Yes, exactly, |rect| is in |target| coordinate in the first function, but in |source| coordinate in the second function. The reason of two functions is because I cannot tell how DXGI APIs return the updated region, i.e. in |source| coordinate or in |target| coordinate. I have updated the comments. It's a trade-off, we need to export two functions, either in current way, or a CopyRotatedRectTo() and RotateRect(). Personally I prefer current solution, since users do not need to think about whether a RotateRect() should be applied, instead they only need to care about whether the |rect| is in |source| coordinate or |target| coordinate. https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/desktop_frame_rotator_unittest.cc (right): https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator_unittest.cc:96: PaintDesktopFrame(&expected, DesktopVector(0, 0), RgbaColor(11)); On 2016/11/15 20:50:22, Sergey Ulanov wrote: > This code would be much simpler with a simple memcpy() from a static uint32_t > array. Done. https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator_unittest.cc:132: PaintDesktopFrame(&expected, DesktopVector(0, 0), RgbaColor(3)); On 2016/11/15 20:50:22, Sergey Ulanov wrote: > Instead of initializing expected frame, maybe just memcmp() the data() with a > static array, e.g. > uint32_t expected_values[] { > 3, 7, 11, // > 2, 6, 10, // > 1, 5, 9, // > 0, 4, 8, // > } > EXPECT_EQ(0, memcpy(target->data(), expected_values, sizeof(expected_values))); > > This is a much simpler code and you wouldn't need to comment that shows the > matrix. Good point. Updated. https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator_unittest.cc:514: // rotating one 4096 x 3072 frame uses around 67ms. On 2016/11/15 20:50:23, Sergey Ulanov wrote: > Is that the number you get with libyuv? Yes. https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/test_utils.cc (right): https://codereview.webrtc.org/2500883004/diff/120001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/test_utils.cc:50: if (memcmp(my_this_line, right_this_line, DesktopFrame::kBytesPerPixel) != On 2016/11/15 20:50:23, Sergey Ulanov wrote: > Why not compare the whole line, instead of comparing each pixel separately? Done.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Patchset #3 (id:140001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/16228)
https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/desktop_frame_rotator.cc (right): https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:30: return DesktopRect::MakeLTRB(size.height() - rect.bottom(), rect.left(), MakeXYWH() here for consistency. https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:37: return DesktopRect::MakeLTRB(rect.top(), size.width() - rect.right(), MakeXYWH()? https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:69: default: Please remove default case. https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:85: default: Don't need this. https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:93: bool CopyRotatedRectTo(const DesktopFrame& source, Do we really need to return any result from this function? It may fail only if the caller passes incorrectly-sized frame or rect, but then it should be be DCHECK'ed instead of returning an error. https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:98: if (!DesktopRect::MakeSize(target->size()).ContainsRect(rect)) { Replace this with DCHECK? https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:108: RotateRect(rect, GetRotatedSize(source.size(), Reverse(rotation)), Don't need really need to reverse rotation for GetRotatedSize() (because GetRotatedSize(r) is always the same as GetRotatedSize(Reverse(rotation)) ), but if you do then cache the result to avoid calling Reverse() twice. https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/desktop_frame_rotator.h (right): https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.h:11: #ifndef WEBRTC_MODULES_DESKTOP_CAPTURE_DESKTOP_FRAME_ROTATOR_H_ suggest renaming it to "desktop_frame_rotation.h" https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.h:31: // coordinate. 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. https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.h:34: bool CopyRotatedRectTo(const DesktopFrame& source, Maybe call it RotateDesktopFrame()? https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.h:44: // This function triggers check failure if |target| is null, returns false if Don't need to mention target==null here. It's obvious that target is not nullable here. https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.h:46: bool CopyUnrotatedRectTo(const DesktopFrame& source, 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 :( https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/rgba_color.h (right): https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/rgba_color.h:45: explicit RgbaColor(int bgra); Do you still need this? https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/test_utils.h (right): https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/test_utils.h:22: void PaintDesktopFrame(DesktopFrame* frame, DesktopVector pos, RgbaColor color); Do you still need this function?
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/desktop_frame_rotator.cc (right): https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:30: return DesktopRect::MakeLTRB(size.height() - rect.bottom(), rect.left(), On 2016/11/17 02:07:39, Sergey Ulanov wrote: > MakeXYWH() here for consistency. Done. https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:37: return DesktopRect::MakeLTRB(rect.top(), size.width() - rect.right(), On 2016/11/17 02:07:39, Sergey Ulanov wrote: > MakeXYWH()? Done. https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:69: default: On 2016/11/17 02:07:38, Sergey Ulanov wrote: > Please remove default case. Done. https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:85: default: On 2016/11/17 02:07:39, Sergey Ulanov wrote: > Don't need this. Done. https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:93: bool CopyRotatedRectTo(const DesktopFrame& source, On 2016/11/17 02:07:39, Sergey Ulanov wrote: > Do we really need to return any result from this function? It may fail only if > the caller passes incorrectly-sized frame or rect, but then it should be be > DCHECK'ed instead of returning an error. Done. https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:98: if (!DesktopRect::MakeSize(target->size()).ContainsRect(rect)) { On 2016/11/17 02:07:39, Sergey Ulanov wrote: > Replace this with DCHECK? Done. https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.cc:108: RotateRect(rect, GetRotatedSize(source.size(), Reverse(rotation)), On 2016/11/17 02:07:39, Sergey Ulanov wrote: > Don't need really need to reverse rotation for GetRotatedSize() (because > GetRotatedSize(r) is always the same as GetRotatedSize(Reverse(rotation)) ), but > if you do then cache the result to avoid calling Reverse() twice. Yes, I know they are same. But I think Reverse(rotation) here make the logic look clearer, as reader does not need to consider to understand the correctness of this line. I will cache the result of Reverse(rotation). https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/desktop_frame_rotator.h (right): https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.h:11: #ifndef WEBRTC_MODULES_DESKTOP_CAPTURE_DESKTOP_FRAME_ROTATOR_H_ On 2016/11/17 02:07:39, Sergey Ulanov wrote: > suggest renaming it to "desktop_frame_rotation.h" Done. https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.h:31: // coordinate. On 2016/11/17 02:07:39, Sergey Ulanov wrote: > 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. 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(). https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.h:34: bool CopyRotatedRectTo(const DesktopFrame& source, On 2016/11/17 02:07:39, Sergey Ulanov wrote: > Maybe call it RotateDesktopFrame()? Done. https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.h:44: // This function triggers check failure if |target| is null, returns false if On 2016/11/17 02:07:39, Sergey Ulanov wrote: > Don't need to mention target==null here. It's obvious that target is not > nullable here. Done. https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.h:46: bool CopyUnrotatedRectTo(const DesktopFrame& source, On 2016/11/17 02:07:39, Sergey Ulanov wrote: > 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 :( 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. https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/rgba_color.h (right): https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/rgba_color.h:45: explicit RgbaColor(int bgra); On 2016/11/17 02:07:39, Sergey Ulanov wrote: > Do you still need this? Yes, it uses in test_utils_unittest.cc. https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/test_utils.h (right): https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/test_utils.h:22: void PaintDesktopFrame(DesktopFrame* frame, DesktopVector pos, RgbaColor color); On 2016/11/17 02:07:39, Sergey Ulanov wrote: > Do you still need this function? Yes, it uses in test_utils_unittest.cc.
Description was changed from ========== Add DesktopFrame rotator functions This change adds a CopyRotatedRectTo() and CopyUnrotatedRectTo() function, so an implementation can use these two free functions to rotate and copy pixels from one DesktopFrame to another at the same time. I cannot tell whether the CopyRotatedRectTo() or CopyUnrotatedRectTo() should be used in DirectX capturer, i.e. whether the updated_region returns from DetectUpdatedRegion() is rotated or not. So I added both, though one is only a combination of the other with several annoymous functions. This is the first part of the change to support rotation in DirectX capturer. In a coming change, one of these two functions will be used in DxgiOutputDuplicator to do the rotation and copying. Background, DirectX APIs always return unrotated data buffer, so we need to rotate it to match the user-selected rotation. What worse is except for the data buffer, other variables return by these APIs are all rotated, e.g. output size. So we will eventually not be able to capture the rotated monitors, because we cannot set their position and size correctly. Though DXGI_OUTDUPL_DESC provides a DXGI_MODE_ROTATION enumeration to indicate the output rotation, it does not provide a simple way to rotate an IDXGIResource, which is the only thing we can get from duplication APIs. A typical user case here is to use a matrix to transform the IDXGIResource and render it to a surface. But since we do not render the IDXGIResource at all, we need to manually rotate it. BUG=314516 ========== to ========== Add DesktopFrame rotation functions This change adds a RotateDesktopFrame() and RotateRect() function, so an implementation can use these two free functions to rotate and copy pixels from one DesktopFrame to another at the same time. This is the first part of the change to support rotation in DirectX capturer. In a coming change, one of these two functions will be used in DxgiOutputDuplicator to do the rotation and copying. Background, DirectX APIs always return unrotated data buffer, so we need to rotate it to match the user-selected rotation. What worse is except for the data buffer, other variables return by these APIs are all rotated, e.g. output size, monitor position. So we will eventually not be able to capture the rotated monitors, because we cannot set their position and size correctly. Though DXGI_OUTDUPL_DESC provides a DXGI_MODE_ROTATION enumeration to indicate the output rotation, it does not provide a simple way to rotate an IDXGIResource, which is the only thing we can get from duplication APIs. A typical user case here is to use a matrix to transform the IDXGIResource and render it to a surface. But since we do not render the IDXGIResource at all, we need to manually rotate it. BUG=314516 ==========
https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/desktop_frame_rotator.h (right): https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.h:31: // coordinate. On 2016/11/17 07:14:55, Hzj_jie wrote: > On 2016/11/17 02:07:39, Sergey Ulanov wrote: > > 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. > > 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(). 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. https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/test_utils.h (right): https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/test_utils.h:22: void PaintDesktopFrame(DesktopFrame* frame, DesktopVector pos, RgbaColor color); On 2016/11/17 07:14:55, Hzj_jie wrote: > On 2016/11/17 02:07:39, Sergey Ulanov wrote: > > Do you still need this function? > > Yes, it uses in test_utils_unittest.cc. Maybe put it in test_utils_unittest.cc. But I think the test could be made simpler by using static 2D array, same as you did in desktop_frame_rotation_unittest.cc https://codereview.webrtc.org/2500883004/diff/180001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/desktop_frame_rotation.cc (right): https://codereview.webrtc.org/2500883004/diff/180001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotation.cc:92: const DesktopRect& rect, I think it's more intuitive if |rect| was specified in |source| coordinates. That would make it more intuitive and more consistent with the ARGBRotate() interface. https://codereview.webrtc.org/2500883004/diff/180001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotation.cc:95: RTC_DCHECK(DesktopRect::MakeSize(target->size()).ContainsRect(rect)); Also need to DCHECK that source and target are the same size https://codereview.webrtc.org/2500883004/diff/180001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/desktop_frame_rotation_unittest.cc (right): https://codereview.webrtc.org/2500883004/diff/180001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotation_unittest.cc:15: #include "webrtc/modules/desktop_capture/rgba_color.h" This doesn't seem to be used anywhere https://codereview.webrtc.org/2500883004/diff/180001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/rgba_color.h (right): https://codereview.webrtc.org/2500883004/diff/180001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/rgba_color.h:45: explicit RgbaColor(int bgra); Do you still need this? I think RgbaColor(0U) can be used to avoid the error. Maybe used it instead?
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/desktop_frame_rotator.h (right): https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotator.h:31: // coordinate. On 2016/11/19 02:11:40, Sergey Ulanov wrote: > On 2016/11/17 07:14:55, Hzj_jie wrote: > > On 2016/11/17 02:07:39, Sergey Ulanov wrote: > > > 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. > > > > 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(). > > 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. 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. https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/test_utils.h (right): https://codereview.webrtc.org/2500883004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/test_utils.h:22: void PaintDesktopFrame(DesktopFrame* frame, DesktopVector pos, RgbaColor color); On 2016/11/19 02:11:40, Sergey Ulanov wrote: > On 2016/11/17 07:14:55, Hzj_jie wrote: > > On 2016/11/17 02:07:39, Sergey Ulanov wrote: > > > Do you still need this function? > > > > Yes, it uses in test_utils_unittest.cc. > > Maybe put it in test_utils_unittest.cc. But I think the test could be made > simpler by using static 2D array, same as you did in > desktop_frame_rotation_unittest.cc Moved to test_utils_unittest.cc. Using 2D array cannot fulfill the requirement of the DoubleStrideShouldBeComparable test. https://codereview.webrtc.org/2500883004/diff/180001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/desktop_frame_rotation.cc (right): https://codereview.webrtc.org/2500883004/diff/180001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotation.cc:92: const DesktopRect& rect, On 2016/11/19 02:11:40, Sergey Ulanov wrote: > I think it's more intuitive if |rect| was specified in |source| coordinates. > That would make it more intuitive and more consistent with the ARGBRotate() > interface. Done. https://codereview.webrtc.org/2500883004/diff/180001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotation.cc:95: RTC_DCHECK(DesktopRect::MakeSize(target->size()).ContainsRect(rect)); On 2016/11/19 02:11:40, Sergey Ulanov wrote: > Also need to DCHECK that source and target are the same size In a typical multi-monitor scenario of DirectX capturer, the source is one monitor, but target is the entire screen. So they usually have different size. https://codereview.webrtc.org/2500883004/diff/180001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/desktop_frame_rotation_unittest.cc (right): https://codereview.webrtc.org/2500883004/diff/180001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotation_unittest.cc:15: #include "webrtc/modules/desktop_capture/rgba_color.h" On 2016/11/19 02:11:40, Sergey Ulanov wrote: > This doesn't seem to be used anywhere Done. https://codereview.webrtc.org/2500883004/diff/180001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/rgba_color.h (right): https://codereview.webrtc.org/2500883004/diff/180001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/rgba_color.h:45: explicit RgbaColor(int bgra); On 2016/11/19 02:11:40, Sergey Ulanov wrote: > Do you still need this? I think RgbaColor(0U) can be used to avoid the error. > Maybe used it instead? Done.
lgtm https://codereview.webrtc.org/2500883004/diff/180001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/desktop_frame_rotation.cc (right): https://codereview.webrtc.org/2500883004/diff/180001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotation.cc:95: RTC_DCHECK(DesktopRect::MakeSize(target->size()).ContainsRect(rect)); On 2016/11/19 05:10:59, Hzj_jie wrote: > On 2016/11/19 02:11:40, Sergey Ulanov wrote: > > Also need to DCHECK that source and target are the same size > > In a typical multi-monitor scenario of DirectX capturer, the source is one > monitor, but target is the entire screen. So they usually have different size. I see. But in this case shouldn't we also shift the rectangle in case the screen is shifted relative to the desktop origin? Also it may be useful to have a DesktopFrame wrapper that references a subset of a bigger frame. That would allow to have source and target with the same size. https://codereview.webrtc.org/2500883004/diff/200001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/desktop_frame_rotation.cc (right): https://codereview.webrtc.org/2500883004/diff/200001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotation.cc:95: RTC_DCHECK(DesktopRect::MakeSize(target->size()).ContainsRect(target_rect)); Also DCHECK that |source_rect| is inside |source|?
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #6 (id:220001) has been deleted
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2500883004/diff/180001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/desktop_frame_rotation.cc (right): https://codereview.webrtc.org/2500883004/diff/180001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotation.cc:95: RTC_DCHECK(DesktopRect::MakeSize(target->size()).ContainsRect(rect)); On 2016/11/21 21:01:12, Sergey Ulanov wrote: > On 2016/11/19 05:10:59, Hzj_jie wrote: > > On 2016/11/19 02:11:40, Sergey Ulanov wrote: > > > Also need to DCHECK that source and target are the same size > > > > In a typical multi-monitor scenario of DirectX capturer, the source is one > > monitor, but target is the entire screen. So they usually have different size. > > I see. But in this case shouldn't we also shift the rectangle in case the screen > is shifted relative to the desktop origin? > Also it may be useful to have a DesktopFrame wrapper that references a subset of > a bigger frame. That would allow to have source and target with the same size. Yes, I missed this point. But I do not think a DesktopFrame wrapper can help to resolve the issue, all of DesktopFrame functions are not overridable. We can use a template here to work around overridable issue, but we still need to provide a different DesktopRegion to forward AddRect to a correct position. (In line 101.) IMO, a simpler solution is to add a DesktopVector parameter in RotateDesktopFrame function to indicate the offset in the target, then we just need to add the offset to |target_rect|. https://codereview.webrtc.org/2500883004/diff/200001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/desktop_frame_rotation.cc (right): https://codereview.webrtc.org/2500883004/diff/200001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/desktop_frame_rotation.cc:95: RTC_DCHECK(DesktopRect::MakeSize(target->size()).ContainsRect(target_rect)); On 2016/11/21 21:01:12, Sergey Ulanov wrote: > Also DCHECK that |source_rect| is inside |source|? Done.
still lgtm. Couple more comments about test_utils.cc https://codereview.webrtc.org/2500883004/diff/240001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/test_utils.cc (right): https://codereview.webrtc.org/2500883004/diff/240001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/test_utils.cc:21: memset(frame->data(), 0, frame->stride() * frame->size().height()); stride() may be negative (see https://codesearch.chromium.org/chromium/src/third_party/webrtc/modules/deskt... ). This code won't work correctly in that case. https://codereview.webrtc.org/2500883004/diff/240001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/test_utils.cc:30: left.stride() == DesktopFrame::kBytesPerPixel * left.size().width()) { I don't think you need this optimization, especially given that this is a test-only code. https://codereview.webrtc.org/2500883004/diff/240001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/test_utils.cc:36: const uint8_t* my_array = left.data(); s/my/left/?
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2500883004/diff/240001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/test_utils.cc (right): https://codereview.webrtc.org/2500883004/diff/240001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/test_utils.cc:21: memset(frame->data(), 0, frame->stride() * frame->size().height()); On 2016/11/22 20:33:07, Sergey Ulanov wrote: > stride() may be negative (see > https://codesearch.chromium.org/chromium/src/third_party/webrtc/modules/deskt... > ). This code won't work correctly in that case. Yes, I noticed this class before. Since ClearDesktopFrame does not take care of this specific implementation, I did not consider it. The InvertDesktopFrame looks weird, is it because of some specific API limitations in MAC? I have updated the logic. https://codereview.webrtc.org/2500883004/diff/240001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/test_utils.cc:30: left.stride() == DesktopFrame::kBytesPerPixel * left.size().width()) { On 2016/11/22 20:33:07, Sergey Ulanov wrote: > I don't think you need this optimization, especially given that this is a > test-only code. Done. https://codereview.webrtc.org/2500883004/diff/240001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/test_utils.cc:36: const uint8_t* my_array = left.data(); On 2016/11/22 20:33:07, Sergey Ulanov wrote: > s/my/left/? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_dbg/builds/8546)
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.webrtc.org/2500883004/#ps260001 (title: "Resolve review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== Add DesktopFrame rotation functions This change adds a RotateDesktopFrame() and RotateRect() function, so an implementation can use these two free functions to rotate and copy pixels from one DesktopFrame to another at the same time. This is the first part of the change to support rotation in DirectX capturer. In a coming change, one of these two functions will be used in DxgiOutputDuplicator to do the rotation and copying. Background, DirectX APIs always return unrotated data buffer, so we need to rotate it to match the user-selected rotation. What worse is except for the data buffer, other variables return by these APIs are all rotated, e.g. output size, monitor position. So we will eventually not be able to capture the rotated monitors, because we cannot set their position and size correctly. Though DXGI_OUTDUPL_DESC provides a DXGI_MODE_ROTATION enumeration to indicate the output rotation, it does not provide a simple way to rotate an IDXGIResource, which is the only thing we can get from duplication APIs. A typical user case here is to use a matrix to transform the IDXGIResource and render it to a surface. But since we do not render the IDXGIResource at all, we need to manually rotate it. BUG=314516 ========== to ========== Add DesktopFrame rotation functions This change adds RotateDesktopFrame(), RotateRect(), RotateSize(), ReverseRotate() functions, so an implementation can use these free functions to rotate and copy pixels from one DesktopFrame to another at the same time. This is the first part of the change to support rotation in DirectX capturer. In a coming change, these functions will be used in DxgiOutputDuplicator to do the rotation and copying. Background, DirectX APIs always return unrotated data buffer, so we need to rotate it to match the user-selected rotation. What worse is except for the data buffer, other variables return by these APIs are all rotated, e.g. output size, monitor position. So we will eventually not be able to capture the rotated monitors, because we cannot set their position and size correctly. Though DXGI_OUTDUPL_DESC provides a DXGI_MODE_ROTATION enumeration to indicate the output rotation, it does not provide a simple way to rotate an IDXGIResource, which is the only thing we can get from duplication APIs. A typical user case here is to use a matrix to transform the IDXGIResource and render it to a surface. But since we do not render the IDXGIResource at all, we need to manually rotate it. BUG=314516 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_more_configs on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_more_configs/bu...)
The CQ bit was checked by zijiehe@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_more_configs on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_more_configs/bu...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_more_configs on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_more_configs/bu...)
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.webrtc.org/2500883004/#ps280001 (title: "Sync latest changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 280001, "attempt_start_ts": 1479863563995340,
"parent_rev": "46a6cf7f8de66d122a8a81663b27f815b80162e2", "commit_rev":
"f2b50e711fa037bc068ecb640125a48a52c38c7c"}
Message was sent while issue was closed.
Description was changed from ========== Add DesktopFrame rotation functions This change adds RotateDesktopFrame(), RotateRect(), RotateSize(), ReverseRotate() functions, so an implementation can use these free functions to rotate and copy pixels from one DesktopFrame to another at the same time. This is the first part of the change to support rotation in DirectX capturer. In a coming change, these functions will be used in DxgiOutputDuplicator to do the rotation and copying. Background, DirectX APIs always return unrotated data buffer, so we need to rotate it to match the user-selected rotation. What worse is except for the data buffer, other variables return by these APIs are all rotated, e.g. output size, monitor position. So we will eventually not be able to capture the rotated monitors, because we cannot set their position and size correctly. Though DXGI_OUTDUPL_DESC provides a DXGI_MODE_ROTATION enumeration to indicate the output rotation, it does not provide a simple way to rotate an IDXGIResource, which is the only thing we can get from duplication APIs. A typical user case here is to use a matrix to transform the IDXGIResource and render it to a surface. But since we do not render the IDXGIResource at all, we need to manually rotate it. BUG=314516 ========== to ========== Add DesktopFrame rotation functions This change adds RotateDesktopFrame(), RotateRect(), RotateSize(), ReverseRotate() functions, so an implementation can use these free functions to rotate and copy pixels from one DesktopFrame to another at the same time. This is the first part of the change to support rotation in DirectX capturer. In a coming change, these functions will be used in DxgiOutputDuplicator to do the rotation and copying. Background, DirectX APIs always return unrotated data buffer, so we need to rotate it to match the user-selected rotation. What worse is except for the data buffer, other variables return by these APIs are all rotated, e.g. output size, monitor position. So we will eventually not be able to capture the rotated monitors, because we cannot set their position and size correctly. Though DXGI_OUTDUPL_DESC provides a DXGI_MODE_ROTATION enumeration to indicate the output rotation, it does not provide a simple way to rotate an IDXGIResource, which is the only thing we can get from duplication APIs. A typical user case here is to use a matrix to transform the IDXGIResource and render it to a surface. But since we do not render the IDXGIResource at all, we need to manually rotate it. BUG=314516 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Add DesktopFrame rotation functions This change adds RotateDesktopFrame(), RotateRect(), RotateSize(), ReverseRotate() functions, so an implementation can use these free functions to rotate and copy pixels from one DesktopFrame to another at the same time. This is the first part of the change to support rotation in DirectX capturer. In a coming change, these functions will be used in DxgiOutputDuplicator to do the rotation and copying. Background, DirectX APIs always return unrotated data buffer, so we need to rotate it to match the user-selected rotation. What worse is except for the data buffer, other variables return by these APIs are all rotated, e.g. output size, monitor position. So we will eventually not be able to capture the rotated monitors, because we cannot set their position and size correctly. Though DXGI_OUTDUPL_DESC provides a DXGI_MODE_ROTATION enumeration to indicate the output rotation, it does not provide a simple way to rotate an IDXGIResource, which is the only thing we can get from duplication APIs. A typical user case here is to use a matrix to transform the IDXGIResource and render it to a surface. But since we do not render the IDXGIResource at all, we need to manually rotate it. BUG=314516 ========== to ========== Add DesktopFrame rotation functions This change adds RotateDesktopFrame(), RotateRect(), RotateSize(), ReverseRotate() functions, so an implementation can use these free functions to rotate and copy pixels from one DesktopFrame to another at the same time. This is the first part of the change to support rotation in DirectX capturer. In a coming change, these functions will be used in DxgiOutputDuplicator to do the rotation and copying. Background, DirectX APIs always return unrotated data buffer, so we need to rotate it to match the user-selected rotation. What worse is except for the data buffer, other variables return by these APIs are all rotated, e.g. output size, monitor position. So we will eventually not be able to capture the rotated monitors, because we cannot set their position and size correctly. Though DXGI_OUTDUPL_DESC provides a DXGI_MODE_ROTATION enumeration to indicate the output rotation, it does not provide a simple way to rotate an IDXGIResource, which is the only thing we can get from duplication APIs. A typical user case here is to use a matrix to transform the IDXGIResource and render it to a surface. But since we do not render the IDXGIResource at all, we need to manually rotate it. BUG=314516 Committed: https://crrev.com/90ea7362fc81db6a677d129a9161ee3d49b77ec1 Cr-Commit-Position: refs/heads/master@{#15205} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/90ea7362fc81db6a677d129a9161ee3d49b77ec1 Cr-Commit-Position: refs/heads/master@{#15205} |
