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

Issue 1233033002: MediaStream: Adding VideoTrackRecorder class and unittests (Closed)

Created:
5 years, 5 months ago by mcasas
Modified:
5 years, 4 months ago
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, wjia+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

MediaStream: Adding VideoTrackRecorder class and unittests VTR is in charge of grabbing VideoFrames, encoding them on IO thread and sending them forward to the registered client (which will be in time a WebmMuxer crrev.com/1225123006). See DD @ https://goo.gl/vSjzC5 (*) for the plan and https://codereview.chromium.org/1211973012/ for a hack of all Chrome parts. (*) Used to be https://goo.gl/kreaQj BUG=262211 Committed: https://crrev.com/a466bd2cc32a9d5bcba758473ebd993a296f7d6d Cr-Commit-Position: refs/heads/master@{#345437}

Patch Set 1 #

Total comments: 13

Patch Set 2 : tomfinegan@ comments, added a capture thread #

Total comments: 23

Patch Set 3 : miu@s comments (except param piggybacking and threading stuff) #

Total comments: 2

Patch Set 4 : Thread cleanup and niklase@ comment #

Total comments: 2

Patch Set 5 : miu@s comment on OnFirstFrameCB and OnEncodedVideoCB #

Total comments: 1

Patch Set 6 : Moved |track_index| and timestamp mgmt from VideoTrackRecorder into WebmMuxer #

Total comments: 33

Patch Set 7 : miu@s comments; rebase; reverted unrelated code cleanups #

Total comments: 6

Patch Set 8 : miu@s comments #

Total comments: 4

Patch Set 9 : miu@s nits #

Total comments: 6

