Refactor VideoTrackRecorder into smaller classes

Refactor VideoTrackRecorder into smaller classes	

This CL does a refactor taking each encoder implementation into its own
file. Additionally, CanEncodeAlphaChannelForTesting() is removed as it
is no longer necessary.

BUG=700433
Review-Url: https://codereview.chromium.org/2793303003
Cr-Commit-Position: refs/heads/master@{#463015}
Committed: https://chromium.googlesource.com/chromium/src/+/7f9c1f75fc3ddb2d9be2d7de0c8954fc0bb6dcfa

[emircan, 2017-04-04 23:43:55]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 23:44:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emircan, 2017-04-04 23:45:29]

Description was changed from

==========
refactor

BUG=
==========

to

==========
Refactor VideoTrackRecorder into smaller classes	

This CL does a refactor taking each encoder implementation into its own
file. Additionally CanEncodeAlphaChannelForTesting() is removed as it
is no longer necessary.

BUG=700433
==========

[emircan, 2017-04-04 23:45:29]

emircan@chromium.org changed reviewers:
+ mcasas@chromium.org

[emircan, 2017-04-04 23:45:54]

PTAL.

[mcasas, 2017-04-05 01:23:59]

Few comments otherwise lgtm

(please re-read all comments once again)

-- having no individual diffs 
new-file vs video_track_recorder.cc
scares and confuses me, but I guess
it's git limitation; good thing we have 
bags of tests ;)

https://codereview.chromium.org/2793303003/diff/1/content/renderer/BUILD.gn
File content/renderer/BUILD.gn (right):

https://codereview.chromium.org/2793303003/diff/1/content/renderer/BUILD.gn#n...
content/renderer/BUILD.gn:700: "media_recorder/h264_encoder.h",
These two should only be added to the sources
if (rtc_use_h264)

https://cs.chromium.org/chromium/src/content/renderer/BUILD.gn?type=cs&q=RTC_...

https://codereview.chromium.org/2793303003/diff/1/content/renderer/media_reco...
File content/renderer/media_recorder/h264_encoder.h (right):

https://codereview.chromium.org/2793303003/diff/1/content/renderer/media_reco...
content/renderer/media_recorder/h264_encoder.h:10: #if BUILDFLAG(RTC_USE_H264)
Following my comment in BUILD.gn, you can do here

#if !BUILDFLAG(#if BUILDFLAG(RTC_USE_H264))
#error RTC_USE_H264 should be defined.
#endif

https://codereview.chromium.org/2793303003/diff/1/content/renderer/media_reco...
content/renderer/media_recorder/h264_encoder.h:43: // |encoding_thread_|.
|encoding_thread_| (here and in l.48) is not around here
anymore, please adapt the comment to say that it's in 
the base class.

https://codereview.chromium.org/2793303003/diff/1/content/renderer/media_reco...
File content/renderer/media_recorder/video_track_recorder.h (right):

https://codereview.chromium.org/2793303003/diff/1/content/renderer/media_reco...
content/renderer/media_recorder/video_track_recorder.h:76: // is not enforced);
- uses an internal |encoding_task_runner_| for actual
The dashes are supposed to go in each line: 
https://cs.chromium.org/chromium/src/content/renderer/media_recorder/video_tr...

[mcasas, 2017-04-05 01:24:35]

On 2017/04/05 01:23:59, mcasas wrote:
> Few comments otherwise lgtm
> 
> (please re-read all comments once again)

And by "comments" I meant function-level comments :-)

> 
> -- having no individual diffs 
> new-file vs video_track_recorder.cc
> scares and confuses me, but I guess
> it's git limitation; good thing we have 
> bags of tests ;)
> 
> https://codereview.chromium.org/2793303003/diff/1/content/renderer/BUILD.gn
> File content/renderer/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2793303003/diff/1/content/renderer/BUILD.gn#n...
> content/renderer/BUILD.gn:700: "media_recorder/h264_encoder.h",
> These two should only be added to the sources
> if (rtc_use_h264)
> 
>
https://cs.chromium.org/chromium/src/content/renderer/BUILD.gn?type=cs&q=RTC_...
> 
>
https://codereview.chromium.org/2793303003/diff/1/content/renderer/media_reco...
> File content/renderer/media_recorder/h264_encoder.h (right):
> 
>
https://codereview.chromium.org/2793303003/diff/1/content/renderer/media_reco...
> content/renderer/media_recorder/h264_encoder.h:10: #if BUILDFLAG(RTC_USE_H264)
> Following my comment in BUILD.gn, you can do here
> 
> #if !BUILDFLAG(#if BUILDFLAG(RTC_USE_H264))
> #error RTC_USE_H264 should be defined.
> #endif
> 
>
https://codereview.chromium.org/2793303003/diff/1/content/renderer/media_reco...
> content/renderer/media_recorder/h264_encoder.h:43: // |encoding_thread_|.
> |encoding_thread_| (here and in l.48) is not around here
> anymore, please adapt the comment to say that it's in 
> the base class.
> 
>
https://codereview.chromium.org/2793303003/diff/1/content/renderer/media_reco...
> File content/renderer/media_recorder/video_track_recorder.h (right):
> 
>
https://codereview.chromium.org/2793303003/diff/1/content/renderer/media_reco...
> content/renderer/media_recorder/video_track_recorder.h:76: // is not
enforced);
> - uses an internal |encoding_task_runner_| for actual
> The dashes are supposed to go in each line: 
>
https://cs.chromium.org/chromium/src/content/renderer/media_recorder/video_tr...

[commit-bot: I haz the power, 2017-04-05 01:38:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 01:38:19]

Dry run: This issue passed the CQ dry run.

[emircan1, 2017-04-05 02:17:12]

Description was changed from

==========
Refactor VideoTrackRecorder into smaller classes	

