Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11680002:
Implement screen capturer for MediaStream API. (Closed)
|
Created:
7 years, 12 months ago by Sergey Ulanov Modified:
7 years, 10 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement Screen Capture adapter for MediaStream API.
The new media::ScreenCaptureDevice implements VideoDeviceCapture interface
that captures screen content. It will be used to support screen capturing in
MediaStream API.
BUG=134249
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180025
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 70
Patch Set 5 : #Patch Set 6 : #
Total comments: 15
Patch Set 7 : #Patch Set 8 : #
Total comments: 2
Patch Set 9 : #
Total comments: 79
Patch Set 10 : #
Total comments: 4
Messages
Total messages: 36 (0 generated)
On 2012/12/26 23:39:21, sergeyu wrote: The bug link leads to 404.
On 2012/12/27 08:08:58, PhistucK wrote: > On 2012/12/26 23:39:21, sergeyu wrote: > > The bug link leads to 404. Fixed now.
ping
Since this will add remoting_screen_capturer under "enable_webrtc", does that module and all its dependencies work on Android (at least build)? https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... File content/browser/renderer_host/media/screen_capturer.cc (right): https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.cc:41: void ScreenCapturer::Start() { check capture_thread_.IsRunning() here too? https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.cc:200: video_frame_capturer_->Stop(); Is this an synchronous API? ScreenCapturer wouldn't receive any frame from video_frame_capturer_ after Stop() is called.
> Since this will add remoting_screen_capturer under "enable_webrtc", does that > module and all its dependencies work on Android (at least build)? No. We don't plan to support it on Android. Moved dependency so that it's always disabled on Android. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... File content/browser/renderer_host/media/screen_capturer.cc (right): https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.cc:41: void ScreenCapturer::Start() { On 2013/01/07 22:15:26, wjia wrote: > check capture_thread_.IsRunning() here too? Done. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.cc:200: video_frame_capturer_->Stop(); On 2013/01/07 22:15:26, wjia wrote: > Is this an synchronous API? ScreenCapturer wouldn't receive any frame from > video_frame_capturer_ after Stop() is called. Yes.
wez: ping
lgtm
https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... File content/browser/renderer_host/media/screen_capturer.cc (right): https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:27: ScreenCapturer::~ScreenCapturer() { If the caller hasn't called DeAllocate then we still have a timer that expects to be torn down on the capture thread but won't be - CHECK() that the capture thread is not running here? https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:32: EventHandler* event_handler) { nit: Consider [D]CHECKing the parameters. e.g. frame_rate == 0 would result in a divide-by-zero fault on the capture thread. https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:33: capture_thread_.Start(); Is there no other thread or thread pool we can allocate from to avoid a thread-per-capturer? https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:56: capture_thread_.Stop(); Does it matter that this might block e.g. for half a second if a screen capture is in-progress on the thread and the host is slow? https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:74: // Invoke OnFrameInfo(). nit: Please make this more descriptive, e.g. "Inform the EventHandler of the frame dimensions". https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:81: base::Time::kMillisecondsPerSecond / frame_rate_; Does it matter that the host platform might not be able to actually match this rate? https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:83: event_handler_->OnFrameInfo(caps); This calls the EventHandler on the capture thread - there's no mention of thread restrictions in the VideoCaptureDevice API comments, so I suspect that that's not safe. https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:86: if (!started_) This is happening on the same thread that DoStop() gets processed on (capture thread), so it'll always be true at this point, surely? https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:87: return; Check this before the |waiting_frame_size_| check - the EventHandler probably doesn't expect to see OnFrameInfo() after it has Stop()ed the capturer. https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:93: // Invalidate resized frame buffer if we previously allocated it. Reword this comment and place it before the if, e.g. "If the captured frame matches the requested size, we don't need to resize it." https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:100: // In case screen size has changed we need to resize the image. Resized image Do we really have no way to inform the caller of the change in resolution? How is this handled for tab-capture? It would be great to avoid all this scaling code - for now I'd be fine with throwing OnError() in this case, to be honest. https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:104: bool redraw_all = false; Why not re-set the CaptureData's rects to contain a single full-frame rect and avoid this special-case? https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:116: capture_data->size().width(); Our capturers won't produce 0x0 frames, but if they _did_ this would give NaN... Perhaps ignore frames w/ "empty" dimensions? https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:125: y = 0; nit: 0.0f https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:134: canvas.scale(scale, scale); Add a comment before this block to summarize the process, i.e. that we wrap the resized bitmap buffer in a Skia device and use a scaling canvas to draw to it, etc. https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:157: i.rect().top() + y / scale, NULL); Does this do the right thing wrt sub-pixel destination coordinates? https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:180: waiting_frame_size_ = true; nit: If you need this, set it before Start()ing the capturer, so that it's clear that it'll be true when the first frame is received. https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:180: waiting_frame_size_ = true; nit: Add blank lines around frame_rate and event_handler init and before DoCapture, to make this function more readable. https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:204: void ScreenCapturer::DoDeAllocate() { ALso deallocate the resize buffer here. https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:209: started_ = false; We should already be stopped via Stop? https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:210: capture_pending_ = true; Why should capture_pending be true when deallocated? https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... File content/browser/renderer_host/media/screen_capturer.h (right): https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.h:41: // VideoFrameCapturer::Delegate interface. nit: Add a comment to clarify that these run on the internal capture thread. https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.h:49: void DoAllocate(int frame_rate, EventHandler* event_handler); Could you fold these helpers into the calls themselves with a thread-trampoline? If not then I'd suggest making clear in the comment that these directly mirror the methods in VideoCaptureDevice. https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.h:53: void DoCapture(); Add a separate comment for DoCapture() to explain what triggers it / what it does, since it doesn't directly mirror a VideoCaptureDevice API. https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.h:55: base::Thread capture_thread_; You're relying on |capture_thread_| stopping during tear-down to make base::Unretained() safe - for that I think you need the thread to be the last-constructed/first-deleted member? https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.h:59: int frame_rate_; Add a comment to indicate that these are parameters from Allocate(), and to explain where |name_| comes from. https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.h:62: bool capture_pending_; Add comments to state explicitly what these are for, please. e.g. For capture_pending_, explain that we only want to capture a single frame at a time. https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.h:65: // |evant_handler_| to specify the size of the frames this capturer will typo: event_handler https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.h:69: bool waiting_frame_size_; Can't you indicate this implicitly via |frame_size_| being empty? https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer.h:73: scoped_ptr<base::RepeatingTimer<ScreenCapturer> > timer_; Add comment to explain this, e.g. why it needs to be a scoped_ptr rather than a straight member. https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... File content/browser/renderer_host/media/screen_capturer_unittest.cc (right): https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer_unittest.cc:41: // A class to perform video frame capturing for Linux. For Linux....? https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer_unittest.cc:76: virtual const SkISize& size_most_recent() const OVERRIDE { This has gone, I think? https://chromiumcodereview.appspot.com/11680002/diff/12012/content/browser/re... content/browser/renderer_host/media/screen_capturer_unittest.cc:110: EXPECT_GT(caps.width, 0); Shouldn't these exactly match the frame1 values?
https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... File content/browser/renderer_host/media/screen_capturer.cc (right): https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.cc:27: ScreenCapturer::~ScreenCapturer() { On 2013/01/10 00:58:14, Wez wrote: > If the caller hasn't called DeAllocate then we still have a timer that expects > to be torn down on the capture thread but won't be - CHECK() that the capture > thread is not running here? Added DeAllocate. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.cc:32: EventHandler* event_handler) { On 2013/01/10 00:58:14, Wez wrote: > nit: Consider [D]CHECKing the parameters. e.g. frame_rate == 0 would result in a > divide-by-zero fault on the capture thread. Done. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.cc:33: capture_thread_.Start(); On 2013/01/10 00:58:14, Wez wrote: > Is there no other thread or thread pool we can allocate from to avoid a > thread-per-capturer? Changed this code to use blocking IO pool. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.cc:56: capture_thread_.Stop(); On 2013/01/10 00:58:14, Wez wrote: > Does it matter that this might block e.g. for half a second if a screen capture > is in-progress on the thread and the host is slow? Removed capture_thread_. thread pool is used instead now. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.cc:74: // Invoke OnFrameInfo(). On 2013/01/10 00:58:14, Wez wrote: > nit: Please make this more descriptive, e.g. "Inform the EventHandler of the > frame dimensions". Done. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.cc:81: base::Time::kMillisecondsPerSecond / frame_rate_; On 2013/01/10 00:58:14, Wez wrote: > Does it matter that the host platform might not be able to actually match this > rate? I don't think so. In either case we have no way to know it in advance. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.cc:83: event_handler_->OnFrameInfo(caps); On 2013/01/10 00:58:14, Wez wrote: > This calls the EventHandler on the capture thread - there's no mention of thread > restrictions in the VideoCaptureDevice API comments, so I suspect that that's > not safe. That's the same behavior that we have in device video capturers. The EventHandler implementation is supposed to handles this properly (see content::VideoCaptureController). https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.cc:86: if (!started_) On 2013/01/10 00:58:14, Wez wrote: > This is happening on the same thread that DoStop() gets processed on (capture > thread), so it'll always be true at this point, surely? OnFrameInfo() needs to be called in response to Allocate() before Start() is called, so DoCapture() is called from DoAllocate(), before DoStart(). https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.cc:87: return; On 2013/01/10 00:58:14, Wez wrote: > Check this before the |waiting_frame_size_| check - the EventHandler probably > doesn't expect to see OnFrameInfo() after it has Stop()ed the capturer. See my previous comment. OnFrameInfo() doesn't depend on Start()/Stop() https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.cc:93: // Invalidate resized frame buffer if we previously allocated it. On 2013/01/10 00:58:14, Wez wrote: > Reword this comment and place it before the if, e.g. > > "If the captured frame matches the requested size, we don't need to resize it." Done. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.cc:100: // In case screen size has changed we need to resize the image. Resized image On 2013/01/10 00:58:14, Wez wrote: > Do we really have no way to inform the caller of the change in resolution? How > is this handled for tab-capture? It would be great to avoid all this scaling > code - for now I'd be fine with throwing OnError() in this case, to be honest. No. The interface was designed for webcams and they don't change resolution unless requested to. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.cc:104: bool redraw_all = false; On 2013/01/10 00:58:14, Wez wrote: > Why not re-set the CaptureData's rects to contain a single full-frame rect and > avoid this special-case? Done. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.cc:116: capture_data->size().width(); On 2013/01/10 00:58:14, Wez wrote: > Our capturers won't produce 0x0 frames, but if they _did_ this would give NaN... > Perhaps ignore frames w/ "empty" dimensions? Added DCHECK on top of this method. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.cc:125: y = 0; On 2013/01/10 00:58:14, Wez wrote: > nit: 0.0f Done. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.cc:134: canvas.scale(scale, scale); On 2013/01/10 00:58:14, Wez wrote: > Add a comment before this block to summarize the process, i.e. that we wrap the > resized bitmap buffer in a Skia device and use a scaling canvas to draw to it, > etc. Done. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.cc:157: i.rect().top() + y / scale, NULL); On 2013/01/10 00:58:14, Wez wrote: > Does this do the right thing wrt sub-pixel destination coordinates? Yes, as far as I know, but it uses simple nearest neighbour scaling. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.cc:180: waiting_frame_size_ = true; On 2013/01/10 00:58:14, Wez wrote: > nit: If you need this, set it before Start()ing the capturer, so that it's clear > that it'll be true when the first frame is received. Start() doesn't really start anything. DoCapture() calls Capture() that actually captures the first frame we use for OnFrameInfo() callback. I've added a comment to make it clearer. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.cc:180: waiting_frame_size_ = true; On 2013/01/10 00:58:14, Wez wrote: > nit: Add blank lines around frame_rate and event_handler init and before > DoCapture, to make this function more readable. Done. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.cc:204: void ScreenCapturer::DoDeAllocate() { On 2013/01/10 00:58:14, Wez wrote: > ALso deallocate the resize buffer here. Added it in DoStop. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.cc:209: started_ = false; On 2013/01/10 00:58:14, Wez wrote: > We should already be stopped via Stop? DeAllocate can be called without calling Stop(). https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.cc:210: capture_pending_ = true; On 2013/01/10 00:58:14, Wez wrote: > Why should capture_pending be true when deallocated? Fixed now. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... File content/browser/renderer_host/media/screen_capturer.h (right): https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.h:41: // VideoFrameCapturer::Delegate interface. On 2013/01/10 00:58:14, Wez wrote: > nit: Add a comment to clarify that these run on the internal capture thread. Done. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.h:49: void DoAllocate(int frame_rate, EventHandler* event_handler); On 2013/01/10 00:58:14, Wez wrote: > Could you fold these helpers into the calls themselves with a thread-trampoline? > > If not then I'd suggest making clear in the comment that these directly mirror > the methods in VideoCaptureDevice. Don't think I can use trampoline approach with worker pool. Done. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.h:53: void DoCapture(); On 2013/01/10 00:58:14, Wez wrote: > Add a separate comment for DoCapture() to explain what triggers it / what it > does, since it doesn't directly mirror a VideoCaptureDevice API. Done. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.h:55: base::Thread capture_thread_; On 2013/01/10 00:58:14, Wez wrote: > You're relying on |capture_thread_| stopping during tear-down to make > base::Unretained() safe - for that I think you need the thread to be the > last-constructed/first-deleted member? Removed the thread. Now using worker pool. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.h:59: int frame_rate_; On 2013/01/10 00:58:14, Wez wrote: > Add a comment to indicate that these are parameters from Allocate(), and to > explain where |name_| comes from. Done. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.h:62: bool capture_pending_; On 2013/01/10 00:58:14, Wez wrote: > Add comments to state explicitly what these are for, please. > > e.g. For capture_pending_, explain that we only want to capture a single frame > at a time. Done. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.h:65: // |evant_handler_| to specify the size of the frames this capturer will On 2013/01/10 00:58:14, Wez wrote: > typo: event_handler Done. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.h:69: bool waiting_frame_size_; On 2013/01/10 00:58:14, Wez wrote: > Can't you indicate this implicitly via |frame_size_| being empty? It's better to have an explicit flag, e.g. for the case when DeAllocate() is called shortly after Allocate(), before we get first frame captured. It also makes code more readable. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer.h:73: scoped_ptr<base::RepeatingTimer<ScreenCapturer> > timer_; On 2013/01/10 00:58:14, Wez wrote: > Add comment to explain this, e.g. why it needs to be a scoped_ptr rather than a > straight member. Not using timer anymore. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... File content/browser/renderer_host/media/screen_capturer_unittest.cc (right): https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer_unittest.cc:41: // A class to perform video frame capturing for Linux. On 2013/01/10 00:58:14, Wez wrote: > For Linux....? Copy-paste failure. Removed this comment. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer_unittest.cc:76: virtual const SkISize& size_most_recent() const OVERRIDE { On 2013/01/10 00:58:14, Wez wrote: > This has gone, I think? Removed now. https://codereview.chromium.org/11680002/diff/12012/content/browser/renderer_... content/browser/renderer_host/media/screen_capturer_unittest.cc:110: EXPECT_GT(caps.width, 0); On 2013/01/10 00:58:14, Wez wrote: > Shouldn't these exactly match the frame1 values? No. This test uses real capturer instead of FakeVideoFrameCapturer.
https://chromiumcodereview.appspot.com/11680002/diff/33001/content/browser/re... File content/browser/renderer_host/media/screen_capturer.cc (right): https://chromiumcodereview.appspot.com/11680002/diff/33001/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:95: bool capture_task_posted_; nit: capture_pending_ https://chromiumcodereview.appspot.com/11680002/diff/33001/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:99: bool capture_pending_; nit: capture_in_progress_ https://chromiumcodereview.appspot.com/11680002/diff/33001/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:145: base::AutoLock auto_lock(event_handler_lock_); This will block if the capture thread is mid-capture at the time it's called, which I don't think is what you want - captures could take e.g. 500ms on some systems. https://chromiumcodereview.appspot.com/11680002/diff/33001/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:327: // and we rely on it. This comment is a little weird; the current interface defines that things work that way, so you're not relying on anything un-defined - lose this and just keep the TODO? https://chromiumcodereview.appspot.com/11680002/diff/33001/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:341: core_->DeAllocate(); nit: Call this->DeAllocate() here in case the impl changes. https://chromiumcodereview.appspot.com/11680002/diff/33001/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:352: core_->Allocate(width, height, frame_rate, event_handler); Can you PostTask() from here directly to ScreenCapturer::Core::Allocate() and avoid the need for DoAllocate()?
https://chromiumcodereview.appspot.com/11680002/diff/33001/content/browser/re... File content/browser/renderer_host/media/screen_capturer.cc (right): https://chromiumcodereview.appspot.com/11680002/diff/33001/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:95: bool capture_task_posted_; On 2013/01/15 06:13:27, Wez wrote: > nit: capture_pending_ IMO capture_task_poster_ better reflects what this flag is used for. https://chromiumcodereview.appspot.com/11680002/diff/33001/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:99: bool capture_pending_; On 2013/01/15 06:13:27, Wez wrote: > nit: capture_in_progress_ Done. https://chromiumcodereview.appspot.com/11680002/diff/33001/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:145: base::AutoLock auto_lock(event_handler_lock_); On 2013/01/15 06:13:27, Wez wrote: > This will block if the capture thread is mid-capture at the time it's called, > which I don't think is what you want - captures could take e.g. 500ms on some > systems. The lock is not acquired during screen capture - it's only acquired when calling the event handler. In either case there is no way to implement media::VideoCaptureDevice interface without some kind of synchronization, and this lock here is better than thread_.Stop() I had in the first version of this CL. https://chromiumcodereview.appspot.com/11680002/diff/33001/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:327: // and we rely on it. On 2013/01/15 06:13:27, Wez wrote: > This comment is a little weird; the current interface defines that things work > that way, so you're not relying on anything un-defined - lose this and just keep > the TODO? We do rely on the fact that OnCaptureCompleted() is called from CaptureFrame() when running this code in a thread pool. Problem is that if video_frame_capturer_ calls that callback later then there may be other task running on a different thread of the thread pool. VideoFrameCapturer doesn't know about SequentialTaskRunner that this class uses, so it would call OnCaptureCompleted() outside of the sequence. https://chromiumcodereview.appspot.com/11680002/diff/33001/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:341: core_->DeAllocate(); On 2013/01/15 06:13:27, Wez wrote: > nit: Call this->DeAllocate() here in case the impl changes. Done. https://chromiumcodereview.appspot.com/11680002/diff/33001/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:352: core_->Allocate(width, height, frame_rate, event_handler); On 2013/01/15 06:13:27, Wez wrote: > Can you PostTask() from here directly to ScreenCapturer::Core::Allocate() and > avoid the need for DoAllocate()? DeAllocate() needs to reset event_handler_ on the calling thread, and I think it's better to keep other methods consistent. Also ScreenCapturer doesn't store reference to task_runner_.
wez: ping
lgtm https://chromiumcodereview.appspot.com/11680002/diff/33001/content/browser/re... File content/browser/renderer_host/media/screen_capturer.cc (right): https://chromiumcodereview.appspot.com/11680002/diff/33001/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:145: base::AutoLock auto_lock(event_handler_lock_); On 2013/01/15 19:29:18, sergeyu wrote: > On 2013/01/15 06:13:27, Wez wrote: > > This will block if the capture thread is mid-capture at the time it's called, > > which I don't think is what you want - captures could take e.g. 500ms on some > > systems. > > The lock is not acquired during screen capture - it's only acquired when calling > the event handler. In either case there is no way to implement > media::VideoCaptureDevice interface without some kind of synchronization, and > this lock here is better than thread_.Stop() I had in the first version of this > CL. Agreed, but I think we should try to resolve this if possible as part of the capturer interface refactoring work. https://chromiumcodereview.appspot.com/11680002/diff/33001/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:327: // and we rely on it. On 2013/01/15 19:29:18, sergeyu wrote: > On 2013/01/15 06:13:27, Wez wrote: > > This comment is a little weird; the current interface defines that things work > > that way, so you're not relying on anything un-defined - lose this and just > keep > > the TODO? > > We do rely on the fact that OnCaptureCompleted() is called from CaptureFrame() > when running this code in a thread pool. Problem is that if > video_frame_capturer_ calls that callback later then there may be other task > running on a different thread of the thread pool. VideoFrameCapturer doesn't > know about SequentialTaskRunner that this class uses, so it would call > OnCaptureCompleted() outside of the sequence. So what the comment should say is that we assume that VideoFrameCapturer::CaptureFrame() will call-back OnCaptureCompleted() before it returns.
https://chromiumcodereview.appspot.com/11680002/diff/33001/content/browser/re... File content/browser/renderer_host/media/screen_capturer.cc (right): https://chromiumcodereview.appspot.com/11680002/diff/33001/content/browser/re... content/browser/renderer_host/media/screen_capturer.cc:327: // and we rely on it. On 2013/01/16 21:26:45, Wez wrote: > On 2013/01/15 19:29:18, sergeyu wrote: > > On 2013/01/15 06:13:27, Wez wrote: > > > This comment is a little weird; the current interface defines that things > work > > > that way, so you're not relying on anything un-defined - lose this and just > > keep > > > the TODO? > > > > We do rely on the fact that OnCaptureCompleted() is called from CaptureFrame() > > when running this code in a thread pool. Problem is that if > > video_frame_capturer_ calls that callback later then there may be other task > > running on a different thread of the thread pool. VideoFrameCapturer doesn't > > know about SequentialTaskRunner that this class uses, so it would call > > OnCaptureCompleted() outside of the sequence. > > So what the comment should say is that we assume that > VideoFrameCapturer::CaptureFrame() will call-back OnCaptureCompleted() before it > returns. Done.
+jam@ for OWNERS review
https://codereview.chromium.org/11680002/diff/49001/content/browser/renderer_... File content/browser/renderer_host/media/DEPS (right): https://codereview.chromium.org/11680002/diff/49001/content/browser/renderer_... content/browser/renderer_host/media/DEPS:3: "+remoting/capturer", remoting is a chrome feature, i.e. it's not part of the web platform. why does content have to know about this? if there's common code in remoting directory that'll be used for webrtc (?), then it should be moved out of remoting..
https://codereview.chromium.org/11680002/diff/49001/content/browser/renderer_... File content/browser/renderer_host/media/DEPS (right): https://codereview.chromium.org/11680002/diff/49001/content/browser/renderer_... content/browser/renderer_host/media/DEPS:3: "+remoting/capturer", On 2013/01/18 02:58:35, John Abd-El-Malek wrote: > remoting is a chrome feature, i.e. it's not part of the web platform. why does > content have to know about this? > > if there's common code in remoting directory that'll be used for webrtc (?), > then it should be moved out of remoting.. remoting/capturer is a separate library that implements screen capturing and it doesn't depend on any other parts of remoting code. Here I'm reusing it to implement screen capturer for webrtc, and so I think it's right to add this dependency in the content layer. Chrome itself doesn't depend on it directly or indirectly (we use use it on the host side, which is not in chrome). This library is used to implement chromoting host, so remoting/ seems like the right place for it, but I'm open to moving it somewhere else. What do you think would be a better place to put it? Somewhere in src/media? Would it be acceptable to do it later in a separate CL, so this one doesn't block other CLs I have that depend on this one?
On 2013/01/18 18:39:04, sergeyu wrote: > https://codereview.chromium.org/11680002/diff/49001/content/browser/renderer_... > File content/browser/renderer_host/media/DEPS (right): > > https://codereview.chromium.org/11680002/diff/49001/content/browser/renderer_... > content/browser/renderer_host/media/DEPS:3: "+remoting/capturer", > On 2013/01/18 02:58:35, John Abd-El-Malek wrote: > > remoting is a chrome feature, i.e. it's not part of the web platform. why does > > content have to know about this? > > > > if there's common code in remoting directory that'll be used for webrtc (?), > > then it should be moved out of remoting.. > > remoting/capturer is a separate library that implements screen capturing and it > doesn't depend on any other parts of remoting code. Here I'm reusing it to > implement screen capturer for webrtc, and so I think it's right to add this > dependency in the content layer. Chrome itself doesn't depend on it directly or > indirectly (we use use it on the host side, which is not in chrome). > This library is used to implement chromoting host, so remoting/ seems like the > right place for it, but I'm open to moving it somewhere else. What do you think > would be a better place to put it? Somewhere in src/media? Would it be > acceptable to do it later in a separate CL, so this one doesn't block other CLs > I have that depend on this one? can you explain why content needs to know about this? is this because chromoting pepper plugin needs this from webrtc? is this stuff now part of the webrtc spec? the bug linked in this change has a description of "Meta item" which means i can't really tell why this change is being done. i would prefer new dependencies don't get added to content and then later removed. move the code that needs to be shared first.
On 2013/01/18 22:28:45, John Abd-El-Malek wrote: > On 2013/01/18 18:39:04, sergeyu wrote: > > > https://codereview.chromium.org/11680002/diff/49001/content/browser/renderer_... > > File content/browser/renderer_host/media/DEPS (right): > > > > > https://codereview.chromium.org/11680002/diff/49001/content/browser/renderer_... > > content/browser/renderer_host/media/DEPS:3: "+remoting/capturer", > > On 2013/01/18 02:58:35, John Abd-El-Malek wrote: > > > remoting is a chrome feature, i.e. it's not part of the web platform. why > does > > > content have to know about this? > > > > > > if there's common code in remoting directory that'll be used for webrtc (?), > > > then it should be moved out of remoting.. > > > > remoting/capturer is a separate library that implements screen capturing and > it > > doesn't depend on any other parts of remoting code. Here I'm reusing it to > > implement screen capturer for webrtc, and so I think it's right to add this > > dependency in the content layer. Chrome itself doesn't depend on it directly > or > > indirectly (we use use it on the host side, which is not in chrome). > > This library is used to implement chromoting host, so remoting/ seems like the > > right place for it, but I'm open to moving it somewhere else. What do you > think > > would be a better place to put it? Somewhere in src/media? Would it be > > acceptable to do it later in a separate CL, so this one doesn't block other > CLs > > I have that depend on this one? > > can you explain why content needs to know about this? Yes, as I mentioned above, it's for WebRTC. > is this because chromoting pepper plugin needs this from webrtc? It's not related to chromoting pepper plugin in any way. Note that remoting/ directory contains things other than the chromoting client plugin. remoting_screen_capturer is a library for screen capturing. It's used for chromoting host, but client plugin doesn't need it. > is this stuff now part of the webrtc spec? It's not in the current draft, but there are plans to make it a part of webrtc standard - juberti@ should have more details. > the bug linked in this change has a description of "Meta item" which means i > can't really tell why this change is being done. > > i would prefer new dependencies don't get added to content and then later > removed. move the code that needs to be shared first. We are not going to remove this dependency in the future, but it's possible to _move_ code of the new dependency from remoting/content to some other place. I can certainly move it before landing this CL (though I disagree that it's necessary). What would be a more appropriate location for this code?
On 2013/01/18 22:55:43, sergeyu wrote: > On 2013/01/18 22:28:45, John Abd-El-Malek wrote: > > On 2013/01/18 18:39:04, sergeyu wrote: > > > > > > https://codereview.chromium.org/11680002/diff/49001/content/browser/renderer_... > > > File content/browser/renderer_host/media/DEPS (right): > > > > > > > > > https://codereview.chromium.org/11680002/diff/49001/content/browser/renderer_... > > > content/browser/renderer_host/media/DEPS:3: "+remoting/capturer", > > > On 2013/01/18 02:58:35, John Abd-El-Malek wrote: > > > > remoting is a chrome feature, i.e. it's not part of the web platform. why > > does > > > > content have to know about this? > > > > > > > > if there's common code in remoting directory that'll be used for webrtc > (?), > > > > then it should be moved out of remoting.. > > > > > > remoting/capturer is a separate library that implements screen capturing and > > it > > > doesn't depend on any other parts of remoting code. Here I'm reusing it to > > > implement screen capturer for webrtc, and so I think it's right to add this > > > dependency in the content layer. Chrome itself doesn't depend on it directly > > or > > > indirectly (we use use it on the host side, which is not in chrome). > > > This library is used to implement chromoting host, so remoting/ seems like > the > > > right place for it, but I'm open to moving it somewhere else. What do you > > think > > > would be a better place to put it? Somewhere in src/media? Would it be > > > acceptable to do it later in a separate CL, so this one doesn't block other > > CLs > > > I have that depend on this one? > > > > can you explain why content needs to know about this? > > Yes, as I mentioned above, it's for WebRTC. yes, that much was clear. but what i was trying to understand is why webrtc needs this? is this really because you want to reimplement remoting using webrtc? > > > is this because chromoting pepper plugin needs this from webrtc? > > It's not related to chromoting pepper plugin in any way. if so, then what's the motivation for adding this functionality to webrtc? it's really hard to figure this out as a reviewer (i.e. when looking at adding new dependencies to content) with very little background in the cl description and bug. > Note that remoting/ > directory contains things other than the chromoting client plugin. > remoting_screen_capturer is a library for screen capturing. It's used for > chromoting host, but client plugin doesn't need it. > > > is this stuff now part of the webrtc spec? > > It's not in the current draft, but there are plans to make it a part of webrtc > standard - juberti@ should have more details. > > > the bug linked in this change has a description of "Meta item" which means i > > can't really tell why this change is being done. > > > > > > i would prefer new dependencies don't get added to content and then later > > removed. move the code that needs to be shared first. > > We are not going to remove this dependency in the future, but it's possible to > _move_ code of the new dependency from remoting/content to some other place. > I can certainly move it before landing this CL (though I disagree that it's > necessary). What would be a more appropriate location for this code? src/remoting seems like it got added for the chromoting plugin, which is a chrome feature. if there's generic code in there that'll be used for webrtc, then it seems it should be factored out to a lower level module which content can depend on it. the equivalent to your argument would be if content depended on src/chrome because there were some useful files in there
On Mon, Jan 21, 2013 at 10:57 PM, <jam@chromium.org> wrote: > On 2013/01/18 22:55:43, sergeyu wrote: > >> On 2013/01/18 22:28:45, John Abd-El-Malek wrote: >> > On 2013/01/18 18:39:04, sergeyu wrote: >> > > >> > >> > > https://codereview.chromium.**org/11680002/diff/49001/** > content/browser/renderer_host/**media/DEPS<https://codereview.chromium.org/11680002/diff/49001/content/browser/renderer_host/media/DEPS> > >> > > File content/browser/renderer_host/**media/DEPS (right): >> > > >> > > >> > >> > > https://codereview.chromium.**org/11680002/diff/49001/** > content/browser/renderer_host/**media/DEPS#newcode3<https://codereview.chromium.org/11680002/diff/49001/content/browser/renderer_host/media/DEPS#newcode3> > >> > > content/browser/renderer_host/**media/DEPS:3: "+remoting/capturer", >> > > On 2013/01/18 02:58:35, John Abd-El-Malek wrote: >> > > > remoting is a chrome feature, i.e. it's not part of the web >> platform. >> > why > >> > does >> > > > content have to know about this? >> > > > >> > > > if there's common code in remoting directory that'll be used for >> webrtc >> (?), >> > > > then it should be moved out of remoting.. >> > > >> > > remoting/capturer is a separate library that implements screen >> capturing >> > and > >> > it >> > > doesn't depend on any other parts of remoting code. Here I'm reusing >> it to >> > > implement screen capturer for webrtc, and so I think it's right to add >> > this > >> > > dependency in the content layer. Chrome itself doesn't depend on it >> > directly > >> > or >> > > indirectly (we use use it on the host side, which is not in chrome). >> > > This library is used to implement chromoting host, so remoting/ seems >> like >> the >> > > right place for it, but I'm open to moving it somewhere else. What do >> you >> > think >> > > would be a better place to put it? Somewhere in src/media? Would it be >> > > acceptable to do it later in a separate CL, so this one doesn't block >> > other > >> > CLs >> > > I have that depend on this one? >> > >> > can you explain why content needs to know about this? >> > > Yes, as I mentioned above, it's for WebRTC. >> > > yes, that much was clear. but what i was trying to understand is why webrtc > needs this? is this really because you want to reimplement remoting using > webrtc? Main motivation for this feature is to enable screencast functionality like in Google+ Hangouts. Currently Hangouts currently relies on an external plugin (not related to remoting plugins). There are many other applications that screen capture support will enable. Yes, it may be useful when moving chromoting to WebRTC, but that's not the main motivation for adding this feature - chromoting host will still need some native plugin for input injection and having screen capture support in Chrome doesn't solve that problem. > > is this because chromoting pepper plugin needs this from webrtc? >> > > It's not related to chromoting pepper plugin in any way. >> > > if so, then what's the motivation for adding this functionality to webrtc? > > it's really hard to figure this out as a reviewer (i.e. when looking at > adding > new dependencies to content) with very little background in the cl > description > and bug. There is no detailed spec for this feature right now as far as I know and the API is not finalized. Initially it will be an experimental feature hidden behind flag. The basic idea is to use screen as a capture source for getUserMedia() (instead of a webcam). getUserMedia() API is documented here: http://www.w3.org/TR/mediacapture-streams/#mediastreamconstraints > > Note that remoting/ >> directory contains things other than the chromoting client plugin. >> remoting_screen_capturer is a library for screen capturing. It's used for >> chromoting host, but client plugin doesn't need it. >> > > > is this stuff now part of the webrtc spec? >> > > It's not in the current draft, but there are plans to make it a part of >> webrtc >> standard - juberti@ should have more details. >> > > > the bug linked in this change has a description of "Meta item" which >> means i >> > can't really tell why this change is being done. >> > > > > >> > i would prefer new dependencies don't get added to content and then >> later >> > removed. move the code that needs to be shared first. >> > > We are not going to remove this dependency in the future, but it's >> possible to >> _move_ code of the new dependency from remoting/content to some other >> place. >> I can certainly move it before landing this CL (though I disagree that >> it's >> necessary). What would be a more appropriate location for this code? >> > > src/remoting seems like it got added for the chromoting plugin, which is a > chrome feature. if there's generic code in there that'll be used for > webrtc, > then it seems it should be factored out to a lower level module which > content > can depend on it. > There are bunch of other things in src/remoting beside the chromoting client plugin, particularly chromoting host and chromoting webapp - they are not part of chrome. The low-level, generic code for screen capturers has already been factored out into a separate module (see https://codereview.chromium.org/11470028). It doesn't depend on any other code in src/remoting. Chrome itself uses client plugin from remoting/client/plugin (see chrome/common/DEPS). That plugin depends on remoting/client and remoting/base, but not remoting/capturers. I.e. the client plugin and screen capturers module are completely separate, and the only thing that's common between them is that they are under src/remoting. They don't share any code except src/base. If I moved that module to src/media, would that resolve your concern? > The equivalent to your argument would be if content depended > on src/chrome because there were some useful files in there > That's not correct comparison because chrome doesn't depend on whole src/remoting. Also src/remoting should not be looked at as a part of chrome (chrome is just linked with a plugin that's build as part of src/remoting) - if that was the case then we would simply move src/remoting to src/chrome/remoting. Closer analogy would be adding src/base/foo/ dependency in src/content when src/chrome depends on src/base/bar/. > https://codereview.chromium.**org/11680002/<https://codereview.chromium.org/1... >
jam: ping
On 2013/01/22 22:08:22, sergeyu wrote: > On Mon, Jan 21, 2013 at 10:57 PM, <mailto:jam@chromium.org> wrote: > > > On 2013/01/18 22:55:43, sergeyu wrote: > > > >> On 2013/01/18 22:28:45, John Abd-El-Malek wrote: > >> > On 2013/01/18 18:39:04, sergeyu wrote: > >> > > > >> > > >> > > > > https://codereview.chromium.**org/11680002/diff/49001/** > > > content/browser/renderer_host/**media/DEPS<https://codereview.chromium.org/11680002/diff/49001/content/browser/renderer_host/media/DEPS> > > > >> > > File content/browser/renderer_host/**media/DEPS (right): > >> > > > >> > > > >> > > >> > > > > https://codereview.chromium.**org/11680002/diff/49001/** > > > content/browser/renderer_host/**media/DEPS#newcode3<https://codereview.chromium.org/11680002/diff/49001/content/browser/renderer_host/media/DEPS#newcode3> > > > >> > > content/browser/renderer_host/**media/DEPS:3: "+remoting/capturer", > >> > > On 2013/01/18 02:58:35, John Abd-El-Malek wrote: > >> > > > remoting is a chrome feature, i.e. it's not part of the web > >> platform. > >> > > why > > > >> > does > >> > > > content have to know about this? > >> > > > > >> > > > if there's common code in remoting directory that'll be used for > >> webrtc > >> (?), > >> > > > then it should be moved out of remoting.. > >> > > > >> > > remoting/capturer is a separate library that implements screen > >> capturing > >> > > and > > > >> > it > >> > > doesn't depend on any other parts of remoting code. Here I'm reusing > >> it to > >> > > implement screen capturer for webrtc, and so I think it's right to add > >> > > this > > > >> > > dependency in the content layer. Chrome itself doesn't depend on it > >> > > directly > > > >> > or > >> > > indirectly (we use use it on the host side, which is not in chrome). > >> > > This library is used to implement chromoting host, so remoting/ seems > >> like > >> the > >> > > right place for it, but I'm open to moving it somewhere else. What do > >> you > >> > think > >> > > would be a better place to put it? Somewhere in src/media? Would it be > >> > > acceptable to do it later in a separate CL, so this one doesn't block > >> > > other > > > >> > CLs > >> > > I have that depend on this one? > >> > > >> > can you explain why content needs to know about this? > >> > > > > Yes, as I mentioned above, it's for WebRTC. > >> > > > > yes, that much was clear. but what i was trying to understand is why webrtc > > needs this? is this really because you want to reimplement remoting using > > webrtc? > > > Main motivation for this feature is to enable screencast functionality like > in Google+ Hangouts. Currently Hangouts currently relies on an external > plugin (not related to remoting plugins). There are many other applications > that screen capture support will enable. > Yes, it may be useful when moving chromoting to WebRTC, but that's not the > main motivation for adding this feature - chromoting host will still need > some native plugin for input injection and having screen capture support in > Chrome doesn't solve that problem. > > > > > > is this because chromoting pepper plugin needs this from webrtc? > >> > > > > It's not related to chromoting pepper plugin in any way. > >> > > > > if so, then what's the motivation for adding this functionality to webrtc? > > > > > it's really hard to figure this out as a reviewer (i.e. when looking at > > adding > > new dependencies to content) with very little background in the cl > > description > > and bug. > > > There is no detailed spec for this feature right now as far as I know and > the API is not finalized. Initially it will be an experimental feature > hidden behind flag. The basic idea is to use screen as a capture source > for getUserMedia() (instead of a webcam). getUserMedia() API is documented > here: http://www.w3.org/TR/mediacapture-streams/#mediastreamconstraints > > > > > > Note that remoting/ > >> directory contains things other than the chromoting client plugin. > >> remoting_screen_capturer is a library for screen capturing. It's used for > >> chromoting host, but client plugin doesn't need it. > >> > > > > > is this stuff now part of the webrtc spec? > >> > > > > It's not in the current draft, but there are plans to make it a part of > >> webrtc > >> standard - juberti@ should have more details. > >> > > > > > the bug linked in this change has a description of "Meta item" which > >> means i > >> > can't really tell why this change is being done. > >> > > > > > > > > >> > i would prefer new dependencies don't get added to content and then > >> later > >> > removed. move the code that needs to be shared first. > >> > > > > We are not going to remove this dependency in the future, but it's > >> possible to > >> _move_ code of the new dependency from remoting/content to some other > >> place. > >> I can certainly move it before landing this CL (though I disagree that > >> it's > >> necessary). What would be a more appropriate location for this code? > >> > > > > src/remoting seems like it got added for the chromoting plugin, which is a > > chrome feature. if there's generic code in there that'll be used for > > webrtc, > > then it seems it should be factored out to a lower level module which > > content > > can depend on it. > > > > There are bunch of other things in src/remoting beside the chromoting > client plugin, particularly chromoting host and chromoting webapp - they > are not part of chrome. The low-level, generic code for screen capturers > has already been factored out into a separate module (see > https://codereview.chromium.org/11470028). It doesn't depend on any other > code in src/remoting. Chrome itself uses client plugin from > remoting/client/plugin (see chrome/common/DEPS). That plugin depends on > remoting/client and remoting/base, but not remoting/capturers. I.e. the > client plugin and screen capturers module are completely separate, and the > only thing that's common between them is that they are under src/remoting. > They don't share any code except src/base. > > If I moved that module to src/media, would that resolve your concern? yes, either src/media or darin also suggested ui/. whatever the owners of these directories are fine with. > > > > The equivalent to your argument would be if content depended > > on src/chrome because there were some useful files in there > > > > That's not correct comparison because chrome doesn't depend on whole > src/remoting. Also src/remoting should not be looked at as a part of chrome > (chrome is just linked with a plugin that's build as part of src/remoting) > - if that was the case then we would simply move src/remoting to > src/chrome/remoting. > Closer analogy would be adding src/base/foo/ dependency in src/content when > src/chrome depends on src/base/bar/. > > > > > > https://codereview.chromium.**org/11680002/%3Chttps://codereview.chromium.org...> > >
+scherkus: Andrew, please approve media.gyp changes. wez: PTAL. I moved screen capturers to src/media, so it makes more sense to put the new adapter code to src/media as well. Moved it and renamed to media::ScreenCaptureDevice (suggestions for a better name are welcome).
On 2013/01/29 19:43:50, sergeyu wrote: > +scherkus: Andrew, please approve media.gyp changes. > wez: PTAL. > > I moved screen capturers to src/media, so it makes more sense to put the new > adapter code to src/media as well. Moved it and renamed to > media::ScreenCaptureDevice (suggestions for a better name are welcome). media.gyp LGTM (feel free to TBR such changes in the future unless you feel it warrants and explicit review of hairy gyp code)
wez: ping
Apologies for the delay. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... File media/video/capture/screen/screen_capture_device.cc (right): https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Drop (c), change to 2013. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:21: } // namespace nit: No need for // namespace comment if it's a one-liner like this, as far as I'm aware. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:27: Core(scoped_refptr<base::SequencedTaskRunner> task_runner); explicit https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:30: void set_test_frame_capturer( See comment on public interface re naming. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:35: void Allocate(int width, int height, nit: Add comment, e.g. "Implementation of VideoCaptureDevice interface." https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:62: void OnCaptureTimer(); nit: CaptureFrame() would be a better name for this, and the comment might be "Captures the next frame. Called on |capture_task_runner_| at regular intervals." https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:65: void DoCapture(); nit: I'd prefer that DoFoo() only be used where there is already a public method Foo() that the function is part of the implementation of; in this case CaptureFrame() would be fine - looking at the implementation, I'm not sure why OnCaptureTimer() and DoCapture() are separate? https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:68: scoped_refptr<base::SequencedTaskRunner> task_runner_; nit: capture_task_runner_ would be clearer, then. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:76: // Frame rate specified in Allocate(). nit: in -> to https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:79: scoped_ptr<ScreenCapturer> video_frame_capturer_; nit: Please add a comment, e.g. "The underlying ScreenCapturer instance used to capture frames". https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:86: bool waiting_frame_size_; nit: waiting_for_frame_size_ or !have_frame_size_ https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:90: // Set to true between DoStart() and DoStop(). nit: I don't think you need "Set to" for this or the following comments. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:90: // Set to true between DoStart() and DoStop(). Please make clear that we can't just test |event_handler_| because this is checked on the capture thread, not the caller one. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:118: int frame_rate, Indentation. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:156: capture_in_progress_ = false; nit: Leave a blank line between the DCHECKs and the code. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:158: nit: Spurious blank line. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:179: return; Move this before |waiting_frame_size_| check? https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:196: // In case screen size has changed we need to resize the image. Resized image Scaling to cope with changes in resolution after startup feels wrong (it'd be a different story if we were supply a frame the size the caller wants, but we're not). How does the Tab Capture API support tab resize? https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:257: DCHECK(task_runner_->RunsTasksOnCurrentThread()); Please add a TODO (and file a bug) for storing the cursor shape and merging it into each captured frame - it's what remote-assistance-style apps will expect, though we may need an option for the app to request capture without the cursor. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:269: video_frame_capturer_->Start(this); Why are we not Start()ing and Stop()ing in DoStart()/DoStop()? Staying started when the consumer isn't actually interested in the data may be expensive, so we shouldn't do so unless it's necessary to provide the desired semantics. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:283: base::TimeDelta::FromSeconds(1) / frame_rate_); Why post a task here rather than triggering a capture immediately; posting just means we have a delay before the first frame. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:313: } else if (!waiting_frame_size_) { If you always have the frame size populated by a synchronously-initiated capture operation then you can't ever get into a state in which the timer goes off and the frame size isn't yet populated, can you? https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:321: DCHECK(!capture_in_progress_); DCHECK(started_ || waiting_frame_size_)? https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... File media/video/capture/screen/screen_capture_device.h (right): https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Drop (c), and year is 2013. Welcome to the future(c). https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.h:21: // ScreenCaptureDevice implements VideoCaptureDevice for screen. It's nit: screen -> the screen https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.h:25: ScreenCaptureDevice(scoped_refptr<base::SequencedTaskRunner> task_runner); explicit https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.h:29: void set_test_screen_capturer( nit: SetScreenCapturerForTest() would read more naturally. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... File media/video/capture/screen/screen_capture_device_unittest.cc (right): https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Drop (c), change to 2013. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device_unittest.cc:24: namespace { nit: Blank line after namespace, for consistency :) https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device_unittest.cc:33: class MockFrameObserver : public VideoCaptureDevice::EventHandler { Is there really no other test that ever needs to mock this interface? https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device_unittest.cc:43: class FakeScreenCapturer : public ScreenCapturer { We used to have a FakeScreenCapturer in remoting/host/ to use for tests, but it got removed because no tests were making use of it; with this test and the ones alexeypa@ is working on, perhaps it's better to move this out to be more re-usable in a follow-up CL? https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device_unittest.cc:43: class FakeScreenCapturer : public ScreenCapturer { If this class stays here it should live in the anonymous namespace, too. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device_unittest.cc:82: nit: Spurious blank line! https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device_unittest.cc:118: EXPECT_EQ(kFrameRate, caps.frame_rate); That seems a peculiar guarantee; do we really intend to always match whatever rate we're asked for? What if it's insanely/infeasibly high? https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device_unittest.cc:122: EXPECT_EQ(caps.width * caps.height * 4, frame_size); Is it required that VideoCapturer implementations have stride==width? https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device_unittest.cc:155: capture_device.DeAllocate(); It's not clear to me what property of the class this is testing?
https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... File media/video/capture/screen/screen_capture_device.cc (right): https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/01/31 00:15:29, Wez wrote: > Drop (c), change to 2013. same as in the previous file. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:21: } // namespace On 2013/01/31 00:15:29, Wez wrote: > nit: No need for // namespace comment if it's a one-liner like this, as far as > I'm aware. It's better to have it for consistency. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:27: Core(scoped_refptr<base::SequencedTaskRunner> task_runner); On 2013/01/31 00:15:29, Wez wrote: > explicit Done. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:30: void set_test_frame_capturer( On 2013/01/31 00:15:29, Wez wrote: > See comment on public interface re naming. Done. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:35: void Allocate(int width, int height, On 2013/01/31 00:15:29, Wez wrote: > nit: Add comment, e.g. "Implementation of VideoCaptureDevice interface." This is not implementation of the interface. Core doesn't implement VideoCaptureDevice. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:62: void OnCaptureTimer(); On 2013/01/31 00:15:29, Wez wrote: > nit: CaptureFrame() would be a better name for this, and the comment might be > "Captures the next frame. Called on |capture_task_runner_| at regular > intervals." IMHO OnCaptureTimer() is better because it makes it clear that this method is used to post a timer tasks, but not called directly. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:65: void DoCapture(); On 2013/01/31 00:15:29, Wez wrote: > nit: I'd prefer that DoFoo() only be used where there is already a public method > Foo() that the function is part of the implementation of; in this case > CaptureFrame() would be fine - looking at the implementation, I'm not sure why > OnCaptureTimer() and DoCapture() are separate? Because OnCaptureTimer() also schedules next capture task. DoCapture() just captures a frame without posting a new task. I believe that Do prefix is useful here to make it clear that it's the method that actually captures next frame, and not OnCaptureTimer(). Also, we often use Do prefix in remoting code when there is no public method with corresponding name. E.g. see DoRead() in remoting::MessageReader. media code also uses Do in similar cases. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:68: scoped_refptr<base::SequencedTaskRunner> task_runner_; On 2013/01/31 00:15:29, Wez wrote: > nit: capture_task_runner_ would be clearer, then. I don't think it's necessary because there is only one task runner used by this class and the class has "Capture" in the name. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:76: // Frame rate specified in Allocate(). On 2013/01/31 00:15:29, Wez wrote: > nit: in -> to Done. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:79: scoped_ptr<ScreenCapturer> video_frame_capturer_; On 2013/01/31 00:15:29, Wez wrote: > nit: Please add a comment, e.g. "The underlying ScreenCapturer instance used to > capture frames". Done. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:86: bool waiting_frame_size_; On 2013/01/31 00:15:29, Wez wrote: > nit: waiting_for_frame_size_ or !have_frame_size_ Done. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:90: // Set to true between DoStart() and DoStop(). On 2013/01/31 00:15:29, Wez wrote: > nit: I don't think you need "Set to" for this or the following comments. Done. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:90: // Set to true between DoStart() and DoStop(). On 2013/01/31 00:15:29, Wez wrote: > Please make clear that we can't just test |event_handler_| because this is > checked on the capture thread, not the caller one. Done. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:118: int frame_rate, On 2013/01/31 00:15:29, Wez wrote: > Indentation. Done. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:156: capture_in_progress_ = false; On 2013/01/31 00:15:29, Wez wrote: > nit: Leave a blank line between the DCHECKs and the code. Done. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:158: On 2013/01/31 00:15:29, Wez wrote: > nit: Spurious blank line. Done. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:179: return; On 2013/01/31 00:15:29, Wez wrote: > Move this before |waiting_frame_size_| check? It's in the right place. We want to call OnFrameInfo() even if Start() hasn't been called. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:196: // In case screen size has changed we need to resize the image. Resized image On 2013/01/31 00:15:29, Wez wrote: > Scaling to cope with changes in resolution after startup feels wrong (it'd be a > different story if we were supply a frame the size the caller wants, but we're > not). How does the Tab Capture API support tab resize? Tab capture scales the screen too: https://code.google.com/p/chromium/source/search?q=CalculateFittedSize&origq=... https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:257: DCHECK(task_runner_->RunsTasksOnCurrentThread()); On 2013/01/31 00:15:29, Wez wrote: > Please add a TODO (and file a bug) for storing the cursor shape and merging it > into each captured frame - it's what remote-assistance-style apps will expect, > though we may need an option for the app to request capture without the cursor. Filed crbug.com/173265 https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:269: video_frame_capturer_->Start(this); On 2013/01/31 00:15:29, Wez wrote: > Why are we not Start()ing and Stop()ing in DoStart()/DoStop()? Staying started > when the consumer isn't actually interested in the data may be expensive, so we > shouldn't do so unless it's necessary to provide the desired semantics. because we need to capture first frame to get screen size and invoke OnFrameInfo(). There is a comment about it two lines below. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:283: base::TimeDelta::FromSeconds(1) / frame_rate_); On 2013/01/31 00:15:29, Wez wrote: > Why post a task here rather than triggering a capture immediately; posting just > means we have a delay before the first frame. Done. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:313: } else if (!waiting_frame_size_) { On 2013/01/31 00:15:29, Wez wrote: > If you always have the frame size populated by a synchronously-initiated capture > operation then you can't ever get into a state in which the timer goes off and > the frame size isn't yet populated, can you? Done. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:321: DCHECK(!capture_in_progress_); On 2013/01/31 00:15:29, Wez wrote: > DCHECK(started_ || waiting_frame_size_)? I don't think it's necessary to check it here. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... File media/video/capture/screen/screen_capture_device.h (right): https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/01/31 00:15:29, Wez wrote: > Drop (c), Why? > and year is 2013. I started this CL in 2012. Besides, CQ should update it if necessary. Please don't nit on the copyright headers. It's waste of time for everybody. We have presubmit check for this. > > Welcome to the future(c). https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.h:21: // ScreenCaptureDevice implements VideoCaptureDevice for screen. It's On 2013/01/31 00:15:29, Wez wrote: > nit: screen -> the screen Done. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.h:25: ScreenCaptureDevice(scoped_refptr<base::SequencedTaskRunner> task_runner); On 2013/01/31 00:15:29, Wez wrote: > explicit Done. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.h:29: void set_test_screen_capturer( On 2013/01/31 00:15:29, Wez wrote: > nit: SetScreenCapturerForTest() would read more naturally. Done. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... File media/video/capture/screen/screen_capture_device_unittest.cc (right): https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/01/31 00:15:29, Wez wrote: > Drop (c), change to 2013. same as in the other two files. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device_unittest.cc:24: namespace { On 2013/01/31 00:15:29, Wez wrote: > nit: Blank line after namespace, for consistency :) Done. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device_unittest.cc:33: class MockFrameObserver : public VideoCaptureDevice::EventHandler { On 2013/01/31 00:15:29, Wez wrote: > Is there really no other test that ever needs to mock this interface? There is one in media/video/capture/video_capture_device_unittest.cc, but it's different. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device_unittest.cc:43: class FakeScreenCapturer : public ScreenCapturer { On 2013/01/31 00:15:29, Wez wrote: > We used to have a FakeScreenCapturer in remoting/host/ to use for tests, but it > got removed because no tests were making use of it; with this test and the ones > alexeypa@ is working on, perhaps it's better to move this out to be more > re-usable in a follow-up CL? Added TODO. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device_unittest.cc:43: class FakeScreenCapturer : public ScreenCapturer { On 2013/01/31 00:15:29, Wez wrote: > If this class stays here it should live in the anonymous namespace, too. Done. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device_unittest.cc:82: On 2013/01/31 00:15:29, Wez wrote: > nit: Spurious blank line! Done. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device_unittest.cc:118: EXPECT_EQ(kFrameRate, caps.frame_rate); On 2013/01/31 00:15:29, Wez wrote: > That seems a peculiar guarantee; do we really intend to always match whatever > rate we're asked for? What if it's insanely/infeasibly high? Actual frame rate may be slower when CPU is slow or busy, but we have no way to know it when OnFrameInfo() is called. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device_unittest.cc:122: EXPECT_EQ(caps.width * caps.height * 4, frame_size); On 2013/01/31 00:15:29, Wez wrote: > Is it required that VideoCapturer implementations have stride==width? Yes. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device_unittest.cc:155: capture_device.DeAllocate(); On 2013/01/31 00:15:29, Wez wrote: > It's not clear to me what property of the class this is testing? That it doesn't crash when screen resolution changes.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/11680002/68001
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/11680002/68001
LGTM w/ a few remaining nits. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... File media/video/capture/screen/screen_capture_device.cc (right): https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:35: void Allocate(int width, int height, On 2013/01/31 02:05:43, sergeyu wrote: > On 2013/01/31 00:15:29, Wez wrote: > > nit: Add comment, e.g. "Implementation of VideoCaptureDevice interface." > > This is not implementation of the interface. Core doesn't implement > VideoCaptureDevice. No, but these are the methods that actually implement that interface for ScreenCaptureDevice. You could equally describe them as the implementation of ScreenCapturedDevice's public API. https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:269: video_frame_capturer_->Start(this); On 2013/01/31 02:05:43, sergeyu wrote: > On 2013/01/31 00:15:29, Wez wrote: > > Why are we not Start()ing and Stop()ing in DoStart()/DoStop()? Staying > started > > when the consumer isn't actually interested in the data may be expensive, so > we > > shouldn't do so unless it's necessary to provide the desired semantics. > > because we need to capture first frame to get screen size and invoke > OnFrameInfo(). There is a comment about it two lines below. So there is. Can we post a delayed task to Stop() the underlying capturer if the wrapper hasn't been Start()ed after, say, a second? https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:269: video_frame_capturer_->Start(this); On 2013/01/31 02:05:43, sergeyu wrote: > On 2013/01/31 00:15:29, Wez wrote: > > Why are we not Start()ing and Stop()ing in DoStart()/DoStop()? Staying > started > > when the consumer isn't actually interested in the data may be expensive, so > we > > shouldn't do so unless it's necessary to provide the desired semantics. > > because we need to capture first frame to get screen size and invoke > OnFrameInfo(). There is a comment about it two lines below. Good point! Can we post a task here to Stop the underlying capturer after, say, a second if the ScreenCaptureDevice isn't Start()ed by then, and then have Start() and Stop() call through the underlying capturer? https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... File media/video/capture/screen/screen_capture_device.h (right): https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/01/31 02:05:43, sergeyu wrote: > On 2013/01/31 00:15:29, Wez wrote: > > Drop (c), > Why? > > > and year is 2013. > I started this CL in 2012. Besides, CQ should update it if necessary. > > Please don't nit on the copyright headers. It's waste of time for everybody. We > have presubmit check for this. From the chromium-dev@ thread: "In related news, the “(c)” will be being dropped from the notice line..." Q: "If one has a CL they carried over from 2012, they should update the copyright to 2013 for new files in that CL, right?" A: "Right." IIUC you're right and the CQ should catch the year, yes. https://chromiumcodereview.appspot.com/11680002/diff/68001/media/video/captur... File media/video/capture/screen/screen_capture_device.cc (right): https://chromiumcodereview.appspot.com/11680002/diff/68001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:202: // changing are copied. The caller is expecting full frames, not differences, so you need to scale the entire frame, not just the changed portions. https://chromiumcodereview.appspot.com/11680002/diff/68001/media/video/captur... File media/video/capture/screen/screen_capture_device_unittest.cc (right): https://chromiumcodereview.appspot.com/11680002/diff/68001/media/video/captur... media/video/capture/screen/screen_capture_device_unittest.cc:126: // Test that screen capturer can handle resolution change. nit: ".. without crashing." Otherwise it's not clear whether you really intended the test to verify that the resize is handled in some specific way (e.g. another OnFrameInfo notification).
Message was sent while issue was closed.
Change committed as 180025
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... File media/video/capture/screen/screen_capture_device.cc (right): https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:269: video_frame_capturer_->Start(this); On 2013/02/01 00:44:34, Wez wrote: > On 2013/01/31 02:05:43, sergeyu wrote: > > On 2013/01/31 00:15:29, Wez wrote: > > > Why are we not Start()ing and Stop()ing in DoStart()/DoStop()? Staying > > started > > > when the consumer isn't actually interested in the data may be expensive, so > > we > > > shouldn't do so unless it's necessary to provide the desired semantics. > > > > because we need to capture first frame to get screen size and invoke > > OnFrameInfo(). There is a comment about it two lines below. > > Good point! Can we post a task here to Stop the underlying capturer after, say, > a second if the ScreenCaptureDevice isn't Start()ed by then, and then have > Start() and Stop() call through the underlying capturer? We could, but it won't make any difference. Stop() doesn't really do anything. Screen capturer don't actually do anything between Start() and Stop() unless CaptureFrame() is called. We should just remove these methods from ScreenCapturer https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... File media/video/capture/screen/screen_capture_device.h (right): https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/02/01 00:44:34, Wez wrote: > On 2013/01/31 02:05:43, sergeyu wrote: > > On 2013/01/31 00:15:29, Wez wrote: > > > Drop (c), > > Why? > > > > > and year is 2013. > > I started this CL in 2012. Besides, CQ should update it if necessary. > > > > Please don't nit on the copyright headers. It's waste of time for everybody. > We > > have presubmit check for this. > > From the chromium-dev@ thread: > > "In related news, the “(c)” will be being dropped from the notice line..." > > Q: "If one has a CL they carried over from 2012, they should update the > copyright to 2013 for new files in that CL, right?" > A: "Right." Ah, I haven't seen this e-mail. It says that (c) is acceptable until it is removed from all files, and it hasn't been removed, so I guess I can keep it. > > IIUC you're right and the CQ should catch the year, yes. Looks like it didn't do it. Will fix it in a separate CL. https://chromiumcodereview.appspot.com/11680002/diff/68001/media/video/captur... File media/video/capture/screen/screen_capture_device.cc (right): https://chromiumcodereview.appspot.com/11680002/diff/68001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:202: // changing are copied. On 2013/02/01 00:44:34, Wez wrote: > The caller is expecting full frames, not differences, so you need to scale the > entire frame, not just the changed portions. Right, but we keep resized_bitmap_ between the frames, so it should already have unchanged regions. https://chromiumcodereview.appspot.com/11680002/diff/68001/media/video/captur... File media/video/capture/screen/screen_capture_device_unittest.cc (right): https://chromiumcodereview.appspot.com/11680002/diff/68001/media/video/captur... media/video/capture/screen/screen_capture_device_unittest.cc:126: // Test that screen capturer can handle resolution change. On 2013/02/01 00:44:34, Wez wrote: > nit: ".. without crashing." > > Otherwise it's not clear whether you really intended the test to verify that the > resize is handled in some specific way (e.g. another OnFrameInfo notification). Will address this in a separate cl
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... File media/video/capture/screen/screen_capture_device.cc (right): https://chromiumcodereview.appspot.com/11680002/diff/61001/media/video/captur... media/video/capture/screen/screen_capture_device.cc:269: video_frame_capturer_->Start(this); On 2013/02/01 02:02:26, sergeyu wrote: > On 2013/02/01 00:44:34, Wez wrote: > > On 2013/01/31 02:05:43, sergeyu wrote: > > > On 2013/01/31 00:15:29, Wez wrote: > > > > Why are we not Start()ing and Stop()ing in DoStart()/DoStop()? Staying > > > started > > > > when the consumer isn't actually interested in the data may be expensive, > so > > > we > > > > shouldn't do so unless it's necessary to provide the desired semantics. > > > > > > because we need to capture first frame to get screen size and invoke > > > OnFrameInfo(). There is a comment about it two lines below. > > > > Good point! Can we post a task here to Stop the underlying capturer after, > say, > > a second if the ScreenCaptureDevice isn't Start()ed by then, and then have > > Start() and Stop() call through the underlying capturer? > > We could, but it won't make any difference. Stop() doesn't really do anything. > Screen capturer don't actually do anything between Start() and Stop() unless > CaptureFrame() is called. We should just remove these methods from > ScreenCapturer SGTM. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
