Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(608)

Issue 100313002: Fix screen capture slowness in Chrome OS feedback report. (Closed)

Created:
7 years ago by hshi1
Modified:
7 years ago
CC:
chromium-reviews, fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org, miu+watch_chromium.org, Alpha Left Google, ncarter (slow)
Visibility:
Public.

Description

Fix screen capture slowness in Chrome OS feedback report. The webkitGetUserMedia maxWidth/maxHeight parameters only define the maximum frame sizes. The actual capture resolution should not exceed that or the actual desktop resolution. In other words it should only downscale the desktop but not upscale it. BUG=324923 TEST=trybot, Chrome OS feedback screen captures in native resolution Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238359

Patch Set 1 #

Patch Set 2 : Add OWNERS for video_capture_device_impl* which got refactored from web_contents_*. #

Total comments: 2

Patch Set 3 : Drop justinlin in OWNERS as suggested by hclam. #

Total comments: 2

Patch Set 4 : Address comments by sergeyu. #

Total comments: 2

Patch Set 5 : More comments from sergeyu@. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+77 lines, -19 lines) Patch
M content/browser/renderer_host/media/OWNERS View 1 2 1 chunk +3 lines, -0 lines 0 comments Download
M content/browser/renderer_host/media/desktop_capture_device_ash.cc View 1 2 3 5 chunks +28 lines, -0 lines 0 comments Download
M content/browser/renderer_host/media/video_capture_device_impl.h View 1 2 3 3 chunks +13 lines, -4 lines 0 comments Download
M content/browser/renderer_host/media/video_capture_device_impl.cc View 1 2 3 4 5 chunks +33 lines, -15 lines 0 comments Download

Messages

Total messages: 15 (0 generated)
hshi1
PTAL. This intends to prevent desktop capture on Chrome OS from upscaling when the requested ...
7 years ago (2013-12-02 22:15:19 UTC) #1
hshi1
+wjia@ (OWNER of content/browser/renderer_host/media)
7 years ago (2013-12-02 22:16:32 UTC) #2
hshi1
+hclam@ as FYI - the video_capture_device_impl* are refactored from the web_contents_video_capture_device* files so I think ...
7 years ago (2013-12-02 22:54:44 UTC) #3
wjia(left Chromium)
The patch looks ok to me. But I need a person working on screen capture ...
7 years ago (2013-12-02 23:03:39 UTC) #4
Alpha Left Google
I'm bouncing this to nick@ to review this as he's most familiar with this code. ...
7 years ago (2013-12-02 23:14:48 UTC) #5
hshi1
https://codereview.chromium.org/100313002/diff/20001/content/browser/renderer_host/media/OWNERS File content/browser/renderer_host/media/OWNERS (right): https://codereview.chromium.org/100313002/diff/20001/content/browser/renderer_host/media/OWNERS#newcode20 content/browser/renderer_host/media/OWNERS:20: per-file video_capture_device_impl*=justinlin@chromium.org On 2013/12/02 23:14:49, Alpha wrote: > Please ...
7 years ago (2013-12-02 23:16:24 UTC) #6
Sergey Ulanov
https://codereview.chromium.org/100313002/diff/40001/content/browser/renderer_host/media/video_capture_device_impl.cc File content/browser/renderer_host/media/video_capture_device_impl.cc (right): https://codereview.chromium.org/100313002/diff/40001/content/browser/renderer_host/media/video_capture_device_impl.cc#newcode134 content/browser/renderer_host/media/video_capture_device_impl.cc:134: capture_size.SetToMin(source_size); There is allow_resolution_change flag in VideoCaptureParams. When it's ...
7 years ago (2013-12-03 00:01:52 UTC) #7
hshi1
https://codereview.chromium.org/100313002/diff/40001/content/browser/renderer_host/media/video_capture_device_impl.cc File content/browser/renderer_host/media/video_capture_device_impl.cc (right): https://codereview.chromium.org/100313002/diff/40001/content/browser/renderer_host/media/video_capture_device_impl.cc#newcode134 content/browser/renderer_host/media/video_capture_device_impl.cc:134: capture_size.SetToMin(source_size); On 2013/12/03 00:01:52, Sergey Ulanov wrote: > There ...
7 years ago (2013-12-03 01:04:52 UTC) #8
hshi1
moving nick & hclam to CC - nick is no longer actively working on chrome ...
7 years ago (2013-12-03 02:05:54 UTC) #9
Sergey Ulanov
https://codereview.chromium.org/100313002/diff/60001/content/browser/renderer_host/media/video_capture_device_impl.cc File content/browser/renderer_host/media/video_capture_device_impl.cc (right): https://codereview.chromium.org/100313002/diff/60001/content/browser/renderer_host/media/video_capture_device_impl.cc#newcode141 content/browser/renderer_host/media/video_capture_device_impl.cc:141: capture_size.SetToMin(source_size); SetToMin() doesn't preserve aspect ratio. I think you ...
7 years ago (2013-12-03 02:14:22 UTC) #10
hshi1
https://codereview.chromium.org/100313002/diff/60001/content/browser/renderer_host/media/video_capture_device_impl.cc File content/browser/renderer_host/media/video_capture_device_impl.cc (right): https://codereview.chromium.org/100313002/diff/60001/content/browser/renderer_host/media/video_capture_device_impl.cc#newcode141 content/browser/renderer_host/media/video_capture_device_impl.cc:141: capture_size.SetToMin(source_size); On 2013/12/03 02:14:23, Sergey Ulanov wrote: > SetToMin() ...
7 years ago (2013-12-03 02:19:36 UTC) #11
Sergey Ulanov
LGTM.
7 years ago (2013-12-03 02:20:58 UTC) #12
wjia(left Chromium)
lgtm
7 years ago (2013-12-03 02:22:37 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hshi@chromium.org/100313002/80001
7 years ago (2013-12-03 02:23:45 UTC) #14
commit-bot: I haz the power
7 years ago (2013-12-03 11:52:42 UTC) #15
Message was sent while issue was closed.
Change committed as 238359

Powered by Google App Engine
This is Rietveld 408576698