Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2908853002:
desktopCapture: scale the cursor image according to screen scale factor on OSX (Closed)
DescriptiondesktopCapture: scale the cursor image according to screen scale factor on OSX
Before 10.12, OSX may report 1X cursor on Retina screen. (See crbug.com/632995.)
After 10.12, OSX may report 2X cursor on non-Retina screen. (See
crbug.com/671436.) So scaling the cursor if the image size doesn't meet the
expected size on either Retina or non-Retina screen.
Also corrects the cursor caching and change detection, so we can only do scalingat cursor changing for better performance.
As to screen capture on OSX, the captured frame already contains the current
cursor. So the MouseCursorMonitorMac is not needed for ScreenCapture for
performance purpose.
BUG=671436
Review-Url: https://codereview.webrtc.org/2908853002
Cr-Commit-Position: refs/heads/master@{#18393}
Committed: https://chromium.googlesource.com/external/webrtc/+/0d1e27f00fd6bf7aeae7cb19845b8cd1ed4a40c1
Patch Set 1 #
Total comments: 6
Patch Set 2 : correct the issue of memory management #
Total comments: 5
Patch Set 3 : address comments #
Total comments: 18
Patch Set 4 : address comments #
Total comments: 2
Patch Set 5 : run git cl format #Messages
Total messages: 49 (33 generated)
The CQ bit was checked by braveyao@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
braveyao@webrtc.org changed reviewers: + sergeyu@chromium.org
Hi, sergeyu@, done the stuff discussed in offline thread. Please take a look.
erikchen@chromium.org changed reviewers: + erikchen@chromium.org
https://codereview.webrtc.org/2908853002/diff/1/webrtc/modules/desktop_captur... File webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm (right): https://codereview.webrtc.org/2908853002/diff/1/webrtc/modules/desktop_captur... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:33: CGImageRef ResizeCGImage(CGImageRef image, int width, int height) { please use scoped_cftyperefs everywhere. It prevents memory leaks. :) https://codereview.webrtc.org/2908853002/diff/1/webrtc/modules/desktop_captur... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:82: NSImage* last_cursor_; use a scoped_nsobject. You're not retaining this object. Also, it would be far more efficient to retain the NSCursor and check for equality there.
Thanks erik@ for the suggestions and off-line talk. Unfortunately not all chromium methods are available in WebRTC. Maybe we should recommend those to WebRTC later. Any other concern/suggestion? https://codereview.webrtc.org/2908853002/diff/1/webrtc/modules/desktop_captur... File webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm (right): https://codereview.webrtc.org/2908853002/diff/1/webrtc/modules/desktop_captur... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:33: CGImageRef ResizeCGImage(CGImageRef image, int width, int height) { On 2017/05/26 23:09:04, erikchen wrote: > please use scoped_cftyperefs everywhere. It prevents memory leaks. :) scoped_cftyperefs is not available in WebRTC yet. https://codereview.webrtc.org/2908853002/diff/1/webrtc/modules/desktop_captur... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:82: NSImage* last_cursor_; On 2017/05/26 23:09:04, erikchen wrote: > use a scoped_nsobject. You're not retaining this object. > > Also, it would be far more efficient to retain the NSCursor and check for > equality there. scoped_nsobject is not available in WebRTC yet. "retain the NSCursor and check for equality" doesn't work. I encountered the same problem as in [1] and switched to NSImage TIFFRepresentation comparison. [1]: https://stackoverflow.com/questions/8198380/how-to-get-current-type-of-mouse-...
https://codereview.webrtc.org/2908853002/diff/1/webrtc/modules/desktop_captur... File webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm (right): https://codereview.webrtc.org/2908853002/diff/1/webrtc/modules/desktop_captur... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:44: CGColorSpaceRelease(colorspace); Please read through https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Me... and fix your CL to correctly manage references to objects. :) [e.g. this release is incorrect. Please go through and try to fix your own CL before asking me to take a look].
The CQ bit was checked by braveyao@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...)
The CQ bit was checked by braveyao@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...)
Yes I found more discussion about the scaling method and corrected my cl accordingly. Sorry for my negligence. https://codereview.webrtc.org/2908853002/diff/1/webrtc/modules/desktop_captur... File webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm (right): https://codereview.webrtc.org/2908853002/diff/1/webrtc/modules/desktop_captur... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:44: CGColorSpaceRelease(colorspace); On 2017/05/30 17:27:46, erikchen wrote: > Please read through > https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Me... > > and fix your CL to correctly manage references to objects. :) > [e.g. this release is incorrect. Please go through and try to fix your own CL > before asking me to take a look]. Done. It's weird that releasing the color space here doesn't cause any problem. I searched around and found the discussion that the release here is not necessary. Also found the discussion that the CGImageRef returned by this method should be manually released. Tried a fix below. Please have a check.
https://codereview.webrtc.org/2908853002/diff/20001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm (right): https://codereview.webrtc.org/2908853002/diff/20001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:264: last_cursor_ = nsimage; You need to retain last_cursor_, and also release the last one before overriding it. You need to override the destructor to release last_cursor_ as well. https://codereview.webrtc.org/2908853002/diff/20001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:285: CreateScaledCGImage(cg_image, size.width(), size.height()); every early return e.g. 293, 299, etc. needs to release this.
https://codereview.webrtc.org/2908853002/diff/20001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm (right): https://codereview.webrtc.org/2908853002/diff/20001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:264: last_cursor_ = nsimage; On 2017/05/30 22:34:31, erikchen wrote: > You need to retain last_cursor_, and also release the last one before overriding > it. > > You need to override the destructor to release last_cursor_ as well. ah, this is arc code. You should modify line 81 to be: __strong NSImage* last_cursor_;
The CQ bit was checked by braveyao@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi erikchen@, thanks so much for the help and all the documents shared. Always good to learn something new! PTAL. https://codereview.webrtc.org/2908853002/diff/20001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm (right): https://codereview.webrtc.org/2908853002/diff/20001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:264: last_cursor_ = nsimage; On 2017/05/30 23:18:27, erikchen wrote: > On 2017/05/30 22:34:31, erikchen wrote: > > You need to retain last_cursor_, and also release the last one before > overriding > > it. > > > > You need to override the destructor to release last_cursor_ as well. > > ah, this is arc code. You should modify line 81 to be: > __strong NSImage* last_cursor_; Done. Really good to know! https://codereview.webrtc.org/2908853002/diff/20001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:285: CreateScaledCGImage(cg_image, size.width(), size.height()); On 2017/05/30 22:34:31, erikchen wrote: > every early return e.g. 293, 299, etc. needs to release this. Done. Sorry for forgetting these.
lgtm
sergeyu@google.com changed reviewers: + sergeyu@google.com
https://codereview.webrtc.org/2908853002/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm (right): https://codereview.webrtc.org/2908853002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:37: CGBitmapContextCreate(NULL, s/NULL/nullptr/ https://codereview.webrtc.org/2908853002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:38: width, Please run 'git cl format'. It should format this code differently, particularly it would put multiple arguments per line. https://codereview.webrtc.org/2908853002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:41: width * 4, width * DesktopFrame::kBytesPerPixel; https://codereview.webrtc.org/2908853002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:45: if (context == NULL) return nil; if (!context) return nil; https://codereview.webrtc.org/2908853002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:47: // draw image to context (resizing it) Please reformat this as follows: // Draw image to the context, resizing it. Note capital D and period at the end. https://codereview.webrtc.org/2908853002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:49: // extract resulting image from context // Extract resulting image from context. https://codereview.webrtc.org/2908853002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:262: if ([[nsimage TIFFRepresentation] isEqual: [last_cursor_ TIFFRepresentation]]) Add a comment, to make it clear clear what happens here. https://codereview.webrtc.org/2908853002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:329: // ScreenCapture already contains current cursor in the captured frame on OSX. This is not the right place to fix this. This class is used in chromoting (see https://codesearch.chromium.org/chromium/src/remoting/host/mouse_cursor_monit... ) and we want to keep it working there. I suggest adding a separate ifdef in content/browser/media/capture/desktop_capture_device.cc
The CQ bit was checked by braveyao@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sergeyu@, thanks for the comments. All addressed/answered. PTAL. https://codereview.webrtc.org/2908853002/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm (right): https://codereview.webrtc.org/2908853002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:37: CGBitmapContextCreate(NULL, On 2017/06/01 00:22:00, Do not use (sergeyu) wrote: > s/NULL/nullptr/ Done. https://codereview.webrtc.org/2908853002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:38: width, On 2017/06/01 00:22:00, Do not use (sergeyu) wrote: > Please run 'git cl format'. It should format this code differently, particularly > it would put multiple arguments per line. Actually I tried to run "git cl format" before filing this cl. It's weird that it formats the mm file into 100 char line length. So I have to give it up. IIRC, it works under chromium though... And here one argument per line is from that cl format result. https://codereview.webrtc.org/2908853002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:41: width * 4, On 2017/06/01 00:22:00, Do not use (sergeyu) wrote: > width * DesktopFrame::kBytesPerPixel; Done. https://codereview.webrtc.org/2908853002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:45: if (context == NULL) return nil; On 2017/06/01 00:21:59, Do not use (sergeyu) wrote: > if (!context) > return nil; Done. https://codereview.webrtc.org/2908853002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:47: // draw image to context (resizing it) On 2017/06/01 00:21:59, Do not use (sergeyu) wrote: > Please reformat this as follows: > // Draw image to the context, resizing it. > Note capital D and period at the end. Done. https://codereview.webrtc.org/2908853002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:49: // extract resulting image from context On 2017/06/01 00:21:59, Do not use (sergeyu) wrote: > // Extract resulting image from context. Done. https://codereview.webrtc.org/2908853002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:262: if ([[nsimage TIFFRepresentation] isEqual: [last_cursor_ TIFFRepresentation]]) On 2017/06/01 00:21:59, Do not use (sergeyu) wrote: > Add a comment, to make it clear clear what happens here. Done. https://codereview.webrtc.org/2908853002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:329: // ScreenCapture already contains current cursor in the captured frame on OSX. On 2017/06/01 00:21:59, Do not use (sergeyu) wrote: > This is not the right place to fix this. This class is used in chromoting (see > https://codesearch.chromium.org/chromium/src/remoting/host/mouse_cursor_monit... > ) and we want to keep it working there. > > I suggest adding a separate ifdef in > content/browser/media/capture/desktop_capture_device.cc OK. Then a separated cl is needed. This is under WebRTC depository. BTW: it seems that mouse_cursor_monitor_proxy.cc can handle NULL MouseCursorMonitor. An why we need to draw the cursor again in chromoting?
https://codereview.webrtc.org/2908853002/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm (right): https://codereview.webrtc.org/2908853002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:38: width, On 2017/06/01 17:50:14, braveyao1 wrote: > On 2017/06/01 00:22:00, Do not use (sergeyu) wrote: > > Please run 'git cl format'. It should format this code differently, > particularly > > it would put multiple arguments per line. > > Actually I tried to run "git cl format" before filing this cl. It's weird that > it formats the mm file into 100 char line length. So I have to give it up. That's by design. Style guide for ObjC allows 100-char lines (see https://google.github.io/styleguide/objcguide.xml#Line_Length) and clang-format is configured to use 100-char lines for ObjC (see .clang-format in the root of the webrtc repo). > IIRC, > it works under chromium though... > And here one argument per line is from that cl format result. https://codereview.webrtc.org/2908853002/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm (right): https://codereview.webrtc.org/2908853002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:34: // create context, keeping original image properties Capital C please, add '.' at the end.
The CQ bit was checked by braveyao@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Lost in all the style guides... https://chromium.googlesource.com/chromium/src/+/master/styleguide/objective-... says it should be 80 char. Anyway, ran the 'git cl format' and PTAL. https://codereview.webrtc.org/2908853002/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm (right): https://codereview.webrtc.org/2908853002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:38: width, On 2017/06/01 18:08:08, Do not use (sergeyu) wrote: > On 2017/06/01 17:50:14, braveyao1 wrote: > > On 2017/06/01 00:22:00, Do not use (sergeyu) wrote: > > > Please run 'git cl format'. It should format this code differently, > > particularly > > > it would put multiple arguments per line. > > > > Actually I tried to run "git cl format" before filing this cl. It's weird that > > it formats the mm file into 100 char line length. So I have to give it up. > > That's by design. Style guide for ObjC allows 100-char lines (see > https://google.github.io/styleguide/objcguide.xml#Line_Length) and clang-format > is configured to use 100-char lines for ObjC (see .clang-format in the root of > the webrtc repo). > > > > IIRC, > > it works under chromium though... > > And here one argument per line is from that cl format result. > > Done. I think 'git cl format' won't be wrong. :) But https://chromium.googlesource.com/chromium/src/+/master/styleguide/objective-... says it should be 80 char. So confused... https://codereview.webrtc.org/2908853002/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm (right): https://codereview.webrtc.org/2908853002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:34: // create context, keeping original image properties On 2017/06/01 18:08:08, Do not use (sergeyu) wrote: > Capital C please, add '.' at the end. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Description was changed from ========== desktopCapture: scale the cursor image according to screen scale factor on OSX Before 10.12, OSX may report 1X cursor on Retina screen. (See crbug.com/632995.) After 10.12, OSX may report 2X cursor on non-Retina screen. (See crbug.com/671436.) So scaling the cursor if the image size doesn't meet the expected size on either Retina or non-Retina screen. Also corrects the cursor caching and change detection, so we can only do scalingat cursor changing for better performance. As to screen capture on OSX, the captured frame already contains the current cursor. So the MouseCursorMonitorMac is not needed for ScreenCapture for performance purpose. BUG=671436 ========== to ========== desktopCapture: scale the cursor image according to screen scale factor on OSX Before 10.12, OSX may report 1X cursor on Retina screen. (See crbug.com/632995.) After 10.12, OSX may report 2X cursor on non-Retina screen. (See crbug.com/671436.) So scaling the cursor if the image size doesn't meet the expected size on either Retina or non-Retina screen. Also corrects the cursor caching and change detection, so we can only do scalingat cursor changing for better performance. As to screen capture on OSX, the captured frame already contains the current cursor. So the MouseCursorMonitorMac is not needed for ScreenCapture for performance purpose. BUG=671436 ==========
sergeyu@chromium.org changed reviewers: - sergeyu@google.com
The CQ bit was checked by braveyao@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from erikchen@chromium.org Link to the patchset: https://codereview.webrtc.org/2908853002/#ps80001 (title: "run git cl format")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496352301518150,
"parent_rev": "8e857d10fd5d2a4138005f560e2d9bf9c3940e5c", "commit_rev":
"0d1e27f00fd6bf7aeae7cb19845b8cd1ed4a40c1"}
Message was sent while issue was closed.
Description was changed from ========== desktopCapture: scale the cursor image according to screen scale factor on OSX Before 10.12, OSX may report 1X cursor on Retina screen. (See crbug.com/632995.) After 10.12, OSX may report 2X cursor on non-Retina screen. (See crbug.com/671436.) So scaling the cursor if the image size doesn't meet the expected size on either Retina or non-Retina screen. Also corrects the cursor caching and change detection, so we can only do scalingat cursor changing for better performance. As to screen capture on OSX, the captured frame already contains the current cursor. So the MouseCursorMonitorMac is not needed for ScreenCapture for performance purpose. BUG=671436 ========== to ========== desktopCapture: scale the cursor image according to screen scale factor on OSX Before 10.12, OSX may report 1X cursor on Retina screen. (See crbug.com/632995.) After 10.12, OSX may report 2X cursor on non-Retina screen. (See crbug.com/671436.) So scaling the cursor if the image size doesn't meet the expected size on either Retina or non-Retina screen. Also corrects the cursor caching and change detection, so we can only do scalingat cursor changing for better performance. As to screen capture on OSX, the captured frame already contains the current cursor. So the MouseCursorMonitorMac is not needed for ScreenCapture for performance purpose. BUG=671436 Review-Url: https://codereview.webrtc.org/2908853002 Cr-Commit-Position: refs/heads/master@{#18393} Committed: https://chromium.googlesource.com/external/webrtc/+/0d1e27f00fd6bf7aeae7cb198... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/external/webrtc/+/0d1e27f00fd6bf7aeae7cb198... |
