795378 - Clean up the high-quality scaler

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Joe Drew (not getting mail)]

Currently the high quality scaler imported as part of bug 486918 imports a bunch of unnecessary Chromium code. It should be cleaned up in various ways, including removing the extra copies of Chromium code (which will require fixing the build when not using them), using SSE support (if and when upstream Chromium turns it on too), and possibly switching it out for the pixman-using version that Jeff implemented.

The relevant files are gfx/2d/image_operations.cpp and gfx/2d/convolver.cpp.

Comment 1

[anthony.gxr.chiang]

Hi,

I am new to the OSS scene and would like to get started on this bug, could you please guide me to getting started?

Thanks.

Comment 2

[Joe Drew (not getting mail)]

Hi Anthony!

The first thing I'd recommend you do is merge in the changes that the Chromium authors have made upstream to their image_operations.cc and image_operations.h. These files are located at http://src.chromium.org/viewvc/chrome/trunk/src/skia/ext/image_operations.cc?view=log and http://src.chromium.org/viewvc/chrome/trunk/src/skia/ext/image_operations.h?view=log. Their counterparts exist in Mozilla's code at gfx/2d/image_operations.cpp and .h.

Note that I had to make a bunch of changes to get their files compiling in Mozilla's code, so you can't easily just copy over their files, but instead you need to see what changes they've made and make analogous changes to our code.

If you don't yet have a build of Firefox, here's where to get started: https://developer.mozilla.org/En/Simple_Firefox_build

Comment 3

[Yazen Ghannam]

Attachment: Merge upstream changes to image_operations.cpp

Hello,

I've found that no changes have been made to image_operations.h, convolver.h, or convolver.cc from upstream since Joe committed these files originally. There have been two changes made to image_operations.cc since then. The first was fixing a half-pixel misalignment; this is included in my patch. The other was updating references to stack_container.h because of a directory change. Since the directories in mozilla-central/ipc/chromium haven't changed, I didn't include it in the patch.

Joe, is there any documentation available that will help guide us in figuring out what parts of the code are relevant to Mozilla so that we can clean up these files? Also, please let me know if I've left anything out, or if something needs to be corrected. Thanks.

Comment 4

[Joe Drew (not getting mail)]

Comment on attachment 692687 [details] [diff] [review]
Merge upstream changes to image_operations.cpp

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

I don't have any particular advice about this, unfortunately. In general the "clean up" work should probably eventually lead to us totally excising the code and using the pixman scaler that Jeff wrote; there are somewhat in-progress patches on bug 486918.

Comment 5

[Joe Drew (not getting mail)]

We should probably leave this open for now for future work, and just merge in the changes. Thanks Yazen!

Comment 6

[Ryan VanderMeulen [:RyanVM]]

(Pushing this without a Try link against my better judgment...)
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf51fc0485f0

Yazen, to make life easier for those committing on your behalf, please make sure that your future patches follow the directions below so that all the needed commit information is present in the patch. Thanks!
https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cf51fc0485f0

Comment 8

[Joe Drew (not getting mail)]

Our next step is probably improving Jeff's pixman lanczos filter, and landing it in upstream pixman, so we don't have to have this separate piece of infrastructure. Tim Terriberry had a bunch of review comments on things that need to be done: https://bugzilla.mozilla.org/show_bug.cgi?id=486918#c140

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Hey there Yazen, are you still working on this bug? Joe, is it still in progress?

Comment 10

[Yazen Ghannam]

Hi Liz, I'm planning on working on it, following Joe's advice, once I finish this semester of school (in another two weeks). Somebody else can take it if it needs to be completed soon. Thanks.

Comment 11

[Joe Drew (not getting mail)]

This doesn't need to be solved quickly, so please continue with your plans, Yazen! Thanks!

Comment 12

[Yazen Ghannam]

Hi everyone. It looks like I won't have time to work on this, so the Assignee should be reset. Thanks.

Comment 13

[Erick Dransch [:edransch]]

These two commits to the pixman repository from December 2012 add some functionality implementing box and Lanczos filters.

http://cgit.freedesktop.org/pixman/commit/?id=6fd480b17c8398c217e4c11e826c82dbb8288006
http://cgit.freedesktop.org/pixman/commit/?id=6915f3e24f4169260a8ad6ab7ff3087388dbe5db

They have also included a demo which scales an image up and down using these filters.

http://cgit.freedesktop.org/pixman/tree/demos/scale.c

