Index8 GPU and CPU raster support.

Index8 GPU and CPU raster support.

Code developed to support Chromium GIF (animation and single frame) Index8
decoding: https://codereview.chromium.org/1460523002/

GPU:
Following handling of YUV planes: palette and indices are in two textures and
GrIndex8toRGBEffect is used to render to RGBA target texture, used further in
Ganesh.

CPU:
Only thing needed there was a change in SkImageGenerator::tryGenerateBitmap:
Chromium image generator declares Index8 colorType, but in rare cases (never
with the first frame, only with some of the frames after) decoder can produce
only N32.

Caveats:
At the moment, there is conversion to N32 before rendering in case
where drawBitmapRect has clipping scalling. Plan is to provide measurements
showing that there is improvement in this case, too and optionally improve.

Using RGBA target texture as existing code is doing the same, good to check
if N32 (so, BGRA for cases where N32 == BGRA) is better.

BUG=chromium:476531, chromium:138421

[aleksandar.stojiljkovic, 2015-12-03 13:30:49]

aleksandar.stojiljkovic@intel.com changed reviewers:
+ noel@chromium.org, reed@chromium.org, scroggo@chromium.org,
senorblanco@chromium.org

[aleksandar.stojiljkovic, 2015-12-03 13:30:49]

Index8 GPU and CPU raster support.

Code developed to support Chromium GIF (animation and single frame) Index8
decoding: https://codereview.chromium.org/1460523002/

GPU:
Following handling of YUV planes: palette and indices are in two textures and
GrIndex8toRGBEffect is used to render to RGBA target texture, used further in
Ganesh.

CPU:
Only thing needed there was a change in SkImageGenerator::tryGenerateBitmap:
Chromium image generator declares Index8 colorType, but in rare cases (never
with the first frame, only with some of the frames after) decoder can produce
only N32.

Caveats:
At the moment, there is conversion to N32 before rendering in case
where drawBitmapRect has clipping scalling. Plan is to provide measurements
showing that there is improvement in this case, too and optionally improve.

Using RGBA target texture as existing code is doing the same, good to check
if N32 (so, BGRA for cases where N32 == BGRA) is better.

BUG=skia:

