New issue
Advanced search Search tips

Issue 715812 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Feature

Blocking:
issue 663569



Sign in to add a comment

Move GIFImageDecoder to SkCodec

Project Member Reported by cblume@chromium.org, Apr 26 2017

Issue description

Move GIFImageDecoder to SkCodec

SkCodec is gaining advantages over Blink's image decoders.
scroggo@ has a nice summary:

- Skia already has some features that Chromium is interested in:
  - decoding scaled versions of images
  - decoding subsets (i.e. issue 468914)
  - SIMD optimizations for writing pixels
- In general, sharing code means that new features/bug fixes in Android
  benefit Chromium and vice versa
- Skia's API is designed to allow the client to handle caching, so that
  the client that knows the bigger picture can make caching decisions in
  a more informed way than ImageDecoder can.
 
Performance measurements of this patch can be found here (requires @google.com account):
https://docs.google.com/a/chromium.org/spreadsheets/d/18xPfE241z4-1hQdTBdFEJzDWbFZIPRyQ2ldBHoSHhqY/edit?usp=sharing

A roadmap of improvements for image decoders (including improvements unlocked by using SkCodec) can be found here (requires @chromium.org account):
https://docs.google.com/a/chromium.org/document/d/1T06pxiff3oy8KDqWGqWL-_nZqUJ8AMppouPQ_DLrvpk/edit?usp=sharing

To recreate the performance measurements on your own, apply patch https://crrev.com/2893023003 and run:
$ out/Release/image_decode_bench mygif.gif 1000 > mygif.csv
(for 1000 iterations). This feeds the timing data into mygif.csv, which can then be imported into a spreadsheet for visualization.
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4fed3346549a90c0de40c02f6388e19e8151e92a

commit 4fed3346549a90c0de40c02f6388e19e8151e92a
Author: cblume <cblume@chromium.org>
Date: Thu Aug 17 18:19:18 2017

Use SkCodec internally in GIFImageDecoder

Previously, GIFImageDecoder used GIFImageReader internally. SkCodec uses a
modified version of that class (SkGifImageReader; adapted in
crrev.com/2045293002). SkCodec provides the following benefits:
- SIMD optimized code for writing pixels
- an API that allows the client to handle caching
- flexibility regarding the required frame to use
- the ability to decode scaled versions of images
- subset decoding (i.e. issue 468914)
  (not fully implemented for GIF)
- the ability to decode to half-width float format

In addition, this patch enables sharing code between Android, Skia, and
Chromium. This means that new features/bug fixes in Android benefit Chromium
and vice versa.

For larger images (above ~60x60), the SIMD optimizations show a much bigger
benefit (up to 24% in one case). For most images, decoding speed is about the
same. Images with many frames that contain tiny update regions are a hair
slower. The mean decode time across all tested images showed an improvement. Raw
performance numbers can be found here:
https://docs.google.com/spreadsheets/d/1JqCPdYmbasOwKRdvuG6ZI4gwq9dPvnJA9oDquh7SxOQ/edit?usp=sharing

GIFImageDecoder still handles the cached frames currently, but this change will
allow future changes in Blink to make wiser caching Decisions (such as keeping
all frames of a 3-frame animation if those frames are small). Full road map:
https://docs.google.com/a/chromium.org/document/d/1T06pxiff3oy8KDqWGqWL-_nZqUJ8AMppouPQ_DLrvpk/edit?usp=sharing

This results in some behavior changes:
- SkCodec does not check the alpha of each pixel during decode (for speed and
  simplicity).
  As a result, GIFImageDecoder no longer corrects opacity or the required frame
  after decoding a frame. No performance penalty has been observed for
  incorrectly leaving a frame marked as having transparency.
- SkCodec guesses transparency based on the presence of a
  transparent index (in addition to being subset) and uses this to
  potentially determine an earlier required frame.

BUG= 715812 

Review-Url: https://codereview.chromium.org/2565323003
Cr-Commit-Position: refs/heads/master@{#495230}

[modify] https://crrev.com/4fed3346549a90c0de40c02f6388e19e8151e92a/skia/BUILD.gn
[modify] https://crrev.com/4fed3346549a90c0de40c02f6388e19e8151e92a/third_party/WebKit/Source/platform/BUILD.gn
[modify] https://crrev.com/4fed3346549a90c0de40c02f6388e19e8151e92a/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h
[modify] https://crrev.com/4fed3346549a90c0de40c02f6388e19e8151e92a/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
[modify] https://crrev.com/4fed3346549a90c0de40c02f6388e19e8151e92a/third_party/WebKit/Source/platform/image-decoders/ImageFrame.h
[add] https://crrev.com/4fed3346549a90c0de40c02f6388e19e8151e92a/third_party/WebKit/Source/platform/image-decoders/SegmentStream.cpp
[add] https://crrev.com/4fed3346549a90c0de40c02f6388e19e8151e92a/third_party/WebKit/Source/platform/image-decoders/SegmentStream.h
[add] https://crrev.com/4fed3346549a90c0de40c02f6388e19e8151e92a/third_party/WebKit/Source/platform/image-decoders/SegmentStreamTest.cpp
[modify] https://crrev.com/4fed3346549a90c0de40c02f6388e19e8151e92a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
[modify] https://crrev.com/4fed3346549a90c0de40c02f6388e19e8151e92a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h
[modify] https://crrev.com/4fed3346549a90c0de40c02f6388e19e8151e92a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp
[delete] https://crrev.com/a89466ce1d4f5aac646b16c0c66a50cdeb8f228d/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.cpp
[delete] https://crrev.com/a89466ce1d4f5aac646b16c0c66a50cdeb8f228d/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.h

Comment 3 by cblume@chromium.org, Aug 30 2017

Status: Fixed (was: Assigned)

Sign in to add a comment