Issue 2242653002: blimp: Make Blimp client use a Display for compositing.

danakj

Description was changed from ========== Blimp uses a Display. BUG= ========== to ========== Blimp uses ...

4 years, 4 months ago (2016-08-12 00:08:06 UTC) #1

danakj

Description was changed from ========== Blimp uses a Display. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== blimp: ...

4 years, 4 months ago (2016-08-12 00:16:28 UTC) #2

danakj

danakj@chromium.org changed reviewers: + nyquist@chromium.org

4 years, 4 months ago (2016-08-12 00:16:28 UTC) #3

danakj

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

4 years, 4 months ago (2016-08-12 00:16:44 UTC) #5

commit-bot: I haz the power

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

4 years, 4 months ago (2016-08-12 00:17:35 UTC) #6

danakj

I verified I can open google.com and scroll with this patch.

4 years, 4 months ago (2016-08-12 00:17:54 UTC) #7

danakj

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

4 years, 4 months ago (2016-08-12 00:38:37 UTC) #8

commit-bot: I haz the power

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

4 years, 4 months ago (2016-08-12 00:39:06 UTC) #9

commit-bot: I haz the power

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

4 years, 4 months ago (2016-08-12 00:51:14 UTC) #10

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/50937)

4 years, 4 months ago (2016-08-12 00:51:15 UTC) #11

Wez

Drive-by! /me learns some compositor-fu... ;) https://codereview.chromium.org/2242653002/diff/40001/blimp/client/feature/compositor/blimp_compositor.cc File blimp/client/feature/compositor/blimp_compositor.cc (right): https://codereview.chromium.org/2242653002/diff/40001/blimp/client/feature/compositor/blimp_compositor.cc#newcode265 blimp/client/feature/compositor/blimp_compositor.cc:265: scoped_refptr<BlimpContextProvider> worker_context_provider = ...

4 years, 4 months ago (2016-08-12 01:56:50 UTC) #13

Khushal

On 2016/08/12 01:57:25, nyquist wrote: > lgtm! Thanks for doing this! I'll land this patch ...

4 years, 4 months ago (2016-08-12 02:14:14 UTC) #15

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2242653002/40001

4 years, 4 months ago (2016-08-12 02:14:57 UTC) #17

commit-bot: I haz the power

Description was changed from ========== blimp: Make Blimp client use a Display for compositing. Gives ...

4 years, 4 months ago (2016-08-12 02:59:59 UTC) #18

commit-bot: I haz the power

Description was changed from ========== blimp: Make Blimp client use a Display for compositing. Gives ...

4 years, 4 months ago (2016-08-12 03:02:18 UTC) #20

commit-bot: I haz the power

Patchset 3 (id:??) landed as https://crrev.com/2b3f40c6da38371a0d1ed1c5753fec651a552944 Cr-Commit-Position: refs/heads/master@{#411532}

4 years, 4 months ago (2016-08-12 03:02:19 UTC) #21

danakj

https://codereview.chromium.org/2242653002/diff/40001/blimp/client/feature/compositor/blimp_compositor.cc File blimp/client/feature/compositor/blimp_compositor.cc (right): https://codereview.chromium.org/2242653002/diff/40001/blimp/client/feature/compositor/blimp_compositor.cc#newcode270 blimp/client/feature/compositor/blimp_compositor.cc:270: // worker_context_provider->BindToCurrentThread(); On 2016/08/12 01:56:49, Wez wrote: > This ...

4 years, 4 months ago (2016-08-12 18:16:53 UTC) #22

Message was sent while issue was closed.

https://codereview.chromium.org/2242653002/diff/40001/blimp/client/feature/co...
File blimp/client/feature/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2242653002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor.cc:270: //
worker_context_provider->BindToCurrentThread();
On 2016/08/12 01:56:49, Wez wrote:
> This is commented out; what is it that will one day have to be removed? :P

When the context isn't null, this will have to be called. Until one day when we
move worker context binding off main thread.

https://codereview.chromium.org/2242653002/diff/40001/blimp/client/feature/co...
File blimp/client/feature/compositor/blimp_delegating_output_surface.h (right):

https://codereview.chromium.org/2242653002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_delegating_output_surface.h:23: public
cc::DisplayClient {
On 2016/08/12 01:56:49, Wez wrote:
> Can we add a brief comment to explain what this is for, plz?
> 
> Specifically, if this is a Blimp-specific OutputSurface, and it is constructed
> with an OutputSurface, then why does the caller not just use that
OutputSurface
> directly..?
> 
> Basically what bits is this tying together that is so Blimp-specific? :P

It's a delegating output surface that delegates compositing to a Display. This
is going to get more clear when
https://bugs.chromium.org/p/chromium/issues/detail?id=606056 is done. Cuz they
won't both be called OutputSurface.

https://codereview.chromium.org/2242653002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_delegating_output_surface.h:29:
gpu::GpuMemoryBufferManager* gpu_memory_buffer_manager,
On 2016/08/12 01:56:50, Wez wrote:
> Is this ref-counted, or passing ownership? If neither then please add a
comment
> to clarify the lifetime expectation. It looks like the GPU memory buffer
manager
> must out-live the OutputSurface in this case.

Neither, it's a raw pointer which are not ownership transfers. I thought this is
generally assumed?

https://codereview.chromium.org/2242653002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_delegating_output_surface.h:31:
base::SingleThreadTaskRunner* task_runner);
On 2016/08/12 01:56:50, Wez wrote:
> nit: const scoped_refptr<>& for clarity here.

So, the stuff it passes the task_runner to don't take a ref, so I'm not sure
what using scoped_refptr<>& is for? To keep a ref inside the constructor while
it's running?

https://codereview.chromium.org/2242653002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_delegating_output_surface.h:54: //
TODO(danakj): These don't to be stored in unique_ptrs when OutputSurface
On 2016/08/12 01:56:49, Wez wrote:
> You mean "don't need to be..." or something else?

Ya, woops, that's in the original place I copied this from too.

> Why does the thread that OutputSurface is owned on affect whether these own
the
> objects they point to or not?

Because they have to be destroyed in DetachFromClient right now, which is our
"impl side destructor" but happens before the object is actually destroyed.

danakj

https://codereview.chromium.org/2242653002/diff/40001/blimp/client/feature/compositor/blimp_delegating_output_surface.h File blimp/client/feature/compositor/blimp_delegating_output_surface.h (right): https://codereview.chromium.org/2242653002/diff/40001/blimp/client/feature/compositor/blimp_delegating_output_surface.h#newcode31 blimp/client/feature/compositor/blimp_delegating_output_surface.h:31: base::SingleThreadTaskRunner* task_runner); On 2016/08/12 18:16:53, danakj wrote: > On ...

4 years, 4 months ago (2016-08-12 18:17:53 UTC) #23

Wez

Apologies for the delay in getting back to you, Dana! HTH https://codereview.chromium.org/2242653002/diff/40001/blimp/client/feature/compositor/blimp_delegating_output_surface.h File blimp/client/feature/compositor/blimp_delegating_output_surface.h (right): ...

4 years, 4 months ago (2016-08-18 23:22:40 UTC) #24

Message was sent while issue was closed.

Apologies for the delay in getting back to you, Dana!

HTH

https://codereview.chromium.org/2242653002/diff/40001/blimp/client/feature/co...
File blimp/client/feature/compositor/blimp_delegating_output_surface.h (right):

https://codereview.chromium.org/2242653002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_delegating_output_surface.h:29:
gpu::GpuMemoryBufferManager* gpu_memory_buffer_manager,
On 2016/08/12 18:16:53, danakj wrote:
> On 2016/08/12 01:56:50, Wez wrote:
> > Is this ref-counted, or passing ownership? If neither then please add a
> comment
> > to clarify the lifetime expectation. It looks like the GPU memory buffer
> manager
> > must out-live the OutputSurface in this case.
> 
> Neither, it's a raw pointer which are not ownership transfers. I thought this
is
> generally assumed?

