Issue 13983010: Use webrtc::DesktopCapturer for screen capturer implementation.

Issue 13983010: Use webrtc::DesktopCapturer for screen capturer implementation. (Closed)

Created:
7 years, 8 months ago by Sergey Ulanov

Modified:
7 years, 7 months ago

Reviewers:
alexeypa (please no reviews), brettw, jln (very slow on Chromium), Ami GONE FROM CHROMIUM, Wez

CC:
chromium-reviews, jamiewalch+watch_chromium.org, hclam+watch_chromium.org, simonmorris+watch_chromium.org, dcaiafa+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, sail+watch_chromium.org, garykac+watch_chromium.org, feature-media-reviews_chromium.org, lambroslambrou+watch_chromium.org, rmsousa+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Use webrtc::DesktopCapturer for screen capturer implementation. Screen capturers are being moved from media/video/capture/screen to third_party/webrtc. This CL is an intermediate step in that process. Depends on https://webrtc-codereview.appspot.com/1322007/ TBR=brettw@chromium.org (third_party/webrtc dependency) Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=200504

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : q #

Total comments: 214

Patch Set 4 : #

Patch Set 5 : #

Total comments: 43

Patch Set 6 : #

Patch Set 7 : #

Patch Set 8 : #

Patch Set 9 : #

Patch Set 10 : #

Patch Set 11 : ~ #

Patch Set 12 : #

Patch Set 13 : #

Total comments: 37

Patch Set 14 : #

Patch Set 15 : #

Total comments: 4

Patch Set 16 : #

Total comments: 4

Patch Set 17 : #

Patch Set 18 : #

Patch Set 19 : #

