Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1110813002:
Fix assert in GetOverlayRect when transforming video with Ozone overlay (Closed)
DescriptionFix assert in GetOverlayRect when transforming video with Ozone overlay
One of my testcases for updating Chromecast to use Ozone overlays has
css animation on a video element's transform. I hit an assert in the
call to ToNearestRect, which assumes the coordinates of the rectangle
are close to integers (within 0.01).
BUG=
Committed: https://crrev.com/56e6db77baec30e48ef94eae9f81e2793db637ea
Cr-Commit-Position: refs/heads/master@{#328218}
Patch Set 1 #Patch Set 2 : Adds unit test #Patch Set 3 : Changes display_rect to float #
Total comments: 5
Patch Set 4 : Added comments on need to snap display_rect when accepting #Patch Set 5 : Reject non-integer rects in Ozone GBM validator #
Total comments: 6
Patch Set 6 : Some nits. #
Messages
Total messages: 34 (5 generated)
halliwell@chromium.org changed reviewers: + achaulk@chromium.org, enne@chromium.org
danakj@chromium.org changed reviewers: + danakj@chromium.org
Unit test?
On 2015/04/27 23:41:24, danakj wrote: > Unit test? Unit test added.
On 2015/04/28 15:43:44, halliwell wrote: > On 2015/04/27 23:41:24, danakj wrote: > > Unit test? > > Unit test added. PTAL
maybe achaulk can answer this, but how can an overlay be positioned at non-integer positions. i assumed they have to be at physical pixels or we'll get weird edges
Yeah, it may look different if we have this situation, the hardware will only work on pixel boundaries. It probably wouldn't be a problem if we could guarantee that we can always show it in an overlay, but since we do it per-frame, maybe we should reject this case. Probably do it in GetOverlayTransform
On 2015/04/29 17:03:05, achaulk wrote: > Yeah, it may look different if we have this situation, the hardware will only > work on pixel boundaries. It probably wouldn't be a problem if we could > guarantee that we can always show it in an overlay, but since we do it > per-frame, maybe we should reject this case. Probably do it in > GetOverlayTransform On Chromecast, we can *only* draw video in an overlay. So we can't afford to reject cases like this - it would result in video disappearing. If you need to reject for the general case, then we probably need to refactor things so we can substitute our own rejection logic?
On Wed, Apr 29, 2015 at 10:08 AM, <halliwell@chromium.org> wrote: > On 2015/04/29 17:03:05, achaulk wrote: > >> Yeah, it may look different if we have this situation, the hardware will >> only >> work on pixel boundaries. It probably wouldn't be a problem if we could >> guarantee that we can always show it in an overlay, but since we do it >> per-frame, maybe we should reject this case. Probably do it in >> GetOverlayTransform >> > > On Chromecast, we can *only* draw video in an overlay. So we can't afford > to > reject cases like this - it would result in video disappearing. > > If you need to reject for the general case, then we probably need to > refactor > things so we can substitute our own rejection logic? > Or change animations to snap to pixel positions. > > https://codereview.chromium.org/1110813002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/04/29 17:08:41, halliwell wrote: > On 2015/04/29 17:03:05, achaulk wrote: > > Yeah, it may look different if we have this situation, the hardware will only > > work on pixel boundaries. It probably wouldn't be a problem if we could > > guarantee that we can always show it in an overlay, but since we do it > > per-frame, maybe we should reject this case. Probably do it in > > GetOverlayTransform > > On Chromecast, we can *only* draw video in an overlay. So we can't afford to > reject cases like this - it would result in video disappearing. > > If you need to reject for the general case, then we probably need to refactor > things so we can substitute our own rejection logic? The easy way to do this is to change display_rect to a RectF and do the conversions only within the OverlayCandidatesOzone implementation. We can reject, you can accept
On 2015/04/29 17:12:22, achaulk wrote: > On 2015/04/29 17:08:41, halliwell wrote: > > On 2015/04/29 17:03:05, achaulk wrote: > > > Yeah, it may look different if we have this situation, the hardware will > only > > > work on pixel boundaries. It probably wouldn't be a problem if we could > > > guarantee that we can always show it in an overlay, but since we do it > > > per-frame, maybe we should reject this case. Probably do it in > > > GetOverlayTransform > > > > On Chromecast, we can *only* draw video in an overlay. So we can't afford to > > reject cases like this - it would result in video disappearing. > > > > If you need to reject for the general case, then we probably need to refactor > > things so we can substitute our own rejection logic? > > The easy way to do this is to change display_rect to a RectF and do the > conversions only within the OverlayCandidatesOzone implementation. We can > reject, you can accept Snapping animation to pixels sounds like it would theoretically be more visually correct. But I don't know what's involved in that, or what other implications it would have. Do you think this is something worth pursuing? On the other hand, it's worth pointing out that we don't currently have a solution for synchronising the overlay frames with the GL frames on our hardware, so we have visual discrepancies along the edges with animations regardless :( Given that, moving the conversion to OverlayCandidatesOzone sounds acceptable for now, and looks straightforward to implement.
On Wed, Apr 29, 2015 at 10:26 AM, <halliwell@chromium.org> wrote: > On 2015/04/29 17:12:22, achaulk wrote: > >> On 2015/04/29 17:08:41, halliwell wrote: >> > On 2015/04/29 17:03:05, achaulk wrote: >> > > Yeah, it may look different if we have this situation, the hardware >> will >> only >> > > work on pixel boundaries. It probably wouldn't be a problem if we >> could >> > > guarantee that we can always show it in an overlay, but since we do it >> > > per-frame, maybe we should reject this case. Probably do it in >> > > GetOverlayTransform >> > >> > On Chromecast, we can *only* draw video in an overlay. So we can't >> afford >> > to > >> > reject cases like this - it would result in video disappearing. >> > >> > If you need to reject for the general case, then we probably need to >> > refactor > >> > things so we can substitute our own rejection logic? >> > > The easy way to do this is to change display_rect to a RectF and do the >> conversions only within the OverlayCandidatesOzone implementation. We can >> reject, you can accept >> > > Snapping animation to pixels sounds like it would theoretically be more > visually > correct. But I don't know what's involved in that, or what other > implications > it would have. Do you think this is something worth pursuing? > Perhaps with keyframe animations that move an integer at a time? > On the other hand, it's worth pointing out that we don't currently have a > solution for synchronising the overlay frames with the GL frames on our > hardware, so we have visual discrepancies along the edges with animations > regardless :( Given that, moving the conversion to OverlayCandidatesOzone > sounds acceptable for now, and looks straightforward to implement. > https://codereview.chromium.org/1110813002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
halliwell@chromium.org changed reviewers: + alexst@chromium.org
On 2015/04/29 17:53:45, danakj wrote: > On Wed, Apr 29, 2015 at 10:26 AM, <mailto:halliwell@chromium.org> wrote: > > > On 2015/04/29 17:12:22, achaulk wrote: > > > >> On 2015/04/29 17:08:41, halliwell wrote: > >> > On 2015/04/29 17:03:05, achaulk wrote: > >> > > Yeah, it may look different if we have this situation, the hardware > >> will > >> only > >> > > work on pixel boundaries. It probably wouldn't be a problem if we > >> could > >> > > guarantee that we can always show it in an overlay, but since we do it > >> > > per-frame, maybe we should reject this case. Probably do it in > >> > > GetOverlayTransform > >> > > >> > On Chromecast, we can *only* draw video in an overlay. So we can't > >> afford > >> > > to > > > >> > reject cases like this - it would result in video disappearing. > >> > > >> > If you need to reject for the general case, then we probably need to > >> > > refactor > > > >> > things so we can substitute our own rejection logic? > >> > > > > The easy way to do this is to change display_rect to a RectF and do the > >> conversions only within the OverlayCandidatesOzone implementation. We can > >> reject, you can accept > >> > > > > Snapping animation to pixels sounds like it would theoretically be more > > visually > > correct. But I don't know what's involved in that, or what other > > implications > > it would have. Do you think this is something worth pursuing? > > > > Perhaps with keyframe animations that move an integer at a time? > I don't follow. I'm hitting this assert on a css transform animation that moves a video element across the screen between just 2 keyframes; interpolation leads to floating-point transform values in between. I followed achaulk's suggestion and changed display_rect to float. > > > > On the other hand, it's worth pointing out that we don't currently have a > > solution for synchronising the overlay frames with the GL frames on our > > hardware, so we have visual discrepancies along the edges with animations > > regardless :( Given that, moving the conversion to OverlayCandidatesOzone > > sounds acceptable for now, and looks straightforward to implement. > > > > https://codereview.chromium.org/1110813002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On Wed, Apr 29, 2015 at 2:31 PM, <halliwell@chromium.org> wrote: > On 2015/04/29 17:53:45, danakj wrote: > > On Wed, Apr 29, 2015 at 10:26 AM, <mailto:halliwell@chromium.org> wrote: >> > > > On 2015/04/29 17:12:22, achaulk wrote: >> > >> >> On 2015/04/29 17:08:41, halliwell wrote: >> >> > On 2015/04/29 17:03:05, achaulk wrote: >> >> > > Yeah, it may look different if we have this situation, the hardware >> >> will >> >> only >> >> > > work on pixel boundaries. It probably wouldn't be a problem if we >> >> could >> >> > > guarantee that we can always show it in an overlay, but since we >> do it >> >> > > per-frame, maybe we should reject this case. Probably do it in >> >> > > GetOverlayTransform >> >> > >> >> > On Chromecast, we can *only* draw video in an overlay. So we can't >> >> afford >> >> >> > to >> > >> >> > reject cases like this - it would result in video disappearing. >> >> > >> >> > If you need to reject for the general case, then we probably need to >> >> >> > refactor >> > >> >> > things so we can substitute our own rejection logic? >> >> >> > >> > The easy way to do this is to change display_rect to a RectF and do the >> >> conversions only within the OverlayCandidatesOzone implementation. We >> can >> >> reject, you can accept >> >> >> > >> > Snapping animation to pixels sounds like it would theoretically be more >> > visually >> > correct. But I don't know what's involved in that, or what other >> > implications >> > it would have. Do you think this is something worth pursuing? >> > >> > > Perhaps with keyframe animations that move an integer at a time? >> > > > I don't follow. I'm hitting this assert on a css transform animation that > moves > a video element across the screen between just 2 keyframes; interpolation > leads > to floating-point transform values in between. > I probably don't understand css animation. I was trying to suggest a stepwise rather than continuous animation. > > I followed achaulk's suggestion and changed display_rect to float. > > > > > On the other hand, it's worth pointing out that we don't currently have >> a >> > solution for synchronising the overlay frames with the GL frames on our >> > hardware, so we have visual discrepancies along the edges with >> animations >> > regardless :( Given that, moving the conversion to >> OverlayCandidatesOzone >> > sounds acceptable for now, and looks straightforward to implement. >> > > > > https://codereview.chromium.org/1110813002/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > > https://codereview.chromium.org/1110813002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1110813002/diff/40001/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/1110813002/diff/40001/cc/output/gl_renderer.c... cc/output/gl_renderer.cc:3452: display_rect, You'll still hit the same problem here, no? On your implementation you'll accept it and it will get here https://codereview.chromium.org/1110813002/diff/40001/cc/output/overlay_unitt... File cc/output/overlay_unittest.cc (right): https://codereview.chromium.org/1110813002/diff/40001/cc/output/overlay_unitt... cc/output/overlay_unittest.cc:67: if (!gfx::IsNearestRectWithinDistance(candidate.display_rect, 0.01f)) I'm not really sure this is useful - the test code is just validating the test code
https://codereview.chromium.org/1110813002/diff/40001/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/1110813002/diff/40001/cc/output/gl_renderer.c... cc/output/gl_renderer.cc:3452: display_rect, On 2015/04/29 21:37:03, achaulk wrote: > You'll still hit the same problem here, no? On your implementation you'll accept > it and it will get here Yep, unless we modify the transform when accepting, but that's maybe not the intended use of the API? What did you have in mind - should we push the float rect right through to ScheduleOverlayPlane? https://codereview.chromium.org/1110813002/diff/40001/cc/output/overlay_unitt... File cc/output/overlay_unittest.cc (right): https://codereview.chromium.org/1110813002/diff/40001/cc/output/overlay_unitt... cc/output/overlay_unittest.cc:67: if (!gfx::IsNearestRectWithinDistance(candidate.display_rect, 0.01f)) On 2015/04/29 21:37:03, achaulk wrote: > I'm not really sure this is useful - the test code is just validating the test > code Agreed, will delete this test now.
https://codereview.chromium.org/1110813002/diff/40001/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/1110813002/diff/40001/cc/output/gl_renderer.c... cc/output/gl_renderer.cc:3452: display_rect, On 2015/04/29 21:42:54, halliwell wrote: > On 2015/04/29 21:37:03, achaulk wrote: > > You'll still hit the same problem here, no? On your implementation you'll > accept > > it and it will get here > > Yep, unless we modify the transform when accepting, but that's maybe not the > intended use of the API? > > What did you have in mind - should we push the float rect right through to > ScheduleOverlayPlane? Probably change this to accept a float as well I would say
On 2015/04/29 21:45:00, achaulk wrote: > https://codereview.chromium.org/1110813002/diff/40001/cc/output/gl_renderer.cc > File cc/output/gl_renderer.cc (right): > > https://codereview.chromium.org/1110813002/diff/40001/cc/output/gl_renderer.c... > cc/output/gl_renderer.cc:3452: display_rect, > On 2015/04/29 21:42:54, halliwell wrote: > > On 2015/04/29 21:37:03, achaulk wrote: > > > You'll still hit the same problem here, no? On your implementation you'll > > accept > > > it and it will get here > > > > Yep, unless we modify the transform when accepting, but that's maybe not the > > intended use of the API? > > > > What did you have in mind - should we push the float rect right through to > > ScheduleOverlayPlane? > > Probably change this to accept a float as well I would say Maybe clipping the display rect in the desired way while accepting is the right thing to do here. Seems like the sort of thing that should be up to the platform, Cast knows that it will always go into the overlay, it should be free to choose a clipping behaviour and proceed.
On 2015/04/30 13:43:52, alexst wrote: > On 2015/04/29 21:45:00, achaulk wrote: > > https://codereview.chromium.org/1110813002/diff/40001/cc/output/gl_renderer.cc > > File cc/output/gl_renderer.cc (right): > > > > > https://codereview.chromium.org/1110813002/diff/40001/cc/output/gl_renderer.c... > > cc/output/gl_renderer.cc:3452: display_rect, > > On 2015/04/29 21:42:54, halliwell wrote: > > > On 2015/04/29 21:37:03, achaulk wrote: > > > > You'll still hit the same problem here, no? On your implementation you'll > > > accept > > > > it and it will get here > > > > > > Yep, unless we modify the transform when accepting, but that's maybe not the > > > intended use of the API? > > > > > > What did you have in mind - should we push the float rect right through to > > > ScheduleOverlayPlane? > > > > Probably change this to accept a float as well I would say > > Maybe clipping the display rect in the desired way while accepting is the right > thing to do here. Seems like the sort of thing that should be up to the > platform, Cast knows that it will always go into the overlay, it should be free > to choose a clipping behaviour and proceed. That's what I had and it was working for our case, that sounds good to me. Maybe I just add some comments there to be clear that you should clip if you accept? I also looked at changing ScheduleOverlayPlane to take floats, it seems to escalate this into a pretty big change and it's not clear anything would actually be making use of that. Thoughts?
On 2015/04/30 14:18:43, halliwell wrote: > On 2015/04/30 13:43:52, alexst wrote: > > On 2015/04/29 21:45:00, achaulk wrote: > > > > https://codereview.chromium.org/1110813002/diff/40001/cc/output/gl_renderer.cc > > > File cc/output/gl_renderer.cc (right): > > > > > > > > > https://codereview.chromium.org/1110813002/diff/40001/cc/output/gl_renderer.c... > > > cc/output/gl_renderer.cc:3452: display_rect, > > > On 2015/04/29 21:42:54, halliwell wrote: > > > > On 2015/04/29 21:37:03, achaulk wrote: > > > > > You'll still hit the same problem here, no? On your implementation > you'll > > > > accept > > > > > it and it will get here > > > > > > > > Yep, unless we modify the transform when accepting, but that's maybe not > the > > > > intended use of the API? > > > > > > > > What did you have in mind - should we push the float rect right through to > > > > ScheduleOverlayPlane? > > > > > > Probably change this to accept a float as well I would say > > > > Maybe clipping the display rect in the desired way while accepting is the > right > > thing to do here. Seems like the sort of thing that should be up to the > > platform, Cast knows that it will always go into the overlay, it should be > free > > to choose a clipping behaviour and proceed. > > That's what I had and it was working for our case, that sounds good to me. > Maybe I just add some comments there to be clear that you should clip if you > accept? > > I also looked at changing ScheduleOverlayPlane to take floats, it seems to > escalate this into a pretty big change and it's not clear anything would > actually be making use of that. > > Thoughts? Yeah, the int to float rabbit hole is very very deep, and the net result is you just clip it in a different place anyway. Clip on accept sounds good to me. The whole point of the validator was to allow platform specific behaviour, this falls well into that category.
On 2015/04/30 14:25:37, alexst wrote: > On 2015/04/30 14:18:43, halliwell wrote: > > On 2015/04/30 13:43:52, alexst wrote: > > > On 2015/04/29 21:45:00, achaulk wrote: > > > > > > https://codereview.chromium.org/1110813002/diff/40001/cc/output/gl_renderer.cc > > > > File cc/output/gl_renderer.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1110813002/diff/40001/cc/output/gl_renderer.c... > > > > cc/output/gl_renderer.cc:3452: display_rect, > > > > On 2015/04/29 21:42:54, halliwell wrote: > > > > > On 2015/04/29 21:37:03, achaulk wrote: > > > > > > You'll still hit the same problem here, no? On your implementation > > you'll > > > > > accept > > > > > > it and it will get here > > > > > > > > > > Yep, unless we modify the transform when accepting, but that's maybe not > > the > > > > > intended use of the API? > > > > > > > > > > What did you have in mind - should we push the float rect right through > to > > > > > ScheduleOverlayPlane? > > > > > > > > Probably change this to accept a float as well I would say > > > > > > Maybe clipping the display rect in the desired way while accepting is the > > right > > > thing to do here. Seems like the sort of thing that should be up to the > > > platform, Cast knows that it will always go into the overlay, it should be > > free > > > to choose a clipping behaviour and proceed. > > > > That's what I had and it was working for our case, that sounds good to me. > > Maybe I just add some comments there to be clear that you should clip if you > > accept? > > > > I also looked at changing ScheduleOverlayPlane to take floats, it seems to > > escalate this into a pretty big change and it's not clear anything would > > actually be making use of that. > > > > Thoughts? > > Yeah, the int to float rabbit hole is very very deep, and the net result is you > just clip it in a different place anyway. Clip on accept sounds good to me. The > whole point of the validator was to allow platform specific behaviour, this > falls well into that category. Ok, so no major changes needed then. I submitted a new patchset with some comments around the need to clip on accept. I also removed the unit test since it ends up testing itself - it may make sense to bring back later as a test for the Cast validator, when I submit that ... we can check that it accepts and snaps float rectangles. PTAL.
On 2015/04/30 18:24:10, halliwell wrote: > On 2015/04/30 14:25:37, alexst wrote: > > On 2015/04/30 14:18:43, halliwell wrote: > > > On 2015/04/30 13:43:52, alexst wrote: > > > > On 2015/04/29 21:45:00, achaulk wrote: > > > > > > > > > https://codereview.chromium.org/1110813002/diff/40001/cc/output/gl_renderer.cc > > > > > File cc/output/gl_renderer.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1110813002/diff/40001/cc/output/gl_renderer.c... > > > > > cc/output/gl_renderer.cc:3452: display_rect, > > > > > On 2015/04/29 21:42:54, halliwell wrote: > > > > > > On 2015/04/29 21:37:03, achaulk wrote: > > > > > > > You'll still hit the same problem here, no? On your implementation > > > you'll > > > > > > accept > > > > > > > it and it will get here > > > > > > > > > > > > Yep, unless we modify the transform when accepting, but that's maybe > not > > > the > > > > > > intended use of the API? > > > > > > > > > > > > What did you have in mind - should we push the float rect right > through > > to > > > > > > ScheduleOverlayPlane? > > > > > > > > > > Probably change this to accept a float as well I would say > > > > > > > > Maybe clipping the display rect in the desired way while accepting is the > > > right > > > > thing to do here. Seems like the sort of thing that should be up to the > > > > platform, Cast knows that it will always go into the overlay, it should be > > > free > > > > to choose a clipping behaviour and proceed. > > > > > > That's what I had and it was working for our case, that sounds good to me. > > > Maybe I just add some comments there to be clear that you should clip if you > > > accept? > > > > > > I also looked at changing ScheduleOverlayPlane to take floats, it seems to > > > escalate this into a pretty big change and it's not clear anything would > > > actually be making use of that. > > > > > > Thoughts? > > > > Yeah, the int to float rabbit hole is very very deep, and the net result is > you > > just clip it in a different place anyway. Clip on accept sounds good to me. > The > > whole point of the validator was to allow platform specific behaviour, this > > falls well into that category. > > Ok, so no major changes needed then. I submitted a new patchset with some > comments around the need to clip on accept. I also removed the unit test since > it ends up testing itself - it may make sense to bring back later as a test for > the Cast validator, when I submit that ... we can check that it accepts and > snaps float rectangles. > > PTAL. Just one request, since you're changing this anyway ui/ozone/platform/drm/gbm_surface_factory.c Change the CheckOverlaySupport test to if (overlay->plane_z_order > 0 && IsTransformSupported(overlay->transform) && gfx::IsNearestRectWithinDistance(overlay->display_rect, 0.01f)) {
On 2015/04/30 21:14:06, achaulk wrote: > On 2015/04/30 18:24:10, halliwell wrote: > > On 2015/04/30 14:25:37, alexst wrote: > > > On 2015/04/30 14:18:43, halliwell wrote: > > > > On 2015/04/30 13:43:52, alexst wrote: > > > > > On 2015/04/29 21:45:00, achaulk wrote: > > > > > > > > > > > > https://codereview.chromium.org/1110813002/diff/40001/cc/output/gl_renderer.cc > > > > > > File cc/output/gl_renderer.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1110813002/diff/40001/cc/output/gl_renderer.c... > > > > > > cc/output/gl_renderer.cc:3452: display_rect, > > > > > > On 2015/04/29 21:42:54, halliwell wrote: > > > > > > > On 2015/04/29 21:37:03, achaulk wrote: > > > > > > > > You'll still hit the same problem here, no? On your implementation > > > > you'll > > > > > > > accept > > > > > > > > it and it will get here > > > > > > > > > > > > > > Yep, unless we modify the transform when accepting, but that's maybe > > not > > > > the > > > > > > > intended use of the API? > > > > > > > > > > > > > > What did you have in mind - should we push the float rect right > > through > > > to > > > > > > > ScheduleOverlayPlane? > > > > > > > > > > > > Probably change this to accept a float as well I would say > > > > > > > > > > Maybe clipping the display rect in the desired way while accepting is > the > > > > right > > > > > thing to do here. Seems like the sort of thing that should be up to the > > > > > platform, Cast knows that it will always go into the overlay, it should > be > > > > free > > > > > to choose a clipping behaviour and proceed. > > > > > > > > That's what I had and it was working for our case, that sounds good to me. > > > > > Maybe I just add some comments there to be clear that you should clip if > you > > > > accept? > > > > > > > > I also looked at changing ScheduleOverlayPlane to take floats, it seems to > > > > escalate this into a pretty big change and it's not clear anything would > > > > actually be making use of that. > > > > > > > > Thoughts? > > > > > > Yeah, the int to float rabbit hole is very very deep, and the net result is > > you > > > just clip it in a different place anyway. Clip on accept sounds good to me. > > The > > > whole point of the validator was to allow platform specific behaviour, this > > > falls well into that category. > > > > Ok, so no major changes needed then. I submitted a new patchset with some > > comments around the need to clip on accept. I also removed the unit test > since > > it ends up testing itself - it may make sense to bring back later as a test > for > > the Cast validator, when I submit that ... we can check that it accepts and > > snaps float rectangles. > > > > PTAL. > > Just one request, since you're changing this anyway > > ui/ozone/platform/drm/gbm_surface_factory.c > Change the CheckOverlaySupport test to > if (overlay->plane_z_order > 0 && > IsTransformSupported(overlay->transform) && > gfx::IsNearestRectWithinDistance(overlay->display_rect, 0.01f)) { Done in patch set 5.
lgtm
On 2015/05/01 14:04:20, alexst wrote: > lgtm @danakj ping on cc/ changes
LGTM https://codereview.chromium.org/1110813002/diff/80001/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/1110813002/diff/80001/cc/output/gl_renderer.c... cc/output/gl_renderer.cc:3437: for (it = overlays.begin(); it != overlays.end(); ++it) { while you're here.. you could change this to for (const OC& overlay : overlays).. if so inclined. https://codereview.chromium.org/1110813002/diff/80001/cc/output/gl_renderer.c... cc/output/gl_renderer.cc:3447: gfx::Rect display_rect(ToNearestRect(overlay.display_rect)); i don't think the temp var is buying much here it's not adding any information, just call ToNearestRect below instead? https://codereview.chromium.org/1110813002/diff/80001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gbm_surface_factory.cc (right): https://codereview.chromium.org/1110813002/diff/80001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gbm_surface_factory.cc:53: gfx::IsNearestRectWithinDistance(overlay->display_rect, 0.01f)) { This magic number, can you leave a comment explaining it?
https://codereview.chromium.org/1110813002/diff/80001/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/1110813002/diff/80001/cc/output/gl_renderer.c... cc/output/gl_renderer.cc:3437: for (it = overlays.begin(); it != overlays.end(); ++it) { On 2015/05/04 19:56:41, danakj wrote: > while you're here.. you could change this to for (const OC& overlay : > overlays).. if so inclined. Done. https://codereview.chromium.org/1110813002/diff/80001/cc/output/gl_renderer.c... cc/output/gl_renderer.cc:3447: gfx::Rect display_rect(ToNearestRect(overlay.display_rect)); On 2015/05/04 19:56:41, danakj wrote: > i don't think the temp var is buying much here it's not adding any information, > just call ToNearestRect below instead? yep, done. https://codereview.chromium.org/1110813002/diff/80001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gbm_surface_factory.cc (right): https://codereview.chromium.org/1110813002/diff/80001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gbm_surface_factory.cc:53: gfx::IsNearestRectWithinDistance(overlay->display_rect, 0.01f)) { On 2015/05/04 19:56:41, danakj wrote: > This magic number, can you leave a comment explaining it? Done.
The CQ bit was checked by halliwell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, alexst@chromium.org Link to the patchset: https://codereview.chromium.org/1110813002/#ps100001 (title: "Some nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1110813002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/56e6db77baec30e48ef94eae9f81e2793db637ea Cr-Commit-Position: refs/heads/master@{#328218} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
