1382683 - SurfacePipe changes regressed GIF decoding time

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P3)

Description

[Jeff Muizelaar [:jrmuizel]]

See:

https://telemetry.mozilla.org/new-pipeline/evo.html#!aggregates=median&cumulative=0&end_date=2017-07-19&keys=!__none__!__none__&max_channel_version=nightly%252F48&measure=IMAGE_DECODE_SPEED_GIF&min_channel_version=nightly%252F47&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2017-06-12&trim=1&use_submission_date=0

Comment 1

[Sotaro Ikeda [:sotaro]]

:aosmond, can you comment to the bug?

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0001-Bug-1382683-Part-1.-Implement-SurfacePipe-WritePixel.patch, v1

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0002-Bug-1382683-Part-2.-Switch-nsGIFDecoder2-to-write-pi.patch, v1

I still need to write new SurfacePipe tests. But comparing nightly with a pgo Linux build with parts 1 and 2 applied gives ~23% reduction in decode time. This should put us more or less where we were without SurfacePipe.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b83718d5d2098158a4c3698ee993e21c77b6590e

This is consistent with my findings on comparing local optimized builds.

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

To be specific, my ThinkPad W540 goes from ~17ms/frame to ~14ms/frame.

Comment 5

[Andrew Osmond [:aosmond] (he/him)]

Testing on a 2013 Macbook Pro, I go from ~19ms/frame to ~11ms/frame, or ~41% reduction in decode time.

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

These numbers are from testing the first GIF in the page reported in bug 1454149.

Comment 7

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0003-Bug-1382683-Part-3.-Add-gtests-for-SurfaceFilter-Pip.patch, v1

And missing tests are written!

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d547295596f6a23974d025616c8378e83ad0885

Comment 8

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8979054 [details] [diff] [review]
0001-Bug-1382683-Part-1.-Implement-SurfacePipe-WritePixel.patch, v1

Review of attachment 8979054 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/SurfacePipe.h
@@ +175,5 @@
>      return *result;
>    }
>  
> +  /**
> +   * Write pixels to the surface bu calling a lambda which may write as many

bu -> by

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b84413763f8
Part 1. Implement SurfacePipe::WritePixelBlocks for faster writing of pixels. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f1623675fdc
Part 2. Switch nsGIFDecoder2 to write pixels in blocks instead of individually. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/b37bf166ab83
Part 3. Add gtests for SurfaceFilter/Pipe::WritePixelBlocks. r=tnikkel

Comment 10

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/3b84413763f8
https://hg.mozilla.org/mozilla-central/rev/1f1623675fdc
https://hg.mozilla.org/mozilla-central/rev/b37bf166ab83