Handle HW encoder errors in VideoTrackRecorder

Handle HW encoder errors in VideoTrackRecorder

This CL adds callback to propagate errors from VEAEncoder to VideoTrackRecorder
so that the recording session can be reinitialized. 

BUG=676424
TEST=Added unittest. WebRTC quality browser tests now pass on Mac H264.

Review-Url: https://codereview.chromium.org/2804843002
Cr-Commit-Position: refs/heads/master@{#462366}
Committed: https://chromium.googlesource.com/chromium/src/+/75762009cfc628424a8af508b91418ce912e0506

[emircan, 2017-04-05 19:53:51]

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

[commit-bot: I haz the power, 2017-04-05 19:54:22]

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 19:54:39]

Patchset #1 (id:1) has been deleted

[emircan, 2017-04-05 19:57:33]

Description was changed from

==========
error fb

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

to

==========
Handle HW encoder errors in VideoTrackRecorder

This CL adds callback to propagate errors from VEAEncoder to VideoTrackRecorder
so that the recording session can be reinitialized. 

BUG=676424
TEST=Added unittest. WebRTC quality browser tests now pass.
==========

[emircan, 2017-04-05 19:57:33]

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

[emircan, 2017-04-05 20:00:22]

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

[commit-bot: I haz the power, 2017-04-05 20:00:32]

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 20:01:19]

Patchset #1 (id:20001) has been deleted

[emircan, 2017-04-05 20:01:32]

Description was changed from

==========
Handle HW encoder errors in VideoTrackRecorder

This CL adds callback to propagate errors from VEAEncoder to VideoTrackRecorder
so that the recording session can be reinitialized. 

BUG=676424
TEST=Added unittest. WebRTC quality browser tests now pass.
==========

to

==========
Handle HW encoder errors in VideoTrackRecorder

This CL adds callback to propagate errors from VEAEncoder to VideoTrackRecorder
so that the recording session can be reinitialized. 

BUG=676424
TEST=Added unittest. WebRTC quality browser tests now pass on Mac H264.
==========

[emircan, 2017-04-05 20:02:22]

PTAL.

[emircan, 2017-04-05 20:03:16]

The CQ bit was unchecked by emircan@chromium.org

[mcasas, 2017-04-05 21:44:33]

https://codereview.chromium.org/2804843002/diff/40001/content/renderer/media_...
File content/renderer/media_recorder/video_track_recorder.cc (right):

https://codereview.chromium.org/2804843002/diff/40001/content/renderer/media_...
content/renderer/media_recorder/video_track_recorder.cc:1365:
base::Bind(initialize_encoder_callback_, allow_vea_encoder)),
...and here
s|allow_vea_encoder|false  /*allow_vea_encoder*/|

https://codereview.chromium.org/2804843002/diff/40001/content/renderer/media_...
File content/renderer/media_recorder/video_track_recorder.h (right):

https://codereview.chromium.org/2804843002/diff/40001/content/renderer/media_...
content/renderer/media_recorder/video_track_recorder.h:57: using OnErrorCB =
base::Callback<void(bool allow_vea_encoder)>;
Actually we don't need the parameter |allow_vea_encoder|
becuase every time someone calls OnError(), it's understood
that this parameter is false, IOW when someone calls
VideoTrackRecorder::OnError(), it's understood that there
has been an error.

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

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

[emircan, 2017-04-05 21:52:56]

https://codereview.chromium.org/2804843002/diff/40001/content/renderer/media_...
File content/renderer/media_recorder/video_track_recorder.cc (right):

https://codereview.chromium.org/2804843002/diff/40001/content/renderer/media_...
content/renderer/media_recorder/video_track_recorder.cc:1365:
base::Bind(initialize_encoder_callback_, allow_vea_encoder)),
On 2017/04/05 21:44:33, mcasas wrote:
> ...and here
> s|allow_vea_encoder|false  /*allow_vea_encoder*/|

Done.

https://codereview.chromium.org/2804843002/diff/40001/content/renderer/media_...
File content/renderer/media_recorder/video_track_recorder.h (right):

https://codereview.chromium.org/2804843002/diff/40001/content/renderer/media_...
content/renderer/media_recorder/video_track_recorder.h:57: using OnErrorCB =
base::Callback<void(bool allow_vea_encoder)>;
On 2017/04/05 21:44:33, mcasas wrote:
> Actually we don't need the parameter |allow_vea_encoder|
> becuase every time someone calls OnError(), it's understood
> that this parameter is false, IOW when someone calls
> VideoTrackRecorder::OnError(), it's understood that there
> has been an error.

Done. Initially, I worked on a generic OnError() method that every encoder
implementation can call, but later realized that those cases aren't available in
SW. This param was left from then.

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

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-05 22:00:43]

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

[commit-bot: I haz the power, 2017-04-05 22:00:44]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mcasas, 2017-04-05 22:43:40]

lgtm with suggestions

https://codereview.chromium.org/2804843002/diff/60001/content/renderer/media_...
File content/renderer/media_recorder/video_track_recorder.h (right):

https://codereview.chromium.org/2804843002/diff/60001/content/renderer/media_...
content/renderer/media_recorder/video_track_recorder.h:57: using OnErrorCB =
base::Callback<void()>;
This is a base::Closure;

