Fix broken SurfaceView usage on < M devices.

Fix broken SurfaceView usage on < M devices.

This partially reverts commit 4b632fce277d847aa2c5a533478da715086c0b55
which added support for SurfaceView changes without a restart on M+
devices. Unfortunately we can't use SetSurface() prior to initialize
on <M devices since the VDA hasn't been created by this point. The
surface must be provided in the initial config.

BUG=662251
TEST=enter/exit fullscreen on <M and >M device.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/5b3c4d675143972351fe4e2d5c3ac903455f0a64
Cr-Commit-Position: refs/heads/master@{#432423}

[DaleCurtis, 2016-11-15 23:57:47]

Description was changed from

==========
Fix broken SurfaceView usage on < M devices.

This partially reverts commit 4b632fce277d847aa2c5a533478da715086c0b55
which added support for SurfaceView changes without a restart on M+
devices. Unfortunately we can't use SetSurface() prior to initialize
on <M devices since the VDA hasn't been created by this point. The
surface must be provided in the initial config.

BUG=662251
TEST=enter/exit fullscreen on <M and >M device.
==========

to

==========
Fix broken SurfaceView usage on < M devices.

This partially reverts commit 4b632fce277d847aa2c5a533478da715086c0b55
which added support for SurfaceView changes without a restart on M+
devices. Unfortunately we can't use SetSurface() prior to initialize
on <M devices since the VDA hasn't been created by this point. The
surface must be provided in the initial config.

BUG=662251
TEST=enter/exit fullscreen on <M and >M device.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[DaleCurtis, 2016-11-15 23:58:12]

dalecurtis@chromium.org changed reviewers:
+ dcheng@chromium.org, watk@chromium.org

[DaleCurtis, 2016-11-15 23:58:13]

Thanks for catching this Chris. +dcheng for media/gpu/ipc.

[dcheng, 2016-11-16 00:58:45]

https://codereview.chromium.org/2507463004/diff/1/media/gpu/android_video_dec...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2507463004/diff/1/media/gpu/android_video_dec...
media/gpu/android_video_decode_accelerator.cc:394: DCHECK(!pending_surface_id_);
This isn't a new issue, but it makes me concerned that we're doing all these
DCHECKs on values that ultimately are controlled by the renderer, which is less
privileged by the GPU process. Will failing any of these DCHECKs horribly break
things in the gpu?

https://codereview.chromium.org/2507463004/diff/1/media/gpu/android_video_dec...
media/gpu/android_video_decode_accelerator.cc:404:
DCHECK(g_avda_manager.Get().AllocateSurface(config_.surface_id, this));
Why do we call this in a DCHECK()? Isn't it a no-op anyway?

[DaleCurtis, 2016-11-16 01:04:29]

https://codereview.chromium.org/2507463004/diff/1/media/gpu/android_video_dec...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2507463004/diff/1/media/gpu/android_video_dec...
media/gpu/android_video_decode_accelerator.cc:394: DCHECK(!pending_surface_id_);
On 2016/11/16 at 00:58:45, dcheng wrote:
> This isn't a new issue, but it makes me concerned that we're doing all these
DCHECKs on values that ultimately are controlled by the renderer, which is less
privileged by the GPU process. Will failing any of these DCHECKs horribly break
things in the gpu?

pending_surface_id_ is a base::Optional() so this is just checking that we have
no value for it. Since VDA construction and Initialize() are called in one fell
swoop, this is a textbook "impossible" DCHECK :) None of these DCHECKS should
affect anything in release mode even if they're failed.

If the surface is unknown by the surface manager we'll fail to create a surface
and fail the VDA creation / setSurface call via notifyError().

https://codereview.chromium.org/2507463004/diff/1/media/gpu/android_video_dec...
media/gpu/android_video_decode_accelerator.cc:404:
DCHECK(g_avda_manager.Get().AllocateSurface(config_.surface_id, this));
On 2016/11/16 at 00:58:45, dcheng wrote:
> Why do we call this in a DCHECK()? Isn't it a no-op anyway?

It is with the current code, it's prevention against future code modifications
to AllocateSurface where it might try to manage allocations for SurfaceTextures.

[dcheng, 2016-11-16 01:06:33]

https://codereview.chromium.org/2507463004/diff/1/media/gpu/android_video_dec...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2507463004/diff/1/media/gpu/android_video_dec...
media/gpu/android_video_decode_accelerator.cc:404:
DCHECK(g_avda_manager.Get().AllocateSurface(config_.surface_id, this));
On 2016/11/16 01:04:29, DaleCurtis wrote:
> On 2016/11/16 at 00:58:45, dcheng wrote:
> > Why do we call this in a DCHECK()? Isn't it a no-op anyway?
> 
> It is with the current code, it's prevention against future code modifications
> to AllocateSurface where it might try to manage allocations for
SurfaceTextures.