This CL does a refactor taking each encoder implementation into its own
file. Additionally CanEncodeAlphaChannelForTesting() is removed as it
is no longer necessary.

BUG=700433
==========

to

==========
Refactor VideoTrackRecorder into smaller classes	

This CL does a refactor taking each encoder implementation into its own
file. Additionally, CanEncodeAlphaChannelForTesting() is removed as it
is no longer necessary.

BUG=700433
==========

[emircan, 2017-04-05 21:13:30]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[emircan, 2017-04-05 21:13:44]

https://codereview.chromium.org/2793303003/diff/1/content/renderer/BUILD.gn
File content/renderer/BUILD.gn (right):

https://codereview.chromium.org/2793303003/diff/1/content/renderer/BUILD.gn#n...
content/renderer/BUILD.gn:700: "media_recorder/h264_encoder.h",
On 2017/04/05 01:23:59, mcasas wrote:
> These two should only be added to the sources
> if (rtc_use_h264)
> 
>
https://cs.chromium.org/chromium/src/content/renderer/BUILD.gn?type=cs&q=RTC_...

Done.

https://codereview.chromium.org/2793303003/diff/1/content/renderer/media_reco...
File content/renderer/media_recorder/h264_encoder.h (right):

https://codereview.chromium.org/2793303003/diff/1/content/renderer/media_reco...
content/renderer/media_recorder/h264_encoder.h:10: #if BUILDFLAG(RTC_USE_H264)
On 2017/04/05 01:23:59, mcasas wrote:
> Following my comment in BUILD.gn, you can do here
> 
> #if !BUILDFLAG(#if BUILDFLAG(RTC_USE_H264))
> #error RTC_USE_H264 should be defined.
> #endif

Done.

https://codereview.chromium.org/2793303003/diff/1/content/renderer/media_reco...
content/renderer/media_recorder/h264_encoder.h:43: // |encoding_thread_|.
On 2017/04/05 01:23:59, mcasas wrote:
> |encoding_thread_| (here and in l.48) is not around here
> anymore, please adapt the comment to say that it's in 
> the base class.

Done.

https://codereview.chromium.org/2793303003/diff/1/content/renderer/media_reco...
File content/renderer/media_recorder/video_track_recorder.h (right):

https://codereview.chromium.org/2793303003/diff/1/content/renderer/media_reco...
content/renderer/media_recorder/video_track_recorder.h:76: // is not enforced);
- uses an internal |encoding_task_runner_| for actual
On 2017/04/05 01:23:59, mcasas wrote:
> The dashes are supposed to go in each line: 
>
https://cs.chromium.org/chromium/src/content/renderer/media_recorder/video_tr...

Done.

[commit-bot: I haz the power, 2017-04-05 21:14:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emircan, 2017-04-05 21:16:13]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[emircan, 2017-04-05 21:16:21]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-04-05 21:17:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emircan, 2017-04-05 21:17:02]

emircan@chromium.org changed reviewers:
+ avi@chromium.org

[emircan, 2017-04-05 21:17:03]

avi@ PTAL at content/renderer/BUILD.gn.

[commit-bot: I haz the power, 2017-04-05 22:35:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 22:35:38]

Dry run: This issue passed the CQ dry run.

[emircan, 2017-04-07 18:30:44]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 18:31:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 18:49:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-07 18:49:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[emircan, 2017-04-07 20:01:42]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[emircan, 2017-04-07 20:01:49]

Patchset #3 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-04-07 20:03:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emircan, 2017-04-07 20:21:51]

emircan@chromium.org changed reviewers:
- avi@chromium.org

[emircan, 2017-04-07 20:23:02]

emircan@chromium.org changed reviewers:
+ avi@chromium.org, esprehn@chromium.org

[emircan, 2017-04-07 20:29:33]

emircan@chromium.org changed reviewers:
- esprehn@chromium.org

[emircan, 2017-04-07 20:29:43]

emircan@chromium.org changed reviewers:
- avi@chromium.org

[emircan, 2017-04-07 20:32:16]

emircan@chromium.org changed reviewers:
+ avi@chromium.org

[Avi (use Gerrit), 2017-04-07 20:53:42]

lgtm

stamp

[commit-bot: I haz the power, 2017-04-07 21:01:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-07 21:01:13]

Dry run: This issue passed the CQ dry run.

[emircan, 2017-04-07 21:02:17]

The CQ bit was checked by emircan@chromium.org

[emircan, 2017-04-07 21:02:17]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org
Link to the patchset: https://codereview.chromium.org/2793303003/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2017-04-07 21:03:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 21:53:27]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491598937001640,
"parent_rev": "4706b082954524b8d1a832a882879478dc8a3e15", "commit_rev":
"7f9c1f75fc3ddb2d9be2d7de0c8954fc0bb6dcfa"}

[commit-bot: I haz the power, 2017-04-07 21:54:18]

Description was changed from

==========
Refactor VideoTrackRecorder into smaller classes	

This CL does a refactor taking each encoder implementation into its own
file. Additionally, CanEncodeAlphaChannelForTesting() is removed as it
is no longer necessary.

BUG=700433
==========

to

==========
Refactor VideoTrackRecorder into smaller classes	

This CL does a refactor taking each encoder implementation into its own
file. Additionally, CanEncodeAlphaChannelForTesting() is removed as it
is no longer necessary.

BUG=700433

Review-Url: https://codereview.chromium.org/2793303003
Cr-Commit-Position: refs/heads/master@{#463015}
Committed:
https://chromium.googlesource.com/chromium/src/+/7f9c1f75fc3ddb2d9be2d7de0c89...
==========

[commit-bot: I haz the power, 2017-04-07 21:54:19]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/7f9c1f75fc3ddb2d9be2d7de0c89...