Copy NV12 picture buffers on demand on the main thread.

Copy NV12 picture buffers on demand on the main thread.

If the image is going to be displayed in an overlay then no copy needs
to happen, because the video processor can take it directly. However
if it's used in GL then it needs to be copied to a texture with the
correct bind flags using the video processor.

BUG=717813
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

Review-Url: https://codereview.chromium.org/2859723002
Cr-Commit-Position: refs/heads/master@{#475162}
Committed: https://chromium.googlesource.com/chromium/src/+/a361008fed7d9652a62fb548c7ea9a89fc3d2ce5

[jbauman, 2017-05-03 00:18:27]

Description was changed from

==========
Copy NV12 picture buffers on demand on the main thread.

If the image is going to be displayed in an overlay then no copy needs
to happen, because the video processor can take it directly. However
if it's used in GL then it needs to be copied to a texture with the
correct bind flags using the video processor.
==========

to

==========
Copy NV12 picture buffers on demand on the main thread.

If the image is going to be displayed in an overlay then no copy needs
to happen, because the video processor can take it directly. However
if it's used in GL then it needs to be copied to a texture with the
correct bind flags using the video processor.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
==========

[jbauman, 2017-05-03 00:18:32]

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

[commit-bot: I haz the power, 2017-05-03 00:19:27]

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-05-03 01:00:18]

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

[commit-bot: I haz the power, 2017-05-03 01:00:19]

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

[jbauman, 2017-05-03 01:14:29]

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

[commit-bot: I haz the power, 2017-05-03 01:15:06]

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-05-03 02:48:54]

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

[commit-bot: I haz the power, 2017-05-03 02:48:55]

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

[jbauman, 2017-05-04 01:38:49]

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

[commit-bot: I haz the power, 2017-05-04 01:39:20]

Dry run: CQ is trying da patch.

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

[jbauman, 2017-05-04 01:47:59]

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

[commit-bot: I haz the power, 2017-05-04 01:48:17]

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-05-04 01:51:09]

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

[commit-bot: I haz the power, 2017-05-04 01:51:10]

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

[jbauman, 2017-05-04 01:57:09]

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

[commit-bot: I haz the power, 2017-05-04 01:57:37]

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-05-04 03:02:46]

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

[commit-bot: I haz the power, 2017-05-04 03:02:46]

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

[jbauman, 2017-05-06 01:33:08]

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

[commit-bot: I haz the power, 2017-05-06 01:33:41]

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-05-06 01:59:11]

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

[commit-bot: I haz the power, 2017-05-06 01:59:12]

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

[jbauman, 2017-05-17 23:23:21]

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

[commit-bot: I haz the power, 2017-05-17 23:24:51]

Dry run: CQ is trying da patch.

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

[jbauman, 2017-05-17 23:42:36]

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

[commit-bot: I haz the power, 2017-05-17 23:44:24]

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-05-18 00:32:31]

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

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

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

[jbauman, 2017-05-19 00:15:02]

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

[commit-bot: I haz the power, 2017-05-19 00:16:18]

Dry run: CQ is trying da patch.

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

[jbauman, 2017-05-19 02:43:35]

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

[commit-bot: I haz the power, 2017-05-19 02:44:35]

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-05-19 06:57:42]

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

[commit-bot: I haz the power, 2017-05-19 06:57:43]

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

[jbauman, 2017-05-22 23:43:48]

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

[commit-bot: I haz the power, 2017-05-22 23:44:42]

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-05-23 02:42:41]

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

[commit-bot: I haz the power, 2017-05-23 02:42:42]

Dry run: This issue passed the CQ dry run.

[jbauman, 2017-05-23 22:18:05]

Description was changed from

==========
Copy NV12 picture buffers on demand on the main thread.

If the image is going to be displayed in an overlay then no copy needs
to happen, because the video processor can take it directly. However
if it's used in GL then it needs to be copied to a texture with the
correct bind flags using the video processor.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

==========
Copy NV12 picture buffers on demand on the main thread.

If the image is going to be displayed in an overlay then no copy needs
to happen, because the video processor can take it directly. However
if it's used in GL then it needs to be copied to a texture with the
correct bind flags using the video processor.

BUG=717813
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
==========

[jbauman, 2017-05-23 22:18:15]

jbauman@chromium.org changed reviewers:
+ sandersd@chromium.org

[sandersd (OOO until July 31), 2017-05-23 23:23:31]

media changes lgtm.

I have nits about the command buffer changes, but someone else should be doing
that review.

https://codereview.chromium.org/2859723002/diff/200001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2859723002/diff/200001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9661: void
GLES2DecoderImpl::DoCopyTexImageIfNeeded(Texture* texture,
Perhaps this needs a new name? (Please update the comment on line 1923 in either
case.)

https://codereview.chromium.org/2859723002/diff/200001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9741: if
(image->BindTexImage(textarget)) {
This is same as the core of DoCopyTexImageIfNeeded(). Can
DoCopyTexImageIfNeeded() be reduce or refactored to share the implementation?

https://codereview.chromium.org/2859723002/diff/200001/media/gpu/dxva_picture...
File media/gpu/dxva_picture_buffer_win.cc (right):

https://codereview.chromium.org/2859723002/diff/200001/media/gpu/dxva_picture...
media/gpu/dxva_picture_buffer_win.cc:35: bool BindTexImage(unsigned target)
override { return true; }
I'd like a comment to explain why this makes sense.

[jbauman, 2017-05-24 01:19:00]

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

[commit-bot: I haz the power, 2017-05-24 01:20:06]

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-05-24 01:34:42]

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

[commit-bot: I haz the power, 2017-05-24 01:34:43]

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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jbauman, 2017-05-24 01:37:35]

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