It seems weird to call something called AllocateX in a dcheck though: at most, I
would have expected this to be querying some state bits?

[watk, 2016-11-16 01:09:52]

On 2016/11/16 01:06:33, dcheng wrote:
>
https://codereview.chromium.org/2507463004/diff/1/media/gpu/android_video_dec...
> File media/gpu/android_video_decode_accelerator.cc (right):
> 
>
https://codereview.chromium.org/2507463004/diff/1/media/gpu/android_video_dec...
> media/gpu/android_video_decode_accelerator.cc:404:
> DCHECK(g_avda_manager.Get().AllocateSurface(config_.surface_id, this));
> On 2016/11/16 01:04:29, DaleCurtis wrote:
> > On 2016/11/16 at 00:58:45, dcheng wrote:
> > > Why do we call this in a DCHECK()? Isn't it a no-op anyway?
> > 
> > It is with the current code, it's prevention against future code
modifications
> > to AllocateSurface where it might try to manage allocations for
> SurfaceTextures.
> 
> It seems weird to call something called AllocateX in a dcheck though: at most,
I
> would have expected this to be querying some state bits?

You can delete this if you want because I can unit test this fact in my upcoming
CL.

[dcheng, 2016-11-16 01:13:19]

In any case, IPC LGTM, the DCHECK seems weird to me, but it's not blocking
either.

[watk, 2016-11-16 01:13:56]

lgtm

[DaleCurtis, 2016-11-16 01:16:15]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2016-11-16 01:16:15]

Thanks for review.

https://codereview.chromium.org/2507463004/diff/1/media/gpu/android_video_dec...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2507463004/diff/1/media/gpu/android_video_dec...
media/gpu/android_video_decode_accelerator.cc:404:
DCHECK(g_avda_manager.Get().AllocateSurface(config_.surface_id, this));
On 2016/11/16 at 01:06:33, dcheng wrote:
> On 2016/11/16 01:04:29, DaleCurtis wrote:
> > On 2016/11/16 at 00:58:45, dcheng wrote:
> > > Why do we call this in a DCHECK()? Isn't it a no-op anyway?
> > 
> > It is with the current code, it's prevention against future code
modifications
> > to AllocateSurface where it might try to manage allocations for
SurfaceTextures.
> 
> It seems weird to call something called AllocateX in a dcheck though: at most,
I would have expected this to be querying some state bits?

Will leave for now and let watk@ remove with upcoming changes. This is mostly
going to be scrapped in the near future.

[commit-bot: I haz the power, 2016-11-16 01:16:45]

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, 2016-11-16 01:31:03]

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

[commit-bot: I haz the power, 2016-11-16 01:31:04]

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...)

[DaleCurtis, 2016-11-16 01:33:02]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2016-11-16 01:33:34]

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, 2016-11-16 01:47:28]

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

[commit-bot: I haz the power, 2016-11-16 01:47:30]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[watk, 2016-11-16 03:04:42]

The CQ bit was checked by watk@chromium.org

[commit-bot: I haz the power, 2016-11-16 03:05:11]

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, 2016-11-16 04:27:03]

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

[commit-bot: I haz the power, 2016-11-16 04:27:04]

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

[DaleCurtis, 2016-11-16 06:28:49]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2016-11-16 06:29:07]

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, 2016-11-16 08:13:40]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-11-16 08:18:37]

Description was changed from

==========
Fix broken SurfaceView usage on < M devices.

This partially reverts commit 4b632fce277d847aa2c5a533478da715086c0b55
which added support for SurfaceView changes without a restart on M+
devices. Unfortunately we can't use SetSurface() prior to initialize
on <M devices since the VDA hasn't been created by this point. The
surface must be provided in the initial config.

BUG=662251
TEST=enter/exit fullscreen on <M and >M device.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Fix broken SurfaceView usage on < M devices.

This partially reverts commit 4b632fce277d847aa2c5a533478da715086c0b55
which added support for SurfaceView changes without a restart on M+
devices. Unfortunately we can't use SetSurface() prior to initialize
on <M devices since the VDA hasn't been created by this point. The
surface must be provided in the initial config.

BUG=662251
TEST=enter/exit fullscreen on <M and >M device.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/5b3c4d675143972351fe4e2d5c3ac903455f0a64
Cr-Commit-Position: refs/heads/master@{#432423}
==========

[commit-bot: I haz the power, 2016-11-16 08:18:38]

Patchset 1 (id:??) landed as
https://crrev.com/5b3c4d675143972351fe4e2d5c3ac903455f0a64
Cr-Commit-Position: refs/heads/master@{#432423}