Created: 7 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1857 lines, -2039 lines)			Patch
M	media/media.gyp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18	6 chunks	+21 lines, -15 lines	0 comments	Download
M	media/video/capture/screen/DEPS	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	media/video/capture/screen/differ.h	View	1	3 chunks	+4 lines, -4 lines	0 comments	Download
M	media/video/capture/screen/differ.cc	View	1 2 3 4 5	3 chunks	+5 lines, -15 lines	0 comments	Download
M	media/video/capture/screen/differ_unittest.cc	View	1 2 3 4 5	11 chunks	+46 lines, -39 lines	0 comments	Download
M	media/video/capture/screen/mac/desktop_configuration.h	View	1	3 chunks	+5 lines, -6 lines	0 comments	Download
M	media/video/capture/screen/mac/desktop_configuration.mm	View	1 2 3 4	7 chunks	+31 lines, -19 lines	0 comments	Download
M	media/video/capture/screen/mouse_cursor_shape.h	View	1 2 3 4 5	1 chunk	+4 lines, -7 lines	0 comments	Download
D	media/video/capture/screen/mouse_cursor_shape.cc	View	1 2 3 4 5	1 chunk	+0 lines, -17 lines	0 comments	Download
D	media/video/capture/screen/screen_capture_data.h	View		1 chunk	+0 lines, -93 lines	0 comments	Download
D	media/video/capture/screen/screen_capture_data.cc	View		1 chunk	+0 lines, -22 lines	0 comments	Download
M	media/video/capture/screen/screen_capture_device.cc	View	1 2 3	12 chunks	+38 lines, -38 lines	0 comments	Download
M	media/video/capture/screen/screen_capture_device_unittest.cc	View	1 2 3 4 5	2 chunks	+19 lines, -18 lines	0 comments	Download
D	media/video/capture/screen/screen_capture_frame.h	View		1 chunk	+0 lines, -64 lines	0 comments	Download
D	media/video/capture/screen/screen_capture_frame.cc	View		1 chunk	+0 lines, -18 lines	0 comments	Download
M	media/video/capture/screen/screen_capture_frame_queue.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13	2 chunks	+22 lines, -23 lines	0 comments	Download
M	media/video/capture/screen/screen_capture_frame_queue.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13	1 chunk	+16 lines, -16 lines	0 comments	Download
M	media/video/capture/screen/screen_capturer.h	View	1 2 3 4 5 6 7	3 chunks	+13 lines, -34 lines	0 comments	Download
M	media/video/capture/screen/screen_capturer_fake.h	View	1	3 chunks	+13 lines, -20 lines	0 comments	Download
M	media/video/capture/screen/screen_capturer_fake.cc	View	1 2 3 4 5 6 7 8 9 10	3 chunks	+53 lines, -44 lines	0 comments	Download
M	media/video/capture/screen/screen_capturer_helper.h	View	1 2 3 4	3 chunks	+11 lines, -12 lines	0 comments	Download
M	media/video/capture/screen/screen_capturer_helper.cc	View	1 2 3 4	3 chunks	+27 lines, -17 lines	0 comments	Download
M	media/video/capture/screen/screen_capturer_helper_unittest.cc	View	1 2 3 4 5	2 chunks	+83 lines, -73 lines	0 comments	Download
M	media/video/capture/screen/screen_capturer_mac.mm	View	1 2 3 4 5 6 7 8 9 10 11 12 13	24 chunks	+151 lines, -183 lines	0 comments	Download
M	media/video/capture/screen/screen_capturer_mac_unittest.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16	3 chunks	+25 lines, -37 lines	0 comments	Download
M	media/video/capture/screen/screen_capturer_mock_objects.h	View	1 2 3 4	2 chunks	+22 lines, -10 lines	0 comments	Download
M	media/video/capture/screen/screen_capturer_mock_objects.cc	View	1 2 3 4	1 chunk	+8 lines, -10 lines	0 comments	Download
M	media/video/capture/screen/screen_capturer_unittest.cc	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+65 lines, -33 lines	0 comments	Download
M	media/video/capture/screen/screen_capturer_win.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	12 chunks	+74 lines, -177 lines	0 comments	Download
M	media/video/capture/screen/screen_capturer_x11.cc	View	1 2 3 4 5 6 7 8 9 10	18 chunks	+120 lines, -144 lines	0 comments	Download
D	media/video/capture/screen/shared_buffer.h	View		1 chunk	+0 lines, -75 lines	0 comments	Download
D	media/video/capture/screen/shared_buffer.cc	View		1 chunk	+0 lines, -42 lines	0 comments	Download
D	media/video/capture/screen/shared_buffer_unittest.cc	View		1 chunk	+0 lines, -55 lines	0 comments	Download
A	media/video/capture/screen/shared_desktop_frame.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13	1 chunk	+43 lines, -0 lines	0 comments	Download
A	media/video/capture/screen/shared_desktop_frame.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13	1 chunk	+56 lines, -0 lines	0 comments	Download
M	media/video/capture/screen/x11/x_server_pixel_buffer.h	View	1	4 chunks	+5 lines, -5 lines	0 comments	Download
M	media/video/capture/screen/x11/x_server_pixel_buffer.cc	View	1	4 chunks	+13 lines, -11 lines	0 comments	Download
M	remoting/codec/DEPS	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	remoting/codec/codec_test.h	View	1	2 chunks	+6 lines, -4 lines	0 comments	Download
M	remoting/codec/codec_test.cc	View	1 2 3	13 chunks	+124 lines, -113 lines	0 comments	Download
M	remoting/codec/video_decoder_vp8_unittest.cc	View	1	2 chunks	+6 lines, -4 lines	0 comments	Download
M	remoting/codec/video_encoder.h	View	1 2 3 4 5	2 chunks	+11 lines, -16 lines	0 comments	Download
M	remoting/codec/video_encoder_verbatim.h	View	1 2 3	3 chunks	+9 lines, -7 lines	0 comments	Download
M	remoting/codec/video_encoder_verbatim.cc	View	1 2 3	5 chunks	+30 lines, -35 lines	0 comments	Download
M	remoting/codec/video_encoder_vp8.h	View	1 2 3	2 chunks	+10 lines, -4 lines	0 comments	Download
M	remoting/codec/video_encoder_vp8.cc	View	1 2 3	8 chunks	+26 lines, -27 lines	0 comments	Download
M	remoting/codec/video_encoder_vp8_unittest.cc	View		3 chunks	+14 lines, -21 lines	0 comments	Download
M	remoting/host/DEPS	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	remoting/host/chromoting_messages.h	View	1 2 3 4 5 6 7 8	7 chunks	+9 lines, -40 lines	0 comments	Download
A	remoting/host/chromoting_param_traits.h	View	1 2 3	1 chunk	+49 lines, -0 lines	0 comments	Download
A	remoting/host/chromoting_param_traits.cc	View	1 2 3	1 chunk	+127 lines, -0 lines	0 comments	Download
M	remoting/host/client_session.cc	View	1 2 3 4 5 6 7 8	2 chunks	+9 lines, -5 lines	0 comments	Download
M	remoting/host/client_session_unittest.cc	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
M	remoting/host/daemon_process.cc	View	1 2 3	3 chunks	+4 lines, -11 lines	0 comments	Download
M	remoting/host/desktop_session_agent.h	View	1 2 3 4 5 6 7 8	8 chunks	+24 lines, -18 lines	0 comments	Download
M	remoting/host/desktop_session_agent.cc	View	1 2 3 4 5 6 7 8	11 chunks	+90 lines, -60 lines	0 comments	Download
M	remoting/host/desktop_session_proxy.h	View	1 2	6 chunks	+12 lines, -8 lines	0 comments	Download
M	remoting/host/desktop_session_proxy.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13	10 chunks	+93 lines, -59 lines	0 comments	Download
M	remoting/host/desktop_session_win.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	1 chunk	+6 lines, -10 lines	0 comments	Download
M	remoting/host/ipc_desktop_environment_unittest.cc	View	1 2 3	8 chunks	+10 lines, -8 lines	0 comments	Download
M	remoting/host/ipc_video_frame_capturer.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13	1 chunk	+15 lines, -7 lines	0 comments	Download
M	remoting/host/ipc_video_frame_capturer.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13	1 chunk	+24 lines, -10 lines	0 comments	Download
M	remoting/host/resizing_host_observer.cc	View	1	1 chunk	+6 lines, -6 lines	0 comments	Download
M	remoting/host/resizing_host_observer_unittest.cc	View		2 chunks	+3 lines, -1 line	0 comments	Download
M	remoting/host/screen_resolution.h	View	1	2 chunks	+14 lines, -14 lines	0 comments	Download
M	remoting/host/screen_resolution.cc	View	1 2 3 4 5	2 chunks	+14 lines, -11 lines	0 comments	Download
M	remoting/host/screen_resolution_unittest.cc	View	1	1 chunk	+19 lines, -50 lines	0 comments	Download
M	remoting/host/video_scheduler.h	View	1 2 3 4 5 6 7 8	6 chunks	+19 lines, -15 lines	0 comments	Download
M	remoting/host/video_scheduler.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13	10 chunks	+47 lines, -48 lines	0 comments	Download
M	remoting/host/video_scheduler_unittest.cc	View	1 2 3	9 chunks	+23 lines, -30 lines	0 comments	Download
M	remoting/remoting.gyp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	6 chunks	+7 lines, -1 line	0 comments	Download

