Clean up unused or overly complex modes in SkSwizzler

Clean up unused or overly complex modes in SkSwizzler

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1582083005

Committed: https://skia.googlesource.com/skia/+/bda86098ab2784968d0a1222dd1f4c18d18fe050

[msarett, 2016-01-14 20:46:01]

Description was changed from

==========
Clean up unused or overly complex modes in SkSwizzler

BUG=skia:
==========

to

==========
Clean up unused or overly complex modes in SkSwizzler

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[msarett, 2016-01-14 20:52:24]

msarett@google.com changed reviewers:
+ scroggo@google.com

[msarett, 2016-01-14 20:52:25]

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.h
File src/codec/SkSwizzler.h (right):

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.h#newc...
src/codec/SkSwizzler.h:31: kBGRX,
kBGRX now really means BGR(anything) instead of BGR(FF).

[scroggo, 2016-01-14 21:28:18]

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.cpp#ne...
src/codec/SkSwizzler.cpp:53: // TODO (msarett): Investigate SIMD optimizations
for swizzle routines.
Not related to this patch set, but time to remove this line?

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.cpp#ne...
src/codec/SkSwizzler.cpp:328: static void fast_swizzle_bgra_to_n32_unpremul(
I thought we were going to use the same methods for n32 and non-native 32.
Should this name take that into account?

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.cpp#ne...
src/codec/SkSwizzler.cpp:443: static void fast_swizzle_rgba_to_n32_unpremul(
This name is confusing. It looks like it actually handles RGB(0xFF) to N32?

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.cpp#ne...
src/codec/SkSwizzler.cpp:733: fastProc = ©
This looks like the right thing to do, except I thought you said you were
planning to add opts for sampling? I was thinking those would be fastProcs as
well. Which means that sometimes we'll be able to use fastProcs for sampling,
but othertimes not (I'm guessing there won't be fastProcs for these samples).

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.cpp#ne...
src/codec/SkSwizzler.cpp:799: fFastProc = nullptr;
This predates this CL, but I could imagine the following:

- setSampleX ( > 1)
  - we null out fFastProc
- decode
- setSampleX (1)
  - ideally we should go back to using fFastProc, but it's gone

Maybe I'm imagining things, since we'll recreate the swizzler anyway? But
perhaps we shouldn't?

Also, I realized that if someone called setSampleX(1) ever we'll unnecessarily
null out the fFastProc. Should we special case 1? I'm guessing no one
(currently) calls it that way, but we could make them (if we ever decide to
reuse the swizzler).

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.cpp#ne...
src/codec/SkSwizzler.cpp:806: RowProc proc = fFastProc ? fFastProc : fProc;
We could fix the (future?) problem I mentioned above and remove this if
statement by adding a third pointer:

class SkSwizzler {
  RowProc fFastProc, fSlowProc, fActualProc;
};

::onSetSampleX(x) {
  if (1 == x) {
    fActualProc = fFastProc;
    ...
  } else {
    fActualProc = fSlowProc;
  }
}

None of this necessarily relates to this CL, but would that be worth doing? Or
am I prematurely optimizing?

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.h
File src/codec/SkSwizzler.h (right):

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.h#newc...
src/codec/SkSwizzler.h:31: kBGRX,
On 2016/01/14 20:52:24, msarett wrote:
> kBGRX now really means BGR(anything) instead of BGR(FF).

Can you add a comment stating that?

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.h#newc...
src/codec/SkSwizzler.h:35: // These modes are used exclusively for sampling and
subsetting.
Looks like the number means the number of bytes. That may be obvious enough (we
already assume the other ones above mean number of bits).

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.h#newc...
src/codec/SkSwizzler.h:37: kNOOP1,
Why not kNoOp1 etc?

[msarett, 2016-01-14 22:25:20]

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.cpp#ne...
src/codec/SkSwizzler.cpp:53: // TODO (msarett): Investigate SIMD optimizations
for swizzle routines.
On 2016/01/14 21:28:18, scroggo wrote:
> Not related to this patch set, but time to remove this line?

Done.

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.cpp#ne...
src/codec/SkSwizzler.cpp:328: static void fast_swizzle_bgra_to_n32_unpremul(
On 2016/01/14 21:28:18, scroggo wrote:
> I thought we were going to use the same methods for n32 and non-native 32.
> Should this name take that into account?

We can and maybe should consider sharing more code between n32 and non-native
32, but we don't actually do that.  Where we used to share code was byte
swapping on Unpremul and Opaque.  I've discussed why we no longer do this below.

We should treat Unpremul BGRA and BGR(FF) identically:

However, only JPEG uses BGR(FF).  And we don't really need to know BGR(FF) in
the JPEG case, since libjpeg-turbo will have already put the bytes in right
order for us.

BMP uses BGRX, but in this case the X really means anything, so it needs to be
handled separately (in order to fill the alpha channel with FF).

So this function is no longer shared because BGR(FF) is gone.

--------------------------------------------------------------------

We should treat Unpremul RGBA and RGB(FF) identically:

JPEG is one user of RGB(FF), but as we said above, it is always a noop.

PNG *was* another user of RGB(FF), but treating it as RGB will allow us to be
more efficient.

There are now no users of RGB(FF) or RGB(X), so it is gone.

RGB remains and is used by PNG (as of this CL) and will be used by RAW.

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.cpp#ne...
src/codec/SkSwizzler.cpp:443: static void fast_swizzle_rgba_to_n32_unpremul(
On 2016/01/14 21:28:18, scroggo wrote:
> This name is confusing. It looks like it actually handles RGB(0xFF) to N32?

Let me know if the name is still confusing after our conversation.

This could handle RGB(FF), but none of the Codecs produce RGB(FF) anymore.

This also handles RGBA (unpremul) because the logic is exactly the same.

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.cpp#ne...
src/codec/SkSwizzler.cpp:733: fastProc = ©
On 2016/01/14 21:28:18, scroggo wrote:
> This looks like the right thing to do, except I thought you said you were
> planning to add opts for sampling? I was thinking those would be fastProcs as
> well. Which means that sometimes we'll be able to use fastProcs for sampling,
> but othertimes not (I'm guessing there won't be fastProcs for these samples).

We will never be able to know that we are sampling (or what the sampleSize is)
in this function, so we will not be able to set the fastProc to a sampling proc
here.

I envision that we will be able to adjust fastProc to a new fastProc (that
samples) in setSampleSize.

Regardless, I don't think this breaks our current pattern except that the names
are confusing.  Maybe it should be:

fast_copy(), copy_8(), copy_16(), copy_32().  All the copy_X() functions do the
same thing as fast_copy(), they just do it one pixel at a time which allows them
to support sampling.

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.cpp#ne...
src/codec/SkSwizzler.cpp:799: fFastProc = nullptr;
On 2016/01/14 21:28:18, scroggo wrote:
> This predates this CL, but I could imagine the following:
> 
> - setSampleX ( > 1)
>   - we null out fFastProc
> - decode
> - setSampleX (1)
>   - ideally we should go back to using fFastProc, but it's gone
> 
> Maybe I'm imagining things, since we'll recreate the swizzler anyway? But
> perhaps we shouldn't?
> 
> Also, I realized that if someone called setSampleX(1) ever we'll unnecessarily
> null out the fFastProc. Should we special case 1? I'm guessing no one
> (currently) calls it that way, but we could make them (if we ever decide to
> reuse the swizzler).

Right on all counts.  Can I fix a follow-up CL?

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.cpp#ne...
src/codec/SkSwizzler.cpp:806: RowProc proc = fFastProc ? fFastProc : fProc;
On 2016/01/14 21:28:18, scroggo wrote:
> We could fix the (future?) problem I mentioned above and remove this if
> statement by adding a third pointer:
> 
> class SkSwizzler {
>   RowProc fFastProc, fSlowProc, fActualProc;
> };
> 
> ::onSetSampleX(x) {
>   if (1 == x) {
>     fActualProc = fFastProc;
>     ...
>   } else {
>     fActualProc = fSlowProc;
>   }
> }
> 
> None of this necessarily relates to this CL, but would that be worth doing? Or
> am I prematurely optimizing?

+1 on this idea.  That will be a nice way to handle things as we add fastProcs
for sampling as well.  Can I do this in a follow-up?

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.h
File src/codec/SkSwizzler.h (right):

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.h#newc...
src/codec/SkSwizzler.h:31: kBGRX,
On 2016/01/14 21:28:18, scroggo wrote:
> On 2016/01/14 20:52:24, msarett wrote:
> > kBGRX now really means BGR(anything) instead of BGR(FF).
> 
> Can you add a comment stating that?

Done.

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.h#newc...
src/codec/SkSwizzler.h:35: // These modes are used exclusively for sampling and
subsetting.
On 2016/01/14 21:28:18, scroggo wrote:
> Looks like the number means the number of bytes. That may be obvious enough
(we
> already assume the other ones above mean number of bits).

Happy to use the number of bits for consistency.

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.h#newc...
src/codec/SkSwizzler.h:37: kNOOP1,
On 2016/01/14 21:28:18, scroggo wrote:
> Why not kNoOp1 etc?

sgtm

[scroggo, 2016-01-19 18:06:35]

lgtm

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.cpp#ne...
src/codec/SkSwizzler.cpp:443: static void fast_swizzle_rgba_to_n32_unpremul(
On 2016/01/14 22:25:20, msarett wrote:
> On 2016/01/14 21:28:18, scroggo wrote:
> > This name is confusing. It looks like it actually handles RGB(0xFF) to N32?
> 
> Let me know if the name is still confusing after our conversation.
> 
> This could handle RGB(FF), but none of the Codecs produce RGB(FF) anymore.
> 
> This also handles RGBA (unpremul) because the logic is exactly the same.

No, the confusion was mine :)

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.cpp#ne...
src/codec/SkSwizzler.cpp:799: fFastProc = nullptr;
On 2016/01/14 22:25:20, msarett wrote:
> On 2016/01/14 21:28:18, scroggo wrote:
> > This predates this CL, but I could imagine the following:
> > 
> > - setSampleX ( > 1)
> >   - we null out fFastProc
> > - decode
> > - setSampleX (1)
> >   - ideally we should go back to using fFastProc, but it's gone
> > 
> > Maybe I'm imagining things, since we'll recreate the swizzler anyway? But
> > perhaps we shouldn't?
> > 
> > Also, I realized that if someone called setSampleX(1) ever we'll
unnecessarily
> > null out the fFastProc. Should we special case 1? I'm guessing no one
> > (currently) calls it that way, but we could make them (if we ever decide to
> > reuse the swizzler).
> 
> Right on all counts.  Can I fix a follow-up CL?

sgtm

https://codereview.chromium.org/1582083005/diff/1/src/codec/SkSwizzler.cpp#ne...
src/codec/SkSwizzler.cpp:806: RowProc proc = fFastProc ? fFastProc : fProc;
On 2016/01/14 22:25:20, msarett wrote:
> On 2016/01/14 21:28:18, scroggo wrote:
> > We could fix the (future?) problem I mentioned above and remove this if
> > statement by adding a third pointer:
> > 
> > class SkSwizzler {
> >   RowProc fFastProc, fSlowProc, fActualProc;
> > };
> > 
> > ::onSetSampleX(x) {
> >   if (1 == x) {
> >     fActualProc = fFastProc;
> >     ...
> >   } else {
> >     fActualProc = fSlowProc;
> >   }
> > }
> > 
> > None of this necessarily relates to this CL, but would that be worth doing?
Or
> > am I prematurely optimizing?
> 
> +1 on this idea.  That will be a nice way to handle things as we add fastProcs
> for sampling as well.  Can I do this in a follow-up?

sgtm

[msarett, 2016-01-19 18:29:16]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2016-01-19 18:29:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1582083005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1582083005/20001

[commit-bot: I haz the power, 2016-01-19 18:40:15]

Description was changed from

==========
Clean up unused or overly complex modes in SkSwizzler

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Clean up unused or overly complex modes in SkSwizzler

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/bda86098ab2784968d0a1222dd1f4c18d18fe050
==========

[commit-bot: I haz the power, 2016-01-19 18:40:15]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/bda86098ab2784968d0a1222dd1f4c18d18fe050