Pass the destination pointer to next

Pass the destination pointer to next() in SkSwizzler

Per our discussion, we can make the swizzler simpler and more usable
for SkCodec and SkScanlineDecoder by only having a single version of
next() which takes a pointer to the srcRow and a pointer to the
dstRow.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/614aa072cbb055a29f37fa25a657313bccc5d666

[msarett, 2015-07-27 20:07:11]

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

[scroggo, 2015-07-27 20:42:07]

A few comments, but overall I think this is a lot cleaner.

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

https://codereview.chromium.org/1256373002/diff/1/src/codec/SkCodec_libbmp.cp...
src/codec/SkCodec_libbmp.cpp:798: fBitsPerPixel));
nit: Can this fit on the previous line now?

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

https://codereview.chromium.org/1256373002/diff/1/src/codec/SkCodec_libpng.cp...
src/codec/SkCodec_libpng.cpp:550: const size_t srcRowBytes = width * bpp;
Yikes! I apparently shadowed rowBytes, which is also a parameter name!

The parameter should probably be renamed dstRowBytes, to more easily
distinguish.

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

https://codereview.chromium.org/1256373002/diff/1/src/codec/SkMaskSwizzler.h#...
src/codec/SkMaskSwizzler.h:36: SkSwizzler::ResultAlpha next(const uint8_t*
SK_RESTRICT src, void* dst);
As I explain in more detail in SkSwizzler.cpp, I think these parameters should
be swapped.

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

https://codereview.chromium.org/1256373002/diff/1/src/codec/SkSwizzler.cpp#ne...
src/codec/SkSwizzler.cpp:421: const ResultAlpha result = fRowProc(dst, src,
fDstInfo.width(),
nit: This could just be return fRowProc(...)

It might be a little weird that src and dst get swapped.

I think we try to make the thing you're modifying be passed as the first
parameter, similar to the signature of memcpy:

void* memcpy(void* dst, const void* src, size_t bytes)

If you use a signature like this:

ResultAlpha next(void* dst, const uint8_t* SK_RESTRICT src)

the order will be consistent when passed to fRowProc.

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

https://codereview.chromium.org/1256373002/diff/1/src/codec/SkSwizzler.h#newc...
src/codec/SkSwizzler.h:121: *  @param dstRowBytes rowBytes for dst.
Please remove the parameter descriptions for parameters that are no longer
there.

https://codereview.chromium.org/1256373002/diff/1/src/codec/SkSwizzler.h#newc...
src/codec/SkSwizzler.h:173: *  @param dst The intended destination row for the
output.
Maybe this should be "where to write the output", or something like that?

https://codereview.chromium.org/1256373002/diff/1/src/codec/SkSwizzler.h#newc...
src/codec/SkSwizzler.h:177: ResultAlpha next(const uint8_t* SK_RESTRICT src,
void* dst);
I believe this function was named "next" because it swizzled the next line. Now
it swizzles whatever line you tell it to, into wherever you tell it to. Maybe it
should be renamed to "swizzle" (same for SkMaskSwizzler)

https://codereview.chromium.org/1256373002/diff/1/src/codec/SkSwizzler.h#newc...
src/codec/SkSwizzler.h:188: *  @param y Line of source.
Please remove this line.

(y was never actually used in any way, which should reassure us that this is a
good change.)

[msarett, 2015-07-27 21:11:34]

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

https://codereview.chromium.org/1256373002/diff/1/src/codec/SkCodec_libbmp.cp...
src/codec/SkCodec_libbmp.cpp:798: fBitsPerPixel));
On 2015/07/27 20:42:06, scroggo wrote:
> nit: Can this fit on the previous line now?

Yes!

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

https://codereview.chromium.org/1256373002/diff/1/src/codec/SkCodec_libpng.cp...
src/codec/SkCodec_libpng.cpp:550: const size_t srcRowBytes = width * bpp;
On 2015/07/27 20:42:06, scroggo wrote:
> Yikes! I apparently shadowed rowBytes, which is also a parameter name!
> 
> The parameter should probably be renamed dstRowBytes, to more easily
> distinguish.

Agreed!  I'll admit that this tricked me for a little while :).

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

https://codereview.chromium.org/1256373002/diff/1/src/codec/SkMaskSwizzler.h#...
src/codec/SkMaskSwizzler.h:36: SkSwizzler::ResultAlpha next(const uint8_t*
SK_RESTRICT src, void* dst);
On 2015/07/27 20:42:06, scroggo wrote:
> As I explain in more detail in SkSwizzler.cpp, I think these parameters should
> be swapped.

Done.

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

https://codereview.chromium.org/1256373002/diff/1/src/codec/SkSwizzler.cpp#ne...
src/codec/SkSwizzler.cpp:421: const ResultAlpha result = fRowProc(dst, src,
fDstInfo.width(),
On 2015/07/27 20:42:06, scroggo wrote:
> nit: This could just be return fRowProc(...)
> 
> It might be a little weird that src and dst get swapped.
> 
> I think we try to make the thing you're modifying be passed as the first
> parameter, similar to the signature of memcpy:
> 
> void* memcpy(void* dst, const void* src, size_t bytes)
> 
> If you use a signature like this:
> 
> ResultAlpha next(void* dst, const uint8_t* SK_RESTRICT src)
> 
> the order will be consistent when passed to fRowProc.

Agreed again!

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

https://codereview.chromium.org/1256373002/diff/1/src/codec/SkSwizzler.h#newc...
src/codec/SkSwizzler.h:121: *  @param dstRowBytes rowBytes for dst.
On 2015/07/27 20:42:06, scroggo wrote:
> Please remove the parameter descriptions for parameters that are no longer
> there.

Done.

https://codereview.chromium.org/1256373002/diff/1/src/codec/SkSwizzler.h#newc...
src/codec/SkSwizzler.h:173: *  @param dst The intended destination row for the
output.
On 2015/07/27 20:42:06, scroggo wrote:
> Maybe this should be "where to write the output", or something like that?

Yeah that is clearer.

https://codereview.chromium.org/1256373002/diff/1/src/codec/SkSwizzler.h#newc...
src/codec/SkSwizzler.h:177: ResultAlpha next(const uint8_t* SK_RESTRICT src,
void* dst);
On 2015/07/27 20:42:06, scroggo wrote:
> I believe this function was named "next" because it swizzled the next line.
Now
> it swizzles whatever line you tell it to, into wherever you tell it to. Maybe
it
> should be renamed to "swizzle" (same for SkMaskSwizzler)

Agreed!

https://codereview.chromium.org/1256373002/diff/1/src/codec/SkSwizzler.h#newc...
src/codec/SkSwizzler.h:188: *  @param y Line of source.
On 2015/07/27 20:42:06, scroggo wrote:
> Please remove this line.
> 
> (y was never actually used in any way, which should reassure us that this is a
> good change.)

Done.

[scroggo, 2015-07-27 21:31:56]

lgtm

[msarett, 2015-07-27 21:53:30]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2015-07-27 21:53:46]

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

[commit-bot: I haz the power, 2015-07-27 21:57:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-27 21:57:27]

Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)

[msarett, 2015-07-27 22:05:26]

The CQ bit was checked by msarett@google.com

[msarett, 2015-07-27 22:05:27]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com
Link to the patchset: https://codereview.chromium.org/1256373002/#ps40001
(title: "Thanks windows bot!")

[commit-bot: I haz the power, 2015-07-27 22:05:42]

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

[commit-bot: I haz the power, 2015-07-27 22:13:21]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/614aa072cbb055a29f37fa25a657313bccc5d666