refactor video capture in renderer process

refactor video capture in renderer process
This patch is a split of http://codereview.chromium.org/8304017/ .
1. Since all clients of VideoCaptureImpl accept pixel frames with different dimension than requested, there is no need to distinguish them. Remove "resolution_fixed" in media::VideoCapture::VideoCaptureCapability.
2. VideoCaptureImpl is now taking the largested dimension from client requests and send it to browser process.
3. remove some unnecessary code in VideoCaptureModuleImpl since webrtc ViE can handle different frame size.

BUG=none
TEST=trybots
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=108126

[wjia(left Chromium), 2011-10-29 16:04:05]

Per and Magnus, please review all.

For owners:
Ami, please review content/renderer/media and media/ part.
Trung, please review ppapi part.

This is a split of http://codereview.chromium.org/8304017/ to facilitate review
with smaller patch. It's almost same as last patch set in
http://codereview.chromium.org/8304017/, except 2 lines of DCHECK are removed in
video_capture_module_impl.cc

[scherkus (not reviewing), 2011-10-31 03:35:05]

http://codereview.chromium.org/8400084/diff/1/content/renderer/media/video_ca...
File content/renderer/media/video_capture_impl.cc (right):

http://codereview.chromium.org/8400084/diff/1/content/renderer/media/video_ca...
content/renderer/media/video_capture_impl.cc:24: base::SharedMemory* dib;
scoped_ptr?

