Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2202443002:
[WebRTC] Add ScreenCapturerDifferWrapper to share Differ across ScreenCapturers (Closed)
DescriptionAn early analysis shows in DirectX based capturer, Windows API returns larger
dirty region than the real screen change. A similar behavior may happen on other
platforms with damage notification support. So it's better to have an individual
layer to handle the Differ logic, and remove capturing independent logic out of
each ScreenCapturer* implementation.
So this change does following things,
1. Update differ_block to handle variable height. differ_block_sse2 has been
renamed to differ_vector_sse2.
2. A new ScreenCapturerDifferWrapper implementation to help set
DesktopFrame::updated_region(). It uses an underlying ScreenCapturer to do
the real capture work, and updates the updated region of DesktopFrame returned
from OnCaptureResult function.
3. FakeDesktopCapturer and FakeScreenCapturer to generate controllable
DesktopFrame by using DesktopFrameGenerator and DesktopFramePainter.
4. Test ScreenCapturerDifferWrapper by using FakeScreenCapturer.
After this change, we can eventually remove all Differ logic from
ScreenCapturer* implementations, and fix a potential crash bug in
ScreenCapturerLinux class. It wrongly assumes previous_frame() has a same size
as current_frame(). https://goo.gl/3nSqOC
BUG=633802
TBR=kjellander@webrtc.org
Committed: https://crrev.com/fef8653c5adcd7c5b7fc2c49ae1a0f6927ddb37f
Cr-Commit-Position: refs/heads/master@{#14076}
Patch Set 1 #
Total comments: 48
Patch Set 2 : Resolve review comments #Patch Set 3 : Move Differ logic into ScreenCapturerDifferWrapper #Patch Set 4 : Disable perf test without SSE2 #
Total comments: 28
Patch Set 5 : Resolve review comments #Patch Set 6 : Resolve review comments #
Total comments: 22
Patch Set 7 : Resolve review comments #Patch Set 8 : Sync latest changes #
Total comments: 20
Patch Set 9 : Resolve review comments #
Total comments: 4
Patch Set 10 : Resolve review comments #Patch Set 11 : Sync latest changes #Patch Set 12 : Sync latest changes #Messages
Total messages: 64 (33 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Add differ::CalcDirtyrRegionWithHints function. BUG= ========== to ========== An early analysis shows in DirectX based capturer, Windows API returns larger dirty region than the real screen change. A similar behavior may happen on other platforms with damage notification support. So it's better to have an individual layer to handle the Differ logic, and remove capturing independent logic out of each ScreenCapturer* implementation. So this change does following things, 1. Update Differ class to support CalcDirtyRegionWithHints. This function receives a const DesktopRegion& as hint, and detects only changes in the hint areas. So on platforms with damage notifications can use the DesktopRegion from platform APIs as the hints to help improve Differ speed by reducing blocks it checks. 2. A new ScreenCapturerDifferWrapper implementation to use Differ class to help set DesktopFrame::updated_region(). It uses an underlying ScreenCapturer to do the real capture work, and updates the updated region of DesktopFrame returned from OnCaptureResult function. 3. DifferTest to cover the new CalcDirtyRegionWithHints function. 4. Improves MockScreenCapturer to make it more useful on testing ScreenCapturerDifferWrapper. Now MockScreenCapturer supports to return controllable DesktopFrames. Users can set various flags to control its behavior. 5. ScreenCapturerDifferWrapperTest. After this change, we can eventually remove all Differ logic from ScreenCapturer* implementations, and fix a potential crash bug in ScreenCapturerLinux class. It wrongly assumes previous_frame() has a same size as current_frame(). https://goo.gl/3nSqOC A coming change is to add another ScreenCapturerExpendToGridWrapper to cover the functionality of ScreenCapturerHelper. BUG=633802 ==========
zijiehe@chromium.org changed reviewers: + sergeyu@chromium.org, wez@chromium.org
Description was changed from ========== An early analysis shows in DirectX based capturer, Windows API returns larger dirty region than the real screen change. A similar behavior may happen on other platforms with damage notification support. So it's better to have an individual layer to handle the Differ logic, and remove capturing independent logic out of each ScreenCapturer* implementation. So this change does following things, 1. Update Differ class to support CalcDirtyRegionWithHints. This function receives a const DesktopRegion& as hint, and detects only changes in the hint areas. So on platforms with damage notifications can use the DesktopRegion from platform APIs as the hints to help improve Differ speed by reducing blocks it checks. 2. A new ScreenCapturerDifferWrapper implementation to use Differ class to help set DesktopFrame::updated_region(). It uses an underlying ScreenCapturer to do the real capture work, and updates the updated region of DesktopFrame returned from OnCaptureResult function. 3. DifferTest to cover the new CalcDirtyRegionWithHints function. 4. Improves MockScreenCapturer to make it more useful on testing ScreenCapturerDifferWrapper. Now MockScreenCapturer supports to return controllable DesktopFrames. Users can set various flags to control its behavior. 5. ScreenCapturerDifferWrapperTest. After this change, we can eventually remove all Differ logic from ScreenCapturer* implementations, and fix a potential crash bug in ScreenCapturerLinux class. It wrongly assumes previous_frame() has a same size as current_frame(). https://goo.gl/3nSqOC A coming change is to add another ScreenCapturerExpendToGridWrapper to cover the functionality of ScreenCapturerHelper. BUG=633802 ========== to ========== An early analysis shows in DirectX based capturer, Windows API returns larger dirty region than the real screen change. A similar behavior may happen on other platforms with damage notification support. So it's better to have an individual layer to handle the Differ logic, and remove capturing independent logic out of each ScreenCapturer* implementation. So this change does following things, 1. Update Differ class to support CalcDirtyRegionWithHints. This function receives a const DesktopRegion& as hint, and detects only changes in the hint areas. So on platforms with damage notifications a ScreenCapturer implementation can use the DesktopRegion from platform APIs as the hints to help improve Differ speed by reducing blocks it checks. 2. A new ScreenCapturerDifferWrapper implementation to use Differ class to help set DesktopFrame::updated_region(). It uses an underlying ScreenCapturer to do the real capture work, and updates the updated region of DesktopFrame returned from OnCaptureResult function. 3. DifferTest to cover the new CalcDirtyRegionWithHints function. 4. Improves MockScreenCapturer to make it more useful on testing ScreenCapturerDifferWrapper. Now MockScreenCapturer supports to return controllable DesktopFrames. Users can set various flags to control its behavior. 5. ScreenCapturerDifferWrapperTest. After this change, we can eventually remove all Differ logic from ScreenCapturer* implementations, and fix a potential crash bug in ScreenCapturerLinux class. It wrongly assumes previous_frame() has a same size as current_frame(). https://goo.gl/3nSqOC A coming change is to add another ScreenCapturerExpendToGridWrapper to cover the functionality of ScreenCapturerHelper. BUG=633802 ==========
Description was changed from ========== An early analysis shows in DirectX based capturer, Windows API returns larger dirty region than the real screen change. A similar behavior may happen on other platforms with damage notification support. So it's better to have an individual layer to handle the Differ logic, and remove capturing independent logic out of each ScreenCapturer* implementation. So this change does following things, 1. Update Differ class to support CalcDirtyRegionWithHints. This function receives a const DesktopRegion& as hint, and detects only changes in the hint areas. So on platforms with damage notifications a ScreenCapturer implementation can use the DesktopRegion from platform APIs as the hints to help improve Differ speed by reducing blocks it checks. 2. A new ScreenCapturerDifferWrapper implementation to use Differ class to help set DesktopFrame::updated_region(). It uses an underlying ScreenCapturer to do the real capture work, and updates the updated region of DesktopFrame returned from OnCaptureResult function. 3. DifferTest to cover the new CalcDirtyRegionWithHints function. 4. Improves MockScreenCapturer to make it more useful on testing ScreenCapturerDifferWrapper. Now MockScreenCapturer supports to return controllable DesktopFrames. Users can set various flags to control its behavior. 5. ScreenCapturerDifferWrapperTest. After this change, we can eventually remove all Differ logic from ScreenCapturer* implementations, and fix a potential crash bug in ScreenCapturerLinux class. It wrongly assumes previous_frame() has a same size as current_frame(). https://goo.gl/3nSqOC A coming change is to add another ScreenCapturerExpendToGridWrapper to cover the functionality of ScreenCapturerHelper. BUG=633802 ========== to ========== This is not a huge change, 624 out of 886 lines are unit test related code change. So this change contains only 262 lines of production code. An early analysis shows in DirectX based capturer, Windows API returns larger dirty region than the real screen change. A similar behavior may happen on other platforms with damage notification support. So it's better to have an individual layer to handle the Differ logic, and remove capturing independent logic out of each ScreenCapturer* implementation. So this change does following things, 1. Update Differ class to support CalcDirtyRegionWithHints. This function receives a const DesktopRegion& as hint, and detects only changes in the hint areas. So on platforms with damage notifications a ScreenCapturer implementation can use the DesktopRegion from platform APIs as the hints to help improve Differ speed by reducing blocks it checks. 2. A new ScreenCapturerDifferWrapper implementation to use Differ class to help set DesktopFrame::updated_region(). It uses an underlying ScreenCapturer to do the real capture work, and updates the updated region of DesktopFrame returned from OnCaptureResult function. 3. DifferTest to cover the new CalcDirtyRegionWithHints function. 4. Improves MockScreenCapturer to make it more useful on testing ScreenCapturerDifferWrapper. Now MockScreenCapturer supports to return controllable DesktopFrames. Users can set various flags to control its behavior. 5. ScreenCapturerDifferWrapperTest. After this change, we can eventually remove all Differ logic from ScreenCapturer* implementations, and fix a potential crash bug in ScreenCapturerLinux class. It wrongly assumes previous_frame() has a same size as current_frame(). https://goo.gl/3nSqOC A coming change is to add another ScreenCapturerExpendToGridWrapper to cover the functionality of ScreenCapturerHelper. BUG=633802 ==========
zijiehe@chromium.org changed reviewers: - wez@chromium.org
Patchset #1 (id:60001) has been deleted
Description was changed from ========== This is not a huge change, 624 out of 886 lines are unit test related code change. So this change contains only 262 lines of production code. An early analysis shows in DirectX based capturer, Windows API returns larger dirty region than the real screen change. A similar behavior may happen on other platforms with damage notification support. So it's better to have an individual layer to handle the Differ logic, and remove capturing independent logic out of each ScreenCapturer* implementation. So this change does following things, 1. Update Differ class to support CalcDirtyRegionWithHints. This function receives a const DesktopRegion& as hint, and detects only changes in the hint areas. So on platforms with damage notifications a ScreenCapturer implementation can use the DesktopRegion from platform APIs as the hints to help improve Differ speed by reducing blocks it checks. 2. A new ScreenCapturerDifferWrapper implementation to use Differ class to help set DesktopFrame::updated_region(). It uses an underlying ScreenCapturer to do the real capture work, and updates the updated region of DesktopFrame returned from OnCaptureResult function. 3. DifferTest to cover the new CalcDirtyRegionWithHints function. 4. Improves MockScreenCapturer to make it more useful on testing ScreenCapturerDifferWrapper. Now MockScreenCapturer supports to return controllable DesktopFrames. Users can set various flags to control its behavior. 5. ScreenCapturerDifferWrapperTest. After this change, we can eventually remove all Differ logic from ScreenCapturer* implementations, and fix a potential crash bug in ScreenCapturerLinux class. It wrongly assumes previous_frame() has a same size as current_frame(). https://goo.gl/3nSqOC A coming change is to add another ScreenCapturerExpendToGridWrapper to cover the functionality of ScreenCapturerHelper. BUG=633802 ========== to ========== This is not a huge change, 559 out of 821 lines are unit test related code change. So this change contains only 262 lines of production code. An early analysis shows in DirectX based capturer, Windows API returns larger dirty region than the real screen change. A similar behavior may happen on other platforms with damage notification support. So it's better to have an individual layer to handle the Differ logic, and remove capturing independent logic out of each ScreenCapturer* implementation. So this change does following things, 1. Update Differ class to support CalcDirtyRegionWithHints. This function receives a const DesktopRegion& as hint, and detects only changes in the hint areas. So on platforms with damage notifications a ScreenCapturer implementation can use the DesktopRegion from platform APIs as the hints to help improve Differ speed by reducing blocks it checks. 2. A new ScreenCapturerDifferWrapper implementation to use Differ class to help set DesktopFrame::updated_region(). It uses an underlying ScreenCapturer to do the real capture work, and updates the updated region of DesktopFrame returned from OnCaptureResult function. 3. DifferTest to cover the new CalcDirtyRegionWithHints function. 4. Improves MockScreenCapturer to make it more useful on testing ScreenCapturerDifferWrapper. Now MockScreenCapturer supports to return controllable DesktopFrames. Users can set various flags to control its behavior. 5. ScreenCapturerDifferWrapperTest. After this change, we can eventually remove all Differ logic from ScreenCapturer* implementations, and fix a potential crash bug in ScreenCapturerLinux class. It wrongly assumes previous_frame() has a same size as current_frame(). https://goo.gl/3nSqOC A coming change is to add another ScreenCapturerExpendToGridWrapper to cover the functionality of ScreenCapturerHelper. BUG=633802 ==========
Description was changed from ========== This is not a huge change, 559 out of 821 lines are unit test related code change. So this change contains only 262 lines of production code. An early analysis shows in DirectX based capturer, Windows API returns larger dirty region than the real screen change. A similar behavior may happen on other platforms with damage notification support. So it's better to have an individual layer to handle the Differ logic, and remove capturing independent logic out of each ScreenCapturer* implementation. So this change does following things, 1. Update Differ class to support CalcDirtyRegionWithHints. This function receives a const DesktopRegion& as hint, and detects only changes in the hint areas. So on platforms with damage notifications a ScreenCapturer implementation can use the DesktopRegion from platform APIs as the hints to help improve Differ speed by reducing blocks it checks. 2. A new ScreenCapturerDifferWrapper implementation to use Differ class to help set DesktopFrame::updated_region(). It uses an underlying ScreenCapturer to do the real capture work, and updates the updated region of DesktopFrame returned from OnCaptureResult function. 3. DifferTest to cover the new CalcDirtyRegionWithHints function. 4. Improves MockScreenCapturer to make it more useful on testing ScreenCapturerDifferWrapper. Now MockScreenCapturer supports to return controllable DesktopFrames. Users can set various flags to control its behavior. 5. ScreenCapturerDifferWrapperTest. After this change, we can eventually remove all Differ logic from ScreenCapturer* implementations, and fix a potential crash bug in ScreenCapturerLinux class. It wrongly assumes previous_frame() has a same size as current_frame(). https://goo.gl/3nSqOC A coming change is to add another ScreenCapturerExpendToGridWrapper to cover the functionality of ScreenCapturerHelper. BUG=633802 ========== to ========== An early analysis shows in DirectX based capturer, Windows API returns larger dirty region than the real screen change. A similar behavior may happen on other platforms with damage notification support. So it's better to have an individual layer to handle the Differ logic, and remove capturing independent logic out of each ScreenCapturer* implementation. This is not a huge change, 559 out of 821 lines are unit test related code change. So this change contains only 262 lines of production code. So this change does following things, 1. Update Differ class to support CalcDirtyRegionWithHints. This function receives a const DesktopRegion& as hint, and detects only changes in the hint areas. So on platforms with damage notifications a ScreenCapturer implementation can use the DesktopRegion from platform APIs as the hints to help improve Differ speed by reducing blocks it checks. 2. A new ScreenCapturerDifferWrapper implementation to use Differ class to help set DesktopFrame::updated_region(). It uses an underlying ScreenCapturer to do the real capture work, and updates the updated region of DesktopFrame returned from OnCaptureResult function. 3. DifferTest to cover the new CalcDirtyRegionWithHints function. 4. Improves MockScreenCapturer to make it more useful on testing ScreenCapturerDifferWrapper. Now MockScreenCapturer supports to return controllable DesktopFrames. Users can set various flags to control its behavior. 5. ScreenCapturerDifferWrapperTest. After this change, we can eventually remove all Differ logic from ScreenCapturer* implementations, and fix a potential crash bug in ScreenCapturerLinux class. It wrongly assumes previous_frame() has a same size as current_frame(). https://goo.gl/3nSqOC A coming change is to add another ScreenCapturerExpendToGridWrapper to cover the functionality of ScreenCapturerHelper. BUG=633802 ==========
Description was changed from ========== An early analysis shows in DirectX based capturer, Windows API returns larger dirty region than the real screen change. A similar behavior may happen on other platforms with damage notification support. So it's better to have an individual layer to handle the Differ logic, and remove capturing independent logic out of each ScreenCapturer* implementation. This is not a huge change, 559 out of 821 lines are unit test related code change. So this change contains only 262 lines of production code. So this change does following things, 1. Update Differ class to support CalcDirtyRegionWithHints. This function receives a const DesktopRegion& as hint, and detects only changes in the hint areas. So on platforms with damage notifications a ScreenCapturer implementation can use the DesktopRegion from platform APIs as the hints to help improve Differ speed by reducing blocks it checks. 2. A new ScreenCapturerDifferWrapper implementation to use Differ class to help set DesktopFrame::updated_region(). It uses an underlying ScreenCapturer to do the real capture work, and updates the updated region of DesktopFrame returned from OnCaptureResult function. 3. DifferTest to cover the new CalcDirtyRegionWithHints function. 4. Improves MockScreenCapturer to make it more useful on testing ScreenCapturerDifferWrapper. Now MockScreenCapturer supports to return controllable DesktopFrames. Users can set various flags to control its behavior. 5. ScreenCapturerDifferWrapperTest. After this change, we can eventually remove all Differ logic from ScreenCapturer* implementations, and fix a potential crash bug in ScreenCapturerLinux class. It wrongly assumes previous_frame() has a same size as current_frame(). https://goo.gl/3nSqOC A coming change is to add another ScreenCapturerExpendToGridWrapper to cover the functionality of ScreenCapturerHelper. BUG=633802 ========== to ========== An early analysis shows in DirectX based capturer, Windows API returns larger dirty region than the real screen change. A similar behavior may happen on other platforms with damage notification support. So it's better to have an individual layer to handle the Differ logic, and remove capturing independent logic out of each ScreenCapturer* implementation. This is not a huge change, 559 out of 821 lines are unit test related code change. So this change contains only 262 lines of production code. So this change does following things, 1. Update Differ class to support CalcDirtyRegionWithHints. This function receives a const DesktopRegion& as hint, and detects only changes in the hint areas. So on platforms with damage notifications, a ScreenCapturer implementation can use the DesktopRegion from platform APIs as the hints to help improve Differ speed by reducing blocks it checks. 2. A new ScreenCapturerDifferWrapper implementation to use Differ class to help set DesktopFrame::updated_region(). It uses an underlying ScreenCapturer to do the real capture work, and updates the updated region of DesktopFrame returned from OnCaptureResult function. 3. DifferTest to cover the new CalcDirtyRegionWithHints function. 4. Improves MockScreenCapturer to make it more useful on testing ScreenCapturerDifferWrapper. Now MockScreenCapturer supports to return controllable DesktopFrames. Users can set various flags to control its behavior. 5. ScreenCapturerDifferWrapperTest. After this change, we can eventually remove all Differ logic from ScreenCapturer* implementations, and fix a potential crash bug in ScreenCapturerLinux class. It wrongly assumes previous_frame() has a same size as current_frame(). https://goo.gl/3nSqOC A coming change is to add another ScreenCapturerExpendToGridWrapper to cover the functionality of ScreenCapturerHelper. BUG=633802 ==========
Description was changed from ========== An early analysis shows in DirectX based capturer, Windows API returns larger dirty region than the real screen change. A similar behavior may happen on other platforms with damage notification support. So it's better to have an individual layer to handle the Differ logic, and remove capturing independent logic out of each ScreenCapturer* implementation. This is not a huge change, 559 out of 821 lines are unit test related code change. So this change contains only 262 lines of production code. So this change does following things, 1. Update Differ class to support CalcDirtyRegionWithHints. This function receives a const DesktopRegion& as hint, and detects only changes in the hint areas. So on platforms with damage notifications, a ScreenCapturer implementation can use the DesktopRegion from platform APIs as the hints to help improve Differ speed by reducing blocks it checks. 2. A new ScreenCapturerDifferWrapper implementation to use Differ class to help set DesktopFrame::updated_region(). It uses an underlying ScreenCapturer to do the real capture work, and updates the updated region of DesktopFrame returned from OnCaptureResult function. 3. DifferTest to cover the new CalcDirtyRegionWithHints function. 4. Improves MockScreenCapturer to make it more useful on testing ScreenCapturerDifferWrapper. Now MockScreenCapturer supports to return controllable DesktopFrames. Users can set various flags to control its behavior. 5. ScreenCapturerDifferWrapperTest. After this change, we can eventually remove all Differ logic from ScreenCapturer* implementations, and fix a potential crash bug in ScreenCapturerLinux class. It wrongly assumes previous_frame() has a same size as current_frame(). https://goo.gl/3nSqOC A coming change is to add another ScreenCapturerExpendToGridWrapper to cover the functionality of ScreenCapturerHelper. BUG=633802 ========== to ========== An early analysis shows in DirectX based capturer, Windows API returns larger dirty region than the real screen change. A similar behavior may happen on other platforms with damage notification support. So it's better to have an individual layer to handle the Differ logic, and remove capturing independent logic out of each ScreenCapturer* implementation. This is not a huge change, 559 out of 821 lines are unit test related code change. So this change contains only 262 lines of production code. So this change does following things, 1. Update Differ class to support CalcDirtyRegionWithHints. This function receives a const DesktopRegion& as hint, and detects only changes in the hint areas. So on platforms with damage notifications, a ScreenCapturer implementation can use the DesktopRegion from platform APIs as the hints to help improve Differ speed by reducing blocks it checks. 2. A new ScreenCapturerDifferWrapper implementation to use Differ class to help set DesktopFrame::updated_region(). It uses an underlying ScreenCapturer to do the real capture work, and updates the updated region of DesktopFrame returned from OnCaptureResult function. 3. DifferTest to cover the new CalcDirtyRegionWithHints function. 4. Improves MockScreenCapturer to make it more useful on testing ScreenCapturerDifferWrapper. Now MockScreenCapturer supports to return controllable DesktopFrames. Users can set various flags to control its behavior. 5. ScreenCapturerDifferWrapperTest. After this change, we can eventually remove all Differ logic from ScreenCapturer* implementations, and fix a potential crash bug in ScreenCapturerLinux class. It wrongly assumes previous_frame() has a same size as current_frame(). https://goo.gl/3nSqOC A coming change is to add another ScreenCapturerExpendToGridWrapper to cover the functionality of ScreenCapturerHelper. BUG=633802 ==========
Description was changed from ========== An early analysis shows in DirectX based capturer, Windows API returns larger dirty region than the real screen change. A similar behavior may happen on other platforms with damage notification support. So it's better to have an individual layer to handle the Differ logic, and remove capturing independent logic out of each ScreenCapturer* implementation. This is not a huge change, 559 out of 821 lines are unit test related code change. So this change contains only 262 lines of production code. So this change does following things, 1. Update Differ class to support CalcDirtyRegionWithHints. This function receives a const DesktopRegion& as hint, and detects only changes in the hint areas. So on platforms with damage notifications, a ScreenCapturer implementation can use the DesktopRegion from platform APIs as the hints to help improve Differ speed by reducing blocks it checks. 2. A new ScreenCapturerDifferWrapper implementation to use Differ class to help set DesktopFrame::updated_region(). It uses an underlying ScreenCapturer to do the real capture work, and updates the updated region of DesktopFrame returned from OnCaptureResult function. 3. DifferTest to cover the new CalcDirtyRegionWithHints function. 4. Improves MockScreenCapturer to make it more useful on testing ScreenCapturerDifferWrapper. Now MockScreenCapturer supports to return controllable DesktopFrames. Users can set various flags to control its behavior. 5. ScreenCapturerDifferWrapperTest. After this change, we can eventually remove all Differ logic from ScreenCapturer* implementations, and fix a potential crash bug in ScreenCapturerLinux class. It wrongly assumes previous_frame() has a same size as current_frame(). https://goo.gl/3nSqOC A coming change is to add another ScreenCapturerExpendToGridWrapper to cover the functionality of ScreenCapturerHelper. BUG=633802 ========== to ========== An early analysis shows in DirectX based capturer, Windows API returns larger dirty region than the real screen change. A similar behavior may happen on other platforms with damage notification support. So it's better to have an individual layer to handle the Differ logic, and remove capturing independent logic out of each ScreenCapturer* implementation. So this change does following things, 1. Update Differ class to support CalcDirtyRegionWithHints. This function receives a const DesktopRegion& as hint, and detects only changes in the hint areas. So on platforms with damage notifications, a ScreenCapturer implementation can use the DesktopRegion from platform APIs as the hints to help improve Differ speed by reducing blocks it checks. 2. A new ScreenCapturerDifferWrapper implementation to use Differ class to help set DesktopFrame::updated_region(). It uses an underlying ScreenCapturer to do the real capture work, and updates the updated region of DesktopFrame returned from OnCaptureResult function. 3. DifferTest to cover the new CalcDirtyRegionWithHints function. 4. Improves MockScreenCapturer to make it more useful on testing ScreenCapturerDifferWrapper. Now MockScreenCapturer supports to return controllable DesktopFrames. Users can set various flags to control its behavior. 5. ScreenCapturerDifferWrapperTest. After this change, we can eventually remove all Differ logic from ScreenCapturer* implementations, and fix a potential crash bug in ScreenCapturerLinux class. It wrongly assumes previous_frame() has a same size as current_frame(). https://goo.gl/3nSqOC This is not a huge change, 559 out of 821 lines are unit test related code change. So this change contains only 262 lines of production code. A coming change is to add another ScreenCapturerExpendToGridWrapper to cover the functionality of ScreenCapturerHelper. BUG=633802 ==========
Description was changed from ========== An early analysis shows in DirectX based capturer, Windows API returns larger dirty region than the real screen change. A similar behavior may happen on other platforms with damage notification support. So it's better to have an individual layer to handle the Differ logic, and remove capturing independent logic out of each ScreenCapturer* implementation. So this change does following things, 1. Update Differ class to support CalcDirtyRegionWithHints. This function receives a const DesktopRegion& as hint, and detects only changes in the hint areas. So on platforms with damage notifications, a ScreenCapturer implementation can use the DesktopRegion from platform APIs as the hints to help improve Differ speed by reducing blocks it checks. 2. A new ScreenCapturerDifferWrapper implementation to use Differ class to help set DesktopFrame::updated_region(). It uses an underlying ScreenCapturer to do the real capture work, and updates the updated region of DesktopFrame returned from OnCaptureResult function. 3. DifferTest to cover the new CalcDirtyRegionWithHints function. 4. Improves MockScreenCapturer to make it more useful on testing ScreenCapturerDifferWrapper. Now MockScreenCapturer supports to return controllable DesktopFrames. Users can set various flags to control its behavior. 5. ScreenCapturerDifferWrapperTest. After this change, we can eventually remove all Differ logic from ScreenCapturer* implementations, and fix a potential crash bug in ScreenCapturerLinux class. It wrongly assumes previous_frame() has a same size as current_frame(). https://goo.gl/3nSqOC This is not a huge change, 559 out of 821 lines are unit test related code change. So this change contains only 262 lines of production code. A coming change is to add another ScreenCapturerExpendToGridWrapper to cover the functionality of ScreenCapturerHelper. BUG=633802 ========== to ========== An early analysis shows in DirectX based capturer, Windows API returns larger dirty region than the real screen change. A similar behavior may happen on other platforms with damage notification support. So it's better to have an individual layer to handle the Differ logic, and remove capturing independent logic out of each ScreenCapturer* implementation. So this change does following things, 1. Update Differ class to support CalcDirtyRegionWithHints. This function receives a const DesktopRegion& as hint, and detects only changes in the hint areas. So on platforms with damage notifications, a ScreenCapturer implementation can use the DesktopRegion from platform APIs as the hints to help improve Differ speed by reducing blocks it checks. 2. A new ScreenCapturerDifferWrapper implementation to use Differ class to help set DesktopFrame::updated_region(). It uses an underlying ScreenCapturer to do the real capture work, and updates the updated region of DesktopFrame returned from OnCaptureResult function. 3. DifferTest to cover the new CalcDirtyRegionWithHints function. 4. Improves MockScreenCapturer to make it more useful on testing ScreenCapturerDifferWrapper. Now MockScreenCapturer supports to return controllable DesktopFrames. Users can set various flags to control its behavior. 5. ScreenCapturerDifferWrapperTest. After this change, we can eventually remove all Differ logic from ScreenCapturer* implementations, and fix a potential crash bug in ScreenCapturerLinux class. It wrongly assumes previous_frame() has a same size as current_frame(). https://goo.gl/3nSqOC This is not a huge change, 559 out of 821 lines are unit test related code. So this change contains only 262 lines of production code. A coming change is to add another ScreenCapturerExpendToGridWrapper to cover the functionality of ScreenCapturerHelper. BUG=633802 ==========
https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/differ.h (right): https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/differ.h:31: class Differ { I would prefer cleaning up this code before extending it. As the TODO above suggests we no longer need diff_info_ array. It still makes sense to compare macro blocks, but the result can be stored directly in DesktopRegion. I think you can just replace this class with a single static function: void CompareFrames(const DesktopFrame& old_frame, const DesktopFrame& new_frame, DestkopRegion* diff_region_out). That would make this code simpler and easier to use. WDYT? https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc (right): https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:68: if (frame->updated_region().is_empty()) { This will cause the this capturer to scan the whole frame, when screen doesn't change and the underlying capturer does support updated_region(). So I think you want to use a different mechanismt to find out if the base capturer support updated_region. Maybe a bool flag passed to the constructor. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h (right): https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h:28: : public ScreenCapturer, public DesktopCapturer::Callback { git cl format btw, you can get clang-format integrated in your text editor, depending on the editor you use, see https://chromium.googlesource.com/chromium/src/+/master/docs/clang_format.md . Saves a lot time for me. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h:28: : public ScreenCapturer, public DesktopCapturer::Callback { Potentially the same mechanism could work for window capturer as well. Currently WindowCapturer and ScreenCapturer are different interfaces, but potentially we remove them and move everything back to DesktopCapturer. The only difference is GetScreenList() vs GetWindowList(). We could just add GetSourceList() and SelectSource() in DesktopCapturer and then remove WindowCapturer and ScreenCapturer. Let me know if you want to take on this task and we can discuss you logistics of how it can be done without breaking chromium build. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h:32: explicit ScreenCapturerDifferWrapper(std::unique_ptr<ScreenCapturer> impl); |impl| is not the best name. maybe call it base_capturer? https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h:35: void Start(DesktopCapturer::Callback* callback) override; // ScreenCapturer interface. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h:42: // Catches callback from underlying implementation, uses Differ to update I don't think you really need this comment here. It's obvious why you need to implement DesktopCapturer::Callback interface. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h:43: // |frame|->updated_region(), and calls callback_. Add // DesktopCapturer::Callback interface. move this to the private section, to make it clear that this method is not expected to be called directly. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h:48: std::unique_ptr<ScreenCapturer> impl_; base_capturer_? https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_capturer_mock_objects.h (right): https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mock_objects.h:26: class MockScreenCapturer : public ScreenCapturer { This class is not used anywhere right now. I suggest removing it completely and adding FakeScreenCapturer that doesn't use on GMOCK. There is also FakeDesktopCapturer in remoting/protocol/fake_desktop_capturer.h which may be useful for you. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mock_objects.h:63: bool* return_frame() { why do you need return bool pointer?
jamiewalch@chromium.org changed reviewers: + jamiewalch@chromium.org
https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/differ.cc (right): https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/differ.cc:40: DesktopRegion* region) { Maybe just call through to CalcDirtyRegionWithHints() with a full-screen region to save duplicating the few additional lines here? https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/differ.cc:83: // Calculate the range of blocks. Since right and bottom are both past-the-end pointers, can't you simplify this to just: x_end = (rect.right() - 1) / kBlockSize; Then you don't need the conditionals. Also, please make sure you include tests for changes on or close to a block boundary since this sort of thing is prone to off-by-one errors. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/differ.cc:124: diff_info += sizeof(bool); This should just be diff_info++, I think, since pointer arithmetic already takes sizeof into account. It only works if sizeof(bool)==1, (which is very likely, but not guaranteed). Since you haven't changed this, you can fix in a follow-up CL if you prefer. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/differ.cc:128: // This condition should rarely, if ever, occur. I suspect this is no longer true; it's rare when talking about the full screen because of typical screen widths, but damage rectangles can be arbitrary. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/differ.cc:143: // 'partial column' case. As above, this second sentence can be removed now. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/differ.h (right): https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/differ.h:43: // Given the previous and current screen buffer, calculates the dirty region FWIW, either calculate or calculates would be appropriate here. Since the other comments here are written as instructions rather than descriptions (eg, "Identify all the blocks", not "Identifies all the blocks"), I'd argue that the original was more consistent. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/differ.h:69: void MergeBlocks(DesktopRegion* region) const; Optional: I'm not sure it's worth adding these consts. I don't have a strong opinion about it, but I don't think it's something we commonly do, so it leaves the reader wondering what's special about this class that needs it. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/differ_unittest.cc (right): https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/differ_unittest.cc:172: bool CheckDirtyRegionContainsPartialRect(const DesktopRegion& region, "Partial" is used in the differ to refer to sub-kBlockSize blocks, so I'm surprised to see the multiplications in this function. How is "partial" being used here? https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h (right): https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h:27: class ScreenCapturerDifferWrapper This class is not currently being used. Will there be a follow-up CL to delete the diffing code from the platform-specific capturers and to use this wrapper instead? https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper_unittest.cc (right): https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper_unittest.cc:29: void AssertUpdatedRegionAre(const DesktopFrame& frame, s/Are/Is/
Patchset #2 (id:100001) has been deleted
https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/differ.cc (right): https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/differ.cc:40: DesktopRegion* region) { On 2016/08/03 23:52:25, Jamie wrote: > Maybe just call through to CalcDirtyRegionWithHints() with a full-screen region > to save duplicating the few additional lines here? If you do not have concern with the unnecessary for-loop in WithHints function, I am happy to do so. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/differ.cc:83: // Calculate the range of blocks. On 2016/08/03 23:52:25, Jamie wrote: > Since right and bottom are both past-the-end pointers, can't you simplify this > to just: > > x_end = (rect.right() - 1) / kBlockSize; > > Then you don't need the conditionals. Also, please make sure you include tests > for changes on or close to a block boundary since this sort of thing is prone to > off-by-one errors. This seems not correct to me, Case 1: considering rect.right() is 1000, kBlockSize is 32, width_ is 1024. Current logic will return x_end = 32. Changing to (rect.right() - 1) / kBlockSize, will return x_end = 31. Case 2: considering rect.right() is 1025, kBlockSize is 32, width_ is 1025. Current logic will return x_end = 32. Changing to (rect.right() - 1) / kBlockSize, will also return x_end = 32. But in both cases, x_end is expected to be 32. As it indicates a set of blocks to cover the |rect|, but leave part of them as partial column width / row height. i.e. partial_column_width and partial_row_height should be always smaller than kBlockSize. I have added two RTC_DCHECK after partial_column_width / partial_row_height assignments to ensure this conclusion is always correct. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/differ.cc:124: diff_info += sizeof(bool); On 2016/08/03 23:52:25, Jamie wrote: > This should just be diff_info++, I think, since pointer arithmetic already takes > sizeof into account. It only works if sizeof(bool)==1, (which is very likely, > but not guaranteed). > > Since you haven't changed this, you can fix in a follow-up CL if you prefer. Oh, I have not noticed these lines (and three lines below). It's trivial, I will fix it in this change. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/differ.cc:128: // This condition should rarely, if ever, occur. On 2016/08/03 23:52:25, Jamie wrote: > I suspect this is no longer true; it's rare when talking about the full screen > because of typical screen widths, but damage rectangles can be arbitrary. We always try our best to cover the damage rectangles by full blocks, as full block comparison is faster. So partial_column_width > 0 should still be rarely, only at the right and bottom edge of the screen. But I think the comment to explain whether it's rarely or not is not so useful. We always need to handle it. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/differ.cc:143: // 'partial column' case. On 2016/08/03 23:52:25, Jamie wrote: > As above, this second sentence can be removed now. Done. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/differ.h (right): https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/differ.h:31: class Differ { On 2016/08/03 23:19:11, Sergey Ulanov wrote: > I would prefer cleaning up this code before extending it. As the TODO above > suggests we no longer need diff_info_ array. It still makes sense to compare > macro blocks, but the result can be stored directly in DesktopRegion. > I think you can just replace this class with a single static function: > void CompareFrames(const DesktopFrame& old_frame, > const DesktopFrame& new_frame, > DestkopRegion* diff_region_out). > > That would make this code simpler and easier to use. WDYT? I am happy with the change. But it seems like a very huge change, which impacts various ScreenCapturer(s). I think we can reduce the change by, 1. Submit this change. 2. Using ScreenCapturerDifferWrapper for each ScreenCapturer implementation, and removing the reference to Differ class from them. 3. Update Differ. Then we will only need to change one place (ScreenCapturerDifferWrapper) instead of three capturers (ScreenCapturerLinux / ScreenCapturerWin / ScreenCapturerMac). https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/differ.h:43: // Given the previous and current screen buffer, calculates the dirty region On 2016/08/03 23:52:25, Jamie wrote: > FWIW, either calculate or calculates would be appropriate here. Since the other > comments here are written as instructions rather than descriptions (eg, > "Identify all the blocks", not "Identifies all the blocks"), I'd argue that the > original was more consistent. Done. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/differ.h:69: void MergeBlocks(DesktopRegion* region) const; On 2016/08/03 23:52:25, Jamie wrote: > Optional: I'm not sure it's worth adding these consts. I don't have a strong > opinion about it, but I don't think it's something we commonly do, so it leaves > the reader wondering what's special about this class that needs it. IMO, a const indeed tells the reader, this function does not need to impact the class internal states. i.e. It impacts only the parameters. And in Google C++ style guide, using const for functions is strongly recommended. (https://goo.gl/sbqlAI) So does Chromium have a different suggestion? https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/differ_unittest.cc (right): https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/differ_unittest.cc:172: bool CheckDirtyRegionContainsPartialRect(const DesktopRegion& region, On 2016/08/03 23:52:26, Jamie wrote: > "Partial" is used in the differ to refer to sub-kBlockSize blocks, so I'm > surprised to see the multiplications in this function. How is "partial" being > used here? The "partial" here means one of the dirty regions is part of the input block (x, y) -> (x + width, y + height). And this function is used only in the case, where the resolution of the screen is not an integral multiple of kBlockSize. (At the bottom of this file.) You know, I am not a native speaker, so feel free to suggest any more appropriate name. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc (right): https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:68: if (frame->updated_region().is_empty()) { On 2016/08/03 23:19:11, Sergey Ulanov wrote: > This will cause the this capturer to scan the whole frame, when screen doesn't > change and the underlying capturer does support updated_region(). So I think you > want to use a different mechanismt to find out if the base capturer support > updated_region. Maybe a bool flag passed to the constructor. Good point. Update. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h (right): https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h:27: class ScreenCapturerDifferWrapper On 2016/08/03 23:52:26, Jamie wrote: > This class is not currently being used. Will there be a follow-up CL to delete > the diffing code from the platform-specific capturers and to use this wrapper > instead? Exactly. Bug 633802 is opened to track all the works. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h:28: : public ScreenCapturer, public DesktopCapturer::Callback { On 2016/08/03 23:19:11, Sergey Ulanov wrote: > Potentially the same mechanism could work for window capturer as well. Currently > WindowCapturer and ScreenCapturer are different interfaces, but potentially we > remove them and move everything back to DesktopCapturer. The only difference is > GetScreenList() vs GetWindowList(). We could just add GetSourceList() and > SelectSource() in DesktopCapturer and then remove WindowCapturer and > ScreenCapturer. Let me know if you want to take on this task and we can discuss > you logistics of how it can be done without breaking chromium build. Sure. We can discuss it later. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h:28: : public ScreenCapturer, public DesktopCapturer::Callback { On 2016/08/03 23:19:11, Sergey Ulanov wrote: > git cl format > > btw, you can get clang-format integrated in your text editor, depending on the > editor you use, see > https://chromium.googlesource.com/chromium/src/+/master/docs/clang_format.md . > Saves a lot time for me. Done. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h:32: explicit ScreenCapturerDifferWrapper(std::unique_ptr<ScreenCapturer> impl); On 2016/08/03 23:19:12, Sergey Ulanov wrote: > |impl| is not the best name. maybe call it base_capturer? Done. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h:35: void Start(DesktopCapturer::Callback* callback) override; On 2016/08/03 23:19:11, Sergey Ulanov wrote: > // ScreenCapturer interface. Done. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h:42: // Catches callback from underlying implementation, uses Differ to update On 2016/08/03 23:19:11, Sergey Ulanov wrote: > I don't think you really need this comment here. It's obvious why you need to > implement DesktopCapturer::Callback interface. Done. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h:43: // |frame|->updated_region(), and calls callback_. On 2016/08/03 23:19:12, Sergey Ulanov wrote: > Add > // DesktopCapturer::Callback interface. > > move this to the private section, to make it clear that this method is not > expected to be called directly. Done. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h:48: std::unique_ptr<ScreenCapturer> impl_; On 2016/08/03 23:19:11, Sergey Ulanov wrote: > base_capturer_? Done. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper_unittest.cc (right): https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper_unittest.cc:29: void AssertUpdatedRegionAre(const DesktopFrame& frame, On 2016/08/03 23:52:26, Jamie wrote: > s/Are/Is/ Done. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_capturer_mock_objects.h (right): https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mock_objects.h:26: class MockScreenCapturer : public ScreenCapturer { On 2016/08/03 23:19:12, Sergey Ulanov wrote: > This class is not used anywhere right now. I suggest removing it completely and > adding FakeScreenCapturer that doesn't use on GMOCK. > There is also FakeDesktopCapturer in remoting/protocol/fake_desktop_capturer.h > which may be useful for you. Yes, I have found fake_desktop_capturer in remoting/protocol. I have no background information, but is there any reason we do not want to use GMock? I think it's convenient EXPECT_CALL is helpful in this specific test case. https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mock_objects.h:63: bool* return_frame() { On 2016/08/03 23:19:12, Sergey Ulanov wrote: > why do you need return bool pointer? 1. According to the coding style, we cannot return a reference. 2. A set / get function is not so good for dirty_region(), i.e. several functions of DesktopRegion can impact it's state, I do not want to write them all here. So I have to return DesktopRegion* dirty_region(). 3. So to keep all the functions consistent, I decided to use pointers for all these fields. Which is also same as protobuf.
https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h (right): https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h:28: : public ScreenCapturer, public DesktopCapturer::Callback { On 2016/08/04 02:28:07, Hzj_jie wrote: > On 2016/08/03 23:19:11, Sergey Ulanov wrote: > > Potentially the same mechanism could work for window capturer as well. > Currently > > WindowCapturer and ScreenCapturer are different interfaces, but potentially we > > remove them and move everything back to DesktopCapturer. The only difference > is > > GetScreenList() vs GetWindowList(). We could just add GetSourceList() and > > SelectSource() in DesktopCapturer and then remove WindowCapturer and > > ScreenCapturer. Let me know if you want to take on this task and we can > discuss > > you logistics of how it can be done without breaking chromium build. > > Sure. We can discuss it later. And I also found, 1. Nobody, both ScreenCapturer and WindowCapturer, uses the input (const DesktopRegion& region) parameter in Capture function. 2. The region parameter can be replaced by a DesktopRect. 3. Though on Linux and Mac, there are different implementations for WindowCapturer and ScreenCapturer, indeed a WindowCapturer can be backed by a ScreenCapturer. i.e. Capture the right screen, and select part of it. Or capture a right rectangle. So I thought we can have an set of DesktopCapturer implementations, with only functions of current DesktopCapturer interface. (Only replace DesktopRegion to DesktopRect) All the differ / extend to grid / directx / gdi / damage notification / shared memory / double buffering, etc, should belong to DesktopCapturer(s). And ScreenCapturer(s) / WindowCapturer(s) just need to send a correct rectangle to the DesktopCapturer(s) to capture a single window or screen. Then we can eventually remove duplicate logic from ScreenCapturer(s) and WindowCapturer(s). I do not see a potential performance loss on Linux and Windows by using a ScreenCapturer to capture a single window. I cannot quite sure about Mac, though it seems fine.
On 2016/08/04 03:28:43, Hzj_jie wrote: > https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... > File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h (right): > > https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... > webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h:28: : public > ScreenCapturer, public DesktopCapturer::Callback { > On 2016/08/04 02:28:07, Hzj_jie wrote: > > On 2016/08/03 23:19:11, Sergey Ulanov wrote: > > > Potentially the same mechanism could work for window capturer as well. > > Currently > > > WindowCapturer and ScreenCapturer are different interfaces, but potentially > we > > > remove them and move everything back to DesktopCapturer. The only difference > > is > > > GetScreenList() vs GetWindowList(). We could just add GetSourceList() and > > > SelectSource() in DesktopCapturer and then remove WindowCapturer and > > > ScreenCapturer. Let me know if you want to take on this task and we can > > discuss > > > you logistics of how it can be done without breaking chromium build. > > > > Sure. We can discuss it later. > > And I also found, > 1. Nobody, both ScreenCapturer and WindowCapturer, uses the input (const > DesktopRegion& region) parameter in Capture function. Right. That parameter can be removed. > > 2. The region parameter can be replaced by a DesktopRect. I suggest removing it completely. > > 3. Though on Linux and Mac, there are different implementations for > WindowCapturer and ScreenCapturer, indeed a WindowCapturer can be backed by a > ScreenCapturer. i.e. Capture the right screen, and select part of it. Or capture > a right rectangle. > > So I thought we can have an set of DesktopCapturer implementations, with only > functions of current DesktopCapturer interface. (Only replace DesktopRegion to > DesktopRect) All the differ / extend to grid / directx / gdi / damage > notification / shared memory / double buffering, etc, should belong to > DesktopCapturer(s). > > And ScreenCapturer(s) / WindowCapturer(s) just need to send a correct rectangle > to the DesktopCapturer(s) to capture a single window or screen. Then we can > eventually remove duplicate logic from ScreenCapturer(s) and WindowCapturer(s). WindowCapturer needs to work even when the target window covered by other window. So to capture windows you cannot always use the same mechanism that used to capture screen. (Capturing windows by cropping them from a desktop may be faster and provide better UX. For that reason there is CroppingWindowCapturer to capture windows by cropping them from screen. It still falls back to WindowCapturerWin when the window is covered with other windows.) So I think overall WindowCapturers and ScreenCapturers are different beasts and I don't think they can always share the underlying impls. What I was suggesting is to remove ScreenCapturer and WindowCapturer _interfaces_ by moving all functionality to DesktopCapturer interface. The implementations of the corresponding capturers will still need to be different. > > I do not see a potential performance loss on Linux and Windows by using a > ScreenCapturer to capture a single window. I cannot quite sure about Mac, though > it seems fine.
https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/differ.h (right): https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/differ.h:31: class Differ { On 2016/08/04 02:28:07, Hzj_jie wrote: > On 2016/08/03 23:19:11, Sergey Ulanov wrote: > > I would prefer cleaning up this code before extending it. As the TODO above > > suggests we no longer need diff_info_ array. It still makes sense to compare > > macro blocks, but the result can be stored directly in DesktopRegion. > > I think you can just replace this class with a single static function: > > void CompareFrames(const DesktopFrame& old_frame, > > const DesktopFrame& new_frame, > > DestkopRegion* diff_region_out). > > > > That would make this code simpler and easier to use. WDYT? > > I am happy with the change. But it seems like a very huge change, which impacts > various ScreenCapturer(s). > I think we can reduce the change by, > 1. Submit this change. > 2. Using ScreenCapturerDifferWrapper for each ScreenCapturer implementation, and > removing the reference to Differ class from them. > 3. Update Differ. > Then we will only need to change one place (ScreenCapturerDifferWrapper) instead > of three capturers (ScreenCapturerLinux / ScreenCapturerWin / > ScreenCapturerMac). Note that ScreenCapturerMac doesn't use Differ. How about a slightly different plan: 1. Implement ScreenCapturerDifferWrapper without using Differ, e.g. put all diff logic directly in screen_capturer_differ_wrapper.cc 2. Remove Differ and it's usage from everywhere. That way we can avoid wasting time working on this old code that needs to be rewritten anyway.
On 2016/08/08 18:26:57, Sergey Ulanov wrote: > On 2016/08/04 03:28:43, Hzj_jie wrote: > > > https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... > > File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h (right): > > > > > https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... > > webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h:28: : public > > ScreenCapturer, public DesktopCapturer::Callback { > > On 2016/08/04 02:28:07, Hzj_jie wrote: > > > On 2016/08/03 23:19:11, Sergey Ulanov wrote: > > > > Potentially the same mechanism could work for window capturer as well. > > > Currently > > > > WindowCapturer and ScreenCapturer are different interfaces, but > potentially > > we > > > > remove them and move everything back to DesktopCapturer. The only > difference > > > is > > > > GetScreenList() vs GetWindowList(). We could just add GetSourceList() and > > > > SelectSource() in DesktopCapturer and then remove WindowCapturer and > > > > ScreenCapturer. Let me know if you want to take on this task and we can > > > discuss > > > > you logistics of how it can be done without breaking chromium build. > > > > > > Sure. We can discuss it later. > > > > And I also found, > > 1. Nobody, both ScreenCapturer and WindowCapturer, uses the input (const > > DesktopRegion& region) parameter in Capture function. > > Right. That parameter can be removed. > > > > > 2. The region parameter can be replaced by a DesktopRect. > > I suggest removing it completely. > > > > > 3. Though on Linux and Mac, there are different implementations for > > WindowCapturer and ScreenCapturer, indeed a WindowCapturer can be backed by a > > ScreenCapturer. i.e. Capture the right screen, and select part of it. Or > capture > > a right rectangle. > > > > So I thought we can have an set of DesktopCapturer implementations, with only > > functions of current DesktopCapturer interface. (Only replace DesktopRegion to > > DesktopRect) All the differ / extend to grid / directx / gdi / damage > > notification / shared memory / double buffering, etc, should belong to > > DesktopCapturer(s). > > > > And ScreenCapturer(s) / WindowCapturer(s) just need to send a correct > rectangle > > to the DesktopCapturer(s) to capture a single window or screen. Then we can > > eventually remove duplicate logic from ScreenCapturer(s) and > WindowCapturer(s). > > WindowCapturer needs to work even when the target window covered by other > window. So to capture windows you cannot always use the same mechanism that used > to capture screen. (Capturing windows by cropping them from a desktop may be > faster and provide better UX. For that reason there is CroppingWindowCapturer to > capture windows by cropping them from screen. It still falls back to > WindowCapturerWin when the window is covered with other windows.) So I think > overall WindowCapturers and ScreenCapturers are different beasts and I don't > think they can always share the underlying impls. What I was suggesting is to > remove ScreenCapturer and WindowCapturer _interfaces_ by moving all > functionality to DesktopCapturer interface. The implementations of the > corresponding capturers will still need to be different. > > Oh, that's totally out of my expectation, and sorry for lacking investigation. I thought we would always call BringSelectedWindowToFront before really capturing the content. > > > > I do not see a potential performance loss on Linux and Windows by using a > > ScreenCapturer to capture a single window. I cannot quite sure about Mac, > though > > it seems fine.
On 2016/08/08 18:42:42, Sergey Ulanov wrote: > https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... > File webrtc/modules/desktop_capture/differ.h (right): > > https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... > webrtc/modules/desktop_capture/differ.h:31: class Differ { > On 2016/08/04 02:28:07, Hzj_jie wrote: > > On 2016/08/03 23:19:11, Sergey Ulanov wrote: > > > I would prefer cleaning up this code before extending it. As the TODO above > > > suggests we no longer need diff_info_ array. It still makes sense to compare > > > macro blocks, but the result can be stored directly in DesktopRegion. > > > I think you can just replace this class with a single static function: > > > void CompareFrames(const DesktopFrame& old_frame, > > > const DesktopFrame& new_frame, > > > DestkopRegion* diff_region_out). > > > > > > That would make this code simpler and easier to use. WDYT? > > > > I am happy with the change. But it seems like a very huge change, which > impacts > > various ScreenCapturer(s). > > I think we can reduce the change by, > > 1. Submit this change. > > 2. Using ScreenCapturerDifferWrapper for each ScreenCapturer implementation, > and > > removing the reference to Differ class from them. > > 3. Update Differ. > > Then we will only need to change one place (ScreenCapturerDifferWrapper) > instead > > of three capturers (ScreenCapturerLinux / ScreenCapturerWin / > > ScreenCapturerMac). > > Note that ScreenCapturerMac doesn't use Differ. > > How about a slightly different plan: > 1. Implement ScreenCapturerDifferWrapper without using Differ, e.g. put all diff > logic directly in screen_capturer_differ_wrapper.cc > 2. Remove Differ and it's usage from everywhere. > > That way we can avoid wasting time working on this old code that needs to be > rewritten anyway. Sounds good. I am working on it.
Patchset #4 (id:160001) has been deleted
I have moved Differ logic into ScreenCapturerDifferWrapper class. And I added several performance tests, but since the hardware performance may vary, and DR. memory slows down the machine, these performance tests are not so stable. Do you happen to have any suggestion on this?
https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_capturer_mock_objects.h (right): https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mock_objects.h:26: class MockScreenCapturer : public ScreenCapturer { On 2016/08/04 02:28:07, Hzj_jie wrote: > On 2016/08/03 23:19:12, Sergey Ulanov wrote: > > This class is not used anywhere right now. I suggest removing it completely > and > > adding FakeScreenCapturer that doesn't use on GMOCK. > > There is also FakeDesktopCapturer in remoting/protocol/fake_desktop_capturer.h > > which may be useful for you. > > Yes, I have found fake_desktop_capturer in remoting/protocol. I have no > background information, but is there any reason we do not want to use GMock? I > think it's convenient EXPECT_CALL is helpful in this specific test case. Canonical way to use gmock is to have a virtual interface stub without any behavior and let the test how each call is handled. Here you are adding default behavior for the interface via ON_CALL() in the .cc file. This is not what most people would expect from Mock* class. (there was a somewhat related discussion in chromium-dev: https://groups.google.com/a/chromium.org/d/topic/chromium-dev/-KH_IP0rIWQ/dis... ). So you can either 1. Keep this class as is and move all behavior to the tests. 2. Add a FakeScreenCapturer without GMock and use it instead. I'd recommend 2 as it usually leads to simpler and more readable tests https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mock_objects.h:51: DesktopRegion* dirty_region() { call it updated_region or next_updated_region for consistency with DesktopFrame? https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mock_objects.h:63: bool* return_frame() { On 2016/08/04 02:28:07, Hzj_jie wrote: > On 2016/08/03 23:19:12, Sergey Ulanov wrote: > > why do you need return bool pointer? > > 1. According to the coding style, we cannot return a reference. > 2. A set / get function is not so good for dirty_region(), i.e. several > functions of DesktopRegion can impact it's state, I do not want to write them > all here. So I have to return DesktopRegion* dirty_region(). What's wrong with [set_]dirty_region()? You could have mutable_updated_region() as in DesktopFrame. Would that work here? > 3. So to keep all the functions consistent, I decided to use pointers for all > these fields. Which is also same as protobuf. I think this approach is rather confusing and inconsistent with most other classes that have get/set methods. https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc (right): https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:30: // kBlockSize), otherwise BlockDifference() should be used. The comment should make it clear if true result indicates that the block are different or if they are the same. Or it would be better to rename the function to make it clear (e.g. PartialBlockDiffer() as in differ.cc) https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:31: bool PartialBlockDifference(const uint8_t* prev, Style guide discourages names like 'prev' and 'curr'. maybe old_buffer, new_buffer? Also the comment above mentions |old| and |new| https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:33: int stride, potentially stride may be different for the two buffers. Pass two different values? Previously the capturers could ensure that it never happens, but you cannot assume it here. https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:36: RTC_DCHECK(width < kBlockSize || height < kBlockSize); Why do you need to check it here? Also, I think code in differ_block.cc can be updated to handle blocks of any arbitrary height (only per-row comparison is optimized there), then this function would only be useful the case when width !=32. https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:115: void ComparePartialBlockRow(const uint8_t* prev, See my previous comment. I think the optimized block differ can be changed to handle blocks of arbitrary height, and then you wouldn't need to duplicate bunch of code here and in CompareBlockRow(). https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:172: const DesktopRect rect, DesktopRect& https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:226: ScreenCapturerDifferWrapper::~ScreenCapturerDifferWrapper() = default; {} instead of = default; would be cleaner here https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:284: frame->set_capture_time_ms(frame->capture_time_ms() + frame may be nullptr here. Handle errors case first in top of this function. https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h (right): https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h:22: // implementation, and help to calculate the dirty region. suggest rewording: // ScreenCapturer wrapper that calculates updated_region() by comparing frames // content. https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h:24: // 1. Underlying capturer should use SharedDesktopFrame to return captured I think it's better to avoid this restriction by always wrapping returned frames in a new SharedDesktopFrame. https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h:26: // 2. If the underlying capturer provides updated_region() as hints, it expects I don't think this needs to be mentioned here. Valid ScreenCapturer implementation should never return updated_region() that outside of the screen boundaries. It's also easy to make this class deal with this. https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper_unittest.cc (right): https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper_unittest.cc:251: TEST(ScreenCapturerDifferWrapperTest, CaptureWithoutHintsPerf) { Perf test shouldn't be mixed with unittests. They would slow-down unittest runs for everybody without much benefit. You can either move them to a separate binary (that has to be ran manually) or mark these tests as manual.
Patchset #5 (id:200001) has been deleted
https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_capturer_mock_objects.h (right): https://codereview.chromium.org/2202443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mock_objects.h:26: class MockScreenCapturer : public ScreenCapturer { On 2016/08/12 05:25:50, Sergey Ulanov wrote: > On 2016/08/04 02:28:07, Hzj_jie wrote: > > On 2016/08/03 23:19:12, Sergey Ulanov wrote: > > > This class is not used anywhere right now. I suggest removing it completely > > and > > > adding FakeScreenCapturer that doesn't use on GMOCK. > > > There is also FakeDesktopCapturer in > remoting/protocol/fake_desktop_capturer.h > > > which may be useful for you. > > > > Yes, I have found fake_desktop_capturer in remoting/protocol. I have no > > background information, but is there any reason we do not want to use GMock? I > > think it's convenient EXPECT_CALL is helpful in this specific test case. > > Canonical way to use gmock is to have a virtual interface stub without any > behavior and let the test how each call is handled. Here you are adding default > behavior for the interface via ON_CALL() in the .cc file. This is not what most > people would expect from Mock* class. (there was a somewhat related discussion > in chromium-dev: > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/-KH_IP0rIWQ/dis... > ). > So you can either > 1. Keep this class as is and move all behavior to the tests. > 2. Add a FakeScreenCapturer without GMock and use it instead. > > I'd recommend 2 as it usually leads to simpler and more readable tests Then I would rather choose the second one, as the implementation would be reused in other test cases. https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc (right): https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:30: // kBlockSize), otherwise BlockDifference() should be used. On 2016/08/12 05:25:51, Sergey Ulanov wrote: > The comment should make it clear if true result indicates that the block are > different or if they are the same. Or it would be better to rename the function > to make it clear (e.g. PartialBlockDiffer() as in differ.cc) Considering Differ class will be removed, I would prefer to make this function consistent with differ_block.h. The comment has been updated. https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:31: bool PartialBlockDifference(const uint8_t* prev, On 2016/08/12 05:25:51, Sergey Ulanov wrote: > Style guide discourages names like 'prev' and 'curr'. > maybe old_buffer, new_buffer? > Also the comment above mentions |old| and |new| Done. https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:33: int stride, On 2016/08/12 05:25:50, Sergey Ulanov wrote: > potentially stride may be different for the two buffers. Pass two different > values? > Previously the capturers could ensure that it never happens, but you cannot > assume it here. These functions are only called from CompareFrames function, which guarantees old_frame.stride() equals to new_frame.stride() by using a RTC_DCHECK_EQ. https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:36: RTC_DCHECK(width < kBlockSize || height < kBlockSize); On 2016/08/12 05:25:50, Sergey Ulanov wrote: > Why do you need to check it here? > > Also, I think code in differ_block.cc can be updated to handle blocks of any > arbitrary height (only per-row comparison is optimized there), then this > function would only be useful the case when width !=32. This seems not true for NEON (though we have not implemented one.) In NEON, we need a 32 bit * 4 x 4 block to use its SIMD instructions. https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:115: void ComparePartialBlockRow(const uint8_t* prev, On 2016/08/12 05:25:50, Sergey Ulanov wrote: > See my previous comment. I think the optimized block differ can be changed to > handle blocks of arbitrary height, and then you wouldn't need to duplicate bunch > of code here and in CompareBlockRow(). I am happy to do so, if we are on the same page for NEON's 4 x 4 block issue. According to current block_differ_sse2 implementation, indeed we can change block differ to handle 32 x 1 block. https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:172: const DesktopRect rect, On 2016/08/12 05:25:51, Sergey Ulanov wrote: > DesktopRect& Done. https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:226: ScreenCapturerDifferWrapper::~ScreenCapturerDifferWrapper() = default; On 2016/08/12 05:25:50, Sergey Ulanov wrote: > {} instead of = default; would be cleaner here Done. https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:284: frame->set_capture_time_ms(frame->capture_time_ms() + On 2016/08/12 05:25:50, Sergey Ulanov wrote: > frame may be nullptr here. Handle errors case first in top of this function. Done. https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h (right): https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h:22: // implementation, and help to calculate the dirty region. On 2016/08/12 05:25:51, Sergey Ulanov wrote: > suggest rewording: > // ScreenCapturer wrapper that calculates updated_region() by comparing frames > // content. Done. https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h:24: // 1. Underlying capturer should use SharedDesktopFrame to return captured On 2016/08/12 05:25:51, Sergey Ulanov wrote: > I think it's better to avoid this restriction by always wrapping returned frames > in a new SharedDesktopFrame. Done. https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h:26: // 2. If the underlying capturer provides updated_region() as hints, it expects On 2016/08/12 05:25:51, Sergey Ulanov wrote: > I don't think this needs to be mentioned here. Valid ScreenCapturer > implementation should never return updated_region() that outside of the screen > boundaries. It's also easy to make this class deal with this. Done. https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper_unittest.cc (right): https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper_unittest.cc:251: TEST(ScreenCapturerDifferWrapperTest, CaptureWithoutHintsPerf) { On 2016/08/12 05:25:51, Sergey Ulanov wrote: > Perf test shouldn't be mixed with unittests. They would slow-down unittest runs > for everybody without much benefit. > You can either move them to a separate binary (that has to be ran manually) or > mark these tests as manual. Done.
https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc (right): https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:33: int stride, On 2016/08/15 00:38:06, Hzj_jie wrote: > On 2016/08/12 05:25:50, Sergey Ulanov wrote: > > potentially stride may be different for the two buffers. Pass two different > > values? > > Previously the capturers could ensure that it never happens, but you cannot > > assume it here. > > These functions are only called from CompareFrames function, which guarantees > old_frame.stride() equals to new_frame.stride() by using a RTC_DCHECK_EQ. Mention it in screen_capturer_differ_wrapper.h? https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:36: RTC_DCHECK(width < kBlockSize || height < kBlockSize); On 2016/08/15 00:38:06, Hzj_jie wrote: > On 2016/08/12 05:25:50, Sergey Ulanov wrote: > > Why do you need to check it here? > > > > Also, I think code in differ_block.cc can be updated to handle blocks of any > > arbitrary height (only per-row comparison is optimized there), then this > > function would only be useful the case when width !=32. > > This seems not true for NEON (though we have not implemented one.) In NEON, we > need a 32 bit * 4 x 4 block to use its SIMD instructions. Why is it 4x4 blocks? can you point me instructions/intrinsics that we would use for NEON and that require 4x4 blocks? In either case, currently we don't have optimized version for NEON and I don't think we are going to need it in foreseeable future. But even if we ever implement it, it will still be possible to make that code work with arbitrary height. It looks wrong to sacrifice code simplicity for slightly more optimal code on architecture where this code is not used. The change I suggested would not only allow to avoid code duplication but will also make it possible to take advantage of SSE optimizations where we are not currently doing so (as it will be used for all blocks of size 32, not just those that are 32 pixels high), and I think it's important as with this change we will have much higher percentage of "partial" block comparisons.
https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc (right): https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:33: int stride, On 2016/08/15 22:04:50, Sergey Ulanov wrote: > On 2016/08/15 00:38:06, Hzj_jie wrote: > > On 2016/08/12 05:25:50, Sergey Ulanov wrote: > > > potentially stride may be different for the two buffers. Pass two different > > > values? > > > Previously the capturers could ensure that it never happens, but you cannot > > > assume it here. > > > > These functions are only called from CompareFrames function, which guarantees > > old_frame.stride() equals to new_frame.stride() by using a RTC_DCHECK_EQ. > > Mention it in screen_capturer_differ_wrapper.h? Done. https://codereview.chromium.org/2202443002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:36: RTC_DCHECK(width < kBlockSize || height < kBlockSize); On 2016/08/15 22:04:50, Sergey Ulanov wrote: > On 2016/08/15 00:38:06, Hzj_jie wrote: > > On 2016/08/12 05:25:50, Sergey Ulanov wrote: > > > Why do you need to check it here? > > > > > > Also, I think code in differ_block.cc can be updated to handle blocks of any > > > arbitrary height (only per-row comparison is optimized there), then this > > > function would only be useful the case when width !=32. > > > > This seems not true for NEON (though we have not implemented one.) In NEON, we > > need a 32 bit * 4 x 4 block to use its SIMD instructions. > > Why is it 4x4 blocks? can you point me instructions/intrinsics that we would use > for NEON and that require 4x4 blocks? > In either case, currently we don't have optimized version for NEON and I don't > think we are going to need it in foreseeable future. But even if we ever > implement it, it will still be possible to make that code work with arbitrary > height. It looks wrong to sacrifice code simplicity for slightly more optimal > code on architecture where this code is not used. The change I suggested would > not only allow to avoid code duplication but will also make it possible to take > advantage of SSE optimizations where we are not currently doing so (as it will > be used for all blocks of size 32, not just those that are 32 pixels high), and > I think it's important as with this change we will have much higher percentage > of "partial" block comparisons. Not really, this class will try to use as many block comparisons as possible. So the partial block comparisons happen only at the right and bottom of the entire frame. The disadvantage is in the worst case, we will compare one block for four times, as we discarded differ-info boolean array. I have tried to use VectorDifference, but found DesktopRegion::AddRect took too much time to add (x, 1) rectangles. So a trade-off is to use BlockDifference with variable height.
https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_frame_generator.cc (right): https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame_generator.cc:46: rect.bottom() + random.Rand(enlarge_range)); There are several other places where we need the same operation. Maybe add Extend() in DesktopRect? https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame_generator.cc:75: memcpy(&frame->data()[row + column], &color, sizeof(color)); Instead of calling memcpy() for every pixel it would be better to get uint32_t and use simple assignment. E.g. see DrawRect() here: https://codesearch.chromium.org/chromium/src/remoting/test/frame_generator_ut... https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame_generator.cc:120: } else { Don't use else after return. Best to move the !return_frame_ case on top. https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_frame_generator.h (right): https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame_generator.h:29: virtual std::unique_ptr<DesktopFrame> operator()( style guide discourages operator overloading. https://google.github.io/styleguide/cppguide.html#Operator_Overloading . Operator() specifically makes the calling code rather confusing, and there is no good reason you need it here. Call this method GetNextFrame()? https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame_generator.h:37: class BaseDesktopFrameGenerator : public DesktopFrameGenerator { I don't think you really need inheritance here. Style guide says that composition should be preferred: https://google.github.io/styleguide/cppguide.html#Inheritance In this case this class can simply take a pointer or rtc::Callback<> to the Paint() function https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/differ_vector_sse2.h (right): https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/differ_vector_sse2.h:23: const uint8_t* image2); indentation https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc (right): https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:138: CompareRow(prev_block_row_start, curr_block_row_start, x_start, x_end, This function compares whole 32x32 blocks and as far as I can tell each block may be compared multiple times - once for each rectangle that intersects with that block. I think that's suboptimal and should be avoided. Another issue is that the updated_region() returned by ScreenCapturerDifferWrapper in many cases will be bigger that then region returned by the underlying capturer. I'd like to avoid this too. What I would prefer is to only compare intersection of each block with the covering rect in the source updated_region(). But there may be other solutions. Let's discuss this offline. https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:229: frame->set_capture_time_ms(frame->GetUnderlyingFrame()->capture_time_ms() + don't really need ->GetUnderlyingFrame(). The value is copied when sharing a frame. https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h (right): https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h:37: bool diff_entire_frame); Couple ideas on how everything could work without this parameter: 1. Capturers that don't know which parts of the screen have changed should set updated_region() to full frame. (I think we already have this implemented for all capturers). 2. Capturers that don't set updated_region() can be detected very easily by checking that udpated_region() is not empty in the first frame. I think (1) is enough and we can avoid (2). But feel free to keep this parameter if you think it's useful.
Hi, Sergey, I will be OOO on Thursday and Friday. I have replied one of your comment. We can discuss offline on next Monday. https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc (right): https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:138: CompareRow(prev_block_row_start, curr_block_row_start, x_start, x_end, On 2016/08/18 05:07:58, Sergey Ulanov wrote: > This function compares whole 32x32 blocks and as far as I can tell each block > may be compared multiple times - once for each rectangle that intersects with > that block. I think that's suboptimal and should be avoided. > Another issue is that the updated_region() returned by > ScreenCapturerDifferWrapper in many cases will be bigger that then region > returned by the underlying capturer. I'd like to avoid this too. > What I would prefer is to only compare intersection of each block with the > covering rect in the source updated_region(). But there may be other solutions. > Let's discuss this offline. Yes, I aware this issue, as this is a disadvantage of removing bool differ-info array. I have tried to compare exact area, by separating 32 x 32 blocks starting from top-left of a rectangle, and do not expand pixels at the right and bottom. But the performance is much worse (about twice time is used to execute the test cases, since we need to exclude comparison and random data generation time, the real perf could be 1/3 or even worse). The performance drop is coming from 1. partial block difference, 2. DesktopRegion, i.e. adding very random desktop rectangles to a DesktopRegion will cause the count of its Rows to increase significantly. But current solution ensures there are at most 24 rows for a monitor with 768 vertical resolution. I believe the impact from the latter, DesktopRegion, is even important. I have not tried several other combinations, but considering the implementation of DesktopRegion, it may have similar performance drop. I have also compared the performance between current implementation and Differ, they two have comparable performance. (Differ is slightly faster, < 5%.) I will be OOO on Thursday and Friday. Let's talk about it next Monday.
On 2016/08/18 08:50:31, Hzj_jie wrote: > Hi, Sergey, I will be OOO on Thursday and Friday. I have replied one of your > comment. We can discuss offline on next Monday. > > https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... > File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc (right): > > https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... > webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:138: > CompareRow(prev_block_row_start, curr_block_row_start, x_start, x_end, > On 2016/08/18 05:07:58, Sergey Ulanov wrote: > > This function compares whole 32x32 blocks and as far as I can tell each block > > may be compared multiple times - once for each rectangle that intersects with > > that block. I think that's suboptimal and should be avoided. > > Another issue is that the updated_region() returned by > > ScreenCapturerDifferWrapper in many cases will be bigger that then region > > returned by the underlying capturer. I'd like to avoid this too. > > What I would prefer is to only compare intersection of each block with the > > covering rect in the source updated_region(). But there may be other > solutions. > > Let's discuss this offline. > > Yes, I aware this issue, as this is a disadvantage of removing bool differ-info > array. I have tried to compare exact area, by separating 32 x 32 blocks starting > from top-left of a rectangle, and do not expand pixels at the right and bottom. > But the performance is much worse (about twice time is used to execute the test > cases, since we need to exclude comparison and random data generation time, the > real perf could be 1/3 or even worse). The performance drop is coming from 1. > partial block difference, Given that now you changed the code to use partial comparison only when rect_width!=32 this should have much smaller effect. > 2. DesktopRegion, i.e. adding very random desktop > rectangles to a DesktopRegion will cause the count of its Rows to increase > significantly. But current solution ensures there are at most 24 rows for a > monitor with 768 vertical resolution. I believe the impact from the latter, > DesktopRegion, is even important. You mentioned in your previous comment that you tried adding rects of size (w,1). Is that when you get much worse perf results? It is expected to be slow in that case, and I don't think it's the best solution here. Also keep in mind that the perf test may not be representative of what happens in real life. In either case it's quite possible that comparing whole blocks is better, but then you can still optimize it with a single bitmap to avoid comparing a block multiple times. > > I have not tried several other combinations, but considering the implementation > of DesktopRegion, it may have similar performance drop. > > I have also compared the performance between current implementation and Differ, > they two have comparable performance. (Differ is slightly faster, < 5%.) > > I will be OOO on Thursday and Friday. Let's talk about it next Monday. I'm OOO next week :-( . Can this wait till I get back?
On 2016/08/20 04:48:13, Sergey Ulanov wrote: > On 2016/08/18 08:50:31, Hzj_jie wrote: > > Hi, Sergey, I will be OOO on Thursday and Friday. I have replied one of your > > comment. We can discuss offline on next Monday. > > > > > https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... > > File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc (right): > > > > > https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... > > webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:138: > > CompareRow(prev_block_row_start, curr_block_row_start, x_start, x_end, > > On 2016/08/18 05:07:58, Sergey Ulanov wrote: > > > This function compares whole 32x32 blocks and as far as I can tell each > block > > > may be compared multiple times - once for each rectangle that intersects > with > > > that block. I think that's suboptimal and should be avoided. > > > Another issue is that the updated_region() returned by > > > ScreenCapturerDifferWrapper in many cases will be bigger that then region > > > returned by the underlying capturer. I'd like to avoid this too. > > > What I would prefer is to only compare intersection of each block with the > > > covering rect in the source updated_region(). But there may be other > > solutions. > > > Let's discuss this offline. > > > > Yes, I aware this issue, as this is a disadvantage of removing bool > differ-info > > array. I have tried to compare exact area, by separating 32 x 32 blocks > starting > > from top-left of a rectangle, and do not expand pixels at the right and > bottom. > > But the performance is much worse (about twice time is used to execute the > test > > cases, since we need to exclude comparison and random data generation time, > the > > real perf could be 1/3 or even worse). The performance drop is coming from 1. > > partial block difference, > > Given that now you changed the code to use partial comparison only when > rect_width!=32 this should have much smaller effect. > > > 2. DesktopRegion, i.e. adding very random desktop > > rectangles to a DesktopRegion will cause the count of its Rows to increase > > significantly. But current solution ensures there are at most 24 rows for a > > monitor with 768 vertical resolution. I believe the impact from the latter, > > DesktopRegion, is even important. > > You mentioned in your previous comment that you tried adding rects of size > (w,1). Is that when you get much worse perf results? It is expected to be slow > in that case, and I don't think it's the best solution here. > > Also keep in mind that the perf test may not be representative of what happens > in real life. > > In either case it's quite possible that comparing whole blocks is better, but > then you can still optimize it with a single bitmap to avoid comparing a block > multiple times. > > > > > I have not tried several other combinations, but considering the > implementation > > of DesktopRegion, it may have similar performance drop. > > > > I have also compared the performance between current implementation and > Differ, > > they two have comparable performance. (Differ is slightly faster, < 5%.) > > > > I will be OOO on Thursday and Friday. Let's talk about it next Monday. > > I'm OOO next week :-( . Can this wait till I get back? I have tried several combinations, include (w, 32) with or without a 32 pixels alignment, and also (w, 1) with and without a 32 pixels alignment. The implementations without a 32 pixels alignment have a lower performance than the ones with 32 pixels alignment. But I have not tried these combinations with a new VerticalDifference() function in this iteration. It should have a positive impact, as the sizes of updated regions are random. I am happy to use (w, 32) without 32 pixels alignment for now, as this change is much more important to DirectX capturer than others, but the performance impact (should be less than 10 ms) is still in an acceptable range, comparing to other phases in the entire end-to-end scenario. And after this change and the one to enable DirectX capturer are submitted, I can use //src/remoting/test/it2me_standalone_host.h to evaluate the impact of this and several other implementation details.
Description was changed from ========== An early analysis shows in DirectX based capturer, Windows API returns larger dirty region than the real screen change. A similar behavior may happen on other platforms with damage notification support. So it's better to have an individual layer to handle the Differ logic, and remove capturing independent logic out of each ScreenCapturer* implementation. So this change does following things, 1. Update Differ class to support CalcDirtyRegionWithHints. This function receives a const DesktopRegion& as hint, and detects only changes in the hint areas. So on platforms with damage notifications, a ScreenCapturer implementation can use the DesktopRegion from platform APIs as the hints to help improve Differ speed by reducing blocks it checks. 2. A new ScreenCapturerDifferWrapper implementation to use Differ class to help set DesktopFrame::updated_region(). It uses an underlying ScreenCapturer to do the real capture work, and updates the updated region of DesktopFrame returned from OnCaptureResult function. 3. DifferTest to cover the new CalcDirtyRegionWithHints function. 4. Improves MockScreenCapturer to make it more useful on testing ScreenCapturerDifferWrapper. Now MockScreenCapturer supports to return controllable DesktopFrames. Users can set various flags to control its behavior. 5. ScreenCapturerDifferWrapperTest. After this change, we can eventually remove all Differ logic from ScreenCapturer* implementations, and fix a potential crash bug in ScreenCapturerLinux class. It wrongly assumes previous_frame() has a same size as current_frame(). https://goo.gl/3nSqOC This is not a huge change, 559 out of 821 lines are unit test related code. So this change contains only 262 lines of production code. A coming change is to add another ScreenCapturerExpendToGridWrapper to cover the functionality of ScreenCapturerHelper. BUG=633802 ========== to ========== An early analysis shows in DirectX based capturer, Windows API returns larger dirty region than the real screen change. A similar behavior may happen on other platforms with damage notification support. So it's better to have an individual layer to handle the Differ logic, and remove capturing independent logic out of each ScreenCapturer* implementation. So this change does following things, 1. Update differ_block to handle variable height. differ_block_sse2 has been renamed to differ_vector_sse2. 2. A new ScreenCapturerDifferWrapper implementation to help set DesktopFrame::updated_region(). It uses an underlying ScreenCapturer to do the real capture work, and updates the updated region of DesktopFrame returned from OnCaptureResult function. 3. FakeDesktopCapturer and FakeScreenCapturer to generate controllable DesktopFrame by using DesktopFrameGenerator and DesktopFramePainter. 4. Test ScreenCapturerDifferWrapper by using FakeScreenCapturer. After this change, we can eventually remove all Differ logic from ScreenCapturer* implementations, and fix a potential crash bug in ScreenCapturerLinux class. It wrongly assumes previous_frame() has a same size as current_frame(). https://goo.gl/3nSqOC BUG=633802 ==========
Hi, Jamie, would you mind to have a look at this change during Sergey's absence? https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_frame_generator.cc (right): https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame_generator.cc:46: rect.bottom() + random.Rand(enlarge_range)); On 2016/08/18 05:07:58, Sergey Ulanov wrote: > There are several other places where we need the same operation. Maybe add > Extend() in DesktopRect? Done. https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame_generator.cc:75: memcpy(&frame->data()[row + column], &color, sizeof(color)); On 2016/08/18 05:07:58, Sergey Ulanov wrote: > Instead of calling memcpy() for every pixel it would be better to get uint32_t > and use simple assignment. E.g. see DrawRect() here: > https://codesearch.chromium.org/chromium/src/remoting/test/frame_generator_ut... Done. https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame_generator.cc:120: } else { On 2016/08/18 05:07:58, Sergey Ulanov wrote: > Don't use else after return. Best to move the !return_frame_ case on top. Done. https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_frame_generator.h (right): https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame_generator.h:29: virtual std::unique_ptr<DesktopFrame> operator()( On 2016/08/18 05:07:58, Sergey Ulanov wrote: > style guide discourages operator overloading. > https://google.github.io/styleguide/cppguide.html#Operator_Overloading . > Operator() specifically makes the calling code rather confusing, and there is no > good reason you need it here. Call this method GetNextFrame()? Done. https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame_generator.h:37: class BaseDesktopFrameGenerator : public DesktopFrameGenerator { On 2016/08/18 05:07:58, Sergey Ulanov wrote: > I don't think you really need inheritance here. Style guide says that > composition should be preferred: > https://google.github.io/styleguide/cppguide.html#Inheritance > In this case this class can simply take a pointer or rtc::Callback<> to the > Paint() function I think you must mean the BlackWhiteDesktopFrameGenerator. I have changed it to BlackWhiteDesktopFramePainter. https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/differ_vector_sse2.h (right): https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/differ_vector_sse2.h:23: const uint8_t* image2); On 2016/08/18 05:07:58, Sergey Ulanov wrote: > indentation Done. https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc (right): https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:138: CompareRow(prev_block_row_start, curr_block_row_start, x_start, x_end, On 2016/08/18 08:50:31, Hzj_jie wrote: > On 2016/08/18 05:07:58, Sergey Ulanov wrote: > > This function compares whole 32x32 blocks and as far as I can tell each block > > may be compared multiple times - once for each rectangle that intersects with > > that block. I think that's suboptimal and should be avoided. > > Another issue is that the updated_region() returned by > > ScreenCapturerDifferWrapper in many cases will be bigger that then region > > returned by the underlying capturer. I'd like to avoid this too. > > What I would prefer is to only compare intersection of each block with the > > covering rect in the source updated_region(). But there may be other > solutions. > > Let's discuss this offline. > > Yes, I aware this issue, as this is a disadvantage of removing bool differ-info > array. I have tried to compare exact area, by separating 32 x 32 blocks starting > from top-left of a rectangle, and do not expand pixels at the right and bottom. > But the performance is much worse (about twice time is used to execute the test > cases, since we need to exclude comparison and random data generation time, the > real perf could be 1/3 or even worse). The performance drop is coming from 1. > partial block difference, 2. DesktopRegion, i.e. adding very random desktop > rectangles to a DesktopRegion will cause the count of its Rows to increase > significantly. But current solution ensures there are at most 24 rows for a > monitor with 768 vertical resolution. I believe the impact from the latter, > DesktopRegion, is even important. > > I have not tried several other combinations, but considering the implementation > of DesktopRegion, it may have similar performance drop. > > I have also compared the performance between current implementation and Differ, > they two have comparable performance. (Differ is slightly faster, < 5%.) > > I will be OOO on Thursday and Friday. Let's talk about it next Monday. After reducing the range of several random values (say, from 500 to 50 updated rectangles), the performance of the new implementation (not 32 pixels alignment) is much better, but no consistent improvement has been found by using Differ. I believe it's because less DesktopRegion rows will be added. And by using Differ, it should have a consistent behavior no matter - how many updated regions returned, - how much overlap between the updated regions returned. The performance is too OS implementation dependent, as we have no idea how OS returns updated region. I am now using an exact area without 32 pixels alignment as you suggested. We can revise this decision on each platform later. https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:229: frame->set_capture_time_ms(frame->GetUnderlyingFrame()->capture_time_ms() + On 2016/08/18 05:07:58, Sergey Ulanov wrote: > don't really need ->GetUnderlyingFrame(). The value is copied when sharing a > frame. This SharedDesktopFrame is generated above @ line 195, so the underlying ScreenCapturer won't set the capture_time_ms to the SharedDesktopFrame, but |input_frame|. But |input_frame| has been moved also in line 195, so we can only use this GetUnderlyingFrame() to access the original capture_time_ms(). https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h (right): https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h:37: bool diff_entire_frame); On 2016/08/18 05:07:58, Sergey Ulanov wrote: > Couple ideas on how everything could work without this parameter: > 1. Capturers that don't know which parts of the screen have changed should set > updated_region() to full frame. (I think we already have this implemented for > all capturers). > 2. Capturers that don't set updated_region() can be detected very easily by > checking that udpated_region() is not empty in the first frame. > > I think (1) is enough and we can avoid (2). But feel free to keep this parameter > if you think it's useful. Thank you for raising this issue again, I have not considered it carefully in last review iteration. The definition of updated_region() function requires a capturer to mark a superset of all the updated regions it returns in the DesktopFrame. So I agree (1) should be enough.
https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_frame_generator.cc (right): https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame_generator.cc:75: memcpy(&frame->data()[row + column], &color, sizeof(color)); On 2016/08/23 00:54:58, Hzj_jie wrote: > On 2016/08/18 05:07:58, Sergey Ulanov wrote: > > Instead of calling memcpy() for every pixel it would be better to get uint32_t > > and use simple assignment. E.g. see DrawRect() here: > > > https://codesearch.chromium.org/chromium/src/remoting/test/frame_generator_ut... > > Done. Seems both memcpy and uint32_t assignment will be impacted by big-ending and little-ending, i.e. the color here is "ARGB" in little-ending but "BGRA" in big-ending. As we always expect the order in uint8_t array is "BGRA". Though there is no impact for this test case, as UINT32_MAX is used to represent a color, it is still a trouble if once we need a complex color instead of white / black. I would like to add a Color structure in desktop_capture folder to resolve this issue. Since it is used in another change 2268093002, I will keep this change as this for now, and use the Color structure in a coming change.
Patchset #8 (id:280001) has been deleted
Patchset #8 (id:300001) has been deleted
https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_frame_generator.cc (right): https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame_generator.cc:75: memcpy(&frame->data()[row + column], &color, sizeof(color)); On 2016/08/25 18:20:12, Hzj_jie wrote: > On 2016/08/23 00:54:58, Hzj_jie wrote: > > On 2016/08/18 05:07:58, Sergey Ulanov wrote: > > > Instead of calling memcpy() for every pixel it would be better to get > uint32_t > > > and use simple assignment. E.g. see DrawRect() here: > > > > > > https://codesearch.chromium.org/chromium/src/remoting/test/frame_generator_ut... > > > > Done. > > Seems both memcpy and uint32_t assignment will be impacted by big-ending and > little-ending, i.e. the color here is "ARGB" in little-ending but "BGRA" in > big-ending. As we always expect the order in uint8_t array is "BGRA". Though > there is no impact for this test case, as UINT32_MAX is used to represent a > color, it is still a trouble if once we need a complex color instead of white / > black. I would like to add a Color structure in desktop_capture folder to > resolve this issue. Since it is used in another change 2268093002, I will keep > this change as this for now, and use the Color structure in a coming change. Simple assignment will still be faster and clearer here, you'd just need to convert Color to uint8_t in the beginning of this function https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/fake_screen_capturer.cc (right): https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/fake_screen_capturer.cc:22: id_(static_cast<ScreenId>(reinterpret_cast<intptr_t>(this))) {} just hard code some value. I don't think we really need it to be random. https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc (right): https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:25: static const int kBlockXOffset = kBlockSize * DesktopFrame::kBytesPerPixel; nit: this const is used in only 2 places, so I don't think you really need it. https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:35: RTC_DCHECK(width < kBlockSize); RTC_DCHECK_LT https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:56: const int last_block_width, Instead of passing block_count and last_block_width here it would be better to pass width and calculate number of blocks here. https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:77: } else if (first_dirty_x_block != -1) { I don't think it's worth to merge the blocks horizontally here. Just add them to the Region one by one and let it handle this case - that would simplify this code significantly. https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:90: if (last_block_width < kBlockSize) { Why would we want last_block_width = kBlockSize? if width is multiple of 32 then last_block_width should be 0, and we just ignore it here https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:125: const int x_block_count = (rect.width() - 1) / kBlockSize; Don't need -1 here https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:127: const int y_block_count = (rect.height() - 1) / kBlockSize; don't need -1 here https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper_unittest.cc (right): https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper_unittest.cc:249: #if defined(WEBRTC_ARCH_X86_FAMILY) Do we still need this given that the tests are disabled? Also remove the SSE check in the tests
Patchset #9 (id:340001) has been deleted
https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_frame_generator.cc (right): https://codereview.chromium.org/2202443002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame_generator.cc:75: memcpy(&frame->data()[row + column], &color, sizeof(color)); On 2016/09/01 19:27:56, Sergey Ulanov wrote: > On 2016/08/25 18:20:12, Hzj_jie wrote: > > On 2016/08/23 00:54:58, Hzj_jie wrote: > > > On 2016/08/18 05:07:58, Sergey Ulanov wrote: > > > > Instead of calling memcpy() for every pixel it would be better to get > > uint32_t > > > > and use simple assignment. E.g. see DrawRect() here: > > > > > > > > > > https://codesearch.chromium.org/chromium/src/remoting/test/frame_generator_ut... > > > > > > Done. > > > > Seems both memcpy and uint32_t assignment will be impacted by big-ending and > > little-ending, i.e. the color here is "ARGB" in little-ending but "BGRA" in > > big-ending. As we always expect the order in uint8_t array is "BGRA". Though > > there is no impact for this test case, as UINT32_MAX is used to represent a > > color, it is still a trouble if once we need a complex color instead of white > / > > black. I would like to add a Color structure in desktop_capture folder to > > resolve this issue. Since it is used in another change 2268093002, I will keep > > this change as this for now, and use the Color structure in a coming change. > > Simple assignment will still be faster and clearer here, you'd just need to > convert Color to uint8_t in the beginning of this function Yes, it will have a ToUInt32() function to handle the ordering correctly. https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/fake_screen_capturer.cc (right): https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/fake_screen_capturer.cc:22: id_(static_cast<ScreenId>(reinterpret_cast<intptr_t>(this))) {} On 2016/09/01 19:27:56, Sergey Ulanov wrote: > just hard code some value. I don't think we really need it to be random. Done. https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc (right): https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:25: static const int kBlockXOffset = kBlockSize * DesktopFrame::kBytesPerPixel; On 2016/09/01 19:27:56, Sergey Ulanov wrote: > nit: this const is used in only 2 places, so I don't think you really need it. Done. https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:35: RTC_DCHECK(width < kBlockSize); On 2016/09/01 19:27:56, Sergey Ulanov wrote: > RTC_DCHECK_LT Done. https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:56: const int last_block_width, On 2016/09/01 19:27:56, Sergey Ulanov wrote: > Instead of passing block_count and last_block_width here it would be better to > pass width and calculate number of blocks here. I believe besides the functionality, the first priority of this class is performance. Both block_count and last_block_width won't change in the for loop in line 138. So we can save the redundant calculating here. https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:77: } else if (first_dirty_x_block != -1) { On 2016/09/01 19:27:56, Sergey Ulanov wrote: > I don't think it's worth to merge the blocks horizontally here. Just add them to > the Region one by one and let it handle this case - that would simplify this > code significantly. By using this merging logic, we can save at least 80% time for each frame. The time to execute test case can be significantly increased from 6s to 10s by removing the merge logic. This should be expected, as DesktopRegion will merge the rectangle by row. The merging logic here can help to reduce the complexity from O(n * log(n) * log(n)) to O(log(n) * log(n)). And though the test cases may not consistent with real world, the real world may be even worse. As the perf drops more with larger rectangles. https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:90: if (last_block_width < kBlockSize) { On 2016/09/01 19:27:56, Sergey Ulanov wrote: > Why would we want last_block_width = kBlockSize? if width is multiple of 32 then > last_block_width should be 0, and we just ignore it here It relates to the merging logic above, we always need to handle the last block differently. i.e. Adding last dirty area into |output|. So we expect a > 0 last_block_width. https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:125: const int x_block_count = (rect.width() - 1) / kBlockSize; On 2016/09/01 19:27:56, Sergey Ulanov wrote: > Don't need -1 here This relates to the merging logic in CompareRow. https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:127: const int y_block_count = (rect.height() - 1) / kBlockSize; On 2016/09/01 19:27:56, Sergey Ulanov wrote: > don't need -1 here By -1 here, and adding a CompareRow in 146, we can save bool comparison in the for loop in line 138. https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper_unittest.cc (right): https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper_unittest.cc:249: #if defined(WEBRTC_ARCH_X86_FAMILY) On 2016/09/01 19:27:56, Sergey Ulanov wrote: > Do we still need this given that the tests are disabled? > Also remove the SSE check in the tests Ah, yes, removed.
LGTM when my comments are addressed https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc (right): https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:56: const int last_block_width, On 2016/09/01 23:26:43, Hzj_jie wrote: > On 2016/09/01 19:27:56, Sergey Ulanov wrote: > > Instead of passing block_count and last_block_width here it would be better > to > > pass width and calculate number of blocks here. > > I believe besides the functionality, the first priority of this class is > performance. Both block_count and last_block_width won't change in the for loop > in line 138. So we can save the redundant calculating here. You also calculate column_right below, so I don't think this optimization can actually have any effect. In either case any potential effect of this perf optimization is negligible, so it's better to make this code more readable. https://codereview.chromium.org/2202443002/diff/360001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc (right): https://codereview.chromium.org/2202443002/diff/360001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:98: if (first_dirty_x_block == -1) { if (first_dirty_x_block == -1) first_dirty_x_block = block_count; and then dedup the AddRect() call https://codereview.chromium.org/2202443002/diff/360001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:193: if (result == Result::SUCCESS) { I don't think you need this. If input_frame != null then it's expected that result == SUCCESS. Replace this with a DCHECK?
https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc (right): https://codereview.chromium.org/2202443002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:56: const int last_block_width, On 2016/09/02 19:23:03, Sergey Ulanov wrote: > On 2016/09/01 23:26:43, Hzj_jie wrote: > > On 2016/09/01 19:27:56, Sergey Ulanov wrote: > > > Instead of passing block_count and last_block_width here it would be better > > to > > > pass width and calculate number of blocks here. > > > > I believe besides the functionality, the first priority of this class is > > performance. Both block_count and last_block_width won't change in the for > loop > > in line 138. So we can save the redundant calculating here. > > You also calculate column_right below, so I don't think this optimization can > actually have any effect. In either case any potential effect of this perf > optimization is negligible, so it's better to make this code more readable. Done. https://codereview.chromium.org/2202443002/diff/360001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc (right): https://codereview.chromium.org/2202443002/diff/360001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:98: if (first_dirty_x_block == -1) { On 2016/09/02 19:23:03, Sergey Ulanov wrote: > if (first_dirty_x_block == -1) > first_dirty_x_block = block_count; > and then dedup the AddRect() call Done. https://codereview.chromium.org/2202443002/diff/360001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.cc:193: if (result == Result::SUCCESS) { On 2016/09/02 19:23:03, Sergey Ulanov wrote: > I don't think you need this. If input_frame != null then it's expected that > result == SUCCESS. Replace this with a DCHECK? Done.
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.chromium.org/2202443002/#ps400001 (title: "Sync latest changes")
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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/8061)
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.chromium.org/2202443002/#ps420001 (title: "Sync latest changes")
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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/8065)
Description was changed from ========== An early analysis shows in DirectX based capturer, Windows API returns larger dirty region than the real screen change. A similar behavior may happen on other platforms with damage notification support. So it's better to have an individual layer to handle the Differ logic, and remove capturing independent logic out of each ScreenCapturer* implementation. So this change does following things, 1. Update differ_block to handle variable height. differ_block_sse2 has been renamed to differ_vector_sse2. 2. A new ScreenCapturerDifferWrapper implementation to help set DesktopFrame::updated_region(). It uses an underlying ScreenCapturer to do the real capture work, and updates the updated region of DesktopFrame returned from OnCaptureResult function. 3. FakeDesktopCapturer and FakeScreenCapturer to generate controllable DesktopFrame by using DesktopFrameGenerator and DesktopFramePainter. 4. Test ScreenCapturerDifferWrapper by using FakeScreenCapturer. After this change, we can eventually remove all Differ logic from ScreenCapturer* implementations, and fix a potential crash bug in ScreenCapturerLinux class. It wrongly assumes previous_frame() has a same size as current_frame(). https://goo.gl/3nSqOC BUG=633802 ========== to ========== An early analysis shows in DirectX based capturer, Windows API returns larger dirty region than the real screen change. A similar behavior may happen on other platforms with damage notification support. So it's better to have an individual layer to handle the Differ logic, and remove capturing independent logic out of each ScreenCapturer* implementation. So this change does following things, 1. Update differ_block to handle variable height. differ_block_sse2 has been renamed to differ_vector_sse2. 2. A new ScreenCapturerDifferWrapper implementation to help set DesktopFrame::updated_region(). It uses an underlying ScreenCapturer to do the real capture work, and updates the updated region of DesktopFrame returned from OnCaptureResult function. 3. FakeDesktopCapturer and FakeScreenCapturer to generate controllable DesktopFrame by using DesktopFrameGenerator and DesktopFramePainter. 4. Test ScreenCapturerDifferWrapper by using FakeScreenCapturer. After this change, we can eventually remove all Differ logic from ScreenCapturer* implementations, and fix a potential crash bug in ScreenCapturerLinux class. It wrongly assumes previous_frame() has a same size as current_frame(). https://goo.gl/3nSqOC BUG=633802 TBR=kjellander@webrtc.org ==========
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/...
Message was sent while issue was closed.
Description was changed from ========== An early analysis shows in DirectX based capturer, Windows API returns larger dirty region than the real screen change. A similar behavior may happen on other platforms with damage notification support. So it's better to have an individual layer to handle the Differ logic, and remove capturing independent logic out of each ScreenCapturer* implementation. So this change does following things, 1. Update differ_block to handle variable height. differ_block_sse2 has been renamed to differ_vector_sse2. 2. A new ScreenCapturerDifferWrapper implementation to help set DesktopFrame::updated_region(). It uses an underlying ScreenCapturer to do the real capture work, and updates the updated region of DesktopFrame returned from OnCaptureResult function. 3. FakeDesktopCapturer and FakeScreenCapturer to generate controllable DesktopFrame by using DesktopFrameGenerator and DesktopFramePainter. 4. Test ScreenCapturerDifferWrapper by using FakeScreenCapturer. After this change, we can eventually remove all Differ logic from ScreenCapturer* implementations, and fix a potential crash bug in ScreenCapturerLinux class. It wrongly assumes previous_frame() has a same size as current_frame(). https://goo.gl/3nSqOC BUG=633802 TBR=kjellander@webrtc.org ========== to ========== An early analysis shows in DirectX based capturer, Windows API returns larger dirty region than the real screen change. A similar behavior may happen on other platforms with damage notification support. So it's better to have an individual layer to handle the Differ logic, and remove capturing independent logic out of each ScreenCapturer* implementation. So this change does following things, 1. Update differ_block to handle variable height. differ_block_sse2 has been renamed to differ_vector_sse2. 2. A new ScreenCapturerDifferWrapper implementation to help set DesktopFrame::updated_region(). It uses an underlying ScreenCapturer to do the real capture work, and updates the updated region of DesktopFrame returned from OnCaptureResult function. 3. FakeDesktopCapturer and FakeScreenCapturer to generate controllable DesktopFrame by using DesktopFrameGenerator and DesktopFramePainter. 4. Test ScreenCapturerDifferWrapper by using FakeScreenCapturer. After this change, we can eventually remove all Differ logic from ScreenCapturer* implementations, and fix a potential crash bug in ScreenCapturerLinux class. It wrongly assumes previous_frame() has a same size as current_frame(). https://goo.gl/3nSqOC BUG=633802 TBR=kjellander@webrtc.org ==========
Message was sent while issue was closed.
Committed patchset #12 (id:420001)
Message was sent while issue was closed.
Description was changed from ========== An early analysis shows in DirectX based capturer, Windows API returns larger dirty region than the real screen change. A similar behavior may happen on other platforms with damage notification support. So it's better to have an individual layer to handle the Differ logic, and remove capturing independent logic out of each ScreenCapturer* implementation. So this change does following things, 1. Update differ_block to handle variable height. differ_block_sse2 has been renamed to differ_vector_sse2. 2. A new ScreenCapturerDifferWrapper implementation to help set DesktopFrame::updated_region(). It uses an underlying ScreenCapturer to do the real capture work, and updates the updated region of DesktopFrame returned from OnCaptureResult function. 3. FakeDesktopCapturer and FakeScreenCapturer to generate controllable DesktopFrame by using DesktopFrameGenerator and DesktopFramePainter. 4. Test ScreenCapturerDifferWrapper by using FakeScreenCapturer. After this change, we can eventually remove all Differ logic from ScreenCapturer* implementations, and fix a potential crash bug in ScreenCapturerLinux class. It wrongly assumes previous_frame() has a same size as current_frame(). https://goo.gl/3nSqOC BUG=633802 TBR=kjellander@webrtc.org ========== to ========== An early analysis shows in DirectX based capturer, Windows API returns larger dirty region than the real screen change. A similar behavior may happen on other platforms with damage notification support. So it's better to have an individual layer to handle the Differ logic, and remove capturing independent logic out of each ScreenCapturer* implementation. So this change does following things, 1. Update differ_block to handle variable height. differ_block_sse2 has been renamed to differ_vector_sse2. 2. A new ScreenCapturerDifferWrapper implementation to help set DesktopFrame::updated_region(). It uses an underlying ScreenCapturer to do the real capture work, and updates the updated region of DesktopFrame returned from OnCaptureResult function. 3. FakeDesktopCapturer and FakeScreenCapturer to generate controllable DesktopFrame by using DesktopFrameGenerator and DesktopFramePainter. 4. Test ScreenCapturerDifferWrapper by using FakeScreenCapturer. After this change, we can eventually remove all Differ logic from ScreenCapturer* implementations, and fix a potential crash bug in ScreenCapturerLinux class. It wrongly assumes previous_frame() has a same size as current_frame(). https://goo.gl/3nSqOC BUG=633802 TBR=kjellander@webrtc.org Committed: https://crrev.com/fef8653c5adcd7c5b7fc2c49ae1a0f6927ddb37f Cr-Commit-Position: refs/heads/master@{#14076} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/fef8653c5adcd7c5b7fc2c49ae1a0f6927ddb37f Cr-Commit-Position: refs/heads/master@{#14076} |
