Issue 1108713004: OffscreenBrowserCompositorOutputSurface for Unified Desktop

Issue 1108713004: OffscreenBrowserCompositorOutputSurface for Unified Desktop (Closed)

Created:
5 years, 8 months ago by oshima

Modified:
5 years, 7 months ago

Reviewers:
piman

CC:
chromium-reviews, sadrul, Ian Vollick, jam, sievers+watch_chromium.org, jbauman+watch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, kalyank, danakj+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Introduce OffscreenBrowsreCompositorOutputSurface for offscreen desktop rendering Allow reflector to copy textures for multiple displays. BUG=365662 Committed: https://crrev.com/6ef578785e6c30e44ab72d17920e6d44ca2c28c2 Cr-Commit-Position: refs/heads/master@{#327385}

Patch Set 1 : #

Total comments: 6

Patch Set 2 : use kNullAcceleratedWidget #

Patch Set 3 : use kNullAcceleratedWidget. enable test for ozon #

Patch Set 4 : fix mac build #

Created: 5 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+294 lines, -32 lines)			Patch
M	ash/display/mirror_window_controller.cc	View		2 chunks	+2 lines, -4 lines	0 comments	Download
M	ash/host/ash_window_tree_host_unified.cc	View	1 2	1 chunk	+0 lines, -1 line	0 comments	Download
M	content/browser/compositor/gpu_process_transport_factory.cc	View	1	4 chunks	+15 lines, -5 lines	0 comments	Download
A	content/browser/compositor/offscreen_browser_compositor_output_surface.h	View	1 2 3	1 chunk	+57 lines, -0 lines	0 comments	Download
A	content/browser/compositor/offscreen_browser_compositor_output_surface.cc	View	1 2 3	1 chunk	+143 lines, -0 lines	0 comments	Download
M	content/browser/compositor/reflector_impl.h	View		3 chunks	+11 lines, -3 lines	0 comments	Download
M	content/browser/compositor/reflector_impl.cc	View		8 chunks	+59 lines, -19 lines	0 comments	Download
M	content/content_browser.gypi	View	1	1 chunk	+2 lines, -0 lines	0 comments	Download
M	ui/compositor/reflector.h	View		1 chunk	+3 lines, -0 lines	0 comments	Download
M	ui/compositor/test/in_process_context_factory.cc	View		1 chunk	+2 lines, -0 lines	0 comments	Download

Messages

Total messages: 35 (23 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

oshima

https://codereview.chromium.org/1108713004/diff/270001/content/browser/compositor/reflector_impl.cc File content/browser/compositor/reflector_impl.cc (right): https://codereview.chromium.org/1108713004/diff/270001/content/browser/compositor/reflector_impl.cc#newcode193 content/browser/compositor/reflector_impl.cc:193: layer_data->layer->SetBounds(gfx::Rect(source_size)); This is inefficient as each reflector has a ...

5 years, 7 months ago (2015-04-28 16:05:39 UTC) #16

piman

https://codereview.chromium.org/1108713004/diff/270001/content/browser/compositor/offscreen_browser_compositor_output_surface.cc File content/browser/compositor/offscreen_browser_compositor_output_surface.cc (right): https://codereview.chromium.org/1108713004/diff/270001/content/browser/compositor/offscreen_browser_compositor_output_surface.cc#newcode66 content/browser/compositor/offscreen_browser_compositor_output_surface.cc:66: GLDataFormat(format), GLDataType(format), nullptr); note: since you never use this ...

5 years, 7 months ago (2015-04-28 18:38:14 UTC) #17

oshima

5 years, 7 months ago (2015-04-28 20:39:01 UTC) #19

piman

On 2015/04/28 20:39:01, oshima wrote: > https://codereview.chromium.org/1108713004/diff/270001/content/browser/compositor/offscreen_browser_compositor_output_surface.cc > File content/browser/compositor/offscreen_browser_compositor_output_surface.cc > (right): > > https://codereview.chromium.org/1108713004/diff/270001/content/browser/compositor/offscreen_browser_compositor_output_surface.cc#newcode66 ...

5 years, 7 months ago (2015-04-28 21:00:21 UTC) #20

On 2015/04/28 20:39:01, oshima wrote:
>
https://codereview.chromium.org/1108713004/diff/270001/content/browser/compos...
> File content/browser/compositor/offscreen_browser_compositor_output_surface.cc
> (right):
> 
>
https://codereview.chromium.org/1108713004/diff/270001/content/browser/compos...
> content/browser/compositor/offscreen_browser_compositor_output_surface.cc:66:
> GLDataFormat(format), GLDataType(format), nullptr);
> On 2015/04/28 18:38:13, piman (Very slow to review) wrote:
> > note: since you never use this one as a texture, only as a render target
that
> > you render to and copy from, you can use a renderbuffer instead (it may be
> more
> > efficient).
> > Though if you are going to use this texture to avoid the copy, then it's
fine
> to
> > leave as is.
> 
> Yes, I'm going to use this texture directly on the reflector's surface.
> 
>
https://codereview.chromium.org/1108713004/diff/270001/ui/gfx/native_widget_t...
> File ui/gfx/native_widget_types.h (right):
> 
>
https://codereview.chromium.org/1108713004/diff/270001/ui/gfx/native_widget_t...
> ui/gfx/native_widget_types.h:264: const AcceleratedWidget
> kOffscreenAcceleratedWidget = 1;
> On 2015/04/28 18:38:13, piman (Very slow to review) wrote:
> > I'm not fond of adding new magic numbers.
> > What's wrong with using kNullAcceleratedWidget for offscreen?
> 
> Is it safe to use it? kNullAcceleratedWidget seems to be used to indicate it's
> uninitialized widget or doesn't exist, and I wasn't sure if it's safe.

