Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
489193002:
Ozone overlay candidates (Closed)
DescriptionSingle overlay test selector
Selects one on top overlay, if possible, from a main plane + single overlay combination
BUG=370522
Committed: https://crrev.com/c2761c323712d2fa9211169ebc68b70840eecbe7
Cr-Commit-Position: refs/heads/master@{#291627}
Patch Set 1 #
Total comments: 20
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : remove implicit ordering #
Total comments: 1
Patch Set 5 : #
Messages
Total messages: 16 (0 generated)
Could you please make the CL title and description a bit more informative? https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... File ui/ozone/platform/dri/gbm_surface_factory.cc (right): https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... ui/ozone/platform/dri/gbm_surface_factory.cc:20: namespace { Please add blank line between namespaces and between namespace and class (same at the end). https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... ui/ozone/platform/dri/gbm_surface_factory.cc:21: class SingleOverlayOzone : public OverlayCandidatesOzone { As a rule, please don't use Ozone in internal implementation names. https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... ui/ozone/platform/dri/gbm_surface_factory.cc:22: public: needs a virtual destructor. Also, please add a DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... ui/ozone/platform/dri/gbm_surface_factory.cc:23: static bool IsTransformSupported(gfx::OverlayTransform transform) { There's no need for a static, just make it private. https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... ui/ozone/platform/dri/gbm_surface_factory.cc:31: virtual void CheckOverlaySupport(OverlaySurfaceCandidateList* surfaces) { nit: rename surfaces to candidates (or surface_candidates) https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... ui/ozone/platform/dri/gbm_surface_factory.cc:33: const OverlayCandidatesOzone::OverlaySurfaceCandidate& first = Is this one marked to true by default? It isn't specified in the documentation. Should we just treat all the candidates the same and mark it as handled in here?
https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... File ui/ozone/platform/dri/gbm_surface_factory.cc (right): https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... ui/ozone/platform/dri/gbm_surface_factory.cc:32: if (surfaces->size() == 2) { So this isn't universally true, some HW may not even have an overlay, and so this will break things. GBM platform is becoming a generic ChromeOS one. I think for testing it's useful, but we should hide this under a flag, --ozone-test-support-one-overlay or something. In the general case, we should add a package to the CROS image, call it overlay_candidates.so which would then handle all the device specifics. We should load it here and defer to HW.
https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... File ui/ozone/platform/dri/gbm_surface_factory.cc (right): https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... ui/ozone/platform/dri/gbm_surface_factory.cc:20: namespace { On 2014/08/20 19:36:38, dnicoara wrote: > Please add blank line between namespaces and between namespace and class (same > at the end). Done. https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... ui/ozone/platform/dri/gbm_surface_factory.cc:21: class SingleOverlayOzone : public OverlayCandidatesOzone { On 2014/08/20 19:36:38, dnicoara wrote: > As a rule, please don't use Ozone in internal implementation names. Acknowledged. https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... ui/ozone/platform/dri/gbm_surface_factory.cc:22: public: On 2014/08/20 19:36:39, dnicoara wrote: > needs a virtual destructor. Also, please add a DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... ui/ozone/platform/dri/gbm_surface_factory.cc:32: if (surfaces->size() == 2) { On 2014/08/20 19:52:36, alexst wrote: > So this isn't universally true, some HW may not even have an overlay, and so > this will break things. GBM platform is becoming a generic ChromeOS one. > > I think for testing it's useful, but we should hide this under a flag, > --ozone-test-support-one-overlay or something. > > In the general case, we should add a package to the CROS image, call it > overlay_candidates.so which would then handle all the device specifics. We > should load it here and defer to HW. Sure, a flag makes sense https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... ui/ozone/platform/dri/gbm_surface_factory.cc:33: const OverlayCandidatesOzone::OverlaySurfaceCandidate& first = On 2014/08/20 19:36:38, dnicoara wrote: > Is this one marked to true by default? It isn't specified in the documentation. > Should we just treat all the candidates the same and mark it as handled in here? I think the idea is we mark all surfaces we want to get sent separately
https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... File ui/ozone/platform/dri/gbm_surface_factory.cc (right): https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... ui/ozone/platform/dri/gbm_surface_factory.cc:33: const OverlayCandidatesOzone::OverlaySurfaceCandidate& first = On 2014/08/21 15:16:32, achaulk wrote: > On 2014/08/20 19:36:38, dnicoara wrote: > > Is this one marked to true by default? It isn't specified in the > documentation. > > Should we just treat all the candidates the same and mark it as handled in > here? > > I think the idea is we mark all surfaces we want to get sent separately Sorry, I think something went over my head. Is the primary plane treated specially? Shouldn't we treat it the same way. I'm thinking of what happens if we want to do some unsupported transformation on the primary? https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... ui/ozone/platform/dri/gbm_surface_factory.cc:36: if (first.plane_z_order == 0 && second.plane_z_order > 0 && Are these guaranteed to be in order? Why not treat them all the same?
https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... File ui/ozone/platform/dri/gbm_surface_factory.cc (right): https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... ui/ozone/platform/dri/gbm_surface_factory.cc:33: const OverlayCandidatesOzone::OverlaySurfaceCandidate& first = On 2014/08/21 15:57:20, dnicoara wrote: > On 2014/08/21 15:16:32, achaulk wrote: > > On 2014/08/20 19:36:38, dnicoara wrote: > > > Is this one marked to true by default? It isn't specified in the > > documentation. > > > Should we just treat all the candidates the same and mark it as handled in > > here? > > > > I think the idea is we mark all surfaces we want to get sent separately > > Sorry, I think something went over my head. Is the primary plane treated > specially? Shouldn't we treat it the same way. I'm thinking of what happens if > we want to do some unsupported transformation on the primary? Well normally the primary plane wouldn't get transformed at all right? And it won't be sent separately as an overlay https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... ui/ozone/platform/dri/gbm_surface_factory.cc:36: if (first.plane_z_order == 0 && second.plane_z_order > 0 && On 2014/08/21 15:57:20, dnicoara wrote: > Are these guaranteed to be in order? Why not treat them all the same? They are in the only thing that sends us planes so far
https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... File ui/ozone/platform/dri/gbm_surface_factory.cc (right): https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... ui/ozone/platform/dri/gbm_surface_factory.cc:33: const OverlayCandidatesOzone::OverlaySurfaceCandidate& first = On 2014/08/21 16:04:29, achaulk wrote: > On 2014/08/21 15:57:20, dnicoara wrote: > > On 2014/08/21 15:16:32, achaulk wrote: > > > On 2014/08/20 19:36:38, dnicoara wrote: > > > > Is this one marked to true by default? It isn't specified in the > > > documentation. > > > > Should we just treat all the candidates the same and mark it as handled in > > > here? > > > > > > I think the idea is we mark all surfaces we want to get sent separately > > > > Sorry, I think something went over my head. Is the primary plane treated > > specially? Shouldn't we treat it the same way. I'm thinking of what happens if > > we want to do some unsupported transformation on the primary? > > Well normally the primary plane wouldn't get transformed at all right? And it > won't be sent separately as an overlay But in surfaceless, the primary plane is an "overlay", isn't it? At least it is part of |surfaces|, so I'd assume we want to do some basic assertions. https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... ui/ozone/platform/dri/gbm_surface_factory.cc:36: if (first.plane_z_order == 0 && second.plane_z_order > 0 && On 2014/08/21 16:04:29, achaulk wrote: > On 2014/08/21 15:57:20, dnicoara wrote: > > Are these guaranteed to be in order? Why not treat them all the same? > > They are in the only thing that sends us planes so far I think we shouldn't assume order. Or if we do, then we should make it explicit in the interface description. If we assume order, it might make more sense to move 'first.plane_z_order == 0' into a DCHECK.
https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... File ui/ozone/platform/dri/gbm_surface_factory.cc (right): https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... ui/ozone/platform/dri/gbm_surface_factory.cc:33: const OverlayCandidatesOzone::OverlaySurfaceCandidate& first = On 2014/08/22 02:00:15, dnicoara wrote: > On 2014/08/21 16:04:29, achaulk wrote: > > On 2014/08/21 15:57:20, dnicoara wrote: > > > On 2014/08/21 15:16:32, achaulk wrote: > > > > On 2014/08/20 19:36:38, dnicoara wrote: > > > > > Is this one marked to true by default? It isn't specified in the > > > > documentation. > > > > > Should we just treat all the candidates the same and mark it as handled > in > > > > here? > > > > > > > > I think the idea is we mark all surfaces we want to get sent separately > > > > > > Sorry, I think something went over my head. Is the primary plane treated > > > specially? Shouldn't we treat it the same way. I'm thinking of what happens > if > > > we want to do some unsupported transformation on the primary? > > > > Well normally the primary plane wouldn't get transformed at all right? And it > > won't be sent separately as an overlay > > But in surfaceless, the primary plane is an "overlay", isn't it? At least it is > part of |surfaces|, so I'd assume we want to do some basic assertions. Yes, but surfaceless unconditionally sends the primary plane as an overlay - it's independent from this path
https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... File ui/ozone/platform/dri/gbm_surface_factory.cc (right): https://codereview.chromium.org/489193002/diff/1/ui/ozone/platform/dri/gbm_su... ui/ozone/platform/dri/gbm_surface_factory.cc:36: if (first.plane_z_order == 0 && second.plane_z_order > 0 && On 2014/08/22 02:00:14, dnicoara wrote: > On 2014/08/21 16:04:29, achaulk wrote: > > On 2014/08/21 15:57:20, dnicoara wrote: > > > Are these guaranteed to be in order? Why not treat them all the same? > > > > They are in the only thing that sends us planes so far > > I think we shouldn't assume order. Or if we do, then we should make it explicit > in the interface description. > > If we assume order, it might make more sense to move 'first.plane_z_order == 0' > into a DCHECK. Done.
lgtm with a nit. https://codereview.chromium.org/489193002/diff/60001/ui/ozone/platform/dri/gb... File ui/ozone/platform/dri/gbm_surface_factory.cc (right): https://codereview.chromium.org/489193002/diff/60001/ui/ozone/platform/dri/gb... ui/ozone/platform/dri/gbm_surface_factory.cc:179: return new OverlayCandidatesOzone(); If we can't support single overlay, we should return NULL because void OverlayCandidatesOzone::CheckOverlaySupport( OverlaySurfaceCandidateList* surfaces) { NOTREACHED(); } Default implementation is only there as a guard against breaking upstream implementers' build with interface changes.
ui/ozone/public lgtm
lgtm
The CQ bit was checked by achaulk@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/achaulk@chromium.org/489193002/80001
Message was sent while issue was closed.
Committed patchset #5 (80001) as 0ba009150be63618bb51e1ae4dcbfca5ba4cae93
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c2761c323712d2fa9211169ebc68b70840eecbe7 Cr-Commit-Position: refs/heads/master@{#291627} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