Messages

Total messages: 32 (0 generated)

Expand Messages | Collapse Messages

Sergey Ulanov

Most of the changes are to transition to new the webrtc::DesktopRect and webrtc::DesktopRegion. Also removed ...

7 years, 8 months ago (2013-04-25 22:47:43 UTC) #1

Wez

First round of comments... :) https://codereview.chromium.org/13983010/diff/6062/media/video/capture/screen/mac/desktop_configuration.mm File media/video/capture/screen/mac/desktop_configuration.mm (right): https://codereview.chromium.org/13983010/diff/6062/media/video/capture/screen/mac/desktop_configuration.mm#newcode130 media/video/capture/screen/mac/desktop_configuration.mm:130: JoinRects(desktop_config.bounds, display_config.bounds); nit: Can ...

7 years, 8 months ago (2013-04-26 18:48:14 UTC) #2

Sergey Ulanov

Wez, do you have further comments for https://webrtc-codereview.appspot.com/1322007/ ? Can we please finish it before ...

7 years, 8 months ago (2013-04-26 19:16:03 UTC) #3

alexeypa (please no reviews)

Here we go. We need to make sure that frame ownership is well understood and ...

7 years, 8 months ago (2013-04-26 21:33:58 UTC) #4

Wez

On 2013/04/26 19:16:03, Sergey Ulanov wrote: > Wez, do you have further comments for > ...

7 years, 7 months ago (2013-04-30 13:00:45 UTC) #5

Sergey Ulanov

https://codereview.chromium.org/13983010/diff/6062/media/video/capture/screen/differ.cc File media/video/capture/screen/differ.cc (left): https://codereview.chromium.org/13983010/diff/6062/media/video/capture/screen/differ.cc#oldcode136 media/video/capture/screen/differ.cc:136: region->setEmpty(); On 2013/04/26 21:33:58, alexeypa wrote: > nit: DCHECK(region->is_empty()) ...

