Fix multiple Initialize() calls and reenable VEA usage in MediaRecorder for Win8

Fix multiple Initialize() calls and reenable VEA usage in MediaRecorder for Win8

This Cl does two things:
- Reverts the CL that disables VEA usage until flaky test is addressed
https://codereview.chromium.org/2727633008
- Fixes the underlying issue of flakiness by not allowing |encoder_| to be 
initialized multiple times.

BUG=698441
TEST=Local run of tests consistently pass.

Review-Url: https://codereview.chromium.org/2775453003
Cr-Commit-Position: refs/heads/master@{#459598}
Committed: https://chromium.googlesource.com/chromium/src/+/1ea19d13673e5fbe40925ba23a49f1bfa6877287

[emircan, 2017-03-23 18:00:54]

Description was changed from

==========
Revert of Disable VEA usage in MediaRecorder until flaky test is addressed
(patchset #2 id:20001 of https://codereview.chromium.org/2727633008/ )

Reason for revert:
I believe this has the same underlying issue as
https://bugs.chromium.org/p/chromium/issues/detail?id=701030, which is fixed. I
will re-enable the tests to see the effects.

Original issue's description:
> Disable VEA usage in MediaRecorder until flaky test is addressed
>
> BUG=698441
>
> Review-Url: https://codereview.chromium.org/2727633008
> Cr-Commit-Position: refs/heads/master@{#454759}
> Committed:
https://chromium.googlesource.com/chromium/src/+/26c6a0e07e73b1e9d357edcea555...

TBR=mcasas@chromium.org
BUG=698441

Review-Url: https://codereview.chromium.org/2754473008
Cr-Commit-Position: refs/heads/master@{#457622}
Committed:
https://chromium.googlesource.com/chromium/src/+/8086b9ac7fe4598925a84e616976...

patch from issue 2754473008 at patchset 1 (http://crrev.com/2754473008#ps1)
==========

to

==========
Fix multiple Initialize() calls and reenable VEA usage in MediaRecorder for Win8

BUG=698441
==========

[emircan, 2017-03-23 18:01:10]

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

[commit-bot: I haz the power, 2017-03-23 18:02:43]

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

[emircan, 2017-03-23 18:05:50]

Description was changed from

==========
Fix multiple Initialize() calls and reenable VEA usage in MediaRecorder for Win8

BUG=698441
==========

to

==========
Fix multiple Initialize() calls and reenable VEA usage in MediaRecorder for Win8

This Cl does two things:
- Reverts the CL that disables VEA usage until flaky test is addressed
https://codereview.chromium.org/2727633008
- Fixes the underlying issue of flakiness by not allowing |encoder_| to be 
initialized multiple times.

BUG=698441
TEST=Local run of tests consistently pass.
==========

[emircan, 2017-03-23 18:06:00]

PTAL.

[commit-bot: I haz the power, 2017-03-23 18:15:07]

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

[commit-bot: I haz the power, 2017-03-23 18:15:08]

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

[emircan, 2017-03-23 18:23:16]

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

[emircan, 2017-03-23 18:23:40]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-03-23 18:23:57]

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-03-23 19:15:23]

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

[commit-bot: I haz the power, 2017-03-23 19:15:24]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[mcasas, 2017-03-23 21:14:08]

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

https://codereview.chromium.org/2775453003/diff/40001/content/renderer/media_...
content/renderer/media_recorder/video_track_recorder.cc:1305: }
I'd do:

  // Avoid reinitializing |encoder_|, https://crbug.com/698441.
  DCHECK(RenderThread::Get());
  if (encoder_ && RenderThread::Get()->GetIOTaskRunner()) {
    RenderThread::Get()->GetIOTaskRunner()->PostTask(
        FROM_HERE, base::Bind(&VideoTrackRecorder::Encoder::StartFrameEncode,
                              encoder_, frame, capture_time));
    return;
  }

If DCHECK() doesn't verify in unit tests, add a 
MockRenderThread [1], wdyt?  RenderThread 
conveys clearlier what you want to do than 
ChildProcess IMHO.

[1]
https://cs.chromium.org/chromium/src/content/public/test/mock_render_thread.h...

[emircan, 2017-03-24 18:13:47]

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

[emircan, 2017-03-24 18:14:11]

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

https://codereview.chromium.org/2775453003/diff/40001/content/renderer/media_...
content/renderer/media_recorder/video_track_recorder.cc:1305: }
On 2017/03/23 21:14:08, mcasas wrote:
> I'd do:
> 
>   // Avoid reinitializing |encoder_|, https://crbug.com/698441.
>   DCHECK(RenderThread::Get());
>   if (encoder_ && RenderThread::Get()->GetIOTaskRunner()) {
>     RenderThread::Get()->GetIOTaskRunner()->PostTask(
>         FROM_HERE, base::Bind(&VideoTrackRecorder::Encoder::StartFrameEncode,
>                               encoder_, frame, capture_time));
>     return;
>   }
> 
> If DCHECK() doesn't verify in unit tests, add a 
> MockRenderThread [1], wdyt?  RenderThread 
> conveys clearlier what you want to do than 
> ChildProcess IMHO.
> 
> [1]
>
https://cs.chromium.org/chromium/src/content/public/test/mock_render_thread.h...

Done. RenderThreadImpl directly calls ChildProcess::io_task_runner() so I wanted
to skip the middle step, see
https://cs.chromium.org/chromium/src/content/renderer/render_thread_impl.cc?r....


Also, we should still return if |!RenderThread::Get()->GetIOTaskRunner()| so I
nested the loop.

[commit-bot: I haz the power, 2017-03-24 18:14:40]

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

[emircan, 2017-03-24 18:14:45]

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

[commit-bot: I haz the power, 2017-03-24 18:15:50]

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-03-24 19:36:17]

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