I would like to determine whether or not we can leverage this code for scaling. 
As someone with more familiarity with Pixman, do you have intuition as to whether we can use it?

If it seems like we can use it, I can write a patch so we can test it on a try build.
If not, we can go ahead with merging Jeff's filter implementations into the Pixman codebase.

Comment 14

[Joe Drew (not getting mail)]

It definitely feels like we can use that, and the bonus is that it already exists and will be optimized for us as time goes on. I say go for it.

Comment 15

[Erick Dransch [:edransch]]

Attachment: WIP patch for scaling

This is a progress update and request for feedback.

I have a few questions to begin with and I'll add more comments on specific lines below.

1) Are there automated tests that verify image scaling? I'd like to run some tests for correctness across a variety of images. 

2) I would also like to test performance of the patch in comparison to the Skia implementation. Do you have any suggestions on how to measure the performance impact?

Comment 16

[Erick Dransch [:edransch]]

Comment on attachment 783915 [details] [diff] [review]
WIP patch for scaling

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

::: gfx/2d/Scale.cpp
@@ +28,5 @@
> +    double fScale_x, fScale_y;
> +    int numSamples = 4;
> +    pixman_fixed_t *params;
> +    // TODO: set this variable based on format argument
> +    pixman_format_code_t pixmanFormat = (format == FORMAT_B8G8R8A8) ? PIXMAN_a8r8g8b8 : PIXMAN_x8r8g8b8;

I only considered two formats here. I think it's probably worth writing a function to convert between these formats.

Do you have a suggestion for which hpp file I could put that function in?

@@ +61,5 @@
> +        PIXMAN_KERNEL_LANCZOS3,
> +        PIXMAN_KERNEL_LANCZOS3,
> +        PIXMAN_KERNEL_LANCZOS3,
> +        numSamples,
> +        numSamples);

I arbitrarily chose both the numSamples value and the kernel type here. 

Perhaps someone with more familiarity with the lanczos filter can comment here to suggest the best choice. 

Otherwise, we can experiment to find a balance of quality and performance.

Comment 17

[Joe Drew (not getting mail)]

(In reply to Erick Dransch [:edransch] from comment #15)
> 1) Are there automated tests that verify image scaling? I'd like to run some
> tests for correctness across a variety of images. 

Not high-quality image scaling, no. I would gladly help to create such a test suite.
 
> 2) I would also like to test performance of the patch in comparison to the
> Skia implementation. Do you have any suggestions on how to measure the
> performance impact?

You could do it in an invasive way, by adding telemetry for time spent downscaling images. I'd gladly r+ that.

Comment 18

[Joe Drew (not getting mail)]

Comment on attachment 783915 [details] [diff] [review]
WIP patch for scaling

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

This broadly looks great. 

A style nit: be sure to line up parameters to a function vertically with the open bracket (e.g.

Foo(a,
    b);

I'm asking Jeff for feedback on the values of the kernel and the number of samples here specifically.

::: gfx/2d/Scale.cpp
@@ +15,5 @@
>  
> +
> +pixman_fixed_t double_to_fixed(double x){
> +    return x * 65536.0 + 0.5;
> +}

Is there a reason you didn't use pixman_double_to_fixed() here?

@@ +24,5 @@
>             SurfaceFormat format)
>  {
> +    int numParams;
> +    double invertScale_x, invertScale_y;
> +    double fScale_x, fScale_y;

I suggest not using fScale as a name :)

@@ +28,5 @@
> +    double fScale_x, fScale_y;
> +    int numSamples = 4;
> +    pixman_fixed_t *params;
> +    // TODO: set this variable based on format argument
> +    pixman_format_code_t pixmanFormat = (format == FORMAT_B8G8R8A8) ? PIXMAN_a8r8g8b8 : PIXMAN_x8r8g8b8;

I think it'd be reasonable to put that in 2d/Tools.h (or 2D/Helpers.h, but that file is empty).

@@ +61,5 @@
> +        PIXMAN_KERNEL_LANCZOS3,
> +        PIXMAN_KERNEL_LANCZOS3,
> +        PIXMAN_KERNEL_LANCZOS3,
> +        numSamples,
> +        numSamples);

If this was in imagelib, I'd say we want numsamples to be a pref so it could be adjusted more easily, but a #define would probably be better than just a stack value in the function.

Comment 19

[Erick Dransch [:edransch]]

Attachment: use pixman scaling

I've addressed Joe's comments regarding this patch. 