Right, in your case it doesn't exist.
It's still the case that you will have to special-case the value in
CreatePerCompositorData so that you don't create a surface_id for it, and pick
the right output surface.

>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/gp...

You would skip that code, because you're not adding the kNullAcceleratedWidget
to the GpuSurfaceTracker.

>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...

That's testing code.

>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

This is essentially Windows-specific and doesn't touch the compositor (not even
sure why that's an AcceleratedWidget actually).

> 
> I'm running try job now, and if this is safe, I'm happy to change.

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1108713004/340001

5 years, 7 months ago (2015-04-28 21:14:11 UTC) #24

oshima

On 2015/04/28 21:00:21, piman (Very slow to review) wrote: > On 2015/04/28 20:39:01, oshima wrote: ...

5 years, 7 months ago (2015-04-28 21:28:30 UTC) #25

On 2015/04/28 21:00:21, piman (Very slow to review) wrote:
> On 2015/04/28 20:39:01, oshima wrote:
> >
>
https://codereview.chromium.org/1108713004/diff/270001/content/browser/compos...
> > File
content/browser/compositor/offscreen_browser_compositor_output_surface.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/1108713004/diff/270001/content/browser/compos...
> >
content/browser/compositor/offscreen_browser_compositor_output_surface.cc:66:
> > GLDataFormat(format), GLDataType(format), nullptr);
> > On 2015/04/28 18:38:13, piman (Very slow to review) wrote:
> > > note: since you never use this one as a texture, only as a render target
> that
> > > you render to and copy from, you can use a renderbuffer instead (it may be
> > more
> > > efficient).
> > > Though if you are going to use this texture to avoid the copy, then it's
> fine
> > to
> > > leave as is.
> > 
> > Yes, I'm going to use this texture directly on the reflector's surface.
> > 
> >
>
https://codereview.chromium.org/1108713004/diff/270001/ui/gfx/native_widget_t...
> > File ui/gfx/native_widget_types.h (right):
> > 
> >
>
https://codereview.chromium.org/1108713004/diff/270001/ui/gfx/native_widget_t...
> > ui/gfx/native_widget_types.h:264: const AcceleratedWidget
> > kOffscreenAcceleratedWidget = 1;
> > On 2015/04/28 18:38:13, piman (Very slow to review) wrote:
> > > I'm not fond of adding new magic numbers.
> > > What's wrong with using kNullAcceleratedWidget for offscreen?
> > 
> > Is it safe to use it? kNullAcceleratedWidget seems to be used to indicate
it's
> > uninitialized widget or doesn't exist, and I wasn't sure if it's safe.
> 
> Right, in your case it doesn't exist.
> It's still the case that you will have to special-case the value in
> CreatePerCompositorData so that you don't create a surface_id for it, and pick
> the right output surface.
> 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/gp...
> 
> You would skip that code, because you're not adding the kNullAcceleratedWidget
> to the GpuSurfaceTracker.
> 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...
> 
> That's testing code.
> 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
> 
> This is essentially Windows-specific and doesn't touch the compositor (not
even
> sure why that's an AcceleratedWidget actually).
> 
> > 
> > I'm running try job now, and if this is safe, I'm happy to change.

Ok, great. thanks!

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1108713004/340002

5 years, 7 months ago (2015-04-28 21:35:37 UTC) #29

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

5 years, 7 months ago (2015-04-28 23:00:35 UTC) #31

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1108713004/340002

5 years, 7 months ago (2015-04-28 23:07:50 UTC) #33

commit-bot: I haz the power

5 years, 7 months ago (2015-04-28 23:11:52 UTC) #35

Message was sent while issue was closed.

Patchset 4 (id:??) landed as
https://crrev.com/6ef578785e6c30e44ab72d17920e6d44ca2c28c2
Cr-Commit-Position: refs/heads/master@{#327385}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages