mac: Finish plumbing for Pepper3D Image Chromium.

mac: Finish plumbing for Pepper3D Image Chromium.

This allows Flash content to use the new Core Animation compositing path. This
CL includes the last plumbing required to turn on the feature. It will be turned
on in a separate CL to allow for a clean revert if needed.

BUG=602484
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/b13637b2ffc6aca611a407e943ffe587ab7e4aa2
Cr-Commit-Position: refs/heads/master@{#404330}

[erikchen, 2016-06-28 19:19:45]

Description was changed from

==========
hook up ppepper for native GMBs

Note that ui/gfx/size is illegal include.

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

to

==========
hook up ppepper for native GMBs

Note that ui/gfx/size is illegal include.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[erikchen, 2016-06-28 19:21:22]

Description was changed from

==========
hook up ppepper for native GMBs

Note that ui/gfx/size is illegal include.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
mac: Enable Pepper3D Image Chromium.

This allows Flash content to use the new Core Animation compositing path. This
CL includes the last plumbing required to turn on the feature. It will be turned
on in a separate CL to allow for a clean revert if needed.

BUG=602484
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[erikchen, 2016-06-28 19:23:30]

Description was changed from

==========
mac: Enable Pepper3D Image Chromium.

This allows Flash content to use the new Core Animation compositing path. This
CL includes the last plumbing required to turn on the feature. It will be turned
on in a separate CL to allow for a clean revert if needed.

BUG=602484
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
mac: Enable Pepper3D Image Chromium.

This allows Flash content to use the new Core Animation compositing path. This
CL includes the last plumbing required to turn on the feature. It will be turned
on in a separate CL to allow for a clean revert if needed.

BUG=602484
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[erikchen, 2016-06-28 19:50:33]

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

[erikchen, 2016-06-28 19:50:43]

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

[erikchen, 2016-06-28 19:50:44]

piman: Please review.

[commit-bot: I haz the power, 2016-06-28 19:52:09]

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, 2016-06-28 19:58:45]

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

[commit-bot: I haz the power, 2016-06-28 19:58:46]

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

[erikchen, 2016-06-28 20:39:13]

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

[commit-bot: I haz the power, 2016-06-28 20:40:25]

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

[piman, 2016-06-28 20:51:41]

https://codereview.chromium.org/2086363004/diff/80001/content/renderer/pepper...
File content/renderer/pepper/ppb_graphics_3d_impl.cc (right):

https://codereview.chromium.org/2086363004/diff/80001/content/renderer/pepper...
content/renderer/pepper/ppb_graphics_3d_impl.cc:192: gfx::Size(width(),
height()),
The size will be wrong here.

PPB_Graphics3D_Shared::ResizeBuffers is called on the plugin side, so the
updated size will never make it here (renderer side).

I think the right thing to do is to grab the size on the plugin side in
Graphics3D::DoSwapBuffers, send it together with the
PpapiHostMsg_PPBGraphics3D_SwapBuffers message, and forward it through here.

One question: what bad things could happen if this size is maliciously
incorrectly set?

[erikchen, 2016-06-28 20:52:00]

Description was changed from

==========
mac: Enable Pepper3D Image Chromium.

This allows Flash content to use the new Core Animation compositing path. This
CL includes the last plumbing required to turn on the feature. It will be turned
on in a separate CL to allow for a clean revert if needed.

BUG=602484
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
mac: Finish plumbing for Pepper3D Image Chromium.

This allows Flash content to use the new Core Animation compositing path. This
CL includes the last plumbing required to turn on the feature. It will be turned
on in a separate CL to allow for a clean revert if needed.

BUG=602484
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-06-28 20:52:22]

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

[commit-bot: I haz the power, 2016-06-28 20:52:26]

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

[erikchen, 2016-06-28 21:48:38]

On 2016/06/28 20:51:41, piman wrote:
>
https://codereview.chromium.org/2086363004/diff/80001/content/renderer/pepper...
> File content/renderer/pepper/ppb_graphics_3d_impl.cc (right):
> 
>
https://codereview.chromium.org/2086363004/diff/80001/content/renderer/pepper...
> content/renderer/pepper/ppb_graphics_3d_impl.cc:192: gfx::Size(width(),
> height()),
> The size will be wrong here.
> 
> PPB_Graphics3D_Shared::ResizeBuffers is called on the plugin side, so the
> updated size will never make it here (renderer side).
> 
> I think the right thing to do is to grab the size on the plugin side in
> Graphics3D::DoSwapBuffers, send it together with the
> PpapiHostMsg_PPBGraphics3D_SwapBuffers message, and forward it through here.
> 
> One question: what bad things could happen if this size is maliciously
> incorrectly set?

To the best of my knowledge, the only place the size is used is in non-overlay
mode (GL compositor), when texturing into the output IOSurface. Theoretically,
it should be possible to avoid passing the size through altogether since the GPU
process already knows it. (even for h264 video? not sure off the top of my
head.) Let me ask ccameron@ to see if he has further thoughts.

[ccameron, 2016-06-28 21:55:40]

On 2016/06/28 21:48:38, erikchen wrote:
> On 2016/06/28 20:51:41, piman wrote:
> >
>
https://codereview.chromium.org/2086363004/diff/80001/content/renderer/pepper...
> > File content/renderer/pepper/ppb_graphics_3d_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2086363004/diff/80001/content/renderer/pepper...
> > content/renderer/pepper/ppb_graphics_3d_impl.cc:192: gfx::Size(width(),
> > height()),
> > The size will be wrong here.
> > 
> > PPB_Graphics3D_Shared::ResizeBuffers is called on the plugin side, so the
> > updated size will never make it here (renderer side).
> > 
> > I think the right thing to do is to grab the size on the plugin side in
> > Graphics3D::DoSwapBuffers, send it together with the
> > PpapiHostMsg_PPBGraphics3D_SwapBuffers message, and forward it through here.
> > 
> > One question: what bad things could happen if this size is maliciously
> > incorrectly set?
> 
> To the best of my knowledge, the only place the size is used is in non-overlay
> mode (GL compositor), when texturing into the output IOSurface. Theoretically,
> it should be possible to avoid passing the size through altogether since the
GPU
> process already knows it. (even for h264 video? not sure off the top of my
> head.) Let me ask ccameron@ to see if he has further thoughts.

IIRC it's only used for normalizing texture coordinates (for rectangle textures)
when in GL compositor mode.

So, if this is wrong, you'll get a strangely-scaled texture -- it's not optional
(h264 sets it by a side-channel at
https://cs.chromium.org/chromium/src/cc/resources/video_resource_updater.cc?r...)

[erikchen, 2016-06-28 22:06:00]

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

[erikchen, 2016-06-28 22:06:28]

erikchen@chromium.org changed reviewers:
+ dcheng@chromium.org

[erikchen, 2016-06-28 22:06:29]

piman: PTAL

dcheng: Please review
gpu/ipc/common/gpu_param_traits_macros.h
ppapi/proxy/ppapi_messages.h

[commit-bot: I haz the power, 2016-06-28 22:07:58]

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

[piman, 2016-06-28 22:16:37]

lgtm

[piman, 2016-06-28 22:17:48]

On Tue, Jun 28, 2016 at 2:55 PM, <ccameron@chromium.org> wrote:

> On 2016/06/28 21:48:38, erikchen wrote:
> > On 2016/06/28 20:51:41, piman wrote:
> > >
> >
>
>
https://codereview.chromium.org/2086363004/diff/80001/content/renderer/pepper...
> > > File content/renderer/pepper/ppb_graphics_3d_impl.cc (right):
> > >
> > >
> >
>
>
https://codereview.chromium.org/2086363004/diff/80001/content/renderer/pepper...
> > > content/renderer/pepper/ppb_graphics_3d_impl.cc:192: gfx::Size(width(),
> > > height()),
> > > The size will be wrong here.
> > >
> > > PPB_Graphics3D_Shared::ResizeBuffers is called on the plugin side, so
> the
> > > updated size will never make it here (renderer side).
> > >
> > > I think the right thing to do is to grab the size on the plugin side in
> > > Graphics3D::DoSwapBuffers, send it together with the
> > > PpapiHostMsg_PPBGraphics3D_SwapBuffers message, and forward it through
> here.
> > >
> > > One question: what bad things could happen if this size is maliciously
> > > incorrectly set?
> >
> > To the best of my knowledge, the only place the size is used is in
> non-overlay
> > mode (GL compositor), when texturing into the output IOSurface.
> Theoretically,
> > it should be possible to avoid passing the size through altogether since
> the
> GPU
> > process already knows it. (even for h264 video? not sure off the top of
> my
> > head.) Let me ask ccameron@ to see if he has further thoughts.
>
> IIRC it's only used for normalizing texture coordinates (for rectangle
> textures)
> when in GL compositor mode.
>
> So, if this is wrong, you'll get a strangely-scaled texture -- it's not
> optional
> (h264 sets it by a side-channel at
>
>
https://cs.chromium.org/chromium/src/cc/resources/video_resource_updater.cc?r...
> )
>
>
>
Great, that makes sense, thanks for the explanation!


>
> https://codereview.chromium.org/2086363004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-06-28 23:10:25]

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

[commit-bot: I haz the power, 2016-06-28 23:10:26]

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

[erikchen, 2016-07-06 16:56:23]

dcheng: Ping?

[erikchen, 2016-07-06 21:37:48]

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

[commit-bot: I haz the power, 2016-07-06 21:38:34]

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

[erikchen, 2016-07-06 21:45:08]

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

[commit-bot: I haz the power, 2016-07-06 21:46:18]

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

[dcheng, 2016-07-07 00:55:38]

It looks like the GPU process might DCHECK on this value:
https://cs.chromium.org/chromium/src/gpu/command_buffer/service/gles2_cmd_dec...

Is there some invariant here that needs to be enforced at the ipc layer?

[commit-bot: I haz the power, 2016-07-07 00:56:41]

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

[commit-bot: I haz the power, 2016-07-07 00:56:42]

Dry run: This issue passed the CQ dry run.

[erikchen, 2016-07-07 00:59:12]

On 2016/07/07 00:55:38, dcheng wrote:
> It looks like the GPU process might DCHECK on this value:
>
https://cs.chromium.org/chromium/src/gpu/command_buffer/service/gles2_cmd_dec...
> 
> Is there some invariant here that needs to be enforced at the ipc layer?

The GPU process checks that if the value is true, then there must also be an
image factory. Note that the logic during initialization where this member gets
set:

"""
  should_use_native_gmb_for_backbuffer_ =                                       
                                                                                
                                                                       
      attrib_helper.should_use_native_gmb_for_backbuffer;                       
    
  if (should_use_native_gmb_for_backbuffer_) {                                  
    
    gpu::ImageFactory* image_factory = group_->image_factory();                 
    
    bool supported = false;                                                     
    
    if (image_factory) {                                                        
    
      switch (image_factory->RequiredTextureType()) {                           
    
        case GL_TEXTURE_RECTANGLE_ARB:                                          
    
          supported = feature_info_->feature_flags().arb_texture_rectangle;     
          break;                                                                
    
        case GL_TEXTURE_2D:                                                     
    
          supported = true;                                                     
    
          break;                                                                
    
        default:                                                                
    
          break;                                                                
    
      }                                                                         
    
    }                                                                           
    
                                                                                
    
    if (!supported) {                                                           
    
      group_ = NULL;  // Must not destroy ContextGroup if it is not initialized.
      Destroy(true);                                                            
    
      return false;                                                             
    
    }                                                                           
    
  }                                                                             
    
      
"""

[dcheng, 2016-07-07 01:06:45]

ipc lgtm

sorry for the delay: feel free to ping more aggressively in the future (i missed
this one in my backlog when travelling to TOK)

[erikchen, 2016-07-07 01:08:31]

erikchen@chromium.org changed reviewers:
+ bbudge@chromium.org

[erikchen, 2016-07-07 01:08:31]

bbudge: Please review ppapi.

[bbudge, 2016-07-07 22:42:52]

ppapi lgtm

[erikchen, 2016-07-07 23:47:51]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2016-07-07 23:47:52]

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

[commit-bot: I haz the power, 2016-07-07 23:48:48]

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-07-08 01:50:21]

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

[commit-bot: I haz the power, 2016-07-08 01:50:22]

Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)

[erikchen, 2016-07-08 03:52:21]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2016-07-08 03:52:41]

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-07-08 09:39:08]

Description was changed from

==========
mac: Finish plumbing for Pepper3D Image Chromium.

This allows Flash content to use the new Core Animation compositing path. This
CL includes the last plumbing required to turn on the feature. It will be turned
on in a separate CL to allow for a clean revert if needed.

BUG=602484
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
mac: Finish plumbing for Pepper3D Image Chromium.

This allows Flash content to use the new Core Animation compositing path. This
CL includes the last plumbing required to turn on the feature. It will be turned
on in a separate CL to allow for a clean revert if needed.

BUG=602484
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-07-08 09:39:10]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-07-08 09:40:56]

Description was changed from

==========
mac: Finish plumbing for Pepper3D Image Chromium.

This allows Flash content to use the new Core Animation compositing path. This
CL includes the last plumbing required to turn on the feature. It will be turned
on in a separate CL to allow for a clean revert if needed.

BUG=602484
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
mac: Finish plumbing for Pepper3D Image Chromium.

This allows Flash content to use the new Core Animation compositing path. This
CL includes the last plumbing required to turn on the feature. It will be turned
on in a separate CL to allow for a clean revert if needed.

BUG=602484
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/b13637b2ffc6aca611a407e943ffe587ab7e4aa2
Cr-Commit-Position: refs/heads/master@{#404330}
==========

[commit-bot: I haz the power, 2016-07-08 09:40:57]

Patchset 9 (id:??) landed as
https://crrev.com/b13637b2ffc6aca611a407e943ffe587ab7e4aa2
Cr-Commit-Position: refs/heads/master@{#404330}