Right, now that we have unique_ptr and scoped_refptr it's most common to use
bare pointers for interface injection, where the recipient doesn't own the
received pointer, but we still need to be clear about how long the interface
impl needs to remain valid for - the duration of the receiving call, the
lifetime of the receiving object, or something else, IMO.

We have comments elsewhere along the lines of "Caller must ensure that |foo|
outlives this Bar."

https://codereview.chromium.org/2242653002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_delegating_output_surface.h:31:
base::SingleThreadTaskRunner* task_runner);
On 2016/08/12 18:17:53, danakj wrote:
> On 2016/08/12 18:16:53, danakj wrote:
> > On 2016/08/12 01:56:50, Wez wrote:
> > > nit: const scoped_refptr<>& for clarity here.
> > 
> > So, the stuff it passes the task_runner to don't take a ref, so I'm not sure
> > what using scoped_refptr<>& is for? To keep a ref inside the constructor
while
> > it's running?
> 
> (It should actually be taking a scoped_refptr here cuz I'm pretty sure
something
> turns it into one way down in the stack, but this is a mess across all our
> output surfaces, and holding a ref in the constructor here doesn't seem to be
> really of assistance to me.)

It's not so much whether the ctor takes a reference as about making the API
contract clearer:

You have two bare pointers here (the memory buffer manager and the task runner),
so if I were to make assumptions about the intention (as you suggest for the
memory buffer manager) then the only safe assumption is that the caller is
responsible for ensuring that both out-live the DelegatingOutputSurface instance
(i.e. we're using objects to inject interfaces for this class to use).

In practice, though, the ctor might just take a reference to task_runner, so
that the caller doesn't actually have to make any guarantee about task_runner's
lifetime - using scoped_refptr<> here makes the lifetime expectations explicit
(similarly to unique_ptr<>).

If the ctor just uses task_runner but doesn't keep hold of it then const
scoped_refptr<>& is still preferable to a bare pointer, in protecting later
editors of this code from accidentally introducing a task_runner lifetime
dependency that isn't valid - they'd have to pull out the bare pointer using
get() to achieve that, which should ring alarm bells.

danakj

On Thu, Aug 18, 2016 at 4:22 PM, <wez@chromium.org> wrote: > Apologies for the delay ...

4 years, 3 months ago (2016-08-26 01:36:37 UTC) #25

Message was sent while issue was closed.

On Thu, Aug 18, 2016 at 4:22 PM, <wez@chromium.org> wrote:

> Apologies for the delay in getting back to you, Dana!
>
> HTH
>
>
> https://codereview.chromium.org/2242653002/diff/40001/
> blimp/client/feature/compositor/blimp_delegating_output_surface.h
> File blimp/client/feature/compositor/blimp_delegating_output_surface.h
> (right):
>
> https://codereview.chromium.org/2242653002/diff/40001/
> blimp/client/feature/compositor/blimp_delegating_
> output_surface.h#newcode29
> blimp/client/feature/compositor/blimp_delegating_output_surface.h:29:
> gpu::GpuMemoryBufferManager* gpu_memory_buffer_manager,
> On 2016/08/12 18:16:53, danakj wrote:
> > On 2016/08/12 01:56:50, Wez wrote:
> > > Is this ref-counted, or passing ownership? If neither then please
> add a
> > comment
> > > to clarify the lifetime expectation. It looks like the GPU memory
> buffer
> > manager
> > > must out-live the OutputSurface in this case.
> >
> > Neither, it's a raw pointer which are not ownership transfers. I
> thought this is
> > generally assumed?
>
> Right, now that we have unique_ptr and scoped_refptr it's most common to
> use bare pointers for interface injection, where the recipient doesn't
> own the received pointer, but we still need to be clear about how long
> the interface impl needs to remain valid for - the duration of the
> receiving call, the lifetime of the receiving object, or something else,
> IMO.
>
> We have comments elsewhere along the lines of "Caller must ensure that
> |foo| outlives this Bar."
>
> https://codereview.chromium.org/2242653002/diff/40001/
> blimp/client/feature/compositor/blimp_delegating_
> output_surface.h#newcode31
> blimp/client/feature/compositor/blimp_delegating_output_surface.h:31:
> base::SingleThreadTaskRunner* task_runner);
> On 2016/08/12 18:17:53, danakj wrote:
> > On 2016/08/12 18:16:53, danakj wrote:
> > > On 2016/08/12 01:56:50, Wez wrote:
> > > > nit: const scoped_refptr<>& for clarity here.
> > >
> > > So, the stuff it passes the task_runner to don't take a ref, so I'm
> not sure
> > > what using scoped_refptr<>& is for? To keep a ref inside the
> constructor while
> > > it's running?
> >
> > (It should actually be taking a scoped_refptr here cuz I'm pretty sure
> something
> > turns it into one way down in the stack, but this is a mess across all
> our
> > output surfaces, and holding a ref in the constructor here doesn't
> seem to be
> > really of assistance to me.)
>
> It's not so much whether the ctor takes a reference as about making the
> API contract clearer:
>
> You have two bare pointers here (the memory buffer manager and the task
> runner), so if I were to make assumptions about the intention (as you
> suggest for the memory buffer manager) then the only safe assumption is
> that the caller is responsible for ensuring that both out-live the
> DelegatingOutputSurface instance (i.e. we're using objects to inject
> interfaces for this class to use).
>