7 years, 7 months ago (2013-05-07 22:25:49 UTC) #6

Sergey Ulanov

Also updated unittests now. https://codereview.chromium.org/13983010/diff/6062/media/video/capture/screen/screen_capturer_helper.cc File media/video/capture/screen/screen_capturer_helper.cc (right): https://codereview.chromium.org/13983010/diff/6062/media/video/capture/screen/screen_capturer_helper.cc#newcode48 media/video/capture/screen/screen_capturer_helper.cc:48: void ScreenCapturerHelper::SwapInvalidRegion( On 2013/05/07 22:25:50, ...

7 years, 7 months ago (2013-05-08 01:26:49 UTC) #7

alexeypa (please no reviews)

https://codereview.chromium.org/13983010/diff/6062/media/video/capture/screen/mouse_cursor_shape.h File media/video/capture/screen/mouse_cursor_shape.h (right): https://codereview.chromium.org/13983010/diff/6062/media/video/capture/screen/mouse_cursor_shape.h#newcode18 media/video/capture/screen/mouse_cursor_shape.h:18: MouseCursorShape(); On 2013/05/07 22:25:50, Sergey Ulanov wrote: > That's ...

7 years, 7 months ago (2013-05-08 22:24:59 UTC) #8

Sergey Ulanov

7 years, 7 months ago (2013-05-09 18:49:02 UTC) #9

https://codereview.chromium.org/13983010/diff/6062/media/video/capture/screen...
File media/video/capture/screen/mouse_cursor_shape.h (right):

https://codereview.chromium.org/13983010/diff/6062/media/video/capture/screen...
media/video/capture/screen/mouse_cursor_shape.h:18: MouseCursorShape();
On 2013/05/08 22:24:59, alexeypa wrote:
> On 2013/05/07 22:25:50, Sergey Ulanov wrote:
> > That's not related to this CL, and I'd rather keep this CL smaller.
> 
> I think it is related. The CL changes the set of members and therefore - their
> initialization code. The initialization code is a part of the constructor's
> body. So you are changing the constructor already. And if so, removing it
> entirely is even a simpler change.
> 
> Feel free to do it as a follow up CL, but I personally think it is just not
> worth a separate CL.

Done.

https://codereview.chromium.org/13983010/diff/6062/remoting/codec/codec_test.cc
File remoting/codec/codec_test.cc (right):

https://codereview.chromium.org/13983010/diff/6062/remoting/codec/codec_test....
remoting/codec/codec_test.cc:399: // Generate random data for the updated
region.
On 2013/05/08 22:24:59, alexeypa wrote:
> On 2013/05/07 22:25:50, Sergey Ulanov wrote:
> > On 2013/04/26 21:33:58, alexeypa wrote:
> > > nit: Maybe clarify that the data is not random between the runs?
> > 
> > It's generated using rand(), thus it's random ;) . 
> > Not related to this CL.
> 
> The seed is fixed by srand(0) call. rand() will generate the same "random"
data
> every run.

Yes, I understand it. Still think calling it "random" is justified. E.g.
dictionary says that "random" = "Made, done, happening, or chosen without method
or conscious decision". Also see http://xkcd.com/221/ .

https://codereview.chromium.org/13983010/diff/6062/remoting/host/desktop_sess...
File remoting/host/desktop_session_agent.cc (left):

https://codereview.chromium.org/13983010/diff/6062/remoting/host/desktop_sess...
remoting/host/desktop_session_agent.cc:87: OnSharedBufferCreated)
On 2013/05/08 22:24:59, alexeypa wrote:
> On 2013/05/07 22:25:50, Sergey Ulanov wrote:
> > I added |last_frame_| to make sure we hold the shared buffer before it's
> > received. Previously shared buffer was ref-counted, and it's not anymore,
> which
> > makes it harder to implement this message correctly. Let me know what you
> think
> > about |last_frame_| solution.
> 
> |last_frame_| will nit work either. First there may be two frames in flight.
> Second, DesktopSessoinAgent has to receive confirmation from
DesktopSessioProxy
> that the latter saw the frame before DesktopSessoinAgent can delete the frame.
> Otherwise it might delete it prematurely.

