ozone: dri: Composite to intermediate surface & copy

ozone: dri: Composite to intermediate surface & copy

This removes the need to read from the scanout buffer. Doing that is really, really slow.

TEST=chrome --ozone-platform=dri on pixel
BUG=366205
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266076

[danakj, 2014-04-23 18:24:13]

On Wed, Apr 23, 2014 at 2:21 PM, <spang@chromium.org> wrote:

> Reviewers: dnicoara, rjkroege, danakj,
>
> Description:
> ozone: dri: Composite to intermediate surface & copy
>
> TEST=chrome --ozone-platform=dri on pixel
> BUG=none


No bug, and a 1-line description. Can you add just explaination about what
the change is and why?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[spang, 2014-04-23 18:40:43]

On 2014/04/23 18:24:13, danakj wrote:
> On Wed, Apr 23, 2014 at 2:21 PM, <mailto:spang@chromium.org> wrote:
> 
> > Reviewers: dnicoara, rjkroege, danakj,
> >
> > Description:
> > ozone: dri: Composite to intermediate surface & copy
> >
> > TEST=chrome --ozone-platform=dri on pixel
> > BUG=none
> 
> 
> No bug, and a 1-line description. Can you add just explaination about what
> the change is and why?
> 

This change removes the need to read from the hardware buffers.

Writing and reading from the hardware buffers is I/O. Reading especially is just
insanely slow. It takes 200ms to memcpy() one entire hardware buffer to another
on the Pixel.

So we need to not do that.

> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-04-23 19:05:38]

Thanks for the added context. I have some possibly-naive questions.

https://codereview.chromium.org/249413003/diff/20001/content/browser/composit...
File content/browser/compositor/software_output_device_ozone.cc (left):

https://codereview.chromium.org/249413003/diff/20001/content/browser/composit...
content/browser/compositor/software_output_device_ozone.cc:66: DCHECK(scheduled)
<< "Failed to present canvas";
Can this be expected to fail more than before? Or if we don't check this return
value, should it be void instead of bool then?

https://codereview.chromium.org/249413003/diff/20001/ui/ozone/platform/dri/dr...
File ui/ozone/platform/dri/dri_surface_factory.cc (right):

