Issue 1460523002: GIF decoding to Index8, unit tests and misusing unit test as benchmark

aleksandar.stojiljkovic

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

5 years, 1 month ago (2015-11-17 22:37:30 UTC) #1

aleksandar.stojiljkovic

This is work in progress code - not intended to be submitted yet. As the ...

5 years, 1 month ago (2015-11-17 22:37:30 UTC) #2

aleksandar.stojiljkovic

Description was changed from ========== GIF decoding to Index8, unit tests and misusing unit test ...

5 years, 1 month ago (2015-11-17 22:39:06 UTC) #3

Description was changed from

==========
GIF decoding to Index8, unit tests and misusing unit test as banchmark

This is a prototype code, not yet integrated to skia - just running it from
GIFImageDecoderTest to verify decoding integrity (checking pixel by pixel)
and adding code to the test to measure.

Decoding to Index8 is doen only when supported (when subframes are not
having offset or transparency with dependency to previous frames. Once
there is subframe detected, index8 s abandoned and next frame decoding
continues from RGBA from there on (and not going back to Index8 at all
for that pixture). Since dependent frames are copied anyway when
advancing to new frame, and RGBA->RGBA copy is cheaper than Index8->RGBA
copy, but this only could happen for one frame and only in multiframe
animations, this is not causing problems (at least the numbers show it
so). Could be optimized by skipping check and clamping to 8 bit.

Measurement is not completelly fair, since optimization identified for
the innermost loop (GIFImageDecoder::haveDecodedRowI8) is not applied
to N32 case and it is applicable. Keeping it this way to verify integrity
and submitting N32 decoding optimization in another patch.

Measurements:
Ubuntu 14.04 Intel® Core™ i7-5500U CPU @ 2.40GHz × 4

ninja -C out/Release webkit_unit_tests && ./out/Release/webkit_unit_tests \
 --release --gtest_filter=GIFImageDecoderTest.* --single-process-tests

At the end, prints banchmark results like this:
GIFImageDecoderTest.tempBench_decodeN32issue_476531_AqLvXJh_gif_x1 (301251 ms)
There is a warmup round and doing random frame decode test with discarding
frames in a loop with variable step. x5 means that tests are repeated 5
times.

dolph.gif has transparency.
Index8issue_476531.gif is taken from the issue, dont know if licence allows
to have it. 374 opaque frames.
large_checkerboard.gif is one frame.

RGBA8888animated_10color_gif_x5 (81 ms)
Index8animated_10color_gif_x5 (71 ms)
RGBA8888radient_gif_x10 (3 ms)
Index8radient_gif_x10 (2 ms)
RGBA8888animated_gif_with_offsets_gif_x5 (10 ms)
Index8animated_gif_with_offsets_gif_x5 (10 ms)
RGBA8888animated_gif_x10 (3 ms)
Index8animated_gif_x10 (4 ms)
RGBA8888quicksort_gif_x5 (11765 ms)
Index8quicksort_gif_x5 (11602 ms)
RGBA8888large_gif_checkerboard_gif_x5 (866 ms)
Index8large_gif_checkerboard_gif_x5 (291 ms)
RGBA88883dolph_gif_x5 (43 ms)
Index83dolph_gif_x5 (23 ms)
RGBA8888issue_476531_AqLvXJh_gif_x1 (303422 ms)
Index8issue_476531_AqLvXJh_gif_x1 (165353 ms)

BUG=
==========

to

==========
GIF decoding to Index8, unit tests and misusing unit test as banchmark

This is a prototype code, not yet integrated to skia - just running it from
GIFImageDecoderTest to verify decoding integrity (checking pixel by pixel)
and adding code to the test to measure.

Decoding to Index8 is doen only when supported (when subframes are not
having offset or transparency with dependency to previous frames. Once
there is subframe detected, index8 s abandoned and next frame decoding
continues from RGBA from there on (and not going back to Index8 at all
for that pixture). Since dependent frames are copied anyway when
advancing to new frame, and RGBA->RGBA copy is cheaper than Index8->RGBA
copy, but this only could happen for one frame and only in multiframe
animations, this is not causing problems (at least the numbers show it
so). Could be optimized by skipping check and clamping to 8 bit.

Measurement is not completelly fair, since optimization identified for
the innermost loop (GIFImageDecoder::haveDecodedRowI8) is not applied
to N32 case and it is applicable. Keeping it this way to verify integrity
and submitting N32 decoding optimization in another patch.

Measurements:
Ubuntu 14.04 Intel® Core™ i7-5500U CPU @ 2.40GHz × 4

ninja -C out/Release webkit_unit_tests && ./out/Release/webkit_unit_tests \
 --release --gtest_filter=GIFImageDecoderTest.* --single-process-tests

At the end, prints banchmark results like this:
GIFImageDecoderTest.tempBench_decodeN32issue_476531_AqLvXJh_gif_x1 (301251 ms)
There is a warmup round and doing random frame decode test with discarding
frames in a loop with variable step. x5 means that tests are repeated 5
times.

dolph.gif has transparency.
Index8issue_476531.gif is taken from the issue, dont know if licence allows
to have it. 374 opaque frames.
large_checkerboard.gif is one frame.

RGBA8888animated_10color_gif_x5 (81 ms)
Index8animated_10color_gif_x5 (71 ms)
RGBA8888radient_gif_x10 (3 ms)
Index8radient_gif_x10 (2 ms)
RGBA8888animated_gif_with_offsets_gif_x5 (10 ms)
Index8animated_gif_with_offsets_gif_x5 (10 ms)
RGBA8888animated_gif_x10 (3 ms)
Index8animated_gif_x10 (4 ms)
RGBA8888quicksort_gif_x5 (11765 ms)
Index8quicksort_gif_x5 (11602 ms)
RGBA8888large_gif_checkerboard_gif_x5 (866 ms)
Index8large_gif_checkerboard_gif_x5 (291 ms)
RGBA88883dolph_gif_x5 (43 ms)
Index83dolph_gif_x5 (23 ms)
RGBA8888issue_476531_AqLvXJh_gif_x1 (303422 ms)
Index8issue_476531_AqLvXJh_gif_x1 (165353 ms)

BUG=476531
==========

aleksandar.stojiljkovic

Description was changed from ========== GIF decoding to Index8, unit tests and misusing unit test ...

5 years, 1 month ago (2015-11-17 22:46:55 UTC) #4

Description was changed from

==========
GIF decoding to Index8, unit tests and misusing unit test as banchmark

This is a prototype code, not yet integrated to skia - just running it from
GIFImageDecoderTest to verify decoding integrity (checking pixel by pixel)
and adding code to the test to measure.

Decoding to Index8 is doen only when supported (when subframes are not
having offset or transparency with dependency to previous frames. Once
there is subframe detected, index8 s abandoned and next frame decoding
continues from RGBA from there on (and not going back to Index8 at all
for that pixture). Since dependent frames are copied anyway when
advancing to new frame, and RGBA->RGBA copy is cheaper than Index8->RGBA
copy, but this only could happen for one frame and only in multiframe
animations, this is not causing problems (at least the numbers show it
so). Could be optimized by skipping check and clamping to 8 bit.

Measurement is not completelly fair, since optimization identified for
the innermost loop (GIFImageDecoder::haveDecodedRowI8) is not applied
to N32 case and it is applicable. Keeping it this way to verify integrity
and submitting N32 decoding optimization in another patch.

Measurements:
Ubuntu 14.04 Intel® Core™ i7-5500U CPU @ 2.40GHz × 4

ninja -C out/Release webkit_unit_tests && ./out/Release/webkit_unit_tests \
 --release --gtest_filter=GIFImageDecoderTest.* --single-process-tests

At the end, prints banchmark results like this:
GIFImageDecoderTest.tempBench_decodeN32issue_476531_AqLvXJh_gif_x1 (301251 ms)
There is a warmup round and doing random frame decode test with discarding
frames in a loop with variable step. x5 means that tests are repeated 5
times.

dolph.gif has transparency.
Index8issue_476531.gif is taken from the issue, dont know if licence allows
to have it. 374 opaque frames.
large_checkerboard.gif is one frame.

RGBA8888animated_10color_gif_x5 (81 ms)
Index8animated_10color_gif_x5 (71 ms)
RGBA8888radient_gif_x10 (3 ms)
Index8radient_gif_x10 (2 ms)
RGBA8888animated_gif_with_offsets_gif_x5 (10 ms)
Index8animated_gif_with_offsets_gif_x5 (10 ms)
RGBA8888animated_gif_x10 (3 ms)
Index8animated_gif_x10 (4 ms)
RGBA8888quicksort_gif_x5 (11765 ms)
Index8quicksort_gif_x5 (11602 ms)
RGBA8888large_gif_checkerboard_gif_x5 (866 ms)
Index8large_gif_checkerboard_gif_x5 (291 ms)
RGBA88883dolph_gif_x5 (43 ms)
Index83dolph_gif_x5 (23 ms)
RGBA8888issue_476531_AqLvXJh_gif_x1 (303422 ms)
Index8issue_476531_AqLvXJh_gif_x1 (165353 ms)

BUG=476531
==========

to

==========
GIF decoding to Index8, unit tests and misusing unit test as benchmark

This is a prototype code, not yet integrated to skia - just running it from
GIFImageDecoderTest to verify decoding integrity (checking pixel by pixel)
and adding code to the test to measure.

Decoding to Index8 is done only when supported (when subframes are not
having offset or transparency with dependency to previous frames. Once
there is subframe detected, index8 s abandoned and next frame decoding
continues from RGBA from there on (and not going back to Index8 at all
for that gif). Since dependent frames are copied anyway when
advancing to new frame, and RGBA->RGBA copy is cheaper than Index8->RGBA
copy, but this only could happen for one frame and only in multiframe
animations, this is not causing problems (at least the numbers show it
so). Could be optimized by skipping check and clamping to 8 bit.

Measurement is not completely fair, since optimization identified for
the innermost loop (GIFImageDecoder::haveDecodedRowI8) is not applied
to N32 case and it is applicable. Keeping it this way to verify integrity
and submitting N32 decoding optimization in another patch.

Measurements:
Ubuntu 14.04 Intel® Core™ i7-5500U CPU @ 2.40GHz × 4

ninja -C out/Release webkit_unit_tests && ./out/Release/webkit_unit_tests \
 --release --gtest_filter=GIFImageDecoderTest.* --single-process-tests

At the end, prints benchmark results like this:
GIFImageDecoderTest.tempBench_decodeN32issue_476531_AqLvXJh_gif_x1 (301251 ms)
There is a warmup round and doing random frame decode test with discarding
frames in a loop with variable step. x5 means that tests are repeated 5
times.

dolph.gif has transparency.
Index8issue_476531.gif is taken from the issue, don't know if license allows
to have it. 374 opaque frames.
large_checkerboard.gif is one frame.

RGBA8888animated_10color_gif_x5 (81 ms)
Index8animated_10color_gif_x5 (71 ms)
RGBA8888radient_gif_x10 (3 ms)
Index8radient_gif_x10 (2 ms)
RGBA8888animated_gif_with_offsets_gif_x5 (10 ms)
Index8animated_gif_with_offsets_gif_x5 (10 ms)
RGBA8888animated_gif_x10 (3 ms)
Index8animated_gif_x10 (4 ms)
RGBA8888quicksort_gif_x5 (11765 ms)
Index8quicksort_gif_x5 (11602 ms)
RGBA8888large_gif_checkerboard_gif_x5 (866 ms)
Index8large_gif_checkerboard_gif_x5 (291 ms)
RGBA88883dolph_gif_x5 (43 ms)
Index83dolph_gif_x5 (23 ms)
RGBA8888issue_476531_AqLvXJh_gif_x1 (303422 ms)
Index8issue_476531_AqLvXJh_gif_x1 (165353 ms)

BUG=476531
==========

aleksandar.stojiljkovic

Description was changed from ========== GIF decoding to Index8, unit tests and misusing unit test ...

5 years, 1 month ago (2015-11-18 12:59:10 UTC) #5

Description was changed from

==========
GIF decoding to Index8, unit tests and misusing unit test as benchmark

This is a prototype code, not yet integrated to skia - just running it from
GIFImageDecoderTest to verify decoding integrity (checking pixel by pixel)
and adding code to the test to measure.

Decoding to Index8 is done only when supported (when subframes are not
having offset or transparency with dependency to previous frames. Once
there is subframe detected, index8 s abandoned and next frame decoding
continues from RGBA from there on (and not going back to Index8 at all
for that gif). Since dependent frames are copied anyway when
advancing to new frame, and RGBA->RGBA copy is cheaper than Index8->RGBA
copy, but this only could happen for one frame and only in multiframe
animations, this is not causing problems (at least the numbers show it
so). Could be optimized by skipping check and clamping to 8 bit.

Measurement is not completely fair, since optimization identified for
the innermost loop (GIFImageDecoder::haveDecodedRowI8) is not applied
to N32 case and it is applicable. Keeping it this way to verify integrity
and submitting N32 decoding optimization in another patch.

Measurements:
Ubuntu 14.04 Intel® Core™ i7-5500U CPU @ 2.40GHz × 4

ninja -C out/Release webkit_unit_tests && ./out/Release/webkit_unit_tests \
 --release --gtest_filter=GIFImageDecoderTest.* --single-process-tests

At the end, prints benchmark results like this:
GIFImageDecoderTest.tempBench_decodeN32issue_476531_AqLvXJh_gif_x1 (301251 ms)
There is a warmup round and doing random frame decode test with discarding
frames in a loop with variable step. x5 means that tests are repeated 5
times.

dolph.gif has transparency.
Index8issue_476531.gif is taken from the issue, don't know if license allows
to have it. 374 opaque frames.
large_checkerboard.gif is one frame.

RGBA8888animated_10color_gif_x5 (81 ms)
Index8animated_10color_gif_x5 (71 ms)
RGBA8888radient_gif_x10 (3 ms)
Index8radient_gif_x10 (2 ms)
RGBA8888animated_gif_with_offsets_gif_x5 (10 ms)
Index8animated_gif_with_offsets_gif_x5 (10 ms)
RGBA8888animated_gif_x10 (3 ms)
Index8animated_gif_x10 (4 ms)
RGBA8888quicksort_gif_x5 (11765 ms)
Index8quicksort_gif_x5 (11602 ms)
RGBA8888large_gif_checkerboard_gif_x5 (866 ms)
Index8large_gif_checkerboard_gif_x5 (291 ms)
RGBA88883dolph_gif_x5 (43 ms)
Index83dolph_gif_x5 (23 ms)
RGBA8888issue_476531_AqLvXJh_gif_x1 (303422 ms)
Index8issue_476531_AqLvXJh_gif_x1 (165353 ms)

BUG=476531
==========

to

==========
GIF decoding to Index8, unit tests and misusing unit test as benchmark

This is a prototype code, not yet integrated to skia - just running it from
GIFImageDecoderTest to verify decoding integrity (checking pixel by pixel)
and adding code to the test to measure.

Decoding to Index8 is done only when supported (when subframes are not
having offset or transparency with dependency to previous frames. Once
there is subframe detected, index8 s abandoned and next frame decoding
continues from RGBA from there on (and not going back to Index8 at all
for that gif). Since dependent frames are copied anyway when
advancing to new frame, and RGBA->RGBA copy is cheaper than Index8->RGBA
copy, but this only could happen for one frame and only in multiframe
animations, this is not causing problems (at least the numbers show it
so). Could be optimized by skipping check and clamping to 8 bit.

Measurement is not completely fair, since optimization identified for
the innermost loop (GIFImageDecoder::haveDecodedRowI8) is not applied
to N32 case and it is applicable. Keeping it this way to verify integrity
and submitting N32 decoding optimization in another patch.

Measurements:
Ubuntu 14.04 Intel® Core™ i7-5500U CPU @ 2.40GHz × 4

ninja -C out/Release webkit_unit_tests && ./out/Release/webkit_unit_tests \
 --release --gtest_filter=GIFImageDecoderTest.* --single-process-tests

At the end, prints benchmark results like this:
GIFImageDecoderTest.tempBench_decodeN32issue_476531_AqLvXJh_gif_x1 (301251 ms)
There is a warmup round and doing random frame decode test with discarding
frames in a loop with variable step. x5 means that tests are repeated 5
times.

dolph.gif has transparency.
Index8issue_476531.gif is taken from the issue, don't know if license allows
to have it. 374 opaque frames.
large_checkerboard.gif is one frame.
animated-gif-withoffsets has 5 frames, animated.gif 2 frames; decoding starts as
index8 and on later frames switched to n32. 

N32animated_10color_gif_x5 (67 ms)
Index8animated_10color_gif_x5 (47 ms)
N32radient_gif_x10 (2 ms)
Index8radient_gif_x10 (2 ms)
N32animated_gif_with_offsets_gif_x5 (11 ms)
Index8animated_gif_with_offsets_gif_x5 (9 ms)
N32animated_gif_x10 (3 ms)
Index8animated_gif_x10 (3 ms)
N32quicksort_gif_x5 (11430 ms)
Index8quicksort_gif_x5 (11349 ms)
N32large_gif_checkerboard_gif_x5 (961 ms)
Index8large_gif_checkerboard_gif_x5 (277 ms)
N323dolph_gif_x5 (39 ms)
Index83dolph_gif_x5 (21 ms)
N32issue_476531_AqLvXJh_gif_x1 (306513 ms)
Index8issue_476531_AqLvXJh_gif_x1 (163808 ms)

BUG=476531
==========

aleksandar.stojiljkovic

Description was changed from ========== GIF decoding to Index8, unit tests and misusing unit test ...

5 years, 1 month ago (2015-11-19 21:22:52 UTC) #6

Description was changed from

==========
GIF decoding to Index8, unit tests and misusing unit test as benchmark

This is a prototype code, not yet integrated to skia - just running it from
GIFImageDecoderTest to verify decoding integrity (checking pixel by pixel)
and adding code to the test to measure.

Decoding to Index8 is done only when supported (when subframes are not
having offset or transparency with dependency to previous frames. Once
there is subframe detected, index8 s abandoned and next frame decoding
continues from RGBA from there on (and not going back to Index8 at all
for that gif). Since dependent frames are copied anyway when
advancing to new frame, and RGBA->RGBA copy is cheaper than Index8->RGBA
copy, but this only could happen for one frame and only in multiframe
animations, this is not causing problems (at least the numbers show it
so). Could be optimized by skipping check and clamping to 8 bit.

Measurement is not completely fair, since optimization identified for
the innermost loop (GIFImageDecoder::haveDecodedRowI8) is not applied
to N32 case and it is applicable. Keeping it this way to verify integrity
and submitting N32 decoding optimization in another patch.

Measurements:
Ubuntu 14.04 Intel® Core™ i7-5500U CPU @ 2.40GHz × 4

ninja -C out/Release webkit_unit_tests && ./out/Release/webkit_unit_tests \
 --release --gtest_filter=GIFImageDecoderTest.* --single-process-tests

At the end, prints benchmark results like this:
GIFImageDecoderTest.tempBench_decodeN32issue_476531_AqLvXJh_gif_x1 (301251 ms)
There is a warmup round and doing random frame decode test with discarding
frames in a loop with variable step. x5 means that tests are repeated 5
times.

dolph.gif has transparency.
Index8issue_476531.gif is taken from the issue, don't know if license allows
to have it. 374 opaque frames.
large_checkerboard.gif is one frame.
animated-gif-withoffsets has 5 frames, animated.gif 2 frames; decoding starts as
index8 and on later frames switched to n32. 

N32animated_10color_gif_x5 (67 ms)
Index8animated_10color_gif_x5 (47 ms)
N32radient_gif_x10 (2 ms)
Index8radient_gif_x10 (2 ms)
N32animated_gif_with_offsets_gif_x5 (11 ms)
Index8animated_gif_with_offsets_gif_x5 (9 ms)
N32animated_gif_x10 (3 ms)
Index8animated_gif_x10 (3 ms)
N32quicksort_gif_x5 (11430 ms)
Index8quicksort_gif_x5 (11349 ms)
N32large_gif_checkerboard_gif_x5 (961 ms)
Index8large_gif_checkerboard_gif_x5 (277 ms)
N323dolph_gif_x5 (39 ms)
Index83dolph_gif_x5 (21 ms)
N32issue_476531_AqLvXJh_gif_x1 (306513 ms)
Index8issue_476531_AqLvXJh_gif_x1 (163808 ms)

BUG=476531
==========

to

==========
GIF decoding to Index8, unit tests and misusing unit test as benchmark

This is a prototype code, it is integrated and usable with browser but
skia's SkImageGenerator is missing. 
GIFImageDecoderTest is extended to verify decoding integrity (checking
pixel by pixel and comparing against current N32 decoding). Also benchmark
code added to the test that would be removed before submitting.

Decoding to Index8 is done almost all the time except in the case it cannot
be supported yet: when frames are having offset or transparency with
dependency to previous frame and there was also colortable change compared to
previous frame.
Once such frame gets detected, index8 is abandoned for next frame next
and it tries to resume back to from N32 to Index8 as soon as possible. 

Current N32 based decoding is kept to function the same way, to verify 
integrity.

Measurements:
Ubuntu 14.04 Intel® Core™ i7-5500U CPU @ 2.40GHz × 4

ninja -C out/Release webkit_unit_tests && ./out/Release/webkit_unit_tests \
 --release --gtest_filter=GIFImageDecoderTest.* --single-process-tests

At the end, prints benchmark results like this:
GIFImageDecoderTest.tempBench_decodeN32issue_476531_AqLvXJh_gif_x1 (301251 ms)
There is a warmup round and doing random frame decode test with discarding
frames in a loop with variable step. x5 means that tests are repeated 5
times.

dolph.gif has transparency.
Index8issue_476531.gif is taken from the issue, don't know if license allows
to have it. 374 opaque frames.
large_checkerboard.gif is one frame.
animated-gif-withoffsets has 5 frames, animated.gif 2 frames; decoding starts as
index8 and on later frames switched to n32.
quicksort_gif is with offset(subrect) and alpha.
disneypixar-disney-pixar-jTXvL4LjakYI8.gif is having alpha and offset (subrect)

N32animated_10color_gif_x5 (67 ms)
Index8animated_10color_gif_x5 (47 ms)
N32radient_gif_x10 (2 ms)
Index8radient_gif_x10 (2 ms)
N32animated_gif_with_offsets_gif_x5 (11 ms)
Index8animated_gif_with_offsets_gif_x5 (9 ms)
N32animated_gif_x10 (3 ms)
Index8animated_gif_x10 (3 ms)
N32quicksort_gif_x5 (11430 ms)
Index8quicksort_gif_x5 (4627 ms)
N32large_gif_checkerboard_gif_x5 (961 ms)
Index8large_gif_checkerboard_gif_x5 (277 ms)
N323dolph_gif_x5 (39 ms)
Index83dolph_gif_x5 (21 ms)
N32issue_476531_AqLvXJh_gif_x1 (306513 ms)
Index8issue_476531_AqLvXJh_gif_x1 (163808 ms)
N32disneypixar_disney_pixar_jTXvL4LjakYI8_gif_x1 (933 ms)
Index8disneypixar_disney_pixar_jTXvL4LjakYI8_gif_x1 (659 ms)

BUG=476531
==========

aleksandar.stojiljkovic

1) Added support for see through frames and subrect decoding: if there was no colortable ...

5 years, 1 month ago (2015-11-19 21:38:23 UTC) #7

aleksandar.stojiljkovic

aleksandar.stojiljkovic@intel.com changed reviewers: + senorblanco@chromium.org

5 years ago (2015-12-03 13:16:41 UTC) #8

aleksandar.stojiljkovic

Skia implementation (CPU and GPU support) is in Issue 1495693003: Index8 GPU and CPU raster ...

5 years ago (2015-12-03 13:16:42 UTC) #9

aleksandar.stojiljkovic

Description was changed from ========== GIF decoding to Index8, unit tests and misusing unit test ...

5 years ago (2015-12-03 13:17:40 UTC) #10

Description was changed from

==========
GIF decoding to Index8, unit tests and misusing unit test as benchmark

This is a prototype code, it is integrated and usable with browser but
skia's SkImageGenerator is missing. 
GIFImageDecoderTest is extended to verify decoding integrity (checking
pixel by pixel and comparing against current N32 decoding). Also benchmark
code added to the test that would be removed before submitting.

Decoding to Index8 is done almost all the time except in the case it cannot
be supported yet: when frames are having offset or transparency with
dependency to previous frame and there was also colortable change compared to
previous frame.
Once such frame gets detected, index8 is abandoned for next frame next
and it tries to resume back to from N32 to Index8 as soon as possible. 

Current N32 based decoding is kept to function the same way, to verify 
integrity.

Measurements:
Ubuntu 14.04 Intel® Core™ i7-5500U CPU @ 2.40GHz × 4

ninja -C out/Release webkit_unit_tests && ./out/Release/webkit_unit_tests \
 --release --gtest_filter=GIFImageDecoderTest.* --single-process-tests

At the end, prints benchmark results like this:
GIFImageDecoderTest.tempBench_decodeN32issue_476531_AqLvXJh_gif_x1 (301251 ms)
There is a warmup round and doing random frame decode test with discarding
frames in a loop with variable step. x5 means that tests are repeated 5
times.

dolph.gif has transparency.
Index8issue_476531.gif is taken from the issue, don't know if license allows
to have it. 374 opaque frames.
large_checkerboard.gif is one frame.
animated-gif-withoffsets has 5 frames, animated.gif 2 frames; decoding starts as
index8 and on later frames switched to n32.
quicksort_gif is with offset(subrect) and alpha.
disneypixar-disney-pixar-jTXvL4LjakYI8.gif is having alpha and offset (subrect)

N32animated_10color_gif_x5 (67 ms)
Index8animated_10color_gif_x5 (47 ms)
N32radient_gif_x10 (2 ms)
Index8radient_gif_x10 (2 ms)
N32animated_gif_with_offsets_gif_x5 (11 ms)
Index8animated_gif_with_offsets_gif_x5 (9 ms)
N32animated_gif_x10 (3 ms)
Index8animated_gif_x10 (3 ms)
N32quicksort_gif_x5 (11430 ms)
Index8quicksort_gif_x5 (4627 ms)
N32large_gif_checkerboard_gif_x5 (961 ms)
Index8large_gif_checkerboard_gif_x5 (277 ms)
N323dolph_gif_x5 (39 ms)
Index83dolph_gif_x5 (21 ms)
N32issue_476531_AqLvXJh_gif_x1 (306513 ms)
Index8issue_476531_AqLvXJh_gif_x1 (163808 ms)
N32disneypixar_disney_pixar_jTXvL4LjakYI8_gif_x1 (933 ms)
Index8disneypixar_disney_pixar_jTXvL4LjakYI8_gif_x1 (659 ms)

BUG=476531
==========

to

==========
GIF decoding to Index8, unit tests and misusing unit test as benchmark

The patch needs to go together with Issue 1495693003 : Index8 GPU and CPU raster
support.

This is a prototype code, it is integrated and usable with browser but
skia's SkImageGenerator is missing. 
GIFImageDecoderTest is extended to verify decoding integrity (checking
pixel by pixel and comparing against current N32 decoding). Also benchmark
code added to the test that would be removed before submitting.

Decoding to Index8 is done almost all the time except in the case it cannot
be supported yet: when frames are having offset or transparency with
dependency to previous frame and there was also colortable change compared to
previous frame.
Once such frame gets detected, index8 is abandoned for next frame next
and it tries to resume back to from N32 to Index8 as soon as possible. 

Current N32 based decoding is kept to function the same way, to verify 
integrity.

Measurements:
Ubuntu 14.04 Intel® Core™ i7-5500U CPU @ 2.40GHz × 4

ninja -C out/Release webkit_unit_tests && ./out/Release/webkit_unit_tests \
 --release --gtest_filter=GIFImageDecoderTest.* --single-process-tests

At the end, prints benchmark results like this:
GIFImageDecoderTest.tempBench_decodeN32issue_476531_AqLvXJh_gif_x1 (301251 ms)
There is a warmup round and doing random frame decode test with discarding
frames in a loop with variable step. x5 means that tests are repeated 5
times.

dolph.gif has transparency.
Index8issue_476531.gif is taken from the issue, don't know if license allows
to have it. 374 opaque frames.
large_checkerboard.gif is one frame.
animated-gif-withoffsets has 5 frames, animated.gif 2 frames; decoding starts as
index8 and on later frames switched to n32.
quicksort_gif is with offset(subrect) and alpha.
disneypixar-disney-pixar-jTXvL4LjakYI8.gif is having alpha and offset (subrect)

N32animated_10color_gif_x5 (67 ms)
Index8animated_10color_gif_x5 (47 ms)
N32radient_gif_x10 (2 ms)
Index8radient_gif_x10 (2 ms)
N32animated_gif_with_offsets_gif_x5 (11 ms)
Index8animated_gif_with_offsets_gif_x5 (9 ms)
N32animated_gif_x10 (3 ms)
Index8animated_gif_x10 (3 ms)
N32quicksort_gif_x5 (11430 ms)
Index8quicksort_gif_x5 (4627 ms)
N32large_gif_checkerboard_gif_x5 (961 ms)
Index8large_gif_checkerboard_gif_x5 (277 ms)
N323dolph_gif_x5 (39 ms)
Index83dolph_gif_x5 (21 ms)
N32issue_476531_AqLvXJh_gif_x1 (306513 ms)
Index8issue_476531_AqLvXJh_gif_x1 (163808 ms)
N32disneypixar_disney_pixar_jTXvL4LjakYI8_gif_x1 (933 ms)
Index8disneypixar_disney_pixar_jTXvL4LjakYI8_gif_x1 (659 ms)

BUG=476531
==========

aleksandar.stojiljkovic

Description was changed from ========== GIF decoding to Index8, unit tests and misusing unit test ...

5 years ago (2015-12-03 14:15:07 UTC) #11

Description was changed from

==========
GIF decoding to Index8, unit tests and misusing unit test as benchmark

The patch needs to go together with Issue 1495693003 : Index8 GPU and CPU raster
support.

This is a prototype code, it is integrated and usable with browser but
skia's SkImageGenerator is missing. 
GIFImageDecoderTest is extended to verify decoding integrity (checking
pixel by pixel and comparing against current N32 decoding). Also benchmark
code added to the test that would be removed before submitting.

Decoding to Index8 is done almost all the time except in the case it cannot
be supported yet: when frames are having offset or transparency with
dependency to previous frame and there was also colortable change compared to
previous frame.
Once such frame gets detected, index8 is abandoned for next frame next
and it tries to resume back to from N32 to Index8 as soon as possible. 

Current N32 based decoding is kept to function the same way, to verify 
integrity.

Measurements:
Ubuntu 14.04 Intel® Core™ i7-5500U CPU @ 2.40GHz × 4

ninja -C out/Release webkit_unit_tests && ./out/Release/webkit_unit_tests \
 --release --gtest_filter=GIFImageDecoderTest.* --single-process-tests

At the end, prints benchmark results like this:
GIFImageDecoderTest.tempBench_decodeN32issue_476531_AqLvXJh_gif_x1 (301251 ms)
There is a warmup round and doing random frame decode test with discarding
frames in a loop with variable step. x5 means that tests are repeated 5
times.

dolph.gif has transparency.
Index8issue_476531.gif is taken from the issue, don't know if license allows
to have it. 374 opaque frames.
large_checkerboard.gif is one frame.
animated-gif-withoffsets has 5 frames, animated.gif 2 frames; decoding starts as
index8 and on later frames switched to n32.
quicksort_gif is with offset(subrect) and alpha.
disneypixar-disney-pixar-jTXvL4LjakYI8.gif is having alpha and offset (subrect)

N32animated_10color_gif_x5 (67 ms)
Index8animated_10color_gif_x5 (47 ms)
N32radient_gif_x10 (2 ms)
Index8radient_gif_x10 (2 ms)
N32animated_gif_with_offsets_gif_x5 (11 ms)
Index8animated_gif_with_offsets_gif_x5 (9 ms)
N32animated_gif_x10 (3 ms)
Index8animated_gif_x10 (3 ms)
N32quicksort_gif_x5 (11430 ms)
Index8quicksort_gif_x5 (4627 ms)
N32large_gif_checkerboard_gif_x5 (961 ms)
Index8large_gif_checkerboard_gif_x5 (277 ms)
N323dolph_gif_x5 (39 ms)
Index83dolph_gif_x5 (21 ms)
N32issue_476531_AqLvXJh_gif_x1 (306513 ms)
Index8issue_476531_AqLvXJh_gif_x1 (163808 ms)
N32disneypixar_disney_pixar_jTXvL4LjakYI8_gif_x1 (933 ms)
Index8disneypixar_disney_pixar_jTXvL4LjakYI8_gif_x1 (659 ms)

BUG=476531
==========

to

==========
GIF decoding to Index8, unit tests and misusing unit test as benchmark

The patch needs to go together with Issue 1495693003 : Index8 GPU and CPU raster
support.

This is a prototype code, it is integrated and usable with browser but
skia's SkImageGenerator is missing. 
GIFImageDecoderTest is extended to verify decoding integrity (checking
pixel by pixel and comparing against current N32 decoding). Also benchmark
code added to the test that would be removed before submitting.

Decoding to Index8 is done almost all the time except in the case it cannot
be supported yet: when frames are having offset or transparency with
dependency to previous frame and there was also colortable change compared to
previous frame.
Once such frame gets detected, index8 is abandoned for next frame next
and it tries to resume back to from N32 to Index8 as soon as possible. 

Current N32 based decoding is kept to function the same way, to verify 
integrity.

Measurements:
Ubuntu 14.04 Intel® Core™ i7-5500U CPU @ 2.40GHz × 4

ninja -C out/Release webkit_unit_tests && ./out/Release/webkit_unit_tests \
 --release --gtest_filter=GIFImageDecoderTest.* --single-process-tests

At the end, prints benchmark results like this:
GIFImageDecoderTest.tempBench_decodeN32issue_476531_AqLvXJh_gif_x1 (301251 ms)
There is a warmup round and doing random frame decode test with discarding
frames in a loop with variable step. x5 means that tests are repeated 5
times.

dolph.gif has transparency.
Index8issue_476531.gif is taken from the issue, don't know if license allows
to have it. 374 opaque frames.
large_checkerboard.gif is one frame.
animated-gif-withoffsets has 5 frames, animated.gif 2 frames; decoding starts as
index8 and on later frames switched to n32.
quicksort_gif is with offset(subrect) and alpha.
disneypixar-disney-pixar-jTXvL4LjakYI8.gif is having alpha and offset (subrect)

N32animated_10color_gif_x5 (67 ms)
Index8animated_10color_gif_x5 (47 ms)
N32radient_gif_x10 (2 ms)
Index8radient_gif_x10 (2 ms)
N32animated_gif_with_offsets_gif_x5 (11 ms)
Index8animated_gif_with_offsets_gif_x5 (9 ms)
N32animated_gif_x10 (3 ms)
Index8animated_gif_x10 (3 ms)
N32quicksort_gif_x5 (11430 ms)
Index8quicksort_gif_x5 (4627 ms)
N32large_gif_checkerboard_gif_x5 (961 ms)
Index8large_gif_checkerboard_gif_x5 (277 ms)
N323dolph_gif_x5 (39 ms)
Index83dolph_gif_x5 (21 ms)
N32issue_476531_AqLvXJh_gif_x1 (306513 ms)
Index8issue_476531_AqLvXJh_gif_x1 (163808 ms)
N32disneypixar_disney_pixar_jTXvL4LjakYI8_gif_x1 (933 ms)
Index8disneypixar_disney_pixar_jTXvL4LjakYI8_gif_x1 (659 ms)

BUG=476531, 138421
==========

scroggo_chromium

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp#newcode138 third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:138: if (frame->status() != ImageFrame::FrameComplete) { It appears that if ...

5 years ago (2015-12-03 21:47:22 UTC) #12

aleksandar.stojiljkovic

I agree that it looks complicated - a lot of the code is left in ...

5 years ago (2015-12-04 00:07:35 UTC) #13

I agree that it looks complicated - a lot of the code is left in some limbo
state between duplication and refactoring, just for testing and comparing n32
and index8 - tests create 2 decoders for every file, index8 and n32, and then
frame by frame decode and check pixels, and index8 is sometimes returning n32 if
asked index8, and it is used to verify that decoding is correct.
If there would be a set of testing images, same in different formats (gif and
webp animations) but with exactly same pixels lot of this code could be removed
and than we could use one decoder against other for verifying correctness while
optimizing.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right):

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:138: if
(frame->status() != ImageFrame::FrameComplete) {
On 2015/12/03 21:47:21, scroggo_chromium wrote:
> It appears that if the frame was complete, it looks like this just returns the
> existing frame, whether it matches the outputColor or not.

Yes, RELEASE_ASSERT there needed.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right):

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:171:
ImageFrame* frameBufferAtIndex(size_t, ImageFrame::ColorType = ImageFrame::N32);
On 2015/12/03 21:47:21, scroggo_chromium wrote:
> It seems odd to me that the client asks for a particular color type. Does that
> mean this might do a copy? (i.e. what happens if it already decoded that frame
> to a different type?) Why doesn't the ImageDecoder just return the frame that
it
> has?

1st Reason is in discussion we had here:
https://codereview.chromium.org/1495693003/#msg8

Main reason though, once 1st reason is fixed, is for testing - wanted to keep
rgb decode having previous functionality and 1) keep it working and 2)compare
rgba-index8 in tests.

There is no copy, as you pointed out - the use case with frame decoded to one
color type and then asked in another is not existing, even in tests. I just
didn't add checks / asserts there. test use one decoder for all frames to rgba
or to "index9 + unsupported index8 frames in rgba)

Splitting to test method would help and some of the decode to this / can decode
to that would go away after changing skia API.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:336: virtual
void decodeTo(size_t index, ImageFrame::ColorType outputColor)
On 2015/12/03 21:47:21, scroggo_chromium wrote:
> If we need to pass outputColor, why not merge this with decode?

Didn't want to change code in other decoders, that is why it is just wrapping
them for N32 (currently only supported output).

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:338: if
(!canDecodeTo(index, outputColor))
On 2015/12/03 21:47:21, scroggo_chromium wrote:
> So the subclass can override decodeTo and not call canDecodeTo?

This would keep the existing decoders unchanged, just change it for gif decoder.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:339: return;
On 2015/12/03 21:47:21, scroggo_chromium wrote:
> So this will silently fail?
If there is no frame in cache, ImageFrameGenerator::decode calling this would
return false since the frame is empty.
If there is frame in cache of wrong type it should release assert if frame is
already cached and of wrong type - and not here but in
ImageDecoder::frameBufferAtIndex since it is from there calling this only if
frame is not complete.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:97: bool
ImageFrame::copyBitmapData(const ImageFrame& other, ColorType targetType)
On 2015/12/03 21:47:21, scroggo_chromium wrote:
> This method is almost identical to the other one. Why not use the same method,
> perhaps with a default parameter?

Again had the same reasoning - as this one is converting and other copying,
wanted to keep existing functionality as it is and maybe rename this one to
convertBitmapData. Also default parameter looked complicated since couldn't use
something like: targetType = other.targetType.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:99: if (this ==
&other)
On 2015/12/03 21:47:21, scroggo_chromium wrote:
> I wonder if we ever reach this condition, or if it's here just in case.
> 
> We have two cases where we call copyBitmapData; in GIF and WEBP. The code
looks
> the same for both ([1] and [2]), and it looks like we should never be copying
to
> itself.
> 
> In your version though, we return true even if the caller requested the other
> ColorType, which seems wrong.
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> [2]
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Yes, that is good point. ASSERT and return false if equal?

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:129: if
(indexOfTransparent < colorMap.size()) {
On 2015/12/03 21:47:21, scroggo_chromium wrote:
> Otherwise, we'll just initialize the bitmap to 255? That seems problematic if
> the color table has 256 colors, but the last color is not transparent. Maybe
we
> should not use Index8 in that case?

Yes, partially downloaded content in that case would have random (255) color the
missing part at the bottom. 
Important to cover this in byte by byte tests.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:133:
memset(getAddr8(0, j), transparent, sizeof(uint8_t) * newWidth);
On 2015/12/03 21:47:21, scroggo_chromium wrote:
> I think you could do this for the entire image with one memset. Since you did
> not specify a rowBytes, the bitmap will be tightly packed, so it wouldn't even
> write to extra memory.

Acknowledged.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:194:
memset(m_bitmap.getAddr8(0, j), 0xFF, sizeof(uint8_t) * xcount);
On 2015/12/03 21:47:21, scroggo_chromium wrote:
> Again, this is going to erase to an arbitrary color in the colortable. I think
> you actually want to use the color of the transparent index, if there is one.

Colortable returned by this decoder always have 256 colors, and all entries from
set of declared colors to the end are transparent color. For partially
downloaded content in opaque frames this seemed needed. Otherwise, I'll check if
making all colortables like this in skia could speed some of the CPU side
rendering (not to check the size)

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
(right):

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:110:
ASSERT(m_colorMode == ImageFrame::N32 || m_forceN32Decoding);
On 2015/12/03 21:47:21, scroggo_chromium wrote:
> It's weird to me that m_colorMode might *not* be N32, while m_forceN32Decoding
> is true.

Yes. m_colorMode holds information about what was requested by client. force is
set when switching from index8 to rgba for some frames.
Especially for testing, index8 decoder always set to index8, n32 decoder always
set to n32 and then compare frame by frame pixel by pixel. For index8 some of
frames are index8 some later are n32.

aleksandar.stojiljkovic

Continuing. https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp#newcode137 third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:137: // Tricky part here - decoding first row ...

5 years ago (2015-12-04 01:27:27 UTC) #14

Continuing.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
(right):

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:137:
// Tricky part here - decoding first row inits frame, and recognized that the
frame doesnt require
On 2015/12/03 21:47:22, scroggo_chromium wrote:
> This is tricky. Maybe it would be better for initFrameBuffer to report that it
> needed to switch?

I actually plan to move init out of looping through rows - it can be called from
client before calling GIFImageDecoder::haveDecodedRowN32 x height times. Again,
I thought that changing as less as possible would help review but not sure
anymore.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:142:
if (m_colorMode == ImageFrame::Index8 && !m_forceN32Decoding)
On 2015/12/03 21:47:22, scroggo_chromium wrote:
> IIUC, if this is true, initFrameBuffer called initFrameBufferIndex8, so it is
> already set up to decode the first row?
yes, for every row check if initialized then initializes and then return to row
processing - I'll change it to initialize, go through rows since you had already
went through this.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:165:
// FIXME - potential optimization: extend colorTable to 256 elems, write 0
On 2015/12/03 21:47:21, scroggo_chromium wrote:
> So this optimization is when the colortable is less than 256 elems only?

no; i meant that for colortables of e.g. 32 elements we could allways create 256
elements colortable and change this code bellow (remove *currentAddress =
colorTableIter[sourceValue])
i.e. replace branching by always doing *currentAddress =
colorTableIter[sourceValue]) (colortable would be extended to max sourceValue so
no need to check and transparent entries wuld be in place in the colorTable).

But, then I measured the benefit on different platforms and didn't see
significant improvement trend. Will remove the comment.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:195:
bool GIFImageDecoder::haveDecodedRowIndex8(size_t frameIndex,
GIFRow::const_iterator rowBegin, size_t width, size_t rowNumber, unsigned
repeatCount, bool writeTransparentPixels)
On 2015/12/03 21:47:21, scroggo_chromium wrote:
> It looks like this is largely the same as haveDecodedRowN32. It would be nice
to
> share more code here.

Agree, duplication for purpose that reviewer easier see that N32 decoding is
kept the same should be highlighted.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:249:
opaque = false;
On 2015/12/03 21:47:22, scroggo_chromium wrote:
> Why do we not want to put index into currentAddress here? (Is it because we
> zero-filled?)

It is faster this way (at least for examples I used in test)
In Patch #3 I had it like this:
            opaque = opaque && (*rowBegin ^ transparentPixel);
            *currentAddress++ = *rowBegin++;

It is prefilled to transparent index in buffer->setSizeIndex8(size().width(),
size().height(), colorTable, 
     frameContext->transparentPixel()).

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:284:
// FIXME do nothing frames for Index8 are probably copy of previous frame.
On 2015/12/03 21:47:21, scroggo_chromium wrote:
> What does this mean, and what is the FIXME? (i.e. what do you need to
> fix/change?)

All fine now, to be removed.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:364:
void GIFImageDecoder::decode(size_t index)
On 2015/12/03 21:47:21, scroggo_chromium wrote:
> So IIUC, decodeTo (which you added) lets the caller choose, but decode (the
> existing one) will just keep decoding to what was already selected?

decodeto wraps decode - decode() is not called directly but only from decodeto.
I was wrong here - by trying to keep the change minimal made it more complicated
for review.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:383:
if (!m_reader->decode(*i)) {
On 2015/12/03 21:47:21, scroggo_chromium wrote:
> So decode() may now fail for another reason (wrong color type)?

it could fail for previous reasons (setFailed() called), and if requesting color
switch (changed m_forceN32Decoding).
That is why getInfo() - related to previous discussion about virtual
SkImageGenerator::getInfo() will have to advance through all previous required
frames - to decode full frame and getPixels would just return it.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:387:
if (notSuitableForIndex8 != m_forceN32Decoding) {
On 2015/12/03 21:47:21, scroggo_chromium wrote:
> So GIFImageReader::decode modified m_forceN32Decoding? Maybe it would be
better
> to have it modify a local variable? (i.e. return a value, or modify an output
> parameter)

Acknowledged.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:387:
if (notSuitableForIndex8 != m_forceN32Decoding) {
On 2015/12/03 21:47:21, scroggo_chromium wrote:
> So GIFImageReader::decode modified m_forceN32Decoding? Maybe it would be
better
> to have it modify a local variable? (i.e. return a value, or modify an output
> parameter)

This relates to that init inside haveDecodedRow and then switch index8 first row
-> init (detects it is rgba and init rgba) -> continue first row and all other
rows in this frame with rgba.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:491:
static bool framesUseSameColorTable(const GIFFrameContext *frame1, const
GIFFrameContext *frame2)
On 2015/12/03 21:47:22, scroggo_chromium wrote:
> I think these should be const references?

Acknowledged.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:496:
// They both use global ColorMap.
On 2015/12/03 21:47:22, scroggo_chromium wrote:
> ASSERT(!frame2->localColorMap().isDefined())?

previous ^ should be resolving it, but it is complicated to read.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:523:
// This is changed here only to affect Index8 and the intention is to keep the
N32 code unchanged.
On 2015/12/03 21:47:21, scroggo_chromium wrote:
> Are you saying that updating required previous frame index is only done in
> Index8? Would N32 benefit from it?

Originally didn't want to change it, tried to deliver another patch for existing
code. Measurements replacing if() with unconditional writes didn't prove as
beneficial for performances - I still have the patch but in some cases it is
faster sometimes slower and didn't like it.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:535:
// Reset not suitable and table changed information as this is full new frame.
On 2015/12/03 21:47:21, scroggo_chromium wrote:
> What does "Reset not suitable" mean?

m_forceN32Decoding can be reset to false, and resume with Index8 if originally
requested by client.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:539:
if (!buffer->setSizeIndex8(size().width(), size().height(), colorTable,
frameContext->transparentPixel()))
This call is prefilling it so that all pixels have transparentPixel index.

aleksandar.stojiljkovic

Updated Skia API (crrev.com/1495693003/) to avoid fetching 2-ce but ask for color type - SkImageGenerator::ongetColorType() ...

5 years ago (2015-12-07 19:24:22 UTC) #15

Updated Skia API (crrev.com/1495693003/) to avoid fetching 2-ce but ask for
color type - SkImageGenerator::ongetColorType()

Refactored code after Leon's comments: removed confusing decodeTo, decoder can
either output N32 or try to output index8, cannot switch and decode same frame
to both outputs.

Moved initframe before outputting rows - simpler to follow logic then switchig
decoding during first row decoding.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
(right):

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:110:
ASSERT(m_colorMode == ImageFrame::N32 || m_forceN32Decoding);
On 2015/12/03 21:47:21, scroggo_chromium wrote:
> It's weird to me that m_colorMode might *not* be N32, while m_forceN32Decoding
> is true.

Done.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:137:
// Tricky part here - decoding first row inits frame, and recognized that the
frame doesnt require
On 2015/12/03 21:47:22, scroggo_chromium wrote:
> This is tricky. Maybe it would be better for initFrameBuffer to report that it
> needed to switch?

done (removed), init happens before outputting rows.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:142:
if (m_colorMode == ImageFrame::Index8 && !m_forceN32Decoding)
On 2015/12/03 21:47:22, scroggo_chromium wrote:
> IIUC, if this is true, initFrameBuffer called initFrameBufferIndex8, so it is
> already set up to decode the first row?

Done.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:165:
// FIXME - potential optimization: extend colorTable to 256 elems, write 0
On 2015/12/03 21:47:21, scroggo_chromium wrote:
> So this optimization is when the colortable is less than 256 elems only?

Done.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:284:
// FIXME do nothing frames for Index8 are probably copy of previous frame.
On 2015/12/03 21:47:21, scroggo_chromium wrote:
> What does this mean, and what is the FIXME? (i.e. what do you need to
> fix/change?)

Done.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:364:
void GIFImageDecoder::decode(size_t index)
On 2015/12/03 21:47:21, scroggo_chromium wrote:
> So IIUC, decodeTo (which you added) lets the caller choose, but decode (the
> existing one) will just keep decoding to what was already selected?

Done - removed decodeTo method. Decoding output set in constructor only.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:491:
static bool framesUseSameColorTable(const GIFFrameContext *frame1, const
GIFFrameContext *frame2)
On 2015/12/03 21:47:22, scroggo_chromium wrote:
> I think these should be const references?

Done.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:496:
// They both use global ColorMap.
On 2015/12/03 21:47:22, scroggo_chromium wrote:
> ASSERT(!frame2->localColorMap().isDefined())?

Done.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:499:
if (frame1->localColorMap().getPosition() ==
frame2->localColorMap().getPosition()) {
On 2015/12/03 21:47:21, scroggo_chromium wrote:
> This block of code is tough to follow. I think some local variables might make
> it easier to follow; something like:
> 
> auto cmap1 = frame1->localColorMap();
> auto cmap2 = frame2->localColorMap();
> 
> if (cmap1.getPosition() != cmap2.getPosition())
>   return false;
> 
> auto trans1 = frame1->transparentPixel();
> auto trans2 = frame2->transparentPixel();
> 
> return trans1 == trans2 || (trans1 > cmap1.table().size() && trans2 >=
> cmap2.table().size());

Done.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:535:
// Reset not suitable and table changed information as this is full new frame.
On 2015/12/03 21:47:21, scroggo_chromium wrote:
> What does "Reset not suitable" mean?

Done.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:550:
if (!m_forceN32Decoding) {
On 2015/12/03 21:47:21, scroggo_chromium wrote:
> Wait, should we reach this method (or get this far) if m_forceN32Decoding is
> true?

I think it looks more logical now - initFrameBufferIndex8 is called to
initialize all frames, except for testing mode when decoder is constructed to do
N32. So, N32 is only for testing.
Back to question: yes, m_forceN32Decoding is used on Index8 mode to mark that
last decoded frame was forced to N32 (althrough m_colorMode is Index8).

scroggo_chromium

As Mike pointed out, you'll need to make the colorType const (no onGetColorType virtual). Sorry ...

5 years ago (2015-12-08 22:24:51 UTC) #16

As Mike pointed out, you'll need to make the colorType const (no onGetColorType
virtual). Sorry I was mistaken there. But I'll review again once you keep the
colorType constant for a generator.

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:194:
memset(m_bitmap.getAddr8(0, j), 0xFF, sizeof(uint8_t) * xcount);
On 2015/12/04 00:07:34, aleksandar.stojiljkovic wrote:
> On 2015/12/03 21:47:21, scroggo_chromium wrote:
> > Again, this is going to erase to an arbitrary color in the colortable. I
think
> > you actually want to use the color of the transparent index, if there is
one.
> 
> Colortable returned by this decoder always have 256 colors, and all entries
from
> set of declared colors to the end are transparent color.

Except that if the image has 256 declared colors, the last one may not be
transparent, as you acknowledge above.

> For partially
> downloaded content in opaque frames this seemed needed.

We need to do something. I'm not convinced that drawing a random color at the
bottom is the right approach. Doesn't GIF optionally have a background color? If
the frame is opaque, using the background color seems like a good option.

> Otherwise, I'll check if
> making all colortables like this in skia could speed some of the CPU side
> rendering (not to check the size)

I don't follow. Make skia colortables like what?

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
(right):

https://codereview.chromium.org/1460523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:137:
// Tricky part here - decoding first row inits frame, and recognized that the
frame doesnt require
On 2015/12/04 01:27:27, aleksandar.stojiljkovic wrote:
> On 2015/12/03 21:47:22, scroggo_chromium wrote:
> > This is tricky. Maybe it would be better for initFrameBuffer to report that
it
> > needed to switch?
> 
> I actually plan to move init out of looping through rows - it can be called
from
> client before calling GIFImageDecoder::haveDecodedRowN32 x height times.
Again,
> I thought that changing as less as possible would help review but not sure
> anymore.

Yeah, I think that has to be a judgment call. Changing less can make sense to
confirm that an idea is good (so you don't keep working down a path that will
need to change), but it also makes it hard to imagine the end result.

Noel Gordon

On 2015/12/08 22:24:51, scroggo_chromium wrote: > Yeah, I think that has to be a judgment ...

5 years ago (2015-12-09 04:24:53 UTC) #17

aleksandar.stojiljkovic

On 2015/12/08 22:24:51, scroggo_chromium wrote: > As Mike pointed out, you'll need to make the ...

5 years ago (2015-12-09 06:51:45 UTC) #18

Noel Gordon

https://codereview.chromium.org/1460523002/diff/100001/third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.h File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.h (right): https://codereview.chromium.org/1460523002/diff/100001/third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.h#newcode67 third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.h:67: SkColorType onGetColorType() override; Interesting given the recent discussions on ...

5 years ago (2015-12-09 08:42:11 UTC) #19

aleksandar.stojiljkovic

On 2015/12/09 08:42:11, noel gordon wrote: > https://codereview.chromium.org/1460523002/diff/100001/third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.h > File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.h > (right): > > ...

5 years ago (2015-12-09 09:30:57 UTC) #20

aleksandar.stojiljkovic

On 2015/12/08 22:24:51, scroggo_chromium wrote: > As Mike pointed out, you'll need to make the ...

5 years ago (2015-12-14 13:09:47 UTC) #21

aleksandar.stojiljkovic

Description was changed from ========== GIF decoding to Index8, unit tests and misusing unit test ...

5 years ago (2015-12-14 13:10:46 UTC) #22

Description was changed from

==========
GIF decoding to Index8, unit tests and misusing unit test as benchmark

The patch needs to go together with Issue 1495693003 : Index8 GPU and CPU raster
support.

This is a prototype code, it is integrated and usable with browser but
skia's SkImageGenerator is missing. 
GIFImageDecoderTest is extended to verify decoding integrity (checking
pixel by pixel and comparing against current N32 decoding). Also benchmark
code added to the test that would be removed before submitting.

Decoding to Index8 is done almost all the time except in the case it cannot
be supported yet: when frames are having offset or transparency with
dependency to previous frame and there was also colortable change compared to
previous frame.
Once such frame gets detected, index8 is abandoned for next frame next
and it tries to resume back to from N32 to Index8 as soon as possible. 

Current N32 based decoding is kept to function the same way, to verify 
integrity.

Measurements:
Ubuntu 14.04 Intel® Core™ i7-5500U CPU @ 2.40GHz × 4

ninja -C out/Release webkit_unit_tests && ./out/Release/webkit_unit_tests \
 --release --gtest_filter=GIFImageDecoderTest.* --single-process-tests

At the end, prints benchmark results like this:
GIFImageDecoderTest.tempBench_decodeN32issue_476531_AqLvXJh_gif_x1 (301251 ms)
There is a warmup round and doing random frame decode test with discarding
frames in a loop with variable step. x5 means that tests are repeated 5
times.

dolph.gif has transparency.
Index8issue_476531.gif is taken from the issue, don't know if license allows
to have it. 374 opaque frames.
large_checkerboard.gif is one frame.
animated-gif-withoffsets has 5 frames, animated.gif 2 frames; decoding starts as
index8 and on later frames switched to n32.
quicksort_gif is with offset(subrect) and alpha.
disneypixar-disney-pixar-jTXvL4LjakYI8.gif is having alpha and offset (subrect)

N32animated_10color_gif_x5 (67 ms)
Index8animated_10color_gif_x5 (47 ms)
N32radient_gif_x10 (2 ms)
Index8radient_gif_x10 (2 ms)
N32animated_gif_with_offsets_gif_x5 (11 ms)
Index8animated_gif_with_offsets_gif_x5 (9 ms)
N32animated_gif_x10 (3 ms)
Index8animated_gif_x10 (3 ms)
N32quicksort_gif_x5 (11430 ms)
Index8quicksort_gif_x5 (4627 ms)
N32large_gif_checkerboard_gif_x5 (961 ms)
Index8large_gif_checkerboard_gif_x5 (277 ms)
N323dolph_gif_x5 (39 ms)
Index83dolph_gif_x5 (21 ms)
N32issue_476531_AqLvXJh_gif_x1 (306513 ms)
Index8issue_476531_AqLvXJh_gif_x1 (163808 ms)
N32disneypixar_disney_pixar_jTXvL4LjakYI8_gif_x1 (933 ms)
Index8disneypixar_disney_pixar_jTXvL4LjakYI8_gif_x1 (659 ms)

BUG=476531, 138421
==========

to

==========
GIF decoding to Index8, unit tests and misusing unit test as benchmark

The patch needs to go together with Issue 1495693003 : Index8 GPU and CPU raster
support.

GIFImageDecoderTest is extended to verify decoding integrity (checking
pixel by pixel and comparing against current N32 decoding). Also benchmark
code added to the test that would be removed before submitting.

Decoding to Index8 is done almost all the time except in the case it cannot
be supported yet: when frames are having offset or transparency with
dependency to previous frame and there was also colortable change compared to
previous frame. Once such frame gets detected, index8 is abandoned for dependent
frames after and they are decoded to N32. Decoding tries to resume back, from
N32 to Index8, as soon as possible.

Measurements:
Ubuntu 14.04 Intel® Core™ i7-5500U CPU @ 2.40GHz × 4

ninja -C out/Release webkit_unit_tests && ./out/Release/webkit_unit_tests \
 --release --gtest_filter=GIFImageDecoderTest.* --single-process-tests

At the end, prints benchmark results like this:
GIFImageDecoderTest.tempBench_decodeN32issue_476531_AqLvXJh_gif_x1 (301251 ms)
There is a warmup round and doing random frame decode test with discarding
frames in a loop with variable step. x5 means that tests are repeated 5
times.

dolph.gif has transparency.
Index8issue_476531.gif is taken from the issue, don't know if license allows
to have it. 374 opaque frames.
large_checkerboard.gif is one frame.
animated-gif-withoffsets has 5 frames, animated.gif 2 frames; decoding starts as
index8 and on later frames switched to n32.
quicksort_gif is with offset(subrect) and alpha.
disneypixar-disney-pixar-jTXvL4LjakYI8.gif is having alpha and offset (subrect)

N32animated_10color_gif_x5 (67 ms)
Index8animated_10color_gif_x5 (47 ms)
N32radient_gif_x10 (2 ms)
Index8radient_gif_x10 (2 ms)
N32animated_gif_with_offsets_gif_x5 (11 ms)
Index8animated_gif_with_offsets_gif_x5 (9 ms)
N32animated_gif_x10 (3 ms)
Index8animated_gif_x10 (3 ms)
N32quicksort_gif_x5 (11430 ms)
Index8quicksort_gif_x5 (4627 ms)
N32large_gif_checkerboard_gif_x5 (961 ms)
Index8large_gif_checkerboard_gif_x5 (277 ms)
N323dolph_gif_x5 (39 ms)
Index83dolph_gif_x5 (21 ms)
N32issue_476531_AqLvXJh_gif_x1 (306513 ms)
Index8issue_476531_AqLvXJh_gif_x1 (163808 ms)
N32disneypixar_disney_pixar_jTXvL4LjakYI8_gif_x1 (933 ms)
Index8disneypixar_disney_pixar_jTXvL4LjakYI8_gif_x1 (659 ms)

BUG=476531, 138421
==========

aleksandar.stojiljkovic

Ready for review after cleanup. First draft of design document with performance measurement on the ...

5 years ago (2015-12-22 15:19:55 UTC) #23

aleksandar.stojiljkovic

On 2015/12/22 15:19:55, aleksandar.stojiljkovic wrote: > Ready for review after cleanup. > First draft of ...

5 years ago (2015-12-22 17:50:05 UTC) #24

scroggo_chromium

Thank you for writing the design doc! Comments inline below: https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp#newcode499 ...

4 years, 11 months ago (2016-01-06 21:50:41 UTC) #25

aleksandar.stojiljkovic

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp#newcode301 third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:301: SkBitmap ImageFrameGenerator::tryToResumeDecode(const SkISize& scaledSize, size_t index, SkColorType outputColor) SkColorType ...

4 years, 11 months ago (2016-01-18 13:58:50 UTC) #26

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
(right):

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:301:
SkBitmap ImageFrameGenerator::tryToResumeDecode(const SkISize& scaledSize,
size_t index, SkColorType outputColor)
 SkColorType outputColor is not necessary. Removed.

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:499: return
(outputType == kN32_SkColorType);
On 2016/01/06 21:50:40, scroggo_chromium wrote:
> Would it make more sense to put this at the beginning of the method?

Done.

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:114:
Vector<PixelData>::const_iterator colorMapEnd = (colorMap.size() <= 256) ?
colorMap.end() : (colorMap.begin() + 256);
On 2016/01/06 21:50:40, scroggo_chromium wrote:
> The style guide seems to recommend using an index rather than an iterator.

Done.

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:126: size_t
rowBytes = (newWidth + sizeof(size_t) - 1) & (~(sizeof(size_t) - 1));
On 2016/01/06 21:50:40, scroggo_chromium wrote:
> Why'd you choose this rowBytes? If you use zero, SkBitmap::setInfo will pick
the
> minimum one for you.
I think the 0 makes rowBytes equals to newWidth. 
What I needed was size_t aligned row (nearest multiplier of size_t size), so
that it is not needed to copy byte by byte but to utilize memory alignment so
that size_t words can be handled. Verified this to be a bit faster on android
arm...

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:145: m_hasAlpha
= !((background >> 24) == 0xFF);
On 2016/01/06 21:50:40, scroggo_chromium wrote:
> Why not SkColorGetA(background)?

Done. Thanks, didn't know about it.

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:192:
memset(m_bitmap.getAddr8(0, j), 0xFF, sizeof(uint8_t) * xcount);
On 2016/01/06 21:50:40, scroggo_chromium wrote:
> As stated before, I'm concerned about setting this to an arbitrary color in
the
> color table. Are we guaranteed that this one is transparent?

Fixed in new patch...

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
(right):

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:38: ,
phaveDecodedRow(&GIFImageDecoder::haveDecodedRowIndex8)
On 2016/01/06 21:50:40, scroggo_chromium wrote:
> If the requested color type is N32, shouldn't this be haveDecodedRowN32?

Done. Removed the line since the initialization or m_haveDecodedRowFunction
happens later in initFrameBuffer (so before writing any rows).

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:182:
bool GIFImageDecoder::haveDecodedRowIndex8(const GIFFrameContext& frameContext,
GIFRow::const_iterator rowBegin, size_t rowNumber, unsigned repeatCount, bool
writeTransparentPixels)
On 2016/01/06 21:50:40, scroggo_chromium wrote:
> Is there a way to share more code here?

Done.

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:287:
const ImageFrame* previousBuffer =
&m_frameBufferCache[buffer.requiredPreviousFrameIndex()];
On 2016/01/06 21:50:41, scroggo_chromium wrote:
> Why the name change?
> 
> This name change distracts from the logical changes that you are making. My
> preference would be to leave the name unchanged.

Done.

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:401:
bool GIFImageDecoder::initFrameBufferFromPreviousN32(ImageFrame* const buffer,
const ImageFrame& previousBuffer)
On 2016/01/06 21:50:41, scroggo_chromium wrote:
> I was a little surprised to see a const pointer. Are you worried the
> implementation is going to change buffer to point to something else? I don't
> think Blink does this elsewhere?

Done. Thanks. Method renamed to initFrameBufferFromPrevious since the same
functionality is required for Index8.

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:430:
const ImageFrame* previousBuffer =
&m_frameBufferCache[requiredPreviousFrameIndex];
On 2016/01/06 21:50:41, scroggo_chromium wrote:
> Again, I think the name change is distracting.

Done.

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:451:
&& (useGlobalColorMap && m_reader->backgroundIndex() >= colorMap.getTableSize())
On 2016/01/06 21:50:40, scroggo_chromium wrote:
> Do you need the parentheses here?

Done.

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:455:
// when colormap is opaque and full (256 entries) and in the same time:
On 2016/01/06 21:50:40, scroggo_chromium wrote:
> nit: at* the same time

Done.

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:457:
// - all frames are not fullscreen, i.e. they are decoded to sub rectangle of
image rectangle.
On 2016/01/06 21:50:40, scroggo_chromium wrote:
> This says "all frames", but this only checks a single frame, correct?

Done. Yes, the check is per frame, not for all - fixed the comment.

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:463:
// If frame is sharing colormap with required previous frame, it is possible to
keep using Index8.
On 2016/01/06 21:50:40, scroggo_chromium wrote:
> nit:
> 
> If frame shares* ...

Done.

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:469:
// Helper method for recomputing if frame decoding has no dependency to previous
frames.
On 2016/01/06 21:50:40, scroggo_chromium wrote:
> I find this sentence a little awkward. How about:
> 
> Helper method to check to see if a previous frame dependency can be removed.

Done, "to see " removed.

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:479:
// e.g. Color table in some of the GIFs is having 40 elements and
transparentPixel set to 0xFF.
On 2016/01/06 21:50:41, scroggo_chromium wrote:
> nit: "is having" -> "has"

Done.

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:480:
bool opaque = frameContext.transparentPixel() >= getColorMap(frameContext,
*m_reader).getTableSize();
On 2016/01/06 21:50:40, scroggo_chromium wrote:
> nit: If isFullScreen is false, we do not need to check for opaque.

Done.

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:497:
return 0xFF;
On 2016/01/06 21:50:40, scroggo_chromium wrote:
> This still concerns me. Used as an index, 0xFF will be an arbitrary color in
the
> color table.

Done.
There is check in isIndex8Applicable is not allowing it to come here, but
instead of hiding it under implicit short->char conversion that you mentioned
before, changed all to unsigned and there is assert in
ImageFrame::setSizeIndex8.

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:505:
return 0;
On 2016/01/06 21:50:40, scroggo_chromium wrote:
> SK_ColorTRANSPARENT ? Or do we use 0 elsewhere?

Changed here to SK_ColorTRANSPARENT.
 
0 is/was in use in haveDecodedRow, but there it is about writing to PixelData
(typedef uint32_t), not to SkColor.

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:515:
setupHaveDecodedRowCallbacks(buffer->getSkBitmap().colorType() ==
kIndex_8_SkColorType);
On 2016/01/06 21:50:40, scroggo_chromium wrote:
> If it is partial, shouldn't we already be using the correct callback?
e.g. if partial frame is N32, and previous required frame was index8 (and got
completely decoded and then removed from cache), then when again decoding
previous required frame, decoding callbacks would be set to index8 and here it
is important to set them for this frame.

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:532:
return setFailed();
On 2016/01/06 21:50:40, scroggo_chromium wrote:
> I don't see this addressed in the style guide, but I find this hard to read -
> the code lines up with the condition. I think this looks better as:
> 
> if (reallyLongCondition(
>     parameter))
> {
>     code;
> }
> 
> or
> 
> if (reallyLongCondition(
>         parameter))
>     code;
> 
> Others may disagree though.

Not sure if I did the right thing, let's see if it improved.

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:567:
if ((index >= frameCount()) || (m_colorMode == ImageFrame::N32) || failed())
On 2016/01/06 21:50:40, scroggo_chromium wrote:
> if outputType == N32, shouldn't we always return true? It seems like you only
> need to go back to see if index8 is supported.

No, the recent changes made this deterministic early on, from the constructor
before decoding. If requested mode for decoder (in constructor) was index8,
output for given frame can only be one, index8 or n32. Note that decode API is
like decode(), not like decode(colortype).
Anyway, there is no call canDecodeTo(N32) now in the code and would like to keep
it as it is.

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h
(right):

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h:62: bool
(GIFImageDecoder::*phaveDecodedRow)(const GIFFrameContext&,
GIFRow::const_iterator rowBegin, size_t rowNumber, unsigned repeatCount, bool
writeTransparentPixels);
On 2016/01/06 21:50:41, scroggo_chromium wrote:
> Does this pointer need to be public?
> 
> Also, maybe there should be a typedef? Or I think C++11 "using" matches the
> style guide?
> 
> Lastly, this name does not match the style guide. Typically, member variables
> start with "m_". I'm not sure how Chromium names function pointers. I've seen
a
> "p" at the start elsewhere, but not in Chromium. Maybe this should be
something
> like "m_haveDecodedRowFunc"?

Done.
All private, typedef, m_XXXXFunction found e.g. in Timer.h. 
typedef it is - looks less ugly then using with class scope.

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h:75: bool
haveDecodedRowN32(const GIFFrameContext&, GIFRow::const_iterator rowBegin,
size_t rowNumber, unsigned repeatCount, bool writeTransparentPixels);
On 2016/01/06 21:50:41, scroggo_chromium wrote:
> Can these be private?

Done.

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h:101:
unsigned char getBackgroundIndex(const GIFFrameContext&) const;
On 2016/01/06 21:50:41, scroggo_chromium wrote:
> I think you defined this as an unsigned short in GIFImageReader. Why the
> difference?

Done. unsigned everywhere.

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h:110: //
Holds information about which decoding output was requested in constructor.
On 2016/01/06 21:50:41, scroggo_chromium wrote:
> If this always matches the constructor, I think it should be const and have a
> more descriptive name. Maybe m_requestedColorMode?

Done.

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.cpp
(right):

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.cpp:208:
for (const unsigned char* ch = block; bytesInBlock-- > 0; ++ch) {
On 2016/01/06 21:50:41, scroggo_chromium wrote:
> This seems like an unnecessary change, and at least unrelated.

Done.

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.cpp:348: if
(!client->initFrameBuffer(m_frameId))
On 2016/01/06 21:50:41, scroggo_chromium wrote:
> This function (GIFFrameContext::decode) may be called multiple times for the
> same frame (if not all of the data is available), resulting in calling
> initFrameBuffer multiple times. Is that intentional? The existing callers
check
> first to see if it's empty.

Acknowledged.
It is intentional.
initFrameBuffer doesnt do much if frame is not empty - just sets proper
callbacks.

About setting callbacks for non empty frame, there is an explanation in
initFrameBuffer(), copying here:
e.g. if partial frame is N32, and previous required frame was index8 (and got
completely decoded and then removed from cache), then when again decoding
previous required frame, decoding callbacks would be set to index8 and here it
is important to set them for this frame.

Previously, (existing callers) it was called on every line, that's where
checking before calling made sense, now it is called on the decode call for the
frame...

aleksandar.stojiljkovic

Description was changed from ========== GIF decoding to Index8, unit tests and misusing unit test ...

4 years, 11 months ago (2016-01-19 11:42:11 UTC) #27

Description was changed from

==========
GIF decoding to Index8, unit tests and misusing unit test as benchmark

The patch needs to go together with Issue 1495693003 : Index8 GPU and CPU raster
support.

GIFImageDecoderTest is extended to verify decoding integrity (checking
pixel by pixel and comparing against current N32 decoding). Also benchmark
code added to the test that would be removed before submitting.

Decoding to Index8 is done almost all the time except in the case it cannot
be supported yet: when frames are having offset or transparency with
dependency to previous frame and there was also colortable change compared to
previous frame. Once such frame gets detected, index8 is abandoned for dependent
frames after and they are decoded to N32. Decoding tries to resume back, from
N32 to Index8, as soon as possible.

Measurements:
Ubuntu 14.04 Intel® Core™ i7-5500U CPU @ 2.40GHz × 4

ninja -C out/Release webkit_unit_tests && ./out/Release/webkit_unit_tests \
 --release --gtest_filter=GIFImageDecoderTest.* --single-process-tests

At the end, prints benchmark results like this:
GIFImageDecoderTest.tempBench_decodeN32issue_476531_AqLvXJh_gif_x1 (301251 ms)
There is a warmup round and doing random frame decode test with discarding
frames in a loop with variable step. x5 means that tests are repeated 5
times.

dolph.gif has transparency.
Index8issue_476531.gif is taken from the issue, don't know if license allows
to have it. 374 opaque frames.
large_checkerboard.gif is one frame.
animated-gif-withoffsets has 5 frames, animated.gif 2 frames; decoding starts as
index8 and on later frames switched to n32.
quicksort_gif is with offset(subrect) and alpha.
disneypixar-disney-pixar-jTXvL4LjakYI8.gif is having alpha and offset (subrect)

N32animated_10color_gif_x5 (67 ms)
Index8animated_10color_gif_x5 (47 ms)
N32radient_gif_x10 (2 ms)
Index8radient_gif_x10 (2 ms)
N32animated_gif_with_offsets_gif_x5 (11 ms)
Index8animated_gif_with_offsets_gif_x5 (9 ms)
N32animated_gif_x10 (3 ms)
Index8animated_gif_x10 (3 ms)
N32quicksort_gif_x5 (11430 ms)
Index8quicksort_gif_x5 (4627 ms)
N32large_gif_checkerboard_gif_x5 (961 ms)
Index8large_gif_checkerboard_gif_x5 (277 ms)
N323dolph_gif_x5 (39 ms)
Index83dolph_gif_x5 (21 ms)
N32issue_476531_AqLvXJh_gif_x1 (306513 ms)
Index8issue_476531_AqLvXJh_gif_x1 (163808 ms)
N32disneypixar_disney_pixar_jTXvL4LjakYI8_gif_x1 (933 ms)
Index8disneypixar_disney_pixar_jTXvL4LjakYI8_gif_x1 (659 ms)

BUG=476531, 138421
==========

to

==========
GIF decoding to Index8, unit tests and misusing unit test as benchmark

The patch needs to go together with Issue 1495693003 : Index8 GPU and CPU raster
support, though it is applicable without it too.

Design document explaining the change and measurements:
https://docs.google.com/document/d/1oWeD8IAqbFhBhu9DjYa3UUXRRbXNypf1S_vnDG7VG...

GIFImageDecoderTest is extended to verify decoding integrity (checking
pixel by pixel and comparing against current N32 decoding). Also benchmark
code added to the test that would be removed before submitting.

Decoding to Index8 is done almost all the time except in the case it cannot
be supported yet: when frames are having offset or transparency with
dependency to previous frame and there was also colortable change compared to
previous frame. Once such frame gets detected, index8 is abandoned for dependent
frames after and they are decoded to N32. Decoding tries to resume back, from
N32 to Index8, as soon as possible.

Measurements:
Ubuntu 14.04 Intel® Core™ i7-5500U CPU @ 2.40GHz × 4

ninja -C out/Release webkit_unit_tests && ./out/Release/webkit_unit_tests \
 --release --gtest_filter=GIFImageDecoderTest.* --single-process-tests

At the end, prints benchmark results like this:
GIFImageDecoderTest.tempBench_decodeN32issue_476531_AqLvXJh_gif_x1 (301251 ms)
There is a warmup round and doing random frame decode test with discarding
frames in a loop with variable step. x5 means that tests are repeated 5
times.

dolph.gif has transparency.
Index8issue_476531.gif is taken from the issue, don't know if license allows
to have it. 374 opaque frames.
large_checkerboard.gif is one frame.
animated-gif-withoffsets has 5 frames, animated.gif 2 frames; decoding starts as
index8 and on later frames switched to n32.
quicksort_gif is with offset(subrect) and alpha.
disneypixar-disney-pixar-jTXvL4LjakYI8.gif is having alpha and offset (subrect)

N32animated_10color_gif_x5 (67 ms)
Index8animated_10color_gif_x5 (47 ms)
N32radient_gif_x10 (2 ms)
Index8radient_gif_x10 (2 ms)
N32animated_gif_with_offsets_gif_x5 (11 ms)
Index8animated_gif_with_offsets_gif_x5 (9 ms)
N32animated_gif_x10 (3 ms)
Index8animated_gif_x10 (3 ms)
N32quicksort_gif_x5 (11430 ms)
Index8quicksort_gif_x5 (4627 ms)
N32large_gif_checkerboard_gif_x5 (961 ms)
Index8large_gif_checkerboard_gif_x5 (277 ms)
N323dolph_gif_x5 (39 ms)
Index83dolph_gif_x5 (21 ms)
N32issue_476531_AqLvXJh_gif_x1 (306513 ms)
Index8issue_476531_AqLvXJh_gif_x1 (163808 ms)
N32disneypixar_disney_pixar_jTXvL4LjakYI8_gif_x1 (933 ms)
Index8disneypixar_disney_pixar_jTXvL4LjakYI8_gif_x1 (659 ms)

BUG=476531, 138421
==========

aleksandar.stojiljkovic

aleksandar.stojiljkovic@intel.com changed reviewers: + pkasting@google.com

4 years, 11 months ago (2016-01-19 11:42:11 UTC) #28

aleksandar.stojiljkovic

Description was changed from ========== GIF decoding to Index8, unit tests and misusing unit test ...

4 years, 11 months ago (2016-01-19 11:45:01 UTC) #29

Description was changed from

==========
GIF decoding to Index8, unit tests and misusing unit test as benchmark

The patch needs to go together with Issue 1495693003 : Index8 GPU and CPU raster
support, though it is applicable without it too.

Design document explaining the change and measurements:
https://docs.google.com/document/d/1oWeD8IAqbFhBhu9DjYa3UUXRRbXNypf1S_vnDG7VG...

GIFImageDecoderTest is extended to verify decoding integrity (checking
pixel by pixel and comparing against current N32 decoding). Also benchmark
code added to the test that would be removed before submitting.

Decoding to Index8 is done almost all the time except in the case it cannot
be supported yet: when frames are having offset or transparency with
dependency to previous frame and there was also colortable change compared to
previous frame. Once such frame gets detected, index8 is abandoned for dependent
frames after and they are decoded to N32. Decoding tries to resume back, from
N32 to Index8, as soon as possible.

Measurements:
Ubuntu 14.04 Intel® Core™ i7-5500U CPU @ 2.40GHz × 4

ninja -C out/Release webkit_unit_tests && ./out/Release/webkit_unit_tests \
 --release --gtest_filter=GIFImageDecoderTest.* --single-process-tests

At the end, prints benchmark results like this:
GIFImageDecoderTest.tempBench_decodeN32issue_476531_AqLvXJh_gif_x1 (301251 ms)
There is a warmup round and doing random frame decode test with discarding
frames in a loop with variable step. x5 means that tests are repeated 5
times.

dolph.gif has transparency.
Index8issue_476531.gif is taken from the issue, don't know if license allows
to have it. 374 opaque frames.
large_checkerboard.gif is one frame.
animated-gif-withoffsets has 5 frames, animated.gif 2 frames; decoding starts as
index8 and on later frames switched to n32.
quicksort_gif is with offset(subrect) and alpha.
disneypixar-disney-pixar-jTXvL4LjakYI8.gif is having alpha and offset (subrect)

N32animated_10color_gif_x5 (67 ms)
Index8animated_10color_gif_x5 (47 ms)
N32radient_gif_x10 (2 ms)
Index8radient_gif_x10 (2 ms)
N32animated_gif_with_offsets_gif_x5 (11 ms)
Index8animated_gif_with_offsets_gif_x5 (9 ms)
N32animated_gif_x10 (3 ms)
Index8animated_gif_x10 (3 ms)
N32quicksort_gif_x5 (11430 ms)
Index8quicksort_gif_x5 (4627 ms)
N32large_gif_checkerboard_gif_x5 (961 ms)
Index8large_gif_checkerboard_gif_x5 (277 ms)
N323dolph_gif_x5 (39 ms)
Index83dolph_gif_x5 (21 ms)
N32issue_476531_AqLvXJh_gif_x1 (306513 ms)
Index8issue_476531_AqLvXJh_gif_x1 (163808 ms)
N32disneypixar_disney_pixar_jTXvL4LjakYI8_gif_x1 (933 ms)
Index8disneypixar_disney_pixar_jTXvL4LjakYI8_gif_x1 (659 ms)

BUG=476531, 138421
==========

to

==========
GIF decoding to Index8, unit tests and misusing unit test as benchmark

The patch needs to go together with Issue 1495693003 : Index8 GPU and CPU raster
support, though it is applicable without it too.

Design document explaining the change and measurements:
https://docs.google.com/document/d/1oWeD8IAqbFhBhu9DjYa3UUXRRbXNypf1S_vnDG7VG...

GIFImageDecoderTest is extended to verify decoding integrity (checking
pixel by pixel and comparing against current N32 decoding). Also benchmark
code added to the test that would be removed before submitting.

Decoding to Index8 is done almost all the time except in the case it cannot
be supported yet: when frames are having offset or transparency with
dependency to previous frame and there was also colortable change compared to
previous frame. Once such frame gets detected, index8 is abandoned for dependent
frames after and they are decoded to N32. Decoding tries to resume back, from
N32 to Index8, as soon as possible.

Measurements:
Ubuntu 14.04 Intel® Core™ i7-5500U CPU @ 2.40GHz × 4

ninja -C out/Release webkit_unit_tests && ./out/Release/webkit_unit_tests \
 --release --gtest_filter=GIFImageDecoderTest.* --single-process-tests

At the end, prints benchmark results like this:
GIFImageDecoderTest.tempBench_decodeN32issue_476531_AqLvXJh_gif_x1 (301251 ms)
There is a warmup round and doing random frame decode test with discarding
frames in a loop with variable step. x5 means that tests are repeated 5
times.

dolph.gif has transparency.
Index8issue_476531.gif is taken from the issue, don't know if license allows
to have it. 374 opaque frames.
large_checkerboard.gif is one frame.
animated-gif-withoffsets has 5 frames, animated.gif 2 frames; decoding starts as
index8 and on later frames switched to n32.
quicksort_gif is with offset(subrect) and alpha.
disneypixar-disney-pixar-jTXvL4LjakYI8.gif is having alpha and offset (subrect)

N32animated_10color_gif_x5 (67 ms)
Index8animated_10color_gif_x5 (47 ms)
N32radient_gif_x10 (2 ms)
Index8radient_gif_x10 (2 ms)
N32animated_gif_with_offsets_gif_x5 (11 ms)
Index8animated_gif_with_offsets_gif_x5 (9 ms)
N32animated_gif_x10 (3 ms)
Index8animated_gif_x10 (3 ms)
N32quicksort_gif_x5 (11430 ms)
Index8quicksort_gif_x5 (4627 ms)
N32large_gif_checkerboard_gif_x5 (961 ms)
Index8large_gif_checkerboard_gif_x5 (277 ms)
N323dolph_gif_x5 (39 ms)
Index83dolph_gif_x5 (21 ms)
N32issue_476531_AqLvXJh_gif_x1 (306513 ms)
Index8issue_476531_AqLvXJh_gif_x1 (163808 ms)
N32disneypixar_disney_pixar_jTXvL4LjakYI8_gif_x1 (933 ms)
Index8disneypixar_disney_pixar_jTXvL4LjakYI8_gif_x1 (659 ms)

BUG=476531, 138421, 490895
==========

aleksandar.stojiljkovic

aleksandar.stojiljkovic@intel.com changed reviewers: + pkasting@chromium.org - pkasting@google.com

4 years, 11 months ago (2016-01-19 11:45:01 UTC) #30

Peter Kasting

I feel like I'm just an observer on this, but since the CL's seen no ...

4 years, 7 months ago (2016-04-29 02:20:01 UTC) #31

scroggo_chromium

I haven't made it through the whole CL again, but I do want to give ...

4 years, 7 months ago (2016-04-29 19:48:16 UTC) #32

I haven't made it through the whole CL again, but I do want to give you the
feedback I have so far. I've put a lot of comments in the code, but at a high
level, this is a large change, which seems to do several different things. It
will be a lot easier for me to process if you break it up into logical pieces.

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:126: size_t
rowBytes = (newWidth + sizeof(size_t) - 1) & (~(sizeof(size_t) - 1));
On 2016/01/18 13:58:49, aleksandar.stojiljkovic wrote:
> On 2016/01/06 21:50:40, scroggo_chromium wrote:
> > Why'd you choose this rowBytes? If you use zero, SkBitmap::setInfo will pick
> the
> > minimum one for you.
> I think the 0 makes rowBytes equals to newWidth. 
> What I needed was size_t aligned row (nearest multiplier of size_t size), so
> that it is not needed to copy byte by byte but to utilize memory alignment so
> that size_t words can be handled. Verified this to be a bit faster on android
> arm...  

Will you please add a comment explaining this?

https://codereview.chromium.org/1460523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:192:
memset(m_bitmap.getAddr8(0, j), 0xFF, sizeof(uint8_t) * xcount);
On 2016/01/18 13:58:49, aleksandar.stojiljkovic wrote:
> On 2016/01/06 21:50:40, scroggo_chromium wrote:
> > As stated before, I'm concerned about setting this to an arbitrary color in
> the
> > color table. Are we guaranteed that this one is transparent?
> 
> Fixed in new patch...

It's not obvious to me that it is. See comments in the new patch.

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
(right):

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:176: static
bool copyIndex8PixelsTo(const SkBitmap& bitmap, const SkImageInfo& info, void*
pixels, size_t rowBytes)
Various thoughts:

- I think it would be helpful to name these how they are used, src versus dst.
e.g.

(const SkBitmap& src, const SkImageInfo& dstInfo, void* dstPixels, size_t
dstRowBytes)

You could also wrap the dst into an SkPixmap, so it would be

copyIndex8PixelsTo(const SkBitmap&src, SkPixmap& dst)

- You actually never use info. It might be useful for error checking (must be
N32, must have the same width and height as the bitmap). My preference would be
to drop it and use clear comments/names to indicate how the function is to be
called, but I'm okay with using it in asserts, too. (I assume this method is
intended to mimic SkBitmap::copyPixelsTo, which does not take an SkImageInfo
argument, FWIW.)

- This isn't really a copy. I suggest calling this convertIndex8ToN32. (Another
confusing part about the current name - it sounds like pixels are the source,
but they are the destination.)

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:181: if
(bitmap.colorType() != kIndex_8_SkColorType)
I think this should be an assert statement, rather than an if, since it will
only be called with index8.

Then this method can return void.

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:232: bool
result = true;
I don't think this variable is necessary. When you would have set it to false
(only when returning from bitmap.copyPixelsTo; as I suggest above,
copyIndex8PixelsTo should never return false) you can just return false (and
skip checking whether we need to memcpy/set the ctableCount).

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:191: void
ImageFrame::zeroFillFrameRectIndex8(const IntRect& rect, unsigned char
transparentIndex)
transparentIndex is never used.

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:200:
memset(m_bitmap.getAddr8(0, j), 0xFF, sizeof(uint8_t) * xcount);
It's not obvious to me that 0xFF in the color table is transparent.

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.h (right):

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:33: #include
"third_party/skia/include/core/SkColorPriv.h"
Why is this needed? I don't think you're using anything from SkColorPriv (and
you shouldn't be, ideally - "Priv" is short for "private").

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:77: typedef
uint8_t PixelData8;
nit: 8 just tells me that this is 8 bit, but not that it is an index. Maybe call
this PixelDataIndex8?

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:96: void
zeroFillFrameRectIndex8(const IntRect&, unsigned char transparentIndex);
I'm not a big fan of this name. Although the intent is to clear to transparent,
arguably this could be used (and potentially is, as far as I can tell) to clear
to a non-zero color. Maybe it would be better named

fillFrameRectIndex8(const IntRect&, unsigned char index);

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:107: bool
setSize(int newWidth, int newHeight, SkColor background = 0);
I think it would be clearer to use SK_ColorTRANSPARENT (a macro for zero) as the
default value.

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
(left):

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:31:
#include "wtf/PassOwnPtr.h"
Unrelated change?

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
(right):

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:130:
ImageFrame& buffer = m_frameBufferCache[frameContext.frameId()];
As I understand it, we will now have always initialized the frame already at
this point? Maybe add a comment, and/or ASSERT(buffer.status() != FrameEmpty).

More importantly, I think this change (calling initFrameBuffer before calling
haveDecodedRow) is a good change on its own, and doesn't really relate to the
rest of the change. Please remove this piece and submit it for review on its
own. This provides a number of benefits:

- we can weigh the benefit of the change on its own
- if it causes a problem, we can revert it, without affecting other changes
- if the rest of this change (which I think is more risky) needs to be reverted,
we won't revert this part
- it reduces the size of this issue, making it easier to review

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:137:
(*m_copyRowNTimesFunction)(buffer, xBegin, xEnd, yBegin, yEnd);
I found it weird that this method does not take N / repeatCount as a parameter.
Digging further, yEnd was computed from repeatCount, which in turn was created
by subtracting a similar end and beginning. All of this precedes your change
though.

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:192:
// writeTransparentPixels is writing without check, but calculates if there
> writeTransparentPixels is writing without check

What does this mean? It sure looks like if (writeTransparentPixels) we check for
opacity (line 199)

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:194:
// previous rows, no need to calculate here.
Would writeRowN32 benefit from this optimization? (i.e.Don't check for
transparency if m_currentBufferSawAlpha is already true.) (If so I think it
should be its own change.)

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:205:
if (index == transparentPixel) {
I personally prefer braces here, but the style guide suggests no braces for a
one line if block.

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:213:
// No transparency to deal with.
I think this comment is misleading. We're not keeping track of whether there is
transparency, but there still might be transparency. It might be clearer to say
something like:

Either there is no transparency, or the buffer has already seen alpha. No need
to check for transparency.

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:219:
while (source != sourceEnd)
Why is this not a memcpy?

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:222:
while (rowBegin != rowEnd)
Can this be a memcpy?

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:231:
// Copies the pixel data at [(xBegin, yBegin), (xEnd, yEnd)) to the
Don't these actually copy the data from [(xBegin, yBegin), (xEnd, yBegin)]?

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:236:
const int rowBytes = (xEnd - xBegin) * sizeof(ImageFrame::PixelData);
I'm not sure "rowBytes" is the correct name here. That usually refers to the
distance from the start of one row to the start of the next (which may be larger
than the width of the pixels * number of pixels, if there's padding). But that's
not the case here. This is the size of the data to copy.

Maybe this should be something like "bytesToCopy"?

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:406:
buffer->zeroFillFrameRectIndex8(prevRect, (transparentIndex > 255) ? 255 :
transparentIndex);
I think I've brought this up before - this looks like we might "zeroFill" with
255, which may not be transparent.

(That said, the implementation ignores this parameter...)

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:422:
// This frame doesn't rely on any previous data.
If it does not rely on previous data, can we not start with index8?

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:447:
if (!transparencySupported
Why not switch this from:

if (condition) {
  return false;
}
return true;

to

return !condition;

?

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:484:
// If fullscreen and opaque, the frame is independent of previous frame.
This comment seems a little out of place to me. How about:

// If the frame is not full screen, it cannot be independent.

You talk below about opacity.

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:505:
return -1;
Again, I think it's weird to return -1 as an unsigned.

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:515:
return (backgroundIndex < table.size() ? table[backgroundIndex]:
SK_ColorTRANSPARENT);
How does this fall back to transparent? It seems like we have to use a color
that's in some color table, right?

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:568:
if ((index >= frameCount()) || (m_requestedColorMode == ImageFrame::N32) ||
failed())
if (failed()), should this just return false? Otherwise we may report that we
canDecodeTo a type when we cannot decode at all.

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h
(right):

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h:45:
GIFImageDecoder(AlphaOption, GammaAndColorProfileOption, size_t maxDecodedBytes,
ImageFrame::ColorType decodeTo = ImageFrame::Index8);
Since this is always Index8 except for testing, why not use a factory function
with a name ending in ForTesting which sets it to N32? (Normal constructor will
still set to Index8) I think that would be clearer.

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h:85: 
nit: Please remove this extra line.

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h:103:
typedef void (GIFImageDecoder::*WriteRowFunction)(const GIFFrameContext&,
ImageFrame&, const GIFColorTable&, GIFRow::const_iterator,
GIFRow::const_iterator, int, int, bool);
Please add parameter names where they are not obvious. (e.g. GIFFrameContext
needs no parameter name, since it will provide no new information. But looking
at this function declaration I have no idea what int, int, bool are)

Same for function pointer on the next line, and for the methods they point to
(e.g. writeRow... above).

Also, should this use "using" instead of typedef?

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h:115:
ImageFrame::ColorType m_requestedColorMode;
Could be const?

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.cpp
(right):

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.cpp:285: if
(rowBegin + width <= rowIter) {
These changes (i.e. this if block) might be better (though it's not obvious to
me that they are), but they seem to be unrelated to the rest of the change. This
is already a very large change, which makes it hard to review. It would be
helpful to remove pieces like this that are orthogonal to the main change being
made.

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.cpp:346: if
(!client->initFrameBuffer(m_frameId))
Why is this needed now?

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.cpp:473:
m_backgroundIndex = currentComponent[5];
It looks like we never looked at this color before. Were we doing something
wrong?

Ah, I think this relates to your comment in your doc that we should use the
background color for partially decoded independent frames. Is that correct?

Please separate behavior changes (i.e. we're doing something different that the
user can see) from performance changes.

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.h
(right):

https://codereview.chromium.org/1460523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.h:297: ,
m_backgroundIndex(-1)
It's a little weird that you initialize this unsigned field to -1.

Peter Kasting

On 2016/04/29 19:48:16, scroggo_chromium_OOO_paternity wrote: > I haven't made it through the whole CL again, ...

4 years, 4 months ago (2016-08-20 02:40:45 UTC) #33

aleksandar.stojiljkovic

On 2016/08/20 02:40:45, Peter Kasting wrote: > On 2016/04/29 19:48:16, scroggo_chromium_OOO_paternity wrote: > > I ...

4 years, 4 months ago (2016-08-22 17:03:33 UTC) #34

Peter Kasting

On 2016/08/22 17:03:33, aleksandar.stojiljkovic wrote: > If preferred, I could close this and reopen once ...

4 years, 4 months ago (2016-08-22 22:30:41 UTC) #35

aleksandar.stojiljkovic

On 2016/08/22 22:30:41, Peter Kasting wrote: > On 2016/08/22 17:03:33, aleksandar.stojiljkovic wrote: > > If ...

4 years ago (2016-12-19 18:06:17 UTC) #36

scroggo_chromium

3 years, 5 months ago (2017-07-12 20:04:34 UTC) #37

We're removing kIndex_8 (skbug.com/6828), so this is obsolete.

Issue 1460523002: GIF decoding to Index8, unit tests and misusing unit test as benchmark (Closed)

Description

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : Remove unecessary ImageFrame::colorTable (build proper SkBitmap) #

Patch Set 4 : Browser (blink and skia integration). Support added for frames with ofset/transparency in case ther… #

Patch Set 5 : cleanup. tableChanged was wrong - do proper check. #

Patch Set 6 : Skia:onGetColorType related implementation. Fix part of Leon's review comments. #

Patch Set 7 : Handle 256 opaque palette. #

Patch Set 8 : Cleanup for review. Design doc ready. #

Patch Set 9 : #

Patch Set 10 : Comment #25 processed. #

Messages