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

Issue 23551011: From Video Capture, abolish OnFrameInfo and enable resolution changes (Closed)

Created:
7 years, 3 months ago by ncarter (slow)
Modified:
7 years, 2 months ago
CC:
chromium-reviews, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, jam, miu+watch_chromium.org, mcasas, wjia(left Chromium)
Visibility:
Public.

Description

VideoCapture: abolish OnFrameInfo almost everywhere. Historically OnFrameInfo was a one-time message that gave the resolution of all subsequent frames. This patch eliminates that, to allow the resolution to be specified on a frame-by-frame basis, at the device's discretion. At the IPC layer two changes were necessary, with the rest of the code falling in line: [1] Resolution/layout info (in the form of a VideoCaptureFormat) is sent with each frame delivery. [2] Shared- memory buffers need to be reallocated, so it was necessary to add a message to drop a buffer. A significant rework of VideoCaptureBufferPool handles a shift to a world where buffers are allocated/re-allocated on demand, and where the buffers in the pool may not all be the same size at any given moment. This allows the buffer pool to be allocated earlier, at the start of the lifetime of the controller. OnFrameInfo methods are gone everywhere but the very beginning of the pipeline (between the Device and its Client) and the very end (an OnFrameInfo event is synthesized in the renderer process for the benefit of the pepper video capture host). BUG=266082 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=230276

Patch Set 1 #

Patch Set 2 : Self-review patch set. #

Patch Set 3 : Fix all the unit tests. #

Patch Set 4 : Significant rework #

Patch Set 5 : Expand scope of patch to rework VideoCaptureBufferPool. Checkpoint: tab capture works. #

Patch Set 6 : Rewrite video_capture_buffer_pool_unittest #

Patch Set 7 : rework video_capture_buffer_pool_unittest #

Patch Set 8 : .gitignore? #

Patch Set 9 : Add unit test coverage of reallocation. #

Patch Set 10 : Fix the controller unittests. #

Patch Set 11 : Self-review fixes. #

Patch Set 12 : Fix media_unittests #

Patch Set 13 : sync+merge #

Patch Set 14 : Trybot and self-review fixes. #

Patch Set 15 : Another clang fix #

Patch Set 16 : Add assert to vcbp unittest #

Total comments: 57

Patch Set 17 : Fix constant declaration issue. #

Total comments: 10

Patch Set 18 : Address most reviewer comments (for self review) #

Patch Set 19 : Rework IPC serialization, VideoCaptureParams switch to composition, eliminate OnFrameInfo for PPAPI… #

Total comments: 12

Patch Set 20 : sync #

Patch Set 21 : Three comment fixes. #

Patch Set 22 : Two more comment fixes. #

Total comments: 4

Patch Set 23 : fixes from bbudge #

