Skip memcpy() swizzles in SkPngCodec

Skip memcpy() swizzles in SkPngCodec

Index8->Index8 PNG Decode Time (Nexus 6P)
0.97x

Gray8->Gray8 PNG Decode Time (Nexus 6P)
0.97x

RGBA->RGBA-Unpremul PNG Decode Time (Nexus 6P)
0.98x

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1671003004
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot

[msarett, 2016-02-05 16:40:43]

Description was changed from

==========
Avoid memcpy() swizzles

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

to

==========
Avoid memcpy() swizzles

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

[msarett, 2016-02-05 19:52:35]

Description was changed from

==========
Avoid memcpy() swizzles

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

to

==========
Avoid memcpy() swizzles

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

[msarett, 2016-02-05 20:13:16]

Description was changed from

==========
Avoid memcpy() swizzles

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

to

==========
Skip memcpy() swizzles in SkPngCodec

Index8->Index8 PNG Decode Time (Nexus 6P)
0.97x

Gray8->Gray8 PNG Decode Time (Nexus 6P)
0.97x

RGBA->RGBA-Unpremul PNG Decode Time (Nexus 6P)
0.98x

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

[msarett, 2016-02-05 20:46:06]

Patchset #2 (id:20001) has been deleted

[msarett, 2016-02-05 20:48:17]

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

[msarett, 2016-02-05 20:48:18]

I'm leaning against landing something like this.  Though I might try to refactor
Patch Set 2.

Right now it looks like the code gets more complicated and the speed-up is small
(and a rare use case).

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

https://codereview.chromium.org/1671003004/diff/1/src/codec/SkPngCodec.cpp#ne...
src/codec/SkPngCodec.cpp:532: fSwizzler->swizzle(dstRow, srcRow);
Problem:

I think this will still perform a function call, even if the function does
nothing.

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

https://codereview.chromium.org/1671003004/diff/1/src/codec/SkPngCodec.h#newc...
src/codec/SkPngCodec.h:41: fInPlaceSwizzle = false;
Problem:

Here we assume that the client is calling getSampler() in order to call
setSampleSize() next (in which case we can't swizzle in place).

This is weird and unnecessarily pessimistic.

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

https://codereview.chromium.org/1671003004/diff/1/src/codec/SkSwizzler.cpp#ne...
src/codec/SkSwizzler.cpp:14: static void noop(void* dst, const uint8_t* src, int
width, int bpp, int deltaSrc, int offset,
This is broken as is.
It works for PNG, but I would need to modify other users of SkSwizzler as well.

https://codereview.chromium.org/1671003004/diff/1/src/codec/SkSwizzler.cpp#ne...
src/codec/SkSwizzler.cpp:884: fastProc = nullptr;
Problem:

We need the swizzler to do a copy for us on subset decodes.  We can get this by
turning off fastProc.

The result is that subset decodes will be slower.

[scroggo, 2016-02-05 20:51:59]

On 2016/02/05 20:48:18, msarett wrote:
> I'm leaning against landing something like this.  Though I might try to
refactor
> Patch Set 2.
> 
> Right now it looks like the code gets more complicated and the speed-up is
small
> (and a rare use case).
> 
> https://codereview.chromium.org/1671003004/diff/1/src/codec/SkPngCodec.cpp
> File src/codec/SkPngCodec.cpp (right):
> 
>
https://codereview.chromium.org/1671003004/diff/1/src/codec/SkPngCodec.cpp#ne...
> src/codec/SkPngCodec.cpp:532: fSwizzler->swizzle(dstRow, srcRow);
> Problem:
> 
> I think this will still perform a function call, even if the function does
> nothing.
> 
> https://codereview.chromium.org/1671003004/diff/1/src/codec/SkPngCodec.h
> File src/codec/SkPngCodec.h (right):
> 
>
https://codereview.chromium.org/1671003004/diff/1/src/codec/SkPngCodec.h#newc...
> src/codec/SkPngCodec.h:41: fInPlaceSwizzle = false;
> Problem:
> 
> Here we assume that the client is calling getSampler() in order to call
> setSampleSize() next (in which case we can't swizzle in place).
> 
> This is weird and unnecessarily pessimistic.
> 
> https://codereview.chromium.org/1671003004/diff/1/src/codec/SkSwizzler.cpp
> File src/codec/SkSwizzler.cpp (right):
> 
>
https://codereview.chromium.org/1671003004/diff/1/src/codec/SkSwizzler.cpp#ne...
> src/codec/SkSwizzler.cpp:14: static void noop(void* dst, const uint8_t* src,
int
> width, int bpp, int deltaSrc, int offset,
> This is broken as is.
> It works for PNG, but I would need to modify other users of SkSwizzler as
well.
> 
>
https://codereview.chromium.org/1671003004/diff/1/src/codec/SkSwizzler.cpp#ne...
> src/codec/SkSwizzler.cpp:884: fastProc = nullptr;
> Problem:
> 
> We need the swizzler to do a copy for us on subset decodes.  We can get this
by
> turning off fastProc.
> 
> The result is that subset decodes will be slower.

I'm not sure what to make of this. Do you want me to review patch set 2?

[msarett, 2016-02-05 20:53:51]

Sorry I meant to mention this.  Yes, please look at Patch Set 2.

No reason to look at Patch Set 1 unless you are curious about what I think is a
worse way to approach this problem :).

[scroggo, 2016-02-05 21:36:52]

lgtm

https://codereview.chromium.org/1671003004/diff/40001/src/codec/SkPngCodec.cpp
File src/codec/SkPngCodec.cpp (right):

https://codereview.chromium.org/1671003004/diff/40001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:563: png_read_rows(fPng_ptr, &rowPtr, nullptr, 1);
if (!swizzler), would it make sense to call this with an array of row pointers
and height for the last parameter, instead of looping?

https://codereview.chromium.org/1671003004/diff/40001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:631: png_read_rows(this->png_ptr(), &rowPtr, nullptr,
1);
Again, instead of looping when there's no swizzler, would it make sense to call
this once with an array of row pointers?

I guess this means a separate code path and we still need to set up the
pointers.

[msarett, 2016-02-05 22:14:16]

I'm closing this.

As we discussed in person, I don't think it makes sense to add this amount of
complexity for minimal gains.

https://codereview.chromium.org/1671003004/diff/40001/src/codec/SkPngCodec.cpp
File src/codec/SkPngCodec.cpp (right):

https://codereview.chromium.org/1671003004/diff/40001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:563: png_read_rows(fPng_ptr, &rowPtr, nullptr, 1);
On 2016/02/05 21:36:52, scroggo wrote:
> if (!swizzler), would it make sense to call this with an array of row pointers
> and height for the last parameter, instead of looping?

This looks like it makes things another half percent faster...