[commit-bot: I haz the power, 2017-03-24 19:36:18]

Dry run: This issue passed the CQ dry run.

[emircan, 2017-03-24 21:04:31]

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

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

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

[emircan, 2017-03-24 21:06:37]

I changed it to drop all the "initializer sink" frames as discussed offline.
PTAL.

[mcasas, 2017-03-24 21:06:41]

On 2017/03/24 18:14:11, emircan wrote:
>
https://codereview.chromium.org/2775453003/diff/40001/content/renderer/media_...
> File content/renderer/media_recorder/video_track_recorder.cc (right):
> 
>
https://codereview.chromium.org/2775453003/diff/40001/content/renderer/media_...
> content/renderer/media_recorder/video_track_recorder.cc:1305: }
> On 2017/03/23 21:14:08, mcasas wrote:
> > I'd do:
> > 
> >   // Avoid reinitializing |encoder_|, https://crbug.com/698441.
> >   DCHECK(RenderThread::Get());
> >   if (encoder_ && RenderThread::Get()->GetIOTaskRunner()) {
> >     RenderThread::Get()->GetIOTaskRunner()->PostTask(
> >         FROM_HERE,
base::Bind(&VideoTrackRecorder::Encoder::StartFrameEncode,
> >                               encoder_, frame, capture_time));
> >     return;
> >   }
> > 
> > If DCHECK() doesn't verify in unit tests, add a 
> > MockRenderThread [1], wdyt?  RenderThread 
> > conveys clearlier what you want to do than 
> > ChildProcess IMHO.
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/content/public/test/mock_render_thread.h...
> 
> Done. RenderThreadImpl directly calls ChildProcess::io_task_runner() so I
wanted
> to skip the middle step, see
>
https://cs.chromium.org/chromium/src/content/renderer/render_thread_impl.cc?r....
> 
> 
> Also, we should still return if |!RenderThread::Get()->GetIOTaskRunner()| so I
> nested the loop.

ps5 (dropping frames, per offline discussion) lgm

[emircan, 2017-03-24 21:28:14]

The CQ bit was unchecked by emircan@chromium.org

[emircan, 2017-03-24 21:28:14]

The CQ bit was checked by emircan@chromium.org

[commit-bot: I haz the power, 2017-03-24 21:28:27]

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-03-24 21:28:28]

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

[commit-bot: I haz the power, 2017-03-24 21:28:29]

No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[emircan, 2017-03-24 23:26:29]

The CQ bit was checked by emircan@chromium.org

[commit-bot: I haz the power, 2017-03-24 23:26:57]

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-03-24 23:26:58]

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

[commit-bot: I haz the power, 2017-03-24 23:26:59]

No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[mcasas, 2017-03-24 23:28:15]

On 2017/03/24 21:28:29, commit-bot: I haz the power wrote:
> No L-G-T-M from a valid reviewer yet.
> CQ run can only be started once the patch has received an L-G-T-M from a full
> committer.
> Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
> full super star committer.
> Committers are members of the group "project-chromium-committers".
> Note that this has nothing to do with OWNERS files.

I meant lgtm before (but somehow the t got dropped).

[emircan, 2017-03-24 23:28:40]

The CQ bit was checked by emircan@chromium.org

[commit-bot: I haz the power, 2017-03-24 23:29:17]

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-03-24 23:35:07]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490398120002140,
"parent_rev": "699c4507d3b4fea0660627db4bef5e500f28545c", "commit_rev":
"1ea19d13673e5fbe40925ba23a49f1bfa6877287"}

[commit-bot: I haz the power, 2017-03-24 23:35:54]

Description was changed from

==========
Fix multiple Initialize() calls and reenable VEA usage in MediaRecorder for Win8

This Cl does two things:
- Reverts the CL that disables VEA usage until flaky test is addressed
https://codereview.chromium.org/2727633008
- Fixes the underlying issue of flakiness by not allowing |encoder_| to be 
initialized multiple times.

BUG=698441
TEST=Local run of tests consistently pass.
==========

to

==========
Fix multiple Initialize() calls and reenable VEA usage in MediaRecorder for Win8

This Cl does two things:
- Reverts the CL that disables VEA usage until flaky test is addressed
https://codereview.chromium.org/2727633008
- Fixes the underlying issue of flakiness by not allowing |encoder_| to be 
initialized multiple times.

BUG=698441
TEST=Local run of tests consistently pass.

Review-Url: https://codereview.chromium.org/2775453003
Cr-Commit-Position: refs/heads/master@{#459598}
Committed:
https://chromium.googlesource.com/chromium/src/+/1ea19d13673e5fbe40925ba23a49...
==========

[commit-bot: I haz the power, 2017-03-24 23:35:55]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/1ea19d13673e5fbe40925ba23a49...