Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
183723003:
Overlay interface for ozone. (Closed)
|
Created:
6 years, 9 months ago by alexst (slow to review) Modified:
6 years, 9 months ago CC:
chromium-reviews, rjkroege, kalyank, ozone-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionOverlay interface for ozone.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255153
Patch Set 1 #
Total comments: 16
Patch Set 2 : Comments #Patch Set 3 : #
Total comments: 17
Patch Set 4 : Comments #Patch Set 5 : #
Total comments: 10
Patch Set 6 : Added AcceleratedWidget #Patch Set 7 : #
Messages
Total messages: 19 (0 generated)
Have a look. These are just ozone entry points.
some questions... Probably I just need to be educated. https://codereview.chromium.org/183723003/diff/1/ui/gfx/ozone/overlay_hal_ozo... File ui/gfx/ozone/overlay_hal_ozone.h (right): https://codereview.chromium.org/183723003/diff/1/ui/gfx/ozone/overlay_hal_ozo... ui/gfx/ozone/overlay_hal_ozone.h:17: class GFX_EXPORT OverlayHALOzone { More explanation comment. And I don't like the name. Sorry. :-) OverlayCandidatesOzone? https://codereview.chromium.org/183723003/diff/1/ui/gfx/ozone/overlay_hal_ozo... ui/gfx/ozone/overlay_hal_ozone.h:19: struct OverlaySurfaceCandidate { would we ever want to tag this with the gfx::AcceleratedWidget associated with this overlay? https://codereview.chromium.org/183723003/diff/1/ui/gfx/ozone/overlay_hal_ozo... ui/gfx/ozone/overlay_hal_ozone.h:25: gfx::Rect rect; This is int coordinates right? Would we ever want a float? https://codereview.chromium.org/183723003/diff/1/ui/gfx/ozone/overlay_hal_ozo... ui/gfx/ozone/overlay_hal_ozone.h:32: virtual void CheckOverlaySupport(OverlaySurfaceCandidateList* surfaces) = 0; aka: iCanHazOverOverlayz? (More comment please?) And: this ought ought to have an implementation that does nothing instead of pure virtual so that it's easier to evolve without breaking downstream implementors? https://codereview.chromium.org/183723003/diff/1/ui/gfx/ozone/surface_factory... File ui/gfx/ozone/surface_factory_ozone.h (right): https://codereview.chromium.org/183723003/diff/1/ui/gfx/ozone/surface_factory... ui/gfx/ozone/surface_factory_ozone.h:61: enum BufferFormat { I would imagine that we would add more here. Is there an existing Chrome construct that encapsulates this? Like SkBitmap::Config? In a software compositing case with overlays, each overlay is a SkBitmap so this is perhaps not so silly? https://codereview.chromium.org/183723003/diff/1/ui/gfx/ozone/surface_factory... ui/gfx/ozone/surface_factory_ozone.h:67: // Describes transformation to be applied to the buffer before presenting do we anticipate that there will be more complex transformations? https://codereview.chromium.org/183723003/diff/1/ui/gfx/ozone/surface_factory... ui/gfx/ozone/surface_factory_ozone.h:162: // Sets the overlay plane to switch to at the next page flip. the comment needs to specify what the args are for. in particular, is the AcceleratedWidget realized or not? (and which processes are allowed to call the entry points.) https://codereview.chromium.org/183723003/diff/1/ui/gfx/ozone/surface_factory... ui/gfx/ozone/surface_factory_ozone.h:169: virtual gfx::AcceleratedWidget CreateNativeBuffer(gfx::Size size, The properties of bing an overlay plan end up being part of the AcceleratedWidget yes? Maybe we should change that.
Patch https://codereview.chromium.org/183723003/diff/1/ui/gfx/ozone/overlay_hal_ozo... File ui/gfx/ozone/overlay_hal_ozone.h (right): https://codereview.chromium.org/183723003/diff/1/ui/gfx/ozone/overlay_hal_ozo... ui/gfx/ozone/overlay_hal_ozone.h:17: class GFX_EXPORT OverlayHALOzone { On 2014/02/27 23:18:19, rjkroege wrote: > More explanation comment. > > And I don't like the name. Sorry. :-) OverlayCandidatesOzone? > Done. https://codereview.chromium.org/183723003/diff/1/ui/gfx/ozone/overlay_hal_ozo... ui/gfx/ozone/overlay_hal_ozone.h:19: struct OverlaySurfaceCandidate { On 2014/02/27 23:18:19, rjkroege wrote: > would we ever want to tag this with the gfx::AcceleratedWidget associated with > this overlay? Sometimes there is no widget yet. The compositor may be asking if a configuration of background|webgl|html controls is possible and if it is, it'll place the html controls into a new buffer. https://codereview.chromium.org/183723003/diff/1/ui/gfx/ozone/overlay_hal_ozo... ui/gfx/ozone/overlay_hal_ozone.h:25: gfx::Rect rect; On 2014/02/27 23:18:19, rjkroege wrote: > This is int coordinates right? Would we ever want a float? This is pixel positions, both HWC and DRM have it as int's. https://codereview.chromium.org/183723003/diff/1/ui/gfx/ozone/overlay_hal_ozo... ui/gfx/ozone/overlay_hal_ozone.h:32: virtual void CheckOverlaySupport(OverlaySurfaceCandidateList* surfaces) = 0; On 2014/02/27 23:18:19, rjkroege wrote: > aka: iCanHazOverOverlayz? (More comment please?) > > And: this ought ought to have an implementation that does nothing instead of > pure virtual so that it's easier to evolve without breaking downstream > implementors? > > > Done. https://codereview.chromium.org/183723003/diff/1/ui/gfx/ozone/surface_factory... File ui/gfx/ozone/surface_factory_ozone.h (right): https://codereview.chromium.org/183723003/diff/1/ui/gfx/ozone/surface_factory... ui/gfx/ozone/surface_factory_ozone.h:61: enum BufferFormat { On 2014/02/27 23:18:19, rjkroege wrote: > I would imagine that we would add more here. > > Is there an existing Chrome construct that encapsulates this? Like > SkBitmap::Config? > > In a software compositing case with overlays, each overlay is a SkBitmap so this > is perhaps not so silly? We may have some new formats like multiplane YUV that are not yet defined elsewhere, so I'd like to keep this here. https://codereview.chromium.org/183723003/diff/1/ui/gfx/ozone/surface_factory... ui/gfx/ozone/surface_factory_ozone.h:67: // Describes transformation to be applied to the buffer before presenting On 2014/02/27 23:18:19, rjkroege wrote: > do we anticipate that there will be more complex transformations? Possibly, but let's walk before we run. This exposes things in a way similar to HWC, so for now it's a good approximation. https://codereview.chromium.org/183723003/diff/1/ui/gfx/ozone/surface_factory... ui/gfx/ozone/surface_factory_ozone.h:162: // Sets the overlay plane to switch to at the next page flip. On 2014/02/27 23:18:19, rjkroege wrote: > the comment needs to specify what the args are for. in particular, is the > AcceleratedWidget realized or not? (and which processes are allowed to call the > entry points.) Done. https://codereview.chromium.org/183723003/diff/1/ui/gfx/ozone/surface_factory... ui/gfx/ozone/surface_factory_ozone.h:169: virtual gfx::AcceleratedWidget CreateNativeBuffer(gfx::Size size, On 2014/02/27 23:18:19, rjkroege wrote: > The properties of bing an overlay plan end up being part of the > AcceleratedWidget yes? Maybe we should change that. > Done.
more bike-shedding. sorry. https://codereview.chromium.org/183723003/diff/40001/ui/gfx/ozone/overlay_can... File ui/gfx/ozone/overlay_candidates_ozone.h (right): https://codereview.chromium.org/183723003/diff/40001/ui/gfx/ozone/overlay_can... ui/gfx/ozone/overlay_candidates_ozone.h:30: gfx::Rect display_rect; if these are not the same, how should the hardware composer handle it? https://codereview.chromium.org/183723003/diff/40001/ui/gfx/ozone/surface_fac... File ui/gfx/ozone/surface_factory_ozone.h (right): https://codereview.chromium.org/183723003/diff/40001/ui/gfx/ozone/surface_fac... ui/gfx/ozone/surface_factory_ozone.h:64: RGBA_8888, in little endian? https://codereview.chromium.org/183723003/diff/40001/ui/gfx/ozone/surface_fac... ui/gfx/ozone/surface_factory_ozone.h:65: RGB_888, padded or un-padded? https://codereview.chromium.org/183723003/diff/40001/ui/gfx/ozone/surface_fac... ui/gfx/ozone/surface_factory_ozone.h:70: enum OverlayTransform { some kinds of scaling and colour space conversions are possible on some hardware. Is it worth adding this now? https://codereview.chromium.org/183723003/diff/40001/ui/gfx/ozone/surface_fac... ui/gfx/ozone/surface_factory_ozone.h:161: virtual gfx::OverlayCandidatesOzone* GetOverlayHAL(); is the name of the method a bit weird now? https://codereview.chromium.org/183723003/diff/40001/ui/gfx/ozone/surface_fac... ui/gfx/ozone/surface_factory_ozone.h:163: // Sets the overlay plane to switch to at the next page flip. bike-sheddery: I'd call this "ScheduleOverlayPlaneChange" or something like that? At any rate, maybe a more descriptive word than "Set". https://codereview.chromium.org/183723003/diff/40001/ui/gfx/ozone/surface_fac... ui/gfx/ozone/surface_factory_ozone.h:167: // |handle| is the native buffer to be presented by the overlay. you could call it "buffer" https://codereview.chromium.org/183723003/diff/40001/ui/gfx/ozone/surface_fac... ui/gfx/ozone/surface_factory_ozone.h:169: // |crop_rect| specifies the region within the buffer. per other file comments: what is expected of implementors when these are not the same size https://codereview.chromium.org/183723003/diff/40001/ui/gfx/ozone/surface_fac... ui/gfx/ozone/surface_factory_ozone.h:170: virtual void SetOverlayPlane(int plane_id, once we have multiple display support, this needs to take an acceleratedWidget handle too... maybe add it now?
Patch. https://codereview.chromium.org/183723003/diff/40001/ui/gfx/ozone/overlay_can... File ui/gfx/ozone/overlay_candidates_ozone.h (right): https://codereview.chromium.org/183723003/diff/40001/ui/gfx/ozone/overlay_can... ui/gfx/ozone/overlay_candidates_ozone.h:30: gfx::Rect display_rect; On 2014/02/28 23:02:47, rjkroege wrote: > if these are not the same, how should the hardware composer handle it? Like texturing in GL, you put the crop rect from the buffer into display rect, resizing as necessary. https://codereview.chromium.org/183723003/diff/40001/ui/gfx/ozone/surface_fac... File ui/gfx/ozone/surface_factory_ozone.h (right): https://codereview.chromium.org/183723003/diff/40001/ui/gfx/ozone/surface_fac... ui/gfx/ozone/surface_factory_ozone.h:65: RGB_888, On 2014/02/28 23:02:47, rjkroege wrote: > padded or un-padded? packed. https://codereview.chromium.org/183723003/diff/40001/ui/gfx/ozone/surface_fac... ui/gfx/ozone/surface_factory_ozone.h:70: enum OverlayTransform { On 2014/02/28 23:02:47, rjkroege wrote: > some kinds of scaling and colour space conversions are possible on some > hardware. Is it worth adding this now? I think scaling is universally supported and could be handled via the display_rect code. Color space conversion would already be handled as part of the format. In other words, can I haz scanout from UYV? If the answer is yes, that means color conversion will happen in the display controller. https://codereview.chromium.org/183723003/diff/40001/ui/gfx/ozone/surface_fac... ui/gfx/ozone/surface_factory_ozone.h:161: virtual gfx::OverlayCandidatesOzone* GetOverlayHAL(); On 2014/02/28 23:02:47, rjkroege wrote: > is the name of the method a bit weird now? Oh yean, I'll change. https://codereview.chromium.org/183723003/diff/40001/ui/gfx/ozone/surface_fac... ui/gfx/ozone/surface_factory_ozone.h:163: // Sets the overlay plane to switch to at the next page flip. On 2014/02/28 23:02:47, rjkroege wrote: > bike-sheddery: I'd call this "ScheduleOverlayPlaneChange" or something like > that? At any rate, maybe a more descriptive word than "Set". Done. https://codereview.chromium.org/183723003/diff/40001/ui/gfx/ozone/surface_fac... ui/gfx/ozone/surface_factory_ozone.h:167: // |handle| is the native buffer to be presented by the overlay. On 2014/02/28 23:02:47, rjkroege wrote: > you could call it "buffer" Done. https://codereview.chromium.org/183723003/diff/40001/ui/gfx/ozone/surface_fac... ui/gfx/ozone/surface_factory_ozone.h:169: // |crop_rect| specifies the region within the buffer. On 2014/02/28 23:02:47, rjkroege wrote: > per other file comments: what is expected of implementors when these are not the > same size Project crop rect within the buffer into display_rect. Similar to how GL texture would work. https://codereview.chromium.org/183723003/diff/40001/ui/gfx/ozone/surface_fac... ui/gfx/ozone/surface_factory_ozone.h:170: virtual void SetOverlayPlane(int plane_id, On 2014/02/28 23:02:47, rjkroege wrote: > once we have multiple display support, this needs to take an acceleratedWidget > handle too... maybe add it now? I thought we were going to have one of these factories or some subset per display?
https://codereview.chromium.org/183723003/diff/150001/ui/gfx/ozone/overlay_ca... File ui/gfx/ozone/overlay_candidates_ozone.h (right): https://codereview.chromium.org/183723003/diff/150001/ui/gfx/ozone/overlay_ca... ui/gfx/ozone/overlay_candidates_ozone.h:17: // This class that can be used to answer questions about possible overlay s/that// https://codereview.chromium.org/183723003/diff/150001/ui/gfx/ozone/overlay_ca... ui/gfx/ozone/overlay_candidates_ozone.h:19: class GFX_EXPORT OverlayCandidatesOzone { we discussed how this dangles of SFOz because it needs to be per-display (aka AcceleratedWidget) right? so the way that you've arranged this is to permit SFOz to continue being a singleton? If I am right in this, please augment the comment. https://codereview.chromium.org/183723003/diff/150001/ui/gfx/ozone/overlay_ca... ui/gfx/ozone/overlay_candidates_ozone.h:28: SurfaceFactoryOzone::BufferFormat format; some of the SoCs have display generators of varying complexity. I assume we can grow the BufferFormat list to encompass such a thing? https://codereview.chromium.org/183723003/diff/150001/ui/gfx/ozone/overlay_ca... ui/gfx/ozone/overlay_candidates_ozone.h:32: gfx::RectF crop_rect; we could imagine over-drawing to pan via hardware display adjustment. Some day. That would be cool... https://codereview.chromium.org/183723003/diff/150001/ui/gfx/ozone/surface_fa... File ui/gfx/ozone/surface_factory_ozone.h (right): https://codereview.chromium.org/183723003/diff/150001/ui/gfx/ozone/surface_fa... ui/gfx/ozone/surface_factory_ozone.h:163: virtual gfx::OverlayCandidatesOzone* GetOverlayCandidates(); there is conceptually a separate OCOz per display so this should probably take an AcceleratedWidget. also: how does atomic flip work with multi-mon? https://codereview.chromium.org/183723003/diff/150001/ui/gfx/ozone/surface_fa... ui/gfx/ozone/surface_factory_ozone.h:166: // |plane_id| specifies the location of the plane above the main framebuffer. the id is merely to specify the the stacking order of the planes? This needs to be stated I think. Perhaps it should be renamed as "plane_z_order" or something similar? this gets called for every plane? is the base plane implied? does there even need to be a base plane? Some SoCs won't have such a thing. I would have think that "base plane" is 0? I am opposed to treating the base plane as special. do the set planes persist? i.e.: after 2 flips if nothing is called, what planes are on the display? perhaps this call should take a flag to "persist" or "transient" the plane on 2nd flip (well, 2nd vblank really when scan out is done)
patch https://codereview.chromium.org/183723003/diff/150001/ui/gfx/ozone/overlay_ca... File ui/gfx/ozone/overlay_candidates_ozone.h (right): https://codereview.chromium.org/183723003/diff/150001/ui/gfx/ozone/overlay_ca... ui/gfx/ozone/overlay_candidates_ozone.h:17: // This class that can be used to answer questions about possible overlay On 2014/03/04 18:54:08, rjkroege wrote: > s/that// Done. https://codereview.chromium.org/183723003/diff/150001/ui/gfx/ozone/overlay_ca... ui/gfx/ozone/overlay_candidates_ozone.h:28: SurfaceFactoryOzone::BufferFormat format; On 2014/03/04 18:54:08, rjkroege wrote: > some of the SoCs have display generators of varying complexity. I assume we can > grow the BufferFormat list to encompass such a thing? Yes, the format is a placeholder for now. https://codereview.chromium.org/183723003/diff/150001/ui/gfx/ozone/overlay_ca... ui/gfx/ozone/overlay_candidates_ozone.h:32: gfx::RectF crop_rect; On 2014/03/04 18:54:08, rjkroege wrote: > we could imagine over-drawing to pan via hardware display adjustment. Some day. > That would be cool... That would actually come for free if CC chose to allocate a layer with one of these native buffers, that's why the rect is there. https://codereview.chromium.org/183723003/diff/150001/ui/gfx/ozone/surface_fa... File ui/gfx/ozone/surface_factory_ozone.h (right): https://codereview.chromium.org/183723003/diff/150001/ui/gfx/ozone/surface_fa... ui/gfx/ozone/surface_factory_ozone.h:166: // |plane_id| specifies the location of the plane above the main framebuffer. On 2014/03/04 18:54:08, rjkroege wrote: > the id is merely to specify the the stacking order of the planes? This needs to > be stated I think. Perhaps it should be renamed as "plane_z_order" or something > similar? > > this gets called for every plane? is the base plane implied? does there even > need to be a base plane? Some SoCs won't have such a thing. I would have think > that "base plane" is 0? > > I am opposed to treating the base plane as special. > > do the set planes persist? i.e.: after 2 flips if nothing is called, what planes > are on the display? perhaps this call should take a flag to "persist" or > "transient" the plane on 2nd flip (well, 2nd vblank really when scan out is > done) This is z_order, I'll rename. We get the base plane (plane 0) from eglSwapBuffers via the platform implementation, nothing prevents us from setting it here too. I envisioned this as persistent, same as the underlying api's, if you set the plane, it stays there until you reset it, I don't want to destroy the image on the screen after a couple of scanouts. If CC doesn't need to redraw, we should be able to keep scanning out from the same buffers.
lgtm
The CQ bit was checked by alexst@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexst@chromium.org/183723003/190001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by alexst@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexst@chromium.org/183723003/190001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) browser_tests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by alexst@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexst@chromium.org/183723003/190001
Message was sent while issue was closed.
Change committed as 255153 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