I think that is correct, the caller is responsible for keeping both alive.
In fact maybe cc does take a ref on this refcounted thing because we allow
that, but it's not clear that it does always. So I think your
interpretation is exactly as it should be. A raw pointer means the caller
keeps it alive as long as the raw pointer is alive in this class.


>
> In practice, though, the ctor might just take a reference to
> task_runner, so that the caller doesn't actually have to make any
> guarantee about task_runner's lifetime - using scoped_refptr<> here
> makes the lifetime expectations explicit (similarly to unique_ptr<>).
>

I agree if this class was to be responsible for the lifetime. As it is, the
caller is responsible which doesn't seem incorrect on its own.


> If the ctor just uses task_runner but doesn't keep hold of it then const
> scoped_refptr<>& is still preferable to a bare pointer, in protecting
> later editors of this code from accidentally introducing a task_runner
> lifetime dependency that isn't valid - they'd have to pull out the bare
> pointer using get() to achieve that, which should ring alarm bells.
>
> https://codereview.chromium.org/2242653002/
>

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

Wez

4 years, 3 months ago (2016-08-26 01:57:27 UTC) #26

Message was sent while issue was closed.

Right - if the caller is responsible, though, then we need to be clear on
what the lifetime requirement is, e.g. need it only exist for the duration
of the relevant call, or the for the lifetime of this object?

On 25 August 2016 at 18:31, <danakj@chromium.org> wrote:

> On Thu, Aug 18, 2016 at 4:22 PM, <wez@chromium.org> wrote:
>
>> Apologies for the delay in getting back to you, Dana!
>>
>> HTH
>>
>>
>> https://codereview.chromium.org/2242653002/diff/40001/blimp/
>> client/feature/compositor/blimp_delegating_output_surface.h
>> File blimp/client/feature/compositor/blimp_delegating_output_surface.h
>> (right):
>>
>> https://codereview.chromium.org/2242653002/diff/40001/blimp/
>> client/feature/compositor/blimp_delegating_output_surface.h#newcode29
>> blimp/client/feature/compositor/blimp_delegating_output_surface.h:29:
>> gpu::GpuMemoryBufferManager* gpu_memory_buffer_manager,
>> On 2016/08/12 18:16:53, danakj wrote:
>> > On 2016/08/12 01:56:50, Wez wrote:
>> > > Is this ref-counted, or passing ownership? If neither then please
>> add a
>> > comment
>> > > to clarify the lifetime expectation. It looks like the GPU memory
>> buffer
>> > manager
>> > > must out-live the OutputSurface in this case.
>> >
>> > Neither, it's a raw pointer which are not ownership transfers. I
>> thought this is
>> > generally assumed?
>>
>> Right, now that we have unique_ptr and scoped_refptr it's most common to
>> use bare pointers for interface injection, where the recipient doesn't
>> own the received pointer, but we still need to be clear about how long
>> the interface impl needs to remain valid for - the duration of the
>> receiving call, the lifetime of the receiving object, or something else,
>> IMO.
>>
>> We have comments elsewhere along the lines of "Caller must ensure that
>> |foo| outlives this Bar."
>>
>> https://codereview.chromium.org/2242653002/diff/40001/blimp/
>> client/feature/compositor/blimp_delegating_output_surface.h#newcode31
>> blimp/client/feature/compositor/blimp_delegating_output_surface.h:31:
>> base::SingleThreadTaskRunner* task_runner);
>> On 2016/08/12 18:17:53, danakj wrote:
>> > On 2016/08/12 18:16:53, danakj wrote:
>> > > On 2016/08/12 01:56:50, Wez wrote:
>> > > > nit: const scoped_refptr<>& for clarity here.
>> > >
>> > > So, the stuff it passes the task_runner to don't take a ref, so I'm
>> not sure
>> > > what using scoped_refptr<>& is for? To keep a ref inside the
>> constructor while
>> > > it's running?
>> >
>> > (It should actually be taking a scoped_refptr here cuz I'm pretty sure
>> something
>> > turns it into one way down in the stack, but this is a mess across all
>> our
>> > output surfaces, and holding a ref in the constructor here doesn't
>> seem to be
>> > really of assistance to me.)
>>
>> It's not so much whether the ctor takes a reference as about making the
>> API contract clearer:
>>
>> You have two bare pointers here (the memory buffer manager and the task
>> runner), so if I were to make assumptions about the intention (as you
>> suggest for the memory buffer manager) then the only safe assumption is
>> that the caller is responsible for ensuring that both out-live the
>> DelegatingOutputSurface instance (i.e. we're using objects to inject
>> interfaces for this class to use).
>>
>
> I think that is correct, the caller is responsible for keeping both alive.
> In fact maybe cc does take a ref on this refcounted thing because we allow
> that, but it's not clear that it does always. So I think your
> interpretation is exactly as it should be. A raw pointer means the caller
> keeps it alive as long as the raw pointer is alive in this class.
>
>
>>
>> In practice, though, the ctor might just take a reference to
>> task_runner, so that the caller doesn't actually have to make any
>> guarantee about task_runner's lifetime - using scoped_refptr<> here
>> makes the lifetime expectations explicit (similarly to unique_ptr<>).
>>
>
> I agree if this class was to be responsible for the lifetime. As it is,
> the caller is responsible which doesn't seem incorrect on its own.
>
>
>> If the ctor just uses task_runner but doesn't keep hold of it then const
>> scoped_refptr<>& is still preferable to a bare pointer, in protecting
>> later editors of this code from accidentally introducing a task_runner
>> lifetime dependency that isn't valid - they'd have to pull out the bare
>> pointer using get() to achieve that, which should ring alarm bells.
>>
>> https://codereview.chromium.org/2242653002/
>>
>
>

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

Issue 2242653002: blimp: Make Blimp client use a Display for compositing. (Closed)

Description

Patch Set 1 #

Patch Set 2 : blimpdisplay: . #

Patch Set 3 : blimpdisplay: . #

Messages