[commit-bot: I haz the power, 2017-05-24 01:38:29]

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-05-24 06:58:55]

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

[commit-bot: I haz the power, 2017-05-24 06:58:56]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jbauman, 2017-05-24 21:59:02]

jbauman@chromium.org changed reviewers:
+ zmo@chromium.org

[jbauman, 2017-05-24 21:59:03]

zmo@: gpu/command_buffer review, please.

[Zhenyao Mo, 2017-05-24 22:19:29]

https://codereview.chromium.org/2859723002/diff/240001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2859723002/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9688:
RestoreCurrentTextureBindings(&state_, textarget);
You need to expend this to take in texture_unit parameter.  This function
assumes the binding that was changed was on texture unit 0.

[jbauman, 2017-05-25 00:36:56]

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

[commit-bot: I haz the power, 2017-05-25 00:37:39]

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-05-25 04:08:02]

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

[commit-bot: I haz the power, 2017-05-25 04:08:03]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jbauman, 2017-05-25 04:18:16]

PTAL.

On 2017/05/24 22:19:29, Zhenyao Mo wrote:
>
https://codereview.chromium.org/2859723002/diff/240001/gpu/command_buffer/ser...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/2859723002/diff/240001/gpu/command_buffer/ser...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:9688:
> RestoreCurrentTextureBindings(&state_, textarget);
> You need to expend this to take in texture_unit parameter.  This function
> assumes the binding that was changed was on texture unit 0.

I changed it so that it never restores the binding if the active texture was
changed. This makes it match the current behavior, though I'm not sure if that
behavior's always correct.

[Zhenyao Mo, 2017-05-25 17:21:58]

https://codereview.chromium.org/2859723002/diff/260001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2859723002/diff/260001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9679: glBindTexture(textarget,
texture->service_id());
I am still concerned. Here we changed the tex binding for textture_unit, but
never restore it.  Won't this be problematic?

It's not your CL causing this, but since you are working on it, it would be nice
to just fix it.

[jbauman, 2017-05-25 22:21:48]

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

[commit-bot: I haz the power, 2017-05-25 22:23:03]

Dry run: CQ is trying da patch.

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

[jbauman, 2017-05-26 00:54:00]

PTAL.

On 2017/05/25 17:21:58, Zhenyao Mo wrote:
>
https://codereview.chromium.org/2859723002/diff/260001/gpu/command_buffer/ser...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/2859723002/diff/260001/gpu/command_buffer/ser...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:9679: glBindTexture(textarget,
> texture->service_id());
> I am still concerned. Here we changed the tex binding for textture_unit, but
> never restore it.  Won't this be problematic?
> 
> It's not your CL causing this, but since you are working on it, it would be
nice
> to just fix it.

Ok, fixed it to handle the case where the current active texture isn't texture
0. And for the case where texture_unit is set in the call to
DoBindOrCopyTexImageIfNeeded, the bindings should be restored after the draw
completes anyway, so it's still not needed.

[Zhenyao Mo, 2017-05-26 01:11:07]

LGTM
Thanks

https://codereview.chromium.org/2859723002/diff/280001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2859723002/diff/280001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:1927: // or bind happened.
Can you add it here that it is the caller's responsibility to recover the active
texture and |textarget| binding?

[jbauman, 2017-05-26 01:35:33]

jbauman@chromium.org changed reviewers:
+ kbr@chromium.org

[jbauman, 2017-05-26 01:35:34]

+kbr@ for third_party/khronos OWNERS review.

[Zhenyao Mo, 2017-05-26 01:59:42]

On 2017/05/26 01:35:34, jbauman wrote:
> +kbr@ for third_party/khronos OWNERS review.

kbr is out until after the long weekend

[jbauman, 2017-05-26 02:00:34]

jbauman@chromium.org changed reviewers:
+ piman@chromium.org

[jbauman, 2017-05-26 02:00:35]

Ok, then +piman@ for third_party/khronos OWNERS review

[piman, 2017-05-26 16:11:33]

lgtm

[jbauman, 2017-05-26 20:24:05]

The CQ bit was checked by jbauman@chromium.org

[jbauman, 2017-05-26 20:24:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
sandersd@chromium.org, zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/2859723002/#ps300001
(title: "change comment")

[commit-bot: I haz the power, 2017-05-26 20:24:28]

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-05-26 22:20:26]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1495830245147170,
"parent_rev": "cb0593b3d8106439bdd6f20ef62f1319529f02f8", "commit_rev":
"a361008fed7d9652a62fb548c7ea9a89fc3d2ce5"}

[commit-bot: I haz the power, 2017-05-26 22:20:38]

Description was changed from

==========
Copy NV12 picture buffers on demand on the main thread.

If the image is going to be displayed in an overlay then no copy needs
to happen, because the video processor can take it directly. However
if it's used in GL then it needs to be copied to a texture with the
correct bind flags using the video processor.

BUG=717813
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

==========
Copy NV12 picture buffers on demand on the main thread.

If the image is going to be displayed in an overlay then no copy needs
to happen, because the video processor can take it directly. However
if it's used in GL then it needs to be copied to a texture with the
correct bind flags using the video processor.

BUG=717813
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

Review-Url: https://codereview.chromium.org/2859723002
Cr-Commit-Position: refs/heads/master@{#475162}
Committed:
https://chromium.googlesource.com/chromium/src/+/a361008fed7d9652a62fb548c7ea...
==========

[commit-bot: I haz the power, 2017-05-26 22:20:40]

Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/a361008fed7d9652a62fb548c7ea...