Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
340613005:
Add scale param to SkAlphaThresholdFilter. (Closed)
|
Created:
6 years, 6 months ago by garykac Modified:
6 years, 4 months ago CC:
reviews_skia.org, danakj Base URL:
https://chromium.googlesource.com/skia.git@master Visibility:
Public. |
DescriptionAdd scale param to SkAlphaThresholdFilter.
This scale is applied to the region before the filter is applied. It is
used to adjust the region by the device's DPI.
Patch Set 1 #Patch Set 2 : Add separate x and y scales #Patch Set 3 : Use SkMatrix for scale #
Total comments: 2
Patch Set 4 : Change |scale| to |transform| #
Total comments: 1
Messages
Total messages: 15 (0 generated)
(For context, see crrev.com/315083004) In this cl, we're adding the scale to the SkAlphaThresholdFilter. Since this filter needs to convert the SkRegions's SkIRects into SkRects before the drawRect() call, applying a scale at that point won't cause any rounding issues.
Are we sure we only need 1 scale? What if this is used on a layer with a Transform that scales 2x in the x-axis but 4x in the y-axis, for example? Are we going to need 2 different scales for that?
On 2014/06/18 15:38:44, danakj wrote: > Are we sure we only need 1 scale? What if this is used on a layer with a > Transform that scales 2x in the x-axis but 4x in the y-axis, for example? Are we > going to need 2 different scales for that? Since this is to handle pixel density, only 1 scale is needed. If a layer has odd scaling requirements, then it seems more appropriate to apply the scaling in the layer. Also, we're the only user of the SkAlphaThresholdFilter, but if another use-case comes along and requires different scales it can trivially be added at that time.
On 2014/06/18 16:35:23, garykac wrote: > On 2014/06/18 15:38:44, danakj wrote: > > Are we sure we only need 1 scale? What if this is used on a layer with a > > Transform that scales 2x in the x-axis but 4x in the y-axis, for example? Are > we > > going to need 2 different scales for that? > > Since this is to handle pixel density, only 1 scale is needed. > > If a layer has odd scaling requirements, then it seems more appropriate to apply > the scaling in the layer. > > Also, we're the only user of the SkAlphaThresholdFilter, but if another use-case > comes along and requires different scales it can trivially be added at that > time. Dana and I discussed this offline and she's right - we need two scales to do this properly.
On 2014/07/09 22:20:31, garykac wrote:
> On 2014/06/18 16:35:23, garykac wrote:
> > On 2014/06/18 15:38:44, danakj wrote:
> > > Are we sure we only need 1 scale? What if this is used on a layer with a
> > > Transform that scales 2x in the x-axis but 4x in the y-axis, for example?
> Are
> > we
> > > going to need 2 different scales for that?
> >
> > Since this is to handle pixel density, only 1 scale is needed.
> >
> > If a layer has odd scaling requirements, then it seems more appropriate to
> apply
> > the scaling in the layer.
> >
> > Also, we're the only user of the SkAlphaThresholdFilter, but if another
> use-case
> > comes along and requires different scales it can trivially be added at that
> > time.
>
> Dana and I discussed this offline and she's right - we need two scales to do
> this properly.
Maybe just generalize it to take a matrix? I don't see a good reason not to.
Also you can just concat the translate or matrix onto the GrContext's matrix
rather than transforming each rect:
SkMatrixs scale;
scale.setScale(fX, fY);
context->concatMatrix(scale);
while (!iter.done()) {
On 2014/07/10 13:36:52, bsalomon wrote:
> On 2014/07/09 22:20:31, garykac wrote:
> > On 2014/06/18 16:35:23, garykac wrote:
> > > On 2014/06/18 15:38:44, danakj wrote:
> > > > Are we sure we only need 1 scale? What if this is used on a layer with a
> > > > Transform that scales 2x in the x-axis but 4x in the y-axis, for
example?
> > Are
> > > we
> > > > going to need 2 different scales for that?
> > >
> > > Since this is to handle pixel density, only 1 scale is needed.
> > >
> > > If a layer has odd scaling requirements, then it seems more appropriate to
> > apply
> > > the scaling in the layer.
> > >
> > > Also, we're the only user of the SkAlphaThresholdFilter, but if another
> > use-case
> > > comes along and requires different scales it can trivially be added at
that
> > > time.
> >
> > Dana and I discussed this offline and she's right - we need two scales to do
> > this properly.
>
> Maybe just generalize it to take a matrix? I don't see a good reason not to.
> Also you can just concat the translate or matrix onto the GrContext's matrix
> rather than transforming each rect:
>
> SkMatrixs scale;
> scale.setScale(fX, fY);
> context->concatMatrix(scale);
>
> while (!iter.done()) {
Done.
Can we call the matrix params/members something other than "scale"? They could just as easily contain a translation, rotation, etc now. Since the matrix is applied to the region how about we call it regionMatrix or regionM? Otherwise, 1gtm. https://codereview.chromium.org/340613005/diff/40001/src/effects/SkAlphaThres... File src/effects/SkAlphaThresholdFilter.cpp (right): https://codereview.chromium.org/340613005/diff/40001/src/effects/SkAlphaThres... src/effects/SkAlphaThresholdFilter.cpp:44: scale.setScale(1.f, 1.f); SkMatrix has a reset() function that will set it to identity. I think that's clearer than calling scale(1,1).
>Can we call the matrix params/members something other >than "scale"? They could just as easily contain a >translation, rotation, etc now. Since the matrix is >applied to the region how about we call it regionMatrix >or regionM? Done. I renamed it |transform| since it seemed more appropriate. >Otherwise, 1gtm. Excellent. Now I just need a 2gtm. ^_^ https://codereview.chromium.org/340613005/diff/40001/src/effects/SkAlphaThres... File src/effects/SkAlphaThresholdFilter.cpp (right): https://codereview.chromium.org/340613005/diff/40001/src/effects/SkAlphaThres... src/effects/SkAlphaThresholdFilter.cpp:44: scale.setScale(1.f, 1.f); On 2014/07/11 13:00:25, bsalomon wrote: > SkMatrix has a reset() function that will set it to identity. I think that's > clearer than calling scale(1,1). Done.
Thanks for making the changes! lgtm
The CQ bit was checked by garykac@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Commit queue rejected this change because it did not recognize the base URL. Please commit your change manually.
https://codereview.chromium.org/340613005/diff/60001/src/effects/SkAlphaThres... File src/effects/SkAlphaThresholdFilter.cpp (right): https://codereview.chromium.org/340613005/diff/60001/src/effects/SkAlphaThres... src/effects/SkAlphaThresholdFilter.cpp:49: const SkMatrix& transform, Is there a reason this needs to be a separate matrix, and can't be part of the CTM? Most filters already transform their parameters by the CTM at draw time.
On 2014/07/11 20:15:24, Stephen White wrote: > https://codereview.chromium.org/340613005/diff/60001/src/effects/SkAlphaThres... > File src/effects/SkAlphaThresholdFilter.cpp (right): > > https://codereview.chromium.org/340613005/diff/60001/src/effects/SkAlphaThres... > src/effects/SkAlphaThresholdFilter.cpp:49: const SkMatrix& transform, > Is there a reason this needs to be a separate matrix, and can't be part of the > CTM? Most filters already transform their parameters by the CTM at draw time. I think the idea here is to transform the region used by the filter rather than to account for changes to the CTM in the filter. I could be misunderstanding, though.
On 2014/07/11 20:24:26, bsalomon wrote: > On 2014/07/11 20:15:24, Stephen White wrote: > > > https://codereview.chromium.org/340613005/diff/60001/src/effects/SkAlphaThres... > > File src/effects/SkAlphaThresholdFilter.cpp (right): > > > > > https://codereview.chromium.org/340613005/diff/60001/src/effects/SkAlphaThres... > > src/effects/SkAlphaThresholdFilter.cpp:49: const SkMatrix& transform, > > Is there a reason this needs to be a separate matrix, and can't be part of the > > CTM? Most filters already transform their parameters by the CTM at draw time. > > I think the idea here is to transform the region used by the filter rather than > to account for changes to the CTM in the filter. I could be misunderstanding, > though. I'm currently testing https://codereview.chromium.org/191123002/ to see if it fixes our HiDPI scaling problems. If it is does, then this cl will not be needed. |