https://cs.chromium.org/chromium/src/base/callback_forward.h?type=cs&l=33

https://codereview.chromium.org/2804843002/diff/60001/content/renderer/media_...
File content/renderer/media_recorder/video_track_recorder_unittest.cc (right):

https://codereview.chromium.org/2804843002/diff/60001/content/renderer/media_...
content/renderer/media_recorder/video_track_recorder_unittest.cc:309: // Insert
OnError() call.
Superfluous comment?

Also, in this test case we don't really test much, IOW:
encode a frame, see a keyframe appearing, insert an
error, and then... just wait for another key frame? Any
other observable quality that would lead us to see the
expected changes? (Maybe we can see the track being
disconnected-reconnected?)

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

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

[emircan, 2017-04-05 23:04:36]

https://codereview.chromium.org/2804843002/diff/60001/content/renderer/media_...
File content/renderer/media_recorder/video_track_recorder.h (right):

https://codereview.chromium.org/2804843002/diff/60001/content/renderer/media_...
content/renderer/media_recorder/video_track_recorder.h:57: using OnErrorCB =
base::Callback<void()>;
On 2017/04/05 22:43:40, mcasas wrote:
> This is a base::Closure;
> 
> https://cs.chromium.org/chromium/src/base/callback_forward.h?type=cs&l=33

Done.

https://codereview.chromium.org/2804843002/diff/60001/content/renderer/media_...
File content/renderer/media_recorder/video_track_recorder_unittest.cc (right):

https://codereview.chromium.org/2804843002/diff/60001/content/renderer/media_...
content/renderer/media_recorder/video_track_recorder_unittest.cc:309: // Insert
OnError() call.
On 2017/04/05 22:43:40, mcasas wrote:
> Superfluous comment?
> 
> Also, in this test case we don't really test much, IOW:
> encode a frame, see a keyframe appearing, insert an
> error, and then... just wait for another key frame? Any
> other observable quality that would lead us to see the
> expected changes? (Maybe we can see the track being
> disconnected-reconnected?)

Right, it can have more. I added checks to make sure that encoder instance is
reset after OnError(), see HasEncoderInstance() calls. Following track events
aren't applicable here as MockMediaStreamVideoTrack isn't available.

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

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-05 23:11:19]

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

[commit-bot: I haz the power, 2017-04-05 23:11:20]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[emircan, 2017-04-05 23:16:49]

The CQ bit was checked by emircan@chromium.org

[emircan, 2017-04-05 23:16:50]

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/2804843002/#ps80001
(title: " ")

[commit-bot: I haz the power, 2017-04-05 23:17:37]

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-05 23:33:44]

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

[commit-bot: I haz the power, 2017-04-05 23:33:45]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[emircan, 2017-04-06 03:23:45]

The CQ bit was checked by emircan@chromium.org

[commit-bot: I haz the power, 2017-04-06 03:24:06]

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

[emircan, 2017-04-06 03:26:37]

The CQ bit was unchecked by emircan@chromium.org

[emircan, 2017-04-06 03:26:53]

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

[commit-bot: I haz the power, 2017-04-06 03:27:05]

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-06 03:34:52]

The CQ bit was unchecked by emircan@chromium.org

[emircan, 2017-04-06 03:57:05]

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

[emircan, 2017-04-06 03:57:12]

Patchset #4 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-04-06 03:57:27]

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-06 04:03:35]

Patchset #3 (id:80001) has been deleted

[emircan, 2017-04-06 04:59:50]

The CQ bit was unchecked by emircan@chromium.org

[emircan, 2017-04-06 04:59:51]

The CQ bit was checked by emircan@chromium.org

[emircan, 2017-04-06 04:59:52]

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/2804843002/#ps120001
(title: " ")

[commit-bot: I haz the power, 2017-04-06 05:00:13]

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-06 05:36:28]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491454791996260,
"parent_rev": "57643b09e8b1558e599d51ceb0ae996ab50e1aae", "commit_rev":
"75762009cfc628424a8af508b91418ce912e0506"}

[commit-bot: I haz the power, 2017-04-06 05:37:46]

Description was changed from

==========
Handle HW encoder errors in VideoTrackRecorder

This CL adds callback to propagate errors from VEAEncoder to VideoTrackRecorder
so that the recording session can be reinitialized. 

BUG=676424
TEST=Added unittest. WebRTC quality browser tests now pass on Mac H264.
==========

to

==========
Handle HW encoder errors in VideoTrackRecorder

This CL adds callback to propagate errors from VEAEncoder to VideoTrackRecorder
so that the recording session can be reinitialized. 

BUG=676424
TEST=Added unittest. WebRTC quality browser tests now pass on Mac H264.

Review-Url: https://codereview.chromium.org/2804843002
Cr-Commit-Position: refs/heads/master@{#462366}
Committed:
https://chromium.googlesource.com/chromium/src/+/75762009cfc628424a8af508b914...
==========

[commit-bot: I haz the power, 2017-04-06 05:37:47]

Committed patchset #3 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/75762009cfc628424a8af508b914...