Unified diffs Side-by-side diffs Delta from patch set Stats (+911 lines, -1302 lines) Patch
M content/browser/renderer_host/media/desktop_capture_device_unittest.cc View 1 2 3 4 5 6 7 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/renderer_host/media/video_capture_buffer_pool.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 5 chunks +60 lines, -43 lines 0 comments Download
M content/browser/renderer_host/media/video_capture_buffer_pool.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 5 chunks +94 lines, -103 lines 0 comments Download
M content/browser/renderer_host/media/video_capture_buffer_pool_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +103 lines, -79 lines 0 comments Download
M content/browser/renderer_host/media/video_capture_controller.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 4 chunks +6 lines, -19 lines 0 comments Download
M content/browser/renderer_host/media/video_capture_controller.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 16 chunks +98 lines, -219 lines 0 comments Download
M content/browser/renderer_host/media/video_capture_controller_event_handler.h View 1 2 3 4 5 6 7 8 9 2 chunks +12 lines, -18 lines 0 comments Download
M content/browser/renderer_host/media/video_capture_controller_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 11 chunks +58 lines, -72 lines 0 comments Download
M content/browser/renderer_host/media/video_capture_host.h View 1 2 3 4 chunks +53 lines, -48 lines 0 comments Download
M content/browser/renderer_host/media/video_capture_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 5 chunks +32 lines, -64 lines 0 comments Download
M content/browser/renderer_host/media/video_capture_host_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 8 chunks +46 lines, -47 lines 0 comments Download
M content/browser/renderer_host/media/video_capture_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +13 lines, -12 lines 0 comments Download
M content/browser/renderer_host/media/video_capture_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +10 lines, -14 lines 0 comments Download
M content/browser/renderer_host/media/web_contents_video_capture_device.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 5 chunks +19 lines, -17 lines 0 comments Download
M content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 2 chunks +5 lines, -12 lines 0 comments Download
M content/common/media/media_param_traits.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +3 lines, -4 lines 0 comments Download
M content/common/media/media_param_traits.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 3 chunks +8 lines, -11 lines 0 comments Download
M content/common/media/video_capture_messages.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 3 chunks +24 lines, -17 lines 0 comments Download
M content/renderer/media/rtc_video_capture_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +1 line, -7 lines 0 comments Download
M content/renderer/media/rtc_video_capture_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 3 chunks +2 lines, -12 lines 0 comments Download
M content/renderer/media/rtc_video_capturer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +9 lines, -8 lines 0 comments Download
M content/renderer/media/video_capture_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 5 chunks +25 lines, -27 lines 0 comments Download
M content/renderer/media/video_capture_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 16 chunks +67 lines, -126 lines 0 comments Download
M content/renderer/media/video_capture_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 10 chunks +19 lines, -48 lines 0 comments Download
M content/renderer/media/video_capture_message_filter.h View 1 2 4 chunks +18 lines, -25 lines 0 comments Download
M content/renderer/media/video_capture_message_filter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 4 chunks +13 lines, -13 lines 0 comments Download
M content/renderer/media/video_capture_message_filter_unittest.cc View 1 2 2 chunks +62 lines, -141 lines 0 comments Download
M content/renderer/pepper/pepper_platform_video_capture.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +1 line, -6 lines 0 comments Download
M content/renderer/pepper/pepper_platform_video_capture.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 4 chunks +2 lines, -17 lines 0 comments Download
M content/renderer/pepper/pepper_video_capture_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 3 chunks +5 lines, -4 lines 0 comments Download
M content/renderer/pepper/pepper_video_capture_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 6 chunks +23 lines, -14 lines 0 comments Download
M media/video/capture/video_capture.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +2 lines, -14 lines 0 comments Download
M media/video/capture/video_capture_device.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +6 lines, -5 lines 0 comments Download
M media/video/capture/video_capture_device_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +2 lines, -1 line 0 comments Download
M media/video/capture/video_capture_proxy.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 3 chunks +1 line, -9 lines 0 comments Download
M media/video/capture/video_capture_proxy.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 3 chunks +0 lines, -21 lines 0 comments Download
M media/video/capture/video_capture_types.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +7 lines, -4 lines 0 comments Download

Messages