https://codereview.chromium.org/1495693003/diff/1/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/1495693003/diff/1/src/gpu/SkGr.cpp#newcode237
src/gpu/SkGr.cpp:237: if (count != 256 || kN32_SkColorType !=
kRGBA_8888_SkColorType) {
Not happy with this one - might make sense to use N32 always also for target
surface.

https://codereview.chromium.org/1495693003/diff/1/src/gpu/SkGr.cpp#newcode303
src/gpu/SkGr.cpp:303: if (caps->isConfigTexturable(kIndex_8_GrPixelConfig)) {
Based on my previous finding with Mali and Adrenos,
OES_compressed_paletted_texture is slower than uploading RGBA, indicating that
it is not implemented in silicon. After measurements, propose to remove this and
always use else branch.

[reed1, 2015-12-03 13:47:23]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2015-12-03 13:47:23]

There will more further complications down the road (e.g. as we add other ways
to access generators, such as scaling and subsetting), if we support the
generator returning a "false" colortype.

What is the labor involved in having the GIF generator actually know its
colortype correctly? (i.e. if its N32, say that instead of Index8).

[aleksandar.stojiljkovic, 2015-12-03 14:12:48]

On 2015/12/03 13:47:23, reed1 wrote:
> There will more further complications down the road (e.g. as we add other ways
> to access generators, such as scaling and subsetting), if we support the
> generator returning a "false" colortype.
> 
> What is the labor involved in having the GIF generator actually know its
> colortype correctly? (i.e. if its N32, say that instead of Index8).

Agree, this can be much better - just this is what I was able to do without
changing current API in SkImageGenerator.
Best way would be if SkImageGenerator::getInfo()  can be virtual and overriden.

Problem description:
For every frame in gif animation, one DecodingImageGenerator is instantiated
early, they all share the same ImageFrameGenerator.
We cannot know what is colorInfo for frame before decoding previous frames -
peeking and getting info would be good if per instance we agree with providing
overridable onGetInfo that SkImageGenerator would call before allocating.

[aleksandar.stojiljkovic, 2015-12-03 14:15:58]

Description was changed from

==========
Index8 GPU and CPU raster support.

Code developed to support Chromium GIF (animation and single frame) Index8
decoding: https://codereview.chromium.org/1460523002/

GPU:
Following handling of YUV planes: palette and indices are in two textures and
GrIndex8toRGBEffect is used to render to RGBA target texture, used further in
Ganesh.

CPU:
Only thing needed there was a change in SkImageGenerator::tryGenerateBitmap:
Chromium image generator declares Index8 colorType, but in rare cases (never
with the first frame, only with some of the frames after) decoder can produce
only N32.

Caveats:
At the moment, there is conversion to N32 before rendering in case
where drawBitmapRect has clipping scalling. Plan is to provide measurements
showing that there is improvement in this case, too and optionally improve.

Using RGBA target texture as existing code is doing the same, good to check
if N32 (so, BGRA for cases where N32 == BGRA) is better.

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

to

==========
Index8 GPU and CPU raster support.

Code developed to support Chromium GIF (animation and single frame) Index8
decoding: https://codereview.chromium.org/1460523002/

GPU:
Following handling of YUV planes: palette and indices are in two textures and
GrIndex8toRGBEffect is used to render to RGBA target texture, used further in
Ganesh.

CPU:
Only thing needed there was a change in SkImageGenerator::tryGenerateBitmap:
Chromium image generator declares Index8 colorType, but in rare cases (never
with the first frame, only with some of the frames after) decoder can produce
only N32.

Caveats:
At the moment, there is conversion to N32 before rendering in case
where drawBitmapRect has clipping scalling. Plan is to provide measurements
showing that there is improvement in this case, too and optionally improve.

Using RGBA target texture as existing code is doing the same, good to check
if N32 (so, BGRA for cases where N32 == BGRA) is better.

BUG=chromium:476531, chromium:138421
==========

[aleksandar.stojiljkovic, 2015-12-03 14:19:41]

On 2015/12/03 14:12:48, aleksandar.stojiljkovic wrote:
...
> peeking and getting info would be good if per instance we agree with providing
> overridable onGetInfo that SkImageGenerator would call before allocating.
Wrong wording - not peeking, just advance decoding (no additional decoding
processing would be needed as getPixels would just advance to the end of the
frame)

[scroggo, 2015-12-03 15:06:06]

On 2015/12/03 14:12:48, aleksandar.stojiljkovic wrote:
> On 2015/12/03 13:47:23, reed1 wrote:
> > There will more further complications down the road (e.g. as we add other
ways
> > to access generators, such as scaling and subsetting), if we support the
> > generator returning a "false" colortype.
> > 
> > What is the labor involved in having the GIF generator actually know its
> > colortype correctly? (i.e. if its N32, say that instead of Index8).
> 
> Agree, this can be much better - just this is what I was able to do without
> changing current API in SkImageGenerator.
> Best way would be if SkImageGenerator::getInfo()  can be virtual and
overriden.

It used to be virtual, but the existing use cases never changed the info after
creation, so we thought it made sense to make it const from creation time. We
were not considering this though...

> 
> Problem description:
> For every frame in gif animation, one DecodingImageGenerator is instantiated
> early, they all share the same ImageFrameGenerator.
> We cannot know what is colorInfo for frame before decoding previous frames -
> peeking and getting info would be good if per instance we agree with providing
> overridable onGetInfo that SkImageGenerator would call before allocating.

[aleksandar.stojiljkovic, 2015-12-07 19:31:02]

Trying different approach according to reed's comment - avoid allocation and
failed pixel fetch of wrong type.

https://codereview.chromium.org/1495693003/diff/20001/include/core/SkImageGen...
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/1495693003/diff/20001/include/core/SkImageGen...
include/core/SkImageGenerator.h:207: virtual SkColorType onGetColorType() {
return fInfo.colorType(); }
On 2015/12/03 13:47:23, reed1 wrote:
> There will more further complications down the road (e.g. as we add other ways
> to access generators, such as scaling and subsetting), if we support the
> generator returning a "false" colortype.
> 
> What is the labor involved in having the GIF generator actually know its
> colortype correctly? (i.e. if its N32, say that instead of Index8).

Does this look better?
Implemented the chromium part here [1], works OK.

[1]
https://codereview.chromium.org/1460523002/diff/100001/third_party/WebKit/Sou...

https://codereview.chromium.org/1495693003/diff/20001/src/core/SkImageCachera...
File src/core/SkImageCacherator.cpp (right):

https://codereview.chromium.org/1495693003/diff/20001/src/core/SkImageCachera...
src/core/SkImageCacherator.cpp:94: return generator->tryGenerateBitmap(bitmap,
genInfo, allocator);
fInfo is constructed based on initial generator->getInfo(). However, as
colortype changed in generator, and fInfo size == geninfo size, using geninfo
here.

[reed1, 2015-12-07 19:45:36]

How will you implement onGetColorType for GIF? I presume by sniffing the encoded
data? If so, I think we should do that before we instantiate the generator
itself, since once its made, the very first thing skia may do is to get its
info, triggering the calculation of colortype.

note: generators are one-shot objects. each frame of an animation is an entirely
different generator object, so the colortype (and anything else) can't change
once the object is created.

[aleksandar.stojiljkovic, 2015-12-07 20:24:49]

On 2015/12/07 19:45:36, reed1 wrote:
> How will you implement onGetColorType for GIF? I presume by sniffing the
encoded
> data? If so, I think we should do that before we instantiate the generator
> itself, since once its made, the very first thing skia may do is to get its
> info, triggering the calculation of colortype.
> 
> note: generators are one-shot objects. each frame of an animation is an
entirely
> different generator object, so the colortype (and anything else) can't change
> once the object is created.

[aleksandar.stojiljkovic, 2015-12-07 20:31:08]

On 2015/12/07 20:24:49, aleksandar.stojiljkovic wrote:
> On 2015/12/07 19:45:36, reed1 wrote:
> > How will you implement onGetColorType for GIF? I presume by sniffing the
> encoded
> > data?
There is something like that done to get the frame count that is usable - so
this code runs before decoding frames [1].

 If so, I think we should do that before we instantiate the generator
> > itself, since once its made, the very first thing skia may do is to get its
> > info, triggering the calculation of colortype.
> > 
> > note: generators are one-shot objects. each frame of an animation is an
> entirely
> > different generator object, so the colortype (and anything else) can't
change
> > once the object is created.
Problem comes with partial decoding - decoding of content that is not yet fully
downloaded (SkImageGenerators are created before all of the content is
downloaded).
Let me try to do it that way and measure.

[reed1, 2015-12-07 20:45:35]

If a generator is created with partial data, and later the complete data is
available, then a different generator will have to be created. we can't update a
previously created generator with new/more data (since each generator has a
uniqueID that is used for caching, so its contents can't change).

[aleksandar.stojiljkovic, 2015-12-07 20:59:43]

On 2015/12/07 20:45:35, reed1 wrote:
> If a generator is created with partial data, and later the complete data is
> available, then a different generator will have to be created. we can't update
a
> previously created generator with new/more data (since each generator has a
> uniqueID that is used for caching, so its contents can't change).

Shouldn't be a problem. 

About:
>How will you implement onGetColorType for GIF?
Failed to provide the info in previous comment - there was no need to sniff
through the data additionally, since all the required info is preparsed when
calculating frame count. It even helps in performances as it optimizes the frame
dependency before actual decoding. That's why I think it shouldn't be a problem
to do it as you said.

[1] 
https://codereview.chromium.org/1460523002/patch/100001/110011
bool GIFImageDecoder::canDecodeTo(size_t index, ImageFrame::ColorType
outputType)

[scroggo_chromium, 2017-07-12 20:08:02]

We're dropping Index8 (skbug.com/6828)