If you have suggestions on how to determine the best parameters for the filter (to balance performance and quality), I would appreciate it.

I can add a separate patch to add Telemetry data for time spent downscaling. Would that be useful data to collect?

Comment 20

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 803055 [details] [diff] [review]
use pixman scaling

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

We should get some basic performance information. I would suggest just adding logging using TimeStamp and the dst and src image sizes and seeing how these compare.

::: gfx/2d/Scale.cpp
@@ +8,5 @@
>  
> +#include <stdlib.h>
> +#include <algorithm>
> +
> +#include <iostream>

Is this needed?

@@ +41,5 @@
> +                                                         (uint32_t *) dstData, dstStride ? dstStride : 4);
> +
> +    pixman_f_transform_init_identity(&ftransform);
> +    pixman_f_transform_scale(&ftransform, NULL, scale_x, scale_y);
> +    pixman_f_transform_invert(&ftransform, &ftransform);

Can we just construct the inverted transform instead of inverting afterwards? i.e. using invertScale? and perhaps we can construct invertScale from src/dst to avoid needless inaccuracy

Comment 21

[Erick Dransch [:edransch]]

Attachment: use pixman scaling

I used http://autoindustry.files.wordpress.com/2009/03/ferrari_dino_concept_car_by_nixseraph.jpg and some crude logging to check the performance difference between pixman and Skia.

On my machine, Skia requires 50-90ms to scale the image, depending on the size of the dest image (smaller is faster).
Pixman requires 500-800ms, depending on dest size (smaller is faster).

I played with a few different configurations for the pixman filters, but I didn't find any that provided high quality and comparable speed to Skia.

I will try to gather some more fine-grained performance information to determine why the performance difference is so large.
If any possible explanations jump to mind, I would appreciate any hints.

Comment 22

[Erick Dransch [:edransch]]

Attachment: Logging of scaling time

This is the logging code I added to provide some basic information about the performance difference between Skia and pixman

Comment 23

[Erick Dransch [:edransch]]

I've played around with performance sampling and the Cleopatra tool, which shows that both implementations spend most of their time in the convolution loops, as expected.
There are a few differences in implementation between Pixman and Skia. I've highlighted the ones that I think might have a big impact:

1) The Pixman implementation uses both a sampling and a reconstruction filter. If I'm interpreting correctly, the Skia implementation uses only the reconstruction filter, not the sampling filter.
We can get the same behaviour as the Skia implementation if we set the sampling filter in Pixman to Box.
If we change the sampling filter in Pixman to Box, we do improve performance, but it's still about 3-4x slower than the Skia implementation.

2) The Skia implementation calculates the filter values around each vertical and horizontal pixels ahead of time, and stores them. While rescaling, it just looks up in the array to get the filter values.
The Pixman implementation calculates a generic filter, and calculates the filter matrix for each specific point as it goes.

3) The Skia implementation runs horizontally on 4 rows at a time, and stores the result in a ring buffer, runs the vertical filters for each pixel in the 4 rows, and repeats.
The Pixman implementation runs the full filter (both horizontal and vertical) for each pixel.
Both 2) and 3) might have a major effect on performance. It's probably a combination of these that causes the performance difference.

I'm not sure what the appropriate next step is here. Am I correct in assuming that a performance difference of 3x means we should stay with the Skia implementation?
We could re-evaluate in the future if new optimizations are added to Pixman.

Any other thoughts?

Comment 24

[Jeff Muizelaar [:jrmuizel]]

(In reply to Erick Dransch [:edransch] from comment #23)
> I'm not sure what the appropriate next step is here. Am I correct in
> assuming that a performance difference of 3x means we should stay with the
> Skia implementation?
> We could re-evaluate in the future if new optimizations are added to Pixman.
> 
> Any other thoughts?

Yeah, it sounds like we should stay with current Skia implementation at least until the performance is improved.

Comment 25

[Kartikaya Gupta (email:[e-mail])]

From a quick read-through of this bug, it sounds like there's nothing else that needs to be done here. Since a patch landed (comment 7) should we just close out this bug?

Comment 26

[Erick Dransch [:edransch]]

I have no objections to closing the bug. 

If we decide to re-launch the investigation about replacing Skia with Pixman for high quality scaling, we can file a new bug for that.

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Fabulous!

Joe, you mentioned possibly writing automated tests for this in Comment 17. Are there any tests for this?

Also, I'll set the target milestone for this to Nightly (Firefox 32) for now.