Total messages: 18 (0 generated)
ncarter (slow)
This is finally ready for review.
7 years, 2 months ago (2013-10-02 22:49:35 UTC) #1
jiayl
Nice work! Thanks! https://codereview.chromium.org/23551011/diff/68002/content/browser/renderer_host/media/video_capture_buffer_pool.cc File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right): https://codereview.chromium.org/23551011/diff/68002/content/browser/renderer_host/media/video_capture_buffer_pool.cc#newcode164 content/browser/renderer_host/media/video_capture_buffer_pool.cc:164: // after the loop. What if ...
7 years, 2 months ago (2013-10-03 00:51:13 UTC) #2
Ami GONE FROM CHROMIUM
Nice https://codereview.chromium.org/23551011/diff/68002/content/browser/renderer_host/media/video_capture_buffer_pool.cc File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right): https://codereview.chromium.org/23551011/diff/68002/content/browser/renderer_host/media/video_capture_buffer_pool.cc#newcode45 content/browser/renderer_host/media/video_capture_buffer_pool.cc:45: return ReserveForProducerInternal(size, buffer_id_to_drop); nit: s/Internal/_Locked/ ? https://codereview.chromium.org/23551011/diff/68002/content/browser/renderer_host/media/video_capture_buffer_pool.h File ...
7 years, 2 months ago (2013-10-04 00:24:14 UTC) #3
Ami GONE FROM CHROMIUM
https://codereview.chromium.org/23551011/diff/68002/content/browser/renderer_host/media/video_capture_controller.cc File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/23551011/diff/68002/content/browser/renderer_host/media/video_capture_controller.cc#newcode562 content/browser/renderer_host/media/video_capture_controller.cc:562: i420_intermediate_buffer_.reset( Is this (unconditionally realloc'ing the buffer) a form ...
7 years, 2 months ago (2013-10-04 00:26:46 UTC) #4
sheu
https://chromiumcodereview.appspot.com/23551011/diff/177001/content/browser/renderer_host/media/video_capture_buffer_pool.cc File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right): https://chromiumcodereview.appspot.com/23551011/diff/177001/content/browser/renderer_host/media/video_capture_buffer_pool.cc#newcode178 content/browser/renderer_host/media/video_capture_buffer_pool.cc:178: Buffer* buffer = new Buffer(); "buffer" leaks here on ...
7 years, 2 months ago (2013-10-12 00:37:51 UTC) #5
ncarter (slow)
Patch set #19 should address all comments against the old base. Patch set 20 will ...
7 years, 2 months ago (2013-10-16 02:08:39 UTC) #6
Ami GONE FROM CHROMIUM
PS#19 LGTM % assorted comments / doco requests / optional suggestions. https://codereview.chromium.org/23551011/diff/68002/content/browser/renderer_host/media/video_capture_buffer_pool.h File content/browser/renderer_host/media/video_capture_buffer_pool.h (right): ...
7 years, 2 months ago (2013-10-17 20:31:44 UTC) #7
sheu
https://chromiumcodereview.appspot.com/23551011/diff/177001/content/renderer/media/video_capture_impl.cc File content/renderer/media/video_capture_impl.cc (right): https://chromiumcodereview.appspot.com/23551011/diff/177001/content/renderer/media/video_capture_impl.cc#newcode178 content/renderer/media/video_capture_impl.cc:178: DCHECK_EQ(params.session_id, 0); Have you thought about making that explicit ...
7 years, 2 months ago (2013-10-18 01:48:43 UTC) #8
sheu
https://chromiumcodereview.appspot.com/23551011/diff/230001/content/browser/renderer_host/media/video_capture_buffer_pool_unittest.cc File content/browser/renderer_host/media/video_capture_buffer_pool_unittest.cc (right): https://chromiumcodereview.appspot.com/23551011/diff/230001/content/browser/renderer_host/media/video_capture_buffer_pool_unittest.cc#newcode25 content/browser/renderer_host/media/video_capture_buffer_pool_unittest.cc:25: void ExpectDroppedId(int expected_dropped_id) { nit: according to style guide, ...
7 years, 2 months ago (2013-10-18 01:48:44 UTC) #9
ncarter (slow)
I am blocked from testing locally (for several days now) by https://code.google.com/p/chromium/issues/detail?id=308740 which breaks the ...
7 years, 2 months ago (2013-10-18 15:54:45 UTC) #10
ncarter (slow)
Thanks for the feedback everybody. https://codereview.chromium.org/23551011/diff/68002/content/browser/renderer_host/media/video_capture_buffer_pool.h File content/browser/renderer_host/media/video_capture_buffer_pool.h (right): https://codereview.chromium.org/23551011/diff/68002/content/browser/renderer_host/media/video_capture_buffer_pool.h#newcode49 content/browser/renderer_host/media/video_capture_buffer_pool.h:49: static const int kInvalidId ...
7 years, 2 months ago (2013-10-22 01:06:19 UTC) #11
ncarter (slow)
kenrb@chromium.org: Please review (as security OWNER) the changes in video_capture_messages.h and media_param_traits.h/.cc
7 years, 2 months ago (2013-10-22 01:09:19 UTC) #12
ncarter (slow)
bbudge@chromium.org: Please review changes in content/renderer/pepper
7 years, 2 months ago (2013-10-22 01:10:48 UTC) #13
kenrb
lgtm
7 years, 2 months ago (2013-10-22 17:58:31 UTC) #14
bbudge
LGTM w/renaming nits for content/renderer/pepper https://codereview.chromium.org/23551011/diff/967001/content/renderer/pepper/pepper_video_capture_host.h File content/renderer/pepper/pepper_video_capture_host.h (right): https://codereview.chromium.org/23551011/diff/967001/content/renderer/pepper/pepper_video_capture_host.h#newcode64 content/renderer/pepper/pepper_video_capture_host.h:64: void Error(); Could you ...
7 years, 2 months ago (2013-10-22 18:13:09 UTC) #15
ncarter (slow)
https://codereview.chromium.org/23551011/diff/967001/content/renderer/pepper/pepper_video_capture_host.h File content/renderer/pepper/pepper_video_capture_host.h (right): https://codereview.chromium.org/23551011/diff/967001/content/renderer/pepper/pepper_video_capture_host.h#newcode64 content/renderer/pepper/pepper_video_capture_host.h:64: void Error(); On 2013/10/22 18:13:10, bbudge1 wrote: > Could ...
7 years, 2 months ago (2013-10-22 18:22:33 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nick@chromium.org/23551011/1177001
7 years, 2 months ago (2013-10-22 23:06:20 UTC) #17
commit-bot: I haz the power
7 years, 2 months ago (2013-10-23 01:36:36 UTC) #18
Message was sent while issue was closed.
Change committed as 230276

Powered by Google App Engine
This is Rietveld 408576698