https://codereview.chromium.org/249413003/diff/20001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_surface_factory.cc:87: SkImageInfo info =
SkImageInfo::MakeN32(
Is there any test to show this working now?

https://codereview.chromium.org/249413003/diff/20001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_surface_factory.cc:94: SkRect real_damage =
RectToSkRect(UnionRects(damage, last_damage_));
Why do you always present a damage rect for two calls to PresentCanvas?

[spang, 2014-04-23 20:57:18]

https://codereview.chromium.org/249413003/diff/20001/content/browser/composit...
File content/browser/compositor/software_output_device_ozone.cc (left):

https://codereview.chromium.org/249413003/diff/20001/content/browser/composit...
content/browser/compositor/software_output_device_ozone.cc:66: DCHECK(scheduled)
<< "Failed to present canvas";
On 2014/04/23 19:05:38, danakj wrote:
> Can this be expected to fail more than before? Or if we don't check this
return
> value, should it be void instead of bool then?

Changing to void sounds fine to me. The error path here never really made sense
anyway. There's no way to pass an error up from EndPaint, so if the
implementation can't paint, there's nothing that can be done here.

A similar argument applies to ResizeCanvas.

https://codereview.chromium.org/249413003/diff/20001/ui/ozone/platform/dri/dr...
File ui/ozone/platform/dri/dri_surface_factory.cc (right):

https://codereview.chromium.org/249413003/diff/20001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_surface_factory.cc:87: SkImageInfo info =
SkImageInfo::MakeN32(
On 2014/04/23 19:05:38, danakj wrote:
> Is there any test to show this working now?

Resize at runtime isn't really supported. I'm using this to create the surface
once the initial size is known.

A resize here does not actually affect the scanout buffer size, since that's
tied to the display mode.

If the sizes don't match, we'll just copy to the top left corner of the scanout
buffer and lose any excess. Given that resize after initialization is never
attempted, this seems fine.

https://codereview.chromium.org/249413003/diff/20001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_surface_factory.cc:94: SkRect real_damage =
RectToSkRect(UnionRects(damage, last_damage_));
On 2014/04/23 19:05:38, danakj wrote:
> Why do you always present a damage rect for two calls to PresentCanvas?

The incoming buffer is missing the prior update due to double buffering. I will
update with a comment.

[danakj, 2014-04-23 21:26:20]

Thanks for the comment. LGTM when the others are happy also. :)

[rjkroege, 2014-04-23 22:29:32]

It's not clear to me if this ends up making a third buffer for the entire
screen. Does it? Several optimizations seem worth considering:

1. single buffer the DRI buffer and schedule the update on the vsync.
(Presumption: we can memcpy a framebuffer in < vblank time.) Probably we can if
we fire off the copy on vblank. Obviously we ought to get vblank events.

2. is the snapshot sized to the damage region only?

https://codereview.chromium.org/249413003/diff/30001/ui/gfx/ozone/surface_ozo...
File ui/gfx/ozone/surface_ozone_canvas.h (right):

https://codereview.chromium.org/249413003/diff/30001/ui/gfx/ozone/surface_ozo...
ui/gfx/ozone/surface_ozone_canvas.h:34: virtual void ResizeCanvas(const
gfx::Size& viewport_size) = 0;
what was your reasoning for removing the bool return?

https://codereview.chromium.org/249413003/diff/30001/ui/gfx/ozone/surface_ozo...
ui/gfx/ozone/surface_ozone_canvas.h:37: // call GetCanvas() to get the next
canvas.
say what damage is and its semantics please? Can I call PresentCanvas > 1 time
per canvas?

https://codereview.chromium.org/249413003/diff/30001/ui/ozone/platform/dri/dr...
File ui/ozone/platform/dri/dri_surface_factory.cc (right):

https://codereview.chromium.org/249413003/diff/30001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_surface_factory.cc:91: virtual void
PresentCanvas(const gfx::Rect& damage) OVERRIDE {
shouldn't there be new lines after the } ?

[spang, 2014-04-23 22:50:09]

On 2014/04/23 22:29:32, rjkroege wrote:
> It's not clear to me if this ends up making a third buffer for the entire
> screen. Does it? Several optimizations seem worth considering:
> 

It does.

> 1. single buffer the DRI buffer and schedule the update on the vsync.
> (Presumption: we can memcpy a framebuffer in < vblank time.) Probably we can
if
> we fire off the copy on vblank. Obviously we ought to get vblank events.

I don't think that is sufficiently synchronized. How do we know when the
hardware is scanning out to avoid tearing?

> 
> 2. is the snapshot sized to the damage region only?

It's the whole screen, but there should be no pixel copy to produce the
snapshot. If there is any chance of that, then we need a better API from skia.

My assumption is that creating an image, reading it, and then destroying it with
no concurrent changes to the surface must not cause a copy.

> 
>
https://codereview.chromium.org/249413003/diff/30001/ui/gfx/ozone/surface_ozo...
> File ui/gfx/ozone/surface_ozone_canvas.h (right):
> 
>
https://codereview.chromium.org/249413003/diff/30001/ui/gfx/ozone/surface_ozo...
> ui/gfx/ozone/surface_ozone_canvas.h:34: virtual void ResizeCanvas(const
> gfx::Size& viewport_size) = 0;
> what was your reasoning for removing the bool return?

Similarly to PresentCanvas(), there's nothing that can be done it the platform
function fails. The existing bool return was just ignored, so we can just remove
it.

> 
>
https://codereview.chromium.org/249413003/diff/30001/ui/gfx/ozone/surface_ozo...
> ui/gfx/ozone/surface_ozone_canvas.h:37: // call GetCanvas() to get the next
> canvas.
> say what damage is and its semantics please? Can I call PresentCanvas > 1 time
> per canvas?

No, that's not allowed. Per the comment, the caller must call GetCanvas() after
PresentCanvas(). I have clarified this to say that old references are
invalidated.

> 
>
https://codereview.chromium.org/249413003/diff/30001/ui/ozone/platform/dri/dr...
> File ui/ozone/platform/dri/dri_surface_factory.cc (right):
> 
>
https://codereview.chromium.org/249413003/diff/30001/ui/ozone/platform/dri/dr...
> ui/ozone/platform/dri/dri_surface_factory.cc:91: virtual void
> PresentCanvas(const gfx::Rect& damage) OVERRIDE {
> shouldn't there be new lines after the } ?

Not sure. Newlines are missing to imply that all these go with the //
SurfaceOzoneCanvas: comment but I have no strong preference.

[danakj, 2014-04-23 23:12:54]

On Wed, Apr 23, 2014 at 6:50 PM, <spang@chromium.org> wrote:

> https://codereview.chromium.org/249413003/diff/30001/ui/
> ozone/platform/dri/dri_surface_factory.cc#newcode91
>
>> ui/ozone/platform/dri/dri_surface_factory.cc:91: virtual void
>> PresentCanvas(const gfx::Rect& damage) OVERRIDE {
>> shouldn't there be new lines after the } ?
>>
>
> Not sure. Newlines are missing to imply that all these go with the //
> SurfaceOzoneCanvas: comment but I have no strong preference.
>

The clean way to do that is to move the function definitions out below the
class, so the declarations can all sit beside eachother.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rjkroege, 2014-04-24 19:01:00]

On 2014/04/23 22:50:09, spang wrote:
> On 2014/04/23 22:29:32, rjkroege wrote:
> > It's not clear to me if this ends up making a third buffer for the entire
> > screen. Does it? Several optimizations seem worth considering:
> > 
> 
> It does.

shucks

> 
> > 1. single buffer the DRI buffer and schedule the update on the vsync.
> > (Presumption: we can memcpy a framebuffer in < vblank time.) Probably we can
> if
> > we fire off the copy on vblank. Obviously we ought to get vblank events.
> 
> I don't think that is sufficiently synchronized. How do we know when the
> hardware is scanning out to avoid tearing?

The display controller offers up an event "vblank" when it would tell the CRT to
turn off the electron beam for the return to top-left. From vblank to start of
scanout is typically 2 ms. Doing a copy at vblank would provide tear-free
update. 

Can this copy happen in the kernel without tearing: yes. The interrupt handler
could do it. DRM could offer a partial update mode like this. Can the kernel
ship the event to Chrome and we schedule a task on the UI thread to handle it in
software soon enough to not tearing? You're right: probably not.

> 
> > 
> > 2. is the snapshot sized to the damage region only?
> 
> It's the whole screen, but there should be no pixel copy to produce the
> snapshot. If there is any chance of that, then we need a better API from skia.
> 
> My assumption is that creating an image, reading it, and then destroying it
with
> no concurrent changes to the surface must not cause a copy.

you might want to check this.

> 
> > 
> >
>
https://codereview.chromium.org/249413003/diff/30001/ui/gfx/ozone/surface_ozo...
> > File ui/gfx/ozone/surface_ozone_canvas.h (right):
> > 
> >
>
https://codereview.chromium.org/249413003/diff/30001/ui/gfx/ozone/surface_ozo...
> > ui/gfx/ozone/surface_ozone_canvas.h:34: virtual void ResizeCanvas(const
> > gfx::Size& viewport_size) = 0;
> > what was your reasoning for removing the bool return?
> 
> Similarly to PresentCanvas(), there's nothing that can be done it the platform
> function fails. The existing bool return was just ignored, so we can just
remove
> it.

ok.

> 
> > 
> >
>
https://codereview.chromium.org/249413003/diff/30001/ui/gfx/ozone/surface_ozo...
> > ui/gfx/ozone/surface_ozone_canvas.h:37: // call GetCanvas() to get the next
> > canvas.
> > say what damage is and its semantics please? Can I call PresentCanvas > 1
time
> > per canvas?
> 
> No, that's not allowed. Per the comment, the caller must call GetCanvas()
after
> PresentCanvas(). I have clarified this to say that old references are
> invalidated.

thanks for updating.

> 
> > 
> >
>
https://codereview.chromium.org/249413003/diff/30001/ui/ozone/platform/dri/dr...
> > File ui/ozone/platform/dri/dri_surface_factory.cc (right):
> > 
> >
>
https://codereview.chromium.org/249413003/diff/30001/ui/ozone/platform/dri/dr...
> > ui/ozone/platform/dri/dri_surface_factory.cc:91: virtual void
> > PresentCanvas(const gfx::Rect& damage) OVERRIDE {
> > shouldn't there be new lines after the } ?
> 
> Not sure. Newlines are missing to imply that all these go with the //
> SurfaceOzoneCanvas: comment but I have no strong preference.

the implementations are large enough that I'd prefer them moved out.

so: lgtm with nits and wistful longing for not having a 50% increase in memory
footprint.

[spang, 2014-04-25 00:30:19]

The CQ bit was checked by spang@chromium.org

[commit-bot: I haz the power, 2014-04-25 00:31:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/spang@chromium.org/249413003/70001

[commit-bot: I haz the power, 2014-04-25 00:57:30]

Change committed as 266076