Patch Set 10 : dalecurtis@ comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+609 lines, -55 lines) Patch
M content/content_renderer.gypi View 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M content/content_tests.gypi View 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M content/renderer/media/DEPS View 1 chunk +1 line, -0 lines 0 comments Download
A content/renderer/media/video_track_recorder.h View 1 2 3 4 5 6 7 1 chunk +62 lines, -0 lines 0 comments Download
A content/renderer/media/video_track_recorder.cc View 1 2 3 4 5 6 7 8 9 1 chunk +313 lines, -0 lines 0 comments Download
A content/renderer/media/video_track_recorder_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +160 lines, -0 lines 0 comments Download
M media/capture/webm_muxer.h View 1 2 3 4 5 6 3 chunks +24 lines, -16 lines 0 comments Download
M media/capture/webm_muxer.cc View 1 2 3 4 5 3 chunks +37 lines, -21 lines 0 comments Download
M media/capture/webm_muxer_unittest.cc View 1 2 3 4 5 5 chunks +9 lines, -18 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 69 (34 generated)
mcasas
tomfinegan@ / miu@ PTAL at: video_track_recorder.cc
5 years, 5 months ago (2015-07-22 16:44:09 UTC) #14
Tom Finegan
https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media/video_track_recorder.cc File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media/video_track_recorder.cc#newcode33 content/renderer/media/video_track_recorder.cc:33: CHECK_EQ(VPX_CODEC_OK, ret) << "Failed to destroy codec"; Won't this ...
5 years, 5 months ago (2015-07-23 22:18:39 UTC) #16
mcasas
guys PTAL. https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media/video_track_recorder.cc File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media/video_track_recorder.cc#newcode33 content/renderer/media/video_track_recorder.cc:33: CHECK_EQ(VPX_CODEC_OK, ret) << "Failed to destroy codec"; ...
5 years, 5 months ago (2015-07-24 15:00:10 UTC) #18
Tom Finegan
https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media/video_track_recorder.cc File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media/video_track_recorder.cc#newcode42 content/renderer/media/video_track_recorder.cc:42: vpx_codec_ctx_t* codec_context) { On 2015/07/24 15:00:10, mcasas wrote: > ...
5 years, 5 months ago (2015-07-24 20:56:16 UTC) #19
Tom Finegan
On 2015/07/24 20:56:16, Tom Finegan wrote: > https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media/video_track_recorder.cc > File content/renderer/media/video_track_recorder.cc (right): > > https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media/video_track_recorder.cc#newcode42 ...
5 years, 5 months ago (2015-07-24 20:57:46 UTC) #20
mcasas
miu@ PTAL (I moved actual encoding to a background thread, PTAL) tomfinegan@: will bundle the ...
5 years, 4 months ago (2015-07-27 11:41:01 UTC) #21
miu
https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media/video_track_recorder.cc File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media/video_track_recorder.cc#newcode17 content/renderer/media/video_track_recorder.cc:17: #define VPX_CODEC_DISABLE_COMPAT 1 Is this legal? You're changing a ...
5 years, 4 months ago (2015-07-27 22:30:09 UTC) #22
Niklas Enbom
https://codereview.chromium.org/1233033002/diff/310001/content/renderer/media/video_track_recorder.cc File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/310001/content/renderer/media/video_track_recorder.cc#newcode281 content/renderer/media/video_track_recorder.cc:281: // prediction for the next frame's duration. Drive by ...
5 years, 4 months ago (2015-07-30 19:46:47 UTC) #24
mcasas
miu@ PTAL https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media/video_track_recorder.cc File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media/video_track_recorder.cc#newcode17 content/renderer/media/video_track_recorder.cc:17: #define VPX_CODEC_DISABLE_COMPAT 1 On 2015/07/27 22:30:09, miu ...
5 years, 4 months ago (2015-07-31 11:26:27 UTC) #27
miu
https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media/video_track_recorder.cc File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media/video_track_recorder.cc#newcode17 content/renderer/media/video_track_recorder.cc:17: #define VPX_CODEC_DISABLE_COMPAT 1 On 2015/07/31 11:26:27, mcasas wrote: > ...
5 years, 4 months ago (2015-08-04 04:06:30 UTC) #28
mcasas
miu@ PTAL Took in your suggestion with a twist, and that needed WebmMuxer adaptations; apologies ...
5 years, 4 months ago (2015-08-11 09:35:17 UTC) #29
miu
https://codereview.chromium.org/1233033002/diff/390001/content/renderer/media/video_track_recorder.h File content/renderer/media/video_track_recorder.h (right): https://codereview.chromium.org/1233033002/diff/390001/content/renderer/media/video_track_recorder.h#newcode29 content/renderer/media/video_track_recorder.h:29: base::Callback<OnEncodedVideoCB(const gfx::Size& frame_size, IMO, this is even more complex ...
5 years, 4 months ago (2015-08-13 19:32:26 UTC) #30
mcasas
miu@ PTAL I followed your suggestion and moved track_index mgmt to WebmMuxer, so VideoTrackRecorder doesn't ...
5 years, 4 months ago (2015-08-14 07:59:38 UTC) #31
mcasas
miu@ ping
5 years, 4 months ago (2015-08-17 22:32:42 UTC) #32
miu
Interface looks good to me. Now, just some threading, tear-down, and codec parameter items: https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media/video_track_recorder.cc ...
5 years, 4 months ago (2015-08-18 02:08:36 UTC) #33
mcasas
miu@ PTAL (tomfinegan@ please have a look at the q's) https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media/video_track_recorder.cc File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media/video_track_recorder.cc#newcode45 ...
5 years, 4 months ago (2015-08-19 00:16:30 UTC) #38
Tom Finegan
I can dig more deeply into this if you two would like: I'm in a ...
5 years, 4 months ago (2015-08-19 04:20:10 UTC) #39
miu
https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media/video_track_recorder.cc File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media/video_track_recorder.cc#newcode118 content/renderer/media/video_track_recorder.cc:118: DCHECK(main_render_thread_checker_.CalledOnValidThread()); On 2015/08/19 00:16:30, mcasas wrote: > On 2015/08/18 ...
5 years, 4 months ago (2015-08-19 19:43:45 UTC) #43
Tom Finegan
https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media/video_track_recorder.cc File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media/video_track_recorder.cc#newcode121 content/renderer/media/video_track_recorder.cc:121: } On 2015/08/19 19:43:45, miu wrote: > My concern ...
5 years, 4 months ago (2015-08-19 20:04:12 UTC) #44
mcasas
miu@ PTAL https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media/video_track_recorder.cc File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media/video_track_recorder.cc#newcode118 content/renderer/media/video_track_recorder.cc:118: DCHECK(main_render_thread_checker_.CalledOnValidThread()); On 2015/08/19 19:43:45, miu wrote: > ...
5 years, 4 months ago (2015-08-20 00:37:05 UTC) #45
miu
On 2015/08/19 20:04:12, Tom Finegan wrote: > https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media/video_track_recorder.cc > File content/renderer/media/video_track_recorder.cc (right): > > https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media/video_track_recorder.cc#newcode121 ...
5 years, 4 months ago (2015-08-20 21:30:57 UTC) #46
miu
lgtm % nits, and assuming tomfinegan's response is that the call to vpx_codec_destroy() by a ...
5 years, 4 months ago (2015-08-20 21:31:37 UTC) #47
Tom Finegan
On 2015/08/20 21:30:57, miu wrote: > On 2015/08/19 20:04:12, Tom Finegan wrote: > > > ...
5 years, 4 months ago (2015-08-20 21:37:15 UTC) #48
mcasas
dalecurtis@ PTAL/Owners RS https://codereview.chromium.org/1233033002/diff/590001/content/renderer/media/video_track_recorder.cc File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/590001/content/renderer/media/video_track_recorder.cc#newcode59 content/renderer/media/video_track_recorder.cc:59: virtual ~VpxEncoder(); On 2015/08/20 21:31:37, miu ...
5 years, 4 months ago (2015-08-20 23:26:33 UTC) #50
DaleCurtis
Just skimmed, deferring to miu@ for full review, but lgtm % some nits. https://codereview.chromium.org/1233033002/diff/610001/content/renderer/media/video_track_recorder.cc File ...
5 years, 4 months ago (2015-08-25 17:02:29 UTC) #51
mcasas
https://codereview.chromium.org/1233033002/diff/610001/content/renderer/media/video_track_recorder.cc File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/610001/content/renderer/media/video_track_recorder.cc#newcode62 content/renderer/media/video_track_recorder.cc:62: const base::TimeTicks& capture_timestamp); On 2015/08/25 17:02:29, DaleCurtis wrote: > ...
5 years, 4 months ago (2015-08-25 17:47:28 UTC) #53
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1233033002/650001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1233033002/650001
5 years, 4 months ago (2015-08-25 17:56:13 UTC) #56
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/92527)
5 years, 4 months ago (2015-08-25 18:07:10 UTC) #58
mcasas
avi@ can you LGTM content/renderer/media/DEPS ? Added third_party/libvpx.
5 years, 4 months ago (2015-08-25 18:12:49 UTC) #60
mcasas
5 years, 4 months ago (2015-08-25 19:36:36 UTC) #62
Tom Finegan
lgtm
5 years, 4 months ago (2015-08-25 19:37:03 UTC) #63
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1233033002/650001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1233033002/650001
5 years, 4 months ago (2015-08-25 19:38:16 UTC) #65
Avi (use Gerrit)
LGTM
5 years, 4 months ago (2015-08-25 19:56:22 UTC) #67
commit-bot: I haz the power
Committed patchset #10 (id:650001)
5 years, 4 months ago (2015-08-25 21:23:06 UTC) #68
commit-bot: I haz the power
5 years, 4 months ago (2015-08-25 21:24:00 UTC) #69
Message was sent while issue was closed.
Patchset 10 (id:??) landed as
https://crrev.com/a466bd2cc32a9d5bcba758473ebd993a296f7d6d
Cr-Commit-Position: refs/heads/master@{#345437}

Powered by Google App Engine
This is Rietveld 408576698