There assumption here is that DesktopCapturer::Capture() cannot be called until
OnCaptureCompleted() callback - this means that we will not receive
ChromotingNetworkDesktopMsg_CaptureFrame will not be received here until the
previous ChromotingDesktopNetworkMsg_CaptureCompleted is received by the network
process. 
Looks like our current VideoScheduler may call Capture() before previous capture
has completed - I think it's wrong and should be fixed (it still should be able
to process multiple frames at the same time).
Anyway, let's discuss this in person.

https://codereview.chromium.org/13983010/diff/56001/media/video/capture/scree...
File media/video/capture/screen/differ.cc (right):

https://codereview.chromium.org/13983010/diff/56001/media/video/capture/scree...
media/video/capture/screen/differ.cc:31: if (!region) {
On 2013/05/08 22:24:59, alexeypa wrote:
> The whole purpose of this method is to return |region|. The caller should not
be
> able to pass NULL. Remove this check.

Done

https://codereview.chromium.org/13983010/diff/56001/media/video/capture/scree...
media/video/capture/screen/differ.cc:35: if (!prev_buffer || !curr_buffer) {
On 2013/05/08 22:24:59, alexeypa wrote:
> It looks like the callers make sure that |prev_buffer| and |curr_buffer| are
not
> NULL. Remove this check.

Done.

https://codereview.chromium.org/13983010/diff/56001/media/video/capture/scree...
media/video/capture/screen/differ.cc:134: DCHECK(region);
On 2013/05/08 22:24:59, alexeypa wrote:
> nit: remove DCHECK. |region->Clear()| will crash if |region| is NULL just
fine.

Done.

https://codereview.chromium.org/13983010/diff/56001/media/video/capture/scree...
File media/video/capture/screen/differ_unittest.cc (right):

https://codereview.chromium.org/13983010/diff/56001/media/video/capture/scree...
media/video/capture/screen/differ_unittest.cc:59:
for(webrtc::DesktopRegion::Iterator iter(region);
On 2013/05/08 22:24:59, alexeypa wrote:
> nit: for (

Done.

https://codereview.chromium.org/13983010/diff/56001/media/video/capture/scree...
File media/video/capture/screen/screen_capture_device.cc (right):

https://codereview.chromium.org/13983010/diff/56001/media/video/capture/scree...
media/video/capture/screen/screen_capture_device.cc:166:
event_handler_->OnError();
On 2013/05/08 22:24:59, alexeypa wrote:
> Below you take a lock and test whether |event_handler_| is not NULL before
> calling it.
> 
> I'm not sure that a failure to capture a frame deserves LOG(ERROR). On one
hand
> there are legitimate cases when a frame cannot be captured. For instance when
> me2me host cannot capture frame while switching between session. It is OK
> because it is temporary state.

This code is not used in Me2Me host - it's for screen capture API, and we want
to know when it stops capturing for any reason.

> 
> On the other hand in the cases when it is indeed an error you don't want to
spam
> the debug log with hundred copies of the same message.

I'll remove it if it becomes a problem.

https://codereview.chromium.org/13983010/diff/56001/media/video/capture/scree...
File media/video/capture/screen/screen_capture_device_unittest.cc (right):

https://codereview.chromium.org/13983010/diff/56001/media/video/capture/scree...
media/video/capture/screen/screen_capture_device_unittest.cc:67: if
(frame_index_ == 0) {
On 2013/05/08 22:24:59, alexeypa wrote:
> Previously, this method was alternating before 1st and 2nd test frames. Now it
> returns the 1st frame only once.

Doesn't really matter for the test that uses this class. Fixed it anyway.

https://codereview.chromium.org/13983010/diff/56001/media/video/capture/scree...
File media/video/capture/screen/screen_capture_frame_queue.cc (right):

https://codereview.chromium.org/13983010/diff/56001/media/video/capture/scree...
media/video/capture/screen/screen_capture_frame_queue.cc:20: class
ScreenCaptureFrameQueue::EmittedFrame
On 2013/05/08 22:24:59, alexeypa wrote:
> How about allowing multiple EmittedFrame object to own a single
> webrtc::DesktopFrame object? Something like:
> 
> class EmittedFrame {
>   EmittedFrame(webrtc::DesktopFrame* frame):
>     owned_frame_(frame),
>     shared_ref_counter_(new int(1)) {}
> 
>   EmittedFrame(EmittedFrame& frame):
>     owned_frame_(frame.owned_frame_),
>     shared_ref_counter_(frame.shared_ref_counter_) {
>     *shared_ref_counter_ += 1;
>   }
> 
>   ~EmittedFrame() {
>     *shared_ref_counter_ -= 1;
>     if (!*shared_ref_counter_) {
>       delete shared_ref_counter_;
>       delete owned_frame_;
>     }
>   }
> 
>   webrtc::DesktopFrame* owned_frame_;
>   int* shared_ref_counter_;
> }
> 
> The reason it might be better is that EmittedFrame does not know anything
about
> the queue so it is easier to reason what it is doing.
> 
> The queue will keep an instance of EmittedFrame for each frame and give out
> another EmittedFrame to the consumer.
> 
> Let's discuss this offline.

Problem is that if consumer tries to keep a frame we may start changing it while
consumer, that's why I don't think we should ever share the same buffer between
multiple emitted frames (we ensure it by verifying that each frame has been
released before we try reusing it).

https://codereview.chromium.org/13983010/diff/56001/media/video/capture/scree...
media/video/capture/screen/screen_capture_frame_queue.cc:39:
queue_->ReturnEmittedFrame(frame_.release(), index_);
On 2013/05/08 22:24:59, alexeypa wrote:
> Can the emitted frames be returned out of order? They are likely returned in
> order now, but it is an assumption that is easy to break.

Order in which frames are returned doesn't matter - that's why we pass index_
here. We only need to make sure that frames are returned before we try reusing
them.

https://codereview.chromium.org/13983010/diff/56001/media/video/capture/scree...
media/video/capture/screen/screen_capture_frame_queue.cc:42: void
DetachFromQueue() {
On 2013/05/08 22:24:59, alexeypa wrote:
> nit: DetachFromQueue() is only called in a situation when all currently
emitted
> frames are being detached from the queue. Will a weak pointer to the queue be
a
> cleaner solution? With a weak pointer the queue will not need to keep a list
of
> emitted frames.

It's also called from queue destructor - for the case when capturer is deleted
before the frames it has emitted. We could use weak pointers here, but the goal
is to move this code to webrtc, and there are no weak pointers in webrtc.

https://codereview.chromium.org/13983010/diff/56001/media/video/capture/scree...
File media/video/capture/screen/screen_capture_frame_queue.h (right):

https://codereview.chromium.org/13983010/diff/56001/media/video/capture/scree...
media/video/capture/screen/screen_capture_frame_queue.h:20: // MoveToNext()
call, if any. Caller should use EmitCurrentFrame() to emit the
On 2013/05/08 22:24:59, alexeypa wrote:
> nit: MoveToNext() -> MoveToNextFrame()

Done.

https://codereview.chromium.org/13983010/diff/56001/media/video/capture/scree...
media/video/capture/screen/screen_capture_frame_queue.h:25: // say, frame
dimensions  change). The queue records which frames need updating
On 2013/05/08 22:24:59, alexeypa wrote:
> nit: "dimensions  change" -> "dimensions change"

Done.

https://codereview.chromium.org/13983010/diff/56001/media/video/capture/scree...
File media/video/capture/screen/screen_capturer_helper_unittest.cc (right):

https://codereview.chromium.org/13983010/diff/56001/media/video/capture/scree...
media/video/capture/screen/screen_capturer_helper_unittest.cc:42: bool
Equals(const DesktopRegion& region1, const DesktopRegion& region2) {
On 2013/05/08 22:24:59, alexeypa wrote:
> nit: DesktopRegionEquals? 
> 
> I guess it is better to use function overloading here. The style guide permits
> it in this case:
> 
> > Use overloaded functions (including constructors) only if a reader looking
at
> a call site can get a good idea of what is happening without having to first
> figure out exactly which overload is being called.

Done.

https://codereview.chromium.org/13983010/diff/56001/media/video/capture/scree...
media/video/capture/screen/screen_capturer_helper_unittest.cc:45: while
(!iter1.IsAtEnd()) {
On 2013/05/08 22:24:59, alexeypa wrote:
> nit: It is easier to follow the logic if the loop looks like this:
> 
> while (!iter1.IsAtEnd() && !iter2.IsAtEnd()) {
>   ...
>   iter1.Advance();
>   iter2.Advance();
> }
> 
> return iter1.IsAtEnd() && iter2.IsAtEnd();

Done.

https://codereview.chromium.org/13983010/diff/56001/media/video/capture/scree...
File media/video/capture/screen/screen_capturer_unittest.cc (right):

https://codereview.chromium.org/13983010/diff/56001/media/video/capture/scree...
media/video/capture/screen/screen_capturer_unittest.cc:26: return !it.IsAtEnd()
&& (it.Advance(), it.IsAtEnd());
On 2013/05/08 22:24:59, alexeypa wrote:
> The name of the matcher does not reflect what it does. How about
> IsUpdatedRegionComplex()?

I think it's correct. This matcher verifies that the region is _not_ complex,
i.e. that it consists of a single rectangle.

> 
> Please comment what |&& (it.Advance(), it.IsAtEnd())| do because it is not
> obvious (and different from before).

No, it's not different.

https://codereview.chromium.org/13983010/diff/56001/media/video/capture/scree...
File media/video/capture/screen/screen_capturer_win.cc (right):

https://codereview.chromium.org/13983010/diff/56001/media/video/capture/scree...
media/video/capture/screen/screen_capturer_win.cc:192: // Emit current frame.
On 2013/05/08 22:24:59, alexeypa wrote:
> nit: the current frame

Done.

https://codereview.chromium.org/13983010/diff/56001/media/video/capture/scree...
media/video/capture/screen/screen_capturer_win.cc:277: void
ScreenCapturerWin::CaptureRegion(
On 2013/05/08 22:24:59, alexeypa wrote:
> nit: The method is still here. It is just empty.

Done.

https://codereview.chromium.org/13983010/diff/56001/media/video/capture/scree...
File media/video/capture/screen/screen_capturer_x11.cc (right):

https://codereview.chromium.org/13983010/diff/56001/media/video/capture/scree...
media/video/capture/screen/screen_capturer_x11.cc:59: // Capture screen pixels
th the current buffer in the queue. In the DAMAGE
On 2013/05/08 22:24:59, alexeypa wrote:
> nit: th -> to

Done.

https://codereview.chromium.org/13983010/diff/56001/remoting/codec/video_enco...
File remoting/codec/video_encoder.h (right):

https://codereview.chromium.org/13983010/diff/56001/remoting/codec/video_enco...
remoting/codec/video_encoder.h:22: // interface is asynchronous to enable
maximum throughput,.
On 2013/05/08 22:24:59, alexeypa wrote:
> nit: remove the comma at the end.

Done.

https://codereview.chromium.org/13983010/diff/56001/remoting/host/chromoting_...
File remoting/host/chromoting_param_traits.cc (right):

https://codereview.chromium.org/13983010/diff/56001/remoting/host/chromoting_...
remoting/host/chromoting_param_traits.cc:48: if (!m->ReadInt(iter, &width) ||
!m->ReadInt(iter, &height))
On 2013/05/08 22:24:59, alexeypa wrote:
> Should this allow negative sizes?

webrtc::DesktopSize type doesn't prohibit negative sizes. Not sure if it will be
useful, but I think we shouldn't limit what can be sent/received to just subset
of possible values for the type, i.e. if we check it here then we should add
DCHECKs in DesktopSize first.

https://codereview.chromium.org/13983010/diff/56001/remoting/host/chromoting_...
remoting/host/chromoting_param_traits.cc:76: !m->ReadInt(iter, &right) ||
!m->ReadInt(iter, &bottom)) {
On 2013/05/08 22:24:59, alexeypa wrote:
> It is legal to have |right < left| or |bottom < top| here?

Same as with DesktopSize. For rects, left > right can be useful in some cases,
e.g. if you apply some linear transformation that mirrors a rectangle.

https://codereview.chromium.org/13983010/diff/56001/remoting/host/desktop_ses...
File remoting/host/desktop_session_agent.h (right):

https://codereview.chromium.org/13983010/diff/56001/remoting/host/desktop_ses...
remoting/host/desktop_session_agent.h:247: // Number of currently allocated
shared buffers.
On 2013/05/08 22:24:59, alexeypa wrote:
> nit: The number

Done.

Sergey Ulanov

in the patch set 9 I added SharedDesktopFrame which uses ref-counting internally and is now ...

7 years, 7 months ago (2013-05-10 02:07:22 UTC) #10

alexeypa (please no reviews)

lgtm once my non-nit comments are addressed. nits are up to you. I think the ...

7 years, 7 months ago (2013-05-13 17:02:00 UTC) #11

Sergey Ulanov

https://codereview.chromium.org/13983010/diff/151001/media/video/capture/screen/screen_capture_frame_queue.cc File media/video/capture/screen/screen_capture_frame_queue.cc (right): https://codereview.chromium.org/13983010/diff/151001/media/video/capture/screen/screen_capture_frame_queue.cc#newcode18 media/video/capture/screen/screen_capture_frame_queue.cc:18: ScreenCaptureFrameQueue::~ScreenCaptureFrameQueue() {} On 2013/05/13 17:02:00, alexeypa wrote: > nit: ...

7 years, 7 months ago (2013-05-13 21:16:51 UTC) #12

alexeypa (please no reviews)

https://codereview.chromium.org/13983010/diff/151001/media/video/capture/screen/shared_desktop_frame.h File media/video/capture/screen/shared_desktop_frame.h (right): https://codereview.chromium.org/13983010/diff/151001/media/video/capture/screen/shared_desktop_frame.h#newcode36 media/video/capture/screen/shared_desktop_frame.h:36: scoped_refptr<Core> core_; On 2013/05/13 21:16:52, Sergey Ulanov wrote: > ...

7 years, 7 months ago (2013-05-13 22:37:37 UTC) #13

Sergey Ulanov

https://codereview.chromium.org/13983010/diff/151001/remoting/host/video_scheduler.cc File remoting/host/video_scheduler.cc (right): https://codereview.chromium.org/13983010/diff/151001/remoting/host/video_scheduler.cc#newcode209 remoting/host/video_scheduler.cc:209: if (pending_frames_ >= kMaxPendingFrames/* || capture_pending_*/) { On 2013/05/13 ...

7 years, 7 months ago (2013-05-13 22:55:37 UTC) #14

Sergey Ulanov

+jln@ - please review chromoting_messages.h and chromoting_param_traits.[h|cc] +fischman@ - please approve changes in media.gyp (not ...

7 years, 7 months ago (2013-05-13 23:41:16 UTC) #15

Ami GONE FROM CHROMIUM

only looked at media.gyp. https://codereview.chromium.org/13983010/diff/173001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/13983010/diff/173001/media/media.gyp#newcode1092 media/media.gyp:1092: ['exclude', '^video/capture/screen/'], Do you need ...

7 years, 7 months ago (2013-05-13 23:48:33 UTC) #16

Sergey Ulanov

https://codereview.chromium.org/13983010/diff/173001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/13983010/diff/173001/media/media.gyp#newcode1092 media/media.gyp:1092: ['exclude', '^video/capture/screen/'], On 2013/05/13 23:48:33, Ami Fischman wrote: > ...

7 years, 7 months ago (2013-05-14 00:20:23 UTC) #17

Ami GONE FROM CHROMIUM

RS LGTM (for media/media.gyp) assuming both component and non-component builds are happy, though I must ...

7 years, 7 months ago (2013-05-14 00:23:38 UTC) #18

Sergey Ulanov

https://codereview.chromium.org/13983010/diff/27005/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/13983010/diff/27005/media/media.gyp#newcode850 media/media.gyp:850: 'dependencies!': [ On 2013/05/14 00:23:38, Ami Fischman wrote: > ...

7 years, 7 months ago (2013-05-14 00:31:19 UTC) #19

jln (very slow on Chromium)

remoting/host/chromoting_messages.h lgtm Please, add some filtering for obviously-wrong values passed over the IPC channel if ...

7 years, 7 months ago (2013-05-15 10:57:19 UTC) #20

jln (very slow on Chromium)

remoting/host/chromoting_messages.h lgtm Please, add some filtering for obviously-wrong values passed over the IPC channel if ...

7 years, 7 months ago (2013-05-15 10:57:44 UTC) #21

Sergey Ulanov

https://codereview.chromium.org/13983010/diff/27005/remoting/host/chromoting_param_traits.cc File remoting/host/chromoting_param_traits.cc (right): https://codereview.chromium.org/13983010/diff/27005/remoting/host/chromoting_param_traits.cc#newcode1 remoting/host/chromoting_param_traits.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. ...

7 years, 7 months ago (2013-05-15 20:12:07 UTC) #22