http://codereview.chromium.org/8400084/diff/1/content/renderer/media/video_ca...
content/renderer/media/video_capture_impl.cc:181: DLOG(INFO) << "StartCapture:
Got client with higher resolution ("
please change all your INFOs to DVLOG(1)

http://codereview.chromium.org/8400084/diff/1/content/renderer/media/video_ca...
content/renderer/media/video_capture_impl.cc:242: if (clients_.size() == 0) {
.empty()

http://codereview.chromium.org/8400084/diff/1/content/renderer/media/video_ca...
content/renderer/media/video_capture_impl.cc:317: if (clients_.size() +
clients_pending_on_restart_.size() > 0)
.empty()

http://codereview.chromium.org/8400084/diff/1/content/renderer/media/video_ca...
File content/renderer/media/video_capture_impl.h (right):

http://codereview.chromium.org/8400084/diff/1/content/renderer/media/video_ca...
content/renderer/media/video_capture_impl.h:86: typedef std::map<int,
DIBBuffer*> CachedDIB;
add /* buffer_id */ ?

http://codereview.chromium.org/8400084/diff/1/content/renderer/media/video_ca...
content/renderer/media/video_capture_impl.h:92: ClientSideDIB client_side_dibs_;
Any reason not to consider adding a "references" field to DIBBuffer instead of
maintaining a second map?

How large can cached_dibs_ get? If it's limited by kMaxNoDIBs in the browser
process then that # would be 3.

The only time you don't have the buffer_id handy is inside DoFeedBuffer(), but I
doubt that the size of cached_dibs_ is so large that iterating over cached_dibs_
wouldn't suffice.

[Ami GONE FROM CHROMIUM, 2011-10-31 16:05:46]

scherkus is on it, so removing myself from reviewers list.  Let me know if you
want me to look at this anyway.

[wjia(left Chromium), 2011-10-31 18:06:13]

Thanks, Andrew! New patch has been updated.

http://codereview.chromium.org/8400084/diff/1/content/renderer/media/video_ca...
File content/renderer/media/video_capture_impl.cc (right):

http://codereview.chromium.org/8400084/diff/1/content/renderer/media/video_ca...
content/renderer/media/video_capture_impl.cc:24: base::SharedMemory* dib;
On 2011/10/31 03:35:06, scherkus wrote:
> scoped_ptr?

Done.

http://codereview.chromium.org/8400084/diff/1/content/renderer/media/video_ca...
content/renderer/media/video_capture_impl.cc:181: DLOG(INFO) << "StartCapture:
Got client with higher resolution ("
On 2011/10/31 03:35:06, scherkus wrote:
> please change all your INFOs to DVLOG(1)

Done.

http://codereview.chromium.org/8400084/diff/1/content/renderer/media/video_ca...
content/renderer/media/video_capture_impl.cc:242: if (clients_.size() == 0) {
On 2011/10/31 03:35:06, scherkus wrote:
> .empty()

Done.

http://codereview.chromium.org/8400084/diff/1/content/renderer/media/video_ca...
content/renderer/media/video_capture_impl.cc:317: if (clients_.size() +
clients_pending_on_restart_.size() > 0)
On 2011/10/31 03:35:06, scherkus wrote:
> .empty()

Done.

http://codereview.chromium.org/8400084/diff/1/content/renderer/media/video_ca...
File content/renderer/media/video_capture_impl.h (right):

http://codereview.chromium.org/8400084/diff/1/content/renderer/media/video_ca...
content/renderer/media/video_capture_impl.h:86: typedef std::map<int,
DIBBuffer*> CachedDIB;
On 2011/10/31 03:35:06, scherkus wrote:
> add /* buffer_id */ ?

Done.

http://codereview.chromium.org/8400084/diff/1/content/renderer/media/video_ca...
content/renderer/media/video_capture_impl.h:92: ClientSideDIB client_side_dibs_;
On 2011/10/31 03:35:06, scherkus wrote:
> Any reason not to consider adding a "references" field to DIBBuffer instead of
> maintaining a second map?
> 
> How large can cached_dibs_ get? If it's limited by kMaxNoDIBs in the browser
> process then that # would be 3.
> 
> The only time you don't have the buffer_id handy is inside DoFeedBuffer(), but
I
> doubt that the size of cached_dibs_ is so large that iterating over
cached_dibs_
> wouldn't suffice.

Added references in DIBBuffer and removed client_side_dibs_.

[viettrungluu, 2011-10-31 23:06:33]

LGTM on the change to ppb_video_capture_impl.cc; let me know if you want me to
look at the rest.

[scherkus (not reviewing), 2011-11-01 02:26:56]

LGTM w/ nit

This is just my $0.02, but I have a quick comment on naming may want to consider
for the future. Both the browser-side and renderer-side capture code have the
notion of "clients". Perhaps it'd be clearer to have the browser-side code refer
to "renderers" instead of "clients" and keep the renderer-side code keep using
"clients"

Are there browser-side clients that aren't renderers?

http://codereview.chromium.org/8400084/diff/7003/content/renderer/media/video...
File content/renderer/media/video_capture_impl.cc (right):

http://codereview.chromium.org/8400084/diff/7003/content/renderer/media/video...
content/renderer/media/video_capture_impl.cc:26: // Number of clients which hold
this dib.
dib -> DIB

[perkj_chrome, 2011-11-01 09:45:44]

lgtm. But consider to rewrite video_capture_impl to use just one map and store
the state in the map.

http://codereview.chromium.org/8400084/diff/7003/content/renderer/media/video...
File content/renderer/media/video_capture_impl.cc (right):

http://codereview.chromium.org/8400084/diff/7003/content/renderer/media/video...
content/renderer/media/video_capture_impl.cc:320: case
media::VideoCapture::kPaused:
What is paused? Ever used?

http://codereview.chromium.org/8400084/diff/7003/content/renderer/media/video...
File content/renderer/media/video_capture_impl.h (right):

http://codereview.chromium.org/8400084/diff/7003/content/renderer/media/video...
content/renderer/media/video_capture_impl.h:93: ClientInfo
clients_pending_on_filter_;
Consider using one map with all clients but add a state to each client. It looks
like you do the same thing a couple of times with the different maps.

[wjia(left Chromium), 2011-11-01 17:24:29]

I will try to use "renderers" in browser process side. Those "renderers" are
represented by hosts.

http://codereview.chromium.org/8400084/diff/7003/content/renderer/media/video...
File content/renderer/media/video_capture_impl.cc (right):

http://codereview.chromium.org/8400084/diff/7003/content/renderer/media/video...
content/renderer/media/video_capture_impl.cc:26: // Number of clients which hold
this dib.
On 2011/11/01 02:26:56, scherkus wrote:
> dib -> DIB

Done.

http://codereview.chromium.org/8400084/diff/7003/content/renderer/media/video...
content/renderer/media/video_capture_impl.cc:320: case
media::VideoCapture::kPaused:
On 2011/11/01 09:45:44, perkj wrote:
> What is paused? Ever used?

"Paused" is a state similar to "started" except that camera (or controller)
doesn't send out any frame data to a specific client, while the session is still
alive. This can be used as power improvement since it requires less computing
and memory access. This is useful for cases where camera is accessed in multiple
tabs and only one tab is visible. The background tabs can be put in "paused"
state